[tor-bugs] #24509 [Core Tor/Tor]: circuit_can_use_tap() should only allow TAP for v2 onion services

[Tor Bug Tracker & Wiki]

#24509: circuit_can_use_tap() should only allow TAP for v2 onion services
-------------------------------------------------+-------------------------
 Reporter:  teor                                 |          Owner:  (none)
     Type:  defect                               |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.3.x-final
Component:  Core Tor/Tor                         |        Version:  Tor:
                                                 |  0.3.2.1-alpha
 Severity:  Normal                               |     Resolution:
 Keywords:  prop224, tor-hs, security-low,       |  Actual Points:
  easy, intro                                    |
Parent ID:                                       |         Points:  0.5
 Reviewer:  dgoulet                              |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by teor):

 * status:  needs_review => needs_revision


Comment:

 Replying to [comment:7 irl]:
 > Not convinced by the naming of these variables/constants. There's a lot
 of "hidden services" in the code and I wonder if it's best to just go with
 that, instead of trying to have onion services (probably it is).

 Naming things is always a fun game.
 We'd typically use something like `uses_tap_for_hs_v2` to be explicit.
 Characters are cheap, confusion is expensive.

 > The test suite passes nicely and I've been able to access v2/v3
 services.

 Are there existing unit tests for this code that you could expand with the
 new flag?
 (I suspect there might not be - if that's the case, a new unit test would
 be nice, but let us know if you'd like one of us to do it.)

 > I've not tested hosting a service

 You might enjoy using chutney to run entire testing tor networks locally:
 {{{
 git clone https://git.torproject.org/tor.git
 git clone https://git.torproject.org/chutney.git
 cd tor
 make test-network
 # or, for comprehensive tests
 make test-network-all
 }}}

 > but it would be good to get a review to make sure I'm on the right track
 and to get some hints as to naming.

 This looks like you have the client rend purpose here, but you need the
 client intro and service rend purposes. We have to use TAP for client
 intro because the descriptor only has TAP onion keys. We have to use TAP
 for service rend because the INTRODUCE cell only has TAP onion keys.

 I wonder why this code still works?

 You might have found another bug - does it still work if you make
 circuit_can_use_tap() always return 0?

 > The only code path I haven't fully explored is
 "should_use_create_fast_for_circuit". Is there ever a case where a v2
 onion service would be trying to use create_fast? I don't want to have it
 fail to use TAP and fall back to create_fast because the v2 flag wasn't
 present on a code path.

 CREATE_FAST should only be used on single-hop directory fetch paths during
 bootstrap. It's used when we don't know a relay's onion keys. I wonder if
 we're checking this correctly?

 And I wonder if this is the bug you've found above. That would be a nice
 catch. Still security-low though.

 > I'm thinking to add some assertions that is_v2 is set in any case where
 rend_data is being added to a circuit, which should provide some level of
 assurance and potentially catch any bugs that appear later on.

 We typically use non-fatal, once-off assertions, to avoid crashing or
 spamming logs:
 {{{
 if (BUG_ONCE(condition)) {
   corrective_action();
 }
 }}}

 {{{
 tor_assert_nonfatal_once(condition);
 }}}

 Feel free to add some for the CREATE_FAST case, too.

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