Reject RIP-relative memory operands in the assembly equivalence checker (scrutineer #2516) - #2409
Draft
JasonGross wants to merge 2 commits into
Draft
Reject RIP-relative memory operands in the assembly equivalence checker (scrutineer #2516)#2409JasonGross wants to merge 2 commits into
JasonGross wants to merge 2 commits into
Conversation
Fixes scrutineer finding #2516 (duplicate #2513). The assembly parser faithfully records RIP-relative memory operands (`[rip + disp]`, or any bare `[disp]` after a `DEFAULT REL` line) in the `rip_relative` field of `MEM`, but neither address model ever read that field: `Symbolic.Address` and `Semantics.DenoteAddress` both computed `base + index + disp` with `base = 0`, i.e. they treated the operand as an absolute displacement. Loads and stores through such an operand happened to fail (the folded constant address is never present in symbolic memory), but `lea` never touches memory, so `lea rcx, [rip + 0x26]` was symbolically executed as `mov rcx, 0x26`. An attacker-supplied `--hints-file` could therefore replace any `mov r64, imm` with `lea r64, [rip + imm]` and have the checker certify assembly that computes `RIP + imm` on real hardware. Confirmed with the pre-fix binary: a curve25519 carry_mul hints file with line 108 `mov rdx, 0x33` replaced by `lea rdx, [rip + 0x33]` was accepted and the assembly emitted. Fix, applied consistently to both models: * `Symbolic.Address` now fails with a new, readable error `error.unsupported_rip_relative_addressing <operand>` whenever the operand is `explicitly_rip_relative` or `implicitly_rip_relative`. Since every memory operand (lea, loads, stores) goes through `Address`, the accidental "lookup fails" protection for loads and stores becomes an explicit error too. Label-based operands keep their existing `unsupported_label_in_memory` error. * `Semantics.DenoteAddress` now returns `option Z`, with `None` for RIP-relative or label-based operands, and `DenoteOperand`, `SetOperand` and `lea` propagate that. The machine state has no instruction pointer, so this is the honest semantics; it is also what keeps `SymbolicProofs.Address_R` (symbolic success implies concrete denotation) true. * `SymbolicProofs.v` is adapted (`Address_R` now yields `DenoteAddress ... = Some v`; `GetOperand_R`, `R_SetOperand` and `SetOperand_same` rewrite with it). `EquivalenceProofs.v` and `WithBedrock/Proofs.v` need no changes. * `DEFAULT REL` lines themselves were already rejected by `SymexRawLine` (`unsupported_line`), so implicitly RIP-relative operands could not previously reach `Address` via `SymexLines`; the new check covers them anyway, and a test pins down both facts. * New regression tests: `src/Assembly/SymbolicTests.v` parses and symbolically executes small snippets (`lea`/load/store with `[rip + disp]` and `[rip - disp]`, `DEFAULT REL`, implicit RIP-relative operand built by the parser, a label operand, and a register-relative `lea` that must still succeed); `src/Assembly/WithBedrock/SemanticsTests.v` checks `DenoteAddress` and `lea` are undefined on such operands. No shipped assembly under fiat-amd64/ uses RIP-relative operands, so the existing verification workflow is unaffected. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Rbn2gww3MGhvrh52fNjpD
Two small regression tests adopted (rewritten) from the parallel codex/fix-2516-rip-relative branch: the top-level parser tracks DEFAULT REL across lines and stamps later bare [disp] operands as implicitly RIP-relative, and DenoteOperand/SetOperand are undefined on RIP-relative operands, not just DenoteAddress and lea. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Rbn2gww3MGhvrh52fNjpD
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes scrutineer security finding #2516 (duplicate #2513): RIP-relative memory operands are parsed and stored but ignored by both address models.
The bug
Parse.vrecords[rip + disp](and every bare[disp]after aDEFAULT RELline) in therip_relativefield ofMEM, butSymbolic.Address(src/Assembly/Symbolic.v) andSemantics.DenoteAddress(src/Assembly/WithBedrock/Semantics.v) never read that field and computed0 + 0 + disp. Loads and stores through such an operand happened to fail because the folded constant address is not in symbolic memory, butleanever touches memory, solea rcx, [rip + 0x26]was checked asmov rcx, 0x26. An attacker-supplied--hints-filecan replace anymov r64, immwithlea r64, [rip + imm]and have the equivalence checker certify assembly that computesRIP + immon hardware.I confirmed this end to end with the installed pre-fix binary (
fiat_crypto unsaturated-solinas curve25519 64 5 '2^255 - 19' carry_mul ... --hints-file): a copy offiat-amd64/fiat_curve25519_carry_mul/seed0000000059842304_ratio12536.asmwith line 108mov rdx, 0x33replaced bylea rdx, [rip + 0x33]was accepted (exit 0) and the hostile assembly emitted.The fix
Both models now fail closed, consistently:
Symbolic.Addressraises a new errorerror.unsupported_rip_relative_addressing <operand>(with a readableShowinstance, next tounsupported_label_in_memory) when the operand isexplicitly_rip_relativeorimplicitly_rip_relative. All memory operands (lea, loads, stores) go throughAddress, so loads and stores now get the explicit error instead of the accidental "index not present" one.Semantics.DenoteAddressnow returnsoption Zand isNonefor RIP-relative and label-based operands;DenoteOperand,SetOperand, andleapropagate that. The machine state has no instruction pointer, so this is the honest semantics and it keepsSymbolicProofs.Address_Rtrue.SymbolicProofs.vadapted (Address_Rnow concludesDenoteAddress ... = Some v;GetOperand_R,R_SetOperand,SetOperand_samerewrite with it).EquivalenceProofs.vandWithBedrock/Proofs.vneeded no changes.DEFAULT RELlines were already rejected bySymexRawLine(unsupported_line), so the implicit case could not previously reachAddressthroughSymexLines; the new check covers it anyway and a test pins both facts down.src/Assembly/SymbolicTests.v(parse + symbolic execution oflea/load/store with[rip + disp],[rip - disp],DEFAULT REL, an implicitly RIP-relative operand as produced by the parser underdefault_rel := true, a label operand, and a register-relativeleathat must still succeed) andsrc/Assembly/WithBedrock/SemanticsTests.v(DenoteAddressandleaundefined on such operands, defined on an absolute displacement).Alternatives considered: keeping
DenoteAddress : Zand only guarding inDenoteOperand/SetOperand/leawould work but leaves a function that returns a meaningless value for these operands; makingDenoteAddresspartial is the smaller trust surface. Modelling RIP properly is out of scope, since the symbolic state has no notion of code addresses.grep -rliw rip fiat-amd64 --include=*.asmfinds no shipped assembly using RIP-relative operands, so the existing CryptOpt verification workflow is unaffected.Verification
Compiled locally with
coqc(Rocq 9.4+alpha,-R src Crypto) against the installedcoq-fiat-crypto-with-bedrock, recompiling the full downstream cone of the changed files:src/Assembly/Symbolic.v,src/Assembly/WithBedrock/Semantics.v(changed) — OKsrc/Assembly/Equivalence.v,src/Assembly/EquivalenceProofs.v,src/Assembly/WithBedrock/SymbolicProofs.v,src/Assembly/WithBedrock/Proofs.v(HEAD versions; the last two differ from the installed build) — OKsrc/BoundsPipeline.v,src/PushButtonSynthesis/{Primitives,SaturatedSolinas,UnsaturatedSolinas,WordByWordMontgomery,BaseConversion,DettmanMultiplication,SolinasReduction}.v,src/CLI.v,src/StandaloneOCamlMain.v,src/ExtractionOCaml/unsaturated_solinas.v— OKsrc/Assembly/SymbolicTests.v,src/Assembly/WithBedrock/SemanticsTests.v(new) — OKRebuilt
src/ExtractionOCaml/unsaturated_solinasfrom the extraction with the Makefile.standaloneocamlfind ocamloptcommand and ran:mov rdx, 0x33→lea rdx, [rip + 0x33]): rejected, exit 2, output ends withSymbolic execution failed: error.unsupported_rip_relative_addressing [rip + 0x33] (RIP-relative memory operands are not supported: the symbolic model has no notion of the instruction pointer).seed0000000059842304_ratio12536.asm: verified, exit 0.fiat-amd64/curve25519-carry_mul.statusinvocation fromgentest.py --makefilewith all 30 hints files: verified, exit 0 (about 9 s).Not verified: a full
make(many hours), the other five separate binaries and the JsOfOCaml/Haskell mains (same Coq source, no separate proofs), and the fulltest-amd64-filessuite beyond curve25519 carry_mul. CI should cover those.🤖 Generated with Claude Code
https://claude.ai/code/session_016Rbn2gww3MGhvrh52fNjpD