Closed Bug 1739114 Opened 3 years ago Closed 3 years ago

Gate each privileged extension feature by a webextension permission

Categories

(WebExtensions :: General, enhancement, P2)

Product:
WebExtensions
Component:
General
Version:
Firefox 103
Type:
enhancement
Points:
3

Tracking

(firefox103 fixed)

Status:
RESOLVED FIXED
Milestone:
103 Branch
Tracking Flags:
Tracking Status
firefox103 --- fixed

People

(Reporter: TheOne, Assigned: mixedpuppy)

References

(Blocks 1 open bug)

Details

(Whiteboard: [addons-jira])

Attachments

(3 files, 1 obsolete file)

Bug 1739114 Enforce addon privilege during manifest parsing for access to privileged features from temporarily installed addons.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1739114 - Moved privileged WebExtensions permissions in their own JSONSchema type and added a new custom privileged keyword on each privileged manifest fields. r?robwu!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1739114 - Port to TB: Enforce addon privilege during manifest parsing for access to privileged features from temporarily installed addons. r=mkmelin
48 bytes, text/x-phabricator-request
Details | Review

In order to reliably distinguish regular add-ons from privileged add-ons at submission time (ie before they are getting signed), and the different submission processes for them, these systems need to be able to assess whether the submitting file can be handled by the receiving system.

In other words, when submitting an add-on/version that uses privileged features to AMO, AMO needs be able to recognize that and deny the submission.
Similarly, when submitting an add-on/version that does not use privileged features to shipit, shipit needs to able to recognize that and deny the submission.

Vice versa, AMO and shipit will be able to recognize and accept regular and privileged submissions respectively.

In order to achieve this, each privileged feature should be gated by a specific webextension permission, so those can be evaluated during submission.

Andreas: Is this a burning issue on AMO, or is it something that can take time to think about?

Flags: needinfo?(awagner)

It is somewhat light-burning as it is a prerequisite for handling privileged add-ons on AMO and shipit. Is this something that could ride the 97 or 98 train?

Flags: needinfo?(awagner)

It is not necessary to make Firefox-specific changes to support handling privileged add-ons in the signing infrastructure.

We could introduce a rule that an add-on needs a privileged permission (or a privileged manifest key such as experiment_apis) in order to get a privileged signature. Then:

  • Add-ons that already have a privileged permission will continue to be signed as desired.
  • Add-ons that do not have a privileged permission yet can use one of the existing privileged permissions.
  • Add-ons that do not have a privileged permission even though they should will discover soon enough that they need a privileged permission, and an existing privileged permission can be used as a dummy value.

The existing mozillaAddons permission has a general name, but it does only one thing: expanding the set of supported match patterns. It's reasonable to expose that functionality.

Andreas, would that work for you?

As far as I understand, not every internal feature that requires a privileged permission is actually gated by one. To my understanding, some are only gated by the check for a privileged signature.

Indeed, some features are only gated by a privileged signature check and not by a privileged permission.
If the goal is merely to recognize supposedly privileged add-ons, then that can be done by recommending privileged add-ons to include a privileged permission (or even a dummy permission) as I mentioned in comment 3.

Otherwise, if we really want to check the use for every individual feature, then we'd need to introduce a new permission (or manifest key) for features that are not already behind a permission or manifest key.

The full list of features tied to the privileged state (= signed with a privileged signature / being a built-in extension / loaded temporarily with the experiments enabled preference) is as follows:

