Closed Bug 101150 Opened 23 years ago Closed 23 years ago

nsScriptSecurityManager does not pick up user prefs correctly (initialized before profile is picked)

Categories

(Core :: Security: CAPS, defect)

defect
Not set
major

Tracking

()

VERIFIED FIXED

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(3 files, 1 obsolete file)

Doron, this is the problem you're having. I've figured out what the deal is..... :) Here is the problem. When we start the browser, the following things happen: 1) an nsScriptSecurityManager is created 2) This calls nsScriptSecurityManager::InitPolicies() 3) This fills out the mClassPolicies hash of classes we have policies for based on _default_ prefs 4) The profile picker is shown 5) A user chooses a profile 6) The user's prefs files are read but the nsScriptSecurityManager does not react to them in any way. Now in Doron's case he was setting: user_pref("capability.policy.default.HTMLImageElement.src.set", "noAccess"); in prefs.js. Since the default prefs files had no policies for HTMLImageElement or *.src, there were no entries in the mClassPolicies hash that matched that pref. As a result, the security manager had no idea that pref existed and never tried to look it up (see nsScriptSecurityManager::GetSecurityLevel(), which only looks up prefs if the class or *.property is in the hash). So it returned the default DOM access level of sameOrigin. Once I added an entry for HTMLImageElement in all.js (for a totally different property, just to test), the pref was picked correctly. So we need to either 1) Make nsScriptSecurityManager notice new prefs being added to the capability.policy pref branch and put the classes in its hash2) Make nsScriptSecurityManager initialize after a profile is selected (not so good with -turbo and profile switching) 3) Include _some_ policy for every class we have in all.js (defeating the point of the hash?)
Attached file Testcase (deleted) —
taking this. reviews?
Assignee: mstoltz → bzbarsky
Attachment #50431 - Attachment is obsolete: true
My bad. I've known about this problem for a while but I hadn't filed a bug. I'm actually thinking about rewriting the policy storage mechanism to make it simpler - it's getting crufty with special cases - and I'd like to do away with mClassPolicies in the process. So the code you've changed may get blown away soon, but in the meantime this patch looks pretty good and I'd be happy to have it in there as a temporary fix. I'm going to give your patch a try later today, and I will wait untill then to give it r=.
actually, I was doing it dynamically in my new scripts preference tab xul/js, not via prefs.js.
Mitch, have you had a chance to try this?
Sorry, I've been swamped. I will try to look at it today.
Comment on attachment 51614 [details] [diff] [review] A few cosmetic changes to Boris's patch Thanks for taking the initiative on this, Boris. I've made a few whitespace changes, moved the declaration of the static strings down to where they're used, and removed the "|| mIsLoadingPrefs" from the added if clause in Observe(), as it's not necessary. r=mstoltz.
Attachment #51614 - Flags: review+
> removed the "|| mIsLoadingPrefs" from the added if clause in Observe(), as it's > not necessary It's not? The comparison to sPrincipalPrefix has it.... and it makes sense to have it -- we don't want to do anything on a pref change if we caused the pref change, do we?
No, nsScriptSecurityManager never changes capablity.policy prefs, it only reads them. So we don't need to check mIsWritingPrefs in this case.
Comment on attachment 51614 [details] [diff] [review] A few cosmetic changes to Boris's patch sr=jst
Attachment #51614 - Flags: superreview+
checked in
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Blocks: 75371
Veerified on 2001-10-10-branch build on WinNT The test case works as expected, I get the following error: Error: uncaught exception: Permission denied to set property HTMLImageElement.src
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: