Rip out optimistic data socks handshake variant (#3875)

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.

added TorBrowserTeam201802R component::applications/tor browser owner::tbb-team priority::medium resolution::fixed severity::normal status::closed type::defect labels

Nobody cares.

Trac:
Resolution: N/A to wontfix
Status: new to closed

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

Trac:
Resolution: wontfix to N/A
Status: closed to reopened

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

Replying to arlolra:

Tor Messenger is reverting this patch (#3875 (moved)) 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 (moved) and #22977 (moved)...

Trac:
Status: reopened to needs_information

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.

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

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.

Trac:
Status: needs_information to new

Trac:
Username: tokotoko
Cc: N/A to fdsfgs@krutt.org

We should consider the security implications of this in 0.3.3.

Trac:
Keywords: N/A deleted, tor-tls, security-low-maybe, regression added
Points: N/A to 1
Component: Applications/Tor Browser to Core Tor/Tor
Milestone: N/A to Tor: 0.3.3.x-final

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 (moved))? 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)

Trac:
Status: new to needs_information

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?

Trac:
Milestone: Tor: 0.3.3.x-final to N/A
Component: Core Tor/Tor to Applications/Tor Browser
Points: 1 to N/A
Keywords: tor-tls, security-low-maybe, regression deleted, N/A added
Status: needs_information to new

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

See #24432 (moved) 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.

Trac:
Cc: fdsfgs@krutt.org to fdsfgs@krutt.org, arthuredelstein@gmail.com

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.

Trac:
Keywords: N/A deleted, TorBrowserTeam201801 added

Replying to arma:

See #24432 (moved) 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.

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 (moved) and also proposed in #5915 (moved). 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.

Ok, I agree that #5915 (moved) 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?

Trac:
Cc: fdsfgs@krutt.org, arthuredelstein@gmail.com to fdsfgs@krutt.org, arthuredelstein@gmail.com, tor@antonela.me

Trac:
Keywords: TorBrowserTeam201801 deleted, TorBrowserTeam201801 ux-team added

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 (moved) could be fixed before moat hits the stable channel (and stable release get affected by the backout).

Moving tickets to Feb

Trac:
Keywords: TorBrowserTeam201801 ux-team deleted, TorBrowserTeam201802, ux-team added

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

Trac:
Status: new to needs_review
Keywords: TorBrowserTeam201802 deleted, TorBrowserTeam201802R added

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).

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

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!

Trac:
Keywords: ux-team deleted, tbb-backport added

Please don't add random keywords, thanks.

Trac:
Keywords: tbb-backport deleted, N/A added

Trac:
Cc: fdsfgs@krutt.org, arthuredelstein@gmail.com, tor@antonela.me to antonela, fdsfgs@krutt.org, arthuredelstein@gmail.com, tor@antonela.me

Stable is still broken by this bug.

please don't remove this feature. it's important roundtrip saver for interactively clients. if you don't want it, just switch: OptimisticData 0

Replying to cypherpunks:

please don't remove this feature. it's important roundtrip saver for interactively clients. if you don't want it, just switch: OptimisticData 0

The implementation was not really compatible with Mozilla's network stack, hence the crashes linked to in this bug. We need a new implementation to fix that, not disabling optimistic data in a config file.

closed

mentioned in issue #23136 (moved)

mentioned in issue #23840 (moved)

mentioned in issue #24432 (moved)

mentioned in issue #24432 (moved)

mentioned in issue #25065 (moved)

moved to tpo/applications/tor-browser#19910 (closed)