Last Comment Bug 750820 - (CVE-2012-1958) Use-after-free in nsGlobalWindow::PageHidden
: Use-after-free in nsGlobalWindow::PageHidden
: sec-moderate
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 14 Branch
: All All
: -- normal (vote)
: mozilla16
Assigned To: Andrew McCreight [:mccr8]
: Andrew Overholt [:overholt]
Depends on:
  Show dependency treegraph
Reported: 2012-05-01 11:19 PDT by Arthur Gerkis
Modified: 2015-10-16 11:41 PDT (History)
11 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

ASan log (12.61 KB, text/plain)
2012-05-01 11:19 PDT, Arthur Gerkis
no flags Details
nsCOMPtrs are your friend (1.18 KB, patch)
2012-05-18 15:25 PDT, Andrew McCreight [:mccr8]
bzbarsky: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Arthur Gerkis 2012-05-01 11:19:24 PDT
Created attachment 619997 [details]
ASan log

Heap-use-after-free caught on Firefox 14.0a1, ASan log attached. Unfortunately no PoC at the moment - will try to catch it again.
Comment 1 David Baron :dbaron: ⌚️UTC-10 2012-05-01 11:47:47 PDT
Comment on attachment 619997 [details]
ASan log

Seems curious that this code address in the first stack:
>    #1 0x7f67ec1439c9 in nsGlobalWindow::PageHidden() /srv/repos/browser/mozilla-central/dom/base/nsGlobalWindow.cpp:7937
and this code address in the second stack:
>    #3 0x7f67ec1439c9 in nsGlobalWindow::PageHidden() /srv/repos/browser/mozilla-central/dom/base/nsGlobalWindow.cpp:7937
are exactly the same despite appearing to call different functions.
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-05-01 11:57:48 PDT
I'd like to see a test case.  These stacks don't make much sense.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2012-05-01 18:56:52 PDT
This sort of makes sense, if you assume that nsFocusManager::WindowHidden got inlined into nsGlobalWindow::PageHidden by PGO without updating debugging symbols accordingly or some such pernicious compiler behavior.

In particular, nsFocusManager::WindowHidden has this code:

957   nsIContent* oldFocusedContent = mFocusedContent;
958   mFocusedContent = nsnull;

mFocusedContent is an nsCOMPtr.  oldFocusedContent probably should be too, in case we're holding the last ref to the content (though how _that_ can be happening is not clear to me).
Comment 4 Al Billings [:abillings] 2012-05-16 10:37:25 PDT
We can't move this forward or rate this without a testcase. Arthur, can you supply one or we'll need to close this as incomplete.
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-05-16 11:45:48 PDT
Comment 3 contains an actionable suggestion, even without a test case.
Comment 6 Al Billings [:abillings] 2012-05-16 13:31:00 PDT
(In reply to Kyle Huey [:khuey] ( from comment #5)
> Comment 3 contains an actionable suggestion, even without a test case.

All right. Who can we assign this to? Since it is a security bug, it is going to be hidden from most people.
Comment 7 Andrew McCreight [:mccr8] 2012-05-18 08:49:53 PDT
I can implement Comment 3.  Looks simple enough.
Comment 8 Andrew McCreight [:mccr8] 2012-05-18 15:25:27 PDT
Created attachment 625285 [details] [diff] [review]
nsCOMPtrs are your friend

The code bz pointed out (that is fixed here) goes back at least to ESR10.  Hard to know if that is all that is needed to cause the problem from comment 0.
Comment 9 Andrew McCreight [:mccr8] 2012-05-19 14:00:45 PDT
Comment on attachment 625285 [details] [diff] [review]
nsCOMPtrs are your friend

Try run looked fine.
Comment 10 Andrew McCreight [:mccr8] 2012-05-23 09:57:21 PDT
This looks like an sg:crit, though the mystery of how to trigger the second condition remains.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2012-05-24 12:36:39 PDT
Comment on attachment 625285 [details] [diff] [review]
nsCOMPtrs are your friend

Comment 12 Andrew McCreight [:mccr8] 2012-05-24 13:44:32 PDT
Given how late it is in the cycle, I suppose it makes sense to wait until the middle of the next one, then land everywhere...
Comment 13 Andrew McCreight [:mccr8] 2012-06-13 11:27:40 PDT
marking sec-critical due to it being a use-after-free.
Comment 14 Daniel Veditz [:dveditz] 2012-06-14 13:26:59 PDT
Should be OK to land now, although it's a potentially attention-grabbing simple patch it's not directly obvious how to get from here to how to trigger it. Make sure it does land everywhere this cycle, though.
Comment 16 Andrew McCreight [:mccr8] 2012-06-14 14:16:39 PDT
Comment 17 Ed Morley [:emorley] 2012-06-15 06:12:17 PDT
Comment 18 Al Billings [:abillings] 2012-06-15 07:20:13 PDT
I expect that the fix will not be verified since no one can trigger the original problem on demand.
Comment 19 Arthur Gerkis 2012-06-15 07:29:08 PDT
At least in build cf4face65451 I don't see it crashing anymore with this ASan stacktrace.
Comment 20 Daniel Veditz [:dveditz] 2012-06-15 10:22:19 PDT
Prior to the patch did the stacktrace come up often enough to make comment 19 reliable evidence that we fixed the bug you reported?
Comment 21 Arthur Gerkis 2012-06-15 10:48:28 PDT
It wasn't crashing often, also, as far as I remember, I haven't seen these stacktrace on something later than 14 branch. Just to avoid confusion: with "At least" I meant that this bug is most likely fixed.
Comment 22 Andrew McCreight [:mccr8] 2012-06-18 11:36:36 PDT
Comment on attachment 625285 [details] [diff] [review]
nsCOMPtrs are your friend

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sg:crit
User impact if declined: possible crashes, sg:crit
Fix Landed on Version: 16
Risk to taking this patch (and alternatives if risky): low, this just makes an object stay alive a little longer in some cases
String or UUID changes made by this patch: none

[Approval Request Comment]
Bug caused by (feature/regressing bug #): It has been in a while.
Testing completed (on m-c, etc.): It has been on m-c for a few days.
Comment 23 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-18 12:27:25 PDT
Comment on attachment 625285 [details] [diff] [review]
nsCOMPtrs are your friend

Given that we're in the middle of this release cycle and Dan's comment 14, approving to land everywhere.
Comment 26 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-21 16:49:43 PDT
qa- for verification as no one has been able to reliably reproduce this bug. Arthur, if you do get a PoC working, can you please help us verify the fix by testing the latest builds? Thanks.
Comment 27 Arthur Gerkis 2012-06-21 16:54:20 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #26)
> qa- for verification as no one has been able to reliably reproduce this bug.
> Arthur, if you do get a PoC working, can you please help us verify the fix
> by testing the latest builds? Thanks.
Comment 28 chris hofmann 2012-06-25 10:45:29 PDT
Hi Arthur,  how did the testing go?
Comment 29 Arthur Gerkis 2012-06-25 11:04:22 PDT
(In reply to chris hofmann from comment #28)
> Hi Arthur,  how did the testing go?

Hi Chris. Unfortunately I still have no test-case to verify patch, but I also haven't seen any similar stacktrace in new builds while fuzzing. Anyhow, if I get the test-case, I will check it.
Comment 30 Daniel Veditz [:dveditz] 2012-06-25 16:23:48 PDT
Arthur: we're leaning against awarding this a bounty because it seems extremely unlikely an attacker could cause a reallocation of anything useful where oldFocusedContent points between when mFocusedContent might be released and when oldFocusedContent is used in the same function. Do you have any arguments as to what might make this a plausible vector?
Comment 31 Arthur Gerkis 2012-06-26 04:00:01 PDT
In current situation without being able to reproduce the bug, I can't provide any additional information.
Comment 32 Daniel Veditz [:dveditz] 2012-07-13 23:46:41 PDT
modifying severity based on comment 30

Note You need to log in before you can comment on or make changes to this bug.