564048 - Nix security checks in nsPrefBranch

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[[e-mail]]

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?

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I say nuke it then. We have lots of belts and braces protections in xpconnect and wrapperland these days. And capabilities are going away.

Comment 3

[[e-mail]]

Attachment: patch v1

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...

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 447358 [details] [diff] [review]
patch v1

Yay!!!

Comment 5

[[e-mail]]

http://hg.mozilla.org/mozilla-central/rev/2f382be70ce7

Comment 6

[[e-mail]]

(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.

Comment 7

[[e-mail]]

This landing was mismerged in the following changeset:

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

What's needed to fix this?

Comment 8

[[e-mail]]

I filed bug 635665 on the mismerge.