DigestedCredentials does not prevent reuse of nonce

[JohnDoeee]

twisted.cred.credentials.DigestedCredentials does not check if a nonce has already been used and thereby allows for Replay Attacks.

[JohnDoeee]

Adds nonce duplicate verification (against reuse, not if the nonce is generated by twisted). Adds a cleanup mechanism to the used nonces as they cannot be used after CHALLENGE_LIFETIME_SECS anyways.

Fixes up the tests by removing the static nonce, removes the multiResponse test as it sends in an invalid nonce.

I am unable to think of a situation where the multiResponse is necessary.

[exarkun]

Thanks for pointing out this weakness, JohnDoeee, and thanks for working on a patch to fix it.

1. Regarding the unit tests,
   1. Since the server previously ignored the "nc", I think it's okay to drop what minimal "support" we had for it, at least for now. Perhaps in the future we can introduce proper support for that feature (described in 3.2.2 and 3.2.3 of  RFC 2617). Such support is desirable because nonce-count allows pipelining; lack of nonce-count prevents it.
   2. It's a bit non-obvious why the code should not bother to remember nonces it has given out, but then remember which nonces get used successfully. It did eventually dawn on me that this avoids making the server vulnerable to resource exhaustion attacks by arbitrary clients (instead limiting the scope of this vulnerability to clients with credentials in the system - which seems maybe okay to me; revoke their credentials if they're abusing the service). I think the code would benefit from a comment explaining this security property, so future maintainers don't need to be so clever to understand what's going on. :)
   3. The docstring for test_nonceTimeoutCleanup explains a different test method, not that one.
   4. The way that test_nonceTimeoutCleanup tests for old state being cleaned up is pretty subtle. It also doesn't seem quite complete to me. The test will pass as long as things are deleted from nonces, even if they are left in nonceTimestamps. I think direct assertions against the state we know we're keeping would be a good approach here.
2. And considering the implementation,
   1. I think that nonces and nonceTimestamps should be private, at least for now. To properly support nonce-count I think we'll want to adjust their shape slightly. Not advertising this as a public API now will make it easier to make those changes later.
   2. They should still both be documented in the DigestCredentialFactory class docstring, though, to help out future maintainers.
   3. When nonceTimestamps is added to, the then-current time is added along with the nonce. This makes expiration occur at (nonce usage + delay) rather than (nonce creation + delay). The latter would seem slightly better to me: (nonce creation + delay) is the point after which the nonce is no longer valid, after all? Some unit tests exercising the boundary conditions for this behavior might be nice.
3. There's also some trailing whitespace added by the patch. We try to avoid that.

Thanks very much for your efforts towards improving this area of Twisted! Please let me know if any part of this review is unclear. I'm looking forward to the next version of the patch.

[JohnDoeee]

Thanks for the feedback, I tried to follow guidance as much as possible.

I went ahead and added pipeline support, therefore this patch is fundamentally different from the previous patch.

Regarding 2.2 (and 1.2): The nonce must be invalidated at or after the opaque is invalid and is part of the thoughts behind my design choice. The other part is that the nonce is only saved when it is used in a valid request. This is how  https://tools.ietf.org/html/draft-ietf-oauth-v2-http-mac-01 is designed and therefore I must assume the choice is secure.

This is, of course, noted in the documentation.

A little sidenote: as nc is actually used now as a counter, I added a missing check for qop and nc/cnonce (as the RFC states).

[dreid]

Easy Stuff:

1. pyflakes

   twisted/test/test_digestauth.py:650: local variable 'nonceOpaque' is assigned to but never used
   

2. Trailing whitespace
   1. _verifyNonce docstring has trailing whitespace (two of the blanks are just 8 spaces.)
   2. test_reuseNonce has trailing whitespace
3. test_nonceCounterOutOfOrder docstring is just "FIXME" it should describe what behavior is being tested
4. There should be 2 blank lines between methods.
5. There should be a newline at the end of test_digestauth.py
6. It is probably valuable to verify that high hex nonce counts are handled correctly.
7. The struct module isn't necessary for converting the hex nonce.

               nc, = struct.unpack('!I', nc.decode('hex')) 
   

could just as easily be

            nc = int(nc, 16)

and encoding can be:

"%08x" % (nc,)

Harder Stuff:

1. You could remove FakeDigestCredentialFactory by making the real DigestCredentialFactory take private keyword arguments.
   1. _time=time.time
   2. _privateKey=None and if _privateKey is None: _privateKey=secureRandom(12)
      1. If privateKey is actually public it might make sense to just set the attribute after instantiating the DigestCredentialFactory. Or atleast make the keyword argument public. There is a use case where you may need to have multiple servers which share a privateKey.

Thank you for working on this part of Twisted. Digest auth implementations tend to be rather forgiving so it's nice to see a more correct implementation.