(In reply to Rob Wu [:robwu] from comment #5)

Indeed, some features are only gated by a privileged signature check and not by a privileged permission.
If the goal is merely to recognize supposedly privileged add-ons, then that can be done by recommending privileged add-ons to include a privileged permission (or even a dummy permission) as I mentioned in comment 3.

:rpl didn't want us to (ab-)use mozillaAddons, which is why I suggested to introduce a new dummy permission last time our team spoke about this issue.

I personally think that relying on a permission rather than the signature is superior because it is easier to lookup the permission in the manifest file.

Having a separate privileged permission for each use case of privileged features seems like too much overhead and bureaucracy, it could make it harder to reason about security and miss the forest for the trees while writing/reviewing code in firefox.

Furthermore, requiring any new permission (in Firefox) for things that previously didn't need one would break all existing already signed addons floating who knows where.

Possible middle ground would be to require mozillaAddons for builtins (since we know of all of them) and temporary loaded extensions -- to obviously break them during development:
https://searchfox.org/mozilla-central/rev/633345116df55e2d37be9be6555aa739656c5a7d/toolkit/components/extensions/Extension.jsm#2224-2225

And after that we can use the presence of mozillaAddons in the manifest to determine if AMO or ShipIt should be used for signing, and refuse in the other one.

Possible middle ground would be to require mozillaAddons for builtins

builtins do not require signing, so this should not be done.

Luca, Shane, Andreas and I met, and we decided to require permissions for privileged features (and re-use mozillaAddons when a feature does not need its own dedicated permission). A benefit of enforcing this in Firefox is the reduction of non-obvious exposed abilities associated with privileged signatures. The allowed private browsing feature won't be tied to permissions, it'll still be tied to the isPrivileged flag.

The goal is to do this before the next ESR.

Severity: -- → N/A
Points: --- → 3
Priority: -- → P2
Whiteboard: [addons-jira]

I'm marking this as dependent on bug 1734987 since the work there simplifies the implementation of parsing and ignoring/warning about the use of privileged stuff.

Depends on: 1734987

I put an overview of the currently active privileged extensions for examination into a spreadsheet.

I've talked through this again with Luca. We will add a new permission, "privilegedAddon". In firefox this will essentially do nothing. The pipeline will need to enforce that all new signing requires this permission. Then AMO can also require it, and fail if any other privileged permission exists without it.

Assignee: nobody → mixedpuppy
Status: NEW → ASSIGNED

(In reply to Shane Caraveo (:mixedpuppy) from comment #12)

I've talked through this again with Luca. We will add a new permission, "privilegedAddon". In firefox this will essentially do nothing. The pipeline will need to enforce that all new signing requires this permission. Then AMO can also require it, and fail if any other privileged permission exists without it.

Why can't we gate privileged features on that permission in Firefox? The downside of it doing nothing is that the add-on will work just fine in Firefox during development without it, so they don't know they need to add it. Then, they'll either submit to AMO which will accept and sign it, only for end-users to notice it's broken, or they will realize it needs to go through shipit, at which point shipit will deny the submission because the permission is missing. This sounds like a bad experience to me in either case.

Linked as a "see also" the github isssue filed in the add-ons linter to track the changes that will be necessary on the addons-linter side.

See Also: → https://github.com/mozilla/addons-linter/issues/4306

This patch includes only the subset of D145687 changes related to the reworked JSONSchema data, plus some minor changes to Schemas.jsm to take the new
JSONSchema type ("PrivilegedPermissions") and the new custom JSONSchema keyword (the boolean "privileged" property used to identify manifest fields
only allowed in privileged extensions).

Besides the changes to the schema data, this patch is not expected to introduce any difference in behavior and so it could also land on its own
if needed (and the rest of the changes landed separately).

Attachment #9274484 - Attachment is obsolete: true
Pushed by luca.greco@alcacoop.it: https://hg.mozilla.org/integration/autoland/rev/d3577f741ef5 Moved privileged WebExtensions permissions in their own JSONSchema type and added a new custom privileged keyword on each privileged manifest fields. r=robwu,mixedpuppy

Backed out changeset d3577f741ef5 (Bug 1739114) for causing xpcshell failures on test_ext_permissions.js.
Backout link
Push with failures <--> X3
Failure Log

Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mixedpuppy) → needinfo?(lgreco)

As I suspected after taking a look to the failure message, there was a one line change to test_ext_permissions.js that was still in the second patch while I should have moved it in the first one (along with the related JSONSchema data changes).

I've just updated the two patches to move that small change into the right patch.

I'm going to push the first patch to autoland again in a few minutes.

Flags: needinfo?(lgreco)
Pushed by luca.greco@alcacoop.it: https://hg.mozilla.org/integration/autoland/rev/37372e77b282 Moved privileged WebExtensions permissions in their own JSONSchema type and added a new custom privileged keyword on each privileged manifest fields. r=robwu,mixedpuppy
Attachment #9275366 - Attachment description: Bug 1739114 enforce addon privilege during manifest parsing for access to privileged features → Bug 1739114 Enforce addon privilege during manifest parsing for access to privileged features from temporarily installed addons.
Pushed by luca.greco@alcacoop.it: https://hg.mozilla.org/integration/autoland/rev/1561d8316c29 Enforce addon privilege during manifest parsing for access to privileged features from temporarily installed addons. r=robwu,mixedpuppy,willdurand

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/b0e5130a7478
Port to TB: Enforce addon privilege during manifest parsing for access to privileged features from temporarily installed addons. r=mkmelin

Keywords: checkin-needed-tb
See Also: → 1771908
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Version: unspecified → Firefox 103

Is this change supposed to break theme_experiments?

(In reply to matthias koplenig [:metasieben] from comment #27)

Is this change supposed to break theme_experiments?

theme_experiments should still be allowed for permanently installed privileged extensions and for development purpose to temporarily installed extensions while "extensions.experiments.enabled" is also set to true (those are both cases that are officially supported, and so they are also covered by automated tests).

may I ask you if you could provide some more context so that I can investigate it?

if you could provide the steps to reproduce and a test extension that we could use to reproduce the issue as you are experiencing it, that would make it investigating the potential change in behavior even quicker.

Thanks in advance for your help!

Flags: needinfo?(mozilla)

Thank you for clarifying the change.

The "use-case" I was wondering about:
Up until now you could disable signing and permanently install a theme with a theme_experiment property.

Flags: needinfo?(mozilla)

(In reply to matthias koplenig [:metasieben] from comment #29)

Thank you for clarifying the change.

The "use-case" I was wondering about:
Up until now you could disable signing and permanently install a theme with a theme_experiment property.

Thanks a lot for reporting it to us quickly, very much appreciated!

I filed Bug 1773076 to track following up on that.

status-firefox103: --- → fixed
Target Milestone: --- → 103 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: