Opened 9 years ago

Closed 9 years ago

#6029 defect closed fixed (fixed)

Invalid Content-Length header raises ValueError

Reported by: Michele Orrù Owned by: therve
Priority: normal Milestone:
Component: web Keywords:
Cc: Michele Orrù Branch:
Author:

Description

Sending an invalid 'Content-Length' header to a twisted.web.http.HTTPChannel raises a ValueError.

$ twistd -n web --path /tmp

$ printf 'GET / HTTP/1.1\r\nContent-Length: x\r\n\r\n\r\n\r\n' | nc localhost 8080

Traceback:

	  File "/[....]/twisted/twisted/web/http.py", line 1587, in headerReceived
	    self.length = int(data)
	exceptions.ValueError: invalid literal for int() with base 10: 'x'

< exarkun> the logged exception is annoying, that seems worth fixing

< exarkun> (potentially extremely annoying, if someone decides to start DoSing your web server with invalid requests to fill up your log files with garbage)

According to rfc standard, the server should return a 400 Error.

[From http://corte.si/posts/code/pathod/pythonservers/index.html]

Attachments (5)

ticket6029.unittest.patch (1.6 KB) - added by Michele Orrù 9 years ago.
Unittest that confirms the issue.
ticket6029.patch (2.1 KB) - added by Michele Orrù 9 years ago.
ticket6029.1.patch (2.2 KB) - added by Michele Orrù 9 years ago.
ticket6029.2.patch (2.4 KB) - added by Michele Orrù 9 years ago.
ticket6029.2.2.patch (2.4 KB) - added by Michele Orrù 9 years ago.

Download all attachments as: .zip

Change History (16)

Changed 9 years ago by Michele Orrù

Attachment: ticket6029.unittest.patch added

Unittest that confirms the issue.

comment:1 Changed 9 years ago by Michele Orrù

Cc: Michele Orrù added

woops, I forgot to add two '\n' to the end of the request in test_invalidHeaders. Updating with a patch. I don't really like the way http error 400 is raised and checked, but seems a convention over web.http.

Changed 9 years ago by Michele Orrù

Attachment: ticket6029.patch added

comment:2 Changed 9 years ago by Jean-Paul Calderone

Summary: Invalid Content-Lenght header raises ValueErrorInvalid Content-Length header raises ValueError

comment:3 Changed 9 years ago by Jean-Paul Calderone

Component: coreweb
Keywords: review added
Owner: set to Jean-Paul Calderone

Seems to be up for review.

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

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Michele Orrù

Thanks. This is a nice extra check to have (as I already mentioned on IRC :). A few points though:

  1. The new test method needs a docstring explaining its intent.
  2. I think the new test method should also be making an assertion that the transport is closed.
  3. There is a stray print added to http.py.
  4. There is a misbehavior with the fix. If I actually issue a request that is invalid in this way to a server run using twistd -n web --path /tmp, then I receive *both* a 400 and a 200 in response.

Thanks again. I'm looking forward to the next version of the patch!

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

Oh, right. One other point I forgot about:

  1. The patch solves the problem in a "look before you leap" way. However, it's better to ask for forgiveness than permission. I'd rather see the patch add exception handling around the int() call than try to predict whether int() is going to raise ValueError.

comment:6 Changed 9 years ago by Michele Orrù

  1. done!
  2. done!
  3. fixed!
  4. That is not a problem concerning only this issue; twisted aswers the same way even for requests such as
$ printf 'GET / HTTP/1.1\r\nContent-Length: 1\r\n\r\n\r\n\r\n' | nc localhost 8080

also, I see at the bottom of the same method:

1607         self._receivedHeaderCount += 1 
1608         if self._receivedHeaderCount > self.maxHeaders:
1609             self.transport.write("HTTP/1.1 400 Bad Request\r\n\r\n")
1610             self.transport.loseConnection()

How shall I proceed here? That's fine for me refactoring all the HTTPChannel class, because the only alternatives that I see here involve hackish reassignment to instances attributes.

  1. done!

A few notes about the new patch:

  • I have used the "shameful" TestCase.assert_ method for homogeneity with the rest of the code; it this worth a ticket for cleanup?
  • even though the HTTPChannel.transport.abortConnection() method would have been probably the best choice in this case, it's not implemented on twisted.test.proto_helpers and neither used whenever another HTTPError400 is raised. So, I am just keeping the HTTPChannel.transport.loseConnection(); is this woth a ticket for updating?

Changed 9 years ago by Michele Orrù

Attachment: ticket6029.1.patch added

comment:7 Changed 9 years ago by Jean-Paul Calderone

  1. That is not a problem concerning only this issue;

Sorry, I don't understand. Can you explain this further? If I issue a request to Twisted trunk@HEAD with "Content-Length: 1" then I get a single response. If I issue a request to Twisted trunk@HEAD with "Content-Length: x" then I get no response and a closed connection.

also, I see at the bottom of the same method: <header counting code>

Sorry, I'm not sure what the relevance of this code is to this ticket.

I have used the "shameful" TestCase.assert_ method for homogeneity with the rest of the code; it this worth a ticket for cleanup?

Definitely do not add new code using assert_. If you want, change the nearby code (say, other tests on the same TestCase) to stop using assert_. You could file a ticket for broad improvements to the quality of the tests in this class or module (enumerate the planned improvements on the ticket), but tickets for making a single mechanical transformation to a large area of code are not particularly welcome (they usually miss the big picture and are difficult to review).

even though the HTTPChannel.transport.abortConnection() method would have been probably the best choice in this case, it's not implemented on twisted.test.proto_helpers and neither used whenever another HTTPError400 is raised. So, I am just keeping the HTTPChannel.transport.loseConnection(); is this woth a ticket for updating?

Directly using abortConnection here would be wrong. You won't actually see the 400 response sent if you use abortConnection. Instead, something using loseConnection and abortConnection after a delay would be correct (this is the pattern that's probably generally correct for code that wants to close connections).

