Buffer and heap overflow in Python’s PyUnicode_FromFormatV()

(original bug report)

There’s a bug in Python 2’s C API function PyUnicode_FromFormatV() (and indirectly in its wrapper PyUnicode_FromFormat()) in Objects/unicodeobject.c which can lead to overflowing both a stack-based and a heap-based buffer.

This happens because the code that ought to compute the size of two buffers, fails to execute. This is the size computation routine:

760     /* step 3: figure out how large a buffer we need */
761     for (f = format; *f; f++) {
762         if (*f == '%') {
763             const char* p = f;
764             width = 0;
765             while (isdigit((unsigned)*f))
766                 width = (width*10) + *f++ - '0';
767             while (*++f && *f != '%' && !isalpha((unsigned)*f))

The expressions on line 762 and line 765 can never both be true; if *f == ‘%’, isdigit((unsigned)%f) can not evaluate as true. Even if it would execute, there’s still code that computes the precision component of the format specifier. Later on, in the loop that actually processes the individual format specifiers, the code is done right:

917             const char* p = f++;
918             int longflag = 0;
919             int size_tflag = 0;
920             zeropad = (*f == '0');
921             /* parse the width.precision part */
922             width = 0;
923             while (isdigit((unsigned)*f))
924                 width = (width*10) + *f++ - '0';
925             precision = 0;
926             if (*f == '.') {
927                 f++;
928                 while (isdigit((unsigned)*f))
929                     precision = (precision*10) + *f++ - '0';
930             }

The actual, current bug comes down to this: both types of padding parameters (width and precision) in the format specifier are ignored when computing the size of the buffers designated to hold sprintf’s output, while in the actual calls to sprintf, these are accounted for.

947             case 'd':
948                 makefmt(fmt, longflag, size_tflag, zeropad, width, precision, 'd');
949                 if (longflag)
950                     sprintf(realbuffer, fmt, va_arg(vargs, long));
951                 else if (size_tflag)
952                     sprintf(realbuffer, fmt, va_arg(vargs, Py_ssize_t));
953                 else
954                     sprintf(realbuffer, fmt, va_arg(vargs, int));
955                 appendstring(realbuffer);
956                 break;
957             case 'u':
958                 makefmt(fmt, longflag, size_tflag, zeropad, width, precision, 'u');
959                 if (longflag)
960                     sprintf(realbuffer, fmt, va_arg(vargs, unsigned long));
961                 else if (size_tflag)
962                     sprintf(realbuffer, fmt, va_arg(vargs, size_t));
963                 else
964                     sprintf(realbuffer, fmt, va_arg(vargs, unsigned int));
965                 appendstring(realbuffer);
966                 break;
967             case 'i':
968                 makefmt(fmt, 0, 0, zeropad, width, precision, 'i');
969                 sprintf(realbuffer, fmt, va_arg(vargs, int));
970                 appendstring(realbuffer);
971                 break;
972             case 'x':
973                 makefmt(fmt, 0, 0, zeropad, width, precision, 'x');
974                 sprintf(realbuffer, fmt, va_arg(vargs, int));
975                 appendstring(realbuffer);
976                 break;

makefmt constructs a format specifier string based on width, precision and other parameters. Subsequently, this format specifier string is supplied to sprintf which will write a padded string to ‘realbuffer’ as requested. Since realbuffer points to the stack-based ‘char buffer[21]’, this will cause a stack-based overwrite:

894     if (abuffersize > 20) {
895         abuffer = PyObject_Malloc(abuffersize);
896         if (!abuffer) {
897             PyErr_NoMemory();
898             goto fail;
899         }
900         realbuffer = abuffer;
901     }
902     else
903         realbuffer = buffer;

The ‘abuffersize > 20’ condition can never be true, since ‘abuffersize’ is defined earlier on as:

808                 if (width < 20)
809                     width = 20;
810                 n += width;
811                 if (abuffersize < width)
812                     abuffersize = width;

which will always cause abuffersize to be 20, since width is always 0, since the code that ought to compute ‘width’ never runs.

After the sprintf, which causes the stack-based buffer overflow, there’s this:

955                 appendstring(realbuffer);
693 #define appendstring(string) 
694     do { 
695         for (copy = string;*copy; copy++) { 
696             *s++ = (unsigned char)*copy; 
697         } 
698     } while (0)

‘s’ is space allocated based on ‘n’:

908     string = PyUnicode_FromUnicode(NULL, n);
909     if (!string)
910         goto fail;
911
912     s = PyUnicode_AS_UNICODE(string);
913     callresult = callresults;

and ‘n’ never accounts for any width or precision parameters either, so ‘s’ is always too small if width and optionally precision parameters are present. Thus, appendstring(realbuffer); results in a heap-based overflow.

Here’s a patch:

diff -r baa5258bef22 Objects/unicodeobject.c
--- a/Objects/unicodeobject.c    Sat Dec 13 16:06:19 2014 -0500
+++ b/Objects/unicodeobject.c    Sun Dec 14 22:14:39 2014 +0100
@@ -760,12 +760,18 @@
/* step 3: figure out how large a buffer we need */
for (f = format; *f; f++) {
if (*f == '%') {
-            const char* p = f;
+            const char* p = f++;
width = 0;
+            precision = 0;
while (isdigit((unsigned)*f))
width = (width*10) + *f++ - '0';
-            while (*++f && *f != '%' && !isalpha((unsigned)*f))
-                ;
+            if (*f == '.') {
+                f++;
+                while (isdigit((unsigned)*f))
+                    precision = (precision*10) + *f++ - '0';
+            }
+            while (*f && *f != '%' && !isalpha((unsigned)*f))
+                f++;

/* skip the 'l' or 'z' in {%ld, %zd, %lu, %zu} since
* they don't affect the amount of space we reserve.
@@ -805,11 +811,9 @@
This isn't enough for octal.
If a width is specified we need more
(which we allocate later). */
-                if (width < 20)
-                    width = 20;
-                n += width;
-                if (abuffersize < width)
-                    abuffersize = width;
+                n += (width + precision) < 20 ? 20 : (width + precision);
+                if (abuffersize < (width + precision) )
+                    abuffersize = width + precision;
break;
case 's':
{

Aside from general memory corruption errors caused by this bug, there’s the additional danger of code execution in instances where a user or entity can control the ‘format’ parameter of the PyUnicode_FromFormat()/PyUnicode_FromFormatV functions, and of sidetracking audits of code that uses these functions because of the unexpected behavior it can entail.

Advertisements