325991 - [FIX]js exploit via "print preview"

Status: RESOLVED FIXED

(Core :: Printing: Output, defect, P1)

Description

[georgi - hopefully not receiving bugspam]

if a user is tricked to select "print preview" twice (the first time a
blank page is shown) it is possible to at least change his preferences.

the problem is execution of js coming from xbl in print preview and
probably a race condition helped by alert()s.

some trunk versions crash, some get exploited (had a hard time avoiding the
crashes).

testcase to follow.

Comment 1

[georgi - hopefully not receiving bugspam]

Attachment: xbl

Comment 2

[georgi - hopefully not receiving bugspam]

Attachment: trying exploit

Comment 3

[georgi - hopefully not receiving bugspam]

Attachment: exploit ver2

Comment 4

[georgi - hopefully not receiving bugspam]

Attachment: exploit ver3

Comment 5

[Daniel Veditz [:dveditz]]

I don't see the exploit on windows, instead I consistently crash at
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xbl/src/nsXBLBinding.cpp&rev=1.208&mark=1206#1190

It looks like mBoundElement is a deleted object which brings up exploitability issues quite apart from running XBL scripts with the wrong permissions. I don't know if my non-debug crash was at the same spot, waiting for the talkback server (TB14798750X)

The aviary101/moz17 branch is not vulnerable to the testcase, don't know if that's because it's safe (my crash was in XBL code that changed in 1.8) or because PrintPreview changed such that the exploit needs simple tweaking.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> I don't see the exploit on windows, instead I consistently crash at

Hmm... That looks an awful lot like bug 317265.  What version of nsBindingManager does the tree that you see the crash in have?

Certainly with a current trunk build I'm seeing this problem (that is, the exploit, not the crash).  :(  And when it happens, the print preview mode gets all confused (as in, I get the print preview toolbar _and_ the various other toolbars) and I'm unable to quit print preview.

We _may_ be able to "fix" this by just stopping all loads from documents that are currently in print preview.... But I'd really like to sort out what's actually going on here.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: sorta-"fix"

So this is mostly a regression from bug 267352.

The sequence of events is approximately as follows:

1)  We start print preview (so we're running chrome JS code and it's on the
    stack).
2)  The print preview code sets up a timeout to do the actual print previewing
    later and kicks off the print preview progress dialog and the print
    engine.
3)  The print preview presentation is created.  The binding constructor fires
    and puts up an alert.  It takes time for a human to react to the alert.
    During this time the timer set in step 2 fires and posts an event into the
    main thread event queue.
4)  The user clicks "ok" on the alert and control returns to print engine code
    (which has just finished creating the presentation; at this point it's NOT
    yet "ready to go).
5)  We call FlushEventQueue() (this was added in bug 267352).
6)  We process PLEvents, with the chrome JS from step 1 still on the stack.
7)  We process settings of "src" attrs on some <iframe>s in the parser.  Since
    there is JS on the stack, and it's chrome JS, one iframe manages to load
    about:config while the other loads a data: URI with a chrome principal.
    This is the actual exploit.  I suspect that you could stick arbitrary JS in
    there (so no need to limit to setting prefs via about:config).
8)  We process the timer event from step 3.  This breaks the UI, since the code
    in step 1 expected to return before the timer fired, but thanks to the
    alerts and the event loop spinning the timer is firing while we're still in
    that code.
9)  When shutting down we crash because we never tore the printing stuff down
    properly.

This patch makes sure we push a null JSContext between step 5 and step 6, so that in step 7 CheckLoadURI prevents the about:config load and the data: URI gets the parent page principal.  But that still leaves the problems in step 8 and step 9.

So is there a reason anyone can see to not back out bug 267352?  It's fixing a regression from bug 228557, but bug 228557 was only checked in on aviary and 1.7 branches -- it never made its way to trunk.  See bug 267352 comment 34, which I wish I had followed up on.  :(  I'll work on testing that backout and post the patch if it does what I think it will for us here.

Oh, in case anyone wonders why we need two print previews to make this work, that's because XBL loads are async the first time and are sync (from binding cache) the second time....  I bet with a little work (and some conditionals in the constructor) one could make the async load happen in the screen presentation, so that opening print preview just once would be exploitable.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Fixes step 7 and almost step 8

With this patch there is no security bug, but the UI gets stuck in print preview mode because mPrintEngine is null in the document viewer...  I'm really not sure how to fix this, offhand.  :(

Perhaps we should not run untrusted XBL constructors while print previewing?  Or something else?

Or can we fix this on the level of the print preview UI?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Fixes everything...

