Skip to content

http_fopen_wrapper.c - Handle HTTP headers with varying white space #1902

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
IMSoP wants to merge 3 commits into from
Closed

http_fopen_wrapper.c - Handle HTTP headers with varying white space #1902

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6fbb2d5
http_fopen_wrapper.c - Handle HTTP headers with varying white space
IMSoP Mar 30, 2016
745c4e2
http_fopen_wrapper.c - Cleanup of previous HTTP whitespace patch
IMSoP May 10, 2016
73c9ebb
ext/standard/tests/http/bug47021.phpt remove unused var
IMSoP Oct 24, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 40 additions & 8 deletions ext/standard/http_fopen_wrapper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -740,8 +740,11 @@ php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper,

while (!body && !php_stream_eof(stream)) {
size_t http_header_line_length;

if (php_stream_get_line(stream, http_header_line, HTTP_HEADER_BLOCK_SIZE, &http_header_line_length) && *http_header_line != '\n' && *http_header_line != '\r') {
char *e = http_header_line + http_header_line_length - 1;
char* http_header_value;
size_t http_header_value_length;
if (*e != '\n') {
do { /* partial header */
if (php_stream_get_line(stream, http_header_line, HTTP_HEADER_BLOCK_SIZE, &http_header_line_length) == NULL) {
Expand All @@ -758,25 +761,54 @@ php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper,
http_header_line_length = e - http_header_line + 1;
http_header_line[http_header_line_length] = '\0';

if (!strncasecmp(http_header_line, "Location: ", 10)) {
/* The primary definition of an HTTP header in RFC 7230 states:
> Each header field consists of a case-insensitive field name followed
> by a colon (":"), optional leading whitespace, the field value, and
> optional trailing whitespace. */
http_header_value = http_header_line + 1;
while ( *http_header_value != ':' && http_header_value < http_header_line + http_header_line_length ) {
http_header_value++;
}
http_header_value++;
Copy link
Member

@nikic nikic May 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the header does not contain :, the while loop will advance to http_header_line + http_header_line_length. The subsequent increment will advance to http_header_line + http_header_line_length + 1. http_header_value_length will be (size_t)-1 in that case.

I did not ascertain what consequences this has, but it smells fishy.

Copy link
Contributor Author

@IMSoP IMSoP May 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I guess it ending up as -1 could be checked below as a flag of "didn't find sane value, don't attempt to use http_header_value".

/* If there is no : in the line, don't leave http_header_value as something bogus */
if ( http_header_value == http_header_line + http_header_line_length ) {
http_header_value_length = 0;
}
else {
while ( (*http_header_value == ' ' || *http_header_value == '\t') && http_header_value < http_header_line + http_header_line_length ) {
http_header_value++;
}
http_header_value_length = http_header_line - http_header_value + http_header_line_length;
e = http_header_value + http_header_value_length - 1;
while ( *e == ' ' || *e == '\t' ) {
e--;
http_header_value_length--;
}
http_header_value[http_header_value_length] = '\0';
}

if (!strncasecmp(http_header_line, "Location:", 9)) {
if (context && php_stream_context_get_option(context, "http", "follow_location", &tmpzval) == SUCCESS) {
SEPARATE_ZVAL(tmpzval);
convert_to_long_ex(tmpzval);
follow_location = Z_LVAL_PP(tmpzval);
} else if (!(response_code >= 300 && response_code < 304 || 307 == response_code || 308 == response_code)) {
} else if (!((response_code >= 300 && response_code < 304) || 307 == response_code || 308 == response_code)) {
/* we shouldn't redirect automatically
if follow_location isn't set and response_code not in (300, 301, 302, 303 and 307)
see http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.3.1
RFC 7238 defines 308: http://tools.ietf.org/html/rfc7238 */
follow_location = 0;
}
strlcpy(location, http_header_line + 10, sizeof(location));
} else if (!strncasecmp(http_header_line, "Content-Type: ", 14)) {
php_stream_notify_info(context, PHP_STREAM_NOTIFY_MIME_TYPE_IS, http_header_line + 14, 0);
} else if (!strncasecmp(http_header_line, "Content-Length: ", 16)) {
file_size = atoi(http_header_line + 16);
strlcpy(location, http_header_value, sizeof(location));
Copy link
Member

@nikic nikic May 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the code above adjusts the e pointer, strlcpy will still walk to the null byte, so this will still include the whitespace.

Copy link
Contributor Author

@IMSoP IMSoP May 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code now inserts a null terminator at the point where it found the end of the value, so the strlcpy now stops in the right place to remove trailing whitespace.

} else if (!strncasecmp(http_header_line, "Content-Type:", 13)) {
php_stream_notify_info(context, PHP_STREAM_NOTIFY_MIME_TYPE_IS, http_header_value, 0);
} else if (!strncasecmp(http_header_line, "Content-Length:", 15)) {
file_size = atoi(http_header_value);
php_stream_notify_file_size(context, file_size, http_header_line, 0);
} else if (!strncasecmp(http_header_line, "Transfer-Encoding: chunked", sizeof("Transfer-Encoding: chunked"))) {
} else if (
!strncasecmp(http_header_line, "Transfer-Encoding:", 18)
&& !strncasecmp(http_header_value, "Chunked", 7)
) {

/* create filter to decode response body */
if (!(options & STREAM_ONLY_GET_HEADERS)) {
Expand Down
93 changes: 93 additions & 0 deletions ext/standard/tests/http/bug47021.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
--TEST--
Bug #47021 (SoapClient stumbles over WSDL delivered with "Transfer-Encoding: chunked")
--INI--
allow_url_fopen=1
--SKIPIF--
<?php require 'server.inc'; http_server_skipif('tcp://127.0.0.1:12342'); ?>
--FILE--
<?php
require 'server.inc';

function stream_notification_callback($notification_code, $severity, $message, $message_code, $bytes_transferred, $bytes_max) {

switch($notification_code) {
case STREAM_NOTIFY_MIME_TYPE_IS:
echo "Type='$message'\n";
break;
case STREAM_NOTIFY_FILE_SIZE_IS:
echo "Size=$bytes_max\n";
break;
}
}

function do_test($num_spaces, $leave_trailing_space=false) {
// SOAPClient exhibits the bug because it forces HTTP/1.1,
// whereas file_get_contents() uses HTTP/1.0 by default.
$options = [
'http' => [
'protocol_version' => '1.1',
'header' => 'Connection: Close'
],
];

$ctx = stream_context_create($options);
stream_context_set_params($ctx, array("notification" => "stream_notification_callback"));

$spaces = str_repeat(' ', $num_spaces);
$trailing = ($leave_trailing_space ? ' ' : '');
$responses = [
"data://text/plain,HTTP/1.1 200 OK\r\n"
. "Content-Type:{$spaces}text/plain{$trailing}\r\n"
. "Transfer-Encoding:{$spaces}Chunked{$trailing}\r\n\r\n"
. "5\nHello\n0\n",
"data://text/plain,HTTP/1.1 200 OK\r\n"
. "Content-Type\r\n" // Deliberately invalid header
. "Content-Length:{$spaces}5{$trailing}\r\n\r\n"
. "World"
];
$pid = http_server('tcp://127.0.0.1:12342', $responses);

echo file_get_contents('http://127.0.0.1:12342/', false, $ctx);
echo "\n";
echo file_get_contents('http://127.0.0.1:12342/', false, $ctx);
echo "\n";

http_server_kill($pid);
}

// Chunked decoding should be recognised by the HTTP stream wrapper regardless of whitespace
// Transfer-Encoding:Chunked
do_test(0);
echo "\n";
// Transfer-Encoding: Chunked
do_test(1);
echo "\n";
// Transfer-Encoding: Chunked
do_test(2);
echo "\n";
// Trailing space at end of header
do_test(1, true);
echo "\n";

?>
--EXPECT--
Type='text/plain'
Hello
Size=5
World

Type='text/plain'
Hello
Size=5
World

Type='text/plain'
Hello
Size=5
World

Type='text/plain'
Hello
Size=5
World