Ticket for tracking the work to implement and test Robert's new Safe Cookie authentication method in stem.
Robert has written a python script to handle the authentication so this task is to...
Distill his script to just what we need to perform the authentication.
Implement safe cookie in the connection module. This involves adding SAFE_COOKIE to the AuthMethod enum, adding a new 'authenticate_safe_cookie' function, and adding this to the 'authenticate' method.
The safe cookie authentication method has not been merged into tor and, until it is, we'll be keeping this feature in a separate branch.
Part of the safe cookie proposal was the deprecation and removal of the previous authentication cookie method. Stem should include this deprecation notice in its pydocs and we should add the upcoming deprecation to the tor workaround deprecation section so we remember to remove authentication support later (otherwise the vulnerability Robert is trying to fix will still exist).
At the moment gsathya has offered to help by taking the first pass at this - good luck!
To upload designs, you'll need to enable LFS and have an admin enable hashed storage. More information
Child items
0
Show closed items
No child items are currently assigned. Use child items to break down this issue into smaller parts.
Linked items
0
Link issues together to show that they're related.
Learn more.
Distill his script to just what we need to perform the authentication.
Done. I'm a little concerned if I have inadvertently compromised on security somewhere.
Implement safe cookie in the connection module. This involves adding SAFE_COOKIE to the AuthMethod enum, adding a new 'authenticate_safe_cookie' function, and adding this to the 'authenticate' method.
Done, somewhat. I have to document it properly. I've used authenticate_cookie as a starting point for this.
Review isn't complete, these are just some thoughts I've scribbled down while waiting for a movie to start. Seems better to send this along now rather than waiting until I'm done. I'll try to finish the code review tomorrow morning.
As always please push back if you disagree on anything!
| |- InvalidClientNonce - The client nonce is invalid.
Are you sure that we want to reuse CookieAuthFailed exceptions for safecookie authentication? If it works then that's certainly preferable (since it simplifies the exception hierarchy), but I wonder if it'll confuse authenticate() callers since they then won't know if an exception is from a cookie or safecookie authentication attempt.
One option would be for CookieAuthFailed to have an attribute that says if it was from a cookie or safecookie failure, but I'm not sure if that's cleaner or more confusing than having a separate branch of exceptions. At present I'm favoring it a bit since normal cookie authentication is slated for deprecation...
+import re
Trivial detail but imports in stem are ordered by length of the module name. Yea, it's arbitrary - I just did it that way cuz it looks nice. I could be persuaded that alphabetical or something else is better.
Looks like these fell out of sync with the header pydocs. Is there a reason that these are a AuthenticationFailure subclass rather than CookieAuthFailed? They only arise during cookie auth, no?
MissingAuthInfo,
Why are you dropping this exception type? Is it somehow impossible for authenticate() to raise these?
AuthChallengeFailed,
AuthSecurityFailure,
InvalidClientNonce,
NoAuthCookie
The AuthChallengeFailed entry negates the need for AuthSecurityFailure and InvalidClientNonce since it's the parent class. Also, the ordering doesn't match the authenticate() function's pydocs.
Admittedly this might be a little confusing - here's how AUTHENTICATE_EXCEPTIONS is supposed to work:
The authenticate() function might try multiple methods of authenticating to tor, and hence have multiple exceptions that it could raise to the caller. The AUTHENTICATE_EXCEPTIONS listing is used to pick which exception has the highest priority. This listing of priorities are stated in the authenticate() function's pydocs.
Feel free to beef up the commenting to help future devs in understanding the code - a fresh pair of eyes are good for that. :)
if AuthMethod.SAFECOOKIE in auth_methods and protocolinfo_response.cookie_path is None:
auth_methods.remove(AuthMethod.SAFECOOKIE)
auth_exceptions.append(NoAuthCookie("our PROTOCOLINFO response did not have the location of our
Might be worthwhile joining this with the below AuthMethod.COOKIE check since they're checking for the same thing (missing a path during some form of cookie auth).
If we had a flag in exceptions for if it's from cookie or safe cookie auth then we could customize this.
log.debug("The authenticate_safecookie method raised an \
+AuthSecurityFailure. This could be an attack. (%s)" % exc)
Please don't try overly hard to meet a certain line length limit. It's good if we can write our code so the length of lines is short, but a line like this is more readable as a single line.
Hi Ravi. Looks great! This will need some more work, but it's definitely on the right track.
"""
See authenticate_cookie.
...
""" # TODO: Add?
Agreed that there's a lot of overlap in the pydocs with cookie auth, but please give at least a basic summary and populate the Arguments/Raises sections. It's fine if you say "See authenticate_cookie for more information." to avoid repeating what's already there.
We also need to explain the precedence of the exceptions you're adding against each other and the other cookie authentication exceptions.
auth_cookie_size = os.path.getsize(cookie_path)
if auth_cookie_size != 32:
exc_msg = "Authentication failed: authentication cookie '%s' is the wrong
+size (%i bytes instead of 32)" % (cookie_path, auth_cookie_size)
raise IncorrectCookieSize(exc_msg, cookie_path)
Maybe we should refactor the common bits into helper functions?
challenge_response = controller.recv()
A minor gotcha that you'll encounter when you rebase onto the current master is that the connection module now accepts a ControlSocket or BaseController. Doing so is trivial - you'll just want to replace send/recv calls with the added _msg() function.
if challenge_response.content()[0][0] != "250":
Hmm, we should probably add an 'is_ok()' method to control messages. You can leave this alone if you'd like and I'll add it in later along with other useful message statuses.
if "AUTHCHALLENGE only supports" in str(challenge_response):
You're doing lots 'o checks against the message's string representation so might as well assign it to a 'challenge_response_str'.
No, no, no, please don't parse responses like this via regexes. This is the path to the dark side.
Several things...
Please read what the spec allows for (can they key=value mappings be reordered? can new ones be added? what is valid and invalid for each field?). This means both checking what future expansion tor is allowed to do, and what the spec says is invalid.
Try to think of every crazy edge case that you can, both for a valid and invalid response - these are the things that your parser should not only handle, but we should add as unit tests. Really try to think of ways of breaking your parser - that's the only way we'll make a good one.
Remember that stem not only needs to accept any valid responses that tor sends our way to function as a reliable library, but it also needs to check for anything that can constitute a bad response so it can act as a validator to test tor.
An AuthChallengeResponse should be its own class, like ProtocolInfoResponse is. Don't forget tests!
Use the ControlLine. It is your friend and, if it isn't your friend, then rewrite it so it is. You'll be needing it a lot for the controller. It hasn't been used much in practice yet (thus far just for PROTOCOLINFO which has some oddities in what is invalid), so there could easily be room for improvement.
The goal of the ControlLine is to make your job of parsing tor responses easier, both making the code more elegant and more easily conform with the future expansion that tor allows for.
Here's an example (probably can be improved)...
for line in challenge_response: if line == "OK": break elif line.is_empty(): continue # blank line line_type = line.pop() if line_type == "AUTHCHALLENGE": server_hash, server_nonce = None, None while not line.is_empty(): if not line.is_next_mapping(): raise Something("That's weird, should this line only have key=value mappings?") key, value = auth_challenge_line.pop_mapping() if key == "SERVERHASH": if not re.match("^[A-Fa-f0-9]*$", value): raise Something("SERVERHASH had an invalid value: %s" % value) server_hash = binascii.a2b_hex(value) elif key == "SERVERNONCE": if not re.match("^[A-Fa-f0-9]*$", value): raise Something("SERVERNONCE had an invalid value: %s" % value) server_nonce = binascii.a2b_hex(value) if not server_hash or not server_nonce: # are these required attributes?
raise AuthSecurityFailure("Server nonce has wrong length -- attack?")
This is tor (or thing-pretending-to-be-tor) provided data. Why would an incorrect size be perceived as an attack rather than simply being malformed?
if auth_response.contents()[0][0] != "515": #Cookie doesn't match
I am yet to complete writing the tests, though, that might change before you review this.
I have two branches, one with a seperate tree of exceptions, and one which shares the cookie authentication exceptions with an additional attribute added to it for storing the authentication method used.
| |- InvalidClientNonce - The client nonce is invalid.
Are you sure that we want to reuse CookieAuthFailed exceptions for safecookie authentication? If it works then that's certainly preferable (since it simplifies the exception hierarchy), but I wonder if it'll confuse authenticate() callers since they then won't know if an exception is from a cookie or safecookie authentication attempt.
One option would be for CookieAuthFailed to have an attribute that says if it was from a cookie or safecookie failure, but I'm not sure if that's cleaner or more confusing than having a separate branch of exceptions. At present I'm favoring it a bit since normal cookie authentication is slated for deprecation...
I thought having seperate exceptions would be a good idea. But, now, after implementing both, I think having an attribute with the authentication method is quite neat.
Hmm, I keep going back and forth about if it would be better as 'authenticate_safecookie' or 'authenticate_safe_cookie' - thoughts?
I don't really mind either. Shall I change it?
No, no, no, please don't parse responses like this via regexes. This is the path to the dark side.
Yeah, I had a feeling I was doing something terribly wrong here.
I thought having seperate exceptions would be a good idea. But, now, after implementing both, I think having an attribute with the authentication method is quite neat.
Agreed. Lets go with that one.
Glad to see those changes to ControlMessage, definitely makes it more usable.
As mentioned on IRC, we're probably getting close enough to merging that I'd rather wait to do a real review when we have the tests. That said, what I'm seeing here looks great! -Damian
I have added the tests and converted the documentation to follow the new format.
Something about the changes in test.unit.connection.authentication is making running the unit tests take more than 6x the time it took before. Unless someone tells me what's wrong, I'm putting it off for later (want to catch up on my gsoc backlog).
This ended up taking me a lot of time to revise, which in retrospect I should have expected since it's a big patch. The one main gotcha that I think we could have avoided was to make sure your tests passed. I suspect that you ran the integ tests, but forgot to run with the 'RUN_ALL' target causing your authenticate_safecookie() additions to be partly unexercised.
That aside, a fantastic addition. Thanks!
Trac: Resolution: N/Ato implemented Status: needs_review to closed