750820 - (CVE-2012-1958) Use-after-free in nsGlobalWindow::PageHidden

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Arthur Gerkis]

Attachment: 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-4, no longer working on Mozilla)]

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 (Exited; not receiving bugmail, old account, do not use)]

I'd like to see a test case.  These stacks don't make much sense.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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 - ex-MoCo]]

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 (Exited; not receiving bugmail, old account, do not use)]

Comment 3 contains an actionable suggestion, even without a test case.

Comment 6

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

(In reply to Kyle Huey [:khuey] ([e-mail]) 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]]

I can implement Comment 3.  Looks simple enough.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: 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]]

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

Try run looked fine.

Comment 10

[Andrew McCreight [:mccr8]]

This looks like an sg:crit, though the mystery of how to trigger the second condition remains.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 12

[Andrew McCreight [:mccr8]]

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]]

marking sec-critical due to it being a use-after-free.

Comment 14

[Daniel Veditz [:dveditz]]

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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d8494a681e3

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1d8494a681e3

Comment 18

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

I expect that the fix will not be verified since no one can trigger the original problem on demand.

Comment 19

[Arthur Gerkis]

At least in build cf4face65451 I don't see it crashing anymore with this ASan stacktrace.

Comment 20

[Daniel Veditz [:dveditz]]

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]

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]]

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]

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 24

[Andrew McCreight [:mccr8]]

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

Comment 26

[u279076]

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]

(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

[chris hofmann]

Hi Arthur,  how did the testing go?

Comment 29

[Arthur Gerkis]

(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]]

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]

In current situation without being able to reproduce the bug, I can't provide any additional information.

Comment 32

[Daniel Veditz [:dveditz]]

modifying severity based on comment 30