Opened 8 years ago

Closed 7 years ago

#8059 closed defect (fixed)

miscounting when parsing versions cell

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client protocol
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


  const uint8_t *cp, *end;
  for (cp = cell->payload; cp+1 < end; ++cp) {
    uint16_t v = ntohs(get_uint16(cp));

So we count the payload one byte at a time, considering that byte plus the one after it?

The payload in a VERSIONS cell is a series of big-endian two-byte

That probably produces some weird behavior. Marking as 0.2.3 since I'm not sure yet what weird behavior.

Reported by bob from irc.

Child Tickets

Change History (4)

comment:1 Changed 8 years ago by nickm

Keywords: tor-client protocol added

Hm. This means that it's possible to negotiate versions in a stupid way: instead of the byte-sequence A B C D E F meaning "We support AB, CD, EF", it's taken to mean "We support AB, BC, CD, and DE, and EF."

If there were any two-byte version numbers, that would produce a big problem. Fortunately there aren't any of those yet.

When we fix this bug, it will make it possible to distinguish clients that have this bug from clients that don't. That might actually make this bug less harmful than its fix. Not sure how harmful the fix really is though, but it's worth considering.

If we wanted to make this bug's behavior documented, we could restrict the space of valid version numbers so that misframing never becomes possible in the future. That's ugly though.

comment:2 Changed 7 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final
Status: newneeds_review

Okay; this isn't actually breaking anything, so it's (IMO) an 0.2.4 item.

I don't think that the distinguishing is a showstopper issue here; there are a ton of other ways to distinguish client versions.

Fix in branch "bug8059" in my public repo, with corresponding torspec fix in branch "bug8059" of my public torspec repo.

comment:3 Changed 7 years ago by asn

Looks good to me.

I agree wrt the distinguisher not being particularly important here.

This is one of the functions that would benefit from some unit tests :)

comment:4 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Great; merging. Thanks, everyone.

Note: See TracTickets for help on using tickets.