Limit size of requests with transfer-encoding: chunked

[abonander]

Feature Request

Motivation

While revisiting the following Reddit thread, I came across the conversation I had with LukeMathWalker (author of Zero2Prod) about input validation: https://www.reddit.com/r/rust/comments/shetb1/_/hv3ch7n

It occurred to me that Axum may not be putting any limits on request bodies, and looking through the source, I'm pretty sure that is the case.

While application-specific input validation is still important, it's something you have to remember to do, and even if you do think to do it, e.g. with domain types like Luke suggested, the Json extractor will still read the whole request into memory before attempting to deserialize.

This means it's relatively trivial to DOS an Axum-based server by running it out of memory. Bandwidth may not necessarily be a limiting factor, either: if the attacker gets a cloud instance running in the same datacenter as your application, they will have more or less an unbounded pipe to shove junk data into your application as fast as possible.

Actix Web has built-in mitigation for this attack, as its version of the Bytes extractor, and thus all the extractors built on it, will limit the body to 256 KiB by default.

I did see the ContentLengthLimit extractor but that literally only checks the Content-Length header which can easily be forged by an attacker.

Proposal

I'm not sure of an acceptable solution here. In that same Reddit thread you explained why you don't like how Actix Web configures its extractors, like for setting the size limit in this case.

It could be a const generic parameter but the Bytes type isn't defined in the crate so a new type would have to be introduced to add it. Ideally this would apply to existing code as well, but that would require changing the behavior of the Bytes extractor. It could also break routes that normally expect bigger bodies than the default we choose.

I'm thinking of an extension layer that does the same work as the Bytes extractor but enforces the given limit, and then sets the extracted body as a request extension that the Bytes extractor checks first before anything else. That's kind of the same "action at a distance" that you said you don't like, though.

Alternatives

Don't limit request bodies, basically the status quo.

[davidpdrsn]

I did see the ContentLengthLimit extractor but that literally only checks the Content-Length header which can easily be forged by an attacker.

Hyper wont read more than the content-length regardless what length the body actually has. So limiting large bodies like this seems like the way to go. Its also how warp's content_length_limit works. Doing that check in a middleware is easy if you want it for more than one route.

Edit: Such as .layer(extractor_middleware::<ContentLengthLimit<(), 1024>>())

[abonander]

Hmm, I dunno.

I think I found where it enforces that in the HTTP/1 implementation: https://github.com/hyperium/hyper/blob/master/src/proto/h1/decode.rs#L115

There's a couple of problems I see here:

• It doesn't emit an error, just silently caps the request body. I guess that's ContentLengthLimit's job, but those would be two completely different components creating the mitigation as an emergent property, which seems even more "action at a distance" than how Actix Web configures extractors.
• It doesn't trim the chunk down to the remainder but returns it whole, so it could actually overshoot by whatever the read buffer size currently is.
• I looked and I couldn't find where it applies the same limitation in the HTTP/2 implementation.
• I also can't find where the behavior is documented, if at all. If it's not guaranteed by the API, and not enforced consistently, then it's not really a good idea to rely on.
• ContentLengthLimit forces there to be a Content-Length header even though it's not supposed to be required otherwise.

[davidpdrsn]

It doesn't emit an error, just silently caps the request body. I guess that's ContentLengthLimit's job

Hyper capping it seems fine to me. ContentLengthLimit will emit an error.

It doesn't trim the chunk down to the remainder but returns it whole, so it could actually overshoot by whatever the read buffer size currently is.

If I understand correctly that sounds like a bug.

I looked and I couldn't find where it applies the same limitation in the HTTP/2 implementation.

Also sounds like a bug to me.

I also can't find where the behavior is documented, if at all. If it's not guaranteed by the API, and not enforced consistently, then it's not really a good idea to rely on.

I suppose it's document in the http spec 😛 not sure if it's in hypers docs but I've heard @seanmonstar mention it many times so it's safe to rely on. I believe it's also mentioned in hypers "Vision" doc PR.

