Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:75197 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 61683 invoked from network); 3 Jul 2014 06:37:19 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 3 Jul 2014 06:37:19 -0000 Authentication-Results: pb1.pair.com smtp.mail=solar@openwall.com; spf=pass; sender-id=pass Authentication-Results: pb1.pair.com header.from=solar@openwall.com; sender-id=pass Received-SPF: pass (pb1.pair.com: domain openwall.com designates 195.42.179.200 as permitted sender) X-PHP-List-Original-Sender: solar@openwall.com X-Host-Fingerprint: 195.42.179.200 mother.openwall.net Received: from [195.42.179.200] ([195.42.179.200:60810] helo=mother.openwall.net) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id 61/A4-47713-D1AF4B35 for ; Thu, 03 Jul 2014 02:37:18 -0400 Received: (qmail 16105 invoked from network); 3 Jul 2014 06:37:15 -0000 Received: from localhost (HELO pvt.openwall.com) (127.0.0.1) by localhost with SMTP; 3 Jul 2014 06:37:15 -0000 Received: by pvt.openwall.com (Postfix, from userid 503) id 73E84487DA; Thu, 3 Jul 2014 10:37:13 +0400 (MSK) Date: Thu, 3 Jul 2014 10:37:13 +0400 To: Ferenc Kovacs Cc: Adam Harvey , Andrea Faulds , Stas Malyshev , PHP internals , D0znpp Message-ID: <20140703063713.GA16777@openwall.com> References: <20140703003646.GA12662@openwall.com> <53B4AC6E.5050401@sugarcrm.com> <20140703012402.GA13015@openwall.com> <20140703060837.GB16494@openwall.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i Subject: Re: [PHP-DEV] multiline HTTP headers support in header() From: solar@openwall.com (Solar Designer) 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