Opened 6 years ago

Last modified 3 years ago

#3453 defect new

twisted.web._auth.digest mostly ignores the value of the uri field

Reported by: exarkun Owned by: retenodus
Priority: high Milestone:
Component: web Keywords: security
Cc: Branch:
Author: Launchpad Bug:

Description

While this digest auth implementation does use the uri value to compute the correct challenge response (as it must), it does nothing to comply with the requirements in section 3.2.2.5 of RFC 2617, reproduced here:

The authenticating server must assure that the resource designated by the "uri" directive is the same as the resource specified in the Request-Line; if they are not, the server SHOULD return a 400 Bad Request error. (Since this may be a symptom of an attack, server implementers may want to consider logging such errors.) The purpose of duplicating information from the request URL in this field is to deal with the possibility that an intermediate proxy may alter the client's Request-Line. This altered (but presumably semantically equivalent) request would not result in the same digest as that calculated by the client.

Attachments (2)

3453.patch (1.5 KB) - added by retenodus 3 years ago.
3453.2.patch (4.5 KB) - added by retenodus 3 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 4 years ago by jesstess

  • Owner changed from jknight to jesstess

comment:2 Changed 4 years ago by <automation>

  • Owner jesstess deleted

comment:3 Changed 3 years ago by exarkun

  • Keywords security added

Changed 3 years ago by retenodus

comment:4 Changed 3 years ago by retenodus

  • Keywords review added

RFC 2617 says that headers of authorization request uri related are :

digest-uri = "uri" "=" digest-uri-value
digest-uri-value = request-uri ; As specified by HTTP/1.1

comment:5 Changed 3 years ago by therve

  • Keywords review removed
  • Owner set to retenodus

There are several problems with that patch. The biggest one being that it doesn't have tests, so it's hard to tell if it works or not. For the behavior, I believe that the change in getChallenge is unnecessary. For the other case, I don't think setting the response code on the request is enough: you should probably raise an exception and catch it in HTTPAuthSessionWrapper to render an ErrorPage. It would be nice to log something, too.

Thanks!

Changed 3 years ago by retenodus

comment:6 Changed 3 years ago by retenodus

  • Keywords review added

In the last patch I've added unittests (if the header "digest-uri" matches "request.uri" it works. Else it doesn't) and I use the wrapper to render an error page and log.

comment:7 Changed 3 years ago by retenodus

  • Owner retenodus deleted

comment:8 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to retenodus

Thanks for working on this. The test coverage is still not quite what it needs to be. The failure case, where the Digest-URI does not match the Request-URI, is a different case - it should be tested in a different test method. The idea is what you did in test_decode, but it needs to be kept separate from the existing test_decode. The new method should have a name and docstring reflecting the new case.

Also:

  1. Don't use try/except:pass - use TestCase.assertRaises.
  2. There needs to be a test for the wrapper.py change
  3. However, I don't think the wrapper needs to log the full traceback. It could log that there was a URI mismatch and maybe some other information about the request, but the stack doesn't provide any extra useful information.
  4. The twisted.web.test.test_web change puts None into the fake request headers for most cases. This is bogus - it should leave headers empty if there is no digest-uri. Also, doing this in FakeRequest seems like a mistake. Instead, have digest auth tests populate the header - perhaps in DigestAuthTestCase.setUp.

Thanks again. Sorry about the review delay. I'll be working on Twisted all this week and next, so anything that goes up for review in that time will be looked at quickly. :)

Note: See TracTickets for help on using tickets.