Opened 3 years ago

Closed 3 years ago

#8868 defect closed fixed (fixed)

Twisted.web should apply backpressure to non-reading clients

Reported by: Nathaniel J. Smith Owned by: Cory Benfield
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: 8868-lukasa-httpchannelproducerconsumer
branch-diff, diff-cov, branch-cov, buildbot
Author:

Description

It's very easy to DoS twisted.web servers. If a client connects and starts sending GET requests, but never reads from its socket, then the server will happily process the first request, queue the response to its send buffer, then process the second requesut, queue the response to its send buffer, ... until the send buffer triggers OOM. So the cost to the attacker is about ~40 bytes data transmitted to cause the server to ~permanently lose memory equal to whatever the largest resource it serves is.

To reproduce:

  • Remember to hit control-C before your machine swaps to death.

It would be better if twisted.web were less willing to buffer unbounded amounts of data in memory on behalf of untrusted clients.

(Checked with Glyph, he said to treat this as a regular bug rather than a security bug.)

Change History (6)

comment:1 Changed 3 years ago by Cory Benfield

Owner: set to Cory Benfield

So I'd like to tackle this fairly soon, because problems like this undermine the idea that Twisted should be your frontend web server, and I genuinely believe that it should.

So the *easiest* thing to do to handle this would probably be to have HTTPChannel register itself as a producer/consumer of its transport. That way, the transport can "pause" the HTTPChannel which can propagate that pause through to both the Request object and *back* to the transport.

The same problem exists in the H2Connection, by the way, and will require a bit more complication because some level of internal buffering is a good idea. However, ultimately, the H2Connection should probably *also* pause its transport when it is paused. That will require that H2Connection start to implement IConsumer (which it currently does not) so that it can register its transport appropriately. We can then discuss whether H2Connection needs a richer API to be able to query the transport about *why* it's being paused.

All of these things together *should* resolve this problem, I think. Does anyone have thoughts on this?

comment:2 Changed 3 years ago by Cory Benfield

A separate question: do we want to provide a formal interface for controlling the write buffer in Twisted? I think the HTTP/2 implementation would like this in particular: the HTTP/2 implementation really wants as small a write buffer as possible, so for HTTP/2 we'd like to set the send buffer to 0 and also set TCP_NOTSENT_LOWAT relatively low if we can do it (e.g. something in the region of SETTINGS_MAX_FRAME_SIZE).

comment:3 Changed 3 years ago by Cory Benfield <lukasaoz@…>

In 7dd928df:

Add topfile for #8868.

comment:4 Changed 3 years ago by Cory Benfield

Branch: 8868-lukasa-httpchannelproducerconsumer
Keywords: review added
Owner: Cory Benfield deleted
Type: enhancementdefect

(Marking this as a defect, which is what it is)

A patch for this issue is available in https://github.com/twisted/twisted/pull/591

I should note that this is *literally* just a patch for this issue. Right now it will take Twisted a *very* long time to time out a client who is stuck in this state, which means it's still trivial to take out a Twisted server using this attack: just spawn a few hundred clients all mounting this exact same attack. It's not quite as trivial as it was, but to improve this we need to start getting a bit more aggressive about timing out clients.

comment:5 Changed 3 years ago by hawkowl

Keywords: review removed
Owner: set to Cory Benfield

comment:6 Changed 3 years ago by Cory Benfield

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.