Skip to content
This repository has been archived by the owner on Jul 13, 2023. It is now read-only.

Limit HTTP request body sizes at the protocol level #501

Closed
ghost opened this issue Jun 30, 2016 · 3 comments
Closed

Limit HTTP request body sizes at the protocol level #501

ghost opened this issue Jun 30, 2016 · 3 comments
Assignees

Comments

@ghost
Copy link

ghost commented Jun 30, 2016

We currently limit body sizes to 4k, but I think we check that after the entire request body has already been read.

IIUC, it's still possible for a malicious client to send gigabytes of data, which we'll buffer into memory (or a temporary file, from the looks of https://github.com/fiorix/cyclone/blob/master/cyclone/httpserver.py#L168-L179) before our request handler gets called.

@jrconlin jrconlin self-assigned this Aug 15, 2016
@jrconlin
Copy link
Member

This might be an odd one.

The code above relies on the data already having been read and stored in the data var. data is filled by lineReceived, which is subclassed from Twisted's basic.LineReceiver which may top out at 16,384 (This might be an interesting issue for things like VAPID, but that's beside the point right now.) The parent process reads data from the source in 65,535 byte chunks, minus the max header size, leaves a fair bit of slop. If the content-length header is specified cyclone limits the data to that length.

The HTTPConnection class that handles the rawDataRecieved() function is set as a class variable for Application. We could monkey patch it to reject any connection that exceeds 65K, but I'm not really sure that's a good idea.
I'm not really sure there's a point where we can inject ourselves into that flow to, say, restrict twisted's TCP handler to only accept one 65K block.

Continuing to dig into this.

@pjenvey
Copy link
Member

pjenvey commented Aug 16, 2016

FYI protocol class vars are a typical pattern for twisted factories. It's made for overwriting -- the preferable way is to do it is via subclassing the factory (vs monkeypatching).

@jrconlin
Copy link
Member

Everytime I see that, I keep thinking it's more an anti-pattern. Reaching into objects directly just feels unclean. Still, if it's the expected behavior...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants