Opened 9 years ago

Closed 4 years ago

#587 closed Bug / Defect (fixed)

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 9 years ago.
Patch file for ticket #587
route.c.2.patch (1.6 KB) - added by dogbert2 9 years ago.
Patch file for ticket #587
ssl_polarssl.c.patch (417 bytes) - added by dogbert2 9 years ago.
Patch file for ticket #587
simple.c.patch (503 bytes) - added by dogbert2 9 years ago.
Patch file for ticket #587
log.c.patch (395 bytes) - added by dogbert2 9 years ago.
Patch file for ticket #587
log_v3.c.patch (449 bytes) - added by dogbert2 9 years ago.
Patch file for ticket #587
crypto.c.patch (330 bytes) - added by dogbert2 9 years ago.
Patch file for ticket #587

Download all attachments as: .zip

Change History (19)

Changed 9 years ago by dogbert2

Attachment: route.c.patch added

Patch file for ticket #587

Changed 9 years ago by dogbert2

Attachment: route.c.2.patch added

Patch file for ticket #587

Changed 9 years ago by dogbert2

Attachment: ssl_polarssl.c.patch added

Patch file for ticket #587

Changed 9 years ago by dogbert2

Attachment: simple.c.patch added

Patch file for ticket #587

Changed 9 years ago by dogbert2

Attachment: log.c.patch added

Patch file for ticket #587

Changed 9 years ago by dogbert2

Attachment: log_v3.c.patch added

Patch file for ticket #587

Changed 9 years ago by dogbert2

Attachment: crypto.c.patch added

Patch file for ticket #587

comment:1 Changed 9 years ago by dogbert2

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

comment:2 Changed 9 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 9 years ago by Gert Döring

Milestone: release 2.3.9
Priority: majorminor

comment:4 Changed 9 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 8 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 8 years ago by Samuli Seppänen

Milestone: release 2.3.9release 2.3.12

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

Replying to syzzer:

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.

The patch in question is this.

Version 0, edited 8 years ago by Samuli Seppänen (next)

comment:8 Changed 7 years ago by Gert Döring

Milestone: release 2.3.12release 2.3.14

comment:9 Changed 4 years ago by Kasey00

Spam

comment:10 Changed 4 years ago by Gert Döring

The route socket call was fixed "en passant" in the *BSD route code unification project, it seems...

    /* transact with routing socket */
    sockfd = socket(PF_ROUTE, SOCK_RAW, 0);
    if (sockfd < 0)
    {
        msg(M_WARN, "GDG: socket #1 failed");
        goto done;
    }

so, done (for 2.4.x and later, 2.3 will only receive critical bugfixes, which this is not).

Plugins next.

comment:12 Changed 4 years ago by Gert Döring

Resolution: fixed
Status: acceptedclosed

commit a61c08a2c80d95dcc2bc30ddcb9a54a462e565ed (master)
commit 5382bdbfbfb9ac26c7c75bc967af86db352b54b3 (release/2.5)
commit 2b8dda69115e1e048ff685bc366705156781548c (release/2.4)
Author: Gert Doering
Date: Wed Sep 9 12:48:37 2020 +0200

Handle NULL returns from calloc() in sample plugins.

finally!

Note: See TracTickets for help on using tickets.