Node 10.14.x Regression in parsing of binary upgrade response body

[shellscape]

• Version: 10.14.1
• Platform: MacOS 10.13.2
• Subsystem: (Docker > ubuntu:xenial)

It appears that the v10.14.x version range introduces a regression which was resolved by #17806. This gist can consistently reproduce the error: https://gist.github.com/shellscape/2020916f92be5d61f4d411fb6d2bce61

Note that stepping back down to any 10.x version lower than 10.14.0 does not reproduce the error. We've spun up brand new containers to verify the issue was isolated to 10.14.x. The same containers (config) from scratch with 10.13.x are completely fine. And have verified this on three different machines (both Windows and Mac OS), in different geographic locations (U.S., Brazil, New Zealand).

This was noticed with use of the websockets/ws module. The immediate error stack produced:

  RangeError: Invalid WebSocket frame: RSV1 must be clear

  Receiver.getInfo (node_modules/ws/lib/receiver.js:168:14)
  Receiver.startLoop (node_modules/ws/lib/receiver.js:121:22)
  Receiver._write (node_modules/ws/lib/receiver.js:69:10)
  Socket.socketOnData (node_modules/ws/lib/websocket.js:816:35)

And the subsequent stack produced is:

Error [ERR_STREAM_WRITE_AFTER_END]: write after end
      at writeAfterEnd (_stream_writable.js:243:12)
      at Socket.Writable.write (_stream_writable.js:291:5)
      at Sender.sendFrame (/node_modules/ws/lib/sender.js:404:20)
      at Sender.doClose (/node_modules/ws/lib/sender.js:141:10)
      at Sender.close (/node_modules/ws/lib/sender.js:128:12)
      at WebSocket.close (/node_modules/ws/lib/websocket.js:215:18)
      at Receiver.receiverOnConclude (/node_modules/ws/lib/websocket.js:695:32)
      at Receiver.emit (events.js:182:13)
      at Receiver.controlMessage (/node_modules/ws/lib/receiver.js:436:14)
      at Receiver.getData (/node_modules/ws/lib/receiver.js:330:42)

Both, when traced to the best of my ability, point back to the same problem as described in #17806.

[MylesBorins]

Managed to bisect with the provided reporo

commit a8532d4d23304d8cc28c968e2eda519a546834ca
Author: Matteo Collina <hello@matteocollina.com>
Date:   Tue Aug 21 17:26:51 2018 +0200

    deps,http: http_parser set max header size to 8KB

    CVE-2018-12121

    PR-URL: https://github.com/nodejs-private/node-private/pull/143
    Ref: https://github.com/nodejs-private/security/issues/139
    Ref: https://github.com/nodejs-private/http-parser-private/pull/2
    Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
    Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
    Reviewed-By: James M Snell <jasnell@gmail.com>
    Reviewed-By: Rod Vagg <rod@vagg.org>
    Reviewed-By: Anna Henningsen <anna@addaleax.net>

/cc @mcollina @rvagg

11.x is not suffering from the same failure, likely due to this additional change to the parser only in the 10.x, 8.x, 6.x version.

--- a/src/node_http_parser.cc
+++ b/src/node_http_parser.cc
@@ -599,6 +599,8 @@ class Parser : public AsyncWrap, public StreamListener {
     size_t nparsed =
       http_parser_execute(&parser_, &settings, data, len);

+    enum http_errno err = HTTP_PARSER_ERRNO(&parser_);
+
     Save();

     // Unassign the 'buffer_' variable
@@ -613,9 +615,7 @@ class Parser : public AsyncWrap, public StreamListener {
     Local<Integer> nparsed_obj = Integer::New(env()->isolate(), nparsed);
     // If there was a parse error in one of the callbacks
     // TODO(bnoordhuis) What if there is an error on EOF?
-    if (!parser_.upgrade && nparsed != len) {
-      enum http_errno err = HTTP_PARSER_ERRNO(&parser_);
-
+    if ((!parser_.upgrade && nparsed != len) || err != HPE_OK) {
       Local<Value> e = Exception::Error(env()->parse_error_string());
       Local<Object> obj = e->ToObject(env()->isolate()->GetCurrentContext())
         .ToLocalChecked();

[MylesBorins]

I can confirm that reverting the above indeed fixes this problem. I think we should delay today's release by at least 24 hours so that we can figure out if we can get a quick patch to this into 10.14.2

edit:

Also worth mentioning that all the tests continue to pass with the above patch reverted

[shellscape]

Thanks very much for jumping on that.

[MylesBorins]

As this regression effects multiple release lines and we don't have a clear fix I'm going to move forward with the 10.14.2 release and we can do a release for 6 / 8 / 10 when we have a fix

[rvagg]

I don't think I can be much help here. I was responsible for the port to 11.x but (a) I don't recall why those lines were removed and commit history in node-private aren't much help and (b) I don't even understand why they were in the originals which landed in <= 10.x. Commit history says that was part of @mcollina's original but given all the squashing, rebasing, amending during these releases I'm not inclined to trust any of this for provenance! It got a bit crazy.

Including @indutny who got involved here as well.

[rvagg]

Maybe it was left out because that was all refactored on 11/master anyway? See this section & discussion when landing llhttp: https://github.com/nodejs/node/pull/24059/files#r233653996

[mcollina]

I definitely remember this being needed when I did the fix as the error was not bubbling up correctly. I've tested this extensively on 10.x and it seems to not be needed for the fix to work, so I'm +1 in removing. I'm testing on v8.x and v6.x as well right now.

It would be very helpful to get a unit test in for this, so we won't regress in the future. Note that neither our tests or CITGM actually catched this one.

[mcollina]

Those lines can be reverted, they are not necessary in v6, v8 and v10.

[MylesBorins]

We should open a PR to each branch and so a release that removes these lines and introduces the flag.

…

On Wed, Dec 12, 2018, 1:45 PM Matteo Collina ***@***.*** wrote: Those lines can be reverted, they are not necessary in v6, v8 and v10. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#24958 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV5o_HxkYcW3kdt16XfI1SZOiQtMzks5u4U6vgaJpZM4ZNkU1> .

[MylesBorins]

@mcollina I see you have opened fixes against 6 / 8 / 10, is there a need for changes against 11 or master?

[mcollina]

no need for 11 or master, but it would be fantastic to have a regression test for this issue as I didn't write one.

[lpinca]

Closing this as fixes landed on all supported branches.

[shellscape]

@lpinca any word on the schedule for these fixes rolling out in a new version?

[lpinca]

It seems v6.16.0, v8.15.0, v10.15.0 include the fix.