[PATCH] Enforce that CR precede LF in chunk lines

[Maxim Dounin]

Hello!

On Thu, Jan 25, 2024 at 03:21:16AM +0000, Ben Kallus wrote:

> The HTTP/1.1 standards allow the recognition of bare LF as a line
> terminator in some contexts.
> 
> From RFC 9112 Section 2.2:
> > Although the line terminator for the start-line and fields is the sequence CRLF, a recipient MAY recognize a single LF as a line terminator and ignore any preceding CR.
> 
> From RFC 7230 Section 3.5:
> > Although the line terminator for the start-line and header fields is
> > the sequence CRLF, a recipient MAY recognize a single LF as a line
> > terminator and ignore any preceding CR.
> 
> From RFC 2616 Section 19.3:
> > The line terminator for message-header fields is the sequence CRLF.
> > However, we recommend that applications, when parsing such headers,
> > recognize a single LF as a line terminator and ignore the leading CR.
> 
> In summary, bare LF can be recognized as a line terminator for a
> field-line (i.e. a header or trailer) or a start-line, but not outside
> of these contexts. In particular, bare LF is not an acceptable line
> terminator for chunk data lines or chunk size lines. One of the
> rejection messages for an RFC 9112 errata report makes the reasoning
> behind this choice clear:
> 
> > The difference was intentional. A chunked parser is not a start line or field parser (it is a message body parser) and it is supposed to be less forgiving because it does not have to retain backwards compatibility with 1.0 parsers.
> > Hence, bare LF around the chunk sizes would be invalid and should result in the connection being marked as invalid.

Still, there is a robustness principle which allows applications 
to parse requests with various deviations from the grammar.  
Quoting RFC 2616:

   Although this document specifies the requirements for the generation
   of HTTP/1.1 messages, not all applications will be correct in their
   implementation. We therefore recommend that operational applications
   be tolerant of deviations whenever those deviations can be
   interpreted unambiguously.

As such, it is certainly valid for a HTTP/1.1 server based on RFC 
2616 to accept LF as line terminator in chunk sizes.

While RFC 7230 and RFC 9112 tried to harden requirements, and now 
say that "the server SHOULD respond with 400" on deviations which 
are not explicitly allowed, it is still not a violation to accept 
such deviations.

> Currently, Nginx allows chunk lines (both size and data) to use bare
> LF as a line terminator. This means that (for example) the following
> payload is erroneously accepted:
> ```
> POST / HTTP/1.1\r\n
> Host: whatever\r\n
> Transfer-Encoding: chunked\r\n
> 0\n                                                    # <--- This is
> missing a \r
> \r\n
> ```
> 
> This is probably not such a big deal, but it is a standards violation,
> and it comes with one small consequence: chunk lengths that are off by
> one will not invalidate the message body.
> 
> I've developed a few request smuggling exploits against other servers
> and proxies in the past that rely upon the attacker's ability to
> predict the length of a request after it has passed through a reverse
> proxy. This is usually straightforward, but if there are any
> unpredictable headers inserted by the proxy, getting the guess right
> becomes nontrivial. Being able to be off by one thus makes the
> attacker's job a little bit easier.

You may want to be more specific what "off by one" means here.  

While it is true that an attacker which isn't able to generate 
proper CRLF for some reason might be stopped by such a 
restriction, it still needs to ensure there is an LF, which makes 
things mostly equivalent in almost all cases.

> Given that many popular HTTP implementations (Apache httpd, Node,
> Boost::Beast, Lighttpd) adhere to the standard on this line
> termination issue, we should expect this change to break almost no
> clients, since any client generating requests that terminate chunk
> lines with bare LF would already be incompatible with a large portion
> of the web.
> 
> It is, however, also true that many HTTP implementations (Go net/http,
> H2O, LiteSpeed) exhibit the same behavior as Nginx, and it's probably
> worth exploring why that is.
> 
> The following patch changes Nginx's parsing behavior to match the
> standard. Note that this patch does not stop Nginx from allowing bare
> LF in request lines, response lines, headers, or trailers. It stops
> Nginx from accepting bare LF only in chunk size and data lines, where
> the standard does not permit LF/CRLF permissiveness. It's also a
> delete-only patch, which is always nice :)
> 
> If you all are open to this change, it will also be necessary to fix
> up the many LF-delimited chunks that are present within the test
> suite.

No, thanks.

There are little-to-no benefits from such a change, but the change 
will needlessly complicate testing, including manual testing such 
as with nc(1).

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