Validator should flag enablePrivilege and codebase_principal_support

VERIFIED FIXED in 5.8

Status

addons.mozilla.org Graveyard
Admin/Editor Tools
P2
enhancement
VERIFIED FIXED
8 years ago
a year ago

People

(Reporter: Jesse Ruderman, Assigned: jorgev)

Tracking

unspecified

Details

(Whiteboard: [ReviewTeam])

Attachments

(2 attachments)

(Reporter)

Description

8 years ago
Addons shouldn't be giving web sites huge swaths of privileges, or encouraging users to make their browser less secure.

Also, Firefox is removing these features in bug 546848.

It's kinda scary how many addons use these according to https://mxr.mozilla.org/addons/.  Even a few non-experimental addons such as https://addons.mozilla.org/en-US/firefox/addon/6511 and https://addons.mozilla.org/en-US/firefox/addon/3688.
(Assignee)

Updated

8 years ago
Assignee: nobody → jorge
Priority: -- → P2
Target Milestone: --- → 5.8

Comment 1

8 years ago
wondering if this should retrigger a review of the addons that have changed this pref, or if its good enough to just wait for another update to the addon and a follow on submission.

Also wonder if we should reorganize and make a stronger statement about the risk of associated with flipping this pref in https://developer.mozilla.org/en/Bypassing_Security_Restrictions_and_Signing_Code

We hint at risks and say it should only be used for testing but some of that is buried at the bottom of the article.   Should we add a strong   ***WARNING*** message at the top of the article that explains the danger of doing any general purpose browsing with this pref set?

Comment 2

8 years ago
The validator should check for explicit setting of the pref, but if it can it might also add some warnings if an addon does any recommending to users that manual pref setting is required/advised.  See this example.

https://mxr.mozilla.org/addons/source/9147/chrome/content3/Toolbar%20Permission.html?force=1

8 <h1>The handwriting tool doesn't work?</h2>
9 <p>The nciku Toolbar should have automatically been granted the necessary security priveliges when it was installed.</p>
10 <p>However, if this has failed (for example, if Windows Vista has prevented it from gaining these privileges), you will need to grant these manually as follows:</p>
11 
12 <ol>
13         <li>Enter "about:config" into Firefox's location bar. Press Enter.</li>
14         <li>Enter "signed.applets.codebase_principal_support" in the filter box.</li>
15         <li>If the item "signed.applets.codebase_principal_support" is false, double click it to set it to true.</li>
16         <li>Try the handwriting tool again. Enjoy.</li>
17 </ol>
https://mxr.mozilla.org/addons/search?string=enablePrivilege
"Too many hits, showing the first 1000" :-(

I guess we don't have to scour intranets, the "Dark Matter" is more visible than I thought.
If it makes you feel any better, the first several I checked were mostly cargo-cult (as in, they're making the enablePrivilege call in chrome code, often with bogus comments like "need this to work with XPCOM").  A number seem to be cribbing from http://www.captain.at/ajax-file-upload.php but doing it in their chrome code....

There are some exceptions to that (e.g. the YSlow "UniversalBrowserRead so we can read our chrome:// logo into a JS string" thing), of course.

Comment 5

8 years ago
as mentioned on IRC we also should figure out what should happen to addons out there right now that will get flagged by the valdiator when the validator gets changed?

back to the sandbox?
removed?
grandfathered for some grace time?

should the addons that have shipped with pref changing code be required to go back and reset the pref to make users safe again?
My 2c:

1) Validators that are flagged should be grandfathered until we ship our next
   Firefox version (the one that drops enablePrivilege support).  Possibly past
   that if they can easily demonstrate that they only use the enablePrivilege
   code in older Geckos.  That said, we should encourage them to remove the code
   ASAP unless they really truly need it for some reason (unlikely, per
   comment 4).
2) Addons that have added such prefs should reset them, yes.  Note that we will
   be making the prefs no-ops going forward if all goes right, so this only
   affects downrev versions.
(Reporter)

Comment 7

8 years ago
I'd prefer not to force addon authors to reset the pref.  It could lead to conflicts between extensions during the grace period, where an updated extension sets it to false while another relies on it being true.

Comment 8

8 years ago
are users seeing conflicts in extensions worse than having a few hundred thousand people surfing to any page on web with codebase_principal_support=true for the next many months until they update to a version of firefox that no longer has support for that feature?

is this the right way to think about the trade off?
(Assignee)

Comment 9

7 years ago
(In reply to comment #8)
> are users seeing conflicts in extensions worse than having a few hundred
> thousand people surfing to any page on web with codebase_principal_support=true
> for the next many months until they update to a version of firefox that no
> longer has support for that feature?
> 
> is this the right way to think about the trade off?

I don't think this is such a big security problem. The preference only enables websites to *ask* for additional permissions, and the user gets this big scary warning that defaults to "Deny". I agree with Jesse that forcing authors to reset the preference will cause more harm than good.

Patch for the validator coming up.
(Assignee)

Comment 10

7 years ago
Created attachment 432864 [details] [diff] [review]
Patch V1. Add validation for the preference and function, plus explanation in validation page

Here's the first version of the patch. Should be pretty straightforward.
Attachment #432864 - Flags: review?(clouserw)
Attachment #432864 - Flags: review?(clouserw) → review+
(Assignee)

Comment 11

7 years ago
Fixed, r64247.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

7 years ago
You can verify here: https://preview.addons.mozilla.org/en-US/developers/versions/validate/55547/run#test-results-39336
Created attachment 433081 [details]
Post-fix screenshot
Verified FIXED on https://preview.addons.mozilla.org/en-US/developers/versions/validate/55547/run.

Thanks, Jorge!
Status: RESOLVED → VERIFIED
(Assignee)

Updated

7 years ago
Whiteboard: [required amo-editors]
(Assignee)

Comment 15

5 years ago
Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...
Whiteboard: [required amo-editors] → [ReviewTeam]
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.