diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f5241f9..a8d94ae8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ Supported `polkadot-sdk` rev: `2604.2.0` ### Fixed - `--newyork`: A bug in dead call value analysis. [#589](https://github.com/paritytech/revive/pull/589) +- `--newyork`: an `mcopy` destination or an external call return range covering the free memory pointer word did not disable the `FMP < heap_size` range proof, which truncated the clobbered `mload(0x40)`. ## v1.4.0 diff --git a/crates/integration/contracts/CopyFmpBug.yul b/crates/integration/contracts/CopyFmpBug.yul new file mode 100644 index 00000000..cc1fbb32 --- /dev/null +++ b/crates/integration/contracts/CopyFmpBug.yul @@ -0,0 +1,22 @@ +/// An `mcopy` onto a calldata supplied destination and a `staticcall` returning into `0x40`. +object "CopyFmpBug" { + code { datacopy(0, dataoffset("CopyFmpBug_deployed"), datasize("CopyFmpBug_deployed")) return(0, datasize("CopyFmpBug_deployed")) } + object "CopyFmpBug_deployed" { + code { + switch calldataload(0) + case 1 { + mstore(0x80, calldataload(32)) + mcopy(and(calldataload(64), 0xff), 0x80, 0x20) + } + case 2 { + if iszero(staticcall(gas(), address(), 0, 0, 0x40, 0x20)) { revert(0, 0) } + } + default { + mstore(0, not(0)) + return(0, 32) + } + mstore(0, mload(0x40)) + return(0, 32) + } + } +} diff --git a/crates/integration/contracts/FmpMcopyStraddle.sol b/crates/integration/contracts/FmpMcopyStraddle.sol new file mode 100644 index 00000000..f71e60e2 --- /dev/null +++ b/crates/integration/contracts/FmpMcopyStraddle.sol @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8; + +/// Reproducer from paritytech/bugbounty_reports#216. +contract FmpMcopyStraddle { + function probe() external view returns (uint256 r) { + assembly { + mstore(0x80, calldataload(0)) + mcopy(0x38, 0x80, 42) + r := mload(0x40) + mstore(0x40, 0x80) + } + } +} diff --git a/crates/integration/contracts/FmpStaticcallReturn.sol b/crates/integration/contracts/FmpStaticcallReturn.sol new file mode 100644 index 00000000..d8c626d8 --- /dev/null +++ b/crates/integration/contracts/FmpStaticcallReturn.sol @@ -0,0 +1,16 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8; + +/// Reproducer from paritytech/bugbounty_reports#216. +contract FmpStaticcallReturn { + function probe() external view returns (uint256 r) { + assembly { + mstore(0x80, 0x100000000000000000000000000000000000000000000000007) + pop(staticcall(gas(), 4, 0x80, 0x20, 0x40, 0x20)) + mstore(add(0x2000, calldatasize()), 0) + r := mload(0x40) + mstore(0x40, 0x80) + } + } +} diff --git a/crates/integration/src/tests.rs b/crates/integration/src/tests.rs index 2aaff9a9..e16793b7 100644 --- a/crates/integration/src/tests.rs +++ b/crates/integration/src/tests.rs @@ -4984,6 +4984,51 @@ fn calldatacopy_dynamic_dest_fmp_corruption() { run_differential(actions); } +/// An `mcopy` or a call return range over the free memory pointer word must disable the range proof. +#[test] +fn copy_onto_fmp_word_disables_range_proof() { + let copied_word: U256 = (U256::from(1u64) << 200) | U256::from(0xabcdefu64); + let mut mcopy_case = U256::from(1).to_be_bytes::<32>().to_vec(); + mcopy_case.extend_from_slice(&copied_word.to_be_bytes::<32>()); + mcopy_case.extend_from_slice(&U256::from(0x40).to_be_bytes::<32>()); + let staticcall_case = U256::from(2).to_be_bytes::<32>().to_vec(); + for data in [mcopy_case, staticcall_case] { + let mut actions = instantiate_yul("contracts/CopyFmpBug.yul", "CopyFmpBug"); + actions.push(Call { + origin: TestAddress::Alice, + dest: TestAddress::Instantiated(0), + value: 0, + gas_limit: Some(GAS_LIMIT), + storage_deposit_limit: None, + data, + }); + run_differential(actions); + } +} + +/// Reproducers from paritytech/bugbounty_reports#216. +#[test] +fn fmp_staticcall_return_and_mcopy_straddle() { + let selector = keccak256(b"probe()")[..4].to_vec(); + let mut mcopy_calldata = selector.clone(); + mcopy_calldata.extend_from_slice(&[0xff; 32]); + for (contract, data) in [ + ("FmpStaticcallReturn", selector), + ("FmpMcopyStraddle", mcopy_calldata), + ] { + let mut actions = instantiate(&format!("contracts/{contract}.sol"), contract); + actions.push(Call { + origin: TestAddress::Alice, + dest: TestAddress::Instantiated(0), + value: 0, + gas_limit: Some(GAS_LIMIT), + storage_deposit_limit: None, + data, + }); + run_differential(actions); + } +} + /// Regression (newyork dead-store elimination): a store read back by an /// intervening unaligned *overlapping* load must not be eliminated as dead. /// `mem_opt` marked a pending store read only on an exact-offset load, so diff --git a/crates/newyork/src/heap_opt.rs b/crates/newyork/src/heap_opt.rs index 01219842..f9efcbf2 100644 --- a/crates/newyork/src/heap_opt.rs +++ b/crates/newyork/src/heap_opt.rs @@ -363,6 +363,7 @@ impl HeapAnalysis { let destination_start = self.extract_static_offset(destination); let source_start = self.extract_static_offset(source); let len = self.extract_static_offset(length); + self.flag_write_covering_fmp(destination, length); self.taint_range(destination_start, len); self.taint_range(source_start, len); } @@ -376,6 +377,7 @@ impl HeapAnalysis { } => { self.mark_escaping_range(args_offset, args_length); self.note_fmp_coverage(args_offset, args_length); + self.flag_write_covering_fmp(ret_offset, ret_length); self.mark_escaping_and_tainted_range(ret_offset, ret_length); self.note_fmp_coverage(ret_offset, ret_length); } @@ -589,6 +591,24 @@ impl HeapAnalysis { } } + /// Flags the free memory pointer as possibly unbounded when a raw write of `length` + /// bytes to `destination` can cover `[0x40, 0x60)`. A zero length never covers it, + /// and neither does a dynamic destination that is free pointer relative. + fn flag_write_covering_fmp(&mut self, destination: &Value, length: &Value) { + let destination_start = self.extract_static_offset(destination); + let len = self.extract_static_offset(length); + + let covers_fmp = match (destination_start, len) { + (_, Some(0)) => false, + (Some(address), Some(size)) => address < 0x60 && address.saturating_add(size) > 0x40, + (Some(address), None) => (0x40..0x60).contains(&address), + (None, _) => !self.is_free_pointer_relative(destination.id.0), + }; + if covers_fmp { + self.fmp_could_be_unbounded = true; + } + } + /// Taints the destination of a copy opcode (`calldatacopy`, `codecopy`, /// `returndatacopy`, …), which writes big-endian bytes that a later native /// (little-endian) `mload` must not byte-reverse. @@ -600,39 +620,11 @@ impl HeapAnalysis { /// native mode for the entire contract (e.g. every ABI-decode `calldatacopy`), /// a large code-size regression for no soundness gain over the existing /// dynamic-offset guards. - /// Records the memory tainted by a copy (`calldatacopy`/`codecopy`/`mcopy`/…) with - /// destination `destination` and length `length`, and flags the free-memory-pointer slot as - /// possibly unbounded when the copy can clobber it. - /// - /// A copy that can overwrite the FMP slot `[0x40, 0x60)` replaces the free-memory - /// pointer with arbitrary, possibly out-of-range bytes. Downstream codegen that assumes - /// `FMP < heap_size` (the narrow `mload(0x40)` read and its range proof) would then - /// mis-read the corrupted value, so such a copy sets `fmp_could_be_unbounded` — exactly - /// as an untrusted `mstore(0x40, ...)` does. Tainting word 0x40 alone only disables the - /// *native-mode* FMP read; the FMP *range proof* in `to_llvm` is gated on - /// `fmp_could_be_unbounded`, so that flag must be set too or the proof silently - /// truncates the clobbered value. - /// - /// `covers_fmp` is kept deliberately narrow to avoid a code-size regression: only a - /// static destination+length that provably overlap the slot, or a static destination - /// *inside* the FMP word with a dynamic length (whose first byte(s) land in the slot), - /// flag unboundedness. A fully-dynamic destination, or a static destination *outside* - /// the word (proxy `calldatacopy(0, 0, size)`, OZ's FMP-relative ABI-decode copies to - /// `mload(0x40) >= 0x80`), is left to `has_dynamic_accesses` / the native-mode guards. fn taint_copy_destination(&mut self, destination: &Value, length: &Value) { let destination_start = self.extract_static_offset(destination); let len = self.extract_static_offset(length); - let covers_fmp = match (destination_start, len) { - (Some(address), Some(size)) => { - size > 0 && address < 0x60 && address.saturating_add(size) > 0x40 - } - (Some(address), None) => (0x40..0x60).contains(&address), - (None, _) => !self.is_free_pointer_relative(destination.id.0), - }; - if covers_fmp { - self.fmp_could_be_unbounded = true; - } + self.flag_write_covering_fmp(destination, length); match (destination_start, len) { (Some(address), Some(size)) => { @@ -2004,6 +1996,76 @@ mod tests { ); } + fn object_with_mcopy(setup: Vec, length: u64) -> Object { + use crate::ir::ValueId; + let mut statements = setup; + statements.push(literal(11, 0x80)); + statements.push(literal(12, length)); + statements.push(Statement::MCopy { + destination: Value::int(ValueId(10)), + source: Value::int(ValueId(11)), + length: Value::int(ValueId(12)), + }); + object_with_code(statements, vec![]) + } + + fn object_with_external_call(setup: Vec, return_length: u64) -> Object { + use crate::ir::{CallKind, ValueId}; + let mut statements = setup; + statements.push(literal(11, return_length)); + statements.push(literal(12, 0)); + statements.push(Statement::ExternalCall { + kind: CallKind::StaticCall, + gas: Value::int(ValueId(12)), + address: Value::int(ValueId(12)), + value: None, + args_offset: Value::int(ValueId(12)), + args_length: Value::int(ValueId(12)), + ret_offset: Value::int(ValueId(10)), + ret_length: Value::int(ValueId(11)), + result: ValueId(13), + }); + object_with_code(statements, vec![]) + } + + #[test] + fn mcopy_onto_fmp_word_flags_unbounded() { + let results = object_with_mcopy(vec![literal(10, 0x40)], 0x20).analyze_heap(); + assert!(results.fmp_could_be_unbounded()); + } + + #[test] + fn mcopy_dynamic_destination_flags_unbounded() { + use crate::ir::ValueId; + let setup = vec![Statement::Let { + bindings: vec![ValueId(10)], + value: Expression::CallDataLoad { + offset: Value::int(ValueId(0)), + }, + }]; + let results = object_with_mcopy(setup, 0x20).analyze_heap(); + assert!(results.fmp_could_be_unbounded()); + } + + #[test] + fn external_call_return_onto_fmp_word_flags_unbounded() { + let results = object_with_external_call(vec![literal(10, 0x40)], 0x20).analyze_heap(); + assert!(results.fmp_could_be_unbounded()); + } + + #[test] + fn external_call_zero_length_return_stays_bounded() { + use crate::ir::ValueId; + let setup = vec![Statement::Let { + bindings: vec![ValueId(10)], + value: Expression::CallDataLoad { + offset: Value::int(ValueId(0)), + }, + }]; + let results = object_with_external_call(setup, 0).analyze_heap(); + assert!(!results.fmp_could_be_unbounded()); + } + #[test] fn test_offset_info_from_literal() { let analysis = HeapAnalysis::new();