Possible memory corruption of nsDeviceContext

RESOLVED FIXED in Firefox 17

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mats)

Tracking

({csectype-uaf, sec-critical})

Trunk
mozilla19
csectype-uaf, sec-critical
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox16 wontfix, firefox17+ fixed, firefox18+ fixed, firefox19 fixed, firefox-esr1017+ fixed)

Details

(Whiteboard: [adv-track-main17+][adv-track-esr17+])

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
Bug 776505 involves an intermittent orange with ASSERTION: nsDeviceContext not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread().  roc landed an assertion in nsDeviceContext's constructor that it is being created on the main thread, but it does not seem to be triggering in that case. Given that, it seems that it is likely that there is some kind of memory corruption. If the field that stores the thread ID gets messed up, then we'll fail this assertion, even though it is only ever used on the main thread.

The stacks seem to always look like:

0  XUL!nsDeviceContext::AddRef() [nsDeviceContext.h : 26 + 0x0]
1  XUL!nsRenderingContext::Init(nsDeviceContext*, gfxContext*) [nsAutoPtr.h : 845 + 0x7]
2  XUL!PresShell::RenderDocument(nsRect const&, unsigned int, unsigned int, gfxContext*) [nsPresShell.cpp : 4359 + 0xd]
3  XUL!mozilla::dom::CanvasRenderingContext2D::DrawWindow(nsIDOMWindow*, double, double, double, double, nsAString_internal const&, unsigned int, mozilla::ErrorResult&) [CanvasRenderingContext2D.cpp : 3962 + 0xb]
4  XUL!mozilla::dom::CanvasRenderingContext2DBinding::drawWindow [CanvasRenderingContext2DBinding.cpp : 1868 + 0x43]
5  XUL!mozilla::dom::CanvasRenderingContext2DBinding::genericMethod [CanvasRenderingContext2DBinding.cpp : 2570 + 0x12]

This is the strip of code in question:
4347   nsDeviceContext* devCtx = mPresContext->DeviceContext();
4348   gfxFloat scale = gfxFloat(devCtx->AppUnitsPerDevPixel())/nsPresContext::AppUnitsPerCSSPixel();
4349   aThebesContext->Scale(scale, scale);
4354   aThebesContext->NudgeCurrentMatrixToIntegers();
4356   AutoSaveRestoreRenderingState _(this);
4358   nsRefPtr<nsRenderingContext> rc = new nsRenderingContext();
4359   rc->Init(devCtx, aThebesContext);

Not knowing anything about this code, my spideysense did start tingling at that raw pointer to the devCtx. |this| strongly holds mPresContext, which strongly holds its device context, but I guess if either of those fields changes in between line 4347 and 4359 then we could have a UAF. To fix that problem, devCtx could become a strong pointer, or just be inlined, as it is only used twice. We could also add an assert that devCtx is the same as mPresContext->DeviceContext() before the Init, assuming that should be the case.

Kyle also suggested the possibility that devCtx is just corrupt from the get-go. This could be tested by landing an assertion in DeviceContext() to check valid threadedness of the nsDeviceContext.

I'm slightly nervous about poking around with exploratory asserts, as it points out possible security problems, but maybe it isn't a big deal.
(Assignee)

Comment 1

6 years ago
> if either of those fields changes in between line 4347 and 4359

Given the code I'm pretty sure that can't happen.  Also, the pres context
is held strongly by the caller (CanvasRenderingContext2D::DrawWindow).
So I believe it must be corrupt already at 4347.

Reading CanvasRenderingContext2D::DrawWindow I see one bug:
http://hg.mozilla.org/mozilla-central/annotate/792a91b91020/content/canvas/src/CanvasRenderingContext2D.cpp#l3961
presContext->PresShell()->RenderDocument(...)
This is wrong, the caller MUST guarantee liveness for the shell while calling
RenderDocument, otherwise the shell might be deleted and 'mIsDestroying'
is garbage here:
http://hg.mozilla.org/mozilla-central/annotate/792a91b91020/layout/base/nsPresShell.cpp#l4283

It seems somewhat unlikely that we would avoid crashing to reach line
4347 with a deleted 'this', then use a garbage mPresContext ptr.
But I guess it could happen in theory; in that case it seems likely there
would be some crashes also in PresShell::RenderDocument, and there is:
https://crash-stats.mozilla.com/query/query?product=Firefox&version=ALL%3AALL&range_value=4&range_unit=weeks&query_search=signature&query_type=contains&query=PresShell%3A%3ARenderDocument&reason=&build_id=&process_type=any&hang_type=any&do_query=1
(eg bug 747602)

We should fix the RenderDocument() call site anyway, so let's start
there and see if it helps.
(Assignee)

Comment 2

6 years ago
Created attachment 675726 [details] [diff] [review]
fix

see last comment
Attachment #675726 - Flags: review?(roc)
Comment on attachment 675726 [details] [diff] [review]
fix

Review of attachment 675726 [details] [diff] [review]:
-----------------------------------------------------------------

good catch(es)
Attachment #675726 - Flags: review?(roc) → review+
(Assignee)

Comment 4

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7aebe2c45ef

Leave open because this is a tentative fix.  We should look at crash-stats
for PresShell::RenderDocument and our own "orange crashes" like bug 747602
in a few weeks time and see if it seems to have fixed it.
Assignee: nobody → matspal
Whiteboard: [leave open]
(Reporter)

Comment 5

6 years ago
speculatively adding keywords based on the fix
Keywords: csec-uaf, sec-critical
(Assignee)

Comment 6

6 years ago
Created attachment 675890 [details] [diff] [review]
for aurora/beta

On aurora/beta there is also a RenderDocument() call in
nsCanvasRenderingContext2DAzure.cpp which needs the same fix
Attachment #675890 - Flags: review?(roc)
(Assignee)

Comment 7

6 years ago
Created attachment 675891 [details] [diff] [review]
for esr10

esr10 is slightly different
Attachment #675891 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #675891 - Attachment is patch: true
Attachment #675891 - Flags: review? → review?(roc)
(Reporter)

Updated

6 years ago
status-firefox-esr10: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → fixed
tracking-firefox-esr10: --- → ?
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?
(Reporter)

Comment 9

6 years ago
No Bug 776505 crashes in the 1.5 days since this landed, which is promising.
(Assignee)

Comment 10

6 years ago
Comment on attachment 675891 [details] [diff] [review]
for esr10

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: crash
Testing completed (on m-c, etc.): on m-c since 2012-10-27
Risk to taking this patch (and alternatives if risky): very low risk
String or UUID changes made by this patch: none
Attachment #675891 - Flags: approval-mozilla-esr10?
(Assignee)

Comment 11

6 years ago
Comment on attachment 675890 [details] [diff] [review]
for aurora/beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: crash
Testing completed (on m-c, etc.): on m-c since 2012-10-27
Risk to taking this patch (and alternatives if risky): very low risk
String or UUID changes made by this patch: none
Attachment #675890 - Flags: approval-mozilla-beta?
Attachment #675890 - Flags: approval-mozilla-aurora?
tracking-firefox-esr10: ? → 17+
tracking-firefox17: ? → +
tracking-firefox18: ? → +
Attachment #675890 - Flags: approval-mozilla-beta?
Attachment #675890 - Flags: approval-mozilla-beta+
Attachment #675890 - Flags: approval-mozilla-aurora?
Attachment #675890 - Flags: approval-mozilla-aurora+
Attachment #675891 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
(Assignee)

Comment 12

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/7d5b6fef4452
https://hg.mozilla.org/releases/mozilla-beta/rev/49fe7370db22
https://hg.mozilla.org/releases/mozilla-esr10/rev/365e0576b2e7
status-firefox-esr10: affected → fixed
status-firefox16: affected → wontfix
status-firefox17: affected → fixed
status-firefox18: affected → fixed
Whiteboard: [leave open] → [leave open][adv-track-main17+][adv-track-esr17+]
(Reporter)

Comment 13

6 years ago
The underlying orange has gone away, so it seems like this can be closed.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open][adv-track-main17+][adv-track-esr17+] → [adv-track-main17+][adv-track-esr17+]
Target Milestone: --- → mozilla19
Group: core-security
You need to log in before you can comment on or make changes to this bug.