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

Status: VERIFIED FIXED

(Core :: DOM: File, --)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Andrew McCreight [:mccr8]]

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

Comment 3

[Andrea Marchesini [:baku]]

Attachment: Bug 1567419 - Ensure the BodyStreamHolder has a valid stream always, r?smaug

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

[Andrea Marchesini [:baku]]

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.

Comment 7

[Daniel Veditz [:dveditz]]

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

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 9

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

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/919cfe3f4d6b6b0a099bc0aa9a40ba8703ba6332

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/919cfe3f4d6b

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Andrea Marchesini [:baku]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1eee7ed3de83

Comment 16

[Hani Yacoub, Desktop QA]

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.

Comment 17

[Andrea Marchesini [:baku]]

Correct.

Comment 18

[Hani Yacoub, Desktop QA]

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

Comment 19

[Andrea Marchesini [:baku]]

This is correct. We should not have additional crashes.

Comment 20

[Hani Yacoub, Desktop QA]

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.

Comment 22

[Daniel Veditz [:dveditz]]

(In reply to Andrea Marchesini [:baku] from comment #6)

• How easily could an exploit be constructed based on the patch?: UAF - no exploitable.

That doesn't match the results in bug 1565526 comment 6. Is that one not a dupe then?
Never mind, it was explicitly confirmed in that bug.