Opened 3 years ago

Last modified 22 months ago

#587 accepted Bug / Defect

Add sanity checks, replace deprecated calls in OpenVPN 2.3.x

Reported by: dogbert2 Owned by: Gert Döring
Priority: minor Milestone: release 2.3.14
Component: Generic / unclassified Version: OpenVPN 2.3.5 (Community Ed)
Severity: Not set (select this one, unless your'e a OpenVPN developer) Keywords:
Cc: Steffan Karger

Description

Hello All,

In reviewing source files in OpenVPN 2.3.x, I found some

missing checks for return values for library function calls
in C. In directory 'openvpn-2.3.x/src/openvpn', file 'route.c'
there is a call to socket() which is not checked for a return
value of < 0, indicating error. Additionally, there are a number
of instances where the deprecated function bzero() is called
in route.c, which should be replaced by memset() (which is used
everywhere else in OpenVPN-2.3.x).

The patch file below should address these issues:

--- route.c.orig 2015-08-01 16:54:22.478831925 -0400
+++ route.c 2015-08-02 11:41:50.451396086 -0400
@@ -2644,9 +2644,9 @@

seq = 0;
rtm_addrs = RTA_DST | RTA_NETMASK;


  • bzero(&so_dst, sizeof(so_dst));
  • bzero(&so_mask, sizeof(so_mask));
  • bzero(&rtm, sizeof(struct rt_msghdr));

+ memset(&so_dst, 0, sizeof(so_dst));
+ memset(&so_mask, 0, sizeof(so_mask));
+ memset(&rtm, 0, sizeof(struct rt_msghdr));

rtm.rtm_type = RTM_GET;
rtm.rtm_flags = RTF_UP | RTF_GATEWAY;

@@ -2665,6 +2665,13 @@

rtm.rtm_msglen = l = cp - (char *)&m_rtmsg;


s = socket(PF_ROUTE, SOCK_RAW, 0);

+ if (s < 0)
+ {
+ msg(M_WARN|M_ERRNO, "Unable to open default gateway from route socket:");
+ gc_free (&gc);
+ close(s);
+ return;
+ }

if (write(s, (char *)&m_rtmsg, l) < 0)

{

@@ -2758,10 +2765,10 @@

seq = 0;
rtm_addrs = RTA_DST | RTA_NETMASK | RTA_IFP;


  • bzero(&m_rtmsg, sizeof(m_rtmsg));
  • bzero(&so_dst, sizeof(so_dst));
  • bzero(&so_mask, sizeof(so_mask));
  • bzero(&rtm, sizeof(struct rt_msghdr));

+ memset(&m_rtmsg, 0, sizeof(m_rtmsg));
+ memset(&so_dst, 0, sizeof(so_dst));
+ memset(&so_mask, 0, sizeof(so_mask));
+ memset(&rtm, 0, sizeof(struct rt_msghdr));

rtm.rtm_type = RTM_GET;
rtm.rtm_flags = RTF_UP | RTF_GATEWAY;

@@ -2965,9 +2972,9 @@

seq = 0;
rtm_addrs = RTA_DST | RTA_NETMASK;


  • bzero(&so_dst, sizeof(so_dst));
  • bzero(&so_mask, sizeof(so_mask));
  • bzero(&rtm, sizeof(struct rt_msghdr));

+ memset(&so_dat, 0, sizeof(so_dat));
+ memset(&so_mask, 0, sizeof(so_mask));
+ memset(&rtm, 0, sizeof(struct rt_msghdr));

rtm.rtm_type = RTM_GET;
rtm.rtm_flags = RTF_UP | RTF_GATEWAY;

In directory 'openvpn-2.3.x/src/openvpn', file 'ssl_polarssl.c',
there is a call to strdup() which is not checked for a return value
of NULL, indicating failure. The patch file below should address
this issue:

--- ssl_polarssl.c.orig 2015-08-01 16:44:03.777831925 -0400
+++ ssl_polarssl.c 2015-08-01 16:47:08.764831925 -0400
@@ -192,7 +192,10 @@

/* Parse allowed ciphers, getting IDs */
i = 0;
tmp_ciphers_orig = tmp_ciphers = strdup(ciphers);

-
+ if (NULL == tmp_ciphers) {
+ msg(M_FATAL, "Unable to allocate memory for ciphers...");
+ return;
+ }

token = strtok (tmp_ciphers, ":");
while(token)

{


In directory 'openvpn-2.3.x/sample/sample-plugins/defer', file
'simple.c', there is a call to calloc() which is not checked for
a return value of NULL, indicating failure. The patch file below
should address this issue:

--- simple.c.orig 2015-08-01 17:13:50.357831925 -0400
+++ simple.c 2015-08-01 17:15:27.551831925 -0400
@@ -132,6 +132,10 @@

  • Allocate our context */

context = (struct plugin_context *) calloc (1, sizeof (struct plugin_context));

+ if (NULL == context) {
+ printf ("MEMORY_ALLOCATION for context failed...\n");
+ return NULL;
+ }

context->test_deferred_auth = atoi_null0 (get_env ("test_deferred_auth", envp));
printf ("TEST_DEFERRED_AUTH %d\n", context->test_deferred_auth);


In directory 'openvpn-2.3.x/sample/sample-plugins/log', file 'log.c',
there is a call to calloc() which is not checked for a return value
of NULL, indicating failure. The patch file below should address
this issue:

--- log.c.orig 2015-08-01 17:21:06.813831925 -0400
+++ log.c 2015-08-01 17:22:13.086831925 -0400
@@ -77,6 +77,10 @@

  • Allocate our context */

context = (struct plugin_context *) calloc (1, sizeof (struct plugin_context));

+ if (NULL == context) {
+ printf("Unable to allocate memory for context\n");
+ return NULL;
+ }

/*

  • Set the username/password we will require.


In file 'log_v3.c' in the above directory, there is a call to calloc()
which is not checked for a return value of NULL, indicating failure.
The patch file below should address this issue:

--- log_v3.c.orig 2015-08-01 17:17:36.741831925 -0400
+++ log_v3.c 2015-08-01 17:19:13.187831925 -0400
@@ -106,6 +106,10 @@

/* Allocate our context */
context = (struct plugin_context *) calloc (1, sizeof (struct plugin_context));

+ if (NULL == context) {
+ printf("Unable to allocate memory for context\n");
+ return OPENVPN_PLUGIN_FUNC_ERROR;
+ }

/* Set the username/password we will require. */
context->username = "foo";

In directory 'openvpn-2.3.x/src/openvpn', file 'crypto.c', a call to
memset() at approximately line 1022 is made with pointer variable
'output', but if memset() is called with a memory area which is set
to NULL, a segmentation violation/fault will occur. The patch file
below adds a check to test the value of 'output' for NULL:

--- crypto.c.orig 2015-08-01 17:27:21.039831925 -0400
+++ crypto.c 2015-08-01 17:38:39.031831925 -0400
@@ -1022,6 +1022,9 @@

md_ctx_t md;


ASSERT (len >= md_kt_size(digest));

+ if (NULL == output)
+ msg(M_FATAL, "memory area for variable 'output' is NULL");
+

memset (output, 0, len);


md_ctx_init(&md, digest);


Comments, Questions, Suggestions, Complaints :)

I am attaching the patch file(s) to this bug report...

Bill Parker (wp02855 at gmail dot com)

Attachments (7)

route.c.patch (1.6 KB) - added by dogbert2 3 years ago.
Patch file for ticket #587
route.c.2.patch (1.6 KB) - added by dogbert2 3 years ago.
Patch file for ticket #587
ssl_polarssl.c.patch (417 bytes) - added by dogbert2 3 years ago.
Patch file for ticket #587
simple.c.patch (503 bytes) - added by dogbert2 3 years ago.
Patch file for ticket #587
log.c.patch (395 bytes) - added by dogbert2 3 years ago.
Patch file for ticket #587
log_v3.c.patch (449 bytes) - added by dogbert2 3 years ago.
Patch file for ticket #587
crypto.c.patch (330 bytes) - added by dogbert2 3 years ago.
Patch file for ticket #587

Download all attachments as: .zip

Change History (15)

Changed 3 years ago by dogbert2

Attachment: route.c.patch added

Patch file for ticket #587

Changed 3 years ago by dogbert2

Attachment: route.c.2.patch added

Patch file for ticket #587

Changed 3 years ago by dogbert2

Attachment: ssl_polarssl.c.patch added

Patch file for ticket #587

Changed 3 years ago by dogbert2

Attachment: simple.c.patch added

Patch file for ticket #587

Changed 3 years ago by dogbert2

Attachment: log.c.patch added

Patch file for ticket #587

Changed 3 years ago by dogbert2

Attachment: log_v3.c.patch added

Patch file for ticket #587

Changed 3 years ago by dogbert2

Attachment: crypto.c.patch added

Patch file for ticket #587

comment:1 Changed 3 years ago by dogbert2

Ignore file 'route.c.2.patch', I pushed the button too quickly :)

comment:2 Changed 3 years ago by Gert Döring

Cc: Steffan Karger added

well, supposedly the PF_ROUTE socket() call cannot fail, but thanks anyway :-)

