1719178 - Always clear nsSHistory::mRootBC before deleting the relevant BrowsingContext

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[Olli Pettay [:smaug][[e-mail]]]

Accessing deleted object is possibly the reason for bug 1704718.

Patch coming.

This is not Fission specific.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1719178 - properly disconnect nsSHistory from the owner object, r=peterv

Writing the test took a bit time, and it is for the CanonicalBrowsingContext owner only.
The test crashes at least in Fission without the patch.
The stack isn't quite the same as in the other bug, but the top part of it looks similar.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1719178 - test, r=peterv

Depends on D119114

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1719178 - properly disconnect nsSHistory from the owner object (for beta 90), r=peterv

Had to expand NS_IMPL_CYCLE_COLLECTION_INHERITED(CanonicalBrowsingContext)
to be able to add the code to unlink and also added destructor for CanonicalBrowsingContext.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

For some reason the testcase doesn't trigger the crash on esr78 although there is similar code there.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1719178 - properly disconnect nsSHistory from the owner object (for esr78), r=peterv

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9229782 [details]
Bug 1719178 - properly disconnect nsSHistory from the owner object, r=peterv

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I don't know how one could trigger the issue from a web page. There is the separate test which runs in the parent process and uses chrome only APIs which can trigger the crash quite easily
• 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?: all
• If not all supported branches, which bug introduced the flaw?: None
• 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?: The patch should be safe. Just clearing a variable when the owner is going away.

Beta/Release Uplift Approval Request

• User impact if declined: Crashes, especially with Fission
• Is this code covered by automated tests?: Yes
• 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):
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined:
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String or UUID changes made by this patch:

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9230161 [details]
Bug 1719178 - properly disconnect nsSHistory from the owner object (for esr78), r=peterv

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined:
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String or UUID changes made by this patch:

Comment 8

[Daniel Veditz [:dveditz]]

We've already built (and maybe released to beta channel?) RC builds for the Fx90 release in 5 days. Given the STR (in the other bug) involve user interaction in chrome-privileged pages, and your comment above about chrome-only APIs this bug doesn't rate a re-spin. and will have to wait for 91. In fact, given those constraints maybe it's really sec-moderate despite being a UAF.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 9230139 [details]
Bug 1719178 - properly disconnect nsSHistory from the owner object (for beta 90), r=peterv

Too late for 90

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 9229782 [details]
Bug 1719178 - properly disconnect nsSHistory from the owner object, r=peterv

sec-approval = dveditz

This can land now on nightly, and then ride the train to Fx91. Will approve for ESR-78 later -- please don't land on ESR during this cycle!

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 9230161 [details]
Bug 1719178 - properly disconnect nsSHistory from the owner object (for esr78), r=peterv

Explicitly setting the sec-approval request on the ESR patch because we will want to approve it after the July 13 release.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Ok, and I'll land the test once the patch is in esr78 too.

Comment 13

[Julien Cristau [:jcristau]]

properly disconnect nsSHistory from the owner object, r=peterv
https://hg.mozilla.org/integration/autoland/rev/1ba3d134e71dbe7dbdcc8d94523c4b83db678884

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/1ba3d134e71d

Comment 15

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9230161 [details]
Bug 1719178 - properly disconnect nsSHistory from the owner object (for esr78), r=peterv

Now that we've cut the branch we can land this in ESR.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 17

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/0546e17473e2

Comment 18

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

test, r=peterv
https://hg.mozilla.org/integration/autoland/rev/4f978602a6c10b3eb412d276c8207362a942222e
https://hg.mozilla.org/mozilla-central/rev/4f978602a6c1

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

oh, some old needinfo here. clearing