dotnet / dotnet/aspnetcore

Create analyzer that warns against checking if HttpRequest.ContentLength > N

Open
#43,080 3 comments 1 reaction 0 assignees View on GitHub
analyzer area-networking
Dominant language
C#
Stars
38.4k
Forks
10.9k
Avg merge
2d 10h
Merged PRs (30d)
281

Description

We should check against `>=` comparisons too. Comparisons against nullables can cause subtle bugs, but it can be very useful. So warning against all nullable comparisons, is extreme.

However. I've come across [multiple](https://stackoverflow.com/a/73111658/719967) [instances](https://github.com/dotnet/AspNetCore.Docs/pull/26516#discussion_r937272305) in the past two days where developers had incorrectly assumed that `if (!(HttpRequest.ContentLength > N))`, then the request body must not have more than N bytes. It was the number one StackOverflow answer to [Set status code when request is larger than MaxRequestBodySize in kestrel](https://stackoverflow.com/questions/62909093/set-status-code-when-request-is-larger-than-maxrequestbodysize-in-kestrel) This is simply not the case.

Here's an example of a **bad** SO answer that demonstrates what we want to warn against. DO NOT DO THIS:

> For ASP.NET Core 6.0:
>
> 1. Disable `MaxRequestBodySize`
>
> ```csharp
> webBuilder.ConfigureKestrel((ctx, options) =>
> {
> options.Limits.MaxRequestBodySize = null;
> });
> ```
>
> 2. Use custom middleware:
>
> ```csharp
> app.Use(async (context, next) =>
> {
> if (context.Request.ContentLength > 10_000_000)
> {
> context.Response.StatusCode = StatusCodes.Status413PayloadTooLarge;
> await context.Response.WriteAsync("Payload Too Large");
> return;
> }
>
> await next.Invoke();
> });
> ```

When the Content-Length header is present, `ContentLength` will be non-null, and the server will enforce it's accuracy. However, when there is no Content-Length header, the request may still have a body, but `ContentLength` will be null because the body size is not known upfront. Assuming that a null `ContentLength` means the body is empty is wrong.

Not knowing this can lead to security holes in applications. People who think they are guarding against too-large requests simply are not.

An analyzer should warn against any `>` or `>=` comparisons against `HttpRequest.ContentLength`. Because all comparisons with null (except `== `) return false, this comparison wrongly implies that the request without a Content-Length header cannot have a body exceeding any limit when it should imply the exact opposite.

Comparing against the nullable `ContentLength` using `<` or `<=` is still fine because it returns false in the null case. It's false to assume that a null `ContentLength` implies the request body will be under any specified length, so it's semantically correct.

It might be tricky to avoid false positives when `null` is accounted for in some other way, but I think it's best to over warn given how common this mistake seems and how serious the implications are.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.