Opened 23 months ago

Closed 4 months ago

Last modified 2 days ago

#19910 closed defect (fixed)

Rip out optimistic data socks handshake variant (#3875)

Reported by: cypherpunks Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201802R
Cc: antonela, fdsfgs@…, arthuredelstein@…, tor@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Optimistic data socks handshake variant violates RFC we could to ignore except total code logic brokenness.

For something like https transport code functionality depends timing of socks proxy. If socks-proxy answer before TLS handshake can to start then browser process socks handshake as server hello therefore violates TLS session.

You can't to use code fully based on race condition. You can't to fix code so it never process any input data as soon as you start TLS handshake. Only solution to rip out that code entirely.

Child Tickets

Change History (30)

comment:1 Changed 23 months ago by cypherpunks

Resolution: wontfix
Status: newclosed

Nobody cares.

comment:2 Changed 23 months ago by gk

Resolution: wontfix
Status: closedreopened

Just because nobody immediately came up with a patch that does not mean nobody cares.

comment:3 Changed 21 months ago by arlolra

Tor Messenger is reverting this patch (#3875) in https://gitweb.torproject.org/tor-messenger-build.git/commit/?id=cd2bb57632ac4278da6d70fe3339bb64b38d29eb since it seems to be breaking STARTTLS in XMPP.

comment:4 in reply to:  3 Changed 11 months ago by gk

Status: reopenedneeds_information

Replying to arlolra:

Tor Messenger is reverting this patch (#3875) in https://gitweb.torproject.org/tor-messenger-build.git/commit/?id=cd2bb57632ac4278da6d70fe3339bb64b38d29eb since it seems to be breaking STARTTLS in XMPP.

arlolra: Did you see any crashes with that patch or how did it break STARTTLS? I actually wonder whether the patch is the culprit for crashes like #22330 and #22977...

comment:5 Changed 11 months ago by arlolra

Did you see any crashes with that patch or how did it break STARTTLS?

It did not crash. If I remember correctly, I would try to connect to an XMPP server and the handshake would fail.

comment:6 Changed 11 months ago by cypherpunks

Wonder whether the patch is the culprit for "Secure Connection Error" during switching results pages of HTML version of DDG...

comment:7 Changed 6 months ago by cypherpunks

Status: needs_informationnew

It is

16:22:33.163 An error occurred during a connection to duckduckgo.com:443.

The client has encountered bad data from the server.

Error code: <a id="errorCode" title="SSL_ERROR_BAD_SERVER">SSL_ERROR_BAD_SERVER</a>
 1 (unknown)

in Tor Browser.

comment:8 Changed 6 months ago by tokotoko

Cc: fdsfgs@… added

comment:9 Changed 6 months ago by teor

Component: Applications/Tor BrowserCore Tor/Tor
Keywords: tor-tls security-low-maybe regression added
Milestone: Tor: 0.3.3.x-final
Points: 1

We should consider the security implications of this in 0.3.3.

comment:10 in reply to:  9 Changed 6 months ago by gk

Status: newneeds_information

Replying to teor:

We should consider the security implications of this in 0.3.3.

Did you mean to file a new ticket for the tor part (the patch in question is a Tor Browser patch, see #3875)? Or did you mean the Tor Browser team should fix that on the tor side as well? (Right now the ticket is still assigned to tbb-team)

comment:11 Changed 6 months ago by teor

Component: Core Tor/TorApplications/Tor Browser
Keywords: tor-tls security-low-maybe regression removed
Milestone: Tor: 0.3.3.x-final
Points: 1
Status: needs_informationnew

Oops, sorry, I thought this was an issue with Tor's optimistic data feature.
Is there anything that Tor can do tohelp here, or is it an issue with how the application uses the
SOCKSPort?

comment:12 Changed 6 months ago by cypherpunks

Maybe, it's time to look from both sides?

comment:13 Changed 5 months ago by arma

See #24432 for an example of a situation where this optimistic data socks handshake patch surprisingly broke stuff.

It's starting to look more like we're going to rip it out.

I think it would be worth exploring whether we can do the same behavior in a smarter way from the browser though -- the feature essentially removes a round-trip across the Tor network from page loads.

comment:14 Changed 5 months ago by arthuredelstein

Cc: arthuredelstein@… added

comment:15 in reply to:  13 ; Changed 5 months ago by arthuredelstein

Replying to arma:

I think it would be worth exploring whether we can do the same behavior in a smarter way from the browser though -- the feature essentially removes a round-trip across the Tor network from page loads.

Instead of implementing a patch in the browser, I wonder if we could patch the tor proxy instead with roughly the same speedup.

  1. The SOCKS client opens a TCP connection to the tor proxy
  2. The SOCKS client sends a SOCKS CONNECT command
  3. The proxy immediately returns a spoofed SOCKS CONNECTED notification to the SOCKS client, and the proxy also sends a BEGIN cell to the Exit.
  4. The SOCKS client sends some data to the proxy (the GET request, for example).
  5. The proxy sends a DATA cell to the Exit

This arrangement doesn't require any modification to the browser's SOCKS implementation, but it means we don't wait for the Exit to respond with CONNECTED before asking the SOCKS client to send its first data. Basically we put the "optimism" in the proxy instead of the browser. That's nice because then (1) we will get a speedup to all ordinary SOCKS clients and (2) we're patching code controlled by Tor Project.

Provided the round trip time between the SOCKS client and proxy is very small compared to the round-trip time through the Tor network, the performance should presumably be about the same as the existing patch in Tor Browser.

Last edited 5 months ago by arthuredelstein (previous) (diff)

comment:16 Changed 5 months ago by gk

Keywords: TorBrowserTeam201801 added

comment:17 in reply to:  13 Changed 5 months ago by gk

Replying to arma:

See #24432 for an example of a situation where this optimistic data socks handshake patch surprisingly broke stuff.

FWIW it's not the first one. Tor Messenger had to back it out, too, see comment:3, presumably for the same reason.

comment:18 in reply to:  15 Changed 5 months ago by arthuredelstein

Replying to arthuredelstein:

Replying to arma:

I think it would be worth exploring whether we can do the same behavior in a smarter way from the browser though -- the feature essentially removes a round-trip across the Tor network from page loads.

Instead of implementing a patch in the browser, I wonder if we could patch the tor proxy instead with roughly the same speedup.

I see now this approach was discussed in #3875 and also proposed in #5915. There was a proposed patch for tor here: https://trac.torproject.org/projects/tor/attachment/ticket/3875/tor-optimistic-data.patch . I think it still looks like a possible solution but there are some concerns expressed about error messages which would need to be considered.

comment:19 Changed 5 months ago by arma

Ok, I agree that #5915 is a promising direction to explore.

Maybe this is also a ticket where we should involve our shiny new UX group? It would be smart to look at what sorts of error messages you get right now out of Tor Browser when Tor fails to establish a connection to the desired destination, and compare those to what we could get if we trick Tor Browser into thinking that the connection worked, and then if it turns out to fail, have Tor essentially mitm the connection and pretend to be the browser giving you an error. In particular, what are the situations where this idea would break down (SSL? html5? images and videos? more?), and what should we do then?

comment:20 Changed 5 months ago by antonela

Cc: tor@… added

comment:21 Changed 5 months ago by arma

Keywords: ux-team added

comment:22 Changed 5 months ago by gk

FWIW: My plan is to rip the patch out as soon as we want to deploy moat (which should be soon). Then moat will be quite a while on the alpha channel I suspect. Meanwhile work on 0.3.4 should be under way which means #5915 could be fixed before moat hits the stable channel (and stable release get affected by the backout).

comment:23 Changed 5 months ago by gk

Keywords: TorBrowserTeam201802 added; TorBrowserTeam201801 removed

Moving tickets to Feb

comment:24 Changed 4 months ago by mcs

Keywords: TorBrowserTeam201802R added; TorBrowserTeam201802 removed
Status: newneeds_review

Here is a patch to back this out (I needed to create this to make some Moat test builds):
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug19910-01

comment:25 Changed 4 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

This is basically reverting 6d594cc227f153364b91e8310d59a07d7d6ce5f6, right? If so, just doing a git revert 6d594cc227f153364b91e8310d59a07d7d6ce5f6 is slightly better as the next rebase would then automatically remove the optimistic SOCKS handshake code. I try to remember doing that manually. :)

Applied to tor-browser-52.6.0esr-8.0-2 (commit 67fa1e520d8cfc420f5bdee4b53b8310df18a977).

comment:26 in reply to:  25 Changed 4 months ago by mcs

Replying to gk:

This is basically reverting 6d594cc227f153364b91e8310d59a07d7d6ce5f6, right? If so, just doing a git revert 6d594cc227f153364b91e8310d59a07d7d6ce5f6 is slightly better as the next rebase would then automatically remove the optimistic SOCKS handshake code. I try to remember doing that manually. :)

You are correct of course. I will try to remember to use git revert next time!

comment:27 Changed 3 months ago by cypherpunks

Keywords: tbb-backport added; ux-team removed

comment:28 Changed 3 months ago by gk

Keywords: tbb-backport removed

Please don't add random keywords, thanks.

comment:29 Changed 3 months ago by antonela

Cc: antonela added

comment:30 Changed 2 days ago by cypherpunks

Stable is still broken by this bug.

Note: See TracTickets for help on using tickets.