[PATCH] Enforce that CR precede LF in chunk lines

[Ben Kallus]

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.

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.

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.

diff -r ee40e2b1d083 src/http/ngx_http_parse.c
--- a/src/http/ngx_http_parse.c Mon Dec 25 21:15:48 2023 +0400
+++ b/src/http/ngx_http_parse.c Wed Jan 24 18:11:50 2024 +0000
@@ -2217,9 +2217,6 @@
                 case CR:
                     state = sw_last_chunk_extension_almost_done;
                     break;
-                case LF:
-                    state = sw_trailer;
-                    break;
                 case ';':
                 case ' ':
                 case '\t':
@@ -2236,9 +2233,6 @@
             case CR:
                 state = sw_chunk_extension_almost_done;
                 break;
-            case LF:
-                state = sw_chunk_data;
-                break;
             case ';':
             case ' ':
             case '\t':
@@ -2255,8 +2249,6 @@
             case CR:
                 state = sw_chunk_extension_almost_done;
                 break;
-            case LF:
-                state = sw_chunk_data;
             }
             break;

@@ -2276,9 +2268,6 @@
             case CR:
                 state = sw_after_data_almost_done;
                 break;
-            case LF:
-                state = sw_chunk_start;
-                break;
             default:
                 goto invalid;
             }
@@ -2296,8 +2285,6 @@
             case CR:
                 state = sw_last_chunk_extension_almost_done;
                 break;
-            case LF:
-                state = sw_trailer;
             }
             break;