Skip to content

Commit

Permalink
http_parser: revert memchr() optimization
Browse files Browse the repository at this point in the history
An optimization was introduced in c6097e1 and 0097de5. The crux of
optimization was to skip all characters in header value until either
of CR or LF. Unfortunately, this optimization comes at cost of
inconsistency in header value validation, which might lead to security
issue due to violated expectations in the user code.

Partially revert the optimization, and add additional check to make
general header value parsing consistent.

Fix: nodejs#468
  • Loading branch information
indutny committed Apr 5, 2019
1 parent 0d0a24e commit 3068cea
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 19 deletions.
30 changes: 11 additions & 19 deletions http_parser.c
Original file line number Diff line number Diff line change
Expand Up @@ -1496,28 +1496,20 @@ size_t http_parser_execute (http_parser *parser,

switch (h_state) {
case h_general:
{
const char* p_cr;
const char* p_lf;
size_t limit = data + len - p;

limit = MIN(limit, max_header_size);

p_cr = (const char*) memchr(p, CR, limit);
p_lf = (const char*) memchr(p, LF, limit);
if (p_cr != NULL) {
if (p_lf != NULL && p_cr >= p_lf)
p = p_lf;
else
p = p_cr;
} else if (UNLIKELY(p_lf != NULL)) {
p = p_lf;
} else {
p = data + len;
for (; p != data + len; p++) {
ch = *p;
if (ch == CR || ch == LF) {
--p;
break;
}
if (!lenient && !IS_HEADER_CHAR(ch)) {
SET_ERRNO(HPE_INVALID_HEADER_TOKEN);
goto error;
}
}
if (p == data + len)
--p;
break;
}

case h_connection:
case h_transfer_encoding:
Expand Down
3 changes: 3 additions & 0 deletions test.c
Original file line number Diff line number Diff line change
Expand Up @@ -4316,6 +4316,9 @@ main (void)
test_simple("GET / HTTP/11.1\r\n\r\n", HPE_INVALID_VERSION);
test_simple("GET / HTTP/1.01\r\n\r\n", HPE_INVALID_VERSION);

test_simple("GET / HTTP/1.0\r\nHello: w\1rld\r\n\r\n", HPE_INVALID_HEADER_TOKEN);
test_simple("GET / HTTP/1.0\r\nHello: woooo\2rld\r\n\r\n", HPE_INVALID_HEADER_TOKEN);

// Extended characters - see nodejs/test/parallel/test-http-headers-obstext.js
test_simple("GET / HTTP/1.1\r\n"
"Test: Düsseldorf\r\n",
Expand Down

0 comments on commit 3068cea

Please sign in to comment.