328851 - [FIX]print preview killing browser's chrome

Status: RESOLVED FIXED

(Core :: Security, defect, P1)

Description

[georgi - hopefully not receiving bugspam]

abuse of print preview and xbl kills browser's chrome and replace it with the print preview one.

don't know if it can be used more.

demo in the url.

Comment 1

[georgi - hopefully not receiving bugspam]

related to Bug 328469 and Bug 325991.

in the description s/used/abused/

Comment 2

[Boris Zbarsky [:bzbarsky]]

So there are two things going on here:

1)  Closing print preview reenables script, then recreates the galley presentation.  This runs the XBL constructor, etc.  So the JS executes before we've exited print preview.  If we do need to reenable script before we restore the galley presentation (seems likely, if XBL is involved), we should probably make sure to finish all UI operations first... or something.  Not sure.

2)  The access to Components.classes really breaks things.  I added some debug code to XPFE to see what's going on:

    var webBrowserPrint = this.getWebBrowserPrint();
    alert('got here');
    try {
      webBrowserPrint.exitPrintPreview(); 
    } catch (e) { alert(e); }
    alert('got here too');

I see one alert (the "got here") alert.  The other alerts do not happen.  No code in this function after this point executes.  So it feels to me like the JS engine thinks an exception got thrown.. but it's not catchable, etc.

I'm trying to come up with a better testcase for that right now.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Not quite a testcase, but:

data:text/html,<iframe src='javascript:window.location="javascript:Components.classes"; alert("aaa");'></iframe>

In this case I never see an error in the JS console.  We do get a false return in nsJSContext::EvaluateString, but when we look for a native call context to notify about the pending exception we don't find one.

Note that if I do s/window/parent/ above then I DO get the error reported, via js_ReportUncaughtException called from JS_EvaluateUCScriptForPrincipals.  I don't know why that's not happening correctly in the data: URI above, but I bet that if we figured it out we could fix this bug.  ;)

Comment 4

[Boris Zbarsky [:bzbarsky]]

So there are several things going on here.  First of all, we lose error reports from javascript: URIs.  It's not obvious how best to fix that; I've filed bug 328902 on it.

After that, the problem is that when we get an error in EvaluateString we set the "exception thrown" boolean on the current XPConnect context... but the current XPConnect context is the one for the chrome call into C++, so chrome gets broken.

The safe fix for this is to make sure that NotifyXPCIfExceptionPending() only messes with XPCCallContexts that correspond to the JSContext it gets passed, imo.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed safe fix

This fixes the testcase for me.  JS does still run when we exit print preview; Georgi, would you mind filing a separate bug for us to sort out whether that should happen?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Not sure what other branches we should also take this on...

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 213513 [details] [diff] [review]
Proposed safe fix

r=jst

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 213513 [details] [diff] [review]
Proposed safe fix

This looks good, but it implies that we don't need to set the exception pending somewhere else.  That seems to be the right thing.  Just looking for a way to deduce that other than testing ;-).

/be

Comment 9

[Boris Zbarsky [:bzbarsky]]

See bug 328902 -- I think the call to NotifyXPCIfExceptionPending() is just wrong in this case.  I could add a warning if cx != aCx if you'd like...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.  Still not sure how much we want this for branches...

Comment 11

[Brendan Eich [:brendan]]

No, no need for a warning if we understand what's going wrong and are tracking it already via bug 328902.

/be

Comment 12

[georgi - hopefully not receiving bugspam]

Attachment: xbl-v1

cloning the attachmets here to make the bug self contained

Comment 13

[georgi - hopefully not receiving bugspam]

Attachment: html-v1

cloning the attachments here to make the bug self contained

Comment 14

[georgi - hopefully not receiving bugspam]

bz fixed the bug (or at least kludged it) on today's suite trunk.
chrome no longer killed.

got assertion:

###!!! ASSERTION: no document: 'doc', file nsXULElement.cpp, line 2205

will file a bug about the js execution later, investigating.

Comment 15

[georgi - hopefully not receiving bugspam]

(In reply to comment #5)
> JS does still run when we exit print preview;

a reason for this may be because of redraw - print preview seems to occupy the browser window. but if this is the case it is strange why the system exploit worked. on todays trunk js doesn work after print preview when js is disabled (it used  to before)

> Georgi, would you mind filing a separate bug for us to sort out whether that
> should happen?
> 

filed Bug 328961 with a testcase when js shouldn't work when print previewing a message on latest suite trunk

Comment 16

[Boris Zbarsky [:bzbarsky]]

> ###!!! ASSERTION: no document: 'doc', file nsXULElement.cpp, line 2205

Known thing; always happens when leaving preview.  :(

Comment 17

[Boris Zbarsky [:bzbarsky]]

Is this something we should fix on the 1.8.0 and 1.7.* branches?

Fixed on the 1.8 branch.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 213513 [details] [diff] [review]
Proposed safe fix

approved for 1.8.0 branch, a=dveditz for drivers

Comment 19

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.4) Gecko/20060505 Firefox/1.5.0.4

Comment 20

[Daniel Veditz [:dveditz]]

This is more of a Denial of Service problem now (as opposed to the more critical issue in bug 328469 where this testcase comes from), right? Please correct me if sg:low is not appropriate.

Comment 21

[georgi - hopefully not receiving bugspam]

(In reply to comment #20)
> This is more of a Denial of Service problem now (as opposed to the more
> critical issue in bug 328469 where this testcase comes from), right? Please
> correct me if sg:low is not appropriate.
> 

i don't mind sg:low. bug 328469 where this testcase comes from is dangerous.