uWebsockets.js continues to provide incoming body chunks through the onData() handler after response has been sent

[kartikk221]

Hello, I maintain the HyperExpress package and recently there was a DoS vulnerability #68 introduced due to the onData() handler providing body chunks after Response had already been sent for a request. Essentially the following would occur:

1. An HTTP GET request would be made with some body data (Even thought GET requests aren't supposed to have any)
2. HyperExpress see that there was a content-length header and would thus start consuming chunks from the onData() handler.
3. Since this was a GET request, the route would almost instantly send a response with some content.
4. After sending the response, uWS.Response would be discarded as expected but the onData() handler would continue to provide ArrayBuffer chunks and since the Readable stream on the request was never flowing, HyperExpress would try and pause the request with uWS.Response.pause() leading to an invalid discarded access.

I ended up patching the vulnerability in HyperExpress by adding a boolean on the request component which would be toggled after the response was completely sent to then ignore all incoming chunks from the onData() handler.

I'm not sure If this behavior was to be expected from uWebsockets.js but wouldn't it be better for uWebsockets.js to stop calling the onData() handler by default once the uWS.Response has been discarded from the response having been fully sent?

Thanks!

[ghost]

This is not a bug. If you register onData, then you will get data chunks until there are no more to get. This has no relation to whether you respond or not. If you have incoming data, it makes sense to respond when you have all data.

[kartikk221]

I see, In that case, would it may be beneficial to add a note or brief description of this behavior under the onData() method's documentation because most implementations will implement automatic pausing/resuming to the uWS.HttpResponse in conjunction with the chunks coming in through onData() and so If there is a potential scenario where onData() continues to operate while uWS.HttpResponse is discarded, there is a risk of discarded access errors from automatic pause()/resume() calls.

[kartikk221]

Your library is adding onData handler for every request that has a body and processing it whether the user intends to process the body or not.

Unless I am mistaken, the onData() handler has to be called in the initial synchronous call from the uWS route handler and this makes any type of async work practically impossible before having to decide to accept the body. The Express.js/Node.js ecosystem in many cases depends on the freedom to be able to do this async work before choosing to accept the body hence HyperExpress begins streaming the data in the initial synchronous call, so If needed, the user can access the data at any time in the future after any arbitary amount of async work.

Regardless of the use case, I don't think it's irrational to have a note or brief description added to note that the onData() handler will continue to provide chunks even after the uWS.HttpResponse has been discarded after the uWS.HttpResponse.end() call has been made. It's a few lines of descriptive behavior that can save someone potential vulnerabilities in their code in the future.

[ghost]

I can buy that it is strange that onData fires after end, but at the same time it is kind of weird to respond to a request before the body has been received.

[e3dio]

@kartikk221 I see you made res.end(body, close = true) the default behavior for your app, I don't think you want that you want the connection open to re-use it for persistent connections https://github.com/kartikk221/hyper-express/commit/c82610df7f33ef6774c2bb90f8fa9eb4dac5246a also it does not solve your issue because you can still receive body while res.end is finishing. Ideally you should be waiting for onData to finish then respond or res.close if over limit. To attach onData to every request on every route for any possible body when is not needed or expected does not seem like great idea, but I guess is the Express way. Docs may not need to explain case for incorrect behavior

[kartikk221]

I see you made res.end(body, close = true) the default behavior for your app, I don't think you want that you want the connection open to re-use it for persistent connections

I see, I misread from #307 thinking it would halt the onData() handler, I think going forward I'll likely just stick with a boolean built into the HyperExpress Request component which ignores all and any chunks being piped from the onData() handler after the Response has been sent.

To attach onData to every request on every route for any possible body when is not needed or expected does not seem like great idea, but I guess is the Express way

As the name states HyperExpress is meant to be Express but with hyper performance. If I could bind the onData() handler at any time throughout the lifecycle of a request then I would do that for most efficiency but since the onData() handler has to be bound in the initial synchronous call from the uWS route handler, the body has to be streamed so the user can choose to accept or pipe the body into their desired resource at any time in the lifecycle of a request.

I can buy that it is strange that onData fires after end, but at the same time it is kind of weird to respond to a request before the body has been received.

I see what you are saying and the behavior of onData() is not inherently wrong. I was just proposing that this behavior should be better described in the documentation as clarifying this behavior can certainly help prevent bugs and vulnerabilities in other people's applications that they build on top of uWS in the future.

[ghost]

Ignoring onData chunks is fine, that's what happens anyways. You cannot refuse data on a tcp connection all you can do is ignore it.