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.

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

Timeout to prevent slow sending attack #389

Closed
underflow00 opened this issue Feb 18, 2020 · 10 comments
Closed

Timeout to prevent slow sending attack #389

underflow00 opened this issue Feb 18, 2020 · 10 comments
Labels

Comments

@underflow00
Copy link

Is there a timeout feature that prevents a malicious client from slowly sending 1 byte at a time to prolong the request?

@dougwilson
Copy link
Contributor

Yes, it is part of Node.js itself, which is why it is not implemented again in this module: https://nodejs.org/dist/latest-v13.x/docs/api/http.html#http_server_timeout

@underflow00
Copy link
Author

underflow00 commented Feb 18, 2020

Yes, it is part of Node.js itself, which is why it is not implemented again in this module: https://nodejs.org/dist/latest-v13.x/docs/api/http.html#http_server_timeout

That is an inactivity timeout. It does not apply here.

@dougwilson
Copy link
Contributor

Ah. Well, I presume the protection needs to be at the Node.js server level regardless, right? Is this something that Node.js is missing? There are many, many body parsing modules on npm, including the ability to make them yourself of course.

@dougwilson
Copy link
Contributor

I see Node.js has a headers timeout, at least: https://nodejs.org/dist/latest-v13.x/docs/api/http.html#http_server_headerstimeout

@underflow00
Copy link
Author

underflow00 commented Feb 18, 2020

I see Node.js has a headers timeout, at least: https://nodejs.org/dist/latest-v13.x/docs/api/http.html#http_server_headerstimeout

Yes, my understanding is that it's the body parser's responsibility to implement the timeout (of parsing the body).

@dougwilson
Copy link
Contributor

my understanding is that it's the body parser's responsibility to implement the timeout.

Why would that be part of parsing and not part of the stream reading itself? Is this stated somewhere you can link to for where your understanding comes from? If you're saying this is a feature you think should exist here, you're welcome to make a pull request. But if I'm understanding your request correctly that would be a hurge disservice to the npm ecosystem at large as I presume every single module would need to implement this?

@dougwilson
Copy link
Contributor

This is the general docs I refer to when looking for the body reading Node.js docs: https://nodejs.org/en/docs/guides/anatomy-of-an-http-transaction/#request-body

There doesn't seem to be any indication that low level timeouts should be the responsibility of the reader and looking at the two "reference" modules linked there they don't seem to be doing any kind of special timeout behavior? Is it really a feature missing from Node.js?

@underflow00
Copy link
Author

underflow00 commented Feb 18, 2020

Is it really a feature missing from Node.js?

I'm going to go with yes.
There are few socket related Node.js issues open for years so it's not surprising.

Relevant issues:
nodejs/node#6286
nodejs/node#3319

The attack can easily be re-created:

redacted

@dougwilson
Copy link
Contributor

I would suggest contacting Node.js following https://nodejs.org/en/security/ is possible. The issue adding it here is that (1) users may not even use this module (2) use a different module (3) use a different framework based on Node.js. It seems strange they have some timeouts and not others. The inactivity timeout for example is on the body, so it's not like there are no body timeouts currently in Node.js. Perhaps this is an overlooked vector.

@underflow00
Copy link
Author

underflow00 commented Sep 17, 2020

This has been fixed in Node.js's latest security release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants