1614704 - Alias-set for MCreateThis should record property loads

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[André Bargull [:anba]]

MCreateThis::getAliasSet() returns AliasSet::None() even though it executes js::GetProperty() (via CreateThisFromIon -> CreateThis -> CreateThisForFunction -> GetPrototypeFromConstructor). Shouldn't MCreateThis at least have AliasSet::Load(AliasSet::ObjectFields | AliasSet::FixedSlot | AliasSet::DynamicSlot) or possibly just inherit the default definition from MDefinition?

When MCreateThis was added in bug 723333, using AliasSet::None() was okay to do, because the prototype value was directly passed to MCreateThis. bug 825705 changed this and should probably have updated the alias-set, too.

---

Marking as security-sensitive because the other recent alias-set issues were also all marked as s-s.

Comment 1

[Jan de Mooij [:jandem]]

Good catch. Using the default definition is worth trying but I think it's annoying because we then need a resume point (which we don't want because this is only part of the JSOp). We should definitely fix the Load part though...

Comment 2

[André Bargull [:anba]]

Attachment: Bug 1614704: Record load in MCreateThis alias set. r=jandem!

Comment 3

[Julien Cristau [:jcristau]]

Is this ready for sec-approval?

Comment 4

[André Bargull [:anba]]

I'm currently waiting for an answer for https://phabricator.services.mozilla.com/D64737#1972411.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to André Bargull [:anba] from comment #4)

I'm currently waiting for an answer for https://phabricator.services.mozilla.com/D64737#1972411.

Whoops sorry, done.

Comment 6

[André Bargull [:anba]]

Comment on attachment 9129741 [details]
Bug 1614704: Record load in MCreateThis alias set. r=jandem!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It's unknown if an exploit can be created for this issue in the first place.
• 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?: Bug 825705
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: A backport for ESR-68 can easily be created, if we want to backport the change. No idea if we need to backport to Beta at this point, given that we're near the end of the release cycle.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause any regressions, because the patch gives MCreateThis a less permissive alias-set. The affected code is covered by our existing unit tests.

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 9129741 [details]
Bug 1614704: Record load in MCreateThis alias set. r=jandem!

sec-approval+ for 76, but given how late this is (we have RC already I think) let's skip 75.

Comment 8

[Steven DeTar [:sdetar]]

anba, is this ready to land now?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/5a92ace91c7f64fec540ec8b325f8e7011504afb

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/5a92ace91c7f

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval and create and nominate an ESR68 patch as well when you get a chance.

Comment 12

[André Bargull [:anba]]

Attachment: Bug 1614704: Record load in MCreateThis alias set. r=jandem!

Comment 13

[André Bargull [:anba]]

Comment on attachment 9129741 [details]
Bug 1614704: Record load in MCreateThis alias set. r=jandem!

Beta/Release Uplift Approval Request

• User impact if declined: Incorrect alias information can lead to security bugs, cf. bug 1537924, also see bug 1607443, bug 1607683, and bug 1608785 for related cases. Although for this bug we haven't found any direct way to create an exploit.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• 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): This change modifies an alias set to be less permissive, which can only lead to applying fewer JIT optimisations and by that reducing any potential issues. (But in this case we don't expect any performance regressions, because the changes only affect the slow case, which already has next to none optimisations.)
• String changes made/needed:

Comment 14

[André Bargull [:anba]]

Comment on attachment 9140691 [details]
Bug 1614704: Record load in MCreateThis alias set. r=jandem!

ESR Uplift Approval Request

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9129741 [details]
Bug 1614704: Record load in MCreateThis alias set. r=jandem!

Approved for 76.0b5.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0f73661feeb7

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9140691 [details]
Bug 1614704: Record load in MCreateThis alias set. r=jandem!

Approved for 68.8esr also.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/1d43439c78e8