Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:57672
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.212.170 as permitted sender)
Message-ID: <4F2C6885.8080007@gmail.com>
Date: Sat, 04 Feb 2012 00:06:45 +0100
User-Agent: Thunderbird
MIME-Version: 1.0
To: Gustavo Lopes <glopes@nebm.ist.utl.pt>
CC: internals@lists.php.net
References: <D5EC076D-8518-4CDC-913F-5A3F3BB97367@sektioneins.de> <op.v83qfyebidpuyk@slws007.slhq.int> <op.v83vw0bcidpuyk@slws007.slhq.int> <4F2C4743.8070609@gmail.com>
In-Reply-To: <4F2C4743.8070609@gmail.com>
Content-Type: multipart/mixed;
 boundary="------------050304010104070504030503"
Subject: Re: [PHP-DEV] The case of HTTP response splitting protection in PHP
From: keisial@gmail.com (=?UTF-8?B?w4FuZ2VsIEdvbnrDoWxleg==?=)

--------------050304010104070504030503
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On 03/02/12 21:44, Ángel González wrote:
> On 03/02/12 15:01, Gustavo Lopes wrote:
>> I've committed a different version that also forbids \0 (since, as
>> Stefan says, a NUL byte can result in the truncation of the rest of
>> the header) and that accepts a CRLF:
>>
>> http://svn.php.net/viewvc/php/php-src/trunk/main/SAPI.c?r1=323043&r2=323042&pathrev=323043
>>
> Looks good. But given that the goal is to make this robust, I would go
> further:
> a) Replace any CRLF + [ \r] with SP
> (rfc2616 allows us "A recipient MAY replace any linear white space
> with a single SP before forwarding the message downstream.", and
> this also protects UAs not following the spec)
>
> b) Bail out on any header_line[i] < ' ' (ie. fail on any special char)
I've gone ahead and written code for that feature. Comments welcome.



--------------050304010104070504030503
Content-Type: text/x-patch;
 name="HTTP_Response_splitting2.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="HTTP_Response_splitting2.diff"

Index: main/SAPI.c
===================================================================
--- main/SAPI.c	(revision 323049)
+++ main/SAPI.c	(working copy)
@@ -710,27 +710,30 @@
 		efree(header_line);
 		return SUCCESS;
 	} else {
-		/* new line/NUL character safety check */
-		int i;
-		for (i = 0; i < header_line_len; i++) {
-			/* RFC 2616 allows new lines if followed by SP or HT */
-			int illegal_break =
-					(header_line[i+1] != ' ' && header_line[i+1] != '\t')
-					&& (
-						header_line[i] == '\n'
-						|| (header_line[i] == '\r' && header_line[i+1] != '\n'));
-			if (illegal_break) {
-				efree(header_line);
-				sapi_module.sapi_error(E_WARNING, "Header may not contain "
-						"more than a single header, new line detected");
-				return FAILURE;
+		/* Forbid new lines or control characters in the headers */
+		int i, j;
+		for (i = 0, j = 0; i < header_line_len; i++, j++) {
+			header[i] = header[j];
+			if (i < header_line_len - 2 &&
+				header_line[i] == '\r' && header_line[i+1] == '\n' &&
+				(header_line[i+2] == ' ' || header_line[i+2] == '\t')) {
+				
+				/* This is a line continuation. Collapse this little-known 
+				 * feature into a single SP, as allowed by RFC 2616
+				 */
+				header_line[i] = ' ';
+				j = i + 2;
 			}
-			if (header_line[i] == '\0') {
+			
+			if (header_line[i] < 32) {
 				efree(header_line);
-				sapi_module.sapi_error(E_WARNING, "Header may not contain NUL bytes");
+				sapi_module.sapi_error(E_WARNING, "header() accepts only a single header, "
+						"new lines and control characters are forbidden");
 				return FAILURE;
 			}
 		}
+		header_line[j] = '\0';
+		header_line_len = j;
 	}
 
 	sapi_header.header = header_line;

--------------050304010104070504030503--