Is this program susceptible to a buffer overflow exploit?

Question

I use it in a number of embedded devices. It loads the query parameters from an HTTP GET/POST request and prints them to stdout in FORM_key=value format which then get set as an environment variable.

Source:

char *
FP_strdup (char *string)
{
 char *result;
 if (string == NULL)
 return NULL;
 if ((result = (char *) malloc (strlen (string) + 1)) == NULL) {
 fprintf (stderr, "proccgi -- out of memory dupping %d bytes\n",
 (int) strlen (string));
 return NULL;
 }
 strcpy (result, string);
 return result;
}
/*
 * Read CGI input
 */
char *
LoadInput (void)
{
 char *result, *method, *p;
 int length, ts;
 if ((method = getenv ("REQUEST_METHOD")) == NULL) {
 return NULL;
 }
 if (strcmp (method, "GET") == 0) {
 if ((p = getenv ("QUERY_STRING")) == NULL)
 return NULL;
 else
 result = FP_strdup (p);
 }
 else if (strcmp (method, "POST") == 0) {
 if ((length = atoi (getenv ("CONTENT_LENGTH"))) == 0)
 return NULL;
 if ((result = malloc (length + 1)) == NULL) {
 fprintf (stderr, "proccgi -- out of memory allocating %d bytes\n",
 length);
 return NULL;
 }
 if ((ts = fread (result, sizeof (char), length, stdin)) < length) {
 fprintf (stderr, "proccgi -- error reading post data, %d bytes read, %d expedted\n",
 ts, length);
 }
 result[length] = '0円';
 }
 else {
 return NULL;
 }
 return result;
}
/*
 * Parse + and %XX in CGI data
 */
char *
ParseString (char *instring)
{
 char *ptr1=instring, *ptr2=instring;
 if (instring == NULL)
 return instring;
 while (isspace (*ptr1))
 ptr1++;
 while (*ptr1) {
 if (*ptr1 == '+') {
 ptr1++; *ptr2++=' ';
 }
 else if (*ptr1 == '%' && isxdigit (*(ptr1+1)) && isxdigit (*(ptr1+2))) {
 ptr1++;
 *ptr2 = ((*ptr1>='0'&&*ptr1<='9')?(*ptr1-'0'):((char)toupper(*ptr1)-'A'+10)) << 4;
 ptr1++;
 *ptr2++ |= ((*ptr1>='0'&&*ptr1<='9')?(*ptr1-'0'):((char)toupper(*ptr1)-'A'+10));
 ptr1++;
 }
 else
 *ptr2++ = *ptr1++;
 }
 while (ptr2>instring && isspace(*(ptr2-1)))
 ptr2--;
 *ptr2 = '0円';
 return instring;
}
/*
 * break into attribute/value pair. Mustn't use strtok, which is
 * already used one level below. We assume that the attribute doesn't
 * have any special characters.
 */
void
HandleString (char *input)
{
 char *data, *ptr, *p2;
 if (input == NULL) {
 return;
 }
 data = FP_strdup (input);
 ptr = ParseString (data);
 /*
 * Security:
 *
 * only accept all-alphanumeric attributes, and don't accept empty
 * values
 */
 if (!isalpha(*ptr) && *ptr != '_') {free (data); return;}
 ptr++;
 while (isalnum(*ptr) || *ptr == '_') ptr++;
 if (*ptr != '=') {free (data); return;}
 *ptr = '0円';
 p2 = ptr+1;
 fprintf (stdout, "FORM_%s=\"", data);
 /*
 * escape value
 */
 while (*p2) {
 switch (*p2) {
 case '"': case '\\': case '`': case '$':
 putc ('\\', stdout);
 default:
 putc (*p2, stdout);
 break;
 }
 p2++;
 }
 putc ('"', stdout);
 putc ('\n', stdout);
 *ptr = '=';
 free (data);
}
int
main (int argc, char *argv[])
{
 char *ptr, *data = LoadInput();
 int i;
 /*
 * Handle CGI data
 */
 if (data) {
 ptr = strtok (data, "&");
 while (ptr) {
 HandleString (ptr);
 ptr = strtok (NULL, "&");
 }
 free (data);
 }
 /*
 * Add Path info
 */
 if (getenv ("PATH_INFO") != NULL) {
 data = FP_strdup (getenv ("PATH_INFO"));
 ptr = strtok (data, "/");
 while (ptr) {
 HandleString (ptr);
 ptr = strtok (NULL, "/");
 }
 free (data);
 }
 /*
 * Add args
 */
 for (i=1; i<argc; i++) {
 HandleString (argv[i]);
 }
 /*
 * done
 */
 return 0;
}

• glampert: When looking at a piece of C code such as this, I'm always suspicious about overflows and corner cases. Have you tried the Address Sanitizer tool?

Answer 1

Your program parses the Content-Length header and stores its value in an int called length:

length = atoi (getenv ("CONTENT_LENGTH")))

Then it increments this value and uses it as the argument of a call to malloc():

result = malloc (length + 1))

Suppose I were to post 2³²−1 bytes of data to your program (about 4 GB in other words). If it's running in a 32-bit environment, the value of length + 1 will overflow to zero. The subsequent call to malloc() will probably succeed, but the call to fread() will crash the program immediately.

Answer 2

• I don't see a buffer overflow.

• If ts = fread (result, sizeof (char), length, stdin) is indeed less than length, terminating the string with result[length] = '0円' leaves garbage between ts and length. I recommend result[ts] = '0円' instead.

• The HandleString parsing step could be simplified as

  while (isalnum(*ptr) || *ptr == '_')
   ptr++;
  if (ptr == data || *ptr != '=') { free(data); return; }
  

• As a general note, is there a reason to not use standard string handling functions (e.g. strdup, strchr, strrchr)?

• As another general note, vertical spacing is very inconsistent. Having both

  while (isalnum(*ptr) || *ptr == '_') ptr++;
  

  and

  while (isspace (*ptr1))
   ptr1++;
  

  is quite strange. I recommend to stick to the second style.