Opened 3 years ago

Last modified 9 months ago

#19011 new defect

Use of maxunmeasuredbw and bwweightscale is broken in consensus

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-dirauth yes-its-a-bug needs-spec
Cc: Actual Points:
Parent ID: #28807 Points: 0.5
Reviewer: Sponsor:

Description

While refactoring, I noticed this code in dirvote.c:

    if (params) {
      if (strcmpstart(params, "bwweightscale=") == 0)
        bw_weight_param = params;
      else
        bw_weight_param = strstr(params, " bwweightscale=");
    }

    if (bw_weight_param) {
      int ok=0;
      char *eq = strchr(bw_weight_param, '=');
      if (eq) {
        weight_scale = tor_parse_long(eq+1, 10, 1, INT32_MAX, &ok,
                                         NULL);
        if (!ok) {
          log_warn(LD_DIR, "Bad element '%s' in bw weight param",
              escaped(bw_weight_param));
          weight_scale = BW_WEIGHT_SCALE;
        }
      } else {
        log_warn(LD_DIR, "Bad element '%s' in bw weight param",
            escaped(bw_weight_param));
        weight_scale = BW_WEIGHT_SCALE;
      }
    }

Looking at the use of tor_parse_ulong(). Since "next" is NULL, any unconverted characters should make it give an error, making us use the default value.

Child Tickets

Change History (11)

comment:1 Changed 3 years ago by nickm

The correct fix here involves allocating a new consensus method, I'm afraid.

comment:2 Changed 3 years ago by nickm

Keywords: 029-nickm-says-yes added

comment:3 Changed 3 years ago by nickm

Keywords: 029-proposed removed
Milestone: Tor: 0.2.9.x-final

Calling these "yes" because they are bugfixes.

comment:4 Changed 3 years ago by isabela

Keywords: isaremoved added
Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

comment:5 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:6 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:7 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:8 Changed 2 years ago by nickm

Keywords: isaremoved removed

comment:9 Changed 2 years ago by nickm

Keywords: 029-nickm-says-yes removed

comment:10 Changed 2 years ago by nickm

Keywords: tor-dirauth yes-its-a-bug needs-spec added

comment:11 Changed 9 months ago by teor

Parent ID: #28807

We need to fix this ticket before we can fix #28807.

Note: See TracTickets for help on using tickets.