Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ecdsa recovery failure may cause executor to panic #1799

Merged
merged 9 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 21 additions & 21 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
[workspace.package]
version = "3.2.1"
version = "3.3.0"
edition = "2021"
license = "MIT OR Apache-2.0"
repository = "https://github.com/succinctlabs/sp1"
Expand Down Expand Up @@ -47,26 +47,26 @@ debug-assertions = true

[workspace.dependencies]
# sp1
sp1-build = { path = "crates/build", version = "3.2.1" }
sp1-cli = { path = "crates/cli", version = "3.2.1", default-features = false }
sp1-core-machine = { path = "crates/core/machine", version = "3.2.1" }
sp1-core-executor = { path = "crates/core/executor", version = "3.2.1" }
sp1-curves = { path = "crates/curves", version = "3.2.1" }
sp1-derive = { path = "crates/derive", version = "3.2.1" }
sp1-eval = { path = "crates/eval", version = "3.2.1" }
sp1-helper = { path = "crates/helper", version = "3.2.1", default-features = false }
sp1-primitives = { path = "crates/primitives", version = "3.2.1" }
sp1-prover = { path = "crates/prover", version = "3.2.1" }
sp1-recursion-compiler = { path = "crates/recursion/compiler", version = "3.2.1" }
sp1-recursion-core = { path = "crates/recursion/core", version = "3.2.1", default-features = false }
sp1-recursion-derive = { path = "crates/recursion/derive", version = "3.2.1", default-features = false }
sp1-recursion-gnark-ffi = { path = "crates/recursion/gnark-ffi", version = "3.2.1", default-features = false }
sp1-recursion-circuit = { path = "crates/recursion/circuit", version = "3.2.1", default-features = false }
sp1-sdk = { path = "crates/sdk", version = "3.2.1" }
sp1-cuda = { path = "crates/cuda", version = "3.2.1" }
sp1-stark = { path = "crates/stark", version = "3.2.1" }
sp1-lib = { path = "crates/zkvm/lib", version = "3.2.1", default-features = false }
sp1-zkvm = { path = "crates/zkvm/entrypoint", version = "3.2.1", default-features = false }
sp1-build = { path = "crates/build", version = "3.3.0" }
sp1-cli = { path = "crates/cli", version = "3.3.0", default-features = false }
sp1-core-machine = { path = "crates/core/machine", version = "3.3.0" }
sp1-core-executor = { path = "crates/core/executor", version = "3.3.0" }
sp1-curves = { path = "crates/curves", version = "3.3.0" }
sp1-derive = { path = "crates/derive", version = "3.3.0" }
sp1-eval = { path = "crates/eval", version = "3.3.0" }
sp1-helper = { path = "crates/helper", version = "3.3.0", default-features = false }
sp1-primitives = { path = "crates/primitives", version = "3.3.0" }
sp1-prover = { path = "crates/prover", version = "3.3.0" }
sp1-recursion-compiler = { path = "crates/recursion/compiler", version = "3.3.0" }
sp1-recursion-core = { path = "crates/recursion/core", version = "3.3.0", default-features = false }
sp1-recursion-derive = { path = "crates/recursion/derive", version = "3.3.0", default-features = false }
sp1-recursion-gnark-ffi = { path = "crates/recursion/gnark-ffi", version = "3.3.0", default-features = false }
sp1-recursion-circuit = { path = "crates/recursion/circuit", version = "3.3.0", default-features = false }
sp1-sdk = { path = "crates/sdk", version = "3.3.0" }
sp1-cuda = { path = "crates/cuda", version = "3.3.0" }
sp1-stark = { path = "crates/stark", version = "3.3.0" }
sp1-lib = { path = "crates/zkvm/lib", version = "3.3.0", default-features = false }
sp1-zkvm = { path = "crates/zkvm/entrypoint", version = "3.3.0", default-features = false }

# p3
p3-air = "0.1.4-succinct"
Expand Down
52 changes: 52 additions & 0 deletions crates/core/executor/src/hook.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ pub type BoxedHook<'a> = Arc<RwLock<dyn Hook + Send + Sync + 'a>>;
/// The file descriptor through which to access `hook_ecrecover`.
pub const FD_ECRECOVER_HOOK: u32 = 5;

