Hi,
Please drop multiline HTTP headers support from PHP header() because it
was never needed in that layer, it is a security risk in combination
with a certain IE bug, IE didn't support such multiline response headers
properly anyway, and they are deprecated by RFC 7230:
https://twitter.com/d0znpp/status/483147480843186176
http://lab.onsec.ru/2012/08/php-multiple-headers-bypass-available.html
http://tools.ietf.org/html/rfc7230#section-3.2.4
I brought this to Pierre Joye's attention on Twitter today, and he
agrees that "yes it should be removed" and asked me to "drop a mail to
internals". So I just did.
Alexander
Please drop multiline HTTP headers support from PHP
header()
Would this be a backwards-compatibility break? We could convert multi-line headers into single-line headers, I suppose, but surely it would still break BC?
Be that the case, we should probably only do this for PHP 6. Though I wonder if multi-line headers are obscure enough, and the security benefits justifiable enough, that we could do it in 5.7.
Andrea Faulds
http://ajf.me/
Please drop multiline HTTP headers support from PHP
header()Would this be a backwards-compatibility break?
Technically, yes.
In practice, I expect that there are no PHP apps that make use of this
feature.
We could convert multi-line headers into single-line headers, I suppose, but surely it would still break BC?
Yes, and I think it's not a good idea anyway.
Why would header() want to support multiline headers on input to that
PHP function anyway, even with old HTTP RFC that included such support
at HTTP protocol level? I see no valid reason. Was such support
declared anywhere in the documentation, or does it simply happen to be
present in the code as an obscure feature? I guess it's the latter.
Be that the case, we should probably only do this for PHP 6. Though I wonder if multi-line headers are obscure enough, and the security benefits justifiable enough, that we could do it in 5.7.
I suggest doing it for 5.4. The new HTTP RFC is already out, so why
keep an undocumented(?) and dangerous misfeature to produce headers that
are already deprecated by the current RFC?
You just need to document the change.
Alexander
Hi!
Please drop multiline HTTP headers support from PHP
header()because it
was never needed in that layer, it is a security risk in combination
Why it's not needed in that layer? If you want to send a multiline
header allowed by RFC 2616 (assuming you do want it, for undefined
reasons), how else you do that? That's the only way to send headers in
PHP as far as I can see.
with a certain IE bug, IE didn't support such multiline response headers
properly anyway, and they are deprecated by RFC 7230:
So IE violates the RFC by misparsing the multiline headers? I'd say it's
an one more reason to never use IE :) RFC 7230 indeed proposes to remove
this capability, but it's not accepted yet, as far as I can see. We can
probably drop this immediately for 5.6, for previous versions I'm not
sure if anybody uses this feature. So if anybody knows any use of it,
please tell, otherwise it's probably a good idea to kill it for stable
versions too.
I brought this to Pierre Joye's attention on Twitter today, and he
agrees that "yes it should be removed" and asked me to "drop a mail to
internals". So I just did.
Thank you!
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
So IE violates the RFC by misparsing the multiline headers? I'd say it's
an one more reason to never use IE :) RFC 7230 indeed proposes to remove
this capability, but it's not accepted yet, as far as I can see. We can
probably drop this immediately for 5.6, for previous versions I'm not
sure if anybody uses this feature. So if anybody knows any use of it,
please tell, otherwise it's probably a good idea to kill it for stable
versions too.
As I’ve had to implement HTTP myself for a particular non-PHP application, I’ve read the original HTTP/1.1 RFC. As far as I know, multi-line headers are semantically equivalent to single-line headers, so couldn’t we just “flatten” them automatically? It shouldn’t break anything unless you’re deliberately misusing header().
--
Andrea Faulds
http://ajf.me/
So IE violates the RFC by misparsing the multiline headers? I'd say it's
an one more reason to never use IE :) RFC 7230 indeed proposes to remove
this capability, but it's not accepted yet, as far as I can see. We can
probably drop this immediately for 5.6, for previous versions I'm not
sure if anybody uses this feature. So if anybody knows any use of it,
please tell, otherwise it's probably a good idea to kill it for stable
versions too.As I?ve had to implement HTTP myself for a particular non-PHP application, I?ve read the original HTTP/1.1 RFC. As far as I know, multi-line headers are semantically equivalent to single-line headers, so couldn?t we just ?flatten? them automatically? It shouldn?t break anything unless you?re deliberately misusing
header().
I think we could, and your analysis looks correct to me, but I see no
good enough reason to go for the extra complexity. Having a function
defined in a more complicated manner and implemented with more code is
asking for more bugs and mis-interactions.
Alexander
So IE violates the RFC by misparsing the multiline headers? I'd say it's
an one more reason to never use IE :) RFC 7230 indeed proposes to remove
this capability, but it's not accepted yet, as far as I can see. We can
probably drop this immediately for 5.6, for previous versions I'm not
sure if anybody uses this feature. So if anybody knows any use of it,
please tell, otherwise it's probably a good idea to kill it for stable
versions too.As I?ve had to implement HTTP myself for a particular non-PHP application, I?ve read the original HTTP/1.1 RFC. As far as I know, multi-line headers are semantically equivalent to single-line headers, so couldn?t we just ?flatten? them automatically? It shouldn?t break anything unless you?re deliberately misusing
header().I think we could, and your analysis looks correct to me, but I see no
good enough reason to go for the extra complexity. Having a function
defined in a more complicated manner and implemented with more code is
asking for more bugs and mis-interactions.
I'd tend to agree: if we're going to do this, let's just rip the
band-aid off completely. I've got a quick and dirty patch at
https://github.com/LawnGnome/php-src/compare/remove-multiline-headers?expand=1
that does this, and applies cleanly against every branch from 5.4 to
master.
I'm not so sure about the versions this should be applied too, though:
my current inclination is to only apply it to master and maybe 5.6 if
the RMs agreed. While it's a very, very small BC break (and hence one
I'm OK with in a minor branch like 5.6), I don't think we should do
this in a 5.4 or 5.5 point release — recent history (the unserialize()
hack) suggests that it's a nightmare to document and explain those
sorts of breaks.
Adam
So IE violates the RFC by misparsing the multiline headers? I'd say
it's
an one more reason to never use IE :) RFC 7230 indeed proposes to
remove
this capability, but it's not accepted yet, as far as I can see. We
can
probably drop this immediately for 5.6, for previous versions I'm not
sure if anybody uses this feature. So if anybody knows any use of it,
please tell, otherwise it's probably a good idea to kill it for stable
versions too.As I?ve had to implement HTTP myself for a particular non-PHP
application, I?ve read the original HTTP/1.1 RFC. As far as I know,
multi-line headers are semantically equivalent to single-line headers, so
couldn?t we just ?flatten? them automatically? It shouldn?t break anything
unless you?re deliberately misusingheader().I think we could, and your analysis looks correct to me, but I see no
good enough reason to go for the extra complexity. Having a function
defined in a more complicated manner and implemented with more code is
asking for more bugs and mis-interactions.I'd tend to agree: if we're going to do this, let's just rip the
band-aid off completely. I've got a quick and dirty patch athttps://github.com/LawnGnome/php-src/compare/remove-multiline-headers?expand=1
that does this, and applies cleanly against every branch from 5.4 to
master.I'm not so sure about the versions this should be applied too, though:
my current inclination is to only apply it to master and maybe 5.6 if
the RMs agreed. While it's a very, very small BC break (and hence one
I'm OK with in a minor branch like 5.6), I don't think we should do
this in a 5.4 or 5.5 point release — recent history (theunserialize()
hack) suggests that it's a nightmare to document and explain those
sorts of breaks.Adam
--
If I'm reading this correctly this would reintroduce
https://bugs.php.net/bug.php?id=60227
those checks aren't there to support header splitting but to prevent them,
as "\r\n" isn't the only separator which will cause browsers to split the
header.
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
So IE violates the RFC by misparsing the multiline headers? I'd say
it's
an one more reason to never use IE :) RFC 7230 indeed proposes to
remove
this capability, but it's not accepted yet, as far as I can see. We
can
probably drop this immediately for 5.6, for previous versions I'm not
sure if anybody uses this feature. So if anybody knows any use of it,
please tell, otherwise it's probably a good idea to kill it for
stable
versions too.As I?ve had to implement HTTP myself for a particular non-PHP
application, I?ve read the original HTTP/1.1 RFC. As far as I know,
multi-line headers are semantically equivalent to single-line headers, so
couldn?t we just ?flatten? them automatically? It shouldn?t break anything
unless you?re deliberately misusingheader().I think we could, and your analysis looks correct to me, but I see no
good enough reason to go for the extra complexity. Having a function
defined in a more complicated manner and implemented with more code is
asking for more bugs and mis-interactions.I'd tend to agree: if we're going to do this, let's just rip the
band-aid off completely. I've got a quick and dirty patch athttps://github.com/LawnGnome/php-src/compare/remove-multiline-headers?expand=1
that does this, and applies cleanly against every branch from 5.4 to
master.I'm not so sure about the versions this should be applied too, though:
my current inclination is to only apply it to master and maybe 5.6 if
the RMs agreed. While it's a very, very small BC break (and hence one
I'm OK with in a minor branch like 5.6), I don't think we should do
this in a 5.4 or 5.5 point release — recent history (theunserialize()
hack) suggests that it's a nightmare to document and explain those
sorts of breaks.Adam
--
If I'm reading this correctly this would reintroduce
https://bugs.php.net/bug.php?id=60227
those checks aren't there to support header splitting but to prevent them,
as "\r\n" isn't the only separator which will cause browsers to split the
header.--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
and for the record, my irc history tells me that Nikita sent an email about
http://lab.onsec.ru/2012/08/php-multiple-headers-bypass-available.html to
security@php.net back in 2012-08-24, so maybe there were some discussion
there which I'm missing.
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
and for the record, my irc history tells me that Nikita sent an email about
http://lab.onsec.ru/2012/08/php-multiple-headers-bypass-available.html to
security@php.net back in 2012-08-24, so maybe there were some discussion
there which I'm missing.
I wasn't party to that discussion, but I think the issue was dismissed
as an IE bug at the time. While there's an IE bug involved, I think it
was wrong to dismiss the issue. Relevant tweets from 2012:
https://twitter.com/d0znpp/status/238778122160443392
Alexander
If I'm reading this correctly this would reintroduce
https://bugs.php.net/bug.php?id=60227
No.
those checks aren't there to support header splitting but to prevent them,
as "\r\n" isn't the only separator which will cause browsers to split the
header.
Individual '\r' or '\n' may also separate headers in specific browsers,
yes. We should continue to disallow them as well.
Adam's patch looks correct to me in this respect: strpbrk() returns
non-NULL when any of the characters is found.
Why did the old code special-case NUL, though? Should we possibly
preserve that?
Alexander
If I'm reading this correctly this would reintroduce
https://bugs.php.net/bug.php?id=60227No.
those checks aren't there to support header splitting but to prevent
them,
as "\r\n" isn't the only separator which will cause browsers to split the
header.Individual '\r' or '\n' may also separate headers in specific browsers,
yes. We should continue to disallow them as well.Adam's patch looks correct to me in this respect:
strpbrk()returns
non-NULL when any of the characters is found.
my bad, I blame morning.
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
Why did the old code special-case NUL, though? Should we possibly
preserve that?
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
Why did the old code special-case NUL, though? Should we possibly
preserve that?
Wow. So Adam's patch is in fact buggy in going back to strpbrk()
without also checking for NUL, whereas the NUL check in the code
currently in PHP is probably unnecessary (at least not for the original
reason). It's good that we're actually reviewing the patch this time,
and with more than two eyes even. Thank you!
I think it might be the simplest to use two memchr() calls in place of
strpbrk(), and not have any loop (unlike the old memchr()-using code
did) because we can reject on any '\r' or '\n' right away. Another good
option is to have a single loop that checks the individual chars and
aborts with failure if it sees a '\r' or '\n'. Either is clean enough.
As to whether we want to check for NUL just in case, even when our
implementation doesn't depend on that, I don't know. Some browsers may
surely be confused by a NUL, but probably not in a way allowing for
header injection. I imagine there could be e.g. header($unsafe .
"suffix") in some script, and $unsafe with a NUL in it would then hide
the suffix from some browsers. This would only be a security issue if
the suffix somehow restricts the meaning of that header. Maybe such
headers exist. So maybe continuing to check for NUL makes sense. It's
3 memchr()'s or 3 chars to check for in a loop, then. Well, or
strpbrk() for "\r\n" and a single memchr() for NUL, but that's more
complicated to review.
Alexander
Why did the old code special-case NUL, though? Should we possibly
preserve that?see
Wow. So Adam's patch is in fact buggy in going back to
strpbrk()
without also checking for NUL, whereas the NUL check in the code
currently in PHP is probably unnecessary (at least not for the original
reason). It's good that we're actually reviewing the patch this time,
and with more than two eyes even. Thank you!I think it might be the simplest to use two memchr() calls in place of
strpbrk(), and not have any loop (unlike the old memchr()-using code
did) because we can reject on any '\r' or '\n' right away. Another good
option is to have a single loop that checks the individual chars and
aborts with failure if it sees a '\r' or '\n'. Either is clean enough.
Or, check for NUL byte first and reject the whole thing if present; then
continue with strpbrk() :)
As to whether we want to check for NUL just in case, even when our
implementation doesn't depend on that, I don't know. Some browsers may
surely be confused by a NUL, but probably not in a way allowing for
header injection. I imagine there could be e.g. header($unsafe .
"suffix") in some script, and $unsafe with a NUL in it would then hide
the suffix from some browsers. This would only be a security issue if
the suffix somehow restricts the meaning of that header. Maybe such
headers exist. So maybe continuing to check for NUL makes sense. It's
3 memchr()'s or 3 chars to check for in a loop, then. Well, or
strpbrk()for "\r\n" and a single memchr() for NUL, but that's more
complicated to review.Alexander
--
--
Tjerk
Hi!
Or, check for NUL byte first and reject the whole thing if present; then
continue withstrpbrk():)
To check for nul, you need to scan the whole string. If you're already
doing this, you don't need strpbrk anymore.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
On Thu, Jul 3, 2014 at 4:24 PM, Stas Malyshev smalyshev@sugarcrm.com
wrote:
Hi!
Or, check for NUL byte first and reject the whole thing if present; then
continue withstrpbrk():)To check for nul, you need to scan the whole string. If you're already
doing this, you don't need strpbrk anymore.
Right, with that in mind we might as well keep the current loop and just
break at those three "bad" characters :)
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
--
Tjerk
On Thu, Jul 3, 2014 at 4:24 PM, Stas Malyshev smalyshev@sugarcrm.com
wrote:Hi!
Or, check for NUL byte first and reject the whole thing if present; then
continue withstrpbrk():)To check for nul, you need to scan the whole string. If you're already
doing this, you don't need strpbrk anymore.Right, with that in mind we might as well keep the current loop and just
break at those three "bad" characters :)
I've updated https://github.com/LawnGnome/php-src/compare/remove-multiline-headers?expand=1
to do just that. I also added a null byte test to the new
header_multiline.phpt test — I don't see a case in the bug60227 tests
that I'm removing that isn't now covered (all the other tests there
were related to multiline support, which has now been removed, so
there's little point retaining those tests).
I'm still thinking master and possibly 5.6 only for this. Does anyone
else have any thoughts (particularly Ferenc and/or Julien on the 5.6
front)?
Adam
Hi!
I'd tend to agree: if we're going to do this, let's just rip the
band-aid off completely. I've got a quick and dirty patch at
https://github.com/LawnGnome/php-src/compare/remove-multiline-headers?expand=1
that does this, and applies cleanly against every branch from 5.4 to
master.
This doesn't look correct - strpbrk will stop at nul bytes, but there's
no guarantee the actual SAPI would. Also no reason to delete so many
tests - some of them tests for other things than multiline headers too.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
Hi Stas,
Please drop multiline HTTP headers support from PHP
header()because it
was never needed in that layer, it is a security risk in combinationWhy it's not needed in that layer? If you want to send a multiline
header allowed by RFC 2616 (assuming you do want it, for undefined
reasons), how else you do that? That's the only way to send headers in
PHP as far as I can see.
As you say, "for undefined reasons". I am unaware of a good reason for
a PHP app to want to explicitly do that. Stretching my imagination, I'd
think a valid reason would be if someone were implementing an HTTP
client/proxy, and wanted to pass the received headers on to another HTTP
client unaltered (including even their protocol level representation).
I think PHP's header() function shouldn't be intended for such use,
especially as it doesn't guarantee there are no extra headers and that
the headers come in a particular order (so it's not "unaltered" anyway).
In other words, PHP header() is not a sufficiently low-level interface
for the existence of individual low-level features in it to matter.
I think it should be a medium-level interface (so to speak), providing
only the somewhat abstract functionality of "set this HTTP header to
this value", without exposing the aspect of how exactly that is done.
with a certain IE bug, IE didn't support such multiline response headers
properly anyway, and they are deprecated by RFC 7230:So IE violates the RFC by misparsing the multiline headers?
That's my current understanding, based on D0znpp's testing.
Alexander
Hi,
Please drop multiline HTTP headers support from PHP
header()because it
was never needed in that layer, it is a security risk in combination
with a certain IE bug, IE didn't support such multiline response headers
properly anyway, and they are deprecated by RFC 7230:https://twitter.com/d0znpp/status/483147480843186176
http://lab.onsec.ru/2012/08/php-multiple-headers-bypass-available.html
http://tools.ietf.org/html/rfc7230#section-3.2.4I brought this to Pierre Joye's attention on Twitter today, and he
agrees that "yes it should be removed" and asked me to "drop a mail to
internals". So I just did.Alexander
--
maybe I'm missing something here, but we don't really "support" multiline
headers with header() anymore since 5.1.2, but from time to time this issue
resurfaces, mostly because some browsers split header lines on other
characters (https://bugs.php.net/bug.php?id=60227 and
http://lab.onsec.ru/2012/08/php-multiple-headers-bypass-available.html)
than we originally assumed or what the RFC 2616 allows.
so I'm not sure how could we fix this other than a one-by-one basis when we
find another browser quirk like this.
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
maybe I'm missing something here,
I guess so.
but we don't really "support" multiline
headers withheader()anymore since 5.1.2,
If you mean bug 60227, then you're confusing things here. That bug was
about having multiple headers sent by header(). I am talking about
individual multiline headers.
but from time to time this issue
resurfaces, mostly because some browsers split header lines on other
characters (https://bugs.php.net/bug.php?id=60227 and
http://lab.onsec.ru/2012/08/php-multiple-headers-bypass-available.html)
than we originally assumed or what the RFC 2616 allows.
so I'm not sure how could we fix this other than a one-by-one basis when we
find another browser quirk like this.
I've seen bug 60227 before. We shouldn't reintroduce that bug, but we
should drop support for multiline headers. There's no contradiction.
Alexander
Hi,
Please drop multiline HTTP headers support from PHP
header()because it
was never needed in that layer, it is a security risk in combination
with a certain IE bug, IE didn't support such multiline response headers
properly anyway, and they are deprecated by RFC 7230:https://twitter.com/d0znpp/status/483147480843186176
http://lab.onsec.ru/2012/08/php-multiple-headers-bypass-available.html
http://tools.ietf.org/html/rfc7230#section-3.2.4I brought this to Pierre Joye's attention on Twitter today, and he
agrees that "yes it should be removed" and asked me to "drop a mail to
internals". So I just did.
I confirm and reiterate my +1 here
Thanks for bringing this topic back to internals.
Cheers,
Pierre