Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#6514 closed defect (fixed)

Clear rs_out between iterations of voting loop

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

Description

In the main loop of networkstatus_compute_consensus, we do not reset rs_out between iterations. That wasn't a problem before consensus method 5, where we set every field in rs_out during every iteration of the loop. But this block here is a problem:

      if (consensus_method >= 6 && num_mbws > 2) {
        rs_out.has_bandwidth = 1;
        rs_out.bandwidth = median_uint32(measured_bws, num_mbws);
      } else if (consensus_method >= 5 && num_bandwidths > 0) {
        rs_out.has_bandwidth = 1;
        rs_out.bandwidth = median_uint32(bandwidths, num_bandwidths);
      }

Fortunately, we set rs_out.has_bandwidth = 0 earlier in the loop, so it is only rs_out.bandwidth that leaks from iteration to iteration. As far as I can tell, we only look at rs_out.bandwidth when has_bandwidth is set. So *that's* okay.

Also see:

        if (chosen_exitsummary) {
          rs_out.has_exitsummary = 1;
          /* yea, discards the const */
          rs_out.exitsummary = (char *)chosen_exitsummary;
        }

Again, has_exitsummary is reset earlier in the loop, but exitsummary isn't inspected unless has_exitsummary is set.

Nevertheless, this logic is an accident waiting to happen.

Reported pseudonymously; I am not 100% convinced there is not a bug here that I am missing.

Child Tickets

Change History (6)

comment:1 Changed 7 years ago by nickm

Incidentally, if there *is* a bug here, then we probably need to add a new consensus method to solve it. Otherwise fixed authorities would consense differently from unfixed ones. :p

comment:2 Changed 7 years ago by nickm

Status: newneeds_review

branch bug6514 in my public repo. As noted above, we shouldn't merge this unless we're confident that it doesn't fix a current bug. :)

comment:3 Changed 7 years ago by ln5

  1. I can't see any leakage between iterations in the loop.
  1. I've looked at what's being accessed in routerstatus_format_entry

and found that to be safe too, since

  • members published_on, nickname, identity_digest, descriptor_digest, addr, or_port, dir_port have all been initialised enough in networkstatus_compute_consensus()

and

(format == NS_V3_CONSENSUS
format == NS_V3_CONSENSUS_MICRODESC)

is always true when called from networkstatus_compute_consensus() so
no other members of the routerstatus is being accessed

It can be noted that only the first DIGEST_LEN bytes of
rs->descriptor_digest is initialised. That's enough though since
digest_to_base64() doesn't read more than that.

comment:4 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Thanks for the review! Merging this now.

comment:5 Changed 7 years ago by nickm

Keywords: tor-auth added

comment:6 Changed 7 years ago by nickm

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