Opened 4 months ago

Last modified 3 weeks 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: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, scaling, onionbalance, 040-backport, 035-backport, needs-proposal, network-team-roadmap-2019-Q1Q2, security, 041-longterm, 041-deferred-20190530
Cc: s7r, twim, gk Actual Points:
Parent ID: #26768 Points: 4
Reviewer: Sponsor: Sponsor27-must

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,
                                           CERT_TYPE_AUTH_HS_IP_KEY,
                                           &ip->auth_key_kp.pubkey,
                                           nearest_hour,
                                           HS_DESC_CERT_LIFETIME,
                                           CERT_FLAG_INCLUDE_SIGNING_KEY);

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 (13)

comment:1 Changed 4 months ago by asn

Description: modified (diff)

comment:2 Changed 4 months ago by asn

Description: modified (diff)

comment:3 Changed 4 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 4 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 4 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 3 months ago by gk

Cc: gk added

comment:7 Changed 3 months ago by asn

Points: 4
Sponsor: Sponsor27-must

comment:8 Changed 7 weeks 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 5 weeks ago by gaba

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

comment:10 Changed 5 weeks ago by nickm

Keywords: security added

comment:11 Changed 5 weeks 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 3 weeks ago by nickm

Keywords: 041-deferred-20190530 added

Marking these tickets as deferred from 041.

comment:13 Changed 3 weeks ago by nickm

Milestone: Tor: 0.4.1.x-finalTor: 0.4.2.x-final
Note: See TracTickets for help on using tickets.