Opened 6 years ago

Closed 6 years ago

#5262 closed task (implemented)

Implement Safe Cookie in Stem

Reported by: atagar Owned by: neena
Priority: Medium Milestone:
Component: Core Tor/Stem Version:
Severity: Keywords:
Cc: neenaoffline@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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...

  1. Distill his script to just what we need to perform the authentication.
  1. 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.
  1. Write integration tests similar to the current auth cookie tests.

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!

Child Tickets

Change History (10)

comment:1 Changed 6 years ago by rransom

Parent ID: #5185

comment:2 Changed 6 years ago by neena

Cc: neenaoffline@… added

comment:3 in reply to:  description Changed 6 years ago by neena

Status: newneeds_review

Replying to atagar:

  1. 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.

  1. 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.

  1. Write integration tests similar to the current auth cookie tests.

My first attempt at this was 'wrong'. I'm doing this right now.

I was hoping to get my code reviewed once it was fully documented/tested.
Don't want to wait any longer, so, my code is here:

http://repo.or.cz/w/stem/neena.git/shortlog/refs/heads/safe-cookie

comment:4 Changed 6 years ago by atagar

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.

+class AuthChallengeFailed(AuthenticationFailure):
+ "AUTHCHALLENGE command has failed."

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).

+ elif auth_type == AuthMethod.SAFECOOKIE:
+ cookie_path = protocolinfo_response.cookie_path

Again, we can share with AuthMethod.COOKIE.

+ log.debug("The authenticate_cookie/authenticate_safecookie method raised...

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.

+def authenticate_safecookie(controller, cookie_path, suppress_ctl_errors = True):

Hmm, I keep going back and forth about if it would be better as 'authenticate_safecookie' or 'authenticate_safe_cookie' - thoughts?

comment:5 Changed 6 years ago by atagar

Status: needs_reviewneeds_revision

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'.

+ parsed_ac_response = re.match("AUTHCHALLENGE SERVERHASH=([A-Fa-f0-9]*) \
+SERVERNONCE=([A-Fa-f0-9]*)$", challenge_response.content()[0][2])

No, no, no, please don't parse responses like this via regexes. This is the path to the dark side.

Several things...

  1. 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.

  1. An AuthChallengeResponse should be its own class, like ProtocolInfoResponse is. Don't forget tests!
  1. 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
+ raise IncorrectCookieValue(str(auth_response), auth_response)
+ else:
+ raise CookieAuthRejected(str(auth_response), auth_response)

Nope. The first condition will receive both rejected authentication values and rejected auth types, for more information see...
https://trac.torproject.org/projects/tor/ticket/4817

That said, the earlier work that we've done has probably established that tor accepts SAFECOOKIE authentication...

+ n (int) - length of random string to be returned in bytes.

Lets go with "length" or "size" instead - it's more descriptive than "n".

+ return .join([chr(random.getrandbits(8)) for x in range(n)])

Is this source of random data alright for security purposes? (ie random vs urandom)

Cheers! -Damian

comment:6 Changed 6 years ago by neena

Owner: changed from gsathya to neena
Status: needs_revisionassigned

comment:7 Changed 6 years ago by neena

Status: assignedneeds_review

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.

http://repo.or.cz/w/stem/neena.git/shortlog/refs/heads/safe-cookie
and
http://repo.or.cz/w/stem/neena.git/shortlog/refs/heads/safe-cookie-alt

Replying to atagar:

+ |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?

Replying to atagar:

+ parsed_ac_response = re.match("AUTHCHALLENGE SERVERHASH=([A-Fa-f0-9]*) \
+SERVERNONCE=([A-Fa-f0-9]*)$", challenge_response.content()[0][2])

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.

comment:8 Changed 6 years ago by atagar

Status: needs_reviewneeds_revision

Hazaa! Looks great.

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

comment:9 Changed 6 years ago by neena

Status: needs_revisionneeds_review
Type: enhancementtask

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).

My branch is here - http://repo.or.cz/w/stem/neena.git/shortlog/refs/heads/safe-cookie-alt

comment:10 Changed 6 years ago by atagar

Resolution: implemented
Status: needs_reviewclosed

Hi Ravi. Merged with some revisions...
https://gitweb.torproject.org/stem.git/commitdiff/560923cb7b572d02046c6ca2bd5eb4502fa591b3

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!

Note: See TracTickets for help on using tickets.