Opened 7 months ago

Closed 4 weeks ago

Last modified 4 weeks ago

#32418 closed defect (fixed)

Torbrowser tells on every start, that it can't update although it is newest

Reported by: Yeti Owned by: mcs
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-update, TorBrowserTeam202004R, tbb-9.5a12
Cc: tbb-team Actual Points: 1.7
Parent ID: Points:
Reviewer: Sponsor:

Description

Torbrowser 9.01/Windows 7 x86

Torbrowser tells on every start, that it can't update although it is newest

(I try to attach a screenshot, this seems to be difficult)

Child Tickets

Attachments (1)

tor.png (47.3 KB) - added by Yeti 7 months ago.
message on startup

Download all attachments as: .zip

Change History (21)

Changed 7 months ago by Yeti

Attachment: tor.png added

message on startup

comment:1 Changed 7 months ago by pili

Status: newneeds_information

Hi,

Can you please enable update logging (visit about:config and update app.update.log preference to true.) You should then be able to see the update check logs in the browser console.

It would be helpful if you can send those on or let us know if you see any errors there.

Thanks!

comment:2 Changed 7 months ago by mcs

Keywords: tbb-update added

comment:3 Changed 7 months ago by Yeti

Thanks, now I see, that Torbrowser misses write access for the program directory. I guess, this check should be done, when updates exist, not before.

I install Torbrowser into %programfiles%, where users don't have write-access. Virtualization is disabled. Datadir I made writable (but not executable), because I didn't find a setting for relocating it. I handle updates manually as administrator. That's the safest way to avoid compromizing program files.

update_messages.log:
Logging current UpdateService status:
UpdateService.canCheckForUpdates - able to check for updates
getCanApplyUpdates - unable to apply updates without write access to the update directory. Exception: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.create]" nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)" location: "JS frame :: resource://gre/modules/FileUtils.jsm :: FileUtils_getDir :: line 76" data: no]
getCanStageUpdates - staging updates is disabled by preference app.update.staging.enabled
Elevation required: false
Update being handled by other instance: false
Downloading: false
End of UpdateService status

comment:4 Changed 6 months ago by mcs

Kathy and I did some investigation and learned that (on Windows only) Mozilla tries to fix file system permissions whenever it detects that it cannot write to the update info directory or to the directory that contains the application. Unfortunately, there are quite a few places where this kind of fix up is attempted (if you are curious, look for calls to fixUpdateDirectoryPermissions() within toolkit/mozapps/update/UpdateService.jsm and trace the call chains).

If the permission fix up fails, a "manual update required" prompt is shown (which is what the reporter of this bug is seeing).

The entire update service relies upon being able to write to the update info directory, so it will be difficult to support a "read only" update info directory. In other words, Firefox's updater was not designed with this scenario in mind.

A better approach, assuming we think it is a good idea to support it, would be to disable the update checks. Unfortunately, the only way to disable update check in recent versions of Firefox is via the enterprise policies mechanism, and that mechanism is disabled in Tor Browser (see #30575).

Tor Browser team: Do we want to provide a way to disable the update check, e.g., by re-implementing support for the old app.update.enabled pref? We would need to ensure that that pref is checked very early; for example, the UpdateService JS constructor calls a getter that causes the file system permission fix to be attempted on Windows, which would trigger the "manual update required" prompt. Mozilla removed the capability for users to disable the update check because they do not want users to run outdated browsers.

An alternative would be to disable the code that tries to fix file system permissions on Windows, but we would then need to verify that doing so does not lead to another code path that causes the browser to display an update error prompt.

Another alternative would be to decide that installing in a read-only area of the file system and updating the browser manually is not something we can support.

comment:5 in reply to:  4 Changed 6 months ago by boklm

Replying to mcs:

Tor Browser team: Do we want to provide a way to disable the update check, e.g., by re-implementing support for the old app.update.enabled pref? We would need to ensure that that pref is checked very early; for example, the UpdateService JS constructor calls a getter that causes the file system permission fix to be attempted on Windows, which would trigger the "manual update required" prompt. Mozilla removed the capability for users to disable the update check because they do not want users to run outdated browsers.

In the blog comments there were a few people unhappy about not having an easy way to disable updates. In some cases it can be useful to have a way to disable the updater, so I think having support for the app.update.enabled pref would be nice.

comment:6 in reply to:  4 Changed 6 months ago by Yeti

Replying to boklm:

In some cases it can be useful to have a way to disable the updater, so I think having support for the app.update.enabled pref would be nice.

I support that.
At least until Mozilla decides to change this doubtful behaviour. Programs started with user rights shouldn't be able to write into program directories. It's easy to click away the warning at every start but when it gets habitual (or someone writes a userscript for that), it's bad.

Replying to mcs:

Another alternative would be to decide that installing in a read-only area of the file system [...] is not something we can support.

This would be the worst case, especially for security related software. (IMHO this even should be default.)

comment:7 Changed 3 months ago by Yeti

Status "needs_information"... which information is needed exactly?

comment:8 in reply to:  7 Changed 3 months ago by mcs

Cc: tbb-team added

Replying to Yeti:

Status "needs_information"... which information is needed exactly?

I believe I changed the status to needs_information after I posted comment:4. We discussed this ticket during our Tor Browser meeting on 25-November-2019. The consensus was that we should avoid creating and maintaining a patch for this if we can possibly avoid doing so (we know Mozilla will not accept such a patch). Some suggestions were made during the meeting which we have now investigated:

Idea 1: Tell users to set app.update.url to a value that will not cause an update prompt. Unfortunately, end-users cannot do this in a way that persists across browser restarts (this is by design; Mozilla does not want malware to be able to easily disable updates).

Idea 2: Learn from what Tails did to address this issue (see https://redmine.tails.boum.org/code/projects/tails/repository/revisions/e43247dd2558dd391342855796e18c3186a43807). Tails uses a multi-faceted approach:

  1. Permanently point the app.update.url preference to a non-existent place. Tails accomplishes this by modifying one of the omni.ja files within the Tor Browser package, which is not a solution we can recommend to end-users.
  2. Set app.update.disabledForTesting to true. Unfortunately, that preference has no effect outside of test runs, i.e., it is ignored unless the browser is running under Marionette control.
  3. Set app.update.doorhanger to false. This will suppress most of the update-related prompts. However, the older update UI will be used which means that eventually a windowed prompt will be shown to tell users that their browser may be out of date. Also, this is not a long term solution because Mozilla removed support for this pref from recent versions of Firefox. However, it is something that end-users can experiment with in Tor Browser 9.x.
  4. Set app.update.auto to false. This will prevent automatic updates but won't suppress prompts or prevent download attempts. Tails sets this as a "defense in depth" measure to avoid any chance of an automatic update. I don't think this will be helpful for the scenario covered by this ticket.

comment:9 Changed 2 months ago by mcs

Revisiting this issue. We could provide a way for advanced users to disable updates through one of two approaches:

  1. Reinstate support for the old app.update.enabled preference. This could be done by modifying the disabledByPolicy() function in toolkit/mozapps/update/UpdateService.jsm.
  2. Turn support for enterprise policies back on (see #29916 and #30575) while removing support for reading policies from the Windows Registry and the macOS user defaults system. We would modify toolkit/components/enterprisepolicies/EnterprisePolicies.js to remove WindowsGPOPoliciesProvider and macOSPoliciesProvider. That would mean that policies would only work if they were added to a distribution/policies.json within the Tor Browser directory. See https://support.mozilla.org/en-US/kb/customizing-firefox-using-policiesjson

Comments? Kathy and I have a slight preference for approach 2 because that would allow advanced users to use enterprise policy mechanism for other things too. Which method do other people prefer?

comment:10 Changed 2 months ago by boklm

Both approaches sound good to me. Approach 2 allows to use the same method as upstream versions of Firefox to disable updates, so I also have a slight preference for this one.

comment:11 Changed 8 weeks ago by mcs

Actual Points: 0.7

comment:12 Changed 8 weeks ago by mcs

Keywords: TorBrowserTeam202004 added
Owner: changed from tbb-team to mcs
Status: needs_informationassigned

At the Tor Browser team meeting today we agreed to pursue approach 2.

comment:13 Changed 6 weeks ago by mcs

Keywords: TorBrowserTeam202004R added; TorBrowserTeam202004 removed
Status: assignedneeds_review

This is ready for review. First, we need to revert the #30575 patch:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug32418-01&id=7d0d47db46531a32c88db85323cd771761b8bb5d

and then we need to make some small changes to ensure that Enterprise Policies are safe and to suppress some update service log messages:

https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug32418-01&id=71c414d5edc5562a176e98fe3d037f0da6e51afc

(both patches are on brade's bug32418-01 branch).

comment:14 Changed 6 weeks ago by cypherpunks

Wow! What if we do customization of Firefox by policies.json instead of patching?
(71c414d5edc5562a176e98fe3d037f0da6e51afc needs some love to become upstreamable)

comment:15 Changed 4 weeks ago by mcs

Actual Points: 0.71.7

comment:16 in reply to:  13 ; Changed 4 weeks ago by sysrqb

Replying to mcs:

https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug32418-01&id=71c414d5edc5562a176e98fe3d037f0da6e51afc

(both patches are on brade's bug32418-01 branch).

Thanks! The patches look good to me, overall, and I'm fine with taking these patches as-is.

Is there a reason for not using MOZ_PROXY_BYPASS_PROTECTION instead of defining a new JSON_POLICIES_ONLY? The latter is definitely more explicit about why it is defined, so I see some benefit for us in using JSON_POLICIES_ONLY, but I wonder if there is another reason as well.

I agree Mozilla likely won't directly accept this patch, but that's okay.

comment:17 Changed 4 weeks ago by sysrqb

(It looks like tom had a similar idea in https://trac.torproject.org/projects/tor/ticket/29916#comment:1, too)

comment:18 in reply to:  16 ; Changed 4 weeks ago by mcs

Replying to sysrqb:

Is there a reason for not using MOZ_PROXY_BYPASS_PROTECTION instead of defining a new JSON_POLICIES_ONLY? The latter is definitely more explicit about why it is defined, so I see some benefit for us in using JSON_POLICIES_ONLY, but I wonder if there is another reason as well.

Kathy and I think using MOZ_PROXY_BYPASS_PROTECTION is a good idea; we just did not think to do so when we wrote the patch. Do you want us to create a fixup or do you want to take care of it?

We could even do something like the following if we want to continue to use JSON_POLICIES_ONLY within that file for readability:

#define JSON_POLICIES_ONLY MOZ_PROXY_BYPASS_PROTECTION

comment:19 in reply to:  18 Changed 4 weeks ago by sysrqb

Resolution: fixed
Status: needs_reviewclosed

Replying to mcs:

Replying to sysrqb:

Is there a reason for not using MOZ_PROXY_BYPASS_PROTECTION instead of defining a new JSON_POLICIES_ONLY? The latter is definitely more explicit about why it is defined, so I see some benefit for us in using JSON_POLICIES_ONLY, but I wonder if there is another reason as well.

Kathy and I think using MOZ_PROXY_BYPASS_PROTECTION is a good idea; we just did not think to do so when we wrote the patch. Do you want us to create a fixup or do you want to take care of it?

We could even do something like the following if we want to continue to use JSON_POLICIES_ONLY within that file for readability:

#define JSON_POLICIES_ONLY MOZ_PROXY_BYPASS_PROTECTION

That's a good idea. I took care of it. I changed the #define as you suggested. I like that defining JSON_POLICIES_ONLY as MOZ_PROXY_BYPASS_PROTECTION self-documents that this is used for avoiding a proxy bypass vector, and the result of this should only allow json policies.

I tweaked your commit and merged it with commit f81583b0d6d7eefd4b7384e8ee95d528ea4fb303 on tor-browser-68.7.0esr-9.5-1. The tweaked commit is e577d655d2044e3b6636b0bccfbb5bd776148582.

Thanks!

comment:20 Changed 4 weeks ago by sysrqb

Keywords: tbb-9.5a12 added
Note: See TracTickets for help on using tickets.