Opened 6 months ago

Closed 5 months ago

#26401 closed task (fixed)

Rebase Orfox patches onto Tor Browser 8.0 for TBA

Reported by: sysrqb Owned by: tbb-team
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, TorBrowserTeam201807R
Cc: igt0, gk, arthuredelstein, sisbell Actual Points:
Parent ID: #26531 Points:
Reviewer: Sponsor:

Description

This is a replacement for #26338. We decided Tor Browser for Android should be based on ESR 60 because Mozilla Firefox for Android (Fennec) is currently in maintenance-mode - the diff between releases will be very small, and we will backport any security-related patches. Basing TBA on ESR60 means we can use the same git branch for desktop and mobile.

[0] https://lists.torproject.org/pipermail/tbb-dev/2018-June/000852.html

Child Tickets

Change History (41)

comment:1 Changed 6 months ago by sysrqb

Parent ID: #26531

Required for first alpha.

comment:2 Changed 6 months ago by arthuredelstein

Cc: arthuredelstein added

comment:3 Changed 5 months ago by sysrqb

Status: newneeds_review

Okay, I'm giving in. I hit a crash-bug a week ago on this branch, and I've been trying to reproduce it, but I've no luck. We should start the review process.

Branch: tor-browser-60.1.0esr-8.0-1+26401
Repo: https://git.torproject.org/user/sysrqb/tor-browser.git

This branch does not include #26028, #24926.
This branch does include #22059, #25363, #25364, #25366, #25367.

comment:4 Changed 5 months ago by gk

Keywords: TorBrowserTeam201807R added

comment:5 Changed 5 months ago by igt0

I will review in batches:

