Untrusted object should not be able to usefully implement nsISecurityCheckedComponent

RESOLVED FIXED

Status

()

Core
XPConnect
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: mano, Assigned: mrbkap)

Tracking

({fixed1.8.0.13, fixed1.8.1.5})

Trunk
fixed1.8.0.13, fixed1.8.1.5
Points:
---
Bug Flags:
blocking1.9 +
blocking1.8.1.5 +
wanted1.8.1.x +
blocking1.8.0.13 +
wanted1.8.0.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:nse?] block potential stepping stone)

Attachments

(4 attachments)

Untrusted object shouldn't be able to usefully implement nsISecurityCheckedComponent.

See bug 352124 comments 40, 47, 48, 55 and attachment 238573 [details]

Comment 1

11 years ago
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.
Flags: blocking1.9?

Comment 2

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
Blake!
Assignee: dbradley → mrbkap
Flags: blocking1.9? → blocking1.9+
(Assignee)

Comment 7

11 years ago
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.
Attachment #267179 - Flags: review?(jst)

Updated

11 years ago
Attachment #267179 - Flags: review?(jst) → review+
(Assignee)

Updated

11 years ago
Attachment #267179 - Flags: superreview?(bzbarsky)
Attachment #267179 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 8

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

Comment 9

11 years ago
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.
(Assignee)

Comment 10

11 years ago
Fix checked into trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
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.
(Assignee)

Comment 12

11 years ago
Yeah, IMO it should be possible to stick a regular JS object in an nsIVariant. I'll file a bug on that.
(Assignee)

Comment 13

11 years ago
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?
Flags: wanted1.8.1.x?
Flags: wanted1.8.0.x?
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
Whiteboard: [sg:nse?] block potential stepping stone
(Assignee)

Comment 15

11 years ago
Yes, this should be OK to land on branch.
(Assignee)

Updated

11 years ago
Attachment #267179 - Flags: approval1.8.1.5?
Attachment #267179 - Flags: approval1.8.0.13?
Flags: wanted1.8.1.x?
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x?
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.13+
Comment on attachment 267179 [details] [diff] [review]
Possible fix

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers
Attachment #267179 - Flags: approval1.8.1.5?
Attachment #267179 - Flags: approval1.8.1.5+
Attachment #267179 - Flags: approval1.8.0.13?
Attachment #267179 - Flags: approval1.8.0.13+
(Assignee)

Comment 17

10 years ago
Fixed on the 1.8 branches.
Keywords: fixed1.8.0.13, fixed1.8.1.5
Can someone give a test case or repro steps to verify this in the branch?
(Assignee)

Comment 19

10 years ago
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 2.0.0.5 Release Build Mozilla/5.0 (Windows; U; Windows NT 6.0; de; rv:1.8.1.5) Gecko/2007071317 Firefox/2.0.0.5
(Assignee)

Comment 21

10 years ago
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.
(Assignee)

Comment 22

10 years ago
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.
(Assignee)

Comment 23

10 years ago
Created attachment 275061 [details] [diff] [review]
Patch to help with verification

This prints "PASS" to the console if the testcase passes and "FAIL" otherwise.
(Assignee)

Comment 24

10 years ago
Created attachment 275062 [details]
Page to help verify

This calls insertBefore with an untrusted object.
(Assignee)

Comment 25

10 years ago
I verified that this patch shows the expected behavior on Linux on the 1.8 branch.

Comment 26

10 years ago
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?
(Assignee)

Comment 27

10 years ago
(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.
Group: security
Flags: in-testsuite?

Updated

10 years ago
Depends on: 388809
You need to log in before you can comment on or make changes to this bug.