The case of HTTP response splitting protection in PHP

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 == 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, p;
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.

https://bugs.php.net/bug.php?id=60227

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, p;
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 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 = 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 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