Closed Bug 1646787 Opened 1 year ago Closed 1 year ago

clear high bits of i32 return values to as short-term Spectre mitigation

Categories

(Core :: Javascript: WebAssembly, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla79
Tracking Status
firefox-esr68 79+ fixed
firefox-esr78 79+ fixed
firefox77 --- wontfix
firefox78 - wontfix
firefox79 + fixed

People

(Reporter: luke, Assigned: lth)

Details

(Keywords: csectype-other, sec-high, Whiteboard: [post-critsmash-triage][sec-survey][adv-main79+r][adv-ESR78.1+r] [adv-esr68.11+r])

Attachments

(3 files)

In bug 1644174, there is a working PoC that uses a Spectre attack on return address speculation to cause a function that returns an i64 to return to a callsite that expects an i32 and then uses that i32 as the operand of a load instruction. On 64-bit systems, this allows the high-bits of the i32 to be non-zero which allows the load to bypass the guard pages and thus access other origins' memory. While Fission is the general fix, the PoC will likely be disclosed before Fission is shipped, so it would be good to mitigate this in the short-term.

I think a minimally-invasive fix would be to, immediately after a call that returns an i32, add an instruction that clears the high bits. (I think just a 32-bit mov on x64 does that?)

Group: core-security → javascript-core-security

Yeah, a plain movl should do it. That sounds pretty nasty. I assume we want to generate this behind some already-existing spectre flag?

(In reply to Lars T Hansen [:lth] from comment #1)

Yeah, a plain movl should do it. That sounds pretty nasty. I assume we want to generate this behind some already-existing spectre flag?

Yeah, I would think javascript.options.spectre.index_masking.

Other questions since I can't see the referenced bug:

  • x64-only or also arm64 (and mips64 for that matter)?
  • intel-only or also amd?

Would be nice not to do this if we don't have to, within reason.

Assignee: nobody → lhansen
Severity: -- → S3
Status: NEW → ASSIGNED
Type: task → enhancement
Priority: -- → P2

Answering my own questions here (after seeing the referenced bug and reading the paper), I guess it's "don't know" across the board, so all 64-bit platforms and both wasm compilers (Cranelift has no Spectre mitigations as of yet).

Then the next question is where these calls can appear (this is WIP, to be updated).

  • We have wasm->wasm calls for sure. A call in wasm code that expects an i32 return should mask the value.
    • This should take care of all calls that go via regular import stubs too
  • With recent wasm feature work we also have some optimized paths for js->wasm and wasm->js calls
    • Here the values should be BigInt and not int64 on the JS side
    • But at least in the js->wasm case the int64->bigInt conversion happens in compiled JS code. Since Ion code can be made to be type-specific there may be some opening here for mispredicting the return
    • We should therefore mask the result of js->wasm calls that expect i32 returns too.
  • Stubs code?

I don't know whether AMD also has analogous RSB behavior, but for simplicity, it probably makes sense to mask on all x64. It's less clear what to do about ARM64; since this whole thing is an incomplete stopgap mitigation until Fission, I think it might be fine to omit ARM64 for now. Adding masks to js->wasm, wasm->js and i32-returning stub code makes sense, since I can imagine the technique being extended to them too.

For the record, I will assume that we only care about sanitizing result registers, not result values that are returned through memory in the multi-value case. I don't see how the latter pose a threat, as those can only be exposed to the wrongly-speculated caller after an explicit (32-bit) load, which will properly clear the high bits. I am thus assuming that there is no microarchitectural speculative forwarding of values that go through memory (near the top of the stack in this case) that will expose the returned 64-bit value as a 64-bit operand in our subsequent address computation without stripping the high bits properly.

[Tracking Requested - why for this release]: This is from a research paper that has been submitted already, so we should ship it sooner rather than later.

Hoping to get something done this week before we branch for FF79. So far the footprint is very light so backports to FF78/ESR should be smooth. ESR68 would almost certainly require a different patch.

(In reply to Lars T Hansen [:lth] from comment #6)
That sounds right to me.

See bug for further information.

Depends on D80819

See bug for more information.

Depends on D80820

The patch adds i32 masking after call to Ion and Baseline, and also in the generated stub code for the direct-jit-to-wasm call path. So far as I can tell, no other stubs or paths need updating because any masks on those paths would be unnecessary (the value is manipulated in a way that is not susceptible to the attack) or redundant (subsequent masking in generated code would just do the work again). I have however added comments in the stubs code at all the places I could find to note why masking is not inserted there. The comments can be removed before landing the patch, if that seems desirable; in practice, since we insert code conditionally on the switch spectreIndexMasking it'll be fairly obvious what's going on anyway.

The white-box test case depends on bug 1647991. It does not expose the attack, it just tests that the correct masking instruction is present in the output. Still, we'd likely land this later.

Comment on attachment 9158807 [details]
Bug 1646787 - Mask i32 wasm results. r?luke

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Very difficult, the bulk of the knowledge required is outside the patch (eg, implementation details of Intel architecture). There are comments in the patch now that are slightly revealing, to aid in review; I intend to remove those before landing the patch, and land them separately later with the test case.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
  • Which older supported branches are affected by this flaw?: all
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: Yes / mostly
  • If not, how different, hard to create, and risky will they be?: ff78 and esr78 should be straightforward - the patch should apply as-is or with minor tweaks. 68 requires a separate patch, i have prepared one for review.
  • How likely is this patch to cause regressions; how much testing does it need?: in principle there may be a slight performance degradation though i'd be astonished if it's actually measurable (and not a result of random fluctuation like changes to code alignment)
Attachment #9158807 - Flags: sec-approval?
Attachment #9158820 - Attachment description: Bug 1646787 - Mask i32 wasm results (ESR68) (WIP) → Bug 1646787 - Mask i32 wasm results (ESR68). r?luke

Re the ESR68 patch, it is simpler (no multi-value to contend with) but can't be tested with the same test cases as for FF79 because we don't have the infrastructure for that in FF68. It passes all wasm tests locally but the rest of the testing will have to be by inspection.

Agreed we don't have to worry too much about painting a target with the patch/tests; you still need a fair amount of rocket science to get to an attack.

sec-approval still pending here, it sure would be nice to land this before merge day. We really believe we're not painting a bulls-eye on anything here, this is pretty obscure stuff, we just want it mitigated.

Flags: needinfo?(dveditz)
Flags: needinfo?(dveditz)
Attachment #9158807 - Flags: sec-approval? → sec-approval+

Comment on attachment 9158807 [details]
Bug 1646787 - Mask i32 wasm results. r?luke

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: There's a risk of data leakage through a soon-to-be-published Spectre attack.
  • Fix Landed on Version: Trunk
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This inserts a single instruction to clear high bits of a value whose high bits are already cleared at the time of execution but which speculation may observe as not-cleared. That is, semantically this is a no-op, it only prevents the CPU from speculating with the wrong value.
  • String or UUID changes made by this patch:
Attachment #9158807 - Flags: approval-mozilla-esr78?

Please advise re desirability of uplifting to ESR 68; a separate patch has been created and approved and is attached to this bug, in case uplift is desirable.

Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla79

(In reply to Lars T Hansen [:lth] from comment #19)

Please advise re desirability of uplifting to ESR 68; a separate patch has been created and approved and is attached to this bug, in case uplift is desirable.

Given the rating, we want this on ESR68 also. Please nominate for approval :)

Comment on attachment 9158820 [details]
Bug 1646787 - Mask i32 wasm results (ESR68). r?luke

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: There's a risk of data leakage through a soon-to-be-published Spectre attack.
  • Fix Landed on Version: Trunk
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This inserts a single instruction to clear high bits of a value whose high bits are already cleared at the time of execution but which speculation may observe as not-cleared. That is, semantically this is a no-op, it only prevents the CPU from speculating with the wrong value.
  • String or UUID changes made by this patch:
Attachment #9158820 - Flags: approval-mozilla-esr68?
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

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.

Flags: needinfo?(lhansen)
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][sec-survey]
Flags: needinfo?(lhansen)

Comment on attachment 9158807 [details]
Bug 1646787 - Mask i32 wasm results. r?luke

Approved for 78.1esr and 68.11esr.

Attachment #9158807 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+
Attachment #9158820 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Flags: in-testsuite?

This was spun off from bug 1644174, and the reporter there was asking about a bounty, so I'm flagging this bug for bounty consideration, as I'm not sure that other bug will get closed any time soon.

Flags: sec-bounty?

We'll address the bounty in the main bug.

Flags: sec-bounty?
Whiteboard: [post-critsmash-triage][sec-survey] → [post-critsmash-triage][sec-survey][adv-main79+r]
Whiteboard: [post-critsmash-triage][sec-survey][adv-main79+r] → [post-critsmash-triage][sec-survey][adv-main79+r][adv-ESR78.1+r]
Whiteboard: [post-critsmash-triage][sec-survey][adv-main79+r][adv-ESR78.1+r] → [post-critsmash-triage][sec-survey][adv-main79+r][adv-ESR78.1+r] [adv-esr68.11+r]
Group: core-security-release
Attachment #9158808 - Attachment description: Bug 1646787 - Test cases. r?luke → Bug 1646787 - Test cases. r=luke
You need to log in before you can comment on or make changes to this bug.