added newline character check in url in response.redirect function

[alpercelik]

currently express framework do not check for newline characters
it is vulnerable to http splitting attack

example:
start the test server https://gist.github.com/1654555

browse http://localhost:3000/?foo=bar%0D%0ASet-Cookie%3Aname%3Dvalue
you will be re redirected to another page and cookie will be set

[tj]

if you're accepting user-input it should be filtered at that point. This issue would apply to any method setting a header field, even node's setHeader() itself, so if the user doesn't trust the input it should be sanitized or cause a client error response

[tj]

http://www.devthought.com/2011/12/22/a-string-is-not-an-error/

[alpercelik]

Thanks for pointing this bad pattern, I fixed that.

https://github.com/alpercelik/express/commit/f8bff5765474e35b5d3d54c3e7a56c3bc94cfd10

[alpercelik]

I completely agree that user-input should be checked this doesn't imply that user-input is not required.

I agree with you that this issue would apply to any method that may cause unintended header injection.
And I was also thinking where it should be handled within nodejs or express. I personally think checking it should be at least in express for now.

But looking at the redirect method, my opinion is that url parameter itself is the user-input (from the API point of view), and url parameter should be a valid url that can be redirected safely. This method passes url parameter to headers as it is and newline will definitively cause input to be injected to headers.

Example shown actually is a simple scenario which accepts the url parameter directly from request parameter. But considering applications that you have not accepted the url from "user" but may be coming from a database where it have passed user-input validations due to missing or insufficient controls in another entry-point, or being provided to your application as a return url.

I added the check right after the url where it is mapped. Here again the question was trustfulness of final url, since it may also be constructed via app.redirect mappings. It may be constructed using some data that comes from model which has such injection and not escaped.

In asp.net framework, redirect method of response class checks the url parameter if it contains any newline characters at first place.If it contains it throws exception and I followed the same approach.

I am not php/ruby guy but as I researched it seems that;

• In php this been fixed in version 5.1.2 (http://php.net/releases/5_1_2.php).
• In rails framework this has been reported and fixed removing CRLF in this commit (rails/rails@ba80ff74a962). Their approach and case was also similar (as mentioned in the comment "CGI.rb and mongrel don't do any sanitization of the contents of HTTP headers, so care needs to be taken"), as nodejs behaves same.

I am not sure which behavior fits best, throwing error or sanitizing, but personally I think there should be a prevention in express framework.

[tj]

I'm -1 for throwing because then nearly every res.redirect() call would need a try/catch. If we were to solve it lower in the stack it should be even lower than res.redirect since that still leaves everything else open. In general it's best practice to filter the input so you know it's "safe" when used throughout but can I see how this would be a big headache like the fs module + nul bytes, so I'll definitely think about it but the lowest we could go in Express is res.header() and not everyone uses that (vs node's res.setHeader())

[alpercelik]

I just started evaluating express and I liked it.

I have not gone through the details express, so I just tried to fix for redirect case. I'll close my pull request and would be happy to see a general and a better solution applied.

[tj]

if you're taking it to node core you can show them something similar to:

http.createServer(function(req, res){
  var body = 'oh noes';

  var userInput = 'foo\r\n'
    + 'Content-Length: ' + body.length + '\r\n'
    + '\r\n'
    + body;

  res.setHeader('Location', userInput);
  res.end();
}).listen(3000);

I cant remember if the fs nul byte thing got in or not but this is pretty similar

[tj]

@alpercelik https://github.com/visionmedia/node/compare/bug%2Fresponse-splitting

[alpercelik]

I had also fixed and pushed to my fork :) I was opening an issue with node and then received the notification :)

I did it it at the same location but I also checked for null bytes.
It is nice that you have also included test :)