The first hunk of this patch is what I _meant_ to post as "Fixes step 7 and almost step 8".  The second hunk is what's needed to keep the untrusted XBL constructor from running.  Note that this change means no untrusted XBL execution of any sort in documents in which printing is disabled on the script context (which basically means "in print preview", iirc).  We already have just such a check in the various nsJSEnvironment methods, and the only other caller of GetScriptsEnabled is the code in the HTML parser that figures out how to parse <noscript>.  Since we should never be running the parser while print previewing, that should be fine.  In other words, the only code whose behavior changes here due to the caps patch is nsXBLBinding::AllowScripts().  And chrome bindings are unaffected, of course.

I _think_ this should be good enough to also fix various variants like having data:-URI <iframe>s with <script> tags in them inserted via the binding, etc.  I do think removing the event processing loop is necessary to not get bitten by those.

Comment 10

[georgi - hopefully not receiving bugspam]

(In reply to comment #8)

> 
> Perhaps we should not run untrusted XBL constructors while print previewing? 

this seems reasonable

Comment 11

[georgi - hopefully not receiving bugspam]

(In reply to comment #7)


> I bet with a little work (and some conditionals in
> the constructor) one could make the async load happen in the screen
> presentation, so that opening print preview just once would be exploitable.
> 

if you have betted you have won - testcase to follow that works with a single print preview.

the about:config iframe isn't needed, so a cleaner exploit.

release 2 exploit works for me on linux branch/trunk and win32/wine.

Comment 12

[georgi - hopefully not receiving bugspam]

Attachment: release2 xbl

Comment 13

[georgi - hopefully not receiving bugspam]

Attachment: release2 exploit v0.1alpha

Comment 14

[georgi - hopefully not receiving bugspam]

Attachment: release2 exploit v0.2alpha

Comment 15

[georgi - hopefully not receiving bugspam]

"release2 exploit v0.2alpha" works for me from bugzilla on linux

Comment 16

[georgi - hopefully not receiving bugspam]

looks like the crashes are caused by the alert (probably freed/overwritten objects?)

bz: can the same side effect of alert crashing be triggered without the help of chrome?

exploit that didn't crash on any box to follow.

Comment 17

[georgi - hopefully not receiving bugspam]

Attachment: release3 xbl

Comment 18

[georgi - hopefully not receiving bugspam]

Attachment: release3 exploit v0.01a

Comment 19

[georgi - hopefully not receiving bugspam]

release3 exploit v0.01a works also with printing, not just print preview.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> looks like the crashes are caused by the alert (probably freed/overwritten
> objects?)

The alert was basically causing code to run when it wasn't expecting to be run (most similar to reentering non-reentrant code).  So yes, I suspect the crashes were due to that.

> bz: can the same side effect of alert crashing be triggered without the help
> of chrome?

I think that in general one can produce weird reentry of layout code from XBL constructors, even without alerts....  :(  I believe we have existing bugs on the fact that said constructors run at "unsafe" times.  I'm not sure whether that can lead to exploits, but there are some known null-deref crashes.

Georgi, can you test the patch I posted, or does it need to make its way into a build first?

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 210831 [details] [diff] [review]
Fixes everything...

r=jst

Comment 22

[georgi - hopefully not receiving bugspam]

(In reply to comment #20)

> Georgi, can you test the patch I posted, or does it need to make its way into a
> build first?

sure, made a build with the patch.
all exploits stopped working, including js execution in "print preview".

some things:
on the first exploit the first print preview shows blank page (as before the patch) and a terminal warning is shown:

WARNING: Adding a presshell that was disconnected from the document as a document observer?  Sounds wrong..., file /opt/firefox-cvs/mozilla/layout/base/nsPresShell.cpp, line 2556



 

Comment 23

[Mike Shaver (:shaver emeritus)]

Comment on attachment 210831 [details] [diff] [review]
Fixes everything...

sr=shaver, if we don't have a bug on file about the docshell strangeness (or it isn't otherwise understood), please file one.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 210831 [details] [diff] [review]
Fixes everything...

Requesting branch approvals...  This is a pretty safe fix, in my opinion.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.  The warning is because printing removes the presshell from the document (shared by print preview and printing code) then readds it (print preview only).  It's not really something presshells support well, and a known issue with print preview...

Comment 26

[georgi - hopefully not receiving bugspam]

bz, you seem to answer technical questions, so can you please comment on this:

layout/forms/nsTextControlFrame.cpp:2981
 nsCOMPtr<nsIJSContextStack> stack =
      do_GetService("@mozilla.org/js/xpc/ContextStack;1");
[1]    PRBool pushed = stack && NS_SUCCEEDED(stack->Push(nsnull));
      
    rv = mEditor->OutputToString(NS_LITERAL_STRING("text/plain"), flags,
                                 aValue);

    if (pushed) {
      JSContext* cx;
[2]      stack->Pop(&cx);
      NS_ASSERTION(!cx, "Unexpected JSContext popped!");
    }


there is time (race condition) between [1] and [2].
managed to get it to few seconds with tens of megs.

would like to interrupt the process between [1] and [2] and start js.
eventually java and signals come to mind, but no luck so far.
(though java seems to get executed in the race period).

any ideas how to inject js in the race?

Comment 27

[Boris Zbarsky [:bzbarsky]]

> would like to interrupt the process between [1] and [2] and start js.

What would that accomplish?  It'd push a JS context on the stack, then pop it when the JS stops executing and before this code runs, no?

Note that there is a separate stack per thread, if that matters.

Comment 28

[georgi - hopefully not receiving bugspam]

(In reply to comment #27)
> > would like to interrupt the process between [1] and [2] and start js.
> 
> What would that accomplish?  It'd push a JS context on the stack, then pop it
> when the JS stops executing and before this code runs, no?
> 
> Note that there is a separate stack per thread, if that matters.
> 

thanks. i was thinking about abusing the null context.

Comment 29

[Daniel Veditz [:dveditz]]

(In reply to comment #5)
> don't know if my non-debug crash was at the same spot, waiting for the
> talkback server (TB14798750X)

Just to close the loop, it was not and the TB looks more like this bug. The top bit is:
0x0000dbe9
PresShell::InitialReflow  [layout/base/nsPresShell.cpp, line 2857]
nsPrintEngine::ReflowPrintObject  [layout/printing/nsPrintEngine.cpp, line 2769]
nsPrintEngine::ReflowDocList  [layout/printing/nsPrintEngine.cpp, line 2498]
nsPrintEngine::SetupToPrintContent  [layout/printing/nsPrintEngine.cpp, line 2308]
nsPrintEngine::DocumentReadyForPrinting  [layout/printing/nsPrintEngine.cpp, line 2134]
DocumentViewerImpl::PrintPreview  [layout/base/nsDocumentViewer.cpp, line 3418]

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 210831 [details] [diff] [review]
Fixes everything...

approved for 1.8.0 branch, a=dveditz for drivers

Comment 31

[Boris Zbarsky [:bzbarsky]]

Fixed on both branches

Comment 32

[georgi - hopefully not receiving bugspam]

bz,

1. How do i enumerate all javascript objects accessible from luserland js - bruteforcing .idl names produced a lot of objects, but there may others.

2. Can you think of missing checks like calling foo() without calling bar() that may have security implications (don't mean strcpy() and friends but higher logical bugs)

10x.

Comment 33

[Boris Zbarsky [:bzbarsky]]

> How do i enumerate all javascript objects accessible from luserland js

Not really sure...  For example, there are various methods (eg getBoxObjectFor()) that returns objects to userland JS...

> Can you think of missing checks like calling foo() without calling bar()

You mean in terms of C++ code patterns?  I might be able to think some up if needed...

Comment 34

[georgi - hopefully not receiving bugspam]

(In reply to comment #33)
> > How do i enumerate all javascript objects accessible from luserland js
> 
> Not really sure...  For example, there are various methods (eg
> getBoxObjectFor()) that returns objects to userland JS...
> 

discarding methods that returns objects, i am trying to find the code that decides whether an object name exists at all and patch it.

for example javascript:alert(HTMLDocument.toString())

> You mean in terms of C++ code patterns?  I might be able to think some up if
> needed...
> 

yes, C++. the idea is to try to make searches for missing checks.
in some cases grep may not be enough.

10x bz.

Comment 35

[Boris Zbarsky [:bzbarsky]]

I'm not sure what that example means.

Some things that come to mind offhand are calling delete manually on refcounted objects and not doing a BeginUpdate/EndUpdate around document observer notifications....  I don't think we do either one much, but...

Comment 36

[georgi - hopefully not receiving bugspam]

Bug 328469 also exploits print preview with patched this bug

Comment 37

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060224 Firefox/1.5.0.1, no exploit or crash with exploit ver3 or release3 exploit v0.01a testcases.

Comment 38

[georgi - hopefully not receiving bugspam]

some random thoughts in case this "issue" gets linked from CVE(tm).

needless to say, in my humble opinion, CVE/mitre are not very good in
sikiurity.
in addiotion, a mitre employee coauthored an RFC about "responsible disclosure"
and was dumb enough to propose it to the ietf. the ietf replied something close
to a word starting with F, containing U and ending in See-Key.

Comment 39

[Boris Zbarsky [:bzbarsky]]

This caused bug 334944