97ca08c7c8bc Bug 25741 - TBA: Add mobile-override of 000-tor-browser prefs

  1. Do we want to disable the app.update.enabled (I am asking because of the #26242)?
  1. About the user agent, I think it would be rv: 60.0, Gecko/60.0 and Firefox/60.0. Do we need to specify the android version?
  1. I think we should disable all the sensors. (device.sensors.*)

69bdd94ecb8e Bug 25741 - TBA: Add default configure options in dedicated file
LGTM

ae36b1d20547 Bug 25741 - TBA: Do not register Stumbler listener at start up
LGTM

6d7a72d3e95b Bug 25741 - TBA: Add an AppConstant for TOR_BROWSER_VERSION
I think we need to add something in the .mozconfigure specifying the current browser version, right?

9d8303c14531 Bug 25741 - TBA: Disable features at compile-time
LGTM

66ecd900c106 Bug 25741 - TBA: Add mozconfig for Android and pertinent branding files.
Are we going to use Orfox assets? if yes, LGTM

2f7b87b25f2b Bug 25741 - TBA: Move GCM Push prefs within preprocessor guard
LGTM

970c95dff0b3 Bug 25741 - TBA: Exclude unwanted Stumbler tests
LGTM

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

comment:6 Changed 5 months ago by igt0

Cc: sisbell added

comment:7 Changed 5 months ago by igt0

cc199e9fda91 Bug 25741 - TBA: Clear state when the app exits, by default
LGTM

5494b0193552 Bug 25741 - TBA: Do not import bookmarks and history from native browser by default
LGTM

d9370c64b055 Bug 25741 - TBA: Do not save browsing history by default
LGTM

47c04b064189 Bug 25741 - TBA: Move CAMERA permission within MOZ_WEBRTC
We are already disabling the MediaDevices API in the prefs. Do we also need to make this change?

908c2f542a82 Bug 25741 - TBA: Conditionally require *_LOCATION permissions
LGTM


85b5d47bfa7f Bug 25741 - TBA: Conditionally require WIFI and NETWORK permissions

I am not sure if we want to disable the access to the network state by default. We already have a fingerprint protection in place for the Network Information API. And again the update service has few checks about where the user is connected.

4b3c94077749 Bug 25741 - TBA: Disable QR Code reader by default
LGTM

91a6e589e9c8 Bug 25741 - TBA: Disable the microphone by default
LGTM

4d1310b1c439 Bug 25741 - TBA: Disable telemetry and experiments
LGTM

25ec90395fc8 Bug 25741 - TBA: Remove sync option from preferences
LGTM

comment:8 Changed 5 months ago by igt0

21149e7a423d Bug 25741 - TBA: Neuter Firefox Accounts
LGTM

e0bb700e3300 Bug 25741 - TBA: Only include GCM permissions if we want them
LGTM

21f0480aff98 Bug 25741 - TBA: Only include Firefox Account permissions if we want them
LGTM

cb7427ebb480 Bug 25741 - TBA: Always Quit, do not restore the last session
LGTM

035fbfb88a3c Bug 25741 - TBA: Disable all data reporting by default
Can we change toolkit.telemetry.enabled preference to false? the other part of the code LGTM.

ad48b4d56152 Bug 25741 - TBA: Disable media autoplay by default
The pref media.autoplay.enabled is already false in the 000-tor-browser-android.js. Should we change it here as well?

comment:9 Changed 5 months ago by igt0

952f0ae78cc1 Orfox: Centralized proxy applied to CrashReporter, SuggestClient, Distribution, AbstractCommunicator and BaseResources.
Can we use the network.proxy.* instead of hardcoding the proxy configuration in the java file?

44c65b7904d8 Orfox: add BroadcastReceiver to receive Tor status from Orbot
Should we initialize the sTorStatus?

db3c54ef05e7 Orfox: NetCipher enabled, checks if orbot is installed
Is import android.os.Looper needed?

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

comment:10 Changed 5 months ago by igt0

e1ed56fc4ed4 Orfox: disable screenshots and prevent page from being in "recent apps"
LGTM

94f9d2324391 Orfox: show only the "restricted" onboarding first time screens
LGTM

14cefdc4bd90 Bug 25741 - TBA: Disable GeckoNetworkManager
LGTM

91eeee498b67 Bug 25741 - TBA: Adjust the User Agent String so it doesn't leak Android version
hmm, are we not using the useragent pref?

a2c4e6217da4 Bug 25741 - TBA: top sites changed, used bookmarks icon temporarily.
Are we going to keep the guardian project in the list of suggested websites?

comment:11 in reply to:  5 Changed 5 months ago by sysrqb

Replying to igt0:

I will review in batches:

Thanks!

97ca08c7c8bc Bug 25741 - TBA: Add mobile-override of 000-tor-browser prefs

  1. Do we want to disable the app.update.enabled (I am asking because of the #26242)?

My plan was disabling this pref for now. We can add a URL when have a plan for #26242.

  1. About the user agent, I think it would be rv: 60.0, Gecko/60.0 and Firefox/60.0. Do we need to specify the android version?

I was following Mozilla's new UAS format. This is the format Tor Browser is using on desktop now, too - but this may change with #26146. I chose that Android version because it is the oldest version of Android we support and the oldest version of Android that Google report 1%+ users.

https://developer.android.com/about/dashboards/
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/AppConstants.java.in#140 (you probably saw this already)

  1. I think we should disable all the sensors. (device.sensors.*)

Yes. I was thinking we can enumerate the other fingerprinting prefs we want disabled/changed in #25703. I only concentrated on the defenses Orfox provided in this patch.

6d7a72d3e95b Bug 25741 - TBA: Add an AppConstant for TOR_BROWSER_VERSION
I think we need to add something in the .mozconfigure specifying the current browser version, right?

We can provide it on the command line when running ./mach configure (this is how tor-browser-build provides it). I was using

./mach configure --with-tor-browser-version=8.0 --with-distribution-id=org.torproject --enable-update-channel=alpha --enable-bundled-fonts

When the environment variable TB_BUILD_WITH_UPDATER is not set, then the updater is excluded at compile-time.

66ecd900c106 Bug 25741 - TBA: Add mozconfig for Android and pertinent branding files.
Are we going to use Orfox assets? if yes, LGTM

I think yes, until we have replacements.

comment:12 in reply to:  8 Changed 5 months ago by sysrqb

Replying to igt0:

035fbfb88a3c Bug 25741 - TBA: Disable all data reporting by default
Can we change toolkit.telemetry.enabled preference to false? the other part of the code LGTM.

Yes, we should inherit this from 000-tor-browser.js:
https://gitweb.torproject.org/user/sysrqb/tor-browser.git/tree/browser/app/profile/000-tor-browser.js?h=tor-browser-60.1.0esr-8.0-1%2b26401#n96

ad48b4d56152 Bug 25741 - TBA: Disable media autoplay by default
The pref media.autoplay.enabled is already false in the 000-tor-browser-android.js. Should we change it here as well?

This may not be necessary. This change was part of a larger patch in the Orfox patchset. We probably don't need it, but defense-in-depth and such.

comment:13 in reply to:  9 Changed 5 months ago by sysrqb

Replying to igt0:

952f0ae78cc1 Orfox: Centralized proxy applied to CrashReporter, SuggestClient, Distribution, AbstractCommunicator and BaseResources.
Can we use the network.proxy.* instead of hardcoding the proxy configuration in the java file?

Unfortunately, no, not easily. This is within GeckoView, and gradle builds it this library early in the build process. Accessing preferences is handled by mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java, and this is built during the Fennec build process. Basically, GeckoPreferences doesn't exist when GeckoView is compiled.

I considered some solutions/workarounds for this, but I decided hardcoding these values is an easy solution and it is probably acceptable for the first alpha. When we integrate Tor into the app we will need a new/different solution anyway.

Do you think there is another easy way we can use the prefs?

44c65b7904d8 Orfox: add BroadcastReceiver to receive Tor status from Orbot
Should we initialize the sTorStatus?

Yes!

db3c54ef05e7 Orfox: NetCipher enabled, checks if orbot is installed
Is import android.os.Looper needed?

No, I'll delete it.

comment:14 in reply to:  10 Changed 5 months ago by sysrqb

Replying to igt0:

91eeee498b67 Bug 25741 - TBA: Adjust the User Agent String so it doesn't leak Android version
hmm, are we not using the useragent pref?

No :( These are used by the Java network connections. We should use the pref in the future, but I didn't want a large patch here.

a2c4e6217da4 Bug 25741 - TBA: top sites changed, used bookmarks icon temporarily.
Are we going to keep the guardian project in the list of suggested websites?

Yes, for this ticket. We have #25917 for changing which sites we present in TBA. (I didn't want to make too many changes).

comment:15 Changed 5 months ago by sysrqb

Sorry, I forgot to update this ticket. I have another branch ready for review. Some of the above comments are different from the changes I made.

Branch tor-browser-60.1.0esr-8.0-1+26401_1

ad48b4d56152 Bug 25741 - TBA: Disable media autoplay by default
The pref media.autoplay.enabled is already false in the 000-tor-browser-android.js. Should we change it here as well?

I reverted this change. The pref is enough.

035fbfb88a3c Bug 25741 - TBA: Disable all data reporting by default
Can we change toolkit.telemetry.enabled preference to false?

I reverted the default checkbox change.

db3c54ef05e7 Orfox: NetCipher enabled, checks if orbot is installed
Is import android.os.Looper needed?

I moved this import into the correct commit. It was included in the wrong one.

comment:16 in reply to:  8 Changed 5 months ago by sysrqb

Replying to igt0:

21f0480aff98 Bug 25741 - TBA: Only include Firefox Account permissions if we want them
LGTM

After the next round of reviews, I'll fixup this commit so it includes:

diff --git a/mobile/android/base/FennecManifest_permissions.xml.in b/mobile/android/base/FennecManifest_permissions.xml.in
index 590429845085..1b7b2d5475ec 100644
--- a/mobile/android/base/FennecManifest_permissions.xml.in
+++ b/mobile/android/base/FennecManifest_permissions.xml.in
@@ -30,7 +30,9 @@
     <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"/>
     <uses-permission android:name="com.android.launcher.permission.INSTALL_SHORTCUT"/>
     <uses-permission android:name="com.android.launcher.permission.UNINSTALL_SHORTCUT"/>
+#ifdef MOZ_ANDROID_LOCATION
     <uses-permission android:name="com.android.browser.permission.READ_HISTORY_BOOKMARKS"/>
+#endif
 
     <uses-permission android:name="android.permission.WAKE_LOCK"/>
     <uses-permission android:name="android.permission.VIBRATE"/>

(based on the comment I made at the bottom of ticket:24796#comment:10)

comment:17 Changed 5 months ago by gk

I started my review based on tor-browser-60.1.0esr-8.0-1+26401 which is what I stuck to. Here comes the first batch of comments:

commit 970c95dff0b30eaf0597e43c442b375fe8a68da0 -- not okay

"We exclude the all testStumbler*.java files" (one of "the" and "all" should be
enough :) )

Maybe use '' instead of "" as all exclude rules are using the former

commit 2f7b87b25f2b2648f2f72addae1bcf7283bbf7da -- okay
commit 66ecd900c106de3bb84ba9f5aa1cdc59ab28cdd7 -- not okay

.mozconfig-android: are we really still using ndk r10e for esr60; I guess the comments before the *strip options are essentially enabling stripping? if so, we should be explicit about it an use --enable-strip like on other platforms as well

mozconfig.common.override: why do we have all of those changes here? Shouldn't the .mozconfig-android file already take care of them? If there are some options missing in the former let's add them there. Looking closer at that file it seems this is intended for Mozilla's try infra? While I think that's worthwhile I think we should have a separate bug for that and thinking about a strategy including all the other platforms we support as well.

"When using --disable-crashreporter the symbole file" -> "When using --disable-crashreporter the symbols file"

commit 9d8303c145317d067b130855eb0b17c70c614d76 -- okay (should we file a bug in Mozilla's bug tracker for the unused defines?)

commit 6d7a72d3e95bf483a9256cfe3bf9b3888a363da3 -- okay
commit ae36b1d20547358c49ee2206af5e28767ea7d48b -- not okay

Indentation:

+        if (!AppConstants.isTorBrowser()) {
         getApplicationContext().sendBroadcast(
                 new Intent(INTENT_REGISTER_STUMBLER_LISTENER)
         );
+        } // !isTorBrowser()

commit 69bdd94ecb8e97e4d590dc75c04963b6659bdae0 -- probably okay (why do we need the duplicated entries we already have in confvars.sh?)

commit 97ca08c7c8bc6d58cbeac4838cf2587dc8603050 -- not okay

I think we can skip the whole UA override thing as it does not play any role anymore once we set the resistfingerprinting pref (which is done for mobile as well)

Why is "#ifdef TOR_BROWSER_VERSION" commented out? It seems to me we don't want to point our users to the aus5 URL. Maybe I am missing something here.

s/URLS/URLs/

no need for dom.battery.enabled anymore (see: #22554)

network.manage-offline-status is already set in 000-tor-browser.js
geo.enabled is already set in 000-tor-browser.js

Do we need network.tickle-wifi.enabled given #18799?

I think we should exclude the VR related pref and fix that in the desktop pref
file instead (#21607).

It's not clear to me why we have some of the prefs set in mobile but not desktop (e.g. the clearOnShutdown ones). I guess we could look over it in a new ticket to make sure we really only include android specific prefs in the new prefs file?

commit 25ec90395fc82795e8b130c3d763d4b8893f1114 -- okay
commit 4d1310b1c43975cfefaa8134f54f1e487db6e431 -- okay
commit 91a6e589e9c89219eb15c721122a0564d7399b4b -- okay
commit 4b3c94077749e620f8d9055412ab01bf4286b435 -- probably okay (What is the threat here?)
commit 85b5d47bfa7f2ecc33767e0d5cd4bfd0a0a33460 -- okay
commit 908c2f542a82df40f5757f0439a054e8067d8da8 -- probably okay (Is the additional, empty line intentional?

     <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/>
 #endif
+
     <uses-permission android:name="android.permission.INTERNET"/>

)
commit 47c04b0641898dfe2a2f1ca53aee101f41809d15 -- okay
commit d9370c64b055fdcb9293aa1fc718eab8c2876257 -- okay
commit 5494b0193552b2833d05bd507264d9796b651cca -- okay
commit cc199e9fda917acee9435eba791a8d1a69536443 -- okay
commit ad48b4d561522353c621a5d5166fc31b2517b624 -- okay
commit 035fbfb88a3c05c9b9776139b4e318ee956a1f6f -- okay
commit cb7427ebb4805c42e67cb0e93b3f8b8a5b19a365 -- okay
commit 21f0480aff989a3853188a2ae424152a8e34b72b -- okay
commit e0bb700e33001643de6d3e58adf8a2c881a61dbc -- okay

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

comment:18 Changed 5 months ago by gk

commit 21149e7a423d1f88866e5781e521021a08fca126 -- not okay

Indentation of code in the else-clauses is missing. Are we good with all the other account manager related things that Orfox patched but this patch omits?

commit db3c54ef05e71234a009bdc1abdbf9b80135cb0a -- ???

That one is hard for me to review. Is that a new NetCipher version included? If so, what's the new one and what was the old one, used for ESR52?

Do we really want to have mobile/android/orfox? What happened to

+  <string name="no_space_to_start_error">&no_space_to_start_error;</string>
+  <string name="error_loading_file">&error_loading_file;</string>

?

comment:19 Changed 5 months ago by gk

I need to look with fresh eyes at commit 44c65b7904d8bf599345991942e0407af2030199 and commit 952f0ae78cc17f66624f3c634d831ff0aedc9692 tomorrow but the comments to the remaining commits are:

commit a2c4e6217da42219327206508015ee24846907c4 -- probably okay (Any reason the Facebook bg color (#ffecf0f3) did not get adjusted as done in the original patch?)

commit 91eeee498b6733075d1de69d9f3568bfbffb5f05 -- not okay

I assume the resistfingerprinting related UA is not being used by Java code automatically? If so, then we should hardcode that one and not a custom one.

commit 14cefdc4bd90083e22ac263954a857d254b346a9 -- not okay

Indentation of the if-clauses

commit 94f9d2324391c5c18ac22a025b322a71adff3fcc -- okay
commit e1ed56fc4ed4a6cdcef8c692cb2d5f909174ddc4 -- okay

comment:20 in reply to:  19 ; Changed 5 months ago by sysrqb

Replying to gk:

I started my review based on tor-browser-60.1.0esr-8.0-1+26401 which is what I stuck to. Here comes the first batch of comments:

Thanks, no problem.

commit 970c95dff0b30eaf0597e43c442b375fe8a68da0 -- not okay

"We exclude the all testStumbler*.java files" (one of "the" and "all" should be
enough :) )

Maybe use '' instead of "" as all exclude rules are using the former

Changed.

commit 66ecd900c106de3bb84ba9f5aa1cdc59ab28cdd7 -- not okay

.mozconfig-android: are we really still using ndk r10e for esr60;

No, I'll update the comment (Fennec is build using NDK r15c now)

I guess the comments before the *strip options are essentially enabling stripping? if so, we should be explicit about it an use --enable-strip like on other platforms as well

The defaults are effectively --disable-strip and --enable-install-strip but we should test these options and understand what improves and what fails when we enable/disable them. I'm still not sure.

dnl ========================================================
dnl = Enable stripping of libs & executables
dnl ========================================================
MOZ_ARG_ENABLE_BOOL(strip,
[  --enable-strip          Enable stripping of libs & executables ],
    ENABLE_STRIP=1,
    ENABLE_STRIP= )
                                                                                                                                                                                                                   
dnl ========================================================
dnl = Enable stripping of libs & executables when packaging
dnl ========================================================
MOZ_ARG_ENABLE_BOOL(install-strip,
[  --enable-install-strip  Enable stripping of libs & executables when packaging ],
    PKG_SKIP_STRIP= ,
    PKG_SKIP_STRIP=1)
$ grep -e ENABLE_STRIP -e PKG_SKIP_STRIP obj-arm-linux-androideabi/config.status
    'ENABLE_STRIP': '',
    'PKG_SKIP_STRIP': '',

mozconfig.common.override: why do we have all of those changes here? Shouldn't the .mozconfig-android file already take care of them? If there are some options missing in the former let's add them there. Looking closer at that file it seems this is intended for Mozilla's try infra? While I think that's worthwhile I think we should have a separate bug for that and thinking about a strategy including all the other platforms we support as well.

Okay, that's fair, I'll factor out these changes.

"When using --disable-crashreporter the symbole file" -> "When using --disable-crashreporter the symbols file"

ack.

commit 9d8303c145317d067b130855eb0b17c70c614d76 -- okay (should we file a bug in Mozilla's bug tracker for the unused defines?)

Yes, I'll open a ticket for that.

commit ae36b1d20547358c49ee2206af5e28767ea7d48b -- not okay

Indentation:

+        if (!AppConstants.isTorBrowser()) {
         getApplicationContext().sendBroadcast(
                 new Intent(INTENT_REGISTER_STUMBLER_LISTENER)
         );
+        } // !isTorBrowser()

I did this so the diff is only the new lines instead of the entire statement. This seemed easier for reviewing and future rebase. I can indent the conditional if that is more readable.

commit 69bdd94ecb8e97e4d590dc75c04963b6659bdae0 -- probably okay (why do we need the duplicated entries we already have in confvars.sh?)

I added those in torbrowser.configure because they look like configure flags but they are environment variables. They are still in torbrowser.configure only for documentation purposes. I can delete them if it's confusing or you think they aren't helpful.

commit 97ca08c7c8bc6d58cbeac4838cf2587dc8603050 -- not okay

I think we can skip the whole UA override thing as it does not play any role anymore once we set the resistfingerprinting pref (which is done for mobile as well)

Okay, I'll delete it.

Why is "#ifdef TOR_BROWSER_VERSION" commented out? It seems to me we don't want to point our users to the aus5 URL. Maybe I am missing something here.

Hm! Those // were only supposed to be visual and not affect the inclusion. I thought the preprocessor preserves and enforces the ifdef when it scans the file - but I see this did not happen. app.update.url.android is still set. I'll look into this more.

s/URLS/URLs/

ack.

no need for dom.battery.enabled anymore (see: #22554)

Okay, I agree. I'll delete that.

network.manage-offline-status is already set in 000-tor-browser.js
geo.enabled is already set in 000-tor-browser.js

I see! Deleted.

Do we need network.tickle-wifi.enabled given #18799?

Okay, no, that looks good.

I think we should exclude the VR related pref and fix that in the desktop pref
file instead (#21607).

I'm okay with this.

It's not clear to me why we have some of the prefs set in mobile but not desktop (e.g. the clearOnShutdown ones). I guess we could look over it in a new ticket to make sure we really only include android specific prefs in the new prefs file?

Yes. I think we should discuss this, in particular I'm not sure if autoplay should be enabled or disabled. This may be one example where it provides a better user experience if it is disabled on mobile but the remains enabled on desktop.

commit 4b3c94077749e620f8d9055412ab01bf4286b435 -- probably okay (What is the threat here?)

I think this provides a safe default. When this is enabled, the user is provided an icon in the awesome bar for scanning a QRCode that (presumably) contains a URL the user wants to visit. When the icon is pressed, another app is opened (if it is installed) where a picture is taken and it is scanned for a QRCode and then decoded. I believe the scanning/decoding is performed on a remote server. Disabling this by default protects against accidentally revealing their location to third parties. The user could tap the icon by mistake and launch the third party app, and we don't have any control over what that app does or what information that app sends.

commit 908c2f542a82df40f5757f0439a054e8067d8da8 -- probably okay (Is the additional, empty line intentional?

     <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/>
 #endif
+
     <uses-permission android:name="android.permission.INTERNET"/>

)

No, I'll revert that new line.

comment:21 Changed 5 months ago by sysrqb

Replying to gk:

commit 21149e7a423d1f88866e5781e521021a08fca126 -- not okay

Indentation of code in the else-clauses is missing.

I'll correct these.

Are we good with all the other account manager related things that Orfox patched but this patch omits?

Did you find specific changes I missed or is this a general question? The only commit in Orfox I see is:
https://github.com/guardianproject/tor-browser/commit/b19ab5c8e300042ffc75022099628997e72be773

commit db3c54ef05e71234a009bdc1abdbf9b80135cb0a -- ???

That one is hard for me to review. Is that a new NetCipher version included? If so, what's the new one and what was the old one, used for ESR52?

Yes. The imported files are from commit 26304115de4939f20f023715ab7b079ce7105c1d and (it looks like) the old version was tag 1.2 (commit 2f3e6f0bbea4755617286813b2dd80907d0a573f in the NetCipher repo)

But I thought about this some more. I'll ask Nathan if there were any critical fixes between the 1.2 tag and the current master. If there weren't any critical patches, then I'll revert this commit and cherry-pick Orfox's netcipher import commit from tag 1.2. It may not be worth the time reviewing the changes between versions if we consider we are only using netcipher and Orbot in the first couple alpha releases, and we'll use a tor launcher before the first stable is released.

Do we really want to have mobile/android/orfox? What happened to

+  <string name="no_space_to_start_error">&no_space_to_start_error;</string>
+  <string name="error_loading_file">&error_loading_file;</string>

?

That came from this commit,
https://github.com/guardianproject/tor-browser/commit/b2c793dc73c23c60d5d3e22c6a138fad9f7cd1b2

Those strings aren't included anymore. I missed the orfox -> torbrowser change. I'll correct that.

comment:22 in reply to:  19 Changed 5 months ago by sysrqb

Replying to gk:

commit a2c4e6217da42219327206508015ee24846907c4 -- probably okay (Any reason the Facebook bg color (#ffecf0f3) did not get adjusted as done in the original patch?)

No, I applied the patch manually because there were many conflict. I think I chose keeping the Fennec background color instead of changing it to #ffecf0f3 because #3B5998 is the normal "Facebook Blue".

commit 91eeee498b6733075d1de69d9f3568bfbffb5f05 -- not okay

I assume the resistfingerprinting related UA is not being used by Java code automatically? If so, then we should hardcode that one and not a custom one.

Okay, I'll change the UA strings so they are hard coded instead of concatenating static values.

commit 14cefdc4bd90083e22ac263954a857d254b346a9 -- not okay

Indentation of the if-clauses

Ack.

comment:23 in reply to:  20 Changed 5 months ago by gk

Replying to sysrqb:

Replying to gk:

[snip]

I guess the comments before the *strip options are essentially enabling stripping? if so, we should be explicit about it an use --enable-strip like on other platforms as well

The defaults are effectively --disable-strip and --enable-install-strip but we should test these options and understand what improves and what fails when we enable/disable them. I'm still not sure.

Well, that makes it so that we build with symbols but then strip them during the packaging step (that can be confusing alone in case you want to get debug symbols after packaging up everything). It seems to be cleaner to me to indicate upfront in the .mozconfig file that we don't have symbols available by specifying --enable-strip.

[snip]

commit 69bdd94ecb8e97e4d590dc75c04963b6659bdae0 -- probably okay (why do we need the duplicated entries we already have in confvars.sh?)

I added those in torbrowser.configure because they look like configure flags but they are environment variables. They are still in torbrowser.configure only for documentation purposes. I can delete them if it's confusing or you think they aren't helpful.

I don't feel that strongly about it. Let's keep the commit as-is if you want.

[snip]

Why is "#ifdef TOR_BROWSER_VERSION" commented out? It seems to me we don't want to point our users to the aus5 URL. Maybe I am missing something here.

Hm! Those // were only supposed to be visual and not affect the inclusion. I thought the preprocessor preserves and enforces the ifdef when it scans the file - but I see this did not happen. app.update.url.android is still set. I'll look into this more.

I don't think that's the case for the .js file. At any rate, I would follow the style Mozilla has in this file which is plain #ifdef's.

[snip]

commit 4b3c94077749e620f8d9055412ab01bf4286b435 -- probably okay (What is the threat here?)

I think this provides a safe default. When this is enabled, the user is provided an icon in the awesome bar for scanning a QRCode that (presumably) contains a URL the user wants to visit. When the icon is pressed, another app is opened (if it is installed) where a picture is taken and it is scanned for a QRCode and then decoded. I believe the scanning/decoding is performed on a remote server. Disabling this by default protects against accidentally revealing their location to third parties. The user could tap the icon by mistake and launch the third party app, and we don't have any control over what that app does or what information that app sends.

Good point. I was not aware that the QRCode treatment is actually done remotely, so, yes, let's keep the commit.

[snip]

comment:24 in reply to:  21 ; Changed 5 months ago by gk

Replying to sysrqb:

Replying to gk:

[snip]

Are we good with all the other account manager related things that Orfox patched but this patch omits?

Did you find specific changes I missed or is this a general question? The only commit in Orfox I see is:
https://github.com/guardianproject/tor-browser/commit/b19ab5c8e300042ffc75022099628997e72be773

I looked at the Orfox links in https://bugzilla.mozilla.org/show_bug.cgi?id=1314778 and grepped a bit in the esr60 source code. I am not sure why Orfox for esr52 does not have all the fixes that could still be applied. Maybe that's not necessary anymore for a reason I don't see at first glance, or maybe that's been an oversight? At any rate there are still unpatched cases for esr60 that were patched back then (e.g. AccountManager related part in SendTab.java)

commit db3c54ef05e71234a009bdc1abdbf9b80135cb0a -- ???

That one is hard for me to review. Is that a new NetCipher version included? If so, what's the new one and what was the old one, used for ESR52?

Yes. The imported files are from commit 26304115de4939f20f023715ab7b079ce7105c1d and (it looks like) the old version was tag 1.2 (commit 2f3e6f0bbea4755617286813b2dd80907d0a573f in the NetCipher repo)

But I thought about this some more. I'll ask Nathan if there were any critical fixes between the 1.2 tag and the current master. If there weren't any critical patches, then I'll revert this commit and cherry-pick Orfox's netcipher import commit from tag 1.2. It may not be worth the time reviewing the changes between versions if we consider we are only using netcipher and Orbot in the first couple alpha releases, and we'll use a tor launcher before the first stable is released.

Sounds reasonable to me. I'll wait with further review for that commit then.

[snip]

comment:25 Changed 5 months ago by gk

Further comments:

commit 44c65b7904d8bf599345991942e0407af2030199 -- okay

Don't we need

https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.8.1esr-8.0-1&id=6c507ea953072deb125d3ad4afa3282ae1ce884b
https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.8.1esr-8.0-1&id=eea65c845366312fe2d6b8d955264b2158044db8

?

It seems you omitted

https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.8.1esr-8.0-1&id=468107a61e87df22cb4d75557aac3d7d82e750dc

which means you lean to WONTFIX #24926? Shouldn't we keep the current Orfox behavior until we make a final decision? (I don't have a strong opinion here)

EDIT: I just saw in comment:3 not including that one is actually intentional.

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

comment:26 Changed 5 months ago by gk

Finally:

commit 952f0ae78cc17f66624f3c634d831ff0aedc9692 -- not okay

It seems we don't need

+import java.net.Proxy;
+import java.net.InetSocketAddress;

and regarding

+import ch.boye.httpclientandroidlib.HttpHost;
+import ch.boye.httpclientandroidlib.conn.params.ConnRoutePNames;

The first gets imported twice with that code snippet and could you move the second one to
where it fits into the ch.boye.httpclientandroidlib.conn.* area?

comment:27 Changed 5 months ago by gk

Oh, and could you adjust the commit message of 952f0ae78cc17f66624f3c634d831ff0aedc9692. It's slightly confusing now that https://bugzilla.mozilla.org/show_bug.cgi?id=1357997 got fixed.

comment:28 Changed 5 months ago by gk

Keywords: TorBrowserTeam201807 added; TorBrowserTeam201807R removed
Status: needs_reviewneeds_revision

comment:29 in reply to:  24 Changed 5 months ago by sysrqb

Replying to gk:

Replying to sysrqb:

Replying to gk:

[snip]

Are we good with all the other account manager related things that Orfox patched but this patch omits?

Did you find specific changes I missed or is this a general question? The only commit in Orfox I see is:
https://github.com/guardianproject/tor-browser/commit/b19ab5c8e300042ffc75022099628997e72be773

I looked at the Orfox links in https://bugzilla.mozilla.org/show_bug.cgi?id=1314778 and grepped a bit in the esr60 source code. I am not sure why Orfox for esr52 does not have all the fixes that could still be applied. Maybe that's not necessary anymore for a reason I don't see at first glance, or maybe that's been an oversight? At any rate there are still unpatched cases for esr60 that were patched back then (e.g. AccountManager related part in SendTab.java)

#26858

comment:30 in reply to:  25 Changed 5 months ago by sysrqb

Replying to gk:

Don't we need

https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.8.1esr-8.0-1&id=6c507ea953072deb125d3ad4afa3282ae1ce884b

I noticed yesterday this commit was lost during a rebase. I'll include this in the next branch.

https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.8.1esr-8.0-1&id=eea65c845366312fe2d6b8d955264b2158044db8

Same. I'll cherry-pick this commit in the next branch, too.

?

It seems you omitted

https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.8.1esr-8.0-1&id=468107a61e87df22cb4d75557aac3d7d82e750dc

which means you lean to WONTFIX #24926? Shouldn't we keep the current Orfox behavior until we make a final decision? (I don't have a strong opinion here)

EDIT: I just saw in comment:3 not including that one is actually intentional.

I'm hesitant including this. On the one hand, it's possible some Orfox uses are using this feature and will want it included in TBA. On the other hand, this is a feature uniquely available on Android and it exposes Tor Browser to a new attack vector (DoS by a third-party app). I'm not against the idea of implementing this functionality, especially considering the situations where some people use their mobile devices, but I'd like this provided behind a pref.

It's easy for me to say "I don't think including this is a good idea" and then try justifying not including it. Instead of that, let's include it and then decide on the best way forward with #24926.

comment:31 Changed 5 months ago by gk

One additional thing while I was thinking about our update setup: the app update URL and enabling the MAR signature and verification machine belong together but are dealt with by different means (once by an environment variable check (TB_BUILD_WITH_UPDATER) and the other time by an preprocessor directive (#ifdef TOR_BROWSER_VERSION). I think using once TB_BUILD_WITH_UPDATER and the other time TOR_BROWSER_VERSION is confusing. And if we really want to enable updates sometime in the future we'd need to deal with both as TOR_BROWSER_VERSION is very likely set in both cases: for builds with and without updating enabled.

So, TB_BUILD_WITH_UPDATER (or something similar) should cover both the app update URL and the pieces in the .mozconfig file.

(Oh, and right now it seems we would need two builds if we'd ship an updating and a non-updating version. I wonder if we could design it in a way that we only needed some preference flip during the final packaging step to take care of that. Right I think we can if we always build with the update mechanism enabled but just make sure we don't publish update files/disable update checks per pref).

comment:32 in reply to:  31 ; Changed 5 months ago by sysrqb

Replying to gk:

One additional thing while I was thinking about our update setup: the app update URL and enabling the MAR signature and verification machine belong together but are dealt with by different means (once by an environment variable check (TB_BUILD_WITH_UPDATER) and the other time by an preprocessor directive (#ifdef TOR_BROWSER_VERSION). I think using once TB_BUILD_WITH_UPDATER and the other time TOR_BROWSER_VERSION is confusing. And if we really want to enable updates sometime in the future we'd need to deal with both as TOR_BROWSER_VERSION is very likely set in both cases: for builds with and without updating enabled.

So, TB_BUILD_WITH_UPDATER (or something similar) should cover both the app update URL and the pieces in the .mozconfig file.

(Oh, and right now it seems we would need two builds if we'd ship an updating and a non-updating version. I wonder if we could design it in a way that we only needed some preference flip during the final packaging step to take care of that. Right I think we can if we always build with the update mechanism enabled but just make sure we don't publish update files/disable update checks per pref).

Yes, these are good points. I think I'll delete TB_BUILD_WITH_UPDATER and we can discuss a better way for handling this later. It may be easier if we introduce another configure flag for this. I'm worried about only using a pref for deciding if the app should update itself or something else installs updates (such as an app store). Although it seems Google are not currently enforcing this, Google Play's policy says apps should not be able to update themselves - so having a toggle-able pref seems risky.

comment:33 Changed 5 months ago by sysrqb

Status: needs_revisionneeds_review

Okay, I believe I addressed/corrected everything.

Branch tor-browser-60.1.0esr-8.0-1+26401_2

comment:34 in reply to:  32 ; Changed 5 months ago by sysrqb

Replying to sysrqb:

I'm worried about only using a pref for deciding if the app should update itself or something else installs updates (such as an app store). Although it seems Google are not currently enforcing this, Google Play's policy says apps should not be able to update themselves - so having a toggle-able pref seems risky.

I take this back, I think we can accomplish this without much risk (Google Play's only says we can't update the app, but it doesn't say the app must not be capable of updating itself). #26528 may give us what we need for this.

comment:35 in reply to:  34 Changed 5 months ago by gk

Replying to sysrqb:

Replying to sysrqb:

I'm worried about only using a pref for deciding if the app should update itself or something else installs updates (such as an app store). Although it seems Google are not currently enforcing this, Google Play's policy says apps should not be able to update themselves - so having a toggle-able pref seems risky.

I take this back, I think we can accomplish this without much risk (Google Play's only says we can't update the app, but it doesn't say the app must not be capable of updating itself). #26528 may give us what we need for this.

That's what I had in mind, yes.

comment:36 Changed 5 months ago by gk

Status: needs_reviewneeds_revision

Okay, we are getting close, just the following comments/remarks are left:

commit 0b932f895e3b2866161048a6007e61e257dc395b -- not okay

There is no need for a --enable-install-strip as we already strip everything with --enable-strip.

commit 408e63083f15d59c98fee619a277dfe123429d8c -- not okay

we should follow the Android version as used in the resist fingerprinting mode, see:

https://dxr.mozilla.org/mozilla-esr60/source/browser/components/resistfingerprinting/test/browser/browser_navigator.js

I.e. "Android 4.1.0" > "Android 6.0" which is way more popuplar than 4.1.0 as well.

commit 7bc689d797439d6394853e421f71eda5d7d1631a -- not okay

+import android.content.IntentFilter; <- seems to be something not used
(probably part of the NetCipher upgrade on the previous branch?)

checkStartOrbot() came before processTabQueue() before (and the if-clause around !mHasResumed got deleted). Doesn't the order matter here? (I don't know just for double-checking)

comment:37 in reply to:  36 Changed 5 months ago by sysrqb

Replying to gk:

Okay, we are getting close, just the following comments/remarks are left:

commit 0b932f895e3b2866161048a6007e61e257dc395b -- not okay

There is no need for a --enable-install-strip as we already strip everything with --enable-strip.

Okay.

commit 408e63083f15d59c98fee619a277dfe123429d8c -- not okay

we should follow the Android version as used in the resist fingerprinting mode, see:

https://dxr.mozilla.org/mozilla-esr60/source/browser/components/resistfingerprinting/test/browser/browser_navigator.js

I.e. "Android 4.1.0" > "Android 6.0" which is way more popuplar than 4.1.0 as well.

Hrm. Yes, I see. I did consider using a higher Android version, but I thought it would be better if we used the lowest supported version number instead of the most popular. In any case, I don't feel strongly about this. I'll change the hard-coded UAS so it matches the RFP string.

Also, https://dxr.mozilla.org/mozilla-esr60/source/toolkit/components/resistfingerprinting/nsRFPService.h#35

commit 7bc689d797439d6394853e421f71eda5d7d1631a -- not okay

+import android.content.IntentFilter; <- seems to be something not used
(probably part of the NetCipher upgrade on the previous branch?)

Ugh. That was added in the wrong commit. The import should be in 694067738b04b6a670d072c64a7afa4be63581b1 ("Orfox: add BroadcastReceiver..."). I'll move it.

checkStartOrbot() came before processTabQueue() before (and the if-clause around !mHasResumed got deleted). Doesn't the order matter here? (I don't know just for double-checking)

Yeah, this was something I looked at while I was rebasing the commit. Calling checkStartOrbot() was moved as a fix for a crash (https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.8.1esr-8.0-1&id=dc6f0c7a561321460ca0f5987f30017ee1066e38).

I think the !mHasResumed clause was deleted in Orfox as a way of preventing the app from bringing itself to the foreground the next time it was paused (put into the background). After it was deleted, mHasResumed would never be set to true in onResume(), so when onPause() is called the mHasResumed conditional isn't executed and Prompt:ShowTop is never registered. I don't know why this was missed during the rebase - I might've been confused while resolving a merge conflict. I'll include this deletion.
https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.8.1esr-8.0-1&id=7040f97c97b238b2adef4cae96471907d38caf1d

comment:38 Changed 5 months ago by sysrqb

Status: needs_revisionneeds_review

Okay, I have another branch for review - tor-browser-60.1.0esr-8.0-1+26401_3

igt0, can you review this new branch, too? (assuming gk doesn't find anything else that needs changing).

comment:39 Changed 5 months ago by gk

LGTM. Let's get this merged tomorrow (plus the other stuff that is ready on top of it).

comment:41 in reply to:  40 Changed 5 months ago by gk

Keywords: TorBrowserTeam201807R added; TorBrowserTeam201807 removed
Resolution: fixed
Status: needs_reviewclosed

Replying to igt0:

I think we dont need this commit:
https://gitweb.torproject.org/user/sysrqb/tor-browser.git/commit/?h=tor-browser-60.1.0esr-8.0-1%2b26401_3&id=109f0a19ed90738210577cdca051564c0f57a1c6

We are going to implement the onboarding in the https://trac.torproject.org/projects/tor/ticket/25696

Good point. I took all the patches from tor-browser-60.1.0esr-8.0-1+26401_3 except the above one and applied them to tor-browser-60.1.0esr-8.0-1, woo!

Besides that, nice work!

Indeed, thanks all!

Note: See TracTickets for help on using tickets.