[FIX]js exploit via "print preview"

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
Printing: Output
P1
normal
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: georgi - hopefully not receiving bugspam, Assigned: bz)

Tracking

({fixed1.8.1, verified1.8.0.2})

Trunk
mozilla1.9alpha1
fixed1.8.1, verified1.8.0.2
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9a1 +
blocking1.8.1 +
blocking1.8.0.2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical][rft-dl])

Attachments

(7 attachments, 5 obsolete attachments)

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.
Created attachment 210767 [details]
xbl
Created attachment 210769 [details]
trying exploit
Created attachment 210771 [details]
exploit ver2
Attachment #210769 - Attachment is obsolete: true
Created attachment 210773 [details]
exploit ver3
Attachment #210771 - Attachment is obsolete: true
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.
> 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.
Assignee: nobody → printing
Blocks: 267352
Component: General → Printing
OS: Linux → All
Product: Firefox → Core
QA Contact: general
Hardware: PC → All
Version: 1.5 Branch → Trunk
Created attachment 210825 [details] [diff] [review]
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.
Created attachment 210826 [details] [diff] [review]
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?
Created attachment 210831 [details] [diff] [review]
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.
Assignee: printing → bzbarsky
Attachment #210825 - Attachment is obsolete: true
Attachment #210826 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #210831 - Flags: superreview?(shaver)
Attachment #210831 - Flags: review?(jst)
Flags: blocking1.9a1+
Flags: blocking1.8.1?
Flags: blocking1.8.0.2?
Priority: -- → P1
Summary: js exploit via "print preview" → [FIX]js exploit via "print preview"
Target Milestone: --- → mozilla1.9alpha
(In reply to comment #8)

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

this seems reasonable
(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.



Created attachment 210846 [details]
release2 xbl
Created attachment 210847 [details]
release2 exploit v0.1alpha
Created attachment 210848 [details]
release2 exploit v0.2alpha
Attachment #210847 - Attachment is obsolete: true
"release2 exploit v0.2alpha" works for me from bugzilla on linux
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.

Created attachment 210853 [details]
release3 xbl
Created attachment 210854 [details]
release3 exploit v0.01a
release3 exploit v0.01a works also with printing, not just print preview.
> 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 on attachment 210831 [details] [diff] [review]
Fixes everything...

r=jst
Attachment #210831 - Flags: review?(jst) → review+
(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 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.
Attachment #210831 - Flags: superreview?(shaver) → superreview+
Comment on attachment 210831 [details] [diff] [review]
Fixes everything...

Requesting branch approvals...  This is a pretty safe fix, in my opinion.
Attachment #210831 - Flags: branch-1.8.1?(jst)
Attachment #210831 - Flags: approval1.8.0.2?
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...
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
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?
> 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.
(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.

(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]
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.2+
Whiteboard: [sg:critical]
Comment on attachment 210831 [details] [diff] [review]
Fixes everything...

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #210831 - Flags: approval1.8.0.2?
Attachment #210831 - Flags: approval1.8.0.2+
Attachment #210831 - Flags: approval-branch-1.8.1?(jst)
Attachment #210831 - Flags: approval-branch-1.8.1+
Fixed on both branches
Keywords: fixed1.8.0.2, fixed1.8.1
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.
> 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...

Updated

12 years ago
Whiteboard: [sg:critical] → [sg:critical][rft-dl]
(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.


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...
Bug 328469 also exploits print preview with patched this bug

Comment 37

12 years ago
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.
Keywords: fixed1.8.0.2 → verified1.8.0.2
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.
Depends on: 334944
This caused bug 334944
Group: security
You need to log in before you can comment on or make changes to this bug.