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 1 commit
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
Next Next commit
http_fopen_wrapper.c - Handle HTTP headers with varying white space 
The stream handler assumed all HTTP headers contained exactly one space,
but the standard says there may be zero or more. Should fix Bug #47021,
and any other edge cases caused by a web server sending unusual spacing,
e.g. the MIME type discovered from Content-Type: can no longer contain
leading whitespace.
  • Loading branch information
@IMSoP
IMSoP committed May 9, 2016
commit 6fbb2d5d6e9382a7c0b44ade32e380f12f44f414
38 changes: 30 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 @@ -758,25 +758,47 @@ 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. */
char *http_header_value = http_header_line + 1;
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.

Declarations need to go at the top of the block in C89.

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".

while ( (*http_header_value == ' ' || *http_header_value == '\t') && http_header_value < http_header_line + http_header_line_length ) {
http_header_value++;
}
size_t 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--;
}

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
49 changes: 49 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,49 @@
--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 do_test($num_spaces) {
// 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);

$spaces = str_repeat(' ', $num_spaces);
$responses = [
"data://text/plain,HTTP/1.1 200 OK\r\nTransfer-Encoding:{$spaces}Chunked\r\n\r\n5\nHello\n0\n"
];
$pid = http_server('tcp://127.0.0.1:12342', $responses);

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

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";

?>
--EXPECT--
Hello
Hello
Hello