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

[dogbert2]

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)

[dogbert2]

Patch file for ticket #600

[dogbert2]

Patch file for ticket #600

[dogbert2]

Patch file for ticket #600

[dogbert2]

Patch file for ticket #600

[Steffan Karger]

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.

[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.

[Gert Döring]

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

[Steffan Karger]

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