Closed Bug 1567419 Opened 2 years ago Closed 2 years ago

AddressSanitizer: heap-use-after-free [@ CCGraphBuilder::NoteXPCOMChild] with READ of size 8

Categories

(Core :: DOM: File, --)

x86_64
Windows
--
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 + verified
firefox70 + verified

People

(Reporter: decoder, Assigned: baku)

References

(Regression)

Details

(4 keywords, Whiteboard: [see bug 1565526 for bounty])

Attachments

(2 files)

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 70.0a1-20190717093640-https://hg.mozilla.org/mozilla-central/rev/29e9dde37bd231a94959394554154ede52670c65.

For detailed crash information, see attachment.

This looks like the same issue as bug 1564895, but this is in a Nightly that already has that patch applied.

Group: core-security → dom-core-security
Component: XPCOM → DOM: File
Depends on: 1564895
Flags: needinfo?(amarchesini)
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Duplicate of this bug: 1565526

IIUC, this affects 69+ only. Please update the status flags if I got that wrong, however.

Comment on attachment 9079594 [details]
Bug 1567419 - Ensure the BodyStreamHolder has a valid stream always, r?smaug

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: UAF - no exploitable.
  • 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?: beta
  • If not all supported branches, which bug introduced the flaw?: Bug 1557781
  • Do you have backports for the affected branches?: Yes
  • If not, how different, hard to create, and risky will they be?: easy.
  • How likely is this patch to cause regressions; how much testing does it need?: This is the second iteration on a similar issue. This time I have proposed a better approach moving the 'ownership of setting the stream in the BodyStream. This is better, and it's low risk it creates regressions.
Attachment #9079594 - Flags: sec-approval?
Keywords: checkin-needed
Blocks: 1564895
No longer depends on: 1564895
Blocks: 1565526

Looben Yang reported this issue in bug 1565526 (originally duped to bug 1564895) which takes precedence for the bounty.

Whiteboard: [see bug 1565526 for bounty]
Regressed by: 1557781

Daniel, can you triage the sec-approval, please? The bug is already set as checkin-needed.

Flags: needinfo?(dveditz)

sec-approval+ for trunk. We'll want a beta patch nominated as well.

Flags: needinfo?(dveditz)
Attachment #9079594 - Flags: sec-approval? → sec-approval+
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

Please nominate this for Beta approval when you get a chance.

Flags: needinfo?(amarchesini)

Comment on attachment 9079594 [details]
Bug 1567419 - Ensure the BodyStreamHolder has a valid stream always, r?smaug

Beta/Release Uplift Approval Request

  • User impact if declined: A crash can occur when a response is aborted and cloned.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: There is a test included. It's racy.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): All these crashes were related to a wrong use of the body stream when the response was cloned. Now we have a different approach, which forces the body to be part of the response object always. Low risk, but it works correctly.
  • String changes made/needed: none
Flags: needinfo?(amarchesini)
Attachment #9079594 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Comment on attachment 9079594 [details]
Bug 1567419 - Ensure the BodyStreamHolder has a valid stream always, r?smaug

Fixes a sec bug and a top crash. Approved for 69.0b9.

Attachment #9079594 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

So this bug could be verified using the steps from https://bugzilla.mozilla.org/show_bug.cgi?id=1564821#c0?
And after that which bug of the 3 bugs should I mark as verified?

Thanks.

Flags: needinfo?(amarchesini)

Correct.

Flags: needinfo?(amarchesini)

And after that which bug of the 3 bugs should I mark as verified since all of them have the same steps?

Flags: needinfo?(amarchesini)

This is correct. We should not have additional crashes.

Flags: needinfo?(amarchesini)
QA Whiteboard: [qa-triaged]

Verified as fixed on the latest Firefox Nightly 70.0a1 and on Firefox 69.0b9 ASAN Builds on Windows 10 x 64, Mac OS X 10.14 and on Ubuntu 18.04 x64.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
Duplicate of this bug: 1565526
Flags: needinfo?(amarchesini)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.