search

dom96 / httpbeast

A highly performant, multi-threaded HTTP 1.1 server written in Nim.
MIT License
450 stars 53 forks source link

Remediating a previously reported DoS #71

Open the-emmon opened 2 years ago

the-emmon commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

the-emmon commented 2 years ago

@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 commented 2 years ago

@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.

the-emmon commented 2 years ago

@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 commented 2 years ago

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.