Opened 2 years ago

Closed 2 months ago

#4817 closed defect (wontfix)

Control port authentication failures don't differentiate failure types

Reported by: atagar Owned by:
Priority: trivial Milestone: Tor: 0.2.5.x-final
Component: Tor Version:
Keywords: easy maybe-proposal tor-relay Cc:
Actual Points: Parent ID:
Points:

Description

The control-spec has a couple status codes for authentication failures...

514 Authentication required
515 Bad authentication

The 515 status is used for both rejection of the authentication credentials and of the authentication method entirely. For instance...

Rejected Credentials:

AUTHENTICATE "blarg"
515 Authentication failed: Password did not match HashedControlPassword value from configuration
Connection closed by foreign host.

Rejected Authentication Method:

AUTHENTICATE
515 Authentication failed: Password did not match HashedControlPassword value from configuration. Maybe you tried a plain text password? If so, the standard requires that you put it in double quotes.
Connection closed by foreign host.

This means that controllers need to read the message to translate these responses into an exception type. Needless to say this isn't great since it leads to sadness if we change or provide localization of the messages.

In stem's case I provide a warning in the pydocs and attempt to use the message...

"""
Authenticates to a control socket that uses a password (via the
HashedControlPassword torrc option). Quotes in the password are escaped.

If authentication fails tor will disconnect and we'll make a best effort
attempt to re-establish the connection. This may not succeed, so check
is_alive() before using the socket further.

For general usage use the authenticate() function instead.

note: If you use this function directly, rather than authenticate(), we may
mistakenly raise a PasswordAuthRejected rather than IncorrectPassword. This
is because we rely on tor's error messaging which is liable to change in
future versions.
"""

...

# if we got anything but an OK response then error
if str(auth_response) != "OK":
  try: control_socket.connect()
  except: pass
  
  # all we have to go on is the error message from tor...
  # Password did not match HashedControlPassword value value from configuration...
  # Password did not match HashedControlPassword *or*...
  
  if "Password did not match HashedControlPassword" in str(auth_response):
    raise IncorrectPassword(str(auth_response), auth_response)
  else:
    raise PasswordAuthRejected(str(auth_response), auth_response)

https://gitweb.torproject.org/stem.git/blob/HEAD:/stem/connection.py#l468

I do this for both the authenticate_password and authenticate_cookie functions. In general this won't be very visible to library users since they'll usually use the authenticate() function instead, which has a PROTOCOLINFO response so it doesn't have this issue.

Feel free to resolve this as 'wont fix'. This isn't an issue if controller users do PROTOCOLINFO first, and the AUTHENTICATE function has no notion of the authentication type being attempted so it would be difficult for tor to differentiate those issues, even if it wanted to. Mostly just noting a minor gotcha I encountered while writing stem. :)

Cheers! -Damian

Child Tickets

Attachments (2)

0001-Returning-514-error-if-empty-AUTHENTICATE-query-is-r.patch (1.8 KB) - added by rl1987 6 months ago.
0002-Changes-file.patch (617 bytes) - added by rl1987 6 months ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 2 years ago by nickm

  • Milestone set to Tor: 0.2.4.x-final

WOrth fixing. I'd take a good patch for 0.2.3.x if it arrives soon, but 0.2.4.x is likelier.

comment:2 Changed 19 months ago by arma

  • Summary changed from Auth failures don't differentiate failure types to Control port authentication failures don't differentiate failure types

comment:3 Changed 19 months ago by arma

  • Keywords easy added

comment:4 Changed 19 months ago by nickm

  • Keywords maybe-proposal added

comment:5 Changed 19 months ago by nickm

  • Keywords tor-relay added

comment:6 Changed 19 months ago by nickm

  • Component changed from Tor Relay to Tor

comment:7 Changed 13 months ago by nickm

  • Milestone changed from Tor: 0.2.4.x-final to Tor: 0.2.5.x-final

comment:8 Changed 6 months ago by rl1987

  • Status changed from new to needs_review

Please review the patch I have uploaded.

Changed 6 months ago by rl1987

comment:9 Changed 2 months ago by nickm

I tweaked this in my branch "feature4817" in my public repository so that it would work.

I'm not sure of whether to merge this, though. Basically, this patch is equivalent to having the controller do, "if you sent no authentication and you get a 515, assume that you were supposed to send authentication." Controller writers, is it IYO worth fixing?

comment:10 Changed 2 months ago by atagar

Hi Nick. Now that I've thought about it some more what I was hoping for from stem's perspective was different status codes for "incorrect auth value" verses "incorrect auth type".

Here's how tor responds to each auth type (except no auth since that's uninteresting as everything's accepted);

==============================

Using Password Auth:

# (A) Attempt No Auth

AUTHENTICATE
515 Authentication failed: Password did not match HashedControlPassword value from configuration. Maybe you tried a plain text password? If so, the standard requires that you put it in double quotes.

# (B) Attempt Password Auth (wrong value)

AUTHENTICATE "my password"
515 Authentication failed: Password did not match HashedControlPassword value from configuration

# (C) Attempt Cookie Auth

AUTHENTICATE b4c9e2effc93bbbf139dcc5c0fc15d0b890a9e7bf7c8bb49b1d34c2eb547c910
515 Authentication failed: Password did not match HashedControlPassword value from configuration. Maybe you tried a plain text password? If so, the standard requires that you put it in double quotes.

Using Cookie Auth:

# (D) Attempt No Auth

AUTHENTICATE
515 Authentication failed: Wrong length on authentication cookie.

# (E) Attempt Password Auth

AUTHENTICATE "my password"
515 Authentication failed: Wrong length on authentication cookie.

# (F) Attempt Cookie Auth (wrong value)

AUTHENTICATE b4c9e2effc93bbbf139dcc5c0fc15d0b890a9e7bf7c8bb49b1d34c2eb547c910
515 Authentication failed: Authentication cookie did not match expected value.

==============================

From my perspective it would be nice if situations 'B' and 'F' had a distinct status code from the rest (rather than everything returning a 515). Stem presently parses the response message to differentiate those cases so we can raise the appropriate exception.

I realize that this is different from the original ask in the ticket which concerned the response when no credentials are provided. Again, feel free to resolve as 'wontfix', this is a very minor nit pick and doesn't apply if controllers check the PROTOCOLINFO first.

comment:11 Changed 2 months ago by nickm

So, (B) and (C) are not technically distinct: You are allowed to specify a password in hex, and passwords are allowed to be 32 bytes long. So you can also say: AUTHENTICATE 6d792070617373776f7264 instead of AUTHENTICATE "my password".

So if a controller says AUTHENTICATE b4c9e2effc93bbbf139dcc5c0fc15d0b890a9e7bf7c8bb49b1d34c2eb547c910, we don't actually know that they're providing a cookie rather than providing a very strange password and encoding it in hex.

Similarly, (E) and (F) are not totally distinct: passwords are allowed to be 32 characters, and users are allowed to send control-cookies as C-encoded strings if they choose.

In other words, we can't actually determine which kind of authentication the controller was trying to send. We know that non-32-byte fields are never cookies, and that's about it. We can *guess* that 32-byte hex-encoded things are usually cookies, but that's only a heuristic.

Is this still worth doing IYO?

comment:12 Changed 2 months ago by atagar

  • Resolution set to wontfix
  • Status changed from needs_review to closed

Is this still worth doing IYO?

Ahhh, gotcha. Thanks for the explanation! Nope, this probably isn't worth keeping open - resolving.

Note: See TracTickets for help on using tickets.