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)
Core
Security: CAPS
Tracking
()
VERIFIED
FIXED
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(3 files, 1 obsolete file)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
security-bugs
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
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?)
Assignee | ||
Comment 1•23 years ago
|
||
Assignee | ||
Comment 2•23 years ago
|
||
Assignee | ||
Comment 4•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Attachment #50431 -
Attachment is obsolete: true
Comment 5•23 years ago
|
||
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=.
Comment 6•23 years ago
|
||
actually, I was doing it dynamically in my new scripts preference tab xul/js,
not via prefs.js.
Assignee | ||
Comment 7•23 years ago
|
||
Mitch, have you had a chance to try this?
Comment 8•23 years ago
|
||
Sorry, I've been swamped. I will try to look at it today.
Comment 9•23 years ago
|
||
Comment 10•23 years ago
|
||
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+
Assignee | ||
Comment 11•23 years ago
|
||
> 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?
Comment 12•23 years ago
|
||
No, nsScriptSecurityManager never changes capablity.policy prefs, it only reads
them. So we don't need to check mIsWritingPrefs in this case.
Comment 13•23 years ago
|
||
Comment on attachment 51614 [details] [diff] [review]
A few cosmetic changes to Boris's patch
sr=jst
Attachment #51614 -
Flags: superreview+
Assignee | ||
Comment 14•23 years ago
|
||
checked in
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 15•23 years ago
|
||
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.
Description
•