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

Binary request header value results in dropped connection #3943

Closed
alaz opened this issue Apr 22, 2019 · 9 comments
Closed

Binary request header value results in dropped connection #3943

alaz opened this issue Apr 22, 2019 · 9 comments
Labels
awaiting more info

Comments

@alaz
Copy link

alaz commented Apr 22, 2019 •
edited
Loading

If Express.js receives a request header with binary data, it silently drops this connection. This is incorrect behavior, Web servers should accept any requests (including malformed) and reply with meaningful responses. This could be a security issue as well: if a load balancer sees too many dropped upstream connections, it will mark the upstream host as down. What if it marks all the hosts as down?

Steps to reproduce

I have set up a minimal project to show this issue: https://github.com/alaz/express-req-header-binary

The Express application is trivial:

const app = express();

app.get('/', (_req, res) => res.status(204).end());
app.listen(8080);

I have also an Nginx configuration that has two identical locations, both of them proxy to the same Express.js path. The only difference is that one of the Nginx locations sets a request header with binary value:

    location /no_header {
      proxy_pass http://localhost:8080/;
    }

    location /bin_header {
      proxy_set_header X-Binary-Header $binary_remote_addr;
      proxy_pass http://localhost:8080/;
    }

Tests

Alaz-Laptop:~ alaz$ curl -i http://localhost:8000/no_header
HTTP/1.1 204 No Content
Server: nginx/1.15.12
Date: Mon, 22 Apr 2019 08:51:54 GMT
Connection: keep-alive
X-Powered-By: Express

The above is a correct behavior. Below we are calling the location that sets binary data in a header:

Alaz-Laptop:~ alaz$ curl -i http://localhost:8000/bin_header
HTTP/1.1 502 Bad Gateway
Server: nginx/1.15.12
Date: Mon, 22 Apr 2019 08:51:59 GMT
Content-Type: text/html
Content-Length: 158
Connection: keep-alive

<html>
<head><title>502 Bad Gateway</title></head>
<body>
<center><h1>502 Bad Gateway</h1></center>
<hr><center>nginx/1.15.12</center>
</body>
</html>

and Nginx error.log will have:

2019/04/22 12:32:20 [error] 57078#0: *1 upstream prematurely closed connection while reading response header from upstream, client: 127.0.0.1, server: _, request: "GET /bin_header HTTP/1.1", upstream: "http://127.0.0.1:8080/", host: "localhost:8000"
2019/04/22 12:32:20 [warn] 57078#0: *1 upstream server temporarily disabled while reading response header from upstream, client: 127.0.0.1, server: _, request: "GET /bin_header HTTP/1.1", upstream: "http://127.0.0.1:8080/", host: "localhost:8000"
@dougwilson
Copy link
Contributor

What version of Express and Node.js are you running? This is likely an issue with Node.js HTTP server, but just need to know the version numbers to confirm.

@dougwilson
Copy link
Contributor

You can also attempt to confirm this if the following server exhibits the same behavior:

 const http = require('http')

const server = http.createServer((_req, res) => {
  res.statusCode = 204
  res.end()
})

server.listen(8080);

alaz added a commit to alaz/express-req-header-binary that referenced this issue Apr 22, 2019
@alaz
provide a test server for vanilla Node HTTP server
c3de5d9 
See the comments in expressjs/express#3943

The issue turned out to be bigger as the behavior is due to Node's HTTP
server.
@alaz
Copy link
Author

alaz commented Apr 22, 2019

My fault! The versions are –

$ node --version
v8.15.1

Express: express@4.16.4

Confirmed. The behavior is the same. I've updated the test repo.

@alaz
Copy link
Author

alaz commented Apr 22, 2019

This bug is fixed in Node 10 (I have 10.15.2):

$ curl -i http://localhost:8000/bin_header
HTTP/1.1 400 Bad Request
Server: nginx/1.15.12
Date: Mon, 22 Apr 2019 11:35:18 GMT
Transfer-Encoding: chunked
Connection: keep-alive

@dougwilson
Copy link
Contributor

Thanks for the versions and confirming. AFAIK this behavior was fixed in Node.js 9 and up, so upgrading Node.js should resolve.

@dougwilson
Copy link
Contributor

Ah just posted at the same time. Sounds like you confirmed your issue was resolved with that version upgrade 👍

@alaz
Copy link
Author

alaz commented Apr 22, 2019

Thanks! And sorry for bothering! ❤️

@dougwilson
Copy link
Contributor

It's no problem! In the future though if you believe it is a security issue, ideally it should be reported following https://github.com/expressjs/express/blob/master/Security.md

Node.js also has a special security report procedure if it did escalate to Node.js

@alaz
Copy link
Author

alaz commented Apr 22, 2019

Thanks for the hint, I shall bear it in mind.

@alaz alaz mentioned this issue May 1, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
awaiting more info
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@alaz @dougwilson