Closed Bug 1756957 (CVE-2022-28285) Opened 1 year ago Closed 11 months ago

Incorrect AliasSet for MLoadTypedArrayElementHole

Categories

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

defect

Tracking

()

RESOLVED FIXED
100 Branch
Tracking Status
firefox-esr91 99+ fixed
firefox98 --- wontfix
firefox99 + fixed
firefox100 + fixed

People

(Reporter: lukas.bernhard, Assigned: lukas.bernhard)

References

(Blocks 2 open bugs, Regression)

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main99+][adv-esr91.8+])

Attachments

(2 files)

Steps to reproduce:

The AliasSet of MLoadTypedArrayElementHole is currently set to AliasSet::Load(AliasSet::UnboxedElement). When lowered to either LLoadTypedArrayElementHole or LLoadTypedArrayElementHoleBigInt and eventually codegened, there is a masm.loadArrayBufferViewLengthIntPtr(...). Therefore, AliasSet::ArrayBufferViewLengthOrOffset should be added to the AliasSet. Furthermore, there is an access to ArrayBufferViewObject::dataOffset(). Therefore, AliasSet::ObjectFields should be added as well.

Group: firefox-core-security → core-security
Component: Untriaged → JavaScript Engine: JIT
Product: Firefox → Core
Group: core-security → javascript-core-security

Sounds like the analysis in comment 0 is correct, from what I understand.
This should also be an easy patch to add it to MLoadTypedArrayElementHole::getAliasSet

Lukas, if you are interested in contributing a patch as well, to have a patch in firefox in addition to having found the issue ;)
Otherwise, any of us can take make the patch.

Severity: -- → S4
Flags: needinfo?(nicolas.b.pierron)
Priority: -- → P2
Assignee: nobody → lukas.bernhard
Status: NEW → ASSIGNED

Setting security rating to sec-moderate, as there is no AliasSet with Store(ArrayBufferViewLengthOrOffset). Not having any of this kind implies that any mutation to the length would have to be done through a call which has a Store(Any).

The other missing AliasSet with Store(ObjectFields), is about the manipulation of a reserved DATA_SLOT slot. The only MIR instruction which might manipulate typed arrays, would be a MMegamorphicStoreSlot, but reserved slot do not have names associated to them.

Thus, this bug does not sounds actionable on its own, but is definitely not correct and could be used with another issue as a way to do random memory read.

Flags: needinfo?(nicolas.b.pierron)
Keywords: sec-moderate

Bug 829896 changed the AliasSet from AliasSet::Store(AliasSet::Any) to AliasSet::Load(AliasSet::TypedArrayElement), CodeGenerator.cpp already accessed dataOffset() at this point.

Regressed by: 829896
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → 100 Branch
Has Regression Range: --- → yes

Nice find, Lukas! Please request Beta and ESR91 approval on this when you get a chance.

Flags: needinfo?(lukas.bernhard)
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

Comment on attachment 9265934 [details]
Bug 1756957 - Fix AliasSet of MLoadTypedArrayElementHole - r=nbp

Beta/Release Uplift Approval Request

  • User impact if declined: In combination with another issue, this bug could serve as a stepping stone for arbitrary memory reads.
  • 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): Unlikely to cause regressions, it just falls back on more conservative assumptions.
  • String changes made/needed: N/A

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: In combination with another issue, this bug could serve as a stepping stone for arbitrary memory reads. It hence is more of a defense-in-depth request than directly affecting user security.
  • User impact if declined: Weakening robustness of the the JS engine in the presence of another flaw.
  • Fix Landed on Version: 100
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Unlikely to cause regressions, it just falls back on more conservative assumptions.
Flags: needinfo?(lukas.bernhard)
Attachment #9265934 - Flags: approval-mozilla-esr91?
Attachment #9265934 - Flags: approval-mozilla-beta?

Comment on attachment 9265934 [details]
Bug 1756957 - Fix AliasSet of MLoadTypedArrayElementHole - r=nbp

Approved for 99.0b5. Thanks.

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

Comment on attachment 9265934 [details]
Bug 1756957 - Fix AliasSet of MLoadTypedArrayElementHole - r=nbp

Approved for 91.8esr.

Attachment #9265934 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main99+]
Attached file advisory.txt
Whiteboard: [post-critsmash-triage][adv-main99+] → [post-critsmash-triage][adv-main99+][adv-esr91.8+]
Alias: CVE-2022-28285
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.