Features Download
From: Stefan Esser <stefan.esser <at> sektioneins.de>
Subject: [PHP-DEV] The case of HTTP response splitting protection in PHP
Newsgroups: gmane.comp.php.devel
Date: Friday 3rd February 2012 11:06:26 UTC (over 6 years ago)

I think current events show how important it is to make this case publicly

On Dec 6th 2005 PHP got a partial protection against HTTP response
splitting. A security mitigation == Security Patch == important

The commit is here: http://svn.php.net/viewvc/php/php-src/trunk/main/SAPI.c?r1=200124&r2=202220

569	 	     /* new line safety check */
570	 	     {
571	 	         char *s = header_line, *e = header_line + header_line_len,
572	 	         while (s < e && (p = memchr(s, '\n', (e - s)))) {
573	 	             if (*(p + 1) == ' ' || *(p + 1) == '\t') {
574	 	                 s = p + 1;
575	 	                 continue;
576	 	             }
577	 	             efree(header_line);
578	 	             sapi_module.sapi_error(E_WARNING, "Header may not
contain more then a single header, new line detected.");
579	 	             return FAILURE;
580	 	         }
581	 	     }

As you can see the code checks for \n and only allows it if it followed by
whitespace to protect against header injections.
Now fast forward to 2011/2012 the PHP developers get a security bug report
that this protection is not sufficient, because browsers are too allowing.


You can see that this bug is not marked as some kind of security bug,
although it is reported as security bug.

This results in the following code being changed in PHP see (http://svn.php.net/viewvc/php/php-src/branches/PHP_5_3/main/SAPI.c?r1=321634&r2=322263):

592	 	     /* new line safety check */
593	 	         char *s = header_line, *e = header_line + header_line_len,
594	 	         while (s < e && ((p = memchr(s, '\n', (e - s))) || (p =
memchr(s, '\r', (e - s))))) {
595	 	             if (*(p + 1) == ' ' || *(p + 1) == '\t') {
596	 	                 s = p + 1;
597	 	                 continue;

Keep in mind this is a security fix/improvement. So one would expect this
to get reviewed. But it is obviously not reviewed, because any \r before
the last \n won't be checked.
So bypassing this is easy as   \rSet-Cookie: XXX=YYY;  \r \n blah

So I point this obvious flaw out several times, yesterday on the internals
mailinglist in public infront of everyone.
But instead of someone from the security team taking action just some guy
gets going and patching it.

The commit is here: http://svn.php.net/viewvc/php/php-src/trunk/main/SAPI.c?r1=321634&r2=323033
Best thing about this commit is the commit message:

- Hopefully correct fix for bug #60227.
#No commit for 5.4 for now

From the commit message it seems obvious that the guy commiting it is not
really sure that he fixed anything. Not a good way to handle a security
But it gets better: without review this code is now merged from Trunk into
PHP 5.3

So the new code looks like this:

714	        char *s = header_line;
715	        while (s = strpbrk(s, "\n\r")) {
716	            if (s[1] == ' ' || s[1] == '\t') {
717	                /* RFC 2616 allows new lines if followed by SP or HT */
718	                s++;
719	                continue;
720	            }

well lets look a bit further:

727	    sapi_header.header = header_line;
728	    sapi_header.header_len = header_line_len;

Now the educated reader might think: Wait a second! There is a
header_line_len? So maybe that header_line can contain NUL bytes. Wait…
that security check is using a string function that will stop at a NUL
And then maybe the person reading the code will realize: wait! they just
killed the whole protection, because now a single NUL byte infront of the
\r\n will bypass it.

Luckily it is not affecting everyone, because at least the Apache SAPI will
stop sending the header at the NUL byte, too.
However everybody running CGI/FastCGI will loose the protection with this.

And that my dear readers is exactly what would happen to the code of
Suhosin if it gets merged into PHP. It would be patched by people that do
not know exactly what they are doing. And it would be broken. And if I
would not sit on every single commit to PHP this would happen, because
obviously inside PHP no one cares about reviewing security patches.

And with Suhosin outside of PHP, there is a secondary protection layer
around PHP that would have catched this problem: also known as defense in


PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
CD: 3ms