1243555 - EventListenerManager::HandleEventInternal() has a raw pointer to a docshell which can be destructed (via event-handling) before we finish using it

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Daniel Holbert [:dholbert]]

I filed bug 1243536 for an innocuous code-cleanup opportunity, but smaug noticed there was actually a security bug that my patch fixed, so I'm re-filing as a security bug.

The code from a security perspective is:

> 1162           nsDocShell* docShell;
[...]
> 1166           if (mIsMainThreadELM &&
> 1167               listener->mListenerType != Listener::eNativeListener) {
> 1168             nsCOMPtr<nsIDocShell> docShellComPtr = GetDocShellForTarget();
> 1169             if (docShellComPtr) {
> 1170               docShell = static_cast<nsDocShell*>(docShellComPtr.get());
[...]
> 1180               }
> 1181             }
> 1182           }
> 1183 
> 1184           if (NS_FAILED(HandleEventSubType(listener, *aDOMEvent, aCurrentTarget))) {
> 1185             aEvent->mFlags.mExceptionHasBeenRisen = true;
> 1186           }
> 1187 
> 1188           if (needsEndEventMarker) {
> 1189             timelines->AddMarkerForDocShell(
> 1190               docShell, "DOMEvent", MarkerTracingType::END);
> 1191           }

The problem is that the event-handling code at line 1184 could run arbitrary JS (I think) and destroy our document & docshell, which means our final "AddMarkerForDocShell" call at line 1189 could be be passing in a pointer to deleted memory -- |docShell|.

The fix is easy -- just hold onto |docShell| in a refcounted pointer.

Comment 1

[Daniel Holbert [:dholbert]]

This is a regression from bug 1202657, specifically from this nsCOMPtr removal:
  http://hg.mozilla.org/mozilla-central/diff/f95c614295a0/dom/events/EventListenerManager.cpp#l1.12

So, it seems we've just shipped this regression in Firefox 44 (which is the first affected release).  We should fix this in our next dot release, if we ship any Firefox 44 dot releases.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Here's the patch (same as in bug 1243536 ).

I'm keeping the commit message a bit deceptive to not give away the problem too much:
{
Bug 1243555: Remove unnecessary nsDocShell static_cast in EventListenerManager::HandleEventInternal(). r=smaug
}

The more important thing that this patch does (from a security perspective) is that it adds back a nsCOMPtr at the outer scope, to keep the nsIDocshell alive.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8712910 [details] [diff] [review]
fix v1

(I need to profile this code anyway. Tiny bit worried those extra AddRefs/Releases showing up in the profiles. But not about this bug.)

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8712910 [details] [diff] [review]
fix v1

[Security approval request comment]
* How easily could an exploit be constructed based on the patch?
  Probably easily.

* Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
  None of those do, and the check-in comment does obfuscate the security problem a bit.  However, if an attacker looked carefully at the code, it's pretty clear that we're declaring a pointer to something in the document, and then handling an event, and then using that pointer.  So I'm pretty sure that motivated attackers could figure out how to exploit this from looking at the fix.

Which older supported branches are affected by this flaw?
  Firefox 44 & newer.

If not all supported branches, which bug introduced the flaw?
  Bug 1202657

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
  The attached patch applies cleanly on branches -- no custom-backport-patch-writing needed.
  
How likely is this patch to cause regressions; how much testing does it need?
  Unlikely to cause regressions. Very little testing needed. (If it passes tests [and I'd be surprised if it doesn't], we should be fine.)

Comment 5

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8712910 [details] [diff] [review]
fix v1

sec-approval+ for checkin on trunk on February 9.

Once it is on trunk, we'll want it on Aurora and Beta as well so we should get nominations for those.

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8712910 [details] [diff] [review]
fix v1

Beta/Aurora Approval Request Comment
[Feature/regressing bug #]:  Bug 1243555

[User impact if declined]:  Security vulnerability. (possible use-after-free)

[Describe test coverage new/current, TreeHerder]:  I believe we have pretty good test coverage for our DOM event code.  This patch doesn't add any new tests, because I haven't been able to make this crash yet; this bug was discovered via code inspection.  (But even if we had a testcase, we wouldn't want to publish it yet.)

[Risks and why]: Extremely low-risk. This patch is just making us keep an owning reference to something slightly longer (keeping it alive as long as we're using it).

[String/UUID change made/needed]: None

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: patch, rebased to current trunk (on top of bug 1236979)

Bug 1236979 changed this area of code a little (mostly just reindented it).

Here's an updated version of this bug's patch, rebased on top of current inbound, to accomodate that.  (The only changes are indentation changes, and minor differences in contextual code.)

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Al Billings [:abillings] from comment #5)
> sec-approval+ for checkin on trunk on February 9.

Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/9eb4816ae9f7

> Once it is on trunk, we'll want it on Aurora and Beta as well

--> setting needinfo=me to land the backport in a few days (once it's gotten a day or so of nightly testing).

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9eb4816ae9f7

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8712910 [details] [diff] [review]
fix v1

Reverting the uplift approval as Daniel thinks it needs baking.

Comment 11

[Daniel Holbert [:dholbert]]

Sorry, I didn't mean to imply that it *needs* baking. This is an extremely safe fix.

I just thought there's was no pressing need to land it on aurora/beta instantly (since it's not going to make it to release users for a few weeks regardless), so we might as well give it a day of running on Nightly as an extreme sanity-check. I was planning on landing this tomorrow morning (but can't, now that the approval flags have been changed).

Could you re-approve for Aurora/Beta?

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8712910 [details] [diff] [review]
fix v1

Sure, should be in 45 beta 5.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8cd6f0454c13

Comment 14

[Carsten Book [:Tomcat]]

has problems during uplift to beta -> 

grafting 328321:8cd6f0454c13 "Bug 1243555: Remove unnecessary nsDocShell static_cast in EventListenerManager::HandleEventInternal(). r=smaug, a=sylvestre"
merging dom/events/EventListenerManager.cpp
warning: conflicts while merging dom/events/EventListenerManager.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

Comment 15

[Daniel Holbert [:dholbert]]

Yeah, sorry about that -- the contextual code changed slightly.

Could you try landing the attached "fix v1" patch on beta? That should apply cleanly.

(I can do so, too, though I see a few oranges/reds on beta right now & I'm not sure if I should push.)

Comment 16

[Daniel Holbert [:dholbert]]

Landed on beta:
  https://hg.mozilla.org/releases/mozilla-beta/rev/07434c70518d