#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
comment:2 follow-up: 3 Changed 8 years ago by
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 Changed 8 years ago by
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 Changed 8 years ago by
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
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
Milestone: | → Tor: 0.2.3.x-final |
---|---|
Owner: | set to nickm |
Status: | new → accepted |
comment:7 Changed 8 years ago by
Parent ID: | → #4933 |
---|
comment:8 Changed 7 years ago by
Milestone: | Tor: 0.2.3.x-final → Tor: 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
Keywords: | spec added |
---|
comment:10 Changed 7 years ago by
Keywords: | tor-relay added |
---|
comment:11 Changed 7 years ago by
Component: | Tor Relay → Tor |
---|
comment:12 Changed 7 years ago by
Resolution: | → fixed |
---|---|
Status: | accepted → closed |
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
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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:
- Should QuotedString also mention that single quotes, tabs, etc are escaped?
- 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
Keywords: | tor-spec added; spec removed |
---|---|
Milestone: | Tor: unspecified → Tor: 0.2.4.x-final |
comment:15 Changed 7 years ago by
Milestone: | Tor: 0.2.4.x-final → Tor: 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
Keywords: | 025-triaged added |
---|---|
Parent ID: | #4933 |
comment:18 Changed 6 years ago by
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
Keywords: | 025-triaged removed |
---|---|
Milestone: | Tor: 0.2.5.x-final → Tor: 0.2.6.x-final |
comment:20 Changed 5 years ago by
Keywords: | 026-triaged-1 026-deferrable added; easy removed |
---|---|
Priority: | trivial → minor |
comment:21 Changed 5 years ago by
Milestone: | Tor: 0.2.6.x-final → Tor: 0.2.??? |
---|
comment:22 Changed 5 years ago by
Resolution: | → duplicate |
---|---|
Status: | reopened → closed |
Closing in favor of #15000.
Oops, sorry - forgot trac has different preformatted blocks.