[tor-bugs] #5262 [Stem]: Implement Safe Cookie in Stem

[Tor Bug Tracker & Wiki]

#5262: Implement Safe Cookie in Stem
-------------------------+--------------------------------------------------
 Reporter:  atagar       |          Owner:  gsathya       
     Type:  enhancement  |         Status:  needs_revision
 Priority:  normal       |      Milestone:                
Component:  Stem         |        Version:                
 Keywords:               |         Parent:                
   Points:               |   Actualpoints:                
-------------------------+--------------------------------------------------
Changes (by atagar):

  * status:  needs_review => needs_revision


Comment:

 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.

 2. An AuthChallengeResponse should be its own class, like
 ProtocolInfoResponse is. Don't forget tests!

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

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/5262#comment:5>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online