[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #5262 [Stem]: Implement Safe Cookie in Stem
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs