xorl %eax, %eax

CVE-2009-1577: CScope Stack Buffer Overflow

leave a comment »

CScope is one of the most popular minimalistic source code browsing tools. Releases prior to 15.6 are vulnerable to some straightforward stack based buffer overflows. The bug was present in a string manipulation function at src/find.c. Here is the vulnerable code:

2  Copyright (c) 1998-2000, The Santa Cruz Operation
3  All rights reserved.
   ...
859 /* put the rest of the cross-reference line into the string */
860
861 void
862 putstring(char *s)
863 {
864         char    *cp;
865         unsigned c;
866
867         setmark('\n');
868         cp = blockp;
869         do {
870                 while ((c = (unsigned)(*cp)) != '\n') {
871                         if (c > '\177') {
872                                 c &= 0177;
873                                 *s++ = dichar1[c / 8];
874                                 *s++ = dichar2[c & 7];
875                         }
876                         else {
877                                 *s++ = c;
878                         }
879                         ++cp;
880                 }
881         } while (*(cp + 1) == '' && (cp = readblock()) != NULL);
882         blockp = cp;
883         *s = '';
884 }

It’s a simple string manipulation function that copies (lines 873-874) data to the string passed as argument until either the block pointer cp (lines 868 and 870) is ‘\n‘ or ‘\177‘. If this is not the case, it keeps performs a final copy (line 877) and it is doing this loop as long as the next character of cp is not NULL (line 881). Finally, it updates blockp pointer with the value of cp (line 882) and NULL terminates the buffer (line 883). Clearly, there is not bound checks being performed in this copy function. The only limitations are the newline and ‘\177‘ characters. To fix this, they patched this function to include a length limit like this:

 void
-putstring(char *s)
+putstring(char *s, int length)
 {
     char    *cp;
     unsigned c;
-    
+    int i=0;    
     setmark('\n');
     cp = blockp;
     do {
-        while ((c = (unsigned)(*cp)) != '\n') {
+        while (((c = (unsigned)(*cp)) != '\n') && (i<length)) {
             if (c > '\177') {
                 c &= 0177;
                 *s++ = dichar1[c / 8];
                 *s++ = dichar2[c & 7];
+                i+=2;
             }
             else {
                 *s++ = c;
+                i++;
             }
             ++cp;
         }
-    } while (*(cp + 1) == '' && (cp = readblock()) != NULL);
+    } while (((*(cp + 1) == '' && (cp = readblock()) != NULL)) && 
+      (i < length));
     blockp = cp;

Now, the while loop checks that the iterations being performed (represented by variable i) have not exceed the maximum length. Of course, all the calls were updated, and those were at the following functions:
find_symbol_or_assignment()
finddef()
findallfcns()
findcalling()
findinclude()
match()
findcalledby()
putpostingref()
getoldfile()
copydata()
copyinverted()
putinclude()
In addition to this, a new routine and a macro for stack corruption reporting was added which were:

+static void blow_up(int line)
+{
+    fprintf(stderr,"STACK CORRUPTION AT %d\n",line);
+    abort();
+}
+
+#define CHECK_STACK() do { if(test != (unsigned int)&test) {\
+blow_up(__LINE__);\
+}} while(0)
+

This implements a simple stack canary value check which is performed in findallfcns() by adding the following code:

 char *
 findallfcns(char *dummy)
 {
+    volatile unsigned int test = 0;
     char    file[PATHLEN + 1];    /* source file name */
     char    function[PATLEN + 1];    /* function name */
-
+    char oldblockp;  
     (void) dummy;        /* unused argument */

     /* find the next file name or definition */
+    test = (unsigned int)&test;
     while (scanpast('\t') != NULL) {
+        CHECK_STACK();
+        oldblockp=*blockp;    
         switch (*blockp) {

             case NEWFILE:
             skiprefchar();    /* save file name */
-            putstring(file);
+            putstring(file, PATHLEN);
+            CHECK_STACK();
             if (*file == '') {    /* if end of symbols */

It initializes ‘test‘ with the value of its address and after the critical operation it invokes CHECK_STACK() macro to ensure that there was no change. Of course, finding the address of that variable and overwriting the cookie with the correct value isn’t that hard… (reinventing the wheel is not a wise solution).

Written by xorl

May 8, 2009 at 14:17

Posted in bugs

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s