3
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – glampert Jan 30 '15 at 2:07
3
\$\begingroup\$

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 232−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.

\$\endgroup\$
2
\$\begingroup\$
  • 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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.