Opened 7 years ago

Closed 4 months ago

#4891 defect closed fixed (fixed)

twisted.web.http does not throttle pipelined requests

Reported by: ivank Owned by: Indradhanush Gupta
Priority: high Milestone:
Component: web Keywords: security
Cc: jknight, ivank Branch:
Author:

Description

twisted.web.http loads an unlimited amount of pipelined HTTP requests into memory, even when it is busy processing. This allows for a very easy DoS attack.

My suggestion is to call .pauseProducing() on the underlying transport when there are 8 pipelined requests queued, and .resumeProducing() when there are fewer.

Note that even browsers may send as many concurrent pipelined requests as they want to (Opera for example has no limit).

Attachments (3)

limit_HTTP_requests.patch (473 bytes) - added by Indradhanush Gupta 4 years ago.
Added functionality for limiting HTTP requests to 8.
limit_HTTP_requests_and_added_tests.patch (2.2 KB) - added by Indradhanush Gupta 4 years ago.
Made a small change, _pauseFlag = False once the resumeProducing() is about to be called.
test.rpy (248 bytes) - added by Tom Prince 4 years ago.
twistd -n web --resource-script=test.rpy

Download all attachments as: .zip

Change History (14)

comment:1 Changed 7 years ago by DefaultCC Plugin

Cc: jknight added

comment:2 Changed 7 years ago by jknight

It might actually make sense to not handle *any* additional requests while processing one. The main win of the pipelining is usually gonna be the RTT, not processing time. And normally the buffering in TCP will handle storing up a few extra requests. So reading more requests is probably not often a very useful thing to do.

Alternatively, web2's http server implementation is awesome in every way, and in particular, restricts it to 4. :)

comment:3 Changed 7 years ago by ivank

Cc: ivank added

Changed 4 years ago by Indradhanush Gupta

Attachment: limit_HTTP_requests.patch added

Added functionality for limiting HTTP requests to 8.

comment:4 Changed 4 years ago by Indradhanush Gupta

Keywords: review added

comment:5 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Indradhanush Gupta

Thanks for your work on this issue. The attached patch looks like a step in the right direction. All changes and new functionality need to be verified by automated unit tests as well. Can you add unit tests for these changes as well? Thanks again! (Also, a random note, trac separates keywords using spaces, not commas)

comment:6 in reply to:  5 Changed 4 years ago by Indradhanush Gupta

Thanks for the 'random note'. This one is my first patch. Felt nervous. I'll try and do something about the unit tests.

Changed 4 years ago by Indradhanush Gupta

Made a small change, _pauseFlag = False once the resumeProducing() is about to be called.

comment:7 Changed 4 years ago by Indradhanush Gupta

Keywords: review added

I've added the test. It creates 9 requests to invoke pauseProducing() and removes 2 of them from the queue to invoke reumeProducing() once again. The problem I am facing is in the first assertTrue() call, even if I'm comparing _pauseFlag with False the test is passing, however _pauseFlag should actually be True once 8 requests are pipelined. I've intentionally left the first assertTrue() statement as :

self.assertTrue(Request._pauseFlag == False)

instead of :

self.assertTrue(Request._pauseFlag == True)

Please someone do have a look at it.

comment:8 Changed 4 years ago by Indradhanush Gupta

Owner: Indradhanush Gupta deleted

comment:9 Changed 4 years ago by Indradhanush Gupta

Owner: set to Tom Prince

I am interested in participating in GSoC 13 under Twisted. itamar suggested to assign it to you.

Changed 4 years ago by Tom Prince

Attachment: test.rpy added

twistd -n web --resource-script=test.rpy

comment:10 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: changed from Tom Prince to Indradhanush Gupta

I think this code is written at the wrong level. The code you have is only in Request, so that self._quededRequests is only ever 0 or 1. The code probably wants to be on HTTPChanel. I figured this out after actually trying to run the attached code.

Accessing 'http://localhost:8080/never' never completes, so should never complete, and 'http://localhost:8080' returns an empty page. So trying to open 8 copies of the former and then one of the later should hang. But the final one will complete if any of the `/never' requests are cancelled.

A couple of points about the code itself.

  1. You appear to be using a large (>4) indent in a couple of places.
  2. You are assigning to a local variable _pauseFlag.
  3. Explanation of instance variables should be in the class docstring, rather than comments.

comment:11 Changed 4 months ago by Cory Benfield

Resolution: fixed
Status: newclosed

This is no longer relevant because of #8320, which removed pipelining support from twisted.web by doing exactly this.

Note: See TracTickets for help on using tickets.