1745170 - table.copy does not consider whether tables are private or public

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P1)

Description

[Lars T Hansen [:lth]]

Attachment: copy.js

The call_indirect optimization avoids creating an indirect stub for module-private functions if the table is also private to the module; this avoids creating redundant stubs. However, it is possible for table.copy to copy from a private table to a public table that is then used for call-indirect in another module. Since table.copy does not create the necessary stubs in this case, the callee will run the function with the wrong tls. See attached test case.

This is going to require an uplift to FF96.

Comment 1

[Lars T Hansen [:lth]]

Attachment: Bug 1745170 - Fix. r?dbezhetskov

Comment 2

[Lars T Hansen [:lth]]

This is probably pretty bad. I would think that it would not be hard to create a wasm program that can read and write pointers fairly freely, going via global variables as in my test case.

Comment 3

[Lars T Hansen [:lth]]

Comment on attachment 9254523 [details]
Bug 1745170 - Fix. r?dbezhetskov

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The exploiter would need an intimate understanding of the recently landed code for indirect wasm calls, but given that i think the patch hints clearly at what's going on. From there to an exploit takes a little thinking but not too much. (The test case is worse, but I've removed it to a different patch.)
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Maybe
• Which older supported branches are affected by this flaw?: FF96
• If not all supported branches, which bug introduced the flaw?: Bug 1742053
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Does not need manual testing; unlikely to cause problems.

Comment 4

[Lars T Hansen [:lth]]

Attachment: Bug 1745170 - Comments and test case. r?yury

Depends on D133345

Comment 5

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

Comment on attachment 9254523 [details]
Bug 1745170 - Fix. r?dbezhetskov

Approved to land and request uplift

Comment 6

[Lars T Hansen [:lth]]

Comment on attachment 9254523 [details]
Bug 1745170 - Fix. r?dbezhetskov

Beta/Release Uplift Approval Request

• User impact if declined: Potentially pretty bad security issue (ability to manufacture pointers). Test case must land later.
• Is this code covered by automated tests?: No
• 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): The bug was caused by an oversight and the change is local.
• String changes made/needed:

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ba02f32699e4

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 9

[Dianna Smith [:diannaS]]

Comment on attachment 9254523 [details]
Bug 1745170 - Fix. r?dbezhetskov

Approved for 96.0b6

Comment 10

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4a58698ea1db