HTTP parser accepts an alternative grammar for chunk size in Transfer-Encoding: chunked requests

[dcepelik]

System Information

• OS: GNU/Linux 5.14.16-arch1-1
• Ruby: ruby 2.7.4p191 (2021-07-07 revision a21a3b7d23) [x86_64-linux]
• Version: 0.7.44
• OpenSSL: irrelevant

Description

RFC7230§4.1 prescribes the following grammar for chunks:

chunk          = chunk-size [ chunk-ext ] CRLF
	         chunk-data CRLF
chunk-size     = 1*HEXDIG
last-chunk     = 1*("0") [ chunk-ext ] CRLF

chunk-data     = 1*OCTET ; a sequence of chunk-size octets

It seems that due to a reuse of some existing hex-number parsing logic, much more is accepted as chunk size, e.g. -0x1, whereas the size should be a plain sequence of hex digits.

Rack App to Reproduce

app = proc { |env| [
  200,
  env.to_h.select { |k,v| k.start_with?('HTTP_') }.map { |k,v| ["foo_" + k, v ] }.to_h,
  ["Hello World\n"]]
}
run app

Testing code

printf "GET /xyz HTTP/1.1\r\nHost: foo\r\nTransfer-encoding: chunked\r\n\r\n1\r\nx\r\n0x0\r\n\r\nGET / HTTP/1.1\r\nHost:x\r\nContent-length: 0\r\n\r\n" | nc 127.0.0.1 3000

This sends the following two requests:

GET /xyz HTTP/1.1
Host: foo
Transfer-encoding: chunked

1
x
0x0

GET / HTTP/1.1
Host:x
Content-length: 0

Expected behavior

As the first request does not conform to the grammar for chunked requests, it should be discarded. I believe client connection should also be closed, to ensure that the misaligned parser operating on the rest of the stream does not accidentally produce any new messages.

Actual behavior

The two requests are both accepted and the output of the command above is:

HTTP/1.1 200 OK
connection:keep-alive
foo_http_version:HTTP/1.1
foo_http_host:foo
content-length:12
date:Fri, 26 Nov 2021 12:45:36 GMT
last-modified:Fri, 26 Nov 2021 12:45:36 GMT

Hello World
HTTP/1.1 200 OK
connection:keep-alive
foo_http_version:HTTP/1.1
foo_http_host:x
content-length:12
date:Fri, 26 Nov 2021 12:45:36 GMT
last-modified:Fri, 26 Nov 2021 12:45:36 GMT

Hello World

[boazsegev]

Yes...

I believe that this is only half an issue... Yes, I should have tested for negative values 🙈😅

But I can't shake the feeling that it's good to be lenient - especially where network parsers are concerned.

To many client softwares have issues with this and I think there is a distinct possibility of a Hex number starting with 0x###...

[boazsegev]

P.S... there's a possible fix for the issues.

Please, if you have the time, let me know what you think.

Cheers,
Bo.

[dcepelik]

Hi,

But I can't shake the feeling that it's good to be lenient - especially where network parsers are concerned.

This is a good read explaining why that might not be a great idea.

I think your intuition is correct by being liberal in what you accept, however that does not apply to a stack of multiple components - each understanding the request differently - and may indeed lead to a host of security issues, as hinted upon by the several issues I reported. I'm afraid that with multiple components handling the request, there will never be perfect alignment in how they understand it, at least not for HTTP/1.1. But you should stick to the RFC as close as possible to minimize chances of that happening.

I see two ways to resolve this:

• Bluntly state in the README that the parser isn't strictly following the relevant RFC, and that the user should always run any Iodine-based apps behind (a particular version of) NGINX (with which it's known to work well), or otherwise you're on your own and you should take care to verify the various edge cases yourself. This is far from ideal, but it is a "fix" of sorts.
• Actually fix the issues. If I were to do it, I would probably check out the linked post above to see how they performed the fuzzy testing, and I would run the battery against Iodine, fixing all issues. It may however turn out that scrapping the parser and either using some existing one.

Regards, David