[tor-bugs] #17568 [Tor Browser]: Clean up tor-control-port.js in Torbutton
Tor Bug Tracker & Wiki
blackhole at torproject.org
Fri Nov 13 00:31:01 UTC 2015
#17568: Clean up tor-control-port.js in Torbutton
-------------------------------------------------+-------------------------
Reporter: gk | Owner: tbb-
Type: task | team
Priority: Medium | Status:
Component: Tor Browser | needs_review
Severity: Normal | Milestone:
Keywords: tbb-torbutton, | Version:
TorBrowserTeam201511R | Resolution:
Parent ID: | Actual Points:
Sponsor: | Points:
-------------------------------------------------+-------------------------
Changes (by arthuredelstein):
* keywords: tbb-torbutton => tbb-torbutton, TorBrowserTeam201511R
* status: new => needs_review
Comment:
Here's a new patch for review, where I tried to address all of issues
mentioned in the above comments:
https://github.com/arthuredelstein/torbutton/commit/17568
Please see my replies below for more details.
(It probably makes sense to leave this ticket open, even if we land the
first patch.)
----
Replying to [comment:1 arthuredelstein]:
> Some useful comments, from
> https://trac.torproject.org/projects/tor/ticket/16990#comment:6
>
> > Why the non-greedy globbing (the "?" modifier added to "+" or "*")
when you are matching all the rest of the line anyway?
> >
> > Compare:
> >
> > `520 - let matchResult = string.match(/^250[ -].+?=(.*?)$/mi) ||`
> > `520 + let matchResult = string.match(/^250[ -].+?=(.*)$/mi) ||`
> >
> > You do this in several other places in this file. Just wondering, I
don't think it makes a difference.
Fixed here. It's important that I turned off the multiline flag as well
here, so we don't accidentally match multiple lines.
> > Also, based on the regex above, shouldn't the comment in line 515 be
like so?
> >
> > {{{
> > 515 - // or `250 circuit-status...`
> > 515 + // or `250 circuit-status=...`
> > }}}
Fixed.
> > To match the rest of the comment.
> >
> > Also, I do not know what the assumptions/pre-conditions are on the
input string, but you do not check that the first line of the multiline
case (`"^250\+"`) ends with "=", as exemplified in the commentary for
"info.keyValueStringsFromMessage".
I've added an end-of-line marker "$" following the "=".
----
Replying to [comment:3 cypherpunks]:
> Why the case-insensitive flag ("i") when the pattern does not contain
any alphabetic character?
Fixed.
> Seems like you should also drop the multiline flag ("m") when you are
only trying to match a single-line reply.
Done.
> Aside:
>
> I was trying to track the input back to Tor's output and stumbled across
the 6500-lines control.c... So what I was wondering was:
> - In general, what is the threat expectation here? What has to be
considered adversary-controlled input?
> - Is it worth re-implementing the full control protocol parser in JS so
that you can verify each reply?
> - Hopefully control.c takes a good defensive parsing approach. Does
control.c offer some guarantees about its output so that JS can just rely
on it?
This is a good point worth considering. What do you consider to be a full
control protocol parser.
Another possibility is to consider whether the Tor control port should
switch to JSON or something similar to simplify code at both ends.
----
Replying to [comment:4 gk]:
> Here are some things I thought about while reading the code in tor-
control-port.js. I am confused about other things(, too) (especially while
looking at the spec + the controller.c file) but these might be more for a
second clean-up (after I had more time to sort them out).
>
> 1)
> {{{
> // GETCONF with a single argument returns results that look like
> // results from GETINFO with multiple arguments."
> }}}
> Does not seem to be so:
> {{{
> [11-10 15:33:46] Torbutton INFO: controlPort << getconf bridge
>
>
> [11-10 15:33:46] Torbutton INFO: controlPort >> 250 Bridge
> [11-10 15:33:46] Torbutton INFO: controlPort << getinfo version config-
file
>
>
> [11-10 15:33:46] Torbutton INFO: controlPort >> 250-version=0.2.7.4-rc
(git-f55d23e1e66e9b0f)
> 250-config-file=/path/to/torbrowser/tor-
browser/Browser/TorBrowser/Data/Tor/torrc
> 250 OK
> }}}
That's a good point. Right now, we're not parsing getconf lines such as
`250 Bridge`. We'd need to write some extra code for this. I tried to
clarify the comment to reflect the current situation.
> 2)
> "kind of parsing for" <- seems the sentence is trailing off
Fixed.
> 3)
> s/displatcher/dispatcher/
Fixed.
> 4) I wonder why we only have `getInfoMultiple()` given that both GETINFO
and GETCONF can have multiple keywords (we use them both with just one
keyword right now as far as I can see which does not explain the different
treatment of both to me)
I never implemented getConfMultiple, but it should be pretty
straightforward if we need it.
> 5) There are a bunch of comments in `getInfoMultiple()` and `getInfo` we
probably might want to get rid of.
I fixed this error handling so it should correctly return a rejected
promise.
> 6) Tor Launcher has a different way of parsing things (leading to
different results it seems (take the case of a `250 Bridge` reply for
`GETCONF bridge`, e.g.). I think we should look at it and have our Tor
Browser controllers behave the same way even if they are using different
parsing means.
That sounds like a good project, but perhaps on a separate ticket? We have
discussed the possibility of merging some of the code.
----
Replying to [comment:5 gk]:
> {{{
> // matchResult finds a single-line result for `250-` or a multi-line
one for `250+`.
> }}}
> is interesting as you are not mentioning the `250 ` case although your
regex is checking for it. But I think the comment is right here (but not
the code) as the `250 ` case should not make it that far as I guess
`EndReplyLine` does not contain a `=` (see the `250 Bridge` example) and
`key` would therefore be `null`. That's one of the things I still need to
figure out which is a bit tricky as `ReplyText` is not specified in the
spec.
If you activate a bridge in Tor Browser, such as obfs3, then you get:
{{{
controlPort << getconf bridge
controlPort >> 250-Bridge=obfs3 [ip] [fp]
250-Bridge=obfs3 [ip] [fp]
250-Bridge=obfs3 [ip] [fp]
250-Bridge=obfs3 [ip] [fp]
250 Bridge=obfs3 [ip] [fp]
}}}
Note the final line without a `-`.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/17568#comment:6>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list