http: fix http-parser regression #5237

jasnell · 2016-02-15T16:49:26Z

The new IS_HEADER_CHAR check in http-parser is improperly
checking char when it should be checking unsigned char.

/cc @ChALkeR

ChALkeR · 2016-02-15T17:12:03Z

Uh-oh. Is there any reason why ch is not unsigned?
The fix looks good. will test it in a few hours.

Previous discussion at 7bef1b7#commitcomment-16090330.

/cc @silentroach

silentroach · 2016-02-15T17:14:38Z

here is my pr with the same code :)

nodejs/http-parser#282

jasnell · 2016-02-15T17:17:46Z

@silentroach ... thank you for that and my apologies for the regression. once we get this reviewed we'll get it included into master, v4.3.1, and new v0.12/v0.10 releases as soon as possible. The v4.3.1 is scheduled for tomorrow.

silentroach · 2016-02-15T17:23:05Z

thank you :)

jasnell · 2016-02-15T17:25:00Z

in the meantime, you ought to be able to use the --security-revert=CVE-2016-2216 command line flag to temporarily revert the check causing this issue.

bnoordhuis · 2016-02-15T17:25:14Z

deps/http_parser/http_parser.c

@@ -1572,7 +1572,7 @@ size_t http_parser_execute (http_parser *parser,
            REEXECUTE();
          }

-          if (!lenient && !IS_HEADER_CHAR(ch)) {
+          if (!lenient && !IS_HEADER_CHAR((unsigned char)ch)) {


Cast inside the macro?

EDIT: Also, didn't I point this out during review? I remember there was a signed/unsigned issue.

It's possible. I likely just missed that particular comment. If so, my apologies. Unfortunately, it also appears we didn't have any tests looking for this.

No biggie. I just wondered if I missed this.

Fixes http-parser regression with IS_HEADER_CHAR check Add test case for obstext characters (> 0x80) is header

jasnell · 2016-02-15T17:42:51Z

@bnoordhuis @ChALkeR ... updated the commit.. this is a more appropriate fix. Needed to bump the http-parser version.

bnoordhuis · 2016-02-15T17:50:57Z

deps/http_parser/test.c

@@ -3356,7 +3356,7 @@ test_double_content_length_error (int req)

  parsed = http_parser_execute(&parser, &settings_null, buf, buflen);
  if (parsed != buflen) {
-    assert(HTTP_PARSER_ERRNO(&parser) == HPE_MULTIPLE_CONTENT_LENGTH);
+    assert(HTTP_PARSER_ERRNO(&parser) == HPE_UNEXPECTED_CONTENT_LENGTH);


Do I read right that HPE_MULTIPLE_CONTENT_LENGTH is not returned anywhere? How did this test pass before?

It's not even defined, it looks like. Does that mean the test didn't compile?

It didn't. This was caught after. We don't run the http-parser tests in our
main build so this snuck through. It was fixed upstream so making sure it's
fixed here too.
On Feb 15, 2016 9:51 AM, "Ben Noordhuis" [email protected] wrote:

In deps/http_parser/test.c
#5237 (comment):

@@ -3356,7 +3356,7 @@ test_double_content_length_error (int req)

parsed = http_parser_execute(&parser, &settings_null, buf, buflen);
if (parsed != buflen) {

assert(HTTP_PARSER_ERRNO(&parser) == HPE_MULTIPLE_CONTENT_LENGTH);

assert(HTTP_PARSER_ERRNO(&parser) == HPE_UNEXPECTED_CONTENT_LENGTH);

Do I read right that HPE_MULTIPLE_CONTENT_LENGTH is not returned
anywhere? How did this test pass before?

—
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/5237/files#r52927301.

@jasnell I think we need some help from @nodejs/build here to be able to run CI tests for both http parser and node.js with updated parser. I don't really like that we open PRs in this repo.

bnoordhuis · 2016-02-15T17:52:07Z

LGTM

MylesBorins · 2016-02-15T18:16:33Z

ci: https://ci.nodejs.org/job/node-test-pull-request/1669/

ChALkeR · 2016-02-15T18:19:41Z

LGTM

MylesBorins · 2016-02-15T18:20:12Z

Can people throw some LGTM on #5238
Need to get this backported and into 4.3.1

ChALkeR · 2016-02-15T19:32:27Z

@jasnell Just occured to me — perhaps the test should actually check that the server is receiving «DÃ¼sseldorf» as the header value?

ChALkeR · 2016-02-15T19:34:08Z

Btw, @silentroach, header value being «DÃ¼sseldorf», not «Düsseldorf», is expected.

jasnell · 2016-02-15T20:43:38Z

@ChALkeR ... that can come separately. It looks like the http module is still interpreting the input as UTF8 when the header value is actually received from the parser.

indutny · 2016-02-15T20:44:47Z

@jasnell no @nodejs/ctc or @nodejs/http mention here, so I missed it :(

indutny · 2016-02-15T20:45:04Z

deps/http_parser/http_parser.c

@@ -440,7 +440,7 @@ enum http_host_state
 * character or %x80-FF
 **/
 #define IS_HEADER_CHAR(ch)                                                     \
-  (ch == CR || ch == LF || ch == 9 || (ch > 31 && ch != 127))


Nit: (unsigned char) ch.

Fixes http-parser regression with IS_HEADER_CHAR check Add test case for obstext characters (> 0x80) is header PR-URL: #5237 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Myles Borins <[email protected]>

jasnell · 2016-02-15T20:46:26Z

Landed in 954a4b4

indutny · 2016-02-15T20:46:28Z

test/parallel/test-http-header-obstext.js

+server.listen(common.PORT, () => {
+  http.get({
+    port: common.PORT,
+    headers: {'Test': 'Düsseldorf'}


This should be added to http-parser repo instead.

jasnell · 2016-02-15T20:50:48Z

I will be adding a test there too @indutny. Given the regression I'm putting priority on getting changes into the main repo first so we can get the releases out. I'll be going back and getting fixes and expanded test cases into http-parser

Fixes http-parser regression with IS_HEADER_CHAR check Add test case for obstext characters (> 0x80) is header PR-URL: #5237 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Myles Borins <[email protected]>

* buffer: - You can now supply an encoding argument when filling a Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying an existing Buffer will also work with Buffer#fill(buffer[, start[, end]]). See the API documentation for details on how this works. (Trevor Norris) #4935 - Buffer#indexOf() no longer requires a byteOffset argument if you also wish to specify an encoding: Buffer#indexOf(val[, byteOffset][, encoding]). (Trevor Norris) #4803 * child_process: spawn() and spawnSync() now support a 'shell' option to allow for optional execution of the given command inside a shell. If set to true, cmd.exe will be used on Windows and /bin/sh elsewhere. A path to a custom shell can also be passed to override these defaults. On Windows, this option allows .bat. and .cmd files to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598 * http_parser: Update to http-parser 2.6.2 to fix an unintentionally strict limitation of allowable header characters. (James M Snell) #5237 * dgram: socket.send() now supports accepts an array of Buffers or Strings as the first argument. See the API docs for details on how this works. (Matteo Collina) #4374 * http: Fix a bug where handling headers will mistakenly trigger an 'upgrade' event where the server is just advertising its protocols. This bug can prevent HTTP clients from communicating with HTTP/2 enabled servers. (Fedor Indutny) #4337 * net: Added a listening Boolean property to net and http servers to indicate whether the server is listening for connections. (José Moreira) #4743 * node: The C++ node::MakeCallback() API is now reentrant and calling it from inside another MakeCallback() call no longer causes the nextTick queue or Promises microtask queue to be processed out of order. (Trevor Norris) #4507 * tls: Add a new tlsSocket.getProtocol() method to get the negotiated TLS protocol version of the current connection. (Brian White) #4995 * vm: Introduce new 'produceCachedData' and 'cachedData' options to new vm.Script() to interact with V8's code cache. When a new vm.Script object is created with the 'produceCachedData' set to true a Buffer with V8's code cache data will be produced and stored in cachedData property of the returned object. This data in turn may be supplied back to another vm.Script() object with a 'cachedData' option if the supplied source is the same. Successfully executing a script from cached data can speed up instantiation time. See the API docs for details. (Fedor Indutny) #4777 * performance: Improvements in: - process.nextTick() (Ruben Bridgewater) #5092 - path module (Brian White) #5123 - querystring module (Brian White) #5012 - streams module when processing small chunks (Matteo Collina) #4354

* buffer: - You can now supply an encoding argument when filling a Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying an existing Buffer will also work with Buffer#fill(buffer[, start[, end]]). See the API documentation for details on how this works. (Trevor Norris) #4935 - Buffer#indexOf() no longer requires a byteOffset argument if you also wish to specify an encoding: Buffer#indexOf(val[, byteOffset][, encoding]). (Trevor Norris) #4803 * child_process: spawn() and spawnSync() now support a 'shell' option to allow for optional execution of the given command inside a shell. If set to true, cmd.exe will be used on Windows and /bin/sh elsewhere. A path to a custom shell can also be passed to override these defaults. On Windows, this option allows .bat. and .cmd files to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598 * http_parser: Update to http-parser 2.6.2 to fix an unintentionally strict limitation of allowable header characters. (James M Snell) #5237 * dgram: socket.send() now supports accepts an array of Buffers or Strings as the first argument. See the API docs for details on how this works. (Matteo Collina) #4374 * http: Fix a bug where handling headers will mistakenly trigger an 'upgrade' event where the server is just advertising its protocols. This bug can prevent HTTP clients from communicating with HTTP/2 enabled servers. (Fedor Indutny) #4337 * net: Added a listening Boolean property to net and http servers to indicate whether the server is listening for connections. (José Moreira) #4743 * node: The C++ node::MakeCallback() API is now reentrant and calling it from inside another MakeCallback() call no longer causes the nextTick queue or Promises microtask queue to be processed out of order. (Trevor Norris) #4507 * tls: Add a new tlsSocket.getProtocol() method to get the negotiated TLS protocol version of the current connection. (Brian White) #4995 * vm: Introduce new 'produceCachedData' and 'cachedData' options to new vm.Script() to interact with V8's code cache. When a new vm.Script object is created with the 'produceCachedData' set to true a Buffer with V8's code cache data will be produced and stored in cachedData property of the returned object. This data in turn may be supplied back to another vm.Script() object with a 'cachedData' option if the supplied source is the same. Successfully executing a script from cached data can speed up instantiation time. See the API docs for details. (Fedor Indutny) #4777 * performance: Improvements in: - process.nextTick() (Ruben Bridgewater) #5092 - path module (Brian White) #5123 - querystring module (Brian White) #5012 - streams module when processing small chunks (Matteo Collina) #4354 PR-URL: #5295

freeall · 2016-02-23T15:57:48Z

I'm investigating a regression we had from v5.6.0 to v5.7.0 where the keystone module would sometimes crash, and I ended on this PR which may or may not relate to it. Sorry if the question is not suited here.

Why is it that https://github.com/nodejs/node is now using http-parser 2.6.2, when https://github.com/nodejs/http-parser is 2.6.1? Is there no relation between the two?

Fixes http-parser regression with IS_HEADER_CHAR check Add test case for obstext characters (> 0x80) is header PR-URL: nodejs#5237 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Myles Borins <[email protected]>

rvagg · 2016-02-24T02:57:27Z

There is a relation between the two, our security release process has kind of got things out of sync, I think @jasnell has an update for http-parser on his TODO list but contributions to http-parser should be made over there rather than here, in general.

* buffer: - You can now supply an encoding argument when filling a Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying an existing Buffer will also work with Buffer#fill(buffer[, start[, end]]). See the API documentation for details on how this works. (Trevor Norris) #4935 - Buffer#indexOf() no longer requires a byteOffset argument if you also wish to specify an encoding: Buffer#indexOf(val[, byteOffset][, encoding]). (Trevor Norris) #4803 * child_process: spawn() and spawnSync() now support a 'shell' option to allow for optional execution of the given command inside a shell. If set to true, cmd.exe will be used on Windows and /bin/sh elsewhere. A path to a custom shell can also be passed to override these defaults. On Windows, this option allows .bat. and .cmd files to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598 * http_parser: Update to http-parser 2.6.2 to fix an unintentionally strict limitation of allowable header characters. (James M Snell) #5237 * dgram: socket.send() now supports accepts an array of Buffers or Strings as the first argument. See the API docs for details on how this works. (Matteo Collina) #4374 * http: Fix a bug where handling headers will mistakenly trigger an 'upgrade' event where the server is just advertising its protocols. This bug can prevent HTTP clients from communicating with HTTP/2 enabled servers. (Fedor Indutny) #4337 * net: Added a listening Boolean property to net and http servers to indicate whether the server is listening for connections. (José Moreira) #4743 * node: The C++ node::MakeCallback() API is now reentrant and calling it from inside another MakeCallback() call no longer causes the nextTick queue or Promises microtask queue to be processed out of order. (Trevor Norris) #4507 * tls: Add a new tlsSocket.getProtocol() method to get the negotiated TLS protocol version of the current connection. (Brian White) #4995 * vm: Introduce new 'produceCachedData' and 'cachedData' options to new vm.Script() to interact with V8's code cache. When a new vm.Script object is created with the 'produceCachedData' set to true a Buffer with V8's code cache data will be produced and stored in cachedData property of the returned object. This data in turn may be supplied back to another vm.Script() object with a 'cachedData' option if the supplied source is the same. Successfully executing a script from cached data can speed up instantiation time. See the API docs for details. (Fedor Indutny) #4777 * performance: Improvements in: - process.nextTick() (Ruben Bridgewater) #5092 - path module (Brian White) #5123 - querystring module (Brian White) #5012 - streams module when processing small chunks (Matteo Collina) #4354 PR-URL: #5295

jasnell · 2016-02-24T06:23:45Z

Yep this got just a bit out of sync due to a bit of a perfect storm of
scheduling issues. I'm buried in a conference this week. This is first on
my to-do list when I'm back.
On Feb 23, 2016 6:57 PM, "Rod Vagg" [email protected] wrote:

There is a relation between the two, our security release process has kind
of got things out of sync, I think @jasnell https://github.com/jasnell
has an update for http-parser on his TODO list but contributions to
http-parser should be made over there rather than here, in general.

—
Reply to this email directly or view it on GitHub
#5237 (comment).

freeall · 2016-02-24T07:22:56Z

Thanks for the explanation. Always good to understand more of node core.

Fixes a header parsing bug for obstext characters (> 0x80) Adaption of nodejs/node@954a4b4b: Author: James M Snell <[email protected]> Date: Mon Feb 15 09:40:58 2016 -0800 deps: update to http-parser 2.6.2 Fixes http-parser regression with IS_HEADER_CHAR check Add test case for obstext characters (> 0x80) is header PR-URL: nodejs/node#5237 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Myles Borins <[email protected]>

Fixes a header parsing bug for obstext characters (> 0x80) Adaption of nodejs/node@954a4b4b: Author: James M Snell <[email protected]> Date: Mon Feb 15 09:40:58 2016 -0800 deps: update to http-parser 2.6.2 Fixes http-parser regression with IS_HEADER_CHAR check Add test case for obstext characters (> 0x80) is header PR-URL: nodejs/node#5237 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Myles Borins <[email protected]> PR-URL: #287 Reviewed-By: James M Snell <[email protected]>

Fixes a header parsing bug for obstext characters (> 0x80) Adaption of nodejs/node@954a4b4b: Author: James M Snell <[email protected]> Date: Mon Feb 15 09:40:58 2016 -0800 deps: update to http-parser 2.6.2 Fixes http-parser regression with IS_HEADER_CHAR check Add test case for obstext characters (> 0x80) is header PR-URL: nodejs/node#5237 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Myles Borins <[email protected]> PR-URL: nodejs#287 Reviewed-By: James M Snell <[email protected]>

jasnell removed the http Issues or PRs related to the http subsystem. label Feb 15, 2016

bnoordhuis reviewed Feb 15, 2016
View reviewed changes

deps: update to http-parser 2.6.2

0eabad7

Fixes http-parser regression with IS_HEADER_CHAR check Add test case for obstext characters (> 0x80) is header

jasnell force-pushed the fix-http-parser-regression branch from 3c0985a to 0eabad7 Compare February 15, 2016 17:42

bnoordhuis reviewed Feb 15, 2016
View reviewed changes

indutny reviewed Feb 15, 2016
View reviewed changes

jasnell closed this Feb 15, 2016

indutny reviewed Feb 15, 2016
View reviewed changes

ChALkeR added the http Issues or PRs related to the http subsystem. label Feb 16, 2016

igorklopov mentioned this pull request Feb 16, 2016

utf8 forbidden in headers since nodejs 4.3.0 jfhbrook/node-ecstatic#186

Closed

rvagg mentioned this pull request Feb 18, 2016

Release proposal: 5.7.0 (Stable) #5295

Merged

MylesBorins mentioned this pull request Mar 10, 2016

Audit commits not found on v4.x #5647

Closed

MylesBorins added the dont-land-on-v4.x label Mar 10, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: fix http-parser regression #5237

http: fix http-parser regression #5237

jasnell commented Feb 15, 2016

ChALkeR commented Feb 15, 2016

silentroach commented Feb 15, 2016

jasnell commented Feb 15, 2016

silentroach commented Feb 15, 2016

jasnell commented Feb 15, 2016

bnoordhuis Feb 15, 2016

jasnell Feb 15, 2016

bnoordhuis Feb 15, 2016

jasnell commented Feb 15, 2016

bnoordhuis Feb 15, 2016

bnoordhuis Feb 15, 2016

jasnell Feb 15, 2016

indutny Feb 15, 2016

bnoordhuis commented Feb 15, 2016

MylesBorins commented Feb 15, 2016

ChALkeR commented Feb 15, 2016

MylesBorins commented Feb 15, 2016

ChALkeR commented Feb 15, 2016

ChALkeR commented Feb 15, 2016

jasnell commented Feb 15, 2016

indutny commented Feb 15, 2016

indutny Feb 15, 2016

jasnell commented Feb 15, 2016

indutny Feb 15, 2016

jasnell commented Feb 15, 2016

freeall commented Feb 23, 2016

rvagg commented Feb 24, 2016

jasnell commented Feb 24, 2016

freeall commented Feb 24, 2016

http: fix http-parser regression #5237

http: fix http-parser regression #5237

Conversation

jasnell commented Feb 15, 2016

ChALkeR commented Feb 15, 2016

silentroach commented Feb 15, 2016

jasnell commented Feb 15, 2016

silentroach commented Feb 15, 2016

jasnell commented Feb 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Feb 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Feb 15, 2016

MylesBorins commented Feb 15, 2016

ChALkeR commented Feb 15, 2016

MylesBorins commented Feb 15, 2016

ChALkeR commented Feb 15, 2016

ChALkeR commented Feb 15, 2016

jasnell commented Feb 15, 2016

indutny commented Feb 15, 2016

Choose a reason for hiding this comment

jasnell commented Feb 15, 2016

Choose a reason for hiding this comment

jasnell commented Feb 15, 2016

freeall commented Feb 23, 2016

rvagg commented Feb 24, 2016

jasnell commented Feb 24, 2016

freeall commented Feb 24, 2016