http: HPE_INVALID_CONSTANT parse error if CONNECT response contains body #7019

Open
bnoordhuis opened this Issue Feb 1, 2014 · 24 comments

Projects

None yet

5 participants

@bnoordhuis
Member

Test case:

var http = require('http');
var net = require('net');

net.createServer(function(conn) {
  var body = 'Proxy authentication required.\n';
  if (true) {
    // Generates HPE_INVALID_CONSTANT parse error on the client.
    conn.end('HTTP/1.1 407 Proxy Authentication Required\r\n' +
             'Content-Length: ' + body.length + '\r\n' +
             '\r\n' +
             body);
  } else {
    // Accepted even though it's malformed.
    conn.end('HTTP/1.1 407 Proxy Authentication Required\r\n' +
             'Content-Length: ' + body.length + '\r\n' +
             '\r\n');
  }
  this.close();
}).listen(0, function() {
  var port = this.address().port;
  http.get({
    method: 'CONNECT',
    path: 'example.org:80',
    port: port,
  }, function(res) {
    res.pipe(process.stdout);
  }).on('connect', function(res, socket, body) {
    res.pipe(process.stdout);
  }).on('error', function(err) {
    console.error('%s %j', err, err);
    process.exit(1);
  });
});

Fails with v0.10 and master in the same way:

$ v0.10/out/Release/node t.js
Error: Parse Error {"bytesParsed":66,"code":"HPE_INVALID_CONSTANT"}
$ master/out/Release/node t.js
Error: Parse Error {"bytesParsed":66,"code":"HPE_INVALID_CONSTANT"}

Probably requires fixing in both http_parser and node.

@indutny
Member
indutny commented Feb 1, 2014

Do you expect more meaningful error?

@bnoordhuis
Member

I expect no error at all. If the response from the proxy server is not 200 OK, it should be treated as a regular HTTP response.

@TooTallNate

You're specifying a content-length, but not actually sending the body! Of course the HTTP parser is going to be tripped up.

@TooTallNate

Oh sorry, nvm, didn't notice the top branch was different. Ya this seems like a bug in http_parser most likely.

@bnoordhuis
Member

You're specifying a content-length, but not actually sending the body!

Right, that's the secondary bug: the response from the false branch, even though it's malformed, is accepted. The primary bug is the one I care about, though. :-)

@trevnorris trevnorris added http master and removed master labels Feb 5, 2014
@bnoordhuis
Member

Any time frame on a fix? It would be nice to have it fixed sometime soon because CONNECT support is pretty unusable right now.

By the way, the test case talks about 4xx responses but the same bug happens with 200 responses; if they have a body, they fail to parse.

var assert = require('assert');
var http = require('http');
var net = require('net');

//var body = '';  // <- this works
var body = 'some random text';  // <- this doesn't
net.createServer(function(c) {
  c.write('HTTP/1.1 200 OK\r\n' +
          'Proxy-Agent: zuul\r\n' +
          '\r\n' +
          body);
}).listen(0, function() {
  this.unref();
  var req = http.request({
    host: this.address().address,
    port: this.address().port,
    method: 'CONNECT',
    path: 'example.org:80',
  });
  req.on('connect', function(res, socket, head) {
    assert.equal(head.toString(), body);
    socket.destroy();
  });
  req.on('error', function(err) {
    console.log(err);
    throw err;
  });
  req.end();
});
@indutny
Member
indutny commented Feb 14, 2014

tick tock tick tock. Going to look at it soon.

@bnoordhuis
Member

Nice. Thanks, Fedor.

@indutny
Member
indutny commented Feb 14, 2014

@bnoordhuis what do you think about:

