http: sanitize http headers and method names

[jasnell]

Simple test case:

var http = require('http');
var server = http.createServer(function(req, res) {
    res.setHeader('Foo\n\nBar', 'Value');
    res.end();
});
server.listen(3000, function() {
  console.log('server started');
});

Then do a curl -v http://localhost:3000

This is not a new issue (nodejs/node-v0.x-archive#2602, nodejs/node-v0.x-archive#9076). It affects HTTP request methods and header names. It happens in v0.10, v0.12, and current io.js:

var http = require('http');
var server = http.createServer(function(req,res) {
  res.writeHead(200, {'Foo\n\nBar': 'Value'});
  res.end('okay');
});
server.listen(3000, function() { console.log('started'); });

I have the code written up to perform the necessary fix but I need a decision: when an invalid http method or header name is passed in, should we throw or attempt to sanitize? Throwing is a API change that breaks backwards compatibility but this is an obvious error with potential security ramifications (fairly low risk). Sanitizing can lead to unexpected results and there is no standard pattern we can follow. Throwing seems to be the best approach.

// http token is defined in RFC2616 as...
//   token          = 1*<any CHAR except CTLs or separators>
//   separators     = "(" | ")" | "<" | ">" | "@"
//                  | "," | ";" | ":" | "\" | <">
//                  | "/" | "[" | "]" | "?" | "="
//                  | "{" | "}" | SP | HT
// and redefined in RFC7230 as ...
//   tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." /
//     "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA
//   token = 1*tchar
//
// This checks the input to make sure it's valid according to the
// http token rule. If the input doesn't match, it throws, if it
// does match, val is returned.
function checkIsHttpToken(val, label) {
  if (!val || typeof val !== 'string')
    return false;
  for (var n = 0, l = val.length; n < l; n++) {
    var c = val.charCodeAt(n);
    if (c <= 32 || c == 34 ||
        c == 44 || c == 47 ||
        c == 123 || c == 125 ||
        c == 40 || c == 41 ||
        (c >= 58 && c <= 64) ||
        (c >= 91 && c <= 93) ||
        c >= 128)
      throw new Error(
          'Invalid character (' +
          String.fromCharCode(c) +
          ') in HTTP ' + label + ' at position ' + (n + 1));
  }
  return val;
}

@nodejs/tsc ... thoughts?

[Fishrock123]

I'd be curious if this will impact perf depending on where it is.

What's the summary of what this fixes, just people setting weird headers? Does that actually produce any thing other than invalid sent headers? Are people just using the API wrong?

[jasnell]

Well, the key impact is that it would allow header injection through malformed header and method name inputs. It's a relatively low risk security and conformance nit really. Not a huge priority but worth fixing if we can. As you can see from the checkIsHttpToken method above, the "fix" is really just to scan the method and header names for invalid characters. If we find them, we can throw or strip. The performance hit should be fairly minimal depending on how many headers we have to scan and how long the names are. These are small strings. We could use a regex instead of a loop but the performance hit is pretty much the same in either case.

[jasnell]

@Fishrock123 ... one possible approach on this is to put the additional checking behind a flag or option. Running with the flag set would essentially put node into a strict conformance checking mode that can be used to identify errors like this, but can be left switched off in production to avoid the additional perf hit.

[brendanashworth]

@jasnell I recall discussion related to an http "strict mode" before somewhere in io.js#1457.

I'm -1 on sanitizing -- a developer should definitely do that before writing user-provided strings in headers -- and unsure about throwing. Also curious about perf though -- will this also affect the value of the header and not just the name? I don't recall us already iterating through the header strings themselves. This is also relevant to iojs#1693. Should we instead trust the developer (also allowing them to use whichever encoding they want)?

[rvagg]

@jasnell I'll leave you to close this one but it seems like this should be moved to nodejs/node now right?

[jasnell]

Yes, I'll handle this one. It hasn't been fixed yet but needs to be.
On Aug 24, 2015 12:31 AM, "Rod Vagg" [email protected] wrote:

@jasnell https://github.com/jasnell I'll leave you to close this one
but it seems like this should be moved to nodejs/node now right?

—
Reply to this email directly or view it on GitHub
#13 (comment)
.

[jasnell]

Ok, new PR opened. Will continue to track the conversation there. Closing this issue here. This does not hold up v4.0.0 at all.