We're not going to use memset() - there's the CLEAR() macro, which is really what is used in the code in most places, and conforms to the official coding standard.

I agree that we should fix the ssl_polarssl.c strdup() return check...

The crypto.c one is less interesting, as it's the callers responsibility to ensure that the pointer is not NULL (and the single caller calls it on a structure element which is always non-NULL).

comment:3 Changed 3 years ago by Gert Döring

Milestone: release 2.3.9
Priority: majorminor

comment:4 Changed 3 years ago by Steffan Karger

Wrt to the one in crypto.c: that code was removed from the master branch entirely, so combined with that the single caller does it right, I do not see a need to fix that.

I just sent a patch containing a fix for the missing ssl_polarssl.c check to the mailing list.

comment:5 Changed 3 years ago by Gert Döring

Owner: set to Gert Döring
Status: newaccepted

OK, looked at this again.

The bzero() calls all seem to be in very BSD-specific parts of the code, and that is exactly the right call to use there ("as per the docs").

strdup() is gone, crypto stuff is fixed.

That leaves the socket() call (btw: do not call close(s) if you just determined that the socket could not be opened) and the plugin calloc() calls... I'll go about a patch for them (as in: single patch for all the plugins, sent to the list for review, merge)

comment:6 Changed 2 years ago by Samuli Seppänen

Milestone: release 2.3.9release 2.3.12

comment:7 in reply to:  4 Changed 2 years ago by Samuli Seppänen

Replying to syzzer:

I just sent a patch containing a fix for the missing ssl_polarssl.c check to the mailing list.

The patch in question is this, which has been applied.

Last edited 2 years ago by Samuli Seppänen (previous) (diff)

comment:8 Changed 22 months ago by Gert Döring

Milestone: release 2.3.12release 2.3.14
Note: See TracTickets for help on using tickets.