Re: multiline HTTP headers support in header()

On Thu, Jul 3, 2014 at 2:37 PM, Solar Designer <solar@openwall.com> 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