Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#9162 closed defect (fixed)

get_configured_bridge_by_addr_port_digest() does not work well if digest == NULL

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

Description

get_configured_bridge_by_addr_port_digest() returns NULL if digest is NULL (the caller doesn't care about the digest) and bridge->identity set.

Normally, the function should return any bridge with addr/port that matches even if it has an identity digest set.

This bug becomes visible when Tor does a second SOCKS request to the pluggable transport proxy after it has learned the identity digest of a bridge. In that second request, the SOCKS arguments are not passed, since digest == NULL but bridge->identity is set.

Child Tickets

Attachments (1)

0001-Fix-a-get_configured_bridge_by_addr_port_digest-func.patch (1.5 KB) - added by rl1987 6 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 6 years ago by nickm

I see that you've opened this against 0.2.5.x. Is that because 0.2.4.x doesn't send SOCKS arguments or something, or is this a bug in 0.2.4.x too?

comment:2 in reply to:  1 Changed 6 years ago by asn

Replying to nickm:

I see that you've opened this against 0.2.5.x. Is that because 0.2.4.x doesn't send SOCKS arguments or something, or is this a bug in 0.2.4.x too?

Indeed. Only 0.2.5.x sends SOCKS arguments. See comment:33:ticket:3594 for a message from your past self.

comment:3 Changed 6 years ago by asn

(Reminder to self: This bug was found and reported by Yawning Angel, btw.)

comment:4 Changed 6 years ago by rl1987

Status: newneeds_review

comment:5 Changed 6 years ago by nickm

That patch would disable digest checking entirely, and allow *any* digest to match so long as the addr/port was correct.

I did a fixed-up version as e6590efaa77c8cf186ce92e6ebad175e9c6450d1. Thanks!

comment:6 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

comment:7 Changed 6 years ago by asn

Nick's patch looks good to me.

comment:8 in reply to:  description Changed 6 years ago by arma

Replying to asn:

This bug becomes visible when Tor does a second SOCKS request to the pluggable transport proxy after it has learned the identity digest of a bridge. In that second request, the SOCKS arguments are not passed, since digest == NULL but bridge->identity is set.

Can you describe this bug from the perspective of the user? (The changelog should be what changed from the perspective of the user, not what function got changed to pass its arguments differently.) Thanks!

comment:9 Changed 6 years ago by nickm

I'd expect that the undesired behavior would have been "some pluggable transports top working after the first connection." George, am I right?

comment:10 Changed 6 years ago by asn

nickm is correct. Maybe a more verbose description would be "Pluggable transports that use client parameters stop working on subsequent connections to a bridge, if the first connection was terminated".

(Also, we don't have any PTs with client-side parameters atm. scramblesuit will soon become the first one.)

comment:11 Changed 6 years ago by arma

Thanks!

  o Minor bugfixes:
    - Fix a bug where the first connection works to a bridge that uses a
      pluggable transport with client-side parameters, but we don't send
      the client-side parameters on subsequent connections. (We don't
      use any pluggable transports with client-side parameters yet,
      but ScrambleSuit will soon become the first one.) Fixes bug 9162;
      bugfix on 0.2.0.3-alpha. Based on a patch from "rl1987".

comment:12 in reply to:  description Changed 6 years ago by dcf

Replying to asn:

This bug becomes visible when Tor does a second SOCKS request to the pluggable transport proxy after it has learned the identity digest of a bridge. In that second request, the SOCKS arguments are not passed, since digest == NULL but bridge->identity is set.

In answer to why there's a second SOCKS connection, it sounds like #7733.

Note: See TracTickets for help on using tickets.