// note: we skip 6 because we have an eddsa hook in dev
nhtyy marked this conversation as resolved.
Show resolved Hide resolved

/// The file descriptor through which to access `hook_ecrecover_2`.
pub const FD_ECRECOVER_HOOK_2: u32 = 7;

/// A runtime hook. May be called during execution by writing to a specified file descriptor,
/// accepting and returning arbitrary data.
pub trait Hook {
Expand Down Expand Up @@ -76,6 +81,7 @@ impl<'a> Default for HookRegistry<'a> {
// Note: To ensure any `fd` value is synced with `zkvm/precompiles/src/io.rs`,
// add an assertion to the test `hook_fds_match` below.
(FD_ECRECOVER_HOOK, hookify(hook_ecrecover)),
(FD_ECRECOVER_HOOK_2, hookify(hook_ecrecover_2)),
]);

Self { table }
Expand Down Expand Up @@ -116,6 +122,8 @@ pub struct HookEnv<'a, 'b: 'a> {
///
/// WARNING: This function is used to recover the public key outside of the zkVM context. These
/// values must be constrained by the zkVM for correctness.
///
/// Note: This hook will be deprecated in future versions.
nhtyy marked this conversation as resolved.
Show resolved Hide resolved
#[must_use]
pub fn hook_ecrecover(_: HookEnv, buf: &[u8]) -> Vec<Vec<u8>> {
assert_eq!(buf.len(), 65 + 32, "ecrecover input should have length 65 + 32");
Expand All @@ -141,6 +149,50 @@ pub fn hook_ecrecover(_: HookEnv, buf: &[u8]) -> Vec<Vec<u8>> {
vec![bytes.to_vec(), s_inverse.to_bytes().to_vec()]
}

/// Recovers the public key from the signature and message hash using the k256 crate.
///
/// # Arguments
///
/// * `env` - The environment in which the hook is invoked.
/// * `buf` - The buffer containing the signature and message hash.
/// - The signature is 65 bytes, the first 64 bytes are the signature and the last byte is the
/// recovery ID.
/// - The message hash is 32 bytes.
///
/// The result is returned as a pair of bytes, where the first 32 bytes are the X coordinate
nhtyy marked this conversation as resolved.
Show resolved Hide resolved
/// and the second 32 bytes are the Y coordinate of the decompressed point.
///
/// WARNING: This function is used to recover the public key outside of the zkVM context. These
/// values must be constrained by the zkVM for correctness.
#[must_use]
pub fn hook_ecrecover_2(_: HookEnv, buf: &[u8]) -> Vec<Vec<u8>> {
nhtyy marked this conversation as resolved.
Show resolved Hide resolved
assert_eq!(buf.len(), 65 + 32, "ecrecover input should have length 65 + 32");
let (sig, msg_hash) = buf.split_at(65);
let sig: &[u8; 65] = sig.try_into().unwrap();
let msg_hash: &[u8; 32] = msg_hash.try_into().unwrap();

let mut recovery_id = sig[64];
let mut sig = Signature::from_slice(&sig[..64]).unwrap();

if let Some(sig_normalized) = sig.normalize_s() {
sig = sig_normalized;
recovery_id ^= 1;
};
let recid = RecoveryId::from_byte(recovery_id).expect("Computed recovery ID is invalid!");

// recovery failed, indicate to the caller
nhtyy marked this conversation as resolved.
Show resolved Hide resolved
let Ok(recovered_key) = VerifyingKey::recover_from_prehash(&msg_hash[..], &sig, recid) else {
return vec![vec![0]];
};

let bytes = recovered_key.to_sec1_bytes();

let (_, s) = sig.split_scalars();
let s_inverse = s.invert();

vec![vec![1], bytes.to_vec(), s_inverse.to_bytes().to_vec()]
}

#[cfg(test)]
pub mod tests {
use super::*;
Expand Down
Loading