Closed Bug 787778 Opened 12 years ago Closed 12 years ago

use-after-free in nsDocumentOpenInfo::DispatchContent

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(firefox16 unaffected, firefox17+ fixed, firefox18+ verified, firefox19 verified, firefox-esr10 unaffected)

Status:
VERIFIED FIXED
Milestone:
mozilla19
Tracking Flags:
Tracking Status
firefox16 --- unaffected
firefox17 + fixed
firefox18 + verified
firefox19 --- verified
firefox-esr10 --- unaffected

People

(Reporter: miaubiz, Assigned: johns)

References

Details

(7 keywords, Whiteboard: [asan][adv-track-main17+][qa-])

Attachments

(7 files, 2 obsolete files)

repro
216 bytes, text/plain
Details
linux mozilla central asan log
7.26 KB, text/plain
Details
linux aurora asan log
7.25 KB, text/plain
Details
osx mozilla-central asan log
6.12 KB, text/plain
Details
stack, re-entrant nsObjectLoadingContent::LoadObject
17.75 KB, text/html
Details
Part 1, be careful with re-entrance around mFinalListener
9.87 KB, patch
jaas
: review+
akeybl
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta+
abillings
: sec-approval+
johns
: checkin+
Details | Diff | Splinter Review
Test. r=josh
2.15 KB, patch
johns
: checkin+
Details | Diff | Splinter Review
when I open this <html> <head> <script> window.onload = function() { window.document.x.src='a' } </script> </head> <body> <embed name="x" src="http://mozilla.org"> </embed> </body> </html> I get this: ================================================================= ==24242== ERROR: AddressSanitizer heap-use-after-free on address 0x7fffc5b03d98 at pc 0x7ffff0da8d98 bp 0x7fffffff9730 sp 0x7fffffff9728 READ of size 8 at 0x7fffc5b03d98 thread T0 #0 0x7ffff0da8d98 in nsCOMPtr_base::assign_assuming_AddRef(nsISupports*) /builds/slave/try-lnx64/build/xpcom/build/../glue/nsCOMPtr.h:436 #1 0x7ffff005516e in nsDocumentOpenInfo::DispatchContent(nsIRequest*, nsISupports*) /builds/slave/try-lnx64/build/uriloader/base/nsURILoader.cpp:375 #2 0x7ffff005487b in nsDocumentOpenInfo::OnStartRequest(nsIRequest*, nsISupports*) /builds/slave/try-lnx64/build/uriloader/base/nsURILoader.cpp:263 0x7fffc5b03d98 is located 24 bytes inside of 72-byte region [0x7fffc5b03d80,0x7fffc5b03dc8) freed by thread T0 here: #0 0x42ae21 in free ??:0 #1 0x7ffff0053f88 in nsAutoRefCnt::operator=(unsigned int) /builds/slave/try-lnx64/build/../../dist/include/mozilla/mozalloc.h:224 #2 0x7ffff0eaf825 in NS_InvokeByIndex_P /builds/slave/try-lnx64/build/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:165 works on linux + m-c, linux + aurora and osx + m-c atleast.
Attached file repro —
Attached file linux aurora asan log —
sorry for the double post. I got some csrf warning page a bunch of times so I didn't realize what happened.
There's a re-entrant call to nsObjectLoadingContent::LoadObject on the same object. It calls CloseChannel which nulls out mFinalListener which destroys it since it's the last reference.
Severity: normal → critical
Component: General → DOM
Keywords: crash, reproducible, sec-critical, testcase
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [asan]
Assignee: nobody → jschoenick
Status: NEW → ASSIGNED
So we're re-entering in mFinalListener->OnStartRequest() and throwing out mFinalListener in the nested call. Adding a nsCOMPtr on the stack to hold mFinalListener alive solves this, though by looking at it, we still might fire a unnecessary notification, so we should also include this call in the mIsLoading re-entry checks This is also the cause behind bug 788107
status-firefox-esr10: --- → unaffected
status-firefox16: --- → unaffected
status-firefox17: --- → affected
status-firefox18: --- → affected
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?
Component: DOM → Plug-ins
Er, wait. Why are we ending up in onload? Which docshell are we calling LoadURI on here, exactly? If it's the child, then shouldn't the load we're in OnStartRequest for still keep onload on the parent from firing? And if it's the parent, why exactly are we doing that for an _embed_?
(In reply to Boris Zbarsky (:bz) from comment #9) > Er, wait. Why are we ending up in onload? Which docshell are we calling > LoadURI on here, exactly? We're calling LoadURI on the embed for its sub-document, firing onload, which calls into LoadURI for a different document. > If it's the child, then shouldn't the load we're > in OnStartRequest for still keep onload on the parent from firing? As in, via a script blocker? We could do that, but it wouldn't fix the recursion in the plugin (as opposed to subdocument) instance, wherein we can't stop plugins from spinning the event loop all the time just for fun.
So we're re-enter inside mFinalListener->OnBlah(), and cause mFinalListener to be destroyed inside a call. This adds much more careful re-entrance checks around the calls that may re-enter, and as a bonus makes sure notifications don't fire out of order if we do-reenter.
Attachment #668208 - Flags: review?(joshmoz)
This explicitly checks for InstantiatePluginInstance() re-entering into LoadObject. That condition successfully cleans up the outer plugin, but Instantiate will go on to do inappropriate things to the object wrapper on unwind.
Attachment #668210 - Flags: review?(joshmoz)
This patch fixes the repro case here, and also fixes bug 790244, and probably bug 788107
Attached patch Test (obsolete) — — Splinter Review
So upon trying to make this a test case, I discovered this only happens when |X-Frame-Options: DENY| is set in the initial document, our handler for which seems to be what allows script to run inside the document load. This would explain why this can happen in documents re: comment 9. I'm not sure if that qualifies as its own bug (I wasn't able to cause any issues with iframes)
Attachment #668238 - Flags: review?(joshmoz)
Attachment #668238 - Flags: review?(joshmoz) → review+
Comment on attachment 668208 [details] [diff] [review] Part 1, be careful with re-entrance around mFinalListener Review of attachment 668208 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsObjectLoadingContent.cpp @@ -1740,5 @@ > - mSrcStreamLoading = true; > - rv = mFinalListener->OnStartRequest(mChannel, nullptr); > - mSrcStreamLoading = false; > - if (NS_SUCCEEDED(rv)) { > - NotifyContentObjectWrapper(); You're removing this but don't seem to be adding it anywhere else, is that OK?
Attachment #668208 - Flags: review?(joshmoz) → review+
Attachment #668210 - Flags: review?(joshmoz) → review+
(In reply to Josh Aas (Mozilla Corporation) from comment #15) > Comment on attachment 668208 [details] [diff] [review] > Part 1, be careful with re-entrance around mFinalListener > > Review of attachment 668208 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/base/src/nsObjectLoadingContent.cpp > @@ -1740,5 @@ > > - mSrcStreamLoading = true; > > - rv = mFinalListener->OnStartRequest(mChannel, nullptr); > > - mSrcStreamLoading = false; > > - if (NS_SUCCEEDED(rv)) { > > - NotifyContentObjectWrapper(); > > You're removing this but don't seem to be adding it anywhere else, is that > OK? InstantiatePluginInstance() already calls NotifyContentObjectWrapper, and nsPluginStreamListener->OnStartRequest ends up calling InstantiatePluginInstance, so I'm 95% sure this call is redundant. (maybe InstantiatePluginInstance was cut out of the loop in the past?)
Comment on attachment 668210 [details] [diff] [review] Part 2 - Take early returns if we re-enter from InstantiatePluginInstance, add more logging This is really a mostly-separate fix for bug 790244, so I'm moving it there
Attachment #668210 - Attachment is obsolete: true
Blocks: 790244
Comment on attachment 668208 [details] [diff] [review] Part 1, be careful with re-entrance around mFinalListener [Security approval request comment] How easily can the security issue be deduced from the patch? The patch immediately reveals that there is a re-entrance issue, however, the cases that can trigger it are more obscure. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The comments also explicitly point out re-entrance issues (the test gives the exact issue away, amd is deliberately split into another patch.) Which older supported branches are affected by this flaw? 17+ If not all supported branches, which bug introduced the flaw? Bug 745030 or one of its followups Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Applies cleanly to all branches How likely is this patch to cause regressions; how much testing does it need? This adds more explicit re-entrance guards - regression odds are fairly low, and would likely be in plugin behavior, and would manifest quickly. This should fix the ~#20 top crash on trunk, so verifying its effect there should be fairly immediate.
Attachment #668208 - Flags: sec-approval?
Comment on attachment 668208 [details] [diff] [review] Part 1, be careful with re-entrance around mFinalListener [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 745030 or one of its followups User impact if declined: sec-critical issue Testing completed (on m-c, etc.): Passes attached test, not on m-c awaiting sec-approval Risk to taking this patch (and alternatives if risky): Moderate-to-low, adds more explicit re-entrance guards, regressions would be in plugin-behavior and should manifest quickly on trunk. String or UUID changes made by this patch: None
Attachment #668208 - Flags: approval-mozilla-beta?
Attachment #668208 - Flags: approval-mozilla-aurora?
status-firefox19: --- → affected
Comment on attachment 668208 [details] [diff] [review] Part 1, be careful with re-entrance around mFinalListener Since this is 17 and up, I'm approving this, security-wise, for now. We should get Release Management to approve it for 17 if possible.
Attachment #668208 - Flags: sec-approval? → sec-approval+
Comment on attachment 668208 [details] [diff] [review] Part 1, be careful with re-entrance around mFinalListener [Triage Comment] sg:crit fix - let's land on all branches asap to get testing on Nightly/Aurora and make it into beta 2.
Attachment #668208 - Flags: approval-mozilla-beta?
Attachment #668208 - Flags: approval-mozilla-beta+
Attachment #668208 - Flags: approval-mozilla-aurora?
Attachment #668208 - Flags: approval-mozilla-aurora+
Attachment #668208 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/576c66cb9f2f
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
status-firefox19: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Keywords: verifyme
Waiting on bug 802355 to see if this needs to be backed out of mozilla-beta since it causes a high volume of crashes.
Comment on attachment 668238 [details] [diff] [review] Test [Security approval request comment] This patch is just the test, this has already been fixed on all branches How easily can the security issue be deduced from the patch? Easily, the test triggers it Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Yep Which older supported branches are affected by this flaw? Fixed in all current releases
Attachment #668238 - Flags: sec-approval?
Depends on: 802355
Please wait until after Firefox 17 ships with a fix for this before landing the tests.
Whiteboard: [asan] → [asan] land test after Fx17 ships
Blocks: 788107
Whiteboard: [asan] land test after Fx17 ships → [asan][adv-track-main17+] land test after Fx17 ships
Since this regression never affected shipping builds it's OK to land the tests any time now: nightly users have had this fix for a while.
Blocks: 745030
Keywords: regression
Whiteboard: [asan][adv-track-main17+] land test after Fx17 ships → [asan][adv-track-main17+]
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Comment on attachment 668238 [details] [diff] [review] Test sec-approval+ on tests. Land the tests.
Attachment #668238 - Flags: sec-approval? → sec-approval+
Flags: in-testsuite? → in-testsuite+
Comment on attachment 668238 [details] [diff] [review] Test Or not: https://hg.mozilla.org/integration/mozilla-inbound/rev/34e2424d1044 Passes locally, fails on test boxes :(
Attachment #668238 - Flags: checkin+ → checkin-
Attached patch Test. r=josh — — Splinter Review
Makefile fail, the helper should be named file_bug787778.sjs, not test_bug787778.sjs
Attachment #668238 - Attachment is obsolete: true
Flags: in-testsuite+ → in-testsuite?
Marking this verified for Firefox 18 and 19 based on in-testsuite+ and landings of test on those branches. I think this still needs verification for Firefox 17 because the tests are not landed there.
status-firefox18: fixed → verified
status-firefox19: fixed → verified
On second thought, this appears to require an ASan build and we don't have builds for Beta. For this reason I don't think QA will be able to verify for Firefox 17. Flagging [qa-].
Status: RESOLVED → VERIFIED
Keywords: verifyme
Whiteboard: [asan][adv-track-main17+] → [asan][adv-track-main17+][qa-]
Group: core-security
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: