Opened 20 months ago

Last modified 12 months ago

#29583 new defect

HSv3: Faulty cross-certs in introduction point keys (allows naive onionbalance for v3s)

Reported by: asn Owned by:
Priority: High Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, scaling, onionbalance, 040-backport, 035-backport, needs-proposal, security, 041-longterm, 041-deferred-20190530
Cc: s7r, twim, gk Actual Points:
Parent ID: Points: 4
Reviewer: Sponsor:

Description (last modified by asn)

During a discussion with Nick and David, we figured out that we messed up the cross certificates of the introduction point keys in the HSv3 descriptors.

In particular the spec asks to cross-certificate the descriptor signing key using the introduction point auth key, but it seems like we are doing it the other way around in the code:

  desc_ip->auth_key_cert = tor_cert_create(signing_kp,

same goes for the intro point encryption key. However, it seems that the legacy encryption key cross-cert was made properly.

This is very related to the onionbalance for v3 ticket (parent of this) because these two cross-certs certs was exactly what was preventing us from doing the naive onionbalance for v3s.

So here are some steps forward:

a) Figure out if there are any other cases where we have messed up
cross certs in the code. Figure out the precise dangers of the cross certs we messed up.

With David we scanned the HSv3 code and it seems like these two are the only certs we messed up. If we are right, this means that this bug allows some fairly low-severity attacks like FairPretender from #15951. We should make sure we know exactly what attacks this opens us up to.

b) We should decide if we want to fix this or not. Fixing this would likely involve making a new descriptor version with the right cross-certs, so that clients can verify both the old version and the new one. It will require some engineering but not too much. If we care about the attacks, we shoul fix it. However, if we don't care about these attacks, we can "not fix it", which will also probably allow us to do naive onionbalance for v3s.

As a side-task that can be done paralelly we should revise and improve the spec when it comes to cross-certs because it's currently quite confusing. We should properly define cross-certs and what they do, and we should also improve the wording in places (e.g. there is bad wording even in descriptor-signing-key-cert even tho the code is right), and we should update the spec based on what we decide in this ticket.

I have CCed twim who reported the FairPretender attack in this ticket, and we should think about this more!

Child Tickets

Change History (19)

comment:1 Changed 20 months ago by asn

Description: modified (diff)

comment:2 Changed 20 months ago by asn

Description: modified (diff)

comment:3 Changed 20 months ago by nickm

If we do decide to fix this (and I think we should), I think we'll need a multistep process. Something like this:

  1. Begin including the correct versions of these certificates. Continue including the current (incorrect) versions so as not to break existing clients, but mark them with an extension to indicate that you should only accept them when the correct certificates are present too.
  2. Check the new (correct certificates) when they are present.
  3. Stop including the old (incorrect) certificates.

For step 1 and step 2, we'll probably want to use a consensus-triggered feature to avoid fingerprinting. We can't do step 3 until 2022, when support for 0.3.5.x ends, unless we decide to backport this or something, which would be ... questionable.

comment:4 Changed 20 months ago by nickm

Keywords: 040-backport 035-backport needs-proposal added
Milestone: Tor: unspecifiedTor: 0.4.1.x-final
Priority: MediumHigh

comment:5 in reply to:  3 Changed 20 months ago by dgoulet

Replying to nickm:

If we do decide to fix this (and I think we should), I think we'll need a multistep process. Something like this:

This means basically that we end up being right now doing OnionBalance v3 easily but then by 2022 when we start removing the cert from the descriptor, we won't be able anymore...

In other words, fixing this probably means not doing the OnionBalance naively..... hmmmm

comment:6 Changed 19 months ago by gk

Cc: gk added

comment:7 Changed 19 months ago by asn

Points: 4
Sponsor: Sponsor27-must

comment:8 Changed 18 months ago by haxxpop

I want to add some opinion here. I think the main reason that we made this mistake is because we didn't document the reason why we need to cross-certify the desc signing key with the intro auth key in the spec.

And honestly, it's quite counterintuitive. It sounds like we want to sign the "signing" key with "something" key?? (intro auth key) and the reader will think that it's a typo in the spec because the "signing" key should be the signing key not the signed key :P

comment:9 Changed 18 months ago by gaba

Keywords: network-team-roadmap-2019-Q1Q2 added

comment:10 Changed 18 months ago by nickm

Keywords: security added

comment:11 Changed 18 months ago by nickm

Keywords: 041-longterm added

Marking tickets that I think are valuable but which are likely to need more work in 0.4.2.

comment:12 Changed 17 months ago by nickm

Keywords: 041-deferred-20190530 added

Marking these tickets as deferred from 041.

comment:13 Changed 17 months ago by nickm

Milestone: Tor: 0.4.1.x-finalTor: 0.4.2.x-final

comment:14 Changed 15 months ago by gaba

Keywords: network-team-roadmap-september added; network-team-roadmap-2019-Q1Q2 removed

comment:15 Changed 15 months ago by pili

Sponsor: Sponsor27-mustSponsor27-can

comment:16 Changed 15 months ago by dgoulet

Milestone: Tor: 0.4.2.x-finalTor: unspecified
Parent ID: #26768
Sponsor: Sponsor27-can

From the Stockholm 2019 tor meeting discussions:

  • As a first step and considering our sponsor27 timeline, we'll not fix this for now but still consider it a security problem and thus needs to be fixed at some point.
  • By not fixing this, we can proceed with the easy implementation for OnionBalance v3 (#26768).
  • Proposal 306 is being proposed for OnionBalance v3 support which assumes that one day this bug is fixed.

I'm moving this one out of Sponsor 27 because again the decision is not to fix it for now. I'm hoping for us to provide a reasoning soon on tor-dev@ on why.

Moving this out of 042 Milestone and unparenting. Prop306 does link to this ticket so we don't forget about it.

comment:17 Changed 14 months ago by gaba

I'm reading your last comment now. Does that mean we are removing it from the roadmap for september?

comment:18 Changed 14 months ago by asn

Sorry for the late reply here gaba. Yes this means we are removing it from the roadmap for September and in general from S27 scope. We are focusing on #26768 head on.

comment:19 Changed 12 months ago by gaba

Keywords: network-team-roadmap-september removed
Note: See TracTickets for help on using tickets.