Opened 6 years ago

Closed 6 years ago

#9200 closed defect (fixed)

named_flag[v_sl_idx] check when computing chosen_name in dirvote.c is wrong

Reported by: nickm Owned by:
Priority: High Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-auth
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When we look to see whether the Named flag is set on a vote, we do:

   rs->flags & (U64_LITERAL(1) << named_flag[v_sl_idx])

without checking whether named_flag[v_sl_idx] is >= 0. This can invoke undefined behavior!

Now, in practice, on x86 chips with gcc or clang, assuming no excessive compiler cleverness, this is probably just going to result in a check of bit 63, which won't be in use unless the directory authority has listed 63 flag. So it won't break the voting algorithm so long as all directories are honest.

But all-honest directories is not our assumption, so this should get fixed in the next release.

Child Tickets

Change History (4)

comment:1 Changed 6 years ago by nickm

Status: newneeds_review

Please review branch "bug9200".

comment:2 Changed 6 years ago by andrea

This patch looks okay to me, but ISO C99 6.5.7.3 also states that the bitwise shift operators have undefined behavior if the shift is greater than the width of the left operand. Shouldn't we perhaps be checking a maximum as well if we're being careful about this?

comment:3 Changed 6 years ago by nickm

I believe that we ensure in that the flag indices can't be above 63 by ensuring at vote parse time that there are no more than 64 flags. It's worth commenting about though. I'll add a comment.

comment:4 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Added a comment and a check; merged. Thanks!

Note: See TracTickets for help on using tickets.