It would be great to have the testing helpers support abortConnection though, so that'd be a great ticket to file. We probably also want a helper for loseConnection/delay/abortConnection too, that would be another nice ticket.

One other comment, the test method docstring makes a couple (very commonly made :) mistakes. First, it starts with "Tests" which is redundant because it's a test method docstring - we know it is testing something. Second, it doesn't actually explain the behavior being tested in sufficient detail. What it should say is something like this:

If a Content-Length header with a non-integer value is received, a 400 (Bad Request) response is sent to the client and the connection is closed.

ie, the test method docstring should be quite detailed - sufficiently detailed for a future human maintainer to understand what you were trying to do and potentially fix bugs in the test. Put another way, the test method docstring should be a specification for the part of Twisted being tested.

Also, make sure to add the "review" keyword to the ticket when you're submitting a new patch for review. :)

Thanks again!

comment:8 in reply to:  7 Changed 9 years ago by Michele Orrù

Keywords: review added

Replying to exarkun:

  1. That is not a problem concerning only this issue;

Sorry, I don't understand. Can you explain this further? If I issue a request to Twisted trunk@HEAD with "Content-Length: 1" then I get a single response. If I issue a request to Twisted trunk@HEAD with "Content-Length: x" then I get no response and a closed connection.

I was referring to:

$ printf 'GET / HTTP/1.1\r\nContent-Length: 1\r\n\r\n\r\n\r\n' | nc localhost 8080 
HTTP/1.1 200 OK
Date: Sun, 30 Sep 2012 12:41:31 GMT
Content-Length: 2303
Content-Type: text/html; charset=utf-8
Server: TwistedWeb/12.2.0+r35841

<html>
-----<chunk of html />-------
</html>
HTTP/1.1 400 Bad Request

Anyway, fixed \o/

Changed 9 years ago by Michele Orrù

Attachment: ticket6029.2.patch added

Changed 9 years ago by Michele Orrù

Attachment: ticket6029.2.2.patch added

comment:9 Changed 9 years ago by therve

Owner: Michele Orrù deleted

comment:10 Changed 9 years ago by therve

Keywords: review removed
Owner: set to therve

Looks great, I'll add a NEWS fragment and merge that.

comment:11 Changed 9 years ago by therve

Resolution: fixed
Status: newclosed

(In [36057]) Handle invalid Content-Length header in HTTPChannel

Author: maker Reviewers: exarkun, therve Fixes: #6029

Note: See TracTickets for help on using tickets.