Opened 8 years ago

Closed 5 years ago

Last modified 3 years ago

#4600 closed defect (duplicate)

Spec doesn't mention password quotes

Reported by: atagar Owned by: nickm
Priority: Low Milestone:
Component: Core Tor/Tor Version:
Severity: Keywords: tor-spec tor-relay 026-triaged-1 026-deferrable
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Section 5.1 of the control-spec [1] provides a nice description of authentication, but doesn't mention how to handle quotes in the password. Unsurprisingly controllers are expected to provide escaped quotes...

<pre>
atagar@morrigan:~$ tor --hash-password "this has a \" in it"

16:E6DC1BCEDF55EDCA607ADDB8781795772E42AAC75F7B7630B6227232E4

atagar@morrigan:~$ telnet localhost 9051

Connected to localhost.
AUTHENTICATE "this has a \" in it"
250 OK

</pre>

I'm gonna guess that only quotes should be escaped by controllers.

I've been finding it a little frustrating to figure out when and what escaping is expected so I'm generally working from the assumption that I should ignore escaping unless specifically called out by the spec (like it is for authentication cookie paths, though that wasn't enough to work from alone [2]).

Cheers! -Damian

[1] https://gitweb.torproject.org/torspec.git/blob/HEAD:/control-spec.txt#l1924
[2] https://gitweb.torproject.org/stem.git/blob/HEAD:/stem/socket.py#l54

Child Tickets

Change History (24)

comment:1 Changed 8 years ago by atagar

Oops, sorry - forgot trac has different preformatted blocks.

comment:2 Changed 8 years ago by nickm

Is this something you can write a spec patch for?

I see the relevant functions as being the ones with names like *_escaped* in control.c. They're called by conrol_setconf_helper, handle_control_authenticate, handle_control_getinfo, handle_control_postdescriptor, control_event_or_authdir_new_descriptor, and conrol_event_networkstatus_changed_helper.

comment:3 in reply to:  2 Changed 8 years ago by nickm

Replying to nickm:

Is this something you can write a spec patch for?

If it isn't, I'd have an easier time if you can rephrase the bug in terms of a list of questions that you want the spec to answer. I get that AUTHENTICATE should document when it accepts quoted strings, -- what else?

comment:4 in reply to:  description Changed 8 years ago by rransom

Replying to atagar:

I've been finding it a little frustrating to figure out when and what escaping is expected so I'm generally working from the assumption that I should ignore escaping unless specifically called out by the spec (like it is for authentication cookie paths, though that wasn't enough to work from alone [2]).

[2] https://gitweb.torproject.org/stem.git/blob/HEAD:/stem/socket.py#l54

esc_for_log is the only function Tor uses to escape strings sent to the control port. Every quoted string Tor emits on its control port should be quoted (and escaped) by a call to esc_for_log.

Unrelated: getinfo_helper_listeners is not dead code; a macro expands listeners in some ‘column’ of the list of GETINFO items (and the helpers that handle them) to getinfo_helper_listeners, and then the function that handles GETINFO commands calls it through a function pointer in the GETINFO item array.

comment:5 Changed 8 years ago by atagar

Is this something you can write a spec patch for?

I can write spec patches for what I think it should be if you'd like.

I'd have an easier time if you can rephrase the bug in terms of a list of questions that you want the spec to answer

My first thought when I saw that AUTHENTICATE took a quoted passphrase was "Hm, guess the passphrase can't contain quotes. Hopefully --hash-password gives an error when users try to create hashed passphrases with quotes.". After a bit of experimenting my next thought was "Uhh, are quotes the only escaped value?".

I'd like the spec to specify...

  • when and what escaping is expected for requests
  • when and what unescaping should be applied to replies
  • a note that says that if there's escaping outside of this then it's either a tor or spec bug

Cheers! -Damian

comment:6 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-final
Owner: set to nickm
Status: newaccepted

comment:7 Changed 8 years ago by asn

Parent ID: #4933

comment:8 Changed 7 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: unspecified

I'd still take a spec patch for this, but it apparently isn't happening in 0.2.3. Fortunately , the spec isn't tied to the tor release cycle.

comment:9 Changed 7 years ago by nickm

Keywords: spec added

comment:10 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:11 Changed 7 years ago by nickm

Component: Tor RelayTor

comment:12 Changed 7 years ago by nickm

Resolution: fixed
Status: acceptedclosed

I think I got a fix for this in 9716716527fc5e8047b57f14870afbb3c864a197. The issue was that QuotedString wasn't actually defined, I think.

comment:13 Changed 7 years ago by atagar

Resolution: fixed
Status: closedreopened

Hi Nick. Sorry about re-opening this old ticket but while looking at #8471 I realized that 9716716 doesn't match esc_for_log. Presently stem escapes the characters mentioned by esc_for_log.

Two questions:

  1. Should QuotedString also mention that single quotes, tabs, etc are escaped?
  2. rransom mentioned that tor only does escaping in a few specific places, are you sure that it's appropriate to say that all QuotedStrings are escaped?

comment:14 Changed 7 years ago by nickm

Keywords: tor-spec added; spec removed
Milestone: Tor: unspecifiedTor: 0.2.4.x-final

comment:15 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

Deferring to 0.2.5 after discussion with atagar: making the metaformat more uniform in 0.2.5 will make this stuff far far easier to document.

comment:16 Changed 6 years ago by nickm

Keywords: 025-triaged added
Parent ID: #4933

comment:17 Changed 6 years ago by andrea

Triage for 0.2.5: keep and document as-is

comment:18 Changed 6 years ago by nickm

This is actually nontrivial since as far as I can tell there are two single-line escaped-string formats here: The one that control.c generates (which uses esc_for_log), and the one we accept (which uses decode_escaped_string). I don't think control-spec does a great job distinguishing between them: it seems to call them both QuotedString, which is just wrong.

comment:19 Changed 6 years ago by andrea

Keywords: 025-triaged removed
Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final

comment:20 Changed 5 years ago by nickm

Keywords: 026-triaged-1 026-deferrable added; easy removed
Priority: trivialminor

comment:21 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.???

comment:22 Changed 5 years ago by nickm

Resolution: duplicate
Status: reopenedclosed

Closing in favor of #15000.

comment:23 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:24 Changed 3 years ago by nickm

Milestone: Tor: 0.3.???

Milestone deleted

Note: See TracTickets for help on using tickets.