Permalink
Browse files

Accepted patches from Jeffrey Hutzelman:

I've attached a patch that fixes several problems we found in CGIC 2.05.
I can split these up into multiple patches, if that's helpful, but as
it is each fix is fairly self-contained.

* In main(), when parsing form input fails, the CGI script exits without
  producing any output whatsoever.  Wouldn't it be better to actually
  emit an error status, instead of expecting the server to do something
  sane with a script that produces no output?

* In mpRead(), a check is done to insure the requested length is not
  greater than the amount of data still available, and to adjust it
  if necessary.  However, this check is currently done _after_ reading
  data from the putback buffer, in which process len is decremented by
  the amount of putback data read, but mpp->offset is not correspondingly
  incremented (this happens later).  As a result, the check uses too
  small a value for len, and so fails to stop reading soon enough if
  the requested length is greater than what is available _and_ there
  was any data in the putback buffer.
  The fix is to move the check to the beginning of mpRead()

* Further, if a read request is satisfied _entirely_ from the putback
  buffer, mpp->offset is not updated at all, resulting in a similar
  problem.  The solution is to update mpp->offset in the "else if (got)"
  case.

* In cgiParsePostMultipartInput(), if the Content-Disposition of a part
  is not "form-data", afterNextBoundary() is not called before beginning
  to process the next part.  As a result, parsing of the next part headers
  begins with the body of the unwanted part.  It is necessary in this case
  to call afterNextBoundary() before continuing with the next cycle.

* In handling out-of-memory conditions in afterNextBoundary(), *outP is
  set to '\0'.  While this is technically legal ('\0' is "an integral
  constant expression with the value 0"), it looks funny.

* In cgiCookieString(), a change was introduced in v2.02 which purports
  to prevent an overrun in cases where cgiCookie is exactly equal to
  the requested cookie name.  In fact, the problem can also occur if
  the requested name occurs with no values at the end of cgiCookie.
  Further, the change from v2.02 does not fix the problem, because it
  compares the _pointers_ p and n to NULL, which they will never equal,
  rather than comparing the pointers they point at to NUL.

* Also in cgiCookieString(), there is a comment suggesting that the main
  loop never terminates except with a return.  This is not the case.
  For example, it will terminate if the requested cookie is not found
  and the cgiCookie string ends in a semicolon.

* Why did days[] (formerly daysOfWeek[]) and months[] become non-static?
  This pollutes the namespace of programs using CGIC.

* In cgiReadEnvironment(), when reading in the contents of an uploaded
  file, it is possible that a temporary file is successfully created
  but then cannot be opened.  In this case, no attempt is made to remove
  the tempoary file.

* Further, when a form entry does _not_ include an uploaded file,
  e->tfileName is set to malloc'd but uninitialized memory.  It should
  be set to an empty string, by setting e->tfileName[0] to zero after
  the 1-byte buffer is allocated.
  • Loading branch information...
boutell committed Jan 23, 2014
1 parent 2574509 commit 2065a2dfa480209bd6171acb2a4edd6e1c27a062
Showing with 21 additions and 9 deletions.
  1. +21 −9 cgic.c
View
30 cgic.c
@@ -235,6 +235,7 @@ int main(int argc, char *argv[]) {
fprintf(dout, "PostFormInput failed\n");
CGICDEBUGEND
#endif /* CGICDEBUG */
+ cgiHeaderStatus(500, "Error reading form data");
cgiFreeResources();
return -1;
}
@@ -255,6 +256,7 @@ int main(int argc, char *argv[]) {
fprintf(dout, "PostMultipartInput failed\n");
CGICDEBUGEND
#endif /* CGICDEBUG */
+ cgiHeaderStatus(500, "Error reading form data");
cgiFreeResources();
return -1;
}
@@ -274,6 +276,7 @@ int main(int argc, char *argv[]) {
fprintf(dout, "GetFormInput failed\n");
CGICDEBUGEND
#endif /* CGICDEBUG */
+ cgiHeaderStatus(500, "Error reading form data");
cgiFreeResources();
return -1;
} else {
@@ -342,6 +345,11 @@ int mpRead(mpStreamPtr mpp, char *buffer, int len)
{
int ilen = len;
int got = 0;
+ /* Refuse to read past the declared length in order to
+ avoid deadlock */
+ if (len > (cgiContentLength - mpp->offset)) {
+ len = cgiContentLength - mpp->offset;
+ }
while (len) {
if (mpp->readPos != mpp->writePos) {
*buffer++ = mpp->putback[mpp->readPos++];
@@ -352,11 +360,6 @@ int mpRead(mpStreamPtr mpp, char *buffer, int len)
break;
}
}
- /* Refuse to read past the declared length in order to
- avoid deadlock */
- if (len > (cgiContentLength - mpp->offset)) {
- len = cgiContentLength - mpp->offset;
- }
if (len) {
int fgot = fread(buffer, 1, len, cgiIn);
if (fgot >= 0) {
@@ -370,6 +373,7 @@ int mpRead(mpStreamPtr mpp, char *buffer, int len)
return fgot;
}
} else if (got) {
+ mpp->offset += got;
return got;
} else if (ilen) {
return EOF;
@@ -508,6 +512,11 @@ static cgiParseResultType cgiParsePostMultipartInput() {
}
if (!cgiStrEqNc(fvalue, "form-data")) {
/* Not form data */
+ result = afterNextBoundary(mpp, 0, 0, 0, 0);
+ if (result != cgiParseSuccess) {
+ /* Lack of a boundary here is an error. */
+ return result;
+ }
continue;
}
/* Body is everything from here until the next
@@ -779,7 +788,7 @@ cgiParseResultType afterNextBoundary(mpStreamPtr mpp, FILE *outf, char **outP,
if (out) {
free(out);
}
- *outP = '\0';
+ *outP = 0;
}
error:
if (bodyLengthP) {
@@ -1716,7 +1725,7 @@ cgiFormResultType cgiCookieString(
genuine security concern. Thanks to Nicolas
Tomadakis. */
while (*p == *n) {
- if ((p == '\0') && (n == '\0')) {
+ if ((*p == '\0') && (*n == '\0')) {
/* Malformed cookie header from client */
return cgiFormNotFound;
}
@@ -1768,6 +1777,7 @@ cgiFormResultType cgiCookieString(
}
/* 2.01: actually the above loop never terminates except
with a return, but do this to placate gcc */
+ /* Actually, it can, so this is real. */
if (space) {
*value = '\0';
}
@@ -1798,7 +1808,7 @@ void cgiHeaderCookieSetInteger(char *name, int value, int secondsToLive,
cgiHeaderCookieSetString(name, svalue, secondsToLive, path, domain);
}
-char *days[] = {
+static char *days[] = {
"Sun",
"Mon",
"Tue",
@@ -1808,7 +1818,7 @@ char *days[] = {
"Sat"
};
-char *months[] = {
+static char *months[] = {
"Jan",
"Feb",
"Mar",
@@ -2157,6 +2167,7 @@ cgiEnvironmentResultType cgiReadEnvironment(char *filename) {
out = fopen(tfileName, "w+b");
if (!out) {
result = cgiEnvironmentIO;
+ unlink(tfileName);
goto error;
}
while (len > 0) {
@@ -2197,6 +2208,7 @@ cgiEnvironmentResultType cgiReadEnvironment(char *filename) {
result = cgiEnvironmentMemory;
goto error;
}
+ e->tfileName[0] = '\0';
}
e->next = 0;
if (p) {

0 comments on commit 2065a2d

Please sign in to comment.