296514 - event handler and modal dialog allows XSS attacks

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[shutdown]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1)
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050602 Firefox/1.0+ (2005060218)

Mozilla stops any scripts in the current page including event handlers
when it loads a new page. But in some cases, mozilla fails to stop event
handlers in the previous page. This is not a problem because such event
handler finish their exection before the new page is loaded. However,
if such event handler shows a modal dialog, the handler stops its
execution and mozilla continues to load the new page. If user closes
the modal dialog after mozilla loaded the new page, the handler
continues its execution in the context of the new page.

Reproducible: Always

Steps to Reproduce:
1. load the testcase.
2. push "invoke an exploit" button.

Actual Results:  
The event handler is executed in the context of the new page.

Expected Results:  
The event handler must be stopped before loading the new page.

Comment 1

[shutdown]

Attachment: testcase

the testcase which tries to read cookies for www.google.com.

Comment 2

[Bob Clary [:bc] (inactive)]

confirm it gets my google cookie.

Comment 3

[Boris Zbarsky [:bzbarsky]]

We really need separate inner windows.... :(

Perhaps taking down a modal dialog in cases like this should throw an exception?
 That seems like the simplest way to abort script execution.

Comment 4

[Brendan Eich [:brendan]]

We could validate cookie access using the script's principals, or some such.

An exception in this case seems like the easiest fix in the short run, and it
should not break anything.

bz: can you remind me whether the inner-window-object bug is on file? If not,
file away and cc: me.

/be

Comment 5

[Boris Zbarsky [:bzbarsky]]

> We could validate cookie access using the script's principals, or some such.

Yes, but that feels like a stopgap... what other things would we need to do such
validation for?

In any case, that should be already happening.  The "cookie" property of
HTMLDocument is under the SameOrigin policy, so we're clearly ending up with the
wrong subject principal anyway... Or is that the needsSecurityCheck thing biting us?

I don't see an existing bug on the window split; I filed bug 296639 on that.

Comment 6

[Johnny Stenback (:jst)]

I bet we're beeing bit by needsSecurityCheck again here, yeah. We need to either
fix that, or make modal dialogs suspend all network activity for the current
window while they're being shown. I tested that with just alert(), and that does
fix this bug, but there are other ways to bring up modal dialogs, so we need
more than what I hacked up so far (probably need to do this in our prompter
implementation). Thoughts?

Comment 7

[Johnny Stenback (:jst)]

I was wrong above when betting that this was due to
documentNeedsSecurityCheck(). It's not. The problem is far worse than that.

What happens here is that we're running script from the previous page after the
new page is done loading. When we do that, and we access document.cookies
XPConnect does do the appropriate security check, but that check fails to notice
that the running code is from a different origin. Caps ends up calling
GetSubjectPrincipal(), which returns the principal of the new page, not the page
where the code came from. It does that by iterating over the JS stack frames and
it finds a cloned function on the stack (second frame, the first one is native,
so we skip it). Since it finds a clone, it can't rely on the principals compiled
into that function, so it walks up the function object's parent chain looking
for the principal... and since we've already loaded a new page, it finds the
principals of the new page.

I don't think we can change how the above works, so it seems like we need to
make modal dialogs suspend/resume network requests, at least for now...

Comment 8

[Daniel Veditz [:dveditz]]

Michael Krax reports the same thing for xul in bug 298249. I assume it's a dupe,
but maybe just parallel implementations.

Comment 9

[Johnny Stenback (:jst)]

I've got what's sort of a fix for this, but it's not complete yet. It'll fix
this problem, but I can still break the security model here by using more than
one window since the fix involves suspending/resuming requests in the parent
window when modal dialogs are opened, but if the parent is different than the
caller, then we've still got problems. Any number of windows could be used to
trigger a similar attack, so a complete fix that involves suspending/resuming
network requests is starting to look pretty nasty...

Comment 10

[shutdown]

(In reply to comment #9)
> I've got what's sort of a fix for this, but it's not complete yet. It'll fix
> this problem, but I can still break the security model here by using more than
> one window since the fix involves suspending/resuming requests in the parent
> window when modal dialogs are opened, but if the parent is different than the
> caller, then we've still got problems. Any number of windows could be used to
> trigger a similar attack, so a complete fix that involves suspending/resuming
> network requests is starting to look pretty nasty...

bug 298315 has already been filed at 2005-06-21.
but it have not been confirmed yet.

Comment 11

[Chase Phillips]

Per 1.0.5 meeting, minusing for 1.0.5, let's get this for 1.0.6.

Comment 12

[Jay Patel [:jay]]

Updating the the 1.7.x flags.

Comment 13

[Mike Shaver (:shaver emeritus)]

Did split window fix this on the trunk?  If so, please mark this fixed.

Comment 14

[shutdown]

The testcase no longer works. Fixed by bug 296639.
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b4) Gecko/20050811 Firefox/1.0+

Comment 15

[Johnny Stenback (:jst)]

Indeed, and it's fixed on the 1.8 branch too...

Comment 16

[Daniel Veditz [:dveditz]]

Fixed on the aviary1.0/mozilla1.7 branches by the split-window alternative (bug 316589)

Comment 17

[Jay Patel [:jay]]

v.fixed on 1.0.1 Aviary branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060220 Firefox/1.0.8, didn't see anything bad happen with testcase, just got an error in the jsc.