Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Limit size of request bodies in Bytes extractor #1346

Merged
merged 4 commits into from
Sep 10, 2022
Merged

Limit size of request bodies in Bytes extractor #1346

merged 4 commits into from
Sep 10, 2022

Conversation

davidpdrsn
Copy link
Member

Motivation

We recently received a report of a vulnerability in axum caused by Bytes::from_request calling hyper::body::into_bytes directly without setting a limit. This meant if someone sent an infinite request body the server would attempt to buffer the whole thing in memory and eventually OOM.

This also applies to extractors that calls Bytes::from_request internally like String, Json, and Form.

Solution

I think the right move is to set a default limit on how much Bytes::from_request will consume. I don't think just documenting it is enough since users might not notice.

This PR fixes it by wrapping the body in http_body::Limited before calling into_bytes. This can be disabled by adding the new DefaultBodyLimit::disable() middleware. That sets a private extension which Bytes::from_request looks for.

Once this is merged I'll backport this to 0.5

@davidpdrsn davidpdrsn requested a review from jplatte September 3, 2022 11:38
davidpdrsn
davidpdrsn commented Sep 3, 2022
View reviewed changes
axum-core/src/extract/request_parts.rs
.map_err(FailedToBufferBody::from_err)?;
// update docs in `axum-core/src/extract/default_body_limit.rs` and
// `axum/src/docs/extract.md` if this changes
const DEFAULT_LIMIT: usize = 2_097_152; // 2 mb
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I picked 2mb because thats what actix-web uses for their Json extractor.

@jplatte
Copy link
Member

jplatte commented Sep 8, 2022

If we do this, why not let the user configure the limit using DefaultBodyLimit, rather than requiring a separate middleware to be used when one wants a different limit than 2MB?

@davidpdrsn
Copy link
Member Author

If we do this, why not let the user configure the limit using DefaultBodyLimit, rather than requiring a separate middleware to be used when one wants a different limit than 2MB?

Yeah we could do that. I did consider it but kinda wanted to this as minimal as possible and then add more stuff as we go.

@davidpdrsn davidpdrsn enabled auto-merge (squash) September 10, 2022 06:25
@davidpdrsn davidpdrsn merged commit 759e988 into main Sep 10, 2022
@davidpdrsn davidpdrsn deleted the default-limit-on-request-body branch September 10, 2022 06:36
davidpdrsn added a commit that referenced this pull request Sep 10, 2022
@davidpdrsn
Limit size of request bodies in Bytes extractor (#1346)
6a69d0c 
* Apply default limit to request body size

* Support disabling the default limit

* docs

* changelog
This was referenced Sep 10, 2022
Merged
Merged
davidpdrsn added a commit that referenced this pull request Sep 10, 2022
@davidpdrsn @jplatte
Limit size of request bodies in Bytes extractor (#1362)
95e21c1 
* Limit size of request bodies in `Bytes` extractor (#1346)

* Apply default limit to request body size

* Support disabling the default limit

* docs

* changelog

* fix doc test

* fix docs links

* Avoid unhelpful compiler suggestion (#1251)

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
@yanns
Copy link
Collaborator

yanns commented Sep 11, 2022

For those who are already using RequestBodyLimitLayer, we should disable the new layer right?

@davidpdrsn
Copy link
Member Author

@yanns yes. See https://docs.rs/axum/latest/axum/extract/struct.DefaultBodyLimit.html

@kellytk kellytk mentioned this pull request Sep 12, 2022
Closed
@yanns
Copy link
Collaborator

yanns commented Sep 12, 2022 •
edited
Loading

@yanns yes. See https://docs.rs/axum/latest/axum/extract/struct.DefaultBodyLimit.html

Thanks for the quick answer.
As we are using

#[debug_handler(body = Limited<Body>)]
pub async fn my_handler(
    ....
    mut request: Request<Limited<Body>>,
) -> Response {

and we read the body ourselves:

    let body = request.body_mut();
    let body_bytes = match hyper::body::to_bytes(body).await {
        Ok(data) => data,
        Err(err) => {
            if err.downcast_ref::<LengthLimitError>().is_some() {
                return StatusCode::PAYLOAD_TOO_LARGE.into_response();
            }
            ...
        }
    };

it seems that we don't need to disable the new layer for this handler. We are not relying on the Bytes extractor.

@davidpdrsn
Copy link
Member Author

Yes. That is also in the docs 😅

Note that if an extractor consumes the body directly with Body::data, or similar, the default limit is not applied.

This was referenced Sep 13, 2022
Open
Closed
Closed
Closed
@github-actions github-actions bot mentioned this pull request Nov 1, 2022
Open
@github-actions github-actions bot mentioned this pull request Nov 8, 2022
Closed
@DanielJoyce
Copy link

What version of axum-core is this in and has it been pushed?

@davidpdrsn
Copy link
Member Author

@DanielJoyce yes. See the changelog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jplatte jplatte jplatte approved these changes

Assignees
No one assigned
Labels
A-axum A-axum-core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@davidpdrsn @jplatte @yanns @DanielJoyce