Opened 2 months ago

Last modified 2 months ago

#24366 new defect

compare_vote_rs() could check more fields for better SHA1 collision resistance

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-dirauth, possible-consensus-failure, needs-proposal
Cc: Actual Points:
Parent ID: Points: 2
Reviewer: Sponsor:

Description

If someone submits descriptors with the same SHA1 hashes, compare_vote_rs() checks a few fields to make sure they are really the same.

We should make sure there is some way of checking all (most?) of the fields. And we should compare new fields when they are added to [vote_]routerstatus_t. But we can't just use a binary comparison, because some of the fields are pointers.

Do we need a new consensus method to add extra tie-breakers?

Here are the fields from vote_routerstatus_t and routerstatus_t, in rough order of size/flexibility/collision-usefulness:

Comparing these is probably necessary, they have 128+ bits of entropy:

  • version
  • protocols
  • exitsummary
  • ed25519_id
  • ipv6_addr

Comparing these might not be necessary, they only have a few bits:

  • ipv6_orport
  • measured_bw_kb / bandwidth_kb ?
  • guardfraction_percentage

I'm not sure if comparing these is necessary, they probably don't have enough bits to lead to a collision:

  • flags / is_x (x is a flag name)
  • supports_x (x is a feature name)
  • has_guardfraction
  • has_measured_bw
  • has_ed25519_listing
  • has_bandwidth
  • has_exitsummary

This is a bug in Tor 0.2.0.3-alpha, which introduced this tie-breaking code. (Or in all the versions since then that added extra fields to [vote_]routerstatus_t, but didn't add them to the tie-breakers.)

Child Tickets

Change History (3)

comment:1 Changed 2 months ago by teor

Ah, but hang on, there are some vote items that authorities do disagree about, like IPv6 ORPorts. So we should take them off the list:

Comparing these is probably necessary, they have 128+ bits of entropy:

  • version
  • protocols
  • exitsummary
  • ed25519_id
  • ipv6_addr

Comparing these might not be necessary, they only have a few bits:

  • ipv6_orport
  • measured_bw_kb / bandwidth_kb ?
  • guardfraction_percentage

I'm not sure if comparing these is necessary, they probably don't have enough bits to lead to a collision:

  • flags / is_x (x is a flag name)
  • supports_x (x is a feature name) ?
  • has_guardfraction ?
  • has_measured_bw
  • has_ed25519_listing
  • has_bandwidth
  • has_exitsummary

comment:2 Changed 2 months ago by nickm

Could we solve this problem by extending the votes to include a better hash of the descriptor?

comment:3 Changed 2 months ago by teor

Keywords: needs-proposal added; needs-proposal? removed

The votes already include a better hash of the microdescriptor, but that's probably not enough. (And they contain *multiple* microdesc hashes.)

So yes, I think the right thing to do is to:

  • add a sha-best (are we at SHA3-256 now?) descriptor hash to votes
  • define a new consensus method
  • when using that new consensus method, use this hash as the tie-breaker

I think this lets us remove all the other tie-breaker fields.

Maybe we could just get away with comparing the sha3-256 hash, but let's stick with the existing two hashes, and the sha3, so we don't accidentally *reduce* security.

Note: See TracTickets for help on using tickets.