Opened 2 years ago

Closed 23 months ago

#26037 closed defect (duplicate)

DirAuths should check vote signatures before parsing

Reported by: isis Owned by: Samdney
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-security, tor-crypto
Cc: isis, teor, dgoulet, Samdney Actual Points:
Parent ID: #1299 Points: 2
Reviewer: Sponsor:

Description (last modified by isis)

teor pointed out that vote parsing occurs before checking the votes signature (both verifying the signature and ensuring that it comes from a known valid directory authority). dgoulet confirmed this is the case:

See dirvote.c, function dirvote_add_vote(). You will notice that the very first thing is parsing the whole thing with networkstatus_parse_vote_from_string(). Now, as far as I can tell, the voter signature check happens in that function. However, by the time we check it out, we've tokenized the votes and parsed _many_ parts of the vote already. (If you look for check_signature_token() in that function).

And then once we are done parsing, we do have a valid signature for the vote which then make us check if we know the authority with trusteddirserver_get_by_v3_auth_digest().

The issue of anyone being able to trigger a hypothetical vulnerability in one of the parsing functions aside, it's also just simply not efficient to do all the parsing work and then chuck the results at the end of networkstatus_parse_vote_from_string() if the signature wasn't from a valid sig from a known authority.

This issue has been apparently been present since f4ce7f9c9b4 in tor-

Child Tickets

Change History (7)

comment:1 Changed 2 years ago by isis

Cc: dgoulet added; dgouler removed

Not sure who "dgouler" is but they just got notified about this bug.

comment:2 Changed 2 years ago by isis

Description: modified (diff)

comment:3 Changed 2 years ago by Samdney

Cc: Samdney added

comment:4 Changed 2 years ago by Samdney

Owner: set to Samdney
Status: newassigned

I'm a tor code newbie. So give me a few days. Else you can get it back :)

comment:5 Changed 2 years ago by Samdney

I read me through the necessary part of the code for this ticket and it turned out two topics for me:

  1. In networkstatus_parse_vote_from_string() (tor/src/or/routerparse.c):

It does all the parsing and the signatur verification happens very late.
=> This verification should be moved to an earlier point
=> Should we separate this part from the current networkstatus_parse_vote_from_string() function? Or only moving within networkstatus_parse_vote_from_string() to an earlier point?

  1. The ticket also mentioned the trusteddirserver_get_by_v3_auth_digest() (tor/src/or/routerparse.c):

From my spontenous thinking, it would be better it could happen before networkstatus_parse_vote_from_string(), but of course we have some dependences here from networkstatus_parse_vote_from_string().

I need some input for the best strategy/respectively what would you prefer.

comment:6 Changed 2 years ago by asn

Ticket #1299 is also for the same thing. We could mark one of them as dup.

comment:7 Changed 23 months ago by teor

Parent ID: #1299
Resolution: duplicate
Status: assignedclosed

#1299 has more info.

Note: See TracTickets for help on using tickets.