[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #24509 [Core Tor/Tor]: circuit_can_use_tap() should only allow TAP for v2 onion services
#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@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs