Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:57655
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain sektioneins.de designates 85.214.103.31 as permitted sender)
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: quoted-printable
Date: Fri, 3 Feb 2012 12:06:26 +0100
Message-ID: <D5EC076D-8518-4CDC-913F-5A3F3BB97367@sektioneins.de>
To: internals PHP <internals@lists.php.net>
Mime-Version: 1.0 (Apple Message framework v1251.1)
Subject: The case of HTTP response splitting protection in PHP
From: stefan.esser@sektioneins.de (Stefan Esser)

Hello,

I think current events show how important it is to make this case =
publicly known.

On Dec 6th 2005 PHP got a partial protection against HTTP response =
splitting. A security mitigation =3D=3D Security Patch =3D=3D important

The commit is here: =
http://svn.php.net/viewvc/php/php-src/trunk/main/SAPI.c?r1=3D200124&r2=3D2=
02220

569	 	     /* new line safety check */
570	 	     {
571	 	         char *s =3D header_line, *e =3D header_line + =
header_line_len, *p;
572	 	         while (s < e && (p =3D memchr(s, '\n', (e - =
s)))) {
573	 	             if (*(p + 1) =3D=3D ' ' || *(p + 1) =3D=3D =
'\t') {
574	 	                 s =3D 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.

https://bugs.php.net/bug.php?id=3D60227

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=3D3=
21634&r2=3D322263):

592	 	     /* new line safety check */
593	 	         char *s =3D header_line, *e =3D header_line + =
header_line_len, *p;
594	 	         while (s < e && ((p =3D memchr(s, '\n', (e - =
s))) || (p =3D memchr(s, '\r', (e - s))))) {
595	 	             if (*(p + 1) =3D=3D ' ' || *(p + 1) =3D=3D =
'\t') {
596	 	                 s =3D 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=3DYYY;  \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=3D321634&r2=3D3=
23033
Best thing about this commit is the commit message:

- Hopefully correct fix for bug #60227.
#No commit for 5.4 for now

=46rom 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 fix/improvement.
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 =3D header_line;
715	        while (s =3D strpbrk(s, "\n\r")) {
716	            if (s[1] =3D=3D ' ' || s[1] =3D=3D '\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 =3D header_line;
728	    sapi_header.header_len =3D 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=85 =
that security check is using a string function that will stop at a NUL =
byte.
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 depth.

Regards,
Stefan