rpms
/
http-parser
21
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity
You can not select more than 25 topics Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
Branches Tags
${ item.name }
Create tag ${ searchTerm }
Create branch ${ searchTerm }
from 'b285bac609'
${ noResults }
http-parser/SOURCES/CVE-2019-15605-HTTP-request...

451 lines
15 KiB
Raw Blame History

From b41d69bedcdbb8fe0cd790d0bcccbb457d6170d3 Mon Sep 17 00:00:00 2001
From: Sergio Correia <scorreia@redhat.com>
Date: Wed, 26 Feb 2020 17:03:26 -0300
Subject: [PATCH] CVE-2019-15605 - HTTP request smuggling
Upstream: https://github.com/nodejs/http-parser/commit/7d5c99d09f6743b055d53fc3f642746d9801479b
Support multi-coding Transfer-Encoding
`Transfer-Encoding` header might have multiple codings in it. Even
though llhttp cares only about `chunked`, it must check that `chunked`
is the last coding (if present).
ABNF from RFC 7230:
```
Transfer-Encoding = *( "," OWS ) transfer-coding *( OWS "," [ OWS
transfer-coding ] )
transfer-coding = "chunked" / "compress" / "deflate" / "gzip" /
transfer-extension
transfer-extension = token *( OWS ";" OWS transfer-parameter )
transfer-parameter = token BWS "=" BWS ( token / quoted-string )
```
However, if `chunked` is not last - llhttp must assume that the encoding
and size of the body is unknown (according to 3.3.3 of RFC 7230) and
read the response until EOF. For request - the error must be raised for
an unknown `Transfer-Encoding`.
Furthermore, 3.3.3 of RFC 7230 explicitly states that presence of both
`Transfer-Encoding` and `Content-Length` indicates the smuggling attack
and "ought to be handled as an error".
For the lenient mode:
* Unknown `Transfer-Encoding` in requests is not an error and request
body is simply read until EOF (end of connection)
* Only `Transfer-Encoding: chunked` together with `Content-Length` would
result an error (just like before the patch)
PR-URL: nodejs-private/http-parser-private#4
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
http_parser.c | 110 ++++++++++++++++++++++++++++++++++++++++++++------
http_parser.h | 8 ++--
test.c | 90 +++++++++++++++++++++++++++++++++++++++--
3 files changed, 189 insertions(+), 19 deletions(-)
diff --git a/http_parser.c b/http_parser.c
index aef4437..cd120d8 100644
--- a/http_parser.c
+++ b/http_parser.c
@@ -176,6 +176,22 @@ static const char *method_strings[] =
#undef XX
};
+/* Added for handling CVE-2019-15605. */
+static void reset_flags(http_parser* p)
+{
+ p->flags = 0;
+ p->transfer_encoding = 0;
+}
+
+static void set_transfer_encoding(http_parser* p)
+{
+ p->transfer_encoding = 1;
+}
+
+static int is_transfer_encoding(const http_parser* p)
+{
+ return p->transfer_encoding;
+}
/* Tokens as defined by rfc 2616. Also lowercases them.
* token = 1*<any CHAR except CTLs or separators>
@@ -378,6 +394,7 @@ enum header_states
, h_upgrade
, h_matching_transfer_encoding_chunked
+
, h_matching_connection_token_start
, h_matching_connection_keep_alive
, h_matching_connection_close
@@ -388,6 +405,10 @@ enum header_states
, h_connection_keep_alive
, h_connection_close
, h_connection_upgrade
+
+ /* CVE-2019-15605 */
+ , h_matching_transfer_encoding_token_start
+ , h_matching_transfer_encoding_token
};
enum http_host_state
@@ -722,7 +743,7 @@ reexecute:
{
if (ch == CR || ch == LF)
break;
- parser->flags = 0;
+ reset_flags(parser);
parser->content_length = ULLONG_MAX;
if (ch == 'H') {
@@ -757,7 +778,7 @@ reexecute:
case s_start_res:
{
- parser->flags = 0;
+ reset_flags(parser);
parser->content_length = ULLONG_MAX;
switch (ch) {
@@ -921,7 +942,7 @@ reexecute:
{
if (ch == CR || ch == LF)
break;
- parser->flags = 0;
+ reset_flags(parser);
parser->content_length = ULLONG_MAX;
if (UNLIKELY(!IS_ALPHA(ch))) {
@@ -1313,6 +1334,7 @@ reexecute:
parser->header_state = h_general;
} else if (parser->index == sizeof(TRANSFER_ENCODING)-2) {
parser->header_state = h_transfer_encoding;
+ set_transfer_encoding(parser);
}
break;
@@ -1393,10 +1415,14 @@ reexecute:
if ('c' == c) {
parser->header_state = h_matching_transfer_encoding_chunked;
} else {
- parser->header_state = h_general;
+ parser->header_state = h_matching_transfer_encoding_token;
}
break;
+ /* Multi-value `Transfer-Encoding` header */
+ case h_matching_transfer_encoding_token_start:
+ break;
+
case h_content_length:
if (UNLIKELY(!IS_NUM(ch))) {
SET_ERRNO(HPE_INVALID_CONTENT_LENGTH);
@@ -1539,16 +1565,41 @@ reexecute:
goto error;
/* Transfer-Encoding: chunked */
+ case h_matching_transfer_encoding_token_start:
+ /* looking for 'Transfer-Encoding: chunked' */
+ if ('c' == c) {
+ h_state = h_matching_transfer_encoding_chunked;
+ } else if (STRICT_TOKEN(c)) {
+ /* TODO(indutny): similar code below does this, but why?
+ * At the very least it seems to be inconsistent given that
+ * h_matching_transfer_encoding_token does not check for
+ * `STRICT_TOKEN`
+ */
+ h_state = h_matching_transfer_encoding_token;
+ } else if (c == ' ' || c == '\t') {
+ /* Skip lws */
+ } else {
+ h_state = h_general;
+ }
+ break;
+
case h_matching_transfer_encoding_chunked:
parser->index++;
if (parser->index > sizeof(CHUNKED)-1
|| c != CHUNKED[parser->index]) {
- h_state = h_general;
+ h_state = h_matching_transfer_encoding_token;
} else if (parser->index == sizeof(CHUNKED)-2) {
h_state = h_transfer_encoding_chunked;
}
break;
+ case h_matching_transfer_encoding_token:
+ if (ch == ',') {
+ h_state = h_matching_transfer_encoding_token_start;
+ parser->index = 0;
+ }
+ break;
+
case h_matching_connection_token_start:
/* looking for 'Connection: keep-alive' */
if (c == 'k') {
@@ -1607,7 +1658,7 @@ reexecute:
break;
case h_transfer_encoding_chunked:
- if (ch != ' ') h_state = h_general;
+ if (ch != ' ') h_state = h_matching_transfer_encoding_token;
break;
case h_connection_keep_alive:
@@ -1732,12 +1783,17 @@ reexecute:
REEXECUTE();
}
- /* Cannot use chunked encoding and a content-length header together
- per the HTTP specification. */
- if ((parser->flags & F_CHUNKED) &&
+ /* Cannot use transfer-encoding and a content-length header together
+ per the HTTP specification. (RFC 7230 Section 3.3.3) */
+ if ((is_transfer_encoding(parser)) &&
(parser->flags & F_CONTENTLENGTH)) {
- SET_ERRNO(HPE_UNEXPECTED_CONTENT_LENGTH);
- goto error;
+ /* Allow it for lenient parsing as long as `Transfer-Encoding` is
+ * not `chunked`
+ */
+ if (!lenient || (parser->flags & F_CHUNKED)) {
+ SET_ERRNO(HPE_UNEXPECTED_CONTENT_LENGTH);
+ goto error;
+ }
}
UPDATE_STATE(s_headers_done);
@@ -1811,8 +1867,31 @@ reexecute:
UPDATE_STATE(NEW_MESSAGE());
CALLBACK_NOTIFY(message_complete);
} else if (parser->flags & F_CHUNKED) {
- /* chunked encoding - ignore Content-Length header */
+ /* chunked encoding - ignore Content-Length header,
+ * prepare for a chunk */
UPDATE_STATE(s_chunk_size_start);
+ } else if (is_transfer_encoding(parser)) {
+ if (parser->type == HTTP_REQUEST && !lenient) {
+ /* RFC 7230 3.3.3 */
+
+ /* If a Transfer-Encoding header field
+ * is present in a request and the chunked transfer coding is not
+ * the final encoding, the message body length cannot be determined
+ * reliably; the server MUST respond with the 400 (Bad Request)
+ * status code and then close the connection.
+ */
+ SET_ERRNO(HPE_INVALID_TRANSFER_ENCODING);
+ RETURN(p - data); /* Error */
+ } else {
+ /* RFC 7230 3.3.3 */
+
+ /* If a Transfer-Encoding header field is present in a response and
+ * the chunked transfer coding is not the final encoding, the
+ * message body length is determined by reading the connection until
+ * it is closed by the server.
+ */
+ UPDATE_STATE(s_body_identity_eof);
+ }
} else {
if (parser->content_length == 0) {
/* Content-Length header given but zero: Content-Length: 0\r\n */
@@ -2064,6 +2143,12 @@ http_message_needs_eof (const http_parser *parser)
return 0;
}
+ /* RFC 7230 3.3.3, see `s_headers_almost_done` */
+ if ((is_transfer_encoding(parser)) &&
+ (parser->flags & F_CHUNKED) == 0) {
+ return 1;
+ }
+
if ((parser->flags & F_CHUNKED) || parser->content_length != ULLONG_MAX) {
return 0;
}
@@ -2107,6 +2192,7 @@ http_parser_init (http_parser *parser, enum http_parser_type t)
parser->type = t;
parser->state = (t == HTTP_REQUEST ? s_start_req : (t == HTTP_RESPONSE ? s_start_res : s_start_req_or_res));
parser->http_errno = HPE_OK;
+ reset_flags(parser);
}
void
diff --git a/http_parser.h b/http_parser.h
index ea7bafe..a4841be 100644
--- a/http_parser.h
+++ b/http_parser.h
@@ -275,8 +275,9 @@ enum flags
XX(INVALID_INTERNAL_STATE, "encountered unexpected internal state")\
XX(STRICT, "strict mode assertion failed") \
XX(PAUSED, "parser is paused") \
- XX(UNKNOWN, "an unknown error occurred")
-
+ XX(UNKNOWN, "an unknown error occurred") \
+ XX(INVALID_TRANSFER_ENCODING, \
+ "request has invalid transfer-encoding")
/* Define HPE_* values for each errno value above */
#define HTTP_ERRNO_GEN(n, s) HPE_##n,
@@ -293,7 +294,7 @@ enum http_errno {
struct http_parser {
/** PRIVATE **/
unsigned int type : 2; /* enum http_parser_type */
- unsigned int flags : 8; /* F_* values from 'flags' enum; semi-public */
+ unsigned int flags : 8; /* F_* values from 'flags' enum; semi-public */
unsigned int state : 7; /* enum state from http_parser.c */
unsigned int header_state : 7; /* enum header_state from http_parser.c */
unsigned int index : 7; /* index into current matcher */
@@ -318,6 +319,7 @@ struct http_parser {
/** PUBLIC **/
void *data; /* A pointer to get hook to the "connection" or "socket" object */
+ unsigned int transfer_encoding : 8; /* CVE-2019-15605 */
};
diff --git a/test.c b/test.c
index a1fa0d3..bb83d14 100644
--- a/test.c
+++ b/test.c
@@ -260,7 +260,6 @@ const struct message requests[] =
,.type= HTTP_REQUEST
,.raw= "POST /post_identity_body_world?q=search#hey HTTP/1.1\r\n"
"Accept: */*\r\n"
- "Transfer-Encoding: identity\r\n"
"Content-Length: 5\r\n"
"\r\n"
"World"
@@ -273,10 +272,9 @@ const struct message requests[] =
,.fragment= "hey"
,.request_path= "/post_identity_body_world"
,.request_url= "/post_identity_body_world?q=search#hey"
- ,.num_headers= 3
+ ,.num_headers= 2
,.headers=
{ { "Accept", "*/*" }
- , { "Transfer-Encoding", "identity" }
, { "Content-Length", "5" }
}
,.body= "World"
@@ -1172,6 +1170,61 @@ const struct message requests[] =
,.body= ""
}
+#define POST_MULTI_TE_LAST_CHUNKED 43
+, {.name= "post - multi coding transfer-encoding chunked body"
+ ,.type= HTTP_REQUEST
+ ,.raw= "POST / HTTP/1.1\r\n"
+ "Transfer-Encoding: deflate, chunked\r\n"
+ "\r\n"
+ "1e\r\nall your base are belong to us\r\n"
+ "0\r\n"
+ "\r\n"
+ ,.should_keep_alive= TRUE
+ ,.message_complete_on_eof= FALSE
+ ,.http_major= 1
+ ,.http_minor= 1
+ ,.method= HTTP_POST
+ ,.query_string= ""
+ ,.fragment= ""
+ ,.request_path= "/"
+ ,.request_url= "/"
+ ,.num_headers= 1
+ ,.headers=
+ { { "Transfer-Encoding" , "deflate, chunked" }
+ }
+ ,.body= "all your base are belong to us"
+ ,.num_chunks_complete= 2
+ ,.chunk_lengths= { 0x1e }
+ }
+
+#define POST_MULTI_LINE_TE_LAST_CHUNKED 43
+, {.name= "post - multi coding transfer-encoding chunked body"
+ ,.type= HTTP_REQUEST
+ ,.raw= "POST / HTTP/1.1\r\n"
+ "Transfer-Encoding: deflate,\r\n"
+ " chunked\r\n"
+ "\r\n"
+ "1e\r\nall your base are belong to us\r\n"
+ "0\r\n"
+ "\r\n"
+ ,.should_keep_alive= TRUE
+ ,.message_complete_on_eof= FALSE
+ ,.http_major= 1
+ ,.http_minor= 1
+ ,.method= HTTP_POST
+ ,.query_string= ""
+ ,.fragment= ""
+ ,.request_path= "/"
+ ,.request_url= "/"
+ ,.num_headers= 1
+ ,.headers=
+ { { "Transfer-Encoding" , "deflate, chunked" }
+ }
+ ,.body= "all your base are belong to us"
+ ,.num_chunks_complete= 2
+ ,.chunk_lengths= { 0x1e }
+ }
+
, {.name= NULL } /* sentinel */
};
@@ -1951,6 +2004,29 @@ const struct message responses[] =
,.chunk_lengths= { 2, 2 }
}
+#define HTTP_200_MULTI_TE_NOT_LAST_CHUNKED 28
+, {.name= "HTTP 200 response with `chunked` being *not last* Transfer-Encoding"
+ ,.type= HTTP_RESPONSE
+ ,.raw= "HTTP/1.1 200 OK\r\n"
+ "Transfer-Encoding: chunked, identity\r\n"
+ "\r\n"
+ "2\r\n"
+ "OK\r\n"
+ "0\r\n"
+ "\r\n"
+ ,.should_keep_alive= FALSE
+ ,.message_complete_on_eof= TRUE
+ ,.http_major= 1
+ ,.http_minor= 1
+ ,.status_code= 200
+ ,.response_status= "OK"
+ ,.num_headers= 1
+ ,.headers= { { "Transfer-Encoding", "chunked, identity" }
+ }
+ ,.body= "2\r\nOK\r\n0\r\n\r\n"
+ ,.num_chunks_complete= 0
+ }
+
, {.name= NULL } /* sentinel */
};
@@ -3629,7 +3705,7 @@ test_chunked_content_length_error (int req)
parsed = http_parser_execute(&parser, &settings_null, buf, strlen(buf));
assert(parsed == strlen(buf));
- buf = "Transfer-Encoding: chunked\r\nContent-Length: 1\r\n\r\n";
+ buf = "Transfer-Encoding: anything\r\nContent-Length: 1\r\n\r\n";
size_t buflen = strlen(buf);
parsed = http_parser_execute(&parser, &settings_null, buf, buflen);
@@ -4277,6 +4353,12 @@ main (void)
"fooba",
HPE_OK);
+ // Unknown Transfer-Encoding in request
+ test_simple("GET / HTTP/1.1\r\n"
+ "Transfer-Encoding: unknown\r\n"
+ "\r\n",
+ HPE_INVALID_TRANSFER_ENCODING);
+
static const char *all_methods[] = {
"DELETE",
"GET",
--
2.18.2
Reference in new issue View Git Blame Copy Permalink