Browse code

Fix potential double-free() in Interactive Service (CVE-2018-9336)

Malformed input data on the service pipe towards the OpenVPN interactive
service (normally used by the OpenVPN GUI to request openvpn instances
from the service) can result in a double free() in the error handling code.

This usually only leads to a process crash (DoS by an unprivileged local
account) but since it could possibly lead to memory corruption if
happening while multiple other threads are active at the same time,
CVE-2018-9336 has been assigned to acknowledge this risk.

Fix by ensuring that sud->directory is set to NULL in GetStartUpData()
for all error cases (thus not being free()ed in FreeStartupData()).

Rewrite control flow to use explicit error label for error exit.

Discovered and reported by Jacob Baines <jbaines@tenable.com>.

CVE: 2018-9336

Signed-off-by: Gert Doering <gert@greenie.muc.de>
Acked-by: Selva Nair <selva.nair@gmail.com>
Message-Id: <20180414072617.25075-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/search?l=mid&q=20180414072617.25075-1-gert@greenie.muc.de

Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit 1394192b210cb3c6624a7419bcf3ff966742e79b)

Gert Doering authored on 2018/04/14 16:26:17
Showing 1 changed files
... ...
@@ -453,7 +453,6 @@ static BOOL
453 453
 GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
454 454
 {
455 455
     size_t len;
456
-    BOOL ret = FALSE;
457 456
     WCHAR *data = NULL;
458 457
     DWORD size, bytes, read;
459 458
 
... ...
@@ -462,7 +461,7 @@ GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
462 462
     {
463 463
         MsgToEventLog(M_SYSERR, TEXT("PeekNamedPipeAsync failed"));
464 464
         ReturnLastError(pipe, L"PeekNamedPipeAsync");
465
-        goto out;
465
+        goto err;
466 466
     }
467 467
 
468 468
     size = bytes / sizeof(*data);
... ...
@@ -470,7 +469,7 @@ GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
470 470
     {
471 471
         MsgToEventLog(M_SYSERR, TEXT("malformed startup data: 1 byte received"));
472 472
         ReturnError(pipe, ERROR_STARTUP_DATA, L"GetStartupData", 1, &exit_event);
473
-        goto out;
473
+        goto err;
474 474
     }
475 475
 
476 476
     data = malloc(bytes);
... ...
@@ -478,7 +477,7 @@ GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
478 478
     {
479 479
         MsgToEventLog(M_SYSERR, TEXT("malloc failed"));
480 480
         ReturnLastError(pipe, L"malloc");
481
-        goto out;
481
+        goto err;
482 482
     }
483 483
 
484 484
     read = ReadPipeAsync(pipe, data, bytes, 1, &exit_event);
... ...
@@ -486,14 +485,14 @@ GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
486 486
     {
487 487
         MsgToEventLog(M_SYSERR, TEXT("ReadPipeAsync failed"));
488 488
         ReturnLastError(pipe, L"ReadPipeAsync");
489
-        goto out;
489
+        goto err;
490 490
     }
491 491
 
492 492
     if (data[size - 1] != 0)
493 493
     {
494 494
         MsgToEventLog(M_ERR, TEXT("Startup data is not NULL terminated"));
495 495
         ReturnError(pipe, ERROR_STARTUP_DATA, L"GetStartupData", 1, &exit_event);
496
-        goto out;
496
+        goto err;
497 497
     }
498 498
 
499 499
     sud->directory = data;
... ...
@@ -503,7 +502,7 @@ GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
503 503
     {
504 504
         MsgToEventLog(M_ERR, TEXT("Startup data ends at working directory"));
505 505
         ReturnError(pipe, ERROR_STARTUP_DATA, L"GetStartupData", 1, &exit_event);
506
-        goto out;
506
+        goto err;
507 507
     }
508 508
 
509 509
     sud->options = sud->directory + len;
... ...
@@ -513,16 +512,16 @@ GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
513 513
     {
514 514
         MsgToEventLog(M_ERR, TEXT("Startup data ends at command line options"));
515 515
         ReturnError(pipe, ERROR_STARTUP_DATA, L"GetStartupData", 1, &exit_event);
516
-        goto out;
516
+        goto err;
517 517
     }
518 518
 
519 519
     sud->std_input = sud->options + len;
520
-    data = NULL; /* don't free data */
521
-    ret = TRUE;
520
+    return TRUE;
522 521
 
523
-out:
522
+err:
523
+    sud->directory = NULL;		/* caller must not free() */
524 524
     free(data);
525
-    return ret;
525
+    return FALSE;
526 526
 }
527 527
 
528 528