# The case of HTTP response splitting protection in PHP - Thread: https://externals.io/message/57655 - Original archive: https://news-web.php.net/php.internals/57655 - Started by Stefan Esser on 2012-02-03T11:06:26+00:00 - Messages: 18 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #57655 — Stefan Esser — 2012-02-03T11:06:26+00:00 URL: https://externals.io/message/57655#57655 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 --- ## #57656 — Gustavo Lopes — 2012-02-03T11:21:32+00:00 URL: https://externals.io/message/57655#57656 · In reply to: #57655 On Fri, 03 Feb 2012 12:06:26 +0100, Stefan Esser wrote: > [snip] > > 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. > [snip] Good point, and I humbly take the blame as I simply assumed the header was being passed as a C string (odd that they aren't since `NULL` bytes are not allowed in headers anyway but besides the point). -- Gustavo Lopes --- ## #57657 — Gustavo Lopes — 2012-02-03T12:03:24+00:00 URL: https://externals.io/message/57655#57657 · In reply to: #57655 On Fri, 03 Feb 2012 12:06:26 +0100, Stefan Esser wrote: > [snip] > obviously inside PHP no one cares about reviewing security patches. > Perhaps then you'd want to comment on: http://nebm.ist.utl.pt/~glopes/misc/bug60227.diff , which addresses the NUL byte issue, although now I'm thinking that since we're in the business of validating HTTP headers, we could also forbid the other control characters that are forbidden by the spec (not just LF and CR). Thank you -- Gustavo Lopes --- ## #57660 — Gustavo Lopes — 2012-02-03T14:01:38+00:00 URL: https://externals.io/message/57655#57660 · In reply to: #57657 On Fri, 03 Feb 2012 13:03:24 +0100, Gustavo Lopes wrote: > On Fri, 03 Feb 2012 12:06:26 +0100, Stefan Esser > wrote: > > > […] > > Perhaps then you'd want to comment on: > http://nebm.ist.utl.pt/~glopes/misc/bug60227.diff , which addresses the > NUL byte issue, although now I'm thinking that since we're in the > business of validating HTTP headers, we could also forbid the other > control characters that are forbidden by the spec (not just LF and CR). > I've committed a different version that also forbids \0 (since, as Stefan says, a NUL byte can result in the truncation of the rest of the header) and that accepts a CRLF: http://svn.php.net/viewvc/php/php-src/trunk/main/SAPI.c?r1=323043&r2=323042&pathrev=323043 If you or anyone else find any problem, please report a bug; otherwise I'll merge to 5.3 and 5.4 once 5.4 is out of code freeze. Thanks -- Gustavo Lopes --- ## #57661 — John LeSueur — 2012-02-03T14:13:56+00:00 URL: https://externals.io/message/57655#57661 · In reply to: #57660 On Fri, Feb 3, 2012 at 7:01 AM, Gustavo Lopes wrote: > On Fri, 03 Feb 2012 13:03:24 +0100, Gustavo Lopes > wrote: > > On Fri, 03 Feb 2012 12:06:26 +0100, Stefan Esser < > > […] > I've committed a different version that also forbids \0 (since, as Stefan > says, a NUL byte can result in the truncation of the rest of the header) > and that accepts a CRLF: > > http://svn.php.net/viewvc/php/**php-src/trunk/main/SAPI.c?r1=** > 323043&r2=323042&pathrev=**323043 > > If you or anyone else find any problem, please report a bug; otherwise > I'll merge to 5.3 and 5.4 once 5.4 is out of code freeze. > > Thanks > > > -- > Gustavo Lopes > > -- > > > > I could be wrong, but doesn't: (header_line[i+1] != ' ' && header_line[i+1] != '\t') access an element past the end of the header_line array on the last iteration of the for loop? shouldn't the for loop go until header_line_len - 1? John --- ## #57662 — Gustavo Lopes — 2012-02-03T14:16:31+00:00 URL: https://externals.io/message/57655#57662 · In reply to: #57661 On Fri, 03 Feb 2012 15:13:56 +0100, John LeSueur wrote: > I could be wrong, but doesn't: > (header_line[i+1] != ' ' && header_line[i+1] != '\t') > access an element past the end of the header_line array on the last > iteration of the for loop? shouldn't the for loop go until > header_line_len > - 1? > No in this case because header_line_len is the length not including the terminator and estrndup guarantees a null terminated string. Thanks -- Gustavo Lopes --- ## #57665 — keisial@gmail.com — 2012-02-03T20:44:51+00:00 URL: https://externals.io/message/57655#57665 · In reply to: #57660 > I've committed a different version that also forbids \0 (since, as > Stefan says, a NUL byte can result in the truncation of the rest of > the header) and that accepts a CRLF: > > http://svn.php.net/viewvc/php/php-src/trunk/main/SAPI.c?r1=323043&r2=323042&pathrev=323043 > Looks good. But given that the goal is to make this robust, I would go further: a) Replace any CRLF + [ \r] with SP (rfc2616 allows us "A recipient MAY replace any linear white space with a single SP before forwarding the message downstream.", and this also protects UAs not following the spec) b) Bail out on any header_line[i] < ' ' (ie. fail on any special char) > If you or anyone else find any problem, please report a bug; otherwise > I'll merge to 5.3 and 5.4 once 5.4 is out of code freeze. > > Thanks As it's a security patch and of small scope, I would consider it for 5.4. Stas, David? --- ## #57666 — Reindl Harald — 2012-02-03T21:37:12+00:00 URL: https://externals.io/message/57655#57666 · In reply to: #57665 Am 03.02.2012 21:44, schrieb Ángel González: > > […] > As it's a security patch and of small scope, I would consider it for > 5.4. Stas, David? as it is SECURITY relevant it has to be considered NOW meaning 5.3 and not sometimes in the future! --- ## #57667 — Ferenc Kovacs — 2012-02-03T21:52:02+00:00 URL: https://externals.io/message/57655#57667 · In reply to: #57666 On Fri, Feb 3, 2012 at 10:37 PM, Reindl Harald wrote: > > > Am 03.02.2012 21:44, schrieb Ángel González: > > […] > > as it is SECURITY relevant it has to be considered NOW meaning 5.3 and not > sometimes in the future! > > of course it will be included in 5.3 5.4 was only mentioned explicitly, because it is under commit freeze and only commits approved by the RMs allowed to be included. in this case I think they will approve this obviously. -- Ferenc Kovács @Tyr43l - http://tyrael.hu --- ## #57668 — Stas Malyshev — 2012-02-03T22:00:11+00:00 URL: https://externals.io/message/57655#57668 · In reply to: #57665 Hi! > As it's a security patch and of small scope, I would consider it for > 5.4. Stas, David? Do we have unit tests for this code? The fix involves changes in header sending so it may have impact on lots of code. Changes like this can be dangerous. I'm thinking maybe we should wait with it until 5.4.1. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #57670 — Pierre Joye — 2012-02-03T22:22:01+00:00 URL: https://externals.io/message/57655#57670 · In reply to: #57666 > > > Am 03.02.2012 21:44, schrieb Ángel González: > > […] > > as it is SECURITY relevant it has to be considered NOW meaning 5.3 and not > sometimes in the future! Point is to apply well tested and approved and reviewed fixes to 5.3 or 5.4. Not blindly apply something only because it is related to a security issue. PS: We are not blind so please use lower case only. -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #57671 — keisial@gmail.com — 2012-02-03T22:46:29+00:00 URL: https://externals.io/message/57655#57671 · In reply to: #57668 > Hi! > > > […] > > Do we have unit tests for this code? The fix involves changes in header > sending so it may have impact on lots of code. Changes like this can be > dangerous. I'm thinking maybe we should wait with it until 5.4.1. PHP_5_4 already contains code banning \n or \r newlines. The one which could be bypassed by the "\n Header: Foo\r Foo". Gustavo patch is fixing it to do what was meant to do. I think that any good application relying on `header()` to send multiple ones would fail with the incomplete fix, so I see little difference in compatibility with using the full one. --- ## #57672 — keisial@gmail.com — 2012-02-03T23:06:45+00:00 URL: https://externals.io/message/57655#57672 · In reply to: #57665 > > > […] > Looks good. But given that the goal is to make this robust, I would go > further: > a) Replace any CRLF + [ \r] with SP > (rfc2616 allows us "A recipient MAY replace any linear white space > with a single SP before forwarding the message downstream.", and > this also protects UAs not following the spec) > > b) Bail out on any header_line[i] < ' ' (ie. fail on any special char) I've gone ahead and written code for that feature. Comments welcome. --- ## #57695 — Nikita Popov — 2012-02-04T10:35:37+00:00 URL: https://externals.io/message/57655#57695 · In reply to: #57672 > I've gone ahead and written code for that feature. Comments welcome. If I understand it correctly your code only allows \r\n[ \t] style line continuations, whereas catahracts previous version allowed (?>\r\n|\r|\n)[ \t]. Is that intentional? Nikita --- ## #57696 — Gustavo Lopes — 2012-02-04T10:42:17+00:00 URL: https://externals.io/message/57655#57696 · In reply to: #57672 > > > […] > I've gone ahead and written code for that feature. Comments welcome. The comparison has a problem: if char is signed (the most common scenario), you'll be making a signed comparison, so any character over 0x7f will match (if it's an unsigned char, though, it will work, because of the integer promotions and usual arithmetic conversions). It suffices to replace ' ' with 0x20U. Other than that, I am a little uncertain about the impact of this strictness could have on current applications, even if if correct. -- Gustavo Lopes --- ## #57697 — Gustavo Lopes — 2012-02-04T11:26:14+00:00 URL: https://externals.io/message/57655#57697 · In reply to: #57668 > Hi! > > > […] > > Do we have unit tests for this code? The fix involves changes in > header sending so it may have impact on lots of code. Changes like > this can be dangerous. I'm thinking maybe we should wait with it > until > 5.4.1. This bug has now four tests and there are some other tests than include calls to `header()`. That said, I wouldn't consider this critical. This is only relevant if the programmer included user data in the header without validation. -- Gustavo Lopes --- ## #57702 — keisial@gmail.com — 2012-02-04T15:35:04+00:00 URL: https://externals.io/message/57655#57702 · In reply to: #57696 Gustavo Lopes wrote: > > > […] > > The comparison has a problem: if char is signed (the most common > scenario), you'll be making a signed comparison, so any character over > 0x7f will match (if it's an unsigned char, though, it will work, > because of the integer promotions and usual arithmetic conversions). > It suffices to replace ' ' with 0x20U. Yes. I woke up this morning thinking on it. That the line should have been if ((unsigned char)header_line[i] < 32) { I wouldn't have thought on using 0x20U instead of 32, but it seems that it would work, too. > Other than that, I am a little uncertain about the impact of this > strictness could have on current applications, even if if correct. In my humble opinion, there won't be applications using header continuation. PHP could even define that `header()` would only accept a single header with no line continuation with very little impact. --- ## #57703 — Gustavo Lopes — 2012-02-04T15:49:09+00:00 URL: https://externals.io/message/57655#57703 · In reply to: #57702 > Gustavo Lopes wrote: > > […] > Yes. I woke up this morning thinking on it. That the line should have > been > if ((unsigned char)header_line[i] < 32) { > > I wouldn't have thought on using 0x20U instead of 32, but it seems > that > it would work, too. > > > […] > In my humble opinion, there won't be applications using header > continuation. PHP could even define that `header()` would only > accept a single header with no line continuation with very little > impact. There's another problem. HTs are allowed in headers, but your patch forbids them. And I think forbidding other C0 characters may be a good idea, but not for the stable releases. -- Gustavo Lopes