security revert CVE-2016-2216 didn't work with HPE_UNEXPECTED_CONTENT_LENGTH

[hefangshi]

• Version: v4.4.0
• Platform: Darwin MacBook-Pro.local 15.3.0 Darwin Kernel Version 15.3.0: Thu Dec 10 18:40:58 PST 2015; root:xnu-3248.30.4~1/RELEASE_X86_64 x86_64

We encounter this issue when we upgrade node.js from v4.2.x to v4.3.x or v4.4.0.

A service we depends on will return both Transfer-Encoding and Content-Length in headers, and the node.js > 4.3.x will throw a HPE_UNEXPECTED_CONTENT_LENGTH error when we make a request with the service. and security revert CVE-2016-2216 also can't resolve this problem.

Also, according to RFC 2616

If a Content-Length header field (section 14.13) is present, its
decimal value in OCTETs represents both the entity-length and the
transfer-length. The Content-Length header field MUST NOT be sent
if these two lengths are different (i.e., if a Transfer-Encoding header field is present). If a message is received with both a
Transfer-Encoding header field and a Content-Length header field,
the latter MUST be ignored.

So I think Node.js should ignore the content-length when both header was given rather than throw a error.

Here is some code the reproduce this issue

const http = require('http');

const server = http.createServer((req, res) => {
    res.setHeader('Transfer-Encoding', 'chunked');
    res.setHeader('Content-Length', 344);
    res.end('ok');
}).listen(8085);

const options = {
    port: 8085,
    hostname: '127.0.0.1',
    method: 'GET',
};

const req = http.request(options);
req.on('error', function (err) {
    console.error(err);
});
req.end();

➜  ~ node --security-revert=CVE-2016-2216 index.js
SECURITY WARNING: Reverting CVE-2016-2216: Strict HTTP Header Parsing
{ [Error: Parse Error] bytesParsed: 123, code: 'HPE_UNEXPECTED_CONTENT_LENGTH' }

[MylesBorins]

/cc @nodejs/lts @nodejs/security

[indutny]

RFC 7230 obsoletes RFC 2616, and there is this statement:

If a message is received with both a Transfer-Encoding and a
       Content-Length header field, the Transfer-Encoding overrides the
       Content-Length.  Such a message might indicate an attempt to
       perform request smuggling (Section 9.5) or response splitting
       (Section 9.4) and ought to be handled as an error.  A sender MUST
       remove the received Content-Length field prior to forwarding such
       a message downstream.

Taking important parts out of it:

• Receiving both Transfer-Encoding and Content-Length is an error case, and should be handled like this
• Sending both Transfer-Encoding and Content-Length is a violation of protocol.

IMO, won't fix. Sorry!

[hefangshi]

I see..But still I think --security-revert=CVE-2016-2216 should be able to revert this break change in the original design.

[indutny]

I agree. cc @jasnell

[rvagg]

I think what you're asking for is --security-revert=CVE-2016-2086 as this is a separate issue to allowable characters which is what CVE-2016-2216 covered.

[jasnell]

I'm open to adding a revert for this CVE.
On Mar 17, 2016 9:32 PM, "Rod Vagg" notifications@github.com wrote:

I think what you're asking for is --security-revert=CVE-2016-2086 as this
is a separate issue to allowable characters which is what CVE-2016-2216
covered.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#5754 (comment)

[joepie91]

@hefangshi Has the duplicate header issue itself been reported to the service in question yet? They really shouldn't be sending these headers in the first place.

[hefangshi]

@joepie91 Sure I did, but I think this would be a common issue, so I posted here :)

[joepie91]

Alright, fair enough, just wanted to check :)

[jasnell]

I'll have to explore making this additional revert available. The
http-parser lib does not make it easy while maintaining ABI compatibility.
It's on my to-do list to make it easier but that'll require a more
significant change.
On Mar 23, 2016 6:42 PM, "Sven Slootweg" notifications@github.com wrote:

Alright, fair enough, just wanted to check :)

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#5754 (comment)

[jasnell]

Closing due to lack of forward progress on this