1683736 - DebuggerFrame::getThis does not always enter correct Realm

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

In bug 1675755, I added a compartment assertion to LookupOwnPropertyInline because it can result in a cross-compartment edge if it happens to cache a Shape. After the fix in that bug, the assert did not fire in any version before esr78, but in esr78 it revealed another way to fail via DebuggerFrame::getThis.

I believe this fix should still be required on current versions. I haven't tracked down why it might not be firing in the 3 failing tests in post-esr78 versions.

Comment 1

[Steve Fink [:sfink] [:s:]]

(Marked as blocking bug 1675755, which is really only true for esr78.)

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1683736 - Additional fix for Debugger-related Realm entry

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9194258 [details]
Bug 1683736 - Additional fix for Debugger-related Realm entry

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Difficult. Requires Debugger. Even then, I'm not sure how to exploit.
• 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?: 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?: To be specific, I wrote this patch for esr78, but I think it'll apply everywhere. (So yes I have a backport, but can't guarantee a forward port.)
• How likely is this patch to cause regressions; how much testing does it need?: minimal

Beta/Release Uplift Approval Request

• User impact if declined: Theoretically exploitable security flaw. (Most likely just a privilege boundary separation.)
• 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: (only known way to detect is via an assertion added in bug 1675755)
• List of other uplifts needed: Bug 1675755
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Does what is already being done in all similar code.
• String changes made/needed: none

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Very similar situation to bug 1675755, which has been approved for uplift.
• User impact if declined: Theoretically exploitable security flaw. (Most likely just a privilege boundary separation.)
• Fix Landed on Version: (unlanded)
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): see above
• String or UUID changes made by this patch: none

Comment 4

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

Comment on attachment 9194258 [details]
Bug 1683736 - Additional fix for Debugger-related Realm entry

Approved to land

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/2166b1830b9325f6c185ac77e29a3daf851b276b

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/2166b1830b93

Comment 7

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9194258 [details]
Bug 1683736 - Additional fix for Debugger-related Realm entry

approved for 85.0b6

Comment 8

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/releases/mozilla-beta/rev/d113935e0a4a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9194258 [details]
Bug 1683736 - Additional fix for Debugger-related Realm entry

Approved for 78.7esr.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/c2ee5ee5c848