cap decompressed size of gzip request bodies#2250
Conversation
|
gentle ping |
|
This shouldn't silently truncate; return a 413? I'm on the fence about this PR; the web server (the outer layer) is really more of a demo than meant to be production ready, so perhaps the right call here is to just remove the input limiting support and add a warning about this at startup. |
| # Upper bound on the decompressed size of a gzip-encoded request body. A small | ||
| # compressed body can otherwise inflate to gigabytes before the request is even | ||
| # dispatched, so the cap mirrors git's GIT_HTTP_MAX_REQUEST_BUFFER (10 MiB). | ||
| DEFAULT_MAX_DECOMPRESSED_REQUEST_SIZE = 10 * 1024 * 1024 |
There was a problem hiding this comment.
This isn't the case for git, from its manpage:
he GIT_HTTP_MAX_REQUEST_BUFFER environment variable (or the http.maxRequestBuffer config option) may be set to change the largest ref negotiation request that git will handle during a fetch; any fetch requiring a larger buffer will not succeed. This value should not normally need to be changed, but may be helpful if you are fetching from a repository with an extremely large number of refs. The value can be specified with a unit (e.g., 100M for 100 megabytes). The default is 10 megabytes.
As far as I can tell git doesn't have specific decompression protection. But happy to be corrected.
There was a problem hiding this comment.
You're right, I had that backwards. http-backend caps the compressed body it buffers (via GIT_HTTP_MAX_REQUEST_BUFFER) and streams the inflated bytes into the service process without any decompressed-size limit. The difference is dulwich inflates in-process, so the decompressed bytes land in server memory rather than a child's stdin. I've reworded the comment to say that instead of claiming git has an equivalent cap; kept the 10 MiB default since legitimate gzipped negotiation bodies stay well under it.
GunzipFilter inflated Content-Encoding: gzip bodies with no bound on the output size, so a ~200 KiB compressed POST could expand to ~200 MiB in server memory before the request was dispatched. Inflate the body up front, bounded at 10 MiB by default, and reject anything larger with 413 Request Entity Too Large instead of truncating.
netliomax25-code
force-pushed
the
gunzip-decompression-cap
branch
from
89f7cc5 to
cb26b2d
Compare
|
Reworked to return a 413 instead of truncating. One wrinkle: handlers call start_response before reading the body, so the filter now inflates the body up front (bounded at the cap) and rejects oversized requests before dispatch. Memory stays bounded at the cap either way. Also rebased onto main and added a NEWS entry. On dropping the input limiting entirely: since the chunked-body bounding went into main I kept this consistent with that direction, but if you'd rather remove the limits and just warn at startup that the server isn't production ready, I can rework it that way. |
2 participants
LimitedInputFiltercaps the gzip-encoded request body at Content-Length, butGunzipFilterthen decompresses it with no bound on the output size.Content-Encoding: gzipon upload-pack requests, reachable before authentication on a public repo) is a decompression bomb: a ~200 KiB body inflates to ~200 MiB that the handler reads into memory.GunzipFilternow inflates the body up front, bounded at 10 MiB by default, and rejects anything larger with413 Request Entity Too Large. The check happens before the request is dispatched because handlers callstart_responsebefore reading the body, at which point a 413 can no longer be sent. Added regression tests and a NEWS entry.Note c git doesn't cap the decompressed size itself; http-backend bounds the compressed body it buffers (
GIT_HTTP_MAX_REQUEST_BUFFER) and streams the inflated bytes into a separate service process, while dulwich inflates in-process.