[PATCH] Enforce that CR precede LF in chunk lines

[Maxim Dounin]

Hello!

On Thu, Jan 25, 2024 at 08:32:17PM +0000, Ben Kallus wrote:

> > Still, there is a robustness principle which allows applications
> > to parse requests with various deviations from the grammar.
> 
> Whether this is principle is good is a matter of opinion. I tend to
> lean toward thinking that it is not (as you can probably tell) but
> reasonable minds will differ on this point.
> 
> > You may want to be more specific what "off by one" means here.
> 
> Happy to :)
> 
> Here's an example of a payload that smuggles a request past Apache
> Traffic Server to a Node.js backend:
> ```
> POST / HTTP/1.1\r\n
> Transfer-Encoding: chunked\r\n
> \r\n
> 2\r\r
> ;a\r\n
> 02\r\n
> 2d\r\n
> 0\r\n
> \r\n
> DELETE / HTTP/1.1\r\n
> Content-Length: 183\r\n
> \r\n
> 0\r\n\r\nGET / HTTP/1.1\r\n\r\n
> ```
> The exact mechanism of this payload is relatively unimportant (it has
> to do with the `2\r\r;a`). The important point is that the POST is
> seen by both ATS and Node, the DELETE is seen only by Node, and the
> GET is seen only by ATS. Thus, the DELETE request is smuggled.
> 
> (A very similar attack worked on Google Cloud's classic application
> load balancer, and on Akamai's load balancer until very recently when
> companies patched the bugs. I'm still working on the writeup for those
> bugs, but you can see us present the material here:
> https://yewtu.be/watch?v=aKPAX00ft5s&t=2h19m0s)
> 
> You'll notice that the DELETE request has a Content-Length header.
> This is because in order for the smuggling to go undetected, the
> response to the DELETE request needs to be sent only after the GET
> request is forwarded. One way to do this is to add a message body to
> the DELETE request, so that it remains incomplete until the arrival of
> the GET request. It is therefore necessary for the attacker to predict
> the length of the GET request after it has passed through the reverse
> proxy, so that this length can be used to compute the Content-Length
> (or chunk size) in the DELETE request. Because reverse proxies often
> modify requests, this is not always straightforward.
> 
> In this instance, I use a Content-Length header of 183 because with
> the configuration of ATS that I was attacking, `GET /
> HTTP/1.1\r\n\r\n` ends up becoming 178 bytes long due to the insertion
> of X-Forwarded-For, Via, etc., +5 for `0\r\n\r\n`. If I had used a
> length less than 183, then Node would send a 400 after responding to
> the DELETE request, which makes the reverse proxy aware that request
> smuggling has occurred. If I had used a length greater than 183, then
> Node would time out waiting for the rest of the DELETE request's
> message body. Thus, I need to guess the length exactly right to pull
> off undetected request smuggling. Guessing correctly can be
> challenging, especially when added headers have unpredictable lengths.
> This is common with CDNs, which often insert random identifiers into
> request headers.
> 
> If instead of using Content-Length, I had used a chunked message body
> to smuggle the DELETE request, and the backend server allows bare LF
> as a chunk line terminator, then my length guess could be one less
> than the correct value without invalidating the message for servers
> that accept bare LF in chunk lines. Thus, when developing future
> request smuggling attacks, getting my length guess correct is a little
> easier when the backend server allows bare LF chunk line endings.

As far as I understand what goes on here and what do you mean by 
using a chunked message body, with length guess which is one less 
than the correct value you'll end up with LF + "\r\n0\r\n\r\n" at 
the request end, which will result in 400.  Length which is one 
more will work though, so understood, thanks.

In the original exploit length which is two less should work 
though, due to the "SHOULD ignore at least one empty line (CRLF) 
received prior to the request-line" robustness exception in 2.2. 
Message parsing of RFC 9112.  And one less might also work, as 
long as empty lines before the request-line accept bare LF (not 
sure about Node though).

Overall, I don't think there is a big difference here.

> > including manual testing such
> > as with nc(1).
> 
> If you pass the -C flag, nc will translate LF to CRLF for you :)

It won't, because "-C" is a non-portable flag provided by a 
Debian-specific patch.  And even if it will work for some, this 
will still complicate testing.

-- 
Maxim Dounin
http://mdounin.ru/