ContentLengthLimit forces there to be a Content-Length header even though it's not supposed to be required otherwise.

If you don't require a content length or use transfer-encoding: chunked how do you how large the body/chunks is?

Imo this is something hyper should handle. It feels very "transport level" to me 🤔 Cc @seanmonstar

[abonander]

If you don't require a content length or use transfer-encoding: chunked how do you how large the body/chunks is?

The whole time I've been talking about being able to place limits on impls that collect the request body to a heap allocation before processing it, so impl FromRequest for Bytes, String, Json and Form mainly.

The idea is that they would be guaranteed to bail with an error when they reached a limit instead of relying on the underlying transport to stop reading the body early.

[abonander]

If you're processing the request body in a streaming fashion then you can enforce the limits yourself (if it makes sense to) but most request handlers out there are going to be using one of the above types to collect or deserialize the request body to a container instead.

[davidpdrsn]

I must be misunderstanding still. Feels like we are going in a circle, because that is what ContentLengthLimit does. Hyper won't read more than the content-length and ContentLengthLimit sets a limit on that.

If that's not what you mean can you provide an example?

[abonander]

Except I would prefer not to require a Content-Length header because the chunked transfer encoding exists and is used by CLI clients if you pipe in input (like a handcrafted JSON body for testing some routes).

[davidpdrsn]

Are requests without a content-length and without transfer-encoding: chunked even valid?

[davidpdrsn]

transfer-encoding: chunked is basically a stream so I guess that requires some special handling by the user. I suppose it could be handled with a middleware that limits how much you can read from the body. But I'm not sure.

[davidpdrsn]

If you make a request like this:

use std::{net::TcpStream, io::Write};

fn main() {
    let mut stream = TcpStream::connect("0.0.0.0:3000").unwrap();
    stream.write_all(b"POST / HTTP/1.1\r\n").unwrap();
    // no content-length and no chunked encoding
    stream.write_all(b"\r\n").unwrap();
    stream.write_all(b"body...").unwrap();
    stream.write_all(b"\r\n").unwrap();
}

The body is empty. This matches what I found here.

[abonander]

Yeah, without a chunked encoding or a Content-Length the body is invalid, that's not what I'm talking about here.

ContentLengthLimit by definition forbids chunked encoding which technically makes the HTTP server non-compliant. I want to still allow clients to use chunked encoding.

HTTP/2 also does not require Content-Length.

[davidpdrsn]

Why didn't you just say that 😅 or maybe you did and I didn't catch it.

I'm not aware of how to limit lengths of chunked requests/streams (which are the same from a user of hyper's perspective afaik). @seanmonstar any advice?

Edit: To me this still kinda feels something that should be configurable in hyper. All the data flows through it after all 🤷

ContentLengthLimit by definition forbids chunked encoding which technically makes the HTTP server non-compliant

Do you have reference for that? The existence of 411 Length Required makes it requiring lengths aren't non-compliant.

[abonander]

I'm not aware of how to limit lengths of chunked requests/streams (which are the same from a user of hyper's perspective afaik).

I'm also getting that "going in circles" feeling because I explained already what I want it to do:
#853 (comment)

[davidpdrsn]

You didn't mention not wanting to require content lengths there.

[abonander]

I would prefer not to in any case.

[seanmonstar]

ContentLengthLimit by definition forbids chunked encoding which technically makes the HTTP server non-compliant.

Rejecting a request with a chunked body does not make the server non-compliant. It's perfectly a reasonable thing to do. (And both the HTTP/1 and h2 implementations check and count content-length.)

If you want to limit how many bytes are streamed with a chunked request body, I think the best place to do that is a layer or at least a pre-condition to a handler that wraps the request body in some Limit<B> that emits an error once too many bytes have been streamed.

[davidpdrsn]

We are working on adding a Limited body to http-body and a middleware to tower-http that applies the body to address this.

See hyperium/http-body#60 and hyperium/http-body#61

So I'll close this now since nothing needs to be added directly to axum. Though we can probably improve ContentLengthLimit when there is something in http-body.