Closed Bug 1486415 Opened Last year Closed 11 months ago

AddressSanitizer: heap-use-after-free [@ mozilla::gfx::SourceSurfaceD2D1::DrawTargetWillChange] with WRITE of size 8

Categories

(Core :: Graphics, defect, P3, critical)

x86_64
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr60 64+ fixed
firefox63 + fixed

People

(Reporter: decoder, Assigned: bas.schouten)

References

(Blocks 1 open bug)

Details

(4 keywords)

Crash Data

Attachments

(2 files)

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 63.0a1-20180823100106-https://hg.mozilla.org/mozilla-central/rev/32c6c1848f149454182911c6635b42956cf719de.

For detailed crash information, see attachment.
Flags: sec-bounty?
Group: core-security → gfx-core-security
Looks probably actionable... :-)
Bas, any thoughts as to what might be happening here?
Flags: needinfo?(bas)
So, this would happen if gfx::SourceSurfaceD2D1::DrawTargetWillChange gets called when the snapshot has only a single reference. This shouldn't happen (see DrawTargetD2D1::MarkChanged), however it can if the snapshot is used off the main thread and released there between the call to hasOneRef and DrawTargetWillChange! Fun!

Basically the solution is making DrawTargetWillChange safe to be called when the object has a single reference. This could be done by grabbing a temp ptr to the DrawTarget in MarkIndependent, but that feels a little fragile for my taste, Patch upcoming.
Flags: needinfo?(bas)
Fwiw, I don't think there's a real securtity risk here, this is the object writing a nullptr to itself after it's just essentially destroyed itself, there's no dereference here and there's no lingering reference to the object after this point. So no bounty seems relevant.
Flags: needinfo?(choller)
(In reply to Bas Schouten (:bas.schouten) from comment #6)
> Fwiw, I don't think there's a real securtity risk here, this is the object
> writing a nullptr to itself after it's just essentially destroyed itself,
> there's no dereference here and there's no lingering reference to the object
> after this point. So no bounty seems relevant.

If code can run between the destruction and the self write with the nullptr, then this can still be exploitable. For exploitability, it is not necessary for this object to still be referenced somewhere, it suffices if you can 1) reallocate another object to be there where 2) it helps to overwrite something with null (for example a JS object where you kill some flags fields with the null write).
Flags: needinfo?(choller)
Assignee: nobody → bas
Status: NEW → ASSIGNED
(In reply to Christian Holler (:decoder) from comment #7)
> (In reply to Bas Schouten (:bas.schouten) from comment #6)
> > Fwiw, I don't think there's a real securtity risk here, this is the object
> > writing a nullptr to itself after it's just essentially destroyed itself,
> > there's no dereference here and there's no lingering reference to the object
> > after this point. So no bounty seems relevant.
> 
> If code can run between the destruction and the self write with the nullptr,
> then this can still be exploitable. For exploitability, it is not necessary
> for this object to still be referenced somewhere, it suffices if you can 1)
> reallocate another object to be there where 2) it helps to overwrite
> something with null (for example a JS object where you kill some flags
> fields with the null write).

I did mean to say 'not real'. I didn't mean completely, utterly impossible.

1) It's the result of a race condition that's rare and very hard to predict.
2) You'd then have to have the main thread have its timeslice interrupted -right- between the deletion and the mDrawTarget = nullptr. This again, is a very unlikely event, as this is not a normal place to yield.
3) Another thread would then have to allocate something in the previously freed space.
4) That object would have to be allocated in such a relative position that writing 4 bytes of 0 gives you something useful.

All in all I'd say the chances of anyone winning 2 lottery jackpots at the same time are roughly equivelant to being able to succesfully exploit this. But yes, it is, theoretically possible. Just not a high risk.
Flags: needinfo?(choller)
Comment on attachment 9006001 [details]
Bug 1486415: Ensure the SourceSurface stays alive for the duration of MarkIndependent. r=jrmuizel

Jeff Muizelaar [:jrmuizel] has approved the revision.
Attachment #9006001 - Flags: review+
Crash Signature: [@ mozilla::gfx::SourceSurfaceD2D1::DrawTargetWillChange]
Flags: needinfo?(choller)
(In reply to Bas Schouten (:bas.schouten) from comment #9)
> 
> 1) It's the result of a race condition that's rare and very hard to predict.
> 2) You'd then have to have the main thread have its timeslice interrupted
> -right- between the deletion and the mDrawTarget = nullptr. This again, is a
> very unlikely event, as this is not a normal place to yield.
> 3) Another thread would then have to allocate something in the previously
> freed space.
> 4) That object would have to be allocated in such a relative position that
> writing 4 bytes of 0 gives you something useful.

Yes, it sounds like this would be very hard to exploit. The points 3 and 4 might be less of a problem, but triggering the issue and then make the main thread yield at exactly that position might be really tricky. It might still be eligible for a reduced bounty, especially considering that things we consider hard sometimes later turn out to be quite feasible.
Comment on attachment 9006001 [details]
Bug 1486415: Ensure the SourceSurface stays alive for the duration of MarkIndependent. r=jrmuizel

Approval Request Comment
[Feature/Bug causing the regression]: OMTP
[User impact if declined]: Potential crash, small security risk
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Rare crash in ASAN builds only
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Just adding a temporary reference to an existing object
[String changes made/needed]: None
Attachment #9006001 - Flags: approval-mozilla-beta?
Comment on attachment 9006001 [details]
Bug 1486415: Ensure the SourceSurface stays alive for the duration of MarkIndependent. r=jrmuizel

Security fix, approved for 63 beta
Attachment #9006001 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 9006001 [details]
Bug 1486415: Ensure the SourceSurface stays alive for the duration of MarkIndependent. r=jrmuizel

This needs sec-approval and landing on m-c before we should be considering it for Beta uplift.
Attachment #9006001 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
Bas, mind requesting sec-approval?
Flags: needinfo?(bas)
Priority: -- → P3
Bas, could you please request sec-approval? Thanks
Comment on attachment 9006001 [details]
Bug 1486415: Ensure the SourceSurface stays alive for the duration of MarkIndependent. r=jrmuizel

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Extremely hard, see earlier comments.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: 

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

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?: This patch could potentially cause some memory usage increases when websites do something bad (there has been one report of this).
Flags: needinfo?(bas)
Attachment #9006001 - Flags: sec-approval?
Comment on attachment 9006001 [details]
Bug 1486415: Ensure the SourceSurface stays alive for the duration of MarkIndependent. r=jrmuizel

Giving beta approval as well.
Attachment #9006001 - Flags: sec-approval?
Attachment #9006001 - Flags: sec-approval+
Attachment #9006001 - Flags: approval-mozilla-beta?
Attachment #9006001 - Flags: approval-mozilla-beta+
AFAICT, this already landed on m-c in early September for Fx63?
https://hg.mozilla.org/mozilla-central/rev/00b25e056441

Also present on mozilla-release:
https://hg.mozilla.org/releases/mozilla-release/rev/00b25e056441

Bas, I believe we still want this on ESR60?
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Flags: needinfo?(bas)
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Attachment #9006001 - Flags: approval-mozilla-beta+
How did this even land in September, not to mention why ask for sec-approval if you've already landed it?

If this is fixed in 63, it should have had an advisory too.
Lowering severity based on analysis in comment 9 and comment 11
Keywords: sec-highsec-moderate
(In reply to Al Billings [:abillings] from comment #21)
> How did this even land in September, not to mention why ask for sec-approval
> if you've already landed it?
> 
> If this is fixed in 63, it should have had an advisory too.

At the time it seemed the agreement was this was hard enough to exploit that the fix should just be landed. When I was later asked to request sec-approval, I did, this should probably have gone through sec approval before landing because of the 'well I suppose you never know how hard it really is argument'.

I'm not sure the severity justifies backporting this to ESR, I'll let people more knowledgeable on security be the judge of that.
Flags: needinfo?(bas)
Comment on attachment 9006001 [details]
Bug 1486415: Ensure the SourceSurface stays alive for the duration of MarkIndependent. r=jrmuizel

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Low risk and in case our estimates of exploitability are off the mark

User impact if declined: UAF exploit

Fix Landed on Version: 61

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Changes lifetime subtly, may lead to higher memory usage on poortly constructed sites, but that should be rare.

String or UUID changes made by this patch: None
Attachment #9006001 - Flags: approval-mozilla-esr60?
(In reply to Bas Schouten (:bas.schouten) from comment #24)
> Comment on attachment 9006001 [details]
> Bug 1486415: Ensure the SourceSurface stays alive for the duration of
> MarkIndependent. r=jrmuizel
> 
> [ESR Uplift Approval Request]
> 
> If this is not a sec:{high,crit} bug, please state case for ESR
> consideration: Low risk and in case our estimates of exploitability are off
> the mark
> 
> User impact if declined: UAF exploit
> 
> Fix Landed on Version: 61
> 
> Risk to taking this patch: Low
> 
> Why is the change risky/not risky? (and alternatives if risky): Changes
> lifetime subtly, may lead to higher memory usage on poortly constructed
> sites, but that should be rare.

I was confusing this with another bug. The higher memory usage bit is not true.
Comment on attachment 9006001 [details]
Bug 1486415: Ensure the SourceSurface stays alive for the duration of MarkIndependent. r=jrmuizel

Let's take this for esr60 since we have some uncertainty over the severity level, and it's already on 63.
Attachment #9006001 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Flags: sec-bounty? → sec-bounty+
Group: gfx-core-security → core-security-release
Adding Jessie (new graphics engineering manager) to all sec-crit and sec-high graphics bugs
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.