Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#600 closed Bug / Defect (fixed)

Missing Sanity checks for strdup() in OpenVPN 2.3.x

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

Description

Hello All,

In reviewing source code in directory 'openvpn-2.3.x/src/openvpn',

I found some instances where calls to strdup() are not checked
for a return value of NULL, indicating failure. The patch files
below should address these issues:

--- 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);

=======================================================================

--- init.c.orig 2015-08-22 09:04:24.518000000 -0700
+++ init.c 2015-08-22 09:06:09.761000000 -0700
@@ -843,6 +843,10 @@

{

if (!options->dev && options->dev_node) {

char *dev_node = strdup(options->dev_node); /* POSIX basename() implementaions may modify its arguments */

+ if (dev_node == NULL) {
+ msg(M_FATAL, "Unable to allocate memory for dev_node in init_options_dev().\n");
+ return;
+ }

options->dev = basename (dev_node);

}

}

=======================================================================

--- misc.c.orig 2015-08-22 09:08:21.807000000 -0700
+++ misc.c 2015-08-22 09:09:36.927000000 -0700
@@ -1651,6 +1651,10 @@

if (path)

{

char *path_cp = strdup(path); /* POSIX basename() implementaions may modify its arguments */

+ if (path_cp == NULL) {
+ msg(M_FATAL, "Unable to allocate memory for path_cp in argv_extract_cmd_name.\n");
+ return NULL;
+ }

const char *bn = basename (path_cp);
if (bn)

{

=======================================================================

--- options.c.orig 2015-08-22 09:12:09.509000000 -0700
+++ options.c 2015-08-22 09:13:41.766000000 -0700
@@ -2581,6 +2581,10 @@

if (type & CHKACC_DIRPATH)

{

char *fullpath = strdup(file); /* POSIX dirname() implementaion may modify its arguments */

+ if (fullpath == NULL) {
+ msg(M_FATAL, "Unable to allocate memory for fullpath in check_file_access.\n");
+ return false;
+ }

char *dirpath = dirname(fullpath);


if (platform_access (dirpath, mode|X_OK) != 0)

=======================================================================

Questions, Comments, Suggestions, Complaints :)

I am attaching the patch files to this bug report...

Bill Parker (wp02855 at gmail dot com)

Attachments (5)

cryptoapi.c.patch (366 bytes) - added by dogbert2 5 years ago.
Patch file for ticket #600
init.c.patch (468 bytes) - added by dogbert2 5 years ago.
Patch file for ticket #600
misc.c.patch (444 bytes) - added by dogbert2 5 years ago.
Patch file for ticket #600
options.c.patch (501 bytes) - added by dogbert2 5 years ago.
Patch file for ticket #600
0001-Add-missing-strdup-return-value-checks.patch (2.9 KB) - added by Steffan Karger 5 years ago.

Download all attachments as: .zip

Change History (9)

Changed 5 years ago by dogbert2

Attachment: cryptoapi.c.patch added

Patch file for ticket #600

Changed 5 years ago by dogbert2

Attachment: init.c.patch added

Patch file for ticket #600

Changed 5 years ago by dogbert2

Attachment: misc.c.patch added

Patch file for ticket #600

Changed 5 years ago by dogbert2

Attachment: options.c.patch added

Patch file for ticket #600

comment:1 Changed 5 years ago by Steffan Karger

Owner: set to Steffan Karger
Status: newaccepted

Hi Bill,

Thanks for the report. I'll take this up and send patches (probably using openvpn's check_malloc_return()) to the list to fix this soon.

Note from a security perspective: all these missing checks are for local options only, which means they are not useful in any way for remote attackers.

comment:2 Changed 5 years ago by Steffan Karger

Ok, I looked into these. Most are valid, but I'd rather use check_malloc_return(). One is an incomplete fix.

cryptoapi.c: ms_error_text() can return NULL when FormatMessage?() fails too. So, add the check before returning, instead of after strdup().

init.c: basename() can deal with a NULL parameter, but will then return '.', which is not what is expected as a dev noce. Indeed, the correct thing to do here is error out. (Note that this is highly unlikely to occur, and would cause other issues during init very soon after this step.)

misc.c: like above. basename() deals with the NULL argument fine, but this will not result in expected behaviour. No real fallout is to be expected, but indeed the correct thing to do is error out.

options.c: libe above. dirname() deal with he NULL argument fine, but this does not result in the expected behaviour.

See the attached file for a suggested patch. I'll send this patch to the list too.

Changed 5 years ago by Steffan Karger

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

Patch was reviewed and NAKed by this guy who thinks that we should use string_alloc() instead of strdup() :-)

Last edited 5 years ago by Gert Döring (previous) (diff)

comment:4 Changed 5 years ago by Steffan Karger

Milestone: release 2.3.9
Resolution: fixed
Status: acceptedclosed

But my second attempt (using string_alloc()) was accepted by this guy ;)

Patches are in git:
master: https://github.com/OpenVPN/openvpn/commit/ddc7692
release/2.3: https://github.com/OpenVPN/openvpn/commit/6d4920e

Last edited 5 years ago by Steffan Karger (previous) (diff)
Note: See TracTickets for help on using tickets.