829310 - compartment mismatch in nsWindowSH::NewResolve for dialogArguments with Xrays

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect)

Description

[Andrew McCreight [:mccr8]]

Found by inspection in bug 826471: "The chunk of code in nsWindowSH::NewResolve for sDialogArguments_id also looks sketchy to me. It grabs the argument object off of the window, then uses the window's context to set a property on obj, which in this Xray case is not in the same compartment as the window, I believe. nsJSContext::SetProperty does an XPCAutoRequest, but doesn't enter a compartment anywhere as far as I can see."

I've tried writing a testcase, but I haven't managed to yet.  dialogArguments is apparently only present with showModalDialog.

Comment 1

[Andrew McCreight [:mccr8]]

I don't know how exploitable this actually is.  It is pretty old code.

Comment 2

[Daniel Veditz [:dveditz]]

Can web content affect the dialog arguments, or is this only a problem for privileged code?

Comment 3

[Andrew McCreight [:mccr8]]

I think it can choose what it is, but this only is a problem when Xrays are involved, and maybe those are only from privileged code?  I haven't seen any crashes with this in the wild, so maybe it is not common for addons to touch this.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> I think it can choose what it is, but this only is a problem when Xrays are
> involved, and maybe those are only from privileged code?

Unprivileged scopes get Xrays to non-same-origin content, and can also create xrays via Components.lookupMethod.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected

Comment 6

[Alex Keybl [:akeybl]]

(In reply to Andrew McCreight [:mccr8] from comment #1)
> I don't know how exploitable this actually is.  It is pretty old code.

Why track this for release if the sec-critical rating is only theoretical right now? We should consider completing an investigation and finding a testcase prior to tracking.

Comment 7

[Andrew McCreight [:mccr8]]

It isn't clear that there's actually a problem here, and bholley is rewriting this, so I'm just going to downgrade it.

Comment 8

[Andrew McCreight [:mccr8]]

Bobby, do you think this is okay to close now?  I don't know if this is part of the code that you rewrote or not.

Comment 9

[Bobby Holley (:bholley)]

Yeah, it all goes through IDL now.