[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #17568 [Tor Browser]: Clean up tor-control-port.js in Torbutton



#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs