Remediating a previously reported DoS

[the-emmon]

Though seemingly small, this single line permits a CWE-617 DoS. With a request, an attacker can bring down any httpbeast-reliant framework, including Jester.

The vulnerability was found during secure code review, then triaged a few months ago by @FedericoCeratto.

[the-emmon]

Is the assert there to avoid any potential major bugs, or is it just an artifact left over from development? Although httpbeast with the line deleted passes all tests, I want to make sure I'm not inserting any new bugs into the library.

[FedericoCeratto]

@the-emmon please be aware that assert is executed only in debug mode and stripped by -d:release unlike doAssert

[the-emmon]

@FedericoCeratto Hmm... If -d:release is supposed to prevent AssertionDefect from being raised then we may have a bigger issue. I've tried running nim c -d:release jestertest.nim on both stable and devel and I'm still getting application crashes when I trigger the DoS curl. Any assert I add to my jester test application works as you've described, but it appears that AssertionDefect being raised in an imported library still crashes the app under -d:release. It seems as though imported libraries are still being compiled in debug mode even when -d:release is used.

The only switches that seem to cause code to compile correctly are -d:danger and --assertions=off - I don't get an AssertionDefect shutdown when using either. However, from what I can tell, neither is a very common-knowledge switch for production code. The Nim documentation states that the -d:release switch should be used for production code and the -d:danger switch should be used for benchmarking.

I may be missing something here. Would you be willing to verify my observation is accurate?

[ghost]

@the-emmon - @FedericoCeratto is a bit wrong - -d:release does not disable assertions (it did before 0.20, but that was long ago). -d:danger does however.

You can verify this by checking the main Nim config file - https://github.com/nim-lang/Nim/blob/devel/config/nim.cfg#L56 - assertions are only disabled when danger is defined. Danger is named like that because it disables all standard Nim runtime checks for the sake of performance.

[dom96]

Thank you for this but I think we need to do something different here. This assertion failing indicates that the data sent by the client is invalid, so ideally we should respond to them accordingly instead of just ignoring the problem.

Can you change this to throw an exception an ensure it gets handled in the code which calls bodyInTransit? :)

[the-emmon]

@Yardanico
Thanks for pointing me toward the main Nim config file, I had entirely forgotten I could check there!

@dom96
I agree. With my proposed solution, the server never replies to requests with improper Content-Length values, which doesn't feel like a great fix. To the best of my ability, I did as you suggested and tried throwing an exception within bodyInTransit(). Unfortunately, I wasn't able to effectively handle it within the while true statement it's called from - everything I tried broke some important POST functionality due to the context of the call.

After some reading, including all of the RFC documentation for HTTP 1.1, it seems like there is no great way to effectively respond to something like curl localhost:5000/endpoint -H 'Content-Length: 10' -d 'aaa'; the only true indicator of POST data size is a user-controlled header that can be changed arbitrarily.

Because I was curious, I read documentation on how Apache deals with it. When given a larger header value than body data, Apache hangs and never replies, then kills the request after a certain amount of time... which is pretty much the same lousy solution that I proposed. Assuming Apache got it right and that's the best way to deal with an improper Content-Length header, should a default request timeout be added to httpbeast? I'm a junior-level developer, so please let me know if I'm missing the obvious right way of implementing the exception you suggested.

I greatly appreciate the time all of you have spent thus far in addressing this :)

[dom96]

Hey @the-emmon, sorry for the late response from my side. Yeah, I would say that HttpBeast needs to handle timing out requests to be truly resilient to attacks. This is one of the reasons I still say to people to use HttpBeast/Jester behind nginx or another hardened HTTP server.

The worry here is that we need to implement timeouts that won't affect the performance too much. We probably want a timeout per client and to have some efficient way to identify these clients and close their FD.