[tor-bugs] #13667 [Tor]: Prevent port scanning of hidden services

[Tor Bug Tracker & Wiki]

#13667: Prevent port scanning of hidden services
------------------------+------------------------------------------
     Reporter:  arma    |      Owner:
         Type:  defect  |     Status:  needs_review
     Priority:  major   |  Milestone:  Tor: 0.2.6.x-final
    Component:  Tor     |    Version:
   Resolution:          |   Keywords:  SponsorR tor-hs 025-backport
Actual Points:          |  Parent ID:
       Points:          |
------------------------+------------------------------------------

Comment (by dgoulet):

 Replying to [comment:13 arma]:
 > Patch looks like it'll work to me. Thanks!
 >
 > 1) Did you test it?

 Yes I did. Chutney network, confirmed with torsocks and logs. I should
 have mention it, good point.

 >
 > And since I'm helping you get up to speed as a developer, some minor
 points (which maybe you and nickm/andrea should talk through and agree
 about how to handle):
 >
 > 2)
 > {{{
 > +                                            Circuit is origin here thus
 > +       * will be set automatically to REASON_NONE anyway. (ref: #13667)
 */
 > +      circuit_mark_for_close(circ, END_CIRC_REASON_NONE);
 > }}}
 >
 > But in circuit_mark_for_close_ I see
 > {{{
 >   if (reason == END_CIRC_AT_ORIGIN) {
 >     if (!CIRCUIT_IS_ORIGIN(circ)) {
 >       log_warn(LD_BUG, "Specified 'at-origin' non-reason for ending
 circuit, "
 >                "but circuit was not at origin. (called %s:%d,
 purpose=%d)",
 >                file, line, circ->purpose);
 >     }
 >     reason = END_CIRC_REASON_NONE;
 >   }
 > }}}
 > Does that mean saying END_CIRC_AT_ORIGIN is the safer plan here? How
 come we don't use ND_CIRC_AT_ORIGIN more often? If we opt not to use it
 here, should we just get rid of it?

 Not sure it's actually a safer plan. I don't know why we don't use it more
 often but providing the real reason on why we are closing the circuit
 seems a better option instead of having it changed for us. IMO, this is
 the caller responsability to provide the right reason, not the callee that
 set it to something else for some conditions.

 >
 > 3) Is "(ref: #13667)" the way we want to point people at the bugtracker?
 I know in the past nickm has wanted to make sure that the code stands
 independently of the bugtracker, but I think it does in this case.

 Clarified by nickm on IRC, I removed it :).

 >
 > 4) "Note that this does not mitigate port scanning" -- I would say it
 actually does mitigate it. It just doesn't completely solve it.

 Fixed.

 >
 > 5) If you happen to reformat the comments and they ended up a few
 columns shorter, I wouldn't object. :)

 Just for you, I setup my vim for the entire tor code base to use 76 char
 :D.

 >
 > 6) The first two rows of your changes file want to right-shift two
 spaces, and s/immetiately/immediately/

 Fixed by 5).

 Updated patch in bug13667_025_v2

 Thanks!

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/13667#comment:14>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online