2.26 KB, patch
|Details | Diff | Splinter Review|
872 bytes, text/html
1.71 KB, patch
|Details | Diff | Splinter Review|
378 bytes, text/html
Untrusted object shouldn't be able to usefully implement nsISecurityCheckedComponent. See bug 352124 comments 40, 47, 48, 55 and attachment 238573 [details]
FYI before it was deemed to unsafe for content to be allowed to set a custom tree view it needed to implement class info or security checked component so that someone could call its methods even once they had been double wrapped.
11 years ago
isn't untrusted a caps question? (not that caps is less orphanned than xpconnect)
It depends on what we really mean by "trusted". There's JS_IsSystemObject, but that would screw over pages with UniversalXPConnect, of course.
(In reply to comment #2) > isn't untrusted a caps question? (not that caps is less orphanned than > xpconnect) (Spare us the attitude -- it's not particularly encouraging to folks like bz who have spent lots of time picking up ownership of xpconnect. Stuff gets orphanned in open source, esr talks about this in CatB. What are you going to do to help?) /be
("orphaned" -- no stress on second syllable; sorry for nit-picking, this is a pet peeve.) Timeless, you're right that unowned code is a problem, even if it's also an inevitable and necessary part of open source ecology. I'm interested in fixing it, and I didn't want to leave this bug without saying so. But it's a topic for another bug. And in xpconnect's case, bz, mrbkap and jst are looking like an ownership troika right now. I'm more concerned about liveconnect ownership. Mail me if you want to talk about this more. /be
Created attachment 267179 [details] [diff] [review] Possible fix So, I'm not exactly sure how to test this fix. It turns out that we need to do a strict test against chrome instead of a UniversalXPConnect check because such a check doesn't really make sense because UniversalXPConnect only applies to a single stack frame, where this object might have been created a long time earlier, and we don't keep enough information around. I'm requesting review, but I'll want to have a testcase before I can check this in.
11 years ago
By the way, I couldn't use JS_IsSystemObject, since we can't set it for JS components, so the patch would defeat nsISecurityCheckedComponent being scriptable in the first place. Unfortunately, we can't really use that bit for JS components at all because the "system" bit is per-object, and we don't have fine-grained control over each object created in components.
Created attachment 268535 [details] testcase This testcase seems to test this patch. We end up trying to wrap the object that we've passed to setUserData, which calls canCreateWrapper on it. This testcase shows an alert on current trunk builds, and throws a security exception with my patch.
Fix checked into trunk.
Hmm.. though shouldn't that testcase really work? Why aren't we able to stick a JS object inside nsIVariant? But that is a different bug.
Yeah, IMO it should be possible to stick a regular JS object in an nsIVariant. I'll file a bug on that.
I filed bug 384632.
Please correct me if wrong: short of finding another case like bug 352124 we don't know a specific security exploit for this at the moment. We're fixing this as a preventive measure to remove a stepping stone to future exploits. Is this OK to land on branch?
Yes, this should be OK to land on branch.
Comment on attachment 267179 [details] [diff] [review] Possible fix approved for 18.104.22.168 and 22.214.171.124, a=dveditz for release-drivers
Fixed on the 1.8 branches.
Can someone give a test case or repro steps to verify this in the branch?
Al, currently, if clicking on the button in the testcase in this bug produces: Error: uncaught exception: Permission denied to create wrapper for object of class UnnamedClass in the error console, then you can verify this (once bug 384632 is fixed, this testcase won't be valid anymore).
(In reply to comment #19) > Al, currently, if clicking on the button in the testcase in this bug produces: > > Error: uncaught exception: Permission denied to create wrapper for object of > class UnnamedClass > > in the error console, then you can verify this (once bug 384632 is fixed, this > testcase won't be valid anymore). > Hi Blake, when i use this testcase i get not this expected Error i get Error: uncaught exception: [Exception... "Failure arg 2 [nsIDOM3Document.setUserData]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: https://bugzilla.mozilla.org/attachment.cgi?id=268535 :: go :: line 16" data: no] with the german 126.96.36.199 Release Build Mozilla/5.0 (Windows; U; Windows NT 6.0; de; rv:188.8.131.52) Gecko/2007071317 Firefox/184.108.40.206
I don't have a way to usefully figure out why the testcase fails altogether on the 1.8 branch right now. I'll see if I can come up with another testcase, but it's going to be pretty difficult -- this requires either a patch to mozilla or a way to create a double-wrapped object to test.
Investigation shows that setUserData and friends aren't implemented on the 1.8 branch. Furthermore, the nsIDOMUserDataHandler interface doesn't even inherit from nsISupports on the 1.8 branch, which is what was throwing the exception in the testcase. I'll verify this by hacking a patch that shows that this bug is fixed.
Created attachment 275061 [details] [diff] [review] Patch to help with verification This prints "PASS" to the console if the testcase passes and "FAIL" otherwise.
Created attachment 275062 [details] Page to help verify This calls insertBefore with an untrusted object.
I verified that this patch shows the expected behavior on Linux on the 1.8 branch.
Blake: On the trunk (with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007081804 Minefield/3.0a8pre), the testcase throws up a "FAIL" js alert. Is that bad?
(In reply to comment #26) > Blake: On the trunk (with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; > rv:1.9a8pre) Gecko/2007081804 Minefield/3.0a8pre), the testcase throws up a > "FAIL" js alert. Is that bad? Sorry for the confusion. Fixing bug 384632 made this very hard to test on trunk. If you'd like, I could create a patch a la attachment 275061 [details] [diff] [review] to help verify.