Closed Bug 1594181 Opened 1 year ago Closed 1 year ago

AddressSanitizer: heap-use-after-free [@ Id] with READ of size 4 through [@ mozilla::PBenchmarkStorageChild::SendPut]

Categories

(Core :: Audio/Video, defect, P1)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox-esr68 --- unaffected
firefox70 --- wontfix
firefox71 + fixed
firefox72 + fixed

People

(Reporter: decoder, Assigned: achronop)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [adv-main71+r])

Crash Data

Attachments

(2 files)

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 72.0a1-20191028215046-https://hg.mozilla.org/mozilla-central/rev/c65ef27b6fc78ec5140068913209bad3b55f1139.

For detailed crash information, see attachment.

mozilla::DecoderBenchmark::Put accesses an instance of PBenchmarkStorageChild via the static method BenchmarkStorageChild::Instance(), which gets the value via the static variable in sChild. Nothing clears this variable, so if the StorageChild goes away, then sChild is a dead pointer. At a minimum, the dtor of BenchmarkStorageChild() should clear sChild if it is equal to this.

Group: core-security → media-core-security
Keywords: csectype-uaf

ContentChild::sSingleton never gets cleared either, but maybe ContentChild is guaranteed to outlive any runnables?

Keywords: sec-high
Flags: sec-bounty?

P1 for sec-high.

Alex can you please have a look at this?

Assignee: nobody → achronop
Flags: needinfo?(achronop)
Priority: -- → P1

(In reply to Andrew McCreight [:mccr8] from comment #4)

ContentChild::sSingleton never gets cleared either, but maybe ContentChild is guaranteed to outlive any runnables?

ContentChild should live as long as the process: it has a stub Release method, and the destructor is never called (except in debug builds for leak checking).

ASan builds also perform the shutdown like debug builds, so this could be an ASan/debug only issue. We have had a few of these already before.

Thank you for filing. The patch above clears the static variable on the destructor of the object that is holding. I avoid adding too many details in the commit message.

Flags: needinfo?(achronop)

Comment on attachment 9108044 [details]
Bug 1594181 - Reset external variable properly on destruction. r?mccr8

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Not easy, reproducing it must depend on timing.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
  • Which older supported branches are affected by this flaw?: 70, 71
  • If not all supported branches, which bug introduced the flaw?: Bug 1530996
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: It's very easy to create. The same patch applies cleanly in beta and release.
  • How likely is this patch to cause regressions; how much testing does it need?: Not likely to cause any regression. It's a small fix that clears properly a static variable on the destruction.
Attachment #9108044 - Flags: sec-approval?

Comment on attachment 9108044 [details]
Bug 1594181 - Reset external variable properly on destruction. r?mccr8

Let's request uplift then land.

Attachment #9108044 - Flags: sec-approval? → sec-approval+

Comment on attachment 9108044 [details]
Bug 1594181 - Reset external variable properly on destruction. r?mccr8

Beta/Release Uplift Approval Request

  • User impact if declined: Intermitten UAF
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It's very easy to create. The same patch applies cleanly in beta and release.
  • String changes made/needed:
Attachment #9108044 - Flags: approval-mozilla-beta?

(In reply to Tom Ritter [:tjr] (needinfo for responses to sec-[approval/ratings/advisories/cve's]) from comment #11)

Comment on attachment 9108044 [details]
Bug 1594181 - Reset external variable properly on destruction. r?mccr8

Let's request uplift then land.

Tom, do you want the uplift and the landing on central to happen at the same time? I am happy to take this patch in the next beta if you are waiting for this information before landing to central.

Flags: needinfo?(tom)

Alex, see question above please, thanks.

Flags: needinfo?(tom) → needinfo?(achronop)

It's low risk, I am fine if uplift and landing in beta happen at the same time. Thank you.

Flags: needinfo?(achronop)
Group: media-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Flags: sec-bounty? → sec-bounty+
Keywords: sec-highsec-moderate

Comment on attachment 9108044 [details]
Bug 1594181 - Reset external variable properly on destruction. r?mccr8

Fixes a sec bug, approved for 71.0b12.

Attachment #9108044 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.