Closed Bug 1614704 Opened 6 months ago Closed 4 months ago

Alias-set for MCreateThis should record property loads

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla77
Tracking Status
firefox-esr68 76+ fixed
firefox73 --- wontfix
firefox74 --- wontfix
firefox75 --- wontfix
firefox76 + fixed
firefox77 + fixed

People

(Reporter: anba, Assigned: anba)

Details

(Keywords: csectype-jit, sec-high, Whiteboard: [post-critsmash-triage][adv-main76+r][adv-ESR68.8+r])

Attachments

(2 files)

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.

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...

Group: core-security → javascript-core-security
Priority: -- → P1
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED

Is this ready for sec-approval?

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

Flags: needinfo?(andrebargull)
Flags: needinfo?(jdemooij)

(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.

Flags: needinfo?(jdemooij)

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.
Attachment #9129741 - Flags: sec-approval?

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.

Attachment #9129741 - Flags: sec-approval? → sec-approval+

anba, is this ready to land now?

Flags: needinfo?(andrebargull)
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77

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

Flags: needinfo?(andrebargull)

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:
Flags: needinfo?(andrebargull)
Attachment #9129741 - Flags: approval-mozilla-beta?

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:
Attachment #9140691 - Flags: approval-mozilla-esr68?

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

Approved for 76.0b5.

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

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

Approved for 68.8esr also.

Attachment #9140691 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main76+r]
Whiteboard: [post-critsmash-triage][adv-main76+r] → [post-critsmash-triage][adv-main76+r][adv-ESR68.8+r]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.