Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#5786 closed defect (fixed)

A single authority can crash the other authorities

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-auth
Cc: mikeperry Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

boboper found a couple of assertion failures in dirauth vote parsing functions:

< boboper> "0 < weight_scale" what a silly style?
< boboper> lol "weight_scale < INT32_MAX" it's natural bugdoor.
< boboper> as well as zero compare
< boboper> good to know you filled your ignore list. I can slowly  develop mine exploits. I found that irc helps me to do it better.
< boboper> "tor_parse_long(eq+1, 10, INT32_MIN, INT32_MAX, &ok, NULL)" "tor_assert(ok)" one mad auth can destroy all network. I wanna be auth.
< boboper> or_parse_long(eq+1, 10, INT32_MIN, INT32_MAX, &ok, NULL); can't detect overflow and underflow for 32bit. LONG_MIN == INT32_MIN
< boboper> "tor_assert(i<n_votes)" you re so funny devs

Child Tickets

Change History (9)

comment:1 Changed 8 years ago by asn

< boboper> networkstatus_parse_vote_from_string() "strcmp(tok->args[i-1], tok->args[i]) >= 0"
< boboper> "a=1" < "a=2"
< boboper> one auth with params "a=20 a=22 a=24" vs legal auths with 1: "a=3", 2: "a=4" means result is "a=20"
< boboper> while leagsl result is "a=4"

comment:2 Changed 8 years ago by nickm

Status: newneeds_review

Branch bug5786_range_022 [*] fixes the behavior of tor_parse_long and its friends so that they can detect overflows on tor_parse_long(x,10,INT32_MIN,INT32_MAX,&ok,NULL) when sizeof(long) == sizeof(int32_t).

Branch bug5786 has the authority-size fixes: it detects bad authority arguments earlier in the bandwidth computation before an assertion would happen[], and tries to detect duplicate entries in networkstatus parameter lines. I'm currently targeting it at 0.2.3.x, but could possibly be persuaded that it's an 0.2.2.x item.

My understanding is that boboper doesn't want me putting his name on any commits. If that's wrong, I hope he'll say so.

[*] all branches here are in my public repo.
[] This changes the previous behavior of the algorithm. However, since the previous behavior was "assertion failure", I'm not too concerned.

Adding mike to the cc in hopes he can review the bandwidth weight issues.

comment:3 Changed 8 years ago by asn

Cc: mikeperry added

comment:4 Changed 8 years ago by rransom

bug5786_range_022 looks good, and the test suite succeeds on my Debian Wheezy amd64 system.

comment:5 Changed 8 years ago by mikeperry

Oh man. Rather embarrassing mistake to use INT32_MIN instead of a positive integer. Not sure if 1 is the best choice, but in addition to the assert, the v10+ consensus methods will also not include bandwidth-weight lines that do not actually solve the system of linear equations and/or if they somehow solve them with values outside of the 0-1.0 (scaled) range for any weight. See the use of networkstatus_check_weights in networkstatus_compute_bw_weights_v10 for those checks.

Those checks should also prevent badness in the event of authorities choosing a ridiculously low bwweightscale, like 1 or 2 (even 10 is probably too low in practice).

But overall, yes, the combination of bug5786_range_022 and your weight_scale commit should cause the code to function as intended (which is to fall back to BW_WEIGHT_SCALE for super crazy values of bwweightscale).

I do not claim to understand rounterparse.c well enough to properly review your "prevent dirauths from voting more than once for the same parameter" commit, though.

comment:6 Changed 8 years ago by nickm

Merged bug5786_range_022 into 0.2.2 and master.

comment:7 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged bug5786 into master.

comment:8 Changed 7 years ago by nickm

Keywords: tor-auth added

comment:9 Changed 7 years ago by nickm

Component: Tor Directory AuthorityTor
Note: See TracTickets for help on using tickets.