Opened 3 years ago

Closed 3 years ago

Last modified 21 months ago

#20951 closed task (fixed)

Back out Unix domain socket related patches for Tor Browser 6.5

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201701R, GeorgKoppen201701
Cc: mcs, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We did not get the Unix domain socket support finished in the 6.5aX timeline and need to back out the patches for 6.5 stable. (For the background see: #20761). According to mcs:

we will need to back out:
> * 8871259c966755233b134a5ddb2b4926539d25c6 (#14272)
> * fa4e114a20067810af0c5cc6a57aa6e849386418 (#14272 fixup)
> * 8ca52414916c3d8bc2a2974017d759901ddc1736 (#20111)
> * 4dd8f6130f931616cf014e0ded444c30e04c8bad (#20185)
> 
> There are also Torbutton patches for #14272 and #20111, although it may be okay to leave the code in place (the code implements fallbacks).

I think we should back out the Torbutton ones as well to not risk any inteference we have not tested. There are tor-browser backports as well we can think about backing out.

Child Tickets

Change History (12)

comment:1 Changed 3 years ago by mcs

Status: newneeds_review

Kathy and I created 3 patches (tor-browser, tor-launcher, torbutton):
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug20951-01&id=093d0348526aed12fd9d15c7211c1a6ea362d93d

https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug20951-01&id=c3d1dde708aa9c5d58814808c6a3f80097f3218d

https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug20951-01&id=d4af1dea13322419e9cc6ce1be87f324685a4d0e

For Tor Launcher and Torbutton we left a few bug fixes in place (read the commit messages for details). We did some testing on OSX, but it would be good for one or two other people to look at the changes and possibly try to use a TB that has these patches applied.

comment:2 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201701R added; TorBrowserTeam201701 removed

comment:3 Changed 3 years ago by gk

So, we are removing the patch for bug 1211567 but keep the fix for #19733? Does that scenario gain anybody anything? Or, if we want to keep #19733, the scenario to keep 1211567 as well?

FWIW: I probably won't apply the tor-browser patches but rather omit the patches we don't want to have when rebasing. The tor-launcher changes look fine to me. Note: master is used for the hardened series where we just keep the unix domain socket support. So, the patches should have been against maint-0.2.10. There is no need to create new ones, though, the resulting small conflict in the prefs file is easy to solve when preparing the stable branch.

comment:4 in reply to:  3 Changed 3 years ago by mcs

Replying to gk:

So, we are removing the patch for bug 1211567 but keep the fix for #19733? Does that scenario gain anybody anything? Or, if we want to keep #19733, the scenario to keep 1211567 as well?

Thanks for the review.

Our rationale for keeping the #19733 fix is that we thought there was a sandbox scenario (reported by Yawning) where Torbutton saw a Unix domain sockets SocksPort (via the control port) even when the browser itself was not directly using a Unix domain socket. Including the #19733 fix seems safe to Kathy and me and avoids an ugly "unexpected tor response" log message. But we don't think it makes sense to keep the 1211567 fix unless we keep all of the browser patches.

FWIW: I probably won't apply the tor-browser patches but rather omit the patches we don't want to have when rebasing.

That makes sense.

The tor-launcher changes look fine to me. Note: master is used for the hardened series where we just keep the unix domain socket support. So, the patches should have been against maint-0.2.10. There is no need to create new ones, though, the resulting small conflict in the prefs file is easy to solve when preparing the stable branch.

OK. Kathy and I forgot that alpha is not built from Tor Launcher master. Sorry!

comment:5 Changed 3 years ago by gk

Okay this looks good to me and I gave it a quick test by assembling Torbutton and Tor-Launcher and copied them over to a fresh 6.0.8. I found no issues while surfing and doing New Identity.

comment:6 Changed 3 years ago by gk

Commit 7afb71eb2adccef261b7882b9893c5948dd549db on maint-1.9.6 has the Torbutton changes.

comment:7 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Commit d99650b338f23d824cbd9d3cc91559ba607a465e on maint-0.2.10 has the TorLauncher changes.

comment:8 in reply to:  1 ; Changed 3 years ago by gk

Replying to mcs:

Kathy and I created 3 patches (tor-browser, tor-launcher, torbutton):
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug20951-01&id=093d0348526aed12fd9d15c7211c1a6ea362d93d

FWIW: this means the fix for #20691 is left in the stable code as well which is fine with me.

comment:9 in reply to:  8 Changed 3 years ago by mcs

Replying to gk:

FWIW: this means the fix for #20691 is left in the stable code as well which is fine with me.

We forgot about that fix, but keeping it does seem fine.

comment:10 in reply to:  1 Changed 3 years ago by gk

Replying to mcs:

Kathy and I created 3 patches (tor-browser, tor-launcher, torbutton):
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug20951-01&id=093d0348526aed12fd9d15c7211c1a6ea362d93d

FWIW: tor-browser-45.7.0esr-6.5-1 has the respective commits backed out.

comment:11 Changed 21 months ago by arma

Is there any ticket about putting the patches back? Or writing better ones? (I see #20775 but it's not clear whether that's the same.)

comment:12 Changed 21 months ago by mcs

Unix domain socket support is available starting with Tor Browser 7.0 (and alphas before then). This ticket was about backing the code out because we found some problems before we shipped Tor Browser 6.5 (the code was left in the alpha and shipped as stable with 7.0). The relevant prefs are:

extensions.torlauncher.control_port_use_ipc
extensions.torlauncher.socks_port_use_ipc

Note: See TracTickets for help on using tickets.