Closed Bug 1418841 Opened 2 years ago Closed 2 years ago

WasmInstanceScope::Data has wrong DeletePolicy

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
thunderbird_esr52 --- unaffected
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 --- fixed
firefox59 + fixed

People

(Reporter: tcampbell, Assigned: tcampbell)

Details

(Keywords: sec-moderate, Whiteboard: [adv-main58+])

Attachments

(1 file)

The WasmInstanceScope struct doesn't set it's DeletePolicy in [1] as GC-managed while using a GCPtr in [2]. This results in missing post-barriers.

[1] https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/js/src/gc/Zone.h#1057-1073
[2] https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/js/src/vm/Scope.h#970

Marking S-S until a GC peer can determine otherwise. These scope objects only seem to be used in debugger which probably reduces any risk.
This adds the missing policy definition. I'm not sure how to write a unit test to cover this.
Attachment #8929894 - Flags: review?(jcoppeard)
Comment on attachment 8929894 [details] [diff] [review]
0001-Bug-1418841-Fix-DeletePolicy-of-WasmInstanceScope-Da.patch

Review of attachment 8929894 [details] [diff] [review]:
-----------------------------------------------------------------

Nice catch.
Attachment #8929894 - Flags: review?(jcoppeard) → review+
Jon, any thoughts on uplifts required / check-in timing? Since these objects only are created when user is actively debugging WASM, it seems concern is reduced.

Also, just want to confirm whether or not something like LexicalScope needs to be GC-managed, since it has a trace function to access it's Atoms. I don't have a good grasp on how our Atom GC behaviour works.
Flags: needinfo?(jcoppeard)
(In reply to Ted Campbell [:tcampbell] from comment #4)
This is s-s but I agree that this isn't too bad due to requiring the debugger.  I don't remember exactly what classification that makes this.  You should request sec approval and they will let you know whether you should delay checkin.

Anything that has GCPtrs should use the GC managed delete policy.  Having said that, the main concern is post barriers which are required for generations, and we don't allocate strings in the nursery (yet) and probably we'll never allocate atoms there.
Flags: needinfo?(jcoppeard)
Comment on attachment 8929894 [details] [diff] [review]
0001-Bug-1418841-Fix-DeletePolicy-of-WasmInstanceScope-Da.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Difficult. The objects in question only come up when the Debugger is running and has paused execution with WASM code on the stack.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes, it is clear which structure field is missing the GC barrier. Would need expertise in GC attacks to go from there though.

Which older supported branches are affected by this flaw?
I beleive this was introduced in FF 53.

If not all supported branches, which bug introduced the flaw?
Added in Bug 1286948.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Patch should be the same for 57,58,59. I haven't checked if they auto-apply though. Risk is low. This is only applies to WASM debugging, and existing code already makes use of the fix strategy.

How likely is this patch to cause regressions; how much testing does it need?
Low risk, but we don't have a testcase that hits it in practice.
Attachment #8929894 - Flags: sec-approval?
Group: core-security → javascript-core-security
Calling this a sec-moderate (maybe I'm wrong!) because of the limited scenario for triggering given the requirements listed. I'll clear the sec-approval (since it isn't needed for moderate issues) and you can check in.
Attachment #8929894 - Flags: sec-approval?
Oh right, Pulsebot isn't here..
https://hg.mozilla.org/integration/mozilla-inbound/rev/f60f58516b1a

Will uplift to beta once this sticks.
https://hg.mozilla.org/mozilla-central/rev/f60f58516b1a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment on attachment 8929894 [details] [diff] [review]
0001-Bug-1418841-Fix-DeletePolicy-of-WasmInstanceScope-Da.patch

Approval Request Comment
[Feature/Bug causing the regression]:
Added in FF 53
https://hg.mozilla.org/mozilla-central/rev/4762c110448a11b0eadbf8f2b523f5729d61db71
[User impact if declined]:
Potential stability / security problems when debugging WASM code.
[Is this code covered by automated tests?]:
No, but the use of GCManagedDeletePolicy by other types of scopes is. This GC issue is tricky to write a useful automated test for.
[Has the fix been verified in Nightly?]:
Patch landed in nightly and browser tests still pass.
[Needs manual test from QE? If yes, steps to reproduce]:
No
[List of other uplifts needed for the feature/fix]:
Just this patch
[Is the change risky?]:
Low
[Why is the change risky/not risky?]:
Existing scope types (correctly) use GCManagedDeletePolicy for this same use case. As well, this is limited to users who are actively debugging WASM code.
[String changes made/needed]:
None
Attachment #8929894 - Flags: approval-mozilla-beta?
Comment on attachment 8929894 [details] [diff] [review]
0001-Bug-1418841-Fix-DeletePolicy-of-WasmInstanceScope-Da.patch

Fix a potential security issue when debugging WASM. Beta58+.
Attachment #8929894 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: javascript-core-security → core-security-release
(In reply to Ted Campbell [:tcampbell] from comment #10)
> [Is this code covered by automated tests?]:
> No, but the use of GCManagedDeletePolicy by other types of scopes is. This
> GC issue is tricky to write a useful automated test for.
> [Has the fix been verified in Nightly?]:
> Patch landed in nightly and browser tests still pass.
> [Needs manual test from QE? If yes, steps to reproduce]:
> No

This does not need manual testing, per Ted.
Flags: qe-verify-
Whiteboard: [adv-main58+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.