FAQ
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

Search Discussions

  • Andrea Faulds at Jul 3, 2014 at 12:40 am

    On 3 Jul 2014, at 01:36, Solar Designer wrote:

    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/
  • Solar Designer at Jul 3, 2014 at 12:50 am

    On Thu, Jul 03, 2014 at 01:40:15AM +0100, Andrea Faulds wrote:
    On 3 Jul 2014, at 01:36, Solar Designer wrote:
    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
  • Stas Malyshev at Jul 3, 2014 at 1:05 am
    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/
  • Andrea Faulds at Jul 3, 2014 at 1:09 am

    On 3 Jul 2014, at 02:05, Stas Malyshev wrote:

    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/
  • Solar Designer at Jul 3, 2014 at 1:24 am

    On Thu, Jul 03, 2014 at 02:09:05AM +0100, Andrea Faulds wrote:
    On 3 Jul 2014, at 02:05, Stas Malyshev wrote:
    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
  • Adam Harvey at Jul 3, 2014 at 1:29 am

    On 2 July 2014 18:24, Solar Designer wrote:
    On Thu, Jul 03, 2014 at 02:09:05AM +0100, Andrea Faulds wrote:
    On 3 Jul 2014, at 02:05, Stas Malyshev wrote:
    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
  • Ferenc Kovacs at Jul 3, 2014 at 5:36 am

    On Thu, Jul 3, 2014 at 3:29 AM, Adam Harvey wrote:
    On 2 July 2014 18:24, Solar Designer wrote:
    On Thu, Jul 03, 2014 at 02:09:05AM +0100, Andrea Faulds wrote:
    On 3 Jul 2014, at 02:05, Stas Malyshev wrote:
    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

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
    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
  • Ferenc Kovacs at Jul 3, 2014 at 5:42 am

    On Thu, Jul 3, 2014 at 7:36 AM, Ferenc Kovacs wrote:

    On Thu, Jul 3, 2014 at 3:29 AM, Adam Harvey wrote:
    On 2 July 2014 18:24, Solar Designer wrote:
    On Thu, Jul 03, 2014 at 02:09:05AM +0100, Andrea Faulds wrote:
    On 3 Jul 2014, at 02:05, Stas Malyshev wrote:
    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

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
    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
  • Solar Designer at Jul 3, 2014 at 6:13 am

    On Thu, Jul 03, 2014 at 07:42:14AM +0200, Ferenc Kovacs wrote:
    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
  • Solar Designer at Jul 3, 2014 at 6:08 am

    On Thu, Jul 03, 2014 at 07:36:26AM +0200, Ferenc Kovacs wrote:
    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
  • Ferenc Kovacs at Jul 3, 2014 at 6:14 am

    On Thu, Jul 3, 2014 at 8:08 AM, Solar Designer wrote:
    On Thu, Jul 03, 2014 at 07:36:26AM +0200, Ferenc Kovacs wrote:
    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.
    my bad, I blame morning.

    --
    Ferenc Kovács
    @Tyr43l - http://tyrael.hu
  • Ferenc Kovacs at Jul 3, 2014 at 6:19 am


    Why did the old code special-case NUL, though? Should we possibly
    preserve that?
    see
    http://grokbase.com/t/php/php-internals/1223makrz1/the-case-of-http-response-splitting-protection-in-php



    --
    Ferenc Kovács
    @Tyr43l - http://tyrael.hu
  • Solar Designer at Jul 3, 2014 at 6:37 am

    On Thu, Jul 03, 2014 at 08:19:16AM +0200, Ferenc Kovacs wrote:
    Why did the old code special-case NUL, though? Should we possibly
    preserve that?
    see
    http://grokbase.com/t/php/php-internals/1223makrz1/the-case-of-http-response-splitting-protection-in-php
    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
  • Tjerk Meesters at Jul 3, 2014 at 7:03 am

    On Thu, Jul 3, 2014 at 2:37 PM, Solar Designer wrote:
    On Thu, Jul 03, 2014 at 08:19:16AM +0200, Ferenc Kovacs wrote:
    Why did the old code special-case NUL, though? Should we possibly
    preserve that?
    see
    http://grokbase.com/t/php/php-internals/1223makrz1/the-case-of-http-response-splitting-protection-in-php

    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

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

    --
    --
    Tjerk
  • Stas Malyshev at Jul 3, 2014 at 8:24 am
    Hi!
    Or, check for NUL byte first and reject the whole thing if present; then
    continue with `strpbrk()` :)
    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/
  • Tjerk Meesters at Jul 3, 2014 at 10:11 am

    On Thu, Jul 3, 2014 at 4:24 PM, Stas Malyshev wrote:

    Hi!
    Or, check for NUL byte first and reject the whole thing if present; then
    continue with `strpbrk()` :)
    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
  • Adam Harvey at Jul 3, 2014 at 5:38 pm

    On 3 July 2014 03:11, Tjerk Meesters wrote:
    On Thu, Jul 3, 2014 at 4:24 PM, Stas Malyshev wrote:

    Hi!
    Or, check for NUL byte first and reject the whole thing if present; then
    continue with `strpbrk()` :)
    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
  • Solar Designer at Jul 3, 2014 at 5:54 pm

    On Thu, Jul 03, 2014 at 10:37:41AM -0700, Adam Harvey wrote:
    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).
    It could make sense to keep the multiline header tests, with the new
    expected results, to make sure they'll fail if the support somehow gets
    reintroduced. %-)

    Alexander
  • Adam Harvey at Jul 3, 2014 at 5:56 pm

    On 3 July 2014 10:54, Solar Designer wrote:
    On Thu, Jul 03, 2014 at 10:37:41AM -0700, Adam Harvey wrote:
    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).
    It could make sense to keep the multiline header tests, with the new
    expected results, to make sure they'll fail if the support somehow gets
    reintroduced. %-)
    Fair, but that _is_ covered by the new test. :)

    Adam
  • Solar Designer at Jul 3, 2014 at 6:02 pm

    On Thu, Jul 03, 2014 at 10:56:03AM -0700, Adam Harvey wrote:
    On 3 July 2014 10:54, Solar Designer wrote:
    On Thu, Jul 03, 2014 at 10:37:41AM -0700, Adam Harvey wrote:
    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).
    It could make sense to keep the multiline header tests, with the new
    expected results, to make sure they'll fail if the support somehow gets
    reintroduced. %-)
    ... and here's how this may get reintroduced: if someone upgrades their
    PHP source tree by applying a patch, and a relevant hunk fails to apply.
    Then if the tests are nevertheless updated, they'll catch that.
    Fair, but that _is_ covered by the new test. :)
    I don't see it among your tests currently at the URL above. You have
    tests for multiple headers in one header() call, but not for multiline
    headers. In other words, you're no longer testing what happens in the
    special case when a CR or/and LF is followed by a space or TAB.

    Here they are:

    https://github.com/LawnGnome/php-src/blob/cc332df0c818596ab375a1c6b1d859eda043d0b9/ext/standard/tests/general_functions/header_multiline.phpt

    --TEST--
    header() cannot generate multiline headers
    --FILE--
    <?php
    ob_start();
    header("X-Foo1: bar");
    header("X-Foo2: quux\nquux");
    header("X-Foo3: quux\rquux");
    header("X-Foo4: quux\r\nquux");
    header("X-Foo5: quux\0quux");
    ?>
    --EXPECTF--
    Warning: Header may not contain multiple lines in %s on line %d

    Warning: Header may not contain multiple lines in %s on line %d

    Warning: Header may not contain multiple lines in %s on line %d

    Warning: Header may not contain NUL bytes in %s on line %d
    --EXPECTHEADERS--
    X-Foo1: bar

    Alexander
  • Adam Harvey at Jul 3, 2014 at 6:14 pm

    On 3 July 2014 11:02, Solar Designer wrote:
    On Thu, Jul 03, 2014 at 10:56:03AM -0700, Adam Harvey wrote:
    On 3 July 2014 10:54, Solar Designer wrote:
    On Thu, Jul 03, 2014 at 10:37:41AM -0700, Adam Harvey wrote:
    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).
    It could make sense to keep the multiline header tests, with the new
    expected results, to make sure they'll fail if the support somehow gets
    reintroduced. %-)
    ... and here's how this may get reintroduced: if someone upgrades their
    PHP source tree by applying a patch, and a relevant hunk fails to apply.
    Then if the tests are nevertheless updated, they'll catch that.
    I'm struggling to imagine that happening in a Git world, but sure.
    Fair, but that _is_ covered by the new test. :)
    I don't see it among your tests currently at the URL above. You have
    tests for multiple headers in one header() call, but not for multiline
    headers. In other words, you're no longer testing what happens in the
    special case when a CR or/and LF is followed by a space or TAB.
    I don't think it's terribly useful to do so, but I've added those
    scenarios to the test.

    Adam, who is apparently insufficiently paranoid.
  • Stas Malyshev at Jul 3, 2014 at 6:45 am
    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/
  • Solar Designer at Jul 3, 2014 at 1:20 am
    Hi Stas,
    On Wed, Jul 02, 2014 at 06:05:50PM -0700, Stas Malyshev wrote:
    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.
    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
  • Ferenc Kovacs at Jul 3, 2014 at 5:33 am

    On Thu, Jul 3, 2014 at 2:36 AM, Solar Designer wrote:

    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

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
    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
  • Solar Designer at Jul 3, 2014 at 6:05 am

    On Thu, Jul 03, 2014 at 07:33:49AM +0200, Ferenc Kovacs wrote:
    maybe I'm missing something here,
    I guess so.
    but we don't really "support" multiline
    headers with header() 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
  • Pierre Joye at Jul 3, 2014 at 6:33 am

    On Jul 3, 2014 2:37 AM, "Solar Designer" wrote:
    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.
    I confirm and reiterate my +1 here

    Thanks for bringing this topic back to internals.

    Cheers,
    Pierre

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupphp-internals @
categoriesphp
postedJul 3, '14 at 12:36a
activeJul 3, '14 at 6:14p
posts27
users7
websitephp.net

People

Translate

site design / logo © 2017 Grokbase