Opened 6 years ago

Closed 6 years ago

#8062 closed defect (fixed)

We try to squeeze a two-byte version into a one-byte link_proto

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:

Description

  int highest_supported_version = 0;
...
    uint16_t v = ntohs(get_uint16(cp));
    if (is_or_protocol_version_known(v) && v > highest_supported_version)
      highest_supported_version = v;
...
  chan->conn->link_proto = highest_supported_version;

But

  uint8_t link_proto; /**< What protocol version are we using? 0 for
                       * "none negotiated yet." */

So these checks in channel_tls_process_versions_cell():

  if (!highest_supported_version) {
...
  } else if (highest_supported_version == 1) {
...
  } else if (highest_supported_version < 3 &&
             chan->conn->base_.state ==  OR_CONN_STATE_OR_HANDSHAKING_V3) {
...
  } else if (highest_supported_version != 2 &&
             chan->conn->base_.state == OR_CONN_STATE_OR_HANDSHAKING_V2) {

can all be bypassed by sending 0x0101 rather than 0x0001, etc.

Reported by bob from irc. He says there are triggerable asserts, but he didn't clarify which one.

See also #8059 for a nearby bug.

Child Tickets

Change History (12)

comment:1 Changed 6 years ago by arma

Keywords: tor-client protocol added

(adding the same keywords as #8059)

comment:2 Changed 6 years ago by nickm

I spoke with him last night; I seemed to persuade him that the assert wasn't triggerable. Did he decide that it was after we spoke, or is this the same assert he was talking about before?

comment:3 Changed 6 years ago by arma

I am transcribing from my notes yesterday, since it seemed no one had filed a ticket. I don't have irc backlog that goes far enough back to see the other discussion.

comment:4 Changed 6 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.

There's an obvious fix in branch "bug8062".

comment:5 Changed 6 years ago by asn

Looks good to me.

comment:6 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Great; merging!

comment:7 Changed 6 years ago by cypherpunks

Resolution: fixed
Status: closedreopened

Bug was reported by oftc_must_be_destroyed and not by unknown pseudonym.

Oftc must be destroyed bcs they klines /16 networks

comment:8 Changed 6 years ago by cypherpunks

finks must be outlawed

comment:9 Changed 6 years ago by blackpaw

Resolution: fixed
Status: reopenedclosed

Fixing the ticket.

comment:10 Changed 6 years ago by cypherpunks

Resolution: fixed
Status: closedreopened

wtf?

comment:11 Changed 6 years ago by nickm

The person who reported this was not using the pseudonym oftc_must_be_destroyed. He was using a pseudonym that he had previously asked me to not use in the ChangeLog. The person whose name starts with "bob" can tell me that he wants credit in his name in the changelog for this bug if that's what he wants.

comment:12 Changed 6 years ago by nickm

Resolution: fixed
Status: reopenedclosed

The original bug reporter has not changed his mind and asked me to use the pseudonym starting with "bob", so I won't. Closing.

Note: See TracTickets for help on using tickets.