1.53 KB, patch
|Details | Diff | Splinter Review|
964 bytes, patch
Alexander Sack: review?
Christopher Aillon (sabbatical, not receiving bugmail)
|Details | Diff | Splinter Review|
Created attachment 221025 [details] testcase 1 - code execution using window.setTimeout This works on fx22.214.171.124, fx1.0.8 and moz1.7.13.
Created attachment 221027 [details] testcase 2 - code execution using location.href setter This works on fx2.0a1, fx126.96.36.199, fx188.8.131.52, fx1.0.8 and moz1.7.13.
Created attachment 221028 [details] testcase 3 - code execution using location.href setter This works on trunk, fx2.0a1, fx184.108.40.206, fx220.127.116.11, fx1.0.8 and moz1.7.13.
When a user left-clicks, selects text or presses Ctrl+A, the safe context that comes from the hidden window is used to call listener.notifySelectionChanged(). On Windows and Linux, the hidden window's url is resource://gre/res/hiddenWindow.html. But, on Mac, it is chrome://global/content/hiddenWindow.xul. Does that mean the safe context's global object has chrome privileges on Mac? If so, is it possible to trigger the code execution PoCs in this bug by left-clicking, selecting text or pressing Ctrl+A?
So... this method (and possibly the whole api) looks like it should just be noscript, imo. The only script we have that uses it is the two copies of viewsource.js; we could add another API they can call (one that will check that the caller has UniversalXPConnect privileges or something). Or would making an interface noscript be an API change? In that case we should just change this method to do the UniversalXPConnect check and add a noscript method for all the C++ callers.
I don't like using [noscript] to carry security information like "not suitable for unprivileged script" -- an implementation of that interface could certainly behave appropriately in the face of an unprivileged caller. So let's put the security checks in, but why would the C++ callers need to call something else? They should have privs, and be able to express that (possibly with a helper to push the right principal?), I think.
> They should have privs Generally speaking, in the current state of our security architecture, any C++ call to any function that does a security check against the subject principal should assume that the subject can be anything from chrome to the null principal. You're right that helpers to push the null principal might work here. We could do this in that case; we'd need to modify almost all callsites for this API, create the helpers for pushing the null principal, make those accessible from a variety of modules, add a fair amount of codesize, etc. I have yet to see a decent workable mockup of this even with codesize considerations excluded. We'd also need to document the security restrictions in the API somehow, of course, but I suppose we could use comments. If we can find someone to do all this, on trunk and all branches, I agree it would be better than my proposal...
So... I can't reproduce with testcase 3 on trunk. I get no alert, no security errors, nothing. I'll post a patch that should fix this and even make shaver somewhat happy. ;) But I can't really test it well, since I can't reproduce the bug.
Created attachment 221219 [details] [diff] [review] Proposed fix
Created attachment 221220 [details] [diff] [review] Patch for real...
Comment on attachment 221220 [details] [diff] [review] Patch for real... sr=shaver. I like!
Comment on attachment 221220 [details] [diff] [review] Patch for real... >+pref("capability.policy.default.Selection.addSelectionListener", "UniversalXPConnect"); >+pref("capability.policy.default.Selection.removeSelectionListener", "UniversalXPConnect"); Is there a reason you didn't use "noAccess" like everywhere else? UniversalXPConnect already has a magic pass: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/caps/src/nsScriptSecurityManager.cpp&rev=1.299&mark=2893-2896#2890 I suppose doing it this way could let you check in with an obfuscated comment like "allow signed script access" and make it look like we're expanding permission, not restricting it. r=dveditz either way though. Maybe "noAccess" on the trunk later on.
Comment on attachment 221220 [details] [diff] [review] Patch for real... The Release Team discussed this. It is a hi risk security issue that is also a red flag when it lands on the trunk or 1.8.1. We need to fix this in 18.104.22.168 now. a=timr for drivers
Fixed on 1.8.0 branch. Waiting for tree opening on the other branches.
Bz - you can/should land for 1.8
Fixed on 1.8 branch too...
verified with 22.214.171.124rc2 on Windows. all three testcases look good.
I tried the three test cases on Intel Mac 126.96.36.199rc2; no problems found.
Fixed on trunk.
Created attachment 225485 [details] [diff] [review] 1.0.x version - ready for checkin