Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:75199
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.128.180 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <20140703063713.GA16777@openwall.com>
References: <20140703003646.GA12662@openwall.com>
	<53B4AC6E.5050401@sugarcrm.com>
	<F494C86C-44E9-4BBC-889F-ADEA38107612@ajf.me>
	<20140703012402.GA13015@openwall.com>
	<CADajX4CvRPVZ7P2UrEynXUJCSD2GCnMhP8zvy7+W29bnecTGGw@mail.gmail.com>
	<CAH-PCH5EbCz+80SyH1M8zqBR4WVqW4K_deDYbheZ5pkU1ch6HA@mail.gmail.com>
	<20140703060837.GB16494@openwall.com>
	<CAH-PCH4YVzU9wrsmY3XNLy=j3MmXfhmc+q6z5kXxy-y4OzTk_A@mail.gmail.com>
	<20140703063713.GA16777@openwall.com>
Date: Thu, 3 Jul 2014 15:03:53 +0800
Message-ID: <CAHMUw2HCwEZxtN5UsA6VRMpot05m-ZnTQE8jWbYMFLsEZW9muQ@mail.gmail.com>
To: Solar Designer <solar@openwall.com>
Cc: Ferenc Kovacs <tyra3l@gmail.com>, Adam Harvey <aharvey@php.net>, Andrea Faulds <ajf@ajf.me>, 
	Stas Malyshev <smalyshev@sugarcrm.com>, PHP internals <internals@lists.php.net>, 
	D0znpp <d0znpp@onsec.ru>
Content-Type: multipart/alternative; boundary=047d7b66f5fb96cb8e04fd449c7d
Subject: Re: [PHP-DEV] multiline HTTP headers support in header()
From: tjerk.meesters@gmail.com (Tjerk Meesters)

--047d7b66f5fb96cb8e04fd449c7d
Content-Type: text/plain; charset=UTF-8

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

--047d7b66f5fb96cb8e04fd449c7d--