Skip to content

fully consume chunked trailer and control lines in http_read_chunk #211

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
tanjiroK-coder wants to merge 1 commit into from
+203 −13
Closed

fully consume chunked trailer and control lines in http_read_chunk #211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
88 changes: 75 additions & 13 deletions cups/http.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,12 @@ static http_t *http_create(const char *host, int port, http_addrlist_t *addrlis
#ifdef DEBUG
static void http_debug_hex(const char *prefix, const char *buffer, int bytes);
#endif // DEBUG
static bool http_gets_chunk(http_t *http, char *line, size_t linesize);
static ssize_t http_read(http_t *http, char *buffer, size_t length, int timeout);
static ssize_t http_read_buffered(http_t *http, char *buffer, size_t length);
static ssize_t http_read_chunk(http_t *http, char *buffer, size_t length);
static bool http_send(http_t *http, http_state_t request, const char *uri);
static void http_skip_trailer(http_t *http);
static off_t http_set_length(http_t *http);
static void http_set_timeout(int fd, double timeout);
static void http_set_wait(http_t *http);
Expand Down Expand Up @@ -1543,7 +1545,7 @@ httpPeek(http_t *http, // I - HTTP connection
{
DEBUG_puts("2httpPeek: Getting chunk length...");

if (httpGets(http, len, sizeof(len)) == NULL)
if (!http_gets_chunk(http, len, sizeof(len)))
{
DEBUG_puts("1httpPeek: Could not get length!");
return (0);
Expand All @@ -1552,7 +1554,7 @@ httpPeek(http_t *http, // I - HTTP connection
if (!len[0])
{
DEBUG_puts("1httpPeek: Blank chunk length, trying again...");
if (!httpGets(http, len, sizeof(len)))
if (!http_gets_chunk(http, len, sizeof(len)))
{
DEBUG_puts("1httpPeek: Could not get chunk length.");
return (0);
Expand All @@ -1577,7 +1579,7 @@ httpPeek(http_t *http, // I - HTTP connection
http_content_coding_finish(http);

if (http->data_encoding == HTTP_ENCODING_CHUNKED)
httpGets(http, len, sizeof(len));
http_skip_trailer(http);

if (http->state == HTTP_STATE_LOCK_RECV || http->state == HTTP_STATE_POST_RECV || http->state == HTTP_STATE_PROPFIND_RECV || http->state == HTTP_STATE_PROPPATCH_RECV)
http->state ++;
Expand Down Expand Up @@ -1891,10 +1893,10 @@ httpRead(http_t *http, // I - HTTP connection

if (http->data_remaining <= 0 && http->data_encoding == HTTP_ENCODING_CHUNKED)
{
// Read the trailing blank line now...
// Consume the trailing line for this chunk now...
char len[32]; // Length string

httpGets(http, len, sizeof(len));
http_gets_chunk(http, len, sizeof(len));
}

bytes = 0;
Expand All @@ -1915,10 +1917,10 @@ httpRead(http_t *http, // I - HTTP connection

if (http->data_remaining <= 0)
{
// Read the trailing blank line now...
// Consume the trailing line for this chunk now...
char len[32]; // Length string

httpGets(http, len, sizeof(len));
http_gets_chunk(http, len, sizeof(len));
}
}
}
Expand All @@ -1940,10 +1942,10 @@ httpRead(http_t *http, // I - HTTP connection

if (http->data_remaining <= 0 && http->data_encoding == HTTP_ENCODING_CHUNKED)
{
// Read the trailing blank line now...
// Consume the trailing line for this chunk now...
char len[32]; // Length string

httpGets(http, len, sizeof(len));
http_gets_chunk(http, len, sizeof(len));
}
}
}
Expand Down Expand Up @@ -3878,7 +3880,7 @@ http_read_chunk(http_t *http, // I - HTTP connection
{
char len[32]; // Length string

if (!httpGets(http, len, sizeof(len)))
if (!http_gets_chunk(http, len, sizeof(len)))
{
DEBUG_puts("8http_read_chunk: Could not get chunk length.");
return (0);
Expand All @@ -3887,7 +3889,7 @@ http_read_chunk(http_t *http, // I - HTTP connection
if (!len[0])
{
DEBUG_puts("8http_read_chunk: Blank chunk length, trying again...");
if (!httpGets(http, len, sizeof(len)))
if (!http_gets_chunk(http, len, sizeof(len)))
{
DEBUG_puts("8http_read_chunk: Could not get chunk length.");
return (0);
Expand All @@ -3906,8 +3908,8 @@ http_read_chunk(http_t *http, // I - HTTP connection

if (http->data_remaining == 0)
{
// 0-length chunk, grab trailing blank line...
httpGets(http, len, sizeof(len));
// 0-length chunk, consume the trailer section...
http_skip_trailer(http);
}
}

Expand All @@ -3922,6 +3924,66 @@ http_read_chunk(http_t *http, // I - HTTP connection
}


//
// 'http_gets_chunk()' - Read a complete chunked transfer-coding control line.
//
// This reads one CR LF or LF terminated line used by the "chunked" transfer
// coding (a chunk-size line or a trailer field line), copying up to
// "linesize - 1" bytes into "line" and consuming (discarding) the rest of the
// line when it is longer than the buffer. httpGets() leaves the remainder of
// an over-long line in the read buffer; consuming the whole line here keeps a
// long chunk extension or trailer field from being misparsed as the start of
// the next message on a persistent connection.
//

static bool // O - `true` if a line was read, `false` on EOF/error
http_gets_chunk(http_t *http, // I - HTTP connection
char *line, // O - Line buffer
size_t linesize) // I - Size of line buffer
{
char discard[256]; // Overflow discard buffer


if (httpGets(http, line, linesize))
return (true);

// httpGets() returns NULL without setting http->error when the line is
// longer than the buffer, so keep reading (and discarding) until we consume
// the line terminator or hit a real EOF/error...
while (!http->error)
{
if (httpGets(http, discard, sizeof(discard)))
return (true);
}

return (false);
}


//
// 'http_skip_trailer()' - Consume the trailer section following a chunked body.
//
// After the final (zero-length) chunk, the "chunked" transfer coding allows an
// optional trailer section of header field lines terminated by an empty line.
// Read and discard the whole section so that no trailer bytes are left in the
// read buffer to be misparsed as the start of the next message on a persistent
// connection.
//

static void
http_skip_trailer(http_t *http) // I - HTTP connection
{
char line[256]; // Trailer line buffer


while (http_gets_chunk(http, line, sizeof(line)))
{
if (!line[0])
break;
}
}


//
// 'http_send()' - Send a request with all fields and the trailing blank line.
//
Expand Down
128 changes: 128 additions & 0 deletions cups/testhttp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ static const char * const base64_tests[][2] =
// Local functions...
//

static bool test_chunk_desync(void);
static bool test_date(time_t t);


Expand Down Expand Up @@ -482,6 +483,10 @@ main(int argc, // I - Number of command-line arguments
else
testEndMessage(true, "%s", buffer);

// Chunked transfer-coding trailer handling (request smuggling)
if (!test_chunk_desync())
failures ++;

return (failures);
}
else if (strstr(argv[1], "._tcp"))
Expand Down Expand Up @@ -942,3 +947,126 @@ test_date(time_t t) // I - Time in seconds since Jan 1, 1970
return (false);
}
}


//
// 'test_chunk_desync()' - Test that a chunked trailer section cannot smuggle a
// second request on a persistent connection.
//

static bool // O - `true` on success, `false` on failure
test_chunk_desync(void)
{
#ifdef _WIN32
// Raw socket plumbing below is POSIX-only; skip on Windows...
return (true);

#else
// The trailer section after the final chunk is longer than the small buffer
// used to consume chunk control lines. It must be fully consumed so its
// bytes are not left to be parsed as a second (smuggled) request; the only
// request after the body is the legitimately pipelined "GET /after".
static const char request[] =
"POST /ipp/print HTTP/1.1\r\n"
"Host: localhost\r\n"
"Transfer-Encoding: chunked\r\n"
"\r\n"
"5\r\nhello\r\n"
"0\r\n"
"X-Long-Trailer-Field-Padding: this-must-not-become-a-request\r\n"
"\r\n"
"GET /after HTTP/1.1\r\n"
"Host: localhost\r\n"
"\r\n";
int lfd = -1, // Listen socket
cfd = -1; // Client socket
http_t *http = NULL; // Server-side connection
http_addr_t addr; // Listen/connect address
socklen_t addrlen; // Address length
char uri[1024], // Request URI
body[1024]; // Request body
http_state_t state; // Request state
bool pass = true; // Test result


testBegin("httpRead chunked trailer (request smuggling)");

memset(&addr, 0, sizeof(addr));
addr.ipv4.sin_family = AF_INET;
addr.ipv4.sin_addr.s_addr = htonl(INADDR_LOOPBACK);

if ((lfd = socket(AF_INET, SOCK_STREAM, 0)) < 0 || bind(lfd, (struct sockaddr *)&addr, sizeof(addr.ipv4)) || listen(lfd, 1))
{
testEndMessage(false, "listen: %s", strerror(errno));
pass = false;
goto done;
}

addrlen = sizeof(addr.ipv4);
getsockname(lfd, (struct sockaddr *)&addr, &addrlen);

if ((cfd = socket(AF_INET, SOCK_STREAM, 0)) < 0 || connect(cfd, (struct sockaddr *)&addr, addrlen))
{
testEndMessage(false, "connect: %s", strerror(errno));
pass = false;
goto done;
}

if (write(cfd, request, sizeof(request) - 1) < 0)
{
testEndMessage(false, "write: %s", strerror(errno));
pass = false;
goto done;
}

shutdown(cfd, SHUT_WR);

if ((http = httpAcceptConnection(lfd, true)) == NULL)
{
testEndMessage(false, "httpAcceptConnection failed");
pass = false;
goto done;
}

// Read the chunked POST and its body...
while ((state = httpReadRequest(http, uri, sizeof(uri))) == HTTP_STATE_WAITING);

if (state != HTTP_STATE_POST)
{
testEndMessage(false, "request 1 was %s, expected POST", httpStateString(state));
pass = false;
goto done;
}

while (httpUpdate(http) == HTTP_STATUS_CONTINUE);
while (httpRead(http, body, sizeof(body)) > 0);

// Return to the waiting state with an empty response...
httpClearFields(http);
httpSetField(http, HTTP_FIELD_CONTENT_LENGTH, "0");
httpWriteResponse(http, HTTP_STATUS_OK);

// The next request must be the pipelined GET /after, not trailer bytes...
while ((state = httpReadRequest(http, uri, sizeof(uri))) == HTTP_STATE_WAITING);

if (state != HTTP_STATE_GET || strcmp(uri, "/after"))
{
testEndMessage(false, "request 2 was %s \"%s\", expected GET \"/after\"", httpStateString(state), uri);
pass = false;
goto done;
}

testEnd(true);

done:

if (http)
httpClose(http);
if (cfd >= 0)
close(cfd);
if (lfd >= 0)
close(lfd);

return (pass);
#endif // _WIN32
}
Loading