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