Closed Bug 564048 Opened 10 years ago Closed 10 years ago

Nix security checks in nsPrefBranch

Categories

(Core :: Preferences: Backend, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dwitte, Assigned: dwitte)

References

Details

Attachments

(1 file)

http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/nsPrefBranch.cpp#791

All of the Get/SetFooPref APIs call through getValidatedPrefName(), which checks if the pref is "capability.*" and goes out to the script security manager if so. Which then checks that the caller has "CapabilityPreferencesAccess":

http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#3271

It baffles me what the point of this is. The only place this makes sense is if the caller is content; but nowadays we already require UniversalXPConnect in order to call into the prefservice at all, right? And we have wrappers to enforce that. Which subsumes this check.

sicking suggests that we kill this off, and replace the checks with assertions that we do indeed have UniversalXPConnect. That seems like a pretty simple thing to do. Objections?
I believe this was a belt-and-braces measure to make sure that someone who accidentally managed to get his hands on a prefbranch couldn't permanently escalate their permissions.
I say nuke it then. We have lots of belts and braces protections in xpconnect and wrapperland these days. And capabilities are going away.
Assignee: nobody → dwitte
Status: NEW → ASSIGNED
Attached patch patch v1Splinter Review
Removes nsISecurityPref and nsIPrefSecurityCheck, which nsPrefBranch and nsScriptSecurityManager implement (resp.), and all related code.

This also removes a CapabilityPreferencesAccess localization string from dom/locales/en-US/chrome/security/caps.properties. I'm not sure whether we want to keep that or not...
Attachment #447358 - Flags: superreview?(jst)
Attachment #447358 - Flags: review?(jonas)
Attachment #447358 - Flags: superreview?(jst) → superreview+
http://hg.mozilla.org/mozilla-central/rev/2f382be70ce7
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to comment #1)
> I believe this was a belt-and-braces measure to make sure that someone who
> accidentally managed to get his hands on a prefbranch couldn't permanently
> escalate their permissions.
Or even navigator.preference, although you nixed that simultaneously.
Depends on: 568069
This landing was mismerged in the following changeset:

http://hg.mozilla.org/mozilla-central/rev/155d97b3f8c9

What's needed to fix this?
Depends on: 635665
I filed bug 635665 on the mismerge.
You need to log in before you can comment on or make changes to this bug.