search

PeterPierinakos / vanilla-rustlang-server

Simple, minimal and open source static web server that uses no dependencies.
Apache License 2.0
39 stars 1 forks source link

It's fairly easy to crash the server in single threaded mode #2

Closed maciejhirsz closed 2 years ago

maciejhirsz commented 2 years ago

There is a lot of unwraps that should be ?s with error handling, ideally returning appropriate HTTP error status code in server response. The most egregious are unwraps on reading from socket, and on parsing utf8, both are trivially exploitable for DOS attacks:

  1. Open a connection, send no data, close connection: read will error, unwrap will panic, server will crash.
  2. Send a request with malformed (non-utf8 URI): from_utf8 will error, unwrap will panic, server will crash.

Multi-threading is safer since panics only kill the thread they occur on, but they should still be avoided.

PeterPierinakos commented 2 years ago

Thanks I'll try to rewrite the multithread_handle_connection and handle_sync_connection after I finish implementing CORS to use the ? instead of .unwrap()

PeterPierinakos commented 2 years ago

I am currently working on fixing many of the unhandled unwraps in the code. Do you believe that returning 500 Internal Server Error for the rare cases like these you mentioned is enough so that the server doesn't crash?

PeterPierinakos commented 2 years ago

@maciejhirsz, I believe the issues you have mentioned have mostly been resolved and flagged for Beta 2.

https://github.com/PeterPierinakos/vanilla-rustlang-server/commit/f1978d5fe4f5c62b86e4760e3ac0ebc7d4165db8

Please inform me if I can further improve the error handling with the changes I made and I will reopen this issue if needed.

maciejhirsz commented 2 years ago

I am currently working on fixing many of the unhandled unwraps in the code. Do you believe that returning 500 Internal Server Error for the rare cases like these you mentioned is enough so that the server doesn't crash?

I reckon 400 Bad Request is probably better in case request comes empty, includes non-utf8 characters, or comes from a client that otherwise misbehaves.

5XX error codes are more for bad / unexpected things happening to the server itself despite the request being sensible: running out of memory, running out of file descriptors in the OS, accessing remote database that crashed, bad configuration, etc.

MDN has a good cheat sheet for status codes.

PeterPierinakos commented 2 years ago

Yes, I see what you mean now. I have changed the returned status codes; https://github.com/PeterPierinakos/vanilla-rustlang-server/commit/d3c57e7bd13429d85fd968e7fd97cb0a0e393a0e

PeterPierinakos commented 2 years ago

Locking issue since the issues mentioned here seem to have been resolved.