Skip to content

Type confusion bug in HTTP parser #12178

Closed
Closed
Type confusion bug in HTTP parser#12178
Labels
c++Issues and PRs that require attention from people who are familiar with C++.http_parserIssues and PRs related to the HTTP parser implementation or http_parser binding.

Description

@deian
deian
opened on Apr 3, 2017

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}/`);
});

Activity

  1. mscdex
    added
    c++Issues and PRs that require attention from people who are familiar with C++.
    http_parserIssues and PRs related to the HTTP parser implementation or http_parser binding.
    on Apr 3, 2017

  2. gireeshpunathil commented on Apr 7, 2017

    @gireeshpunathil
    gireeshpunathil
    on Apr 7, 2017
    Member

    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

  3. gireeshpunathil commented on Apr 8, 2017

    @gireeshpunathil
    gireeshpunathil
    on Apr 8, 2017
    Member

    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

  4. deian commented on Apr 9, 2017

    @deian
    deian
    on Apr 9, 2017
    MemberAuthor

    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.

  5. gireeshpunathil commented on Apr 9, 2017

    @gireeshpunathil
    gireeshpunathil
    on Apr 9, 2017
    Member

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

  6. refack
    added a commit that references this issue on Jun 5, 2017

    http: assert parser.consume argument's type

    efab784
  7. refack
    closed this as completedin efab784on Jun 5, 2017
  8. jasnell
    added a commit that references this issue on Jun 7, 2017

    http: assert parser.consume argument's type

    afe91ec
  9. MylesBorins
    added a commit that references this issue on Sep 19, 2017

    http: assert parser.consume argument's type

    e65c9ec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    c++Issues and PRs that require attention from people who are familiar with C++.http_parserIssues and PRs related to the HTTP parser implementation or http_parser binding.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions