Re: multiline HTTP headers support in header()

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