Closed Bug 1464766 Opened 6 years ago Closed 6 years ago

Allow to relax the signature requirements

Categories

(Toolkit :: Add-ons Manager, defect, P2)

Product:
Toolkit
Component:
Add-ons Manager
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox-esr60 --- fixed
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: glandium, Assigned: glandium)

Details

Attachments

(2 files)

Bug 1464766 - Allow to relax the addon signature requirements.
59 bytes, text/x-review-board-request
aswan
: review+
lizzard
: approval-mozilla-beta+
Details
Backport for ESR60
4.92 KB, patch
RyanVM
: approval-mozilla-esr60+
Details | Diff | Splinter Review 
Downstream, in Linux distros, Firefox is installed as a system application, in a directory that is not writable by users, and that is handled by the distro package manager. The package manager can be used by users to install addons such as language packs.

Signing the xpis in those packages is a hard problem, and from a downstream perspective, doesn't seem required. If malware is able to install an unsigned addon that can be picked up by firefox, the system has bigger problems than an unsigned addon.

There is some precedent in treating the package manager-handled directories in a special way, in the form of the extensions.autoDisableScopes preference, which allows to override the auto-disabling of "side loaded" addons in specific addon scopes.

I propose that we add a similar preference allowing to specify what scopes addons need to be signed in.
Assignee: nobody → mh+mozilla
Something I'm not entirely sure about is whether to hide the warning about unsigned addons or not for those.
Attachment #8981080 - Flags: review?(rhelmer) → review?(aswan)
(In reply to Mike Hommey [:glandium] from comment #0)
> Downstream, in Linux distros, Firefox is installed as a system application,
> in a directory that is not writable by users, and that is handled by the
> distro package manager. The package manager can be used by users to install
> addons such as language packs.

Does this have applications other than language packs?  We're now signing all the language packs produced in automation so ideally Linux distros could just use those.

If that's not sufficient, product will need to approve any weakening of the signing policy.  A more complete explanation of when/how this would be used would be helpful.
Flags: needinfo?(mh+mozilla)
Speaking for Debian, downloading and shipping signed langpacks is not a solution. Packages need to be built from source. We build the langpacks from source tarballs generated from the l10n-central repository. The best that could be done is to extract the signatures from the signed langpacks and regenerate the langpacks. Which means the xpis have to have the exact same content, which is probably not too much of a problem, since the source should be identical... except manifest.json also needs to have the exact same buildid... which doesn't sound great. Also, this would be very fragile and likely to generate invalid xpis.
Flags: needinfo?(mh+mozilla)
It'd also be desirable for system-wide webextensions packages.
If this was just about langpacks, those have a separate pref for whether signing is required and I would have suggested letting distributions create builds that honor that preference.  But given comment 7 it sounds like the request is broader than just langpacks.

So, lets start with an opinion from security here, Dan?

(In reply to Mike Hommey [:glandium] from comment #7)
> It'd also be desirable for system-wide webextensions packages.

How are these handled today?  Or is this something new the distributions want to start doing?  Mostly for my own curiosity, what sort of system-wide webextensions would be installed?
Flags: needinfo?(dveditz)
Debian stable is still on 52esr at the moment, so Debian stable still has mostly old xul extensions. In Debian testing/unstable, things are likely moving towards webextensions. See for example:

https://qa.debian.org/developer.php?login=pkg-mozext-maintainers%40lists.alioth.debian.org

Some of those are probably webextensions already (eg. I suspect tree-style-tab is). There may be others not maintained by the "Debian Mozilla Extension Maintainers" team.

> If this was just about langpacks, those have a separate pref for whether signing is required

AIUI, that pref would drop signing requirements for *all* langpacks. The change I'm proposing still makes user-installed langpacks (from amo, or dropped in directly in ~/.mozilla/...) require a signature.
(In reply to Andrew Swan [:aswan] from comment #8)
> So, lets start with an opinion from security here, Dan?

The biggest danger is users downloading stuff from random websites -- those need to be signed. For a distro-built Firefox packages installed via the distro's package manager (which should be using signatures itself) and then installed into a privileged (not user-writable) directory isn't less safe than installing Firefox in the first place. It's also equivalent to how Mozilla treats system-addons: the ones bundled with Firefox aren't signed (but updates put in the user's profile must be).

This sounds like what glandium wants in comment 0. I'm not sure that's what this patch does, though. We'd want something like a non-user-overridable (i.e. default-only) pref to define magic directories -- this one looks like the user could override. Permission check the directory/files when we load them? Just the install directory and sub-directories?

We'd also want it to be a build-time config switch (like the signing requirement, maybe protected by the same switch?) so that people can't enable this in the Firefox builds Mozilla ships.
Flags: needinfo?(dveditz)
Well, users could override because there's no change in the corresponding prefs files, but it could be added as a locked pref in greprefs.js.
Attachment #8981080 - Flags: review?(rhelmer) → review?(aswan)
(In reply to Mike Hommey [:glandium] from comment #11)
> Well, users could override because there's no change in the corresponding
> prefs files, but it could be added as a locked pref in greprefs.js.

The updated patch does that.
Priority: -- → P2
(In reply to Mike Hommey [:glandium] from comment #13)
> (In reply to Mike Hommey [:glandium] from comment #11)
> > Well, users could override because there's no change in the corresponding
> > prefs files, but it could be added as a locked pref in greprefs.js.
> 
> The updated patch does that.

I'm not seeing this in the patch, just a regular pref that any user could use to disable signing.
Using preferences at all for this seems like overkill, how about just a build-time flag for whether signing is required in system scopes, defaulting to true, but can be overridden by Linux distros?
Flags: needinfo?(mh+mozilla)
You're missing the part where it's locked. Users can't change it.
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #15)
> You're missing the part where it's locked. Users can't change it.

Ah yes, the original comment mentioned greprefs.js.
Anyway, why would we use a locked pref instead of a build-time flag for this.
--disable-signing-in-system-scopes?
Canonical needs this fix so that they can roll out 61 to their users.

Do we have any thoughts on what to do next? Or can they take the fix as is?
Sorry i didn't notice this had been updated, but I'm about to be offline for a week, redirecting...
Attachment #8981080 - Flags: review?(aswan) → review?(kmaglione+bmo)
Attachment #8981080 - Flags: review?(kmaglione+bmo) → review?(aswan)
Hasn't moved in a week, so moving back to :aswan, who should be back, now.
Comment on attachment 8981080 [details]
Bug 1464766 - Allow to relax the addon signature requirements.

https://reviewboard.mozilla.org/r/247192/#review267340

sorry for the delay...
Attachment #8981080 - Flags: review?(aswan) → review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/e5cfaecd8a95
Allow to relax the addon signature requirements. r=aswan
https://hg.mozilla.org/mozilla-central/rev/e5cfaecd8a95
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: See comment 0
User impact if declined: See comment 0
Fix Landed on Version: 63
Risk to taking this patch (and alternatives if risky): For mozilla.org builds, this is effectively a no-op.
String or UUID changes made by this patch: N/A

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8996476 - Flags: approval-mozilla-esr60?
Comment on attachment 8981080 [details]
Bug 1464766 - Allow to relax the addon signature requirements.

Approval Request Comment
[Feature/Bug causing the regression]: See comment 0
[User impact if declined]: See comment 0
[Is this code covered by automated tests?]: N/A
[Has the fix been verified in Nightly?]: It's NPOTB, but I'm using this patch downstream (Debian)
[Needs manual test from QE? If yes, steps to reproduce]: N/A
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's effectively a no-op for mozilla.org builds. Downstreams can opt-in to disable signature requirement in very specific locations.
[String changes made/needed]: N/A
Attachment #8981080 - Flags: approval-mozilla-beta?
Comment on attachment 8981080 [details]
Bug 1464766 - Allow to relax the addon signature requirements.

Sounds useful for distros and shouldn't be risky for our builds, let's uplift for beta 15.
Attachment #8981080 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8996476 [details] [diff] [review]
Backport for ESR60

Makes life easier for downstream Linux distros and doesn't affect Mozilla-built Firefox binaries. Approved for ESR 60.2.
Attachment #8996476 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
In a move I can only attribute to post-PTO brain, I landed the wrong patch on ESR60 (and hilarity ensued). Backed out:
https://hg.mozilla.org/releases/mozilla-esr60/rev/a98818a640cc

And re-landed with the right patch this time:
https://hg.mozilla.org/releases/mozilla-esr60/rev/5ae88d739d8c
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: