Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#2893 closed defect (fixed)

Tor controller requires space in 'authenticate' command

Reported by: arma Owned by: Sebastian
Priority: Low Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: easy tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

arma@last-request:~/old/torgit/torspec/proposals$ telnet localhost 9051
Trying ::1...
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
authenticate
250 OK
quit
250 closing connection
Connection closed by foreign host.
arma@last-request:~/old/torgit/torspec/proposals$ nc localhost 9051
authenticate
551 Invalid quoted string.  You need to put the password in double quotes.
arma@last-request:~/old/torgit/torspec/proposals$ nc localhost 9051
authenticate ""
250 OK
quit
250 closing connection

Setting up logging on Tor's side in handle_control_authenticate():

 log_notice(LD_CONTROL, "'%s'", body);

in the first case I get

Apr 11 16:39:13.633 [notice] '
'

and in the second case I get

Apr 11 16:39:23.258 [notice] ''

It looks like that function is checking

  } else if (TOR_ISSPACE(body[0])) {
    password = tor_strdup("");
    password_len = 0;
  }

So Tor demands that you have some sort of whitespace before your newline. That runs counter to our spec:

  Wherever CRLF is specified to be accepted from the controller, Tor MAY also
  accept LF.  Tor, however, MUST NOT generate LF instead of CRLF.
  Controllers SHOULD always send CRLF.

Child Tickets

Change History (14)

comment:1 Changed 8 years ago by arma

Status: newneeds_review

bug2893 in my arma fixes this.

comment:2 Changed 8 years ago by arma

Actually, is bug2893-part2 a better fix?

comment:3 Changed 8 years ago by Sebastian

Yes, part2 is a better fix.

But looking at this, it seems weird that we pass in something where the first character could be a whitespace at all. I think the caller should probably eat it?

comment:4 Changed 8 years ago by arma

Yes, that sounds fine too.

Best to make sure that a change we make in fetch_from_buf_line() is wanted in the other places that call it too.

comment:5 Changed 8 years ago by nickm

Milestone: Tor: 0.2.2.x-final

comment:6 Changed 8 years ago by nickm

Owner: set to Sebastian
Status: needs_reviewassigned

(looks like we're waiting on a revised patch here; reassigning pack to sebastian)

comment:7 Changed 8 years ago by nickm

Priority: normalminor

comment:8 Changed 8 years ago by arma

Milestone: Tor: 0.2.2.x-finalTor: 0.2.3.x-final

comment:9 Changed 8 years ago by Sebastian

Status: assignedneeds_review

branch bug2893 in my repo

comment:10 Changed 8 years ago by nickm

Looks good at first glance. Could you have a quick look over the other TOR_ISSPACE users in control.c to make sure they're safe with this new behavior, and see if any of them have become redundant?

comment:11 Changed 8 years ago by Sebastian

Oh, I guess I should've said so from the beginning. I did make a pass through the control_X_helper functions and didn't spot any more redundancies/problems.

comment:12 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merging! Thanks!

comment:13 Changed 7 years ago by nickm

Keywords: tor-client added

comment:14 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.