Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HTTP connection prematurely closed when unescaped space present in query string #13407

Closed
alexkwolfe opened this issue Jun 2, 2017 · 6 comments
Closed

HTTP connection prematurely closed when unescaped space present in query string #13407

alexkwolfe opened this issue Jun 2, 2017 · 6 comments
Labels
http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding.

Comments

@alexkwolfe
Copy link

alexkwolfe commented Jun 2, 2017 •
edited
Loading

  • Version: v6.10.3, v8.0.0
  • Platform: Darwin emerald.local 16.6.0 Darwin Kernel Version 16.6.0: Fri Apr 14 16:21:16 PDT 2017; root:xnu-3789.60.24~6/RELEASE_X86_64 x86_64
  • Subsystem: http

When handling an HTTP request with an unescaped space in the query string, the connection is prematurely closed.

Example

const http = require('http');

http.createServer((req, res) => {
  res.writeHead(200);
  res.write('ok');
  res.end();
}).listen(8080);
$ curl -v 'http://localhost:8080?foo=bar &baz=bip'
> GET /?foo=bar &baz=bip HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.51.0
> Accept: */*
> 
curl: (52) Empty reply from server
@alexkwolfe
Copy link
Author

alexkwolfe commented Jun 2, 2017 •
edited
Loading

Using this bug it is possible to craft a denial of service attack when running Node behind an NGINX reverse proxy. NGINX treats the closed connection as though the server has gone offline. When requests crafted in this way are sent in rapid succession, NGINX may start returning HTTP 504 to all callers because there are "no live upstreams" left to service the request.

The preferred behavior, in my opinion, would be to return an HTTP 400 instead of closing the connection. This seems to be the prevailing treatment on the interwebs.

$ curl -v 'https://www.google.com/?s=foo &bar=baz'
> GET /?s=foo &bar=baz HTTP/1.1
> Host: www.google.com
> User-Agent: curl/7.51.0
> Accept: */*
> 
< HTTP/1.0 400 Bad Request
< Content-Type: text/html; charset=UTF-8
< Referrer-Policy: no-referrer
< Content-Length: 1555
< Date: Fri, 02 Jun 2017 16:42:45 GMT
< 
<!DOCTYPE html>
<html lang=en>
  <meta charset=utf-8>
...

$ curl -v 'https://www.netflix.com?foo=bar &baz=qux' 
> GET /?foo=bar &baz=qux HTTP/1.1
> Host: www.netflix.com
> User-Agent: curl/7.51.0
> Accept: */*
> 
< HTTP/1.1 400 BAD_REQUEST
< Content-Length: 0
< Connection: Close
<

@mscdex mscdex added the http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. label Jun 2, 2017
@bnoordhuis
Copy link
Member

Node.js won't do it for you but you can send a 400 response if you listen for the 'clientError' event on the server instance. There is an example in the documentation.

@alexkwolfe
Copy link
Author

Oh nice! Thanks!

@alexkwolfe
Copy link
Author

Is this is the correct way to handle this case? Or is it worth leaving this issue open?

@bnoordhuis
Copy link
Member

We're currently discussing whether to change the default behavior from 'close connection' to 'send 400 response and close connection'. This issue can stay open until that discussion is resolved.

@bnoordhuis bnoordhuis mentioned this issue Sep 29, 2017
Closed
3 tasks
@apapirovski
Copy link
Contributor

This was resolved in #15324

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@alexkwolfe @apapirovski @mscdex @bnoordhuis