diff --git a/deps/http_parser/http_parser.c b/deps/http_parser/http_parser.c
index 55d7716..f0b61dc 100644
--- a/deps/http_parser/http_parser.c
+++ b/deps/http_parser/http_parser.c
@@ -1636,6 +1636,14 @@ size_t http_parser_execute (http_parser *parser,
         }

         if (parser->flags & F_SKIPBODY) {
+          /*
+           * Request/response should not have body and
+           * therefore content-length too
+           */
+          if (parser->content_length != ULLONG_MAX) {
+            SET_ERRNO(HPE_INVALID_CONTENT_LENGTH);
+            goto error;
+          }
           parser->state = NEW_MESSAGE();
           CALLBACK_NOTIFY(message_complete);
         } else if (parser->flags & F_CHUNKED) {
@indutny
Member
indutny commented Feb 14, 2014

Seems to be breaking test/simple/test-http-head-request.js

@indutny
Member
indutny commented Feb 14, 2014

The only relevant section of RFC that I found is:

The presence of a message-body in a request is signaled by the inclusion of a Content-Length or Transfer-Encoding header field in the request's message-headers. A message-body MUST NOT be included in a request if the specification of the request method (section 5.1.1) does not allow sending an entity-body in requests. A server SHOULD read and forward a message-body on any request; if the request method does not include defined semantics for an entity-body, then the message-body SHOULD be ignored when handling the request.

I'm not sure if content-length is really prohibited in such responses after all. @bnoordhuis if it is allowed, then there are no bug with your test case.

@bnoordhuis
Member

Is that from RFC 2616? HTTP CONNECT is described in 2817. The relevant section is 5.3:

Any successful (2xx) response to a CONNECT request indicates that the
proxy has established a connection to the requested host and port,
and has switched to tunneling the current connection to that server
connection.

The RFC doesn't say it explicitly but that should be construed as that any non-2xx response means no tunnel has been established and both ends are still talking regular HTTP.

It raises an interesting point though: what to do when the response is 2xx but not 200? For example, what does 204 No Content mean in the context of a TCP tunnel?

@indutny
Member
indutny commented Feb 15, 2014

Wait, I'm just trying to figure out if Content-Length header is allowed in such responses, and if it is ignored if the response has no body. If this is the case - then current behavior is correct.

@bnoordhuis
Member

You mean in a non-2xx response? Yes, it should be treated as regular HTTP response. That it's a response to a CONNECT request doesn't factor into it.

@indutny
Member
indutny commented Feb 15, 2014

Omg, I'm talking about Content-Length, not response codes :)

@bnoordhuis
Member

Sorry, I'm not sure what the confusion is about. if it's a regular HTTP response (as is the case with the 407 response from the test case) then the normal rules apply and a Content-Length header is allowed.

@indutny
Member
indutny commented Feb 15, 2014

Ok, if it is allowed - then this is not a bug. Because everything after '\r\n' is treated as a separate response.

@bnoordhuis
Member

I'm afraid you've lost me... Let's try and step through it. If the server replies with this:

HTTP/1.1 407 Proxy Authentication Required
Content-Length: 31

Proxy authentication required.

Then why should what comes after the headers get interpreted as a new response? (Assuming that's what you're saying.)

@indutny
Member
indutny commented Feb 15, 2014

Then the problem is in _http_client.js:

  // Responses to CONNECT request is handled as Upgrade.
  if (req.method === 'CONNECT') {
    res.upgrade = true;
    return true; // skip body
  }
@bnoordhuis
Member

That's plausible. IIRC, http_parser has some special handling for CONNECT requests (and responses?) as well but my memory might be deceiving me here.

@indutny
Member
indutny commented Feb 16, 2014

No, it doesn't AFAIK

@indutny
Member
indutny commented Feb 16, 2014

Oh, it does the same thing as we do in js...

@jasnell
Member
jasnell commented May 28, 2015

@bnoordhuis @indutny ... how about this one? was this ever resolved?

@bnoordhuis
Member

No, still a bug.

@bnoordhuis bnoordhuis referenced this issue in nodejs/node May 20, 2016
Closed

http: avoid obscure http parser error on CONNECT. #6886

3 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment