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

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Looks probably actionable... :-)

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Bas, any thoughts as to what might be happening here?

Comment 5

[Bas Schouten (:bas.schouten)]

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.

Comment 6

[Bas Schouten (:bas.schouten)]

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.

Comment 7

[Christian Holler (:decoder)]

(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).

Comment 8

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1486415: Ensure the SourceSurface stays alive for the duration of MarkIndependent. r=jrmuizel

Comment 9

[Bas Schouten (:bas.schouten)]

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

Comment 10

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 11

[Christian Holler (:decoder)]

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

[Bas Schouten (:bas.schouten)]

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

Comment 13

[Pascal Chevrel (relman team) -> :pascalc]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 15

[Julien Cristau [:jcristau]]

Bas, mind requesting sec-approval?

Comment 16

[Pascal Chevrel (relman team) -> :pascalc]

Bas, could you please request sec-approval? Thanks

Comment 17

[Bas Schouten (:bas.schouten)]

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).

Comment 18

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

sec-approval+ (a month later).

Comment 19

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

Comment on attachment 9006001 [details]
Bug 1486415: Ensure the SourceSurface stays alive for the duration of MarkIndependent. r=jrmuizel

Giving beta approval as well.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 21

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

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.

Comment 22

[Daniel Veditz [:dveditz]]

Lowering severity based on analysis in comment 9 and comment 11

Comment 23

[Bas Schouten (:bas.schouten)]

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

Comment 24

[Bas Schouten (:bas.schouten)]

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

Comment 25

[Bas Schouten (:bas.schouten)]

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

[Liz Henry (:lizzard)]

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.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/ad2e4899a821

Comment 28

[Maire Reavy [:mreavy]]

Adding Jessie (new graphics engineering manager) to all sec-crit and sec-high graphics bugs