Bug 750820 (CVE-2012-1958)

Use-after-free in nsGlobalWindow::PageHidden

RESOLVED FIXED in Firefox 14

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
13 days ago

People

(Reporter: Arthur Gerkis, Assigned: mccr8)

Tracking

({csectype-uaf, sec-moderate})

14 Branch
mozilla16
csectype-uaf, sec-moderate
Points:
---

Firefox Tracking Flags

(firefox13 wontfix, firefox14+ fixed, firefox15+ fixed, firefox16+ fixed, firefox-esr1014+ fixed)

Details

(Whiteboard: [asan][qa-][advisory-tracking+])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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 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.
Component: Untriaged → DOM
Product: Firefox → Core
QA Contact: untriaged → general
I'd like to see a test case.  These stacks don't make much sense.
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).
Keywords: testcase-wanted
Whiteboard: [asan]
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 3 contains an actionable suggestion, even without a test case.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
(Assignee)

Comment 7

5 years ago
I can implement Comment 3.  Looks simple enough.
Assignee: nobody → continuation
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
Comment on attachment 625285 [details] [diff] [review]
nsCOMPtrs are your friend

Try run looked fine.
Attachment #625285 - Flags: review?(bzbarsky)
(Assignee)

Comment 10

5 years ago
This looks like an sg:crit, though the mystery of how to trigger the second condition remains.
Comment on attachment 625285 [details] [diff] [review]
nsCOMPtrs are your friend

r=me
Attachment #625285 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 12

5 years ago
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...
(Assignee)

Comment 13

5 years ago
marking sec-critical due to it being a use-after-free.
Keywords: sec-critical
(Assignee)

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
status-firefox-esr10: --- → affected
status-firefox13: --- → wontfix
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
tracking-firefox-esr10: --- → 14+
tracking-firefox14: --- → +
tracking-firefox15: --- → +
tracking-firefox16: --- → +
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d8494a681e3
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/1d8494a681e3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox16: affected → fixed
Resolution: --- → FIXED
I expect that the fix will not be verified since no one can trigger the original problem on demand.
(Reporter)

Comment 19

5 years ago
At least in build cf4face65451 I don't see it crashing anymore with this ASan stacktrace.
Prior to the patch did the stacktrace come up often enough to make comment 19 reliable evidence that we fixed the bug you reported?
(Reporter)

Comment 21

5 years ago
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.
(Assignee)

Comment 22

5 years ago
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.
Attachment #625285 - Flags: approval-mozilla-esr10?
Attachment #625285 - Flags: approval-mozilla-beta?
Attachment #625285 - Flags: approval-mozilla-aurora?
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.
Attachment #625285 - Flags: approval-mozilla-esr10?
Attachment #625285 - Flags: approval-mozilla-esr10+
Attachment #625285 - Flags: approval-mozilla-beta?
Attachment #625285 - Flags: approval-mozilla-beta+
Attachment #625285 - Flags: approval-mozilla-aurora?
Attachment #625285 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 24

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/943bca296b0c
https://hg.mozilla.org/releases/mozilla-beta/rev/2ea979a7f531
https://hg.mozilla.org/releases/mozilla-esr10/rev/f1dfb11df383
status-firefox-esr10: affected → fixed
status-firefox14: affected → fixed
status-firefox15: affected → fixed
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.
Whiteboard: [asan] → [asan][qa-]
(Reporter)

Comment 27

5 years ago
(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.
Sure.

Comment 28

5 years ago
Hi Arthur,  how did the testing go?
(Reporter)

Comment 29

5 years ago
(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.
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?
(Reporter)

Comment 31

5 years ago
In current situation without being able to reproduce the bug, I can't provide any additional information.
Whiteboard: [asan][qa-] → [asan][qa-][advisory-tracking+]
Alias: CVE-2012-1958
modifying severity based on comment 30
Keywords: sec-critical → sec-moderate
Group: core-security
Keywords: testcase-wanted
Keywords: csectype-uaf
You need to log in before you can comment on or make changes to this bug.