dtsession, DtSvc: fix CVE-2020-2696/VU#308289

Marco Ivaldi <marco.ivaldi@mediaservice.net> has identified 3
vulnerabilities in CDE.

Two of them could affect our CDE (open-source version), while the 3rd
(sdtcm_convert) is Solaris specific.

The two vulnerabilities, both of which affect dtsession could allow a
local privilege escalation to root.  A POC exists for Solaris.  The
POC will not function on our CDE for two main reasons:

- the POC is Solaris specific
- The overflowed variables in question are allocated on the heap,
  whereas in Solaris these variables are located on the stack.

The first vulnerability allows an extra long palette name to be used
to cause a crash via insufficient validation in
SrvPalette.c:CheckMonitor().

The second, which has not yet been assigned a CERT CVE resides in
SmCreateDirs.c:_DtCreateDtDirs() in libDtSvc.  Due to insufficient
bounds checking, a crash or corruption can be achieved by using a very
long DISPLAY name.

This one is considered difficult to exploit, and no POC code is
available at this time.  CDE 2.x code-bases are also listed as not
vulnerable, however some work has been done anyway to do some proper
bounds checking in this function.

The following text portions are copied from the relevant advisories,
which have not been released as of this writing.

NOTE: Oracle CDE does NOT use CDE 2.3.0a or earlier as mentioned
below.  They are completely different code-bases):

Regarding CVE-2020-2692:

  A buffer overflow in the CheckMonitor() function in the Common
  Desktop Environment 2.3.0a and earlier, as distributed with Oracle
  Solaris 10 1/13 (Update 11) and earlier, allows local users to gain
  root privileges via a long palette name passed to dtsession in a
  malicious .Xdefaults file.

  Note that Oracle Solaris CDE is based on the original CDE 1.x train,
  which is different from the CDE 2.x codebase that was later open
  sourced. Most notably, the vulnerable buffer in the Oracle Solaris
  CDE is stack-based, while in the open source version it is
  heap-based.

Regarding the DtSvc bug, which does not currently have a CERT CVE:

  A difficult to exploit stack-based buffer overflow in the
  _DtCreateDtDirs() function in the Common Desktop Environment version
  distributed with Oracle Solaris 10 1/13 (Update 11) and earlier may
  allow local users to corrupt memory and potentially execute
  arbitrary code in order to escalate privileges via a long X11
  display name. The vulnerable function is located in the libDtSvc
  library and can be reached by executing the setuid program
  dtsession.

  The open source version of CDE (based on the CDE 2.x codebase) is
  not affected.

diff --git a/cde/lib/DtSvc/DtUtil1/SmUtil.c b/cde/lib/DtSvc/DtUtil1/SmUtil.c
index 3fa75b7c6819601f14181d09c2c942012217d2c1..af4a83b22cc5004959eecb4d8afbe2a14aab40d8 100644 (file)
--- a/cde/lib/DtSvc/DtUtil1/SmUtil.c
+++ b/cde/lib/DtSvc/DtUtil1/SmUtil.c
@@ -176,10 +176,23 @@ getSessionPath(
 
    /*
     * NOTE: it is assumed that _DtCreateDtDirs() returns a buffer of 
-    *       size MAXPATHLEN+1. This allows us to avoid a extra alloc
+    *       size MAXPATHLEN. This allows us to avoid a extra alloc
     *       and copy -- at the expense of code maintainability.
+    *
+    * JET - 2020.  This is stupid.  At least account for the strings
+    * you are adding further on down...  This "solution" isn't great
+    * either.  Real fix would be to have all callers pass in bufptr
+    * and len all the way down the chain instead of tmpPath.
     */
-    if ((strlen(tmpPath) + 1 + strlen(property)) > MAXPATHLEN) goto abort;
+    if ((strlen(tmpPath)
+         + 1 /* "/" */
+         + strlen(property)
+         + 1 /* "/" */
+         + ((*saveFile == NULL) ? strlen("dtXXXXXX") + 1 : strlen(*saveFile))
+        ) >= MAXPATHLEN)
+    {
+        goto abort;
+    }
 
    /* 
     * parse the property string and create directory if needed 

diff --git a/cde/lib/DtSvc/DtUtil2/SmCreateDirs.c b/cde/lib/DtSvc/DtUtil2/SmCreateDirs.c
index 81308c42a0ffc6a06d42ae0d269d8012a7570951..4e8c0b11446e96640febf7bbe6b05c9950a4a45d 100644 (file)
--- a/cde/lib/DtSvc/DtUtil2/SmCreateDirs.c
+++ b/cde/lib/DtSvc/DtUtil2/SmCreateDirs.c
@@ -83,33 +83,32 @@ char *
 _DtCreateDtDirs(
         Display *display )
 {
-    char               *tmpPath;
+    char               *tmpPath = NULL;
     Boolean            needSessionsDir = False;
     Boolean            useOldSession = False;
     struct stat        buf;
     int                status;
-    char               *home;
-    char               *sessionDir;
-    char               *displayName;
+    char               *home = NULL;
+    char               *sessionDir = NULL;
+    char               *displayName = NULL;
 
     /*
      * Sanity check - make sure there's an existing display
      */
     if(!display)
        return(NULL);
-    
-    if ((home =getenv("HOME")) == NULL)
+
+    if ((home = getenv("HOME")) == NULL)
         home = "";
-    
-    tmpPath = XtCalloc(1, MAXPATHLEN + 1);
+
+    tmpPath = XtCalloc(1, MAXPATHLEN);
     if(tmpPath == NULL)
        return(NULL);
 
     /*
      * If the $HOME/.dt directory does not exist, create it
      */
-    strncpy(tmpPath, home, MAXPATHLEN);
-    strncat(tmpPath, "/" DtPERSONAL_CONFIG_DIRECTORY, MAXPATHLEN);
+    snprintf(tmpPath, MAXPATHLEN, "%s/%s", home, DtPERSONAL_CONFIG_DIRECTORY);
 
     status = stat(tmpPath, &buf);
     if (status == -1) {
@@ -122,11 +121,10 @@ _DtCreateDtDirs(
     }
 
     /*
-     * Create the personal DB directory if it does not exist.  
+     * Create the personal DB directory if it does not exist.
      */
-    strncpy(tmpPath, home, MAXPATHLEN);
-    strncat(tmpPath, "/" DtPERSONAL_DB_DIRECTORY, MAXPATHLEN);
-    
+    snprintf(tmpPath, MAXPATHLEN, "%s/%s", home, DtPERSONAL_DB_DIRECTORY);
+
     if ((status = stat (tmpPath, &buf)) == -1) {
         if ((status = mkdir (tmpPath, 0000)) != -1)
            (void) chmod (tmpPath, 0755);
@@ -135,8 +133,7 @@ _DtCreateDtDirs(
     /*
      * Create the personal tmp dir if it does not exist.
      */
-    strncpy(tmpPath, home, MAXPATHLEN);
-    strncat(tmpPath, "/" DtPERSONAL_TMP_DIRECTORY, MAXPATHLEN);
+    snprintf(tmpPath, MAXPATHLEN, "%s/%s", home, DtPERSONAL_TMP_DIRECTORY);
 
     if ((status = stat (tmpPath, &buf)) == -1) {
        if ((status = mkdir (tmpPath, 0000)) != -1)
@@ -173,12 +170,13 @@ _DtCreateDtDirs(
             */
            if ((displayName = GetDisplayName (display)) != NULL) {
 
-               strncpy (tmpPath, home, MAXPATHLEN);
-               strncat (tmpPath, "/" DtPERSONAL_CONFIG_DIRECTORY, MAXPATHLEN);
-                strncat (tmpPath, "/", MAXPATHLEN);
-                strncat (tmpPath, displayName, MAXPATHLEN);
+                snprintf(tmpPath, MAXPATHLEN, "%s/%s/%s",
+                         home,
+                         DtPERSONAL_CONFIG_DIRECTORY,
+                         displayName);
 
                free(displayName);  /* CDExc22771 */
+                displayName = NULL;
 
                 if ((status = stat (tmpPath, &buf)) == -1) {
                    if ((status = mkdir (tmpPath, 0000)) != -1)
@@ -215,12 +213,13 @@ _DtCreateDtDirs(
         */
        if ((displayName = GetDisplayName (display)) != NULL) {
 
-           strncpy (tmpPath, home, MAXPATHLEN);
-           strncat (tmpPath, "/" DtPERSONAL_CONFIG_DIRECTORY, MAXPATHLEN);
-           strncat (tmpPath, "/", MAXPATHLEN);
-           strncat (tmpPath, displayName, MAXPATHLEN);
+            snprintf(tmpPath, MAXPATHLEN, "%s/%s/%s",
+                     home,
+                     DtPERSONAL_CONFIG_DIRECTORY,
+                     displayName);
 
            free(displayName);  /* CDExc22771 */
+            displayName = NULL;
 
            if ((status = stat(tmpPath, &buf)) != 0)
                /*
@@ -238,9 +237,10 @@ _DtCreateDtDirs(
         *  If we don't have an old style directory - we check for a sessions
         *  directory, and create it if it doesn't exist
         */
-       strncpy (tmpPath, home, MAXPATHLEN);
-       strncat (tmpPath, "/" DtPERSONAL_CONFIG_DIRECTORY, MAXPATHLEN);
-       strncat (tmpPath, "/" DtSM_SESSION_DIRECTORY, MAXPATHLEN);
+        snprintf(tmpPath, MAXPATHLEN, "%s/%s/%s",
+                 home,
+                 DtPERSONAL_CONFIG_DIRECTORY,
+                 DtSM_SESSION_DIRECTORY);
 
        if ((status = stat(tmpPath, &buf)) == -1) {
            if ((status = mkdir(tmpPath, 0000)) == -1) {

diff --git a/cde/programs/dtsession/SrvPalette.c b/cde/programs/dtsession/SrvPalette.c
index b967c7ee04b5ede5c66de8f2033a38019af318fe..55b722c968974afda91b5d35aa13d38245897709 100644 (file)
--- a/cde/programs/dtsession/SrvPalette.c
+++ b/cde/programs/dtsession/SrvPalette.c
@@ -501,10 +501,10 @@ CheckMonitor(
     int n, screen_number, result;
     Arg args[4];
     char screenStr[5], cust_msg[24];
-    char *tmpStr;
-    char            tmpPalette[SRVBUFSIZE];
-    char            *token1;
-    char           *xrdb_string;
+    char *tmpStr = NULL;
+    char tmpPalette[SRVBUFSIZE];
+    char *token1 = NULL;
+    char *xrdb_string = NULL;
 
     Widget mainShell;
     XtAppContext app_context;
@@ -541,7 +541,7 @@ CheckMonitor(
    /* cycle through each screen */
     for(screen_number=0;screen_number != colorSrv.NumOfScreens;screen_number++)
     {
-       sprintf(screenStr,"%d",screen_number);
+       snprintf(screenStr, sizeof(screenStr), "%d", screen_number);
        n = 0;
        XtSetArg(args[n], XmNbackground, 
            BlackPixelOfScreen(DefaultScreenOfDisplay(dpy))); n++;
@@ -559,7 +559,8 @@ CheckMonitor(
        
        XtRealizeWidget(shell[screen_number]);
        
-       sprintf(cust_msg,"%s%d", XmSCUSTOMIZE_DATA, screen_number);
+       snprintf(cust_msg, sizeof(cust_msg), "%s%d",
+                XmSCUSTOMIZE_DATA, screen_number);
        colorSrv.XA_CUSTOMIZE[screen_number] = 
           XInternAtom(dpy, cust_msg, FALSE);
        
@@ -574,11 +575,16 @@ CheckMonitor(
           /*
            * Don't forget to add length for the extra characters.
            */
-          tmpStr = (char *)SRV_MALLOC(strlen(MSG1) + 25 + 5 + 1 + 1);
-          sprintf(tmpStr,"%s colorSrv.XA_CUSTOMIZE[%d].\n", 
-                  MSG1, screen_number);
-          _DtSimpleError(XmSCOLOR_SRV_NAME, DtWarning, NULL, tmpStr, NULL);
-          SRV_FREE(tmpStr);
+           int len = strlen(MSG1) + 25 + 5 + 1 + 1;
+          tmpStr = (char *)SRV_MALLOC(len);
+           if (tmpStr)
+           {
+               snprintf(tmpStr, len, "%s colorSrv.XA_CUSTOMIZE[%d].\n",
+                        MSG1, screen_number);
+               _DtSimpleError(XmSCOLOR_SRV_NAME, DtWarning, NULL, tmpStr, NULL);
+               SRV_FREE(tmpStr);
+               tmpStr = NULL;
+           }
           return(-1);
        }
 
@@ -608,15 +614,19 @@ CheckMonitor(
                (struct _palette *) SRV_MALLOC( sizeof(struct _palette) + 1 );
 
            /*  allocate enough space for the name */
-           strcpy(tmpPalette, pColorSrvRsrc.MonochromePalette); 
-           for (token1=tmpPalette; *token1; token1++);
-           while (token1!=tmpPalette && *token1!='.') token1--;
-          if (!strcmp(token1,PALETTE_SUFFIX)) *token1 = '\0';
+           snprintf(tmpPalette, SRVBUFSIZE, "%s",
+                    pColorSrvRsrc.MonochromePalette);
+           for (token1=tmpPalette; *token1; token1++)
+               ;
+           while (token1 != tmpPalette && *token1 != '.')
+               token1--;
+          if (!strcmp(token1, PALETTE_SUFFIX))
+               *token1 = '\0';
            colorSrv.pCurrentPalette[screen_number]->name = 
                (char *)SRV_MALLOC(strlen(tmpPalette) + 1);
            strcpy(colorSrv.pCurrentPalette[screen_number]->name,
                   (char *) tmpPalette);
-           colorSrv.pCurrentPalette[screen_number]->converted=NULL;
+           colorSrv.pCurrentPalette[screen_number]->converted = NULL;
        }
 
        if (colorSrv.pCurrentPalette[screen_number] == (struct _palette *) NULL)
@@ -627,19 +637,21 @@ CheckMonitor(
       /* write out the color or monochrome palette resource for the screen */
 
        xrdb_string = XtMalloc(BUFSIZ);
+       if (!xrdb_string)
+           return -1;
 
        if (colorSrv.TypeOfMonitor[0] == XmCO_HIGH_COLOR || 
            colorSrv.TypeOfMonitor[0] == XmCO_MEDIUM_COLOR ||
            colorSrv.TypeOfMonitor[0] == XmCO_LOW_COLOR)
        {
-           sprintf(xrdb_string, "*%d*ColorPalette: %s%s\n",
+           snprintf(xrdb_string, BUFSIZ, "*%d*ColorPalette: %s%s\n",
                    screen_number,
                   colorSrv.pCurrentPalette[screen_number]->name,
                   PALETTE_SUFFIX);
        }
        else /* XmCO_BLACK_WHITE */
        {
-           sprintf(xrdb_string, "*%d*MonochromePalette: %s%s\n",
+           snprintf(xrdb_string, BUFSIZ, "*%d*MonochromePalette: %s%s\n",
                    screen_number,
                   colorSrv.pCurrentPalette[screen_number]->name,
                   PALETTE_SUFFIX);
@@ -647,7 +659,7 @@ CheckMonitor(
        _DtAddToResource(dpy, xrdb_string);
 
        XtFree(xrdb_string);
-    
+
    } /* for each screen */
    return(0);
 }