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

Strict validation of content length and chunk length. #20

Merged
merged 1 commit into from
Jul 30, 2023

Conversation

ioquatix
Copy link
Member

@ioquatix ioquatix commented Jul 30, 2023

Security researchers @mukeran and @chenjj have reported issues with parsing content length and chunk lengths:

RFC 9112 Section 7.1 defined the format of chunk size, chunk data and chunk extension (detailed ABNF is in Appendix secion).

In a word:

  • The value of Content-Length header should be a string of 0-9 digits.
  • The chunk size should be a string of hex digits and should split from chunk data using CRLF.
  • The chunk extension shouldn't contain any invisible character.

However, we found that Falcon has following behaviors while disobey the corresponding RFCs.

  • Falcon accepts Content-Length header values that have "+" prefix.
  • Falcon accepts Content-Length header values that written in hexadecimal with "0x" prefix.
  • Falcon accepts "0x" and "+" prefixed chunk size.
  • Falcon accepts LF in chunk extension.

This behavior can lead to desync when forwarding through multiple HTTP parsers, potentially results in HTTP request smuggling and firewall bypassing.

When sending the following requests to the server, falcon successfully parsed our requests and our application successfully read request body.

Impact

The above is a valid assessment of the behaviour of the HTTP/1 parser. However, as this gem does not expose the raw body or forward it in any way, I don't believe it can cause issues in the real world. However, I accept that stricter parsing of the length fields is a good idea.

Regarding chunk extensions, these are ignored and not exposed to the user. Therefore, I don't believe it would be valuable to validate these. Feel free to correct me here.

Actions

  • Adopt stricter parsing of the content length field according to the RFC.
  • Adopt stricter parsing of the chunk length according to the RFC.
  • Explicitly ignore any chunk extension fields.

Types of Changes

  • Security

Contribution

@ioquatix ioquatix added the bug Something isn't working label Jul 30, 2023
@ioquatix ioquatix self-assigned this Jul 30, 2023
@ioquatix ioquatix merged commit e11fc16 into main Jul 30, 2023
34 of 36 checks passed
@ioquatix ioquatix deleted the strict-lengths branch July 30, 2023 00:46
@ioquatix ioquatix added this to the v0.15.1 milestone Jul 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant