>
> I will look later into the patch. But from a first look:
> the patch is bullshit. It opens up about 1000 security
> vulnerabilities because of double frees.
Yes - when I was going over the body of the code, I'd forgotten the
definition of SAFE_RETURN, which takes care of the frees. That was
carelessness on my part, I will admit.
> And: show me one system where memchr doesnt like zero as count. This
> would be a 100% misbehaviour and it is the libc programmers
> fault. If they don't write working libraries thats not our
> fault. (Hmm right now I am unsure if memchr is a gcc builtin
> function, too) And it is 100% assured that the buffer is zero
> terminated. All the complicated checks around the str* functions are
> not needed, cause those compares will fail on the terminating '\0'
> and stop. All the checks for more data aren't needed, cause the
> state maschine automaticly terminates if there is no data after a
> state swap.
Philosphical comment follows after.
I actually commented on where I was patching for this (to the effect
that it was probably overkill) and where there were other
problems. And there are other problems.
Line 233 does a memchr to find a new line. What happens if it fails?
Look at the two lines following. That portion of the submitted patch,
by any standard is not bogus nor bullshit.
loc2 = memchr(loc + 1, '\n', rem);
rem -= (loc2 - ptr) + 1;
ptr = loc2 + 1;
That's bad code.
Line 276 looks for a backslash in a submitted filename. If that
backslash is the last character, it will then call
safe_php_register_variable on an empty string. This is probably not a
good idea.
Line 308 assumes that loc2 (a pointer to a newline) is followed by at
least one more character before the terminating zero. It runs a while
loop, looking for newlines, always making this assumption. This is not
clever.
line 389 contains a peculiar if() statement which has some bits in it
which have no sense and should leave any competent programmer
wondering what in the hell the intention was. And it may well not work
in many cases.
Buf is the full line to be parsed. By the time you reach here, loc is
a pointer beyond the Content-Dispostion string and a lot else). It is
simply wrong code which happens to work.
It's not possible to guess the programmer's intention. I think it was
that either
> Printing out 4 bytes after the terminating '\0' may crash. But its
> very unlikely and f.e. on linux, solaris it cannot happen. Maybe it
> could crash on *BSD, but thats untested.
I don't follow this at all. Unless you can be sure that this function
is always called with buf being an automatic variable of the caller,
or that it's a static global which is known not to end the data
segment, or that the memory allocate routines always add housekeeping
data after every malloced buffer, then you have no way of knowing if
buf+cnt+1 is part of the process data space. If it's not, an attempt
to read it will get a SEGV. Under Linux. Under Solaris. Under *BSD.
And a philosophical note:
Adding extra checks to code seldom hurts.
1) It means that the next person looking at the code doesn't need to
track all the execution paths to ensure that the following
block of code will not cause problems.
2) Often it's cheaper to see if there's room for a string before
initiating the search, so a check like that may be more efficient.
3) Most programmer's will spend more time looking at other people's
code than writing new code. And every bit of that code which shows
that the original author(s) have considered all the edge cases
simply makes it easier for the next person.
4) Code which is 'very unlikely' to crash is code which is begging for
a script kiddy to improve the odds. PHP is deployed on web servers
and web servers are a popular target for defacement and/or DOS
attacks. So the fact that you believe it's 'unlikely' that it can
be caused to crash (instead of wanting code where it *will not*
crash, does not speak well for the basic approach to coding.
5) The entire php project has not released a working fix for an
exploitable bug (it may stop the exploit, the code is still wide
open to crashing). Worse, the entire world has been told that 4.1.2
is a fix, and it contains, besides the points raised above, two
attrocious non-working attempts to fix other problems (the
memchar()+1 code). The response in the developers lists is:
Version 4.2 has new code. The bad patch is fixed in CVS.
This overlooks the fact that CERT and most other security services
(like most of the Linux distributions) are putting out 4.1.2. And
the attitude in the devlopers list appears to be a "Don't care,
it's not my problem". I already had a previous response to the
effect that a php server core-dumping wasn't really a problem.
At any rate, since you don't like to see any patches which aren't
provable necessary, here's a reduced set (although I simply can't, in
good conscience leave in quoted strings and magic-number string
lengths in code.
Version 2:
--- rfc1867.c.save Sun Mar 3 16:45:36 2002
+++ rfc1867.c Sun Mar 3 17:26:28 2002
@@ -152,8 +152,11 @@
}
break;
case 1: /* Check content-disposition */
- while (strncasecmp(ptr, "Content-Disposition:
form-data;", 31)) {
- if (rem < 31) {
+ /* let the complier do the counting */
+#define CONT_DISP "Content-Disposition: form-data;"
+
+ while (strncasecmp(ptr, CONT_DISP, strlen(CONT_DISP)))
+{
+ if (rem < strlen(CONT_DISP)) {
SAFE_RETURN;
}
if (ptr[1] == '\n') {
@@ -162,20 +165,22 @@
SAFE_RETURN;
}
/* some other headerfield found, skip it */
- loc = (char *) memchr(ptr, '\n', rem)+1;
+ loc = (char *) memchr(ptr, '\n', rem);
if (!loc) {
/* broken */
php_error(E_WARNING, "File Upload Mime
headers garbled ptr: [%c%c%c%c%c]", *ptr, *(ptr + 1), *(ptr + 2), *(ptr + 3), *(ptr +
4));
SAFE_RETURN;
}
+ ++loc;
while (*loc == ' ' || *loc == '\t') {
/* other field is folded, skip it */
- loc = (char *) memchr(loc, '\n',
rem-(loc-ptr))+1;
+ loc = (char *) memchr(loc, '\n',
+rem-(loc-ptr));
if (!loc) {
/* broken */
php_error(E_WARNING, "File
Upload Mime headers garbled ptr: [%c%c%c%c%c]", *ptr, *(ptr + 1), *(ptr + 2), *(ptr +
3), *(ptr + 4));
SAFE_RETURN;
}
+ ++loc;
}
rem -= (loc - ptr);
ptr = loc;
@@ -200,10 +205,12 @@
php_error(E_WARNING, "File Upload Mime headers
garbled ptr: [%c%c%c%c%c]", *ptr, *(ptr + 1), *(ptr + 2), *(ptr + 3), *(ptr + 4));
SAFE_RETURN;
}
- name = strstr(ptr, " name=");
+
+#define NAME_EQUAL " name="
+ name = strstr(ptr, NAME_EQUAL);
ptr = loc;
if (name && name < loc) {
- name += 6;
+ name += strlen(NAME_EQUAL);
if ( *name == '\"' ) {
name++;
s = memchr(name, '\"', loc - name);
@@ -211,7 +218,10 @@
s = strpbrk(name, "
\t()<>@,;:\\\"/[]?=\r\n");
}
if (!s) {
- php_error(E_WARNING, "File Upload Mime
headers garbled name: [%c%c%c%c%c]", *name, *(name + 1), *(name + 2), *(name + 3),
*(name + 4));
+ /* we don't know what's left in the
+buffer, so don't
+ * try to print it (used to print
+name[0..4])
+ */
+ php_error(E_WARNING, "File Upload Mime
+headers garbled name: ");
SAFE_RETURN;
}
@@ -230,7 +240,11 @@
* below. fix moved there and restored
* pre 4.0.6 code here
*/
- loc2 = memchr(loc + 1, '\n', rem);
+
+ if ((loc2 = memchr(loc + 1, '\n', rem)) == 0) {
+ php_error(E_WARNING, "Garbled file
+upload headers");
+ SAFE_RETURN;
+ }
rem -= (loc2 - ptr) + 1;
ptr = loc2 + 1;
/* is_arr_upload is true when name of file
upload field
@@ -250,12 +264,13 @@
php_error(E_WARNING, "File upload error - no
name component in content disposition");
SAFE_RETURN;
}
- filename = strstr(s, "filename=\"");
+#define FILEN_EQ "filename=\""
+ filename = strstr(s, FILEN_EQ);
if (filename && filename < loc) {
- filename += 10;
+ filename += strlen(FILEN_EQ);
s = memchr(filename, '\"', loc - filename);
- if (!s) {
- php_error(E_WARNING, "File Upload Mime
headers garbled filename: [%c%c%c%c%c]", *filename, *(filename + 1), *(filename + 2),
*(filename + 3), *(filename + 4));
+ if (!s || strlen(s) == 0) {
+ php_error(E_WARNING, "File Upload Mime
+headers garbled filename");
SAFE_RETURN;
}
if (filenamebuf) {
@@ -274,6 +289,11 @@
sprintf(lbuf, "%s_name", namebuf);
}
s = strrchr(filenamebuf, '\\');
+ if (s && s[1] == 0) {
+ /* ends in a trailing backslash */
+ php_error(E_WARNING, "File Upload Mime
+headers garbled filename");
+ SAFE_RETURN;
+ }
if (s && s > filenamebuf) {
safe_php_register_variable(lbuf, s+1,
NULL, 0 TSRMLS_CC);
} else {
@@ -294,24 +314,33 @@
state = 3;
s = "";
+#define CONT_TYPE "Content-Type:"
if ((loc2 - loc) > 2) {
- if (!strncasecmp(loc + 1,
"Content-Type:", 13)) {
+ /* loc => newline, loc2 to next
+newline */
+ if (!strncasecmp(loc + 1, CONT_TYPE,
+strlen(CONT_TYPE))) {
c = *(loc2 - 1);
*(loc2 - 1) = '\0';
- s = loc+15;
+ s = loc+strlen(CONT_TYPE) + 2;
}
/* end of header fix fixed and moved
here
* find the double newline that marks
the
* end of the headers
*/
+
+ if (rem < 2) {
+ php_error(E_WARNING, "File
+Upload Mime headers garbled header3");
+ SAFE_RETURN;
+ }
+
loc3 = loc2;
- while (loc3[2] != '\n') {
+
+ while ((cnt - (loc3 - buf) >= 2) &&
+loc3[2] != '\n') {
/* empty line as end of
headers not yet found */
loc3 = memchr(loc3 + 1, '\n',
rem-(loc3-ptr)-1);
if (loc3==NULL) {
- php_error(E_WARNING,
"File Upload Mime headers garbled header3: [%c%c%c%c%c]", *loc2, *(loc2 + 1), *(loc2 +
2), *(loc2 + 3), *(loc2 + 4));
- SAFE_RETURN;
+ php_error(E_WARNING,
+"File Upload Mime headers garbled header3");
+ SAFE_RETURN;
}
}
rem -= (loc3 - ptr) + 3;
@@ -388,8 +417,8 @@
while (loc) {
if (!strncmp(loc, boundary, len)
#if NEW_BOUNDARY_CHECK
- && (loc-2>buf && *(loc-2)=='-' &&
*(loc-1)=='-') /* ensure boundary is prefixed with -- */
- && (loc-2==buf || *(loc-3)=='\n') /*
ensure beginning of line */
+ && *(loc-2)=='-' && *(loc-1)=='-' /*
+ensure boundary is prefixed with -- */
+ && *(loc-3)=='\n') /* ensure beginning
+of line */
#endif
) {
break;
--
Jim Segrave [EMAIL PROTECTED]
--
PHP Development Mailing List <http://www.php.net/>
To unsubscribe, visit: http://www.php.net/unsub.php
