From 95c6cda87c16f6ca0623f14369ba5218b994908e Mon Sep 17 00:00:00 2001 From: Mark Logan <103447440+mystenmark@users.noreply.github.com> Date: Mon, 20 Nov 2023 22:14:11 -0800 Subject: [PATCH] Sui private security patches 1.14 (#84) * Tests and checks for transaction input edge cases * Fix lints * cherry pick fix * Additional checks * Remove unnecessary checks * Fix authority_tests::test_handle_transfer_transaction_bad_signature --- .../benches/verified_cert_cache_bench.rs | 1 + crates/sui-core/src/authority.rs | 4 + .../authority/authority_per_epoch_store.rs | 4 + crates/sui-core/src/authority_server.rs | 9 +- crates/sui-core/src/signature_verifier.rs | 64 +- .../src/unit_tests/authority_tests.rs | 6 +- .../src/unit_tests/transaction_tests.rs | 609 ++++++++++++++++++ crates/sui-e2e-tests/tests/zklogin_tests.rs | 6 +- crates/sui-types/src/base_types.rs | 14 +- crates/sui-types/src/effects/mod.rs | 4 + crates/sui-types/src/error.rs | 2 +- crates/sui-types/src/message_envelope.rs | 3 + crates/sui-types/src/messages_checkpoint.rs | 4 + crates/sui-types/src/transaction.rs | 58 +- .../src/unit_tests/messages_tests.rs | 4 +- crates/sui-types/src/unit_tests/utils.rs | 42 +- .../sui-types/src/zk_login_authenticator.rs | 6 +- 17 files changed, 783 insertions(+), 57 deletions(-) create mode 100644 crates/sui-core/src/unit_tests/transaction_tests.rs diff --git a/crates/sui-core/benches/verified_cert_cache_bench.rs b/crates/sui-core/benches/verified_cert_cache_bench.rs index 9ed2b41232f5a..483d09d642aa2 100644 --- a/crates/sui-core/benches/verified_cert_cache_bench.rs +++ b/crates/sui-core/benches/verified_cert_cache_bench.rs @@ -27,6 +27,7 @@ fn verified_cert_cache_bench(c: &mut Criterion) { let metrics = SignatureVerifierMetrics::new(®istry); let cache = VerifiedDigestCache::::new( metrics.certificate_signatures_cache_hits.clone(), + metrics.certificate_signatures_cache_misses.clone(), metrics.certificate_signatures_cache_evictions.clone(), ); diff --git a/crates/sui-core/src/authority.rs b/crates/sui-core/src/authority.rs index 661a74d2a7e08..3c09a4e96c412 100644 --- a/crates/sui-core/src/authority.rs +++ b/crates/sui-core/src/authority.rs @@ -145,6 +145,10 @@ use crate::transaction_manager::TransactionManager; #[path = "unit_tests/authority_tests.rs"] pub mod authority_tests; +#[cfg(test)] +#[path = "unit_tests/transaction_tests.rs"] +pub mod transaction_tests; + #[cfg(test)] #[path = "unit_tests/batch_transaction_tests.rs"] mod batch_transaction_tests; diff --git a/crates/sui-core/src/authority/authority_per_epoch_store.rs b/crates/sui-core/src/authority/authority_per_epoch_store.rs index 77bdc976d7545..082af03663af7 100644 --- a/crates/sui-core/src/authority/authority_per_epoch_store.rs +++ b/crates/sui-core/src/authority/authority_per_epoch_store.rs @@ -2768,6 +2768,10 @@ impl AuthorityPerEpochStore { self.signature_verifier.insert_jwk(jwk_id, jwk); } } + + pub fn clear_signature_cache(&self) { + self.signature_verifier.clear_signature_cache(); + } } impl GetSharedLocks for AuthorityPerEpochStore { diff --git a/crates/sui-core/src/authority_server.rs b/crates/sui-core/src/authority_server.rs index 0bff8968c69ef..5ba8196421242 100644 --- a/crates/sui-core/src/authority_server.rs +++ b/crates/sui-core/src/authority_server.rs @@ -15,7 +15,6 @@ use sui_network::{ api::{Validator, ValidatorServer}, tonic, }; -use sui_types::effects::TransactionEffectsAPI; use sui_types::effects::TransactionEvents; use sui_types::messages_consensus::ConsensusTransaction; use sui_types::messages_grpc::{ @@ -25,6 +24,7 @@ use sui_types::messages_grpc::{ }; use sui_types::multiaddr::Multiaddr; use sui_types::sui_system_state::SuiSystemState; +use sui_types::{effects::TransactionEffectsAPI, message_envelope::Message}; use sui_types::{error::*, transaction::*}; use sui_types::{ fp_ensure, @@ -289,6 +289,9 @@ impl ValidatorService { } = self; let transaction = request.into_inner(); + + transaction.verify_user_input()?; + let epoch_store = state.load_epoch_store_one_call_per_task(); if !epoch_store.protocol_config().zklogin_auth() && transaction.has_zklogin_sig() { @@ -375,6 +378,7 @@ impl ValidatorService { let epoch_store = state.load_epoch_store_one_call_per_task(); let certificate = request.into_inner(); + certificate.verify_user_input()?; let shared_object_tx = certificate.contains_shared_object(); @@ -539,7 +543,9 @@ impl Validator for ValidatorService { &self, request: tonic::Request, ) -> Result, tonic::Status> { + request.get_ref().verify_user_input()?; let validator_service = self.clone(); + // Spawns a task which handles the certificate. The task will unconditionally continue // processing in the event that the client connection is dropped. spawn_monitored_task!(async move { @@ -561,6 +567,7 @@ impl Validator for ValidatorService { &self, request: tonic::Request, ) -> Result, tonic::Status> { + request.get_ref().verify_user_input()?; self.handle_certificate_v2(request) .await .map(|v| tonic::Response::new(v.into_inner().into())) diff --git a/crates/sui-core/src/signature_verifier.rs b/crates/sui-core/src/signature_verifier.rs index 57875bb5007bb..1d5167c691b21 100644 --- a/crates/sui-core/src/signature_verifier.rs +++ b/crates/sui-core/src/signature_verifier.rs @@ -17,7 +17,6 @@ use std::hash::Hash; use std::sync::Arc; use sui_types::digests::SenderSignedDataDigest; use sui_types::digests::ZKLoginInputsDigest; -use sui_types::signature::GenericSignature; use sui_types::transaction::SenderSignedData; use sui_types::{ committee::Committee, @@ -133,14 +132,17 @@ impl SignatureVerifier { committee, certificate_cache: VerifiedDigestCache::new( metrics.certificate_signatures_cache_hits.clone(), + metrics.certificate_signatures_cache_misses.clone(), metrics.certificate_signatures_cache_evictions.clone(), ), signed_data_cache: VerifiedDigestCache::new( metrics.signed_data_cache_hits.clone(), + metrics.signed_data_cache_misses.clone(), metrics.signed_data_cache_evictions.clone(), ), zklogin_inputs_cache: VerifiedDigestCache::new( metrics.zklogin_inputs_cache_hits.clone(), + metrics.zklogin_inputs_cache_misses.clone(), metrics.zklogin_inputs_cache_evictions.clone(), ), jwks: Default::default(), @@ -353,34 +355,28 @@ impl SignatureVerifier { self.zk_login_params.env.clone(), self.zk_login_params.verify_legacy_zklogin_address, ); - signed_tx - .tx_signatures() - .iter() - .try_for_each(|sig| match sig { - // If a zkLogin signature is found, check cache first. If it is in - // the cache, just verifies for uncached checks, otherwise verifies - // the entire zkLogin signature. - GenericSignature::ZkLoginAuthenticator(z) => { - self.zklogin_inputs_cache.is_verified( - z.hash_inputs(), - || signed_tx.verify_message_signature(&verify_params), - || signed_tx.verify_uncached_checks(&verify_params), - ) - } - _ => signed_tx.verify_message_signature(&verify_params), - }) + signed_tx.verify_message_signature(&verify_params) }, || Ok(()), ) } + + pub fn clear_signature_cache(&self) { + self.certificate_cache.clear(); + self.signed_data_cache.clear(); + self.zklogin_inputs_cache.clear(); + } } pub struct SignatureVerifierMetrics { pub certificate_signatures_cache_hits: IntCounter, + pub certificate_signatures_cache_misses: IntCounter, pub certificate_signatures_cache_evictions: IntCounter, pub signed_data_cache_hits: IntCounter, + pub signed_data_cache_misses: IntCounter, pub signed_data_cache_evictions: IntCounter, pub zklogin_inputs_cache_hits: IntCounter, + pub zklogin_inputs_cache_misses: IntCounter, pub zklogin_inputs_cache_evictions: IntCounter, timeouts: IntCounter, full_batches: IntCounter, @@ -398,6 +394,12 @@ impl SignatureVerifierMetrics { registry ) .unwrap(), + certificate_signatures_cache_misses: register_int_counter_with_registry!( + "certificate_signatures_cache_misses", + "Number of certificates which missed the signature cache", + registry + ) + .unwrap(), certificate_signatures_cache_evictions: register_int_counter_with_registry!( "certificate_signatures_cache_evictions", "Number of times we evict a pre-existing key were known to be verified because of signature cache.", @@ -409,7 +411,13 @@ impl SignatureVerifierMetrics { "Number of signed data which were known to be verified because of signature cache.", registry ) - .unwrap(), + .unwrap(), + signed_data_cache_misses: register_int_counter_with_registry!( + "signed_data_cache_misses", + "Number of signed data which missed the signature cache.", + registry + ) + .unwrap(), signed_data_cache_evictions: register_int_counter_with_registry!( "signed_data_cache_evictions", "Number of times we evict a pre-existing signed data were known to be verified because of signature cache.", @@ -422,6 +430,12 @@ impl SignatureVerifierMetrics { registry ) .unwrap(), + zklogin_inputs_cache_misses: register_int_counter_with_registry!( + "zklogin_inputs_cache_misses", + "Number of zklogin signatures which missed the zklogin inputs cache.", + registry + ) + .unwrap(), zklogin_inputs_cache_evictions: register_int_counter_with_registry!( "zklogin_inputs_cache_evictions", "Number of times we evict a pre-existing zklogin inputs digest that was known to be verified because of zklogin inputs cache.", @@ -529,16 +543,22 @@ const VERIFIED_CERTIFICATE_CACHE_SIZE: usize = 20000; pub struct VerifiedDigestCache { inner: RwLock>, cache_hits_counter: IntCounter, + cache_misses_counter: IntCounter, cache_evictions_counter: IntCounter, } impl VerifiedDigestCache { - pub fn new(cache_hits_counter: IntCounter, cache_evictions_counter: IntCounter) -> Self { + pub fn new( + cache_hits_counter: IntCounter, + cache_misses_counter: IntCounter, + cache_evictions_counter: IntCounter, + ) -> Self { Self { inner: RwLock::new(LruCache::new( std::num::NonZeroUsize::new(VERIFIED_CERTIFICATE_CACHE_SIZE).unwrap(), )), cache_hits_counter, + cache_misses_counter, cache_evictions_counter, } } @@ -549,6 +569,7 @@ impl VerifiedDigestCache { self.cache_hits_counter.inc(); true } else { + self.cache_misses_counter.inc(); false } } @@ -587,4 +608,9 @@ impl VerifiedDigestCache { } Ok(()) } + + pub fn clear(&self) { + let mut inner = self.inner.write(); + inner.clear(); + } } diff --git a/crates/sui-core/src/unit_tests/authority_tests.rs b/crates/sui-core/src/unit_tests/authority_tests.rs index de5361e9fe9e0..9d02974bda8fc 100644 --- a/crates/sui-core/src/unit_tests/authority_tests.rs +++ b/crates/sui-core/src/unit_tests/authority_tests.rs @@ -1235,7 +1235,7 @@ async fn test_handle_transfer_transaction_bad_signature() { authority_state.clone(), consensus_address, ); - let metrics = server.metrics.clone(); + let _metrics = server.metrics.clone(); let server_handle = server.spawn_for_test().await.unwrap(); @@ -1258,7 +1258,9 @@ async fn test_handle_transfer_transaction_bad_signature() { .await .is_err()); - assert_eq!(metrics.signature_errors.get(), 1); + // This metric does not increment because of the early check for correct sender address in + // verify_user_input (transaction.rs) + // assert_eq!(metrics.signature_errors.get(), 1); let object = authority_state .get_object(&object_id) diff --git a/crates/sui-core/src/unit_tests/transaction_tests.rs b/crates/sui-core/src/unit_tests/transaction_tests.rs new file mode 100644 index 0000000000000..4a22829effb4f --- /dev/null +++ b/crates/sui-core/src/unit_tests/transaction_tests.rs @@ -0,0 +1,609 @@ +// Copyright (c) Mysten Labs, Inc. +// SPDX-License-Identifier: Apache-2.0 + +use fastcrypto::ed25519::Ed25519KeyPair; +use fastcrypto::traits::KeyPair; +use fastcrypto_zkp::bn254::zk_login::{parse_jwks, OIDCProvider, ZkLoginInputs}; +use rand::{rngs::StdRng, SeedableRng}; +use sui_types::{ + authenticator_state::ActiveJwk, + base_types::dbg_addr, + crypto::AccountKeyPair, + crypto::{get_key_pair, Signature}, + signature::GenericSignature, + transaction::{AuthenticatorStateUpdate, TransactionDataAPI, TransactionExpiration}, + utils::to_sender_signed_transaction, + zk_login_authenticator::ZkLoginAuthenticator, +}; + +use crate::{ + authority_client::{AuthorityAPI, NetworkAuthorityClient}, + authority_server::{AuthorityServer, AuthorityServerHandle}, + stake_aggregator::{InsertResult, StakeAggregator}, +}; + +use super::*; + +pub use crate::authority::authority_test_utils::init_state_with_ids; + +#[tokio::test] +async fn test_handle_transfer_transaction_bad_signature() { + do_transaction_test( + 1, + |_| {}, + |mut_tx| { + let (_unknown_address, unknown_key): (_, AccountKeyPair) = get_key_pair(); + let data = mut_tx.data_mut_for_testing(); + *data.tx_signatures_mut_for_testing() = + vec![Signature::new_secure(data.intent_message(), &unknown_key).into()]; + }, + ) + .await; +} + +#[tokio::test] +async fn test_handle_transfer_transaction_no_signature() { + do_transaction_test( + 1, + |_| {}, + |tx| { + *tx.data_mut_for_testing().tx_signatures_mut_for_testing() = vec![]; + }, + ) + .await; +} + +#[tokio::test] +async fn test_handle_transfer_transaction_extra_signature() { + do_transaction_test( + 1, + |_| {}, + |tx| { + let sigs = tx.data_mut_for_testing().tx_signatures_mut_for_testing(); + sigs.push(sigs[0].clone()); + }, + ) + .await; +} + +// TODO: verify that these cases are not exploitable via consensus input +#[tokio::test] +async fn test_empty_sender_signed_data() { + do_transaction_test( + 0, + |_| {}, + |tx| { + let data = tx.data_mut_for_testing(); + *data.inner_vec_mut_for_testing() = vec![]; + }, + ) + .await; +} + +#[tokio::test] +async fn test_multiple_sender_signed_data() { + do_transaction_test( + 0, + |_| {}, + |tx| { + let data = tx.data_mut_for_testing(); + let tx_vec = data.inner_vec_mut_for_testing(); + assert_eq!(tx_vec.len(), 1); + let mut new = tx_vec[0].clone(); + // make sure second message has unique digest + *new.intent_message.value.expiration_mut_for_testing() = + TransactionExpiration::Epoch(123); + tx_vec.push(new); + }, + ) + .await; +} + +#[tokio::test] +async fn test_duplicate_sender_signed_data() { + do_transaction_test( + 0, + |_| {}, + |tx| { + let data = tx.data_mut_for_testing(); + let tx_vec = data.inner_vec_mut_for_testing(); + assert_eq!(tx_vec.len(), 1); + let new = tx_vec[0].clone(); + tx_vec.push(new); + }, + ) + .await; +} + +#[tokio::test] +async fn test_empty_gas_data() { + do_transaction_test_skip_cert_checks( + 0, + |tx| { + tx.gas_data_mut().payment = vec![]; + }, + |_| {}, + ) + .await; +} + +#[tokio::test] +async fn test_duplicate_gas_data() { + do_transaction_test_skip_cert_checks( + 0, + |tx| { + let gas_data = tx.gas_data_mut(); + let new_gas = gas_data.payment[0]; + gas_data.payment.push(new_gas); + }, + |_| {}, + ) + .await; +} + +#[tokio::test] +async fn test_gas_wrong_owner_matches_sender() { + do_transaction_test( + 1, + |tx| { + let gas_data = tx.gas_data_mut(); + let (new_addr, _): (_, AccountKeyPair) = get_key_pair(); + gas_data.owner = new_addr; + *tx.sender_mut_for_testing() = new_addr; + }, + |_| {}, + ) + .await; +} + +#[tokio::test] +async fn test_gas_wrong_owner() { + do_transaction_test( + 1, + |tx| { + let gas_data = tx.gas_data_mut(); + let (new_addr, _): (_, AccountKeyPair) = get_key_pair(); + gas_data.owner = new_addr; + }, + |_| {}, + ) + .await; +} + +pub fn init_transfer_transaction( + pre_sign_mutations: impl FnOnce(&mut TransactionData), + sender: SuiAddress, + secret: &AccountKeyPair, + recipient: SuiAddress, + object_ref: ObjectRef, + gas_object_ref: ObjectRef, + gas_budget: u64, + gas_price: u64, +) -> Transaction { + let mut data = TransactionData::new_transfer( + recipient, + object_ref, + sender, + gas_object_ref, + gas_budget, + gas_price, + ); + pre_sign_mutations(&mut data); + to_sender_signed_transaction(data, secret) +} + +async fn do_transaction_test_skip_cert_checks( + expected_sig_errors: u64, + pre_sign_mutations: impl FnOnce(&mut TransactionData), + post_sign_mutations: impl FnOnce(&mut Transaction), +) { + do_transaction_test_impl( + expected_sig_errors, + false, + pre_sign_mutations, + post_sign_mutations, + ) + .await +} + +async fn do_transaction_test( + expected_sig_errors: u64, + pre_sign_mutations: impl FnOnce(&mut TransactionData), + post_sign_mutations: impl FnOnce(&mut Transaction), +) { + do_transaction_test_impl( + expected_sig_errors, + true, + pre_sign_mutations, + post_sign_mutations, + ) + .await +} + +async fn do_transaction_test_impl( + _expected_sig_errors: u64, + check_forged_cert: bool, + pre_sign_mutations: impl FnOnce(&mut TransactionData), + post_sign_mutations: impl FnOnce(&mut Transaction), +) { + telemetry_subscribers::init_for_testing(); + let (sender, sender_key): (_, AccountKeyPair) = get_key_pair(); + let recipient = dbg_addr(2); + let object_id = ObjectID::random(); + let gas_object_id = ObjectID::random(); + let authority_state = + init_state_with_ids(vec![(sender, object_id), (sender, gas_object_id)]).await; + let rgp = authority_state.reference_gas_price_for_testing().unwrap(); + let object = authority_state + .get_object(&object_id) + .await + .unwrap() + .unwrap(); + let gas_object = authority_state + .get_object(&gas_object_id) + .await + .unwrap() + .unwrap(); + + let mut transfer_transaction = init_transfer_transaction( + pre_sign_mutations, + sender, + &sender_key, + recipient, + object.compute_object_reference(), + gas_object.compute_object_reference(), + rgp * TEST_ONLY_GAS_UNIT_FOR_TRANSFER, + rgp, + ); + + let consensus_address = "/ip4/127.0.0.1/tcp/0/http".parse().unwrap(); + + let server = AuthorityServer::new_for_test( + "/ip4/127.0.0.1/tcp/0/http".parse().unwrap(), + authority_state.clone(), + consensus_address, + ); + + let server_handle = server.spawn_for_test().await.unwrap(); + + let client = NetworkAuthorityClient::connect(server_handle.address()) + .await + .unwrap(); + + post_sign_mutations(&mut transfer_transaction); + + assert!(client + .handle_transaction(transfer_transaction.clone()) + .await + .is_err()); + + check_locks(authority_state.clone(), vec![object_id]).await; + + // now verify that the same transaction is rejected if a false certificate is somehow formed and sent + if check_forged_cert { + let epoch_store = authority_state.epoch_store_for_testing(); + let signed_transaction = VerifiedSignedTransaction::new( + epoch_store.epoch(), + VerifiedTransaction::new_unchecked(transfer_transaction), + authority_state.name, + &*authority_state.secret, + ); + let mut agg = StakeAggregator::new(epoch_store.committee().clone()); + + let InsertResult::QuorumReached(cert_sig) = agg.insert(signed_transaction.clone().into()) + else { + panic!("quorum expected"); + }; + + let plain_tx = signed_transaction.into_inner(); + + let ct = CertifiedTransaction::new_from_data_and_sig(plain_tx.into_data(), cert_sig); + + assert!(client.handle_certificate(ct.clone()).await.is_err()); + epoch_store.clear_signature_cache(); + assert!(client.handle_certificate(ct.clone()).await.is_err()); + } +} + +#[tokio::test] +async fn test_zklogin_transfer_with_bad_ephemeral_sig() { + do_zklogin_transaction_test( + 1, + |_| {}, + |tx| { + let data = tx.data_mut_for_testing(); + let intent_message = data.intent_message().clone(); + let sigs = data.tx_signatures_mut_for_testing(); + let GenericSignature::ZkLoginAuthenticator(zklogin) = sigs.get_mut(0).unwrap() else { + panic!(); + }; + + let (_unknown_address, unknown_key): (_, AccountKeyPair) = get_key_pair(); + let sig = Signature::new_secure(&intent_message, &unknown_key); + *zklogin.user_signature_mut_for_testing() = sig; + }, + ) + .await; +} + +fn zklogin_key_pair_and_inputs() -> Vec<(Ed25519KeyPair, ZkLoginInputs)> { + let key1 = Ed25519KeyPair::generate(&mut StdRng::from_seed([1; 32])); + let key2 = Ed25519KeyPair::generate(&mut StdRng::from_seed([2; 32])); + + let inputs1 = ZkLoginInputs::from_json("{\"proofPoints\":{\"a\":[\"7351610957585487046328875967050889651854514987235893782501043846344306437586\",\"15901581830174345085102528605366245320934422564305327249129736514949843983391\",\"1\"],\"b\":[[\"8511334686125322419369086121569737536249817670014553268281989325333085952301\",\"4879445774811020644521006463993914729416121646921376735430388611804034116132\"],[\"17435652898871739253945717312312680537810513841582909477368887889905134847157\",\"14885460127400879557124294989610467103783286587437961743305395373299049315863\"],[\"1\",\"0\"]],\"c\":[\"18935582624804960299209074901817240117999581542763303721451852621662183299378\",\"5367019427921492326304024952457820199970536888356564030410757345854117465786\",\"1\"]},\"issBase64Details\":{\"value\":\"wiaXNzIjoiaHR0cHM6Ly9pZC50d2l0Y2gudHYvb2F1dGgyIiw\",\"indexMod4\":2},\"headerBase64\":\"eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6IjEifQ\"}", "20794788559620669596206457022966176986688727876128223628113916380927502737911").unwrap(); + let inputs2 = ZkLoginInputs::from_json("{\"proofPoints\":{\"a\":[\"7351610957585487046328875967050889651854514987235893782501043846344306437586\",\"15901581830174345085102528605366245320934422564305327249129736514949843983391\",\"1\"],\"b\":[[\"8511334686125322419369086121569737536249817670014553268281989325333085952301\",\"4879445774811020644521006463993914729416121646921376735430388611804034116132\"],[\"17435652898871739253945717312312680537810513841582909477368887889905134847157\",\"14885460127400879557124294989610467103783286587437961743305395373299049315863\"],[\"1\",\"0\"]],\"c\":[\"18935582624804960299209074901817240117999581542763303721451852621662183299378\",\"5367019427921492326304024952457820199970536888356564030410757345854117465786\",\"1\"]},\"issBase64Details\":{\"value\":\"wiaXNzIjoiaHR0cHM6Ly9pZC50d2l0Y2gudHYvb2F1dGgyIiw\",\"indexMod4\":2},\"headerBase64\":\"eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6IjEifQ\"}", "20794788559620669596206457022966176986688727876128223628113916380927502737911").unwrap(); + + vec![(key1, inputs1), (key2, inputs2)] +} + +#[tokio::test] +async fn zklogin_test_cached_proof_wrong_key() { + telemetry_subscribers::init_for_testing(); + let ( + mut object_ids, + gas_object_ids, + authority_state, + _epoch_store, + transfer_transaction, + metrics, + _server, + client, + ) = setup_zklogin_network(|_| {}).await; + + assert!(client + .handle_transaction(transfer_transaction) + .await + .is_ok()); + + /* + assert_eq!( + epoch_store + .signature_verifier + .metrics + .zklogin_inputs_cache_misses + .get(), + 1 + ); + */ + + let (ephemeral_key, zklogin) = &zklogin_key_pair_and_inputs()[0]; + let sender: SuiAddress = zklogin.try_into().unwrap(); + let recipient = dbg_addr(2); + + let mut transfer_transaction2 = init_zklogin_transfer( + &authority_state, + object_ids[2], + gas_object_ids[2], + recipient, + sender, + |_| {}, + ephemeral_key, + zklogin, + ) + .await; + + let intent_message = transfer_transaction2.data().intent_message().clone(); + match &mut transfer_transaction2 + .data_mut_for_testing() + .tx_signatures_mut_for_testing()[0] + { + GenericSignature::ZkLoginAuthenticator(zklogin) => { + let (_unknown_address, unknown_key): (_, AccountKeyPair) = get_key_pair(); + // replace the signature with a bogus one + *zklogin.user_signature_mut_for_testing() = + Signature::new_secure(&intent_message, &unknown_key); + } + _ => panic!(), + } + + // This tx should fail, but passes because we skip the ephemeral sig check when hitting the zklogin check! + assert!(client + .handle_transaction(transfer_transaction2) + .await + .is_err()); + + // TODO: re-enable when cache is re-enabled. + /* + assert_eq!( + epoch_store + .signature_verifier + .metrics + .zklogin_inputs_cache_hits + .get(), + 1 + ); + */ + + assert_eq!(metrics.signature_errors.get(), 1); + + object_ids.remove(0); // first object was successfully locked. + check_locks(authority_state, object_ids).await; +} + +async fn do_zklogin_transaction_test( + expected_sig_errors: u64, + pre_sign_mutations: impl FnOnce(&mut TransactionData), + post_sign_mutations: impl FnOnce(&mut Transaction), +) { + let ( + object_ids, + _gas_object_id, + authority_state, + _epoch_store, + mut transfer_transaction, + metrics, + _server, + client, + ) = setup_zklogin_network(pre_sign_mutations).await; + + post_sign_mutations(&mut transfer_transaction); + + assert!(client + .handle_transaction(transfer_transaction) + .await + .is_err()); + + // TODO: re-enable when cache is re-enabled. + /* + assert_eq!( + epoch_store + .signature_verifier + .metrics + .zklogin_inputs_cache_misses + .get(), + 1 + ); + */ + + assert_eq!(metrics.signature_errors.get(), expected_sig_errors); + + check_locks(authority_state, object_ids).await; +} + +async fn check_locks(authority_state: Arc, object_ids: Vec) { + for object_id in object_ids { + let object = authority_state + .get_object(&object_id) + .await + .unwrap() + .unwrap(); + assert!(authority_state + .get_transaction_lock( + &object.compute_object_reference(), + &authority_state.epoch_store_for_testing() + ) + .await + .unwrap() + .is_none()); + } +} + +async fn setup_zklogin_network( + pre_sign_mutations: impl FnOnce(&mut TransactionData), +) -> ( + Vec, // objects + Vec, // gas objects + Arc, + Guard>, + sui_types::message_envelope::Envelope, + Arc, + AuthorityServerHandle, + NetworkAuthorityClient, +) { + let (ephemeral_key, zklogin) = &zklogin_key_pair_and_inputs()[0]; + + let sender: SuiAddress = zklogin.try_into().unwrap(); + + let recipient = dbg_addr(2); + let objects: Vec<_> = (0..10).map(|_| (sender, ObjectID::random())).collect(); + let gas_objects: Vec<_> = (0..10).map(|_| (sender, ObjectID::random())).collect(); + let object_ids: Vec<_> = objects.iter().map(|(_, id)| *id).collect(); + let gas_object_ids: Vec<_> = gas_objects.iter().map(|(_, id)| *id).collect(); + + let authority_state = + init_state_with_ids(objects.into_iter().chain(gas_objects).collect::>()).await; + + let object_id = object_ids[0]; + let gas_object_id = gas_object_ids[0]; + let jwks = "{\"keys\":[{\"alg\":\"RS256\",\"e\":\"AQAB\",\"kid\":\"1\",\"kty\":\"RSA\",\"n\":\"6lq9MQ-q6hcxr7kOUp-tHlHtdcDsVLwVIw13iXUCvuDOeCi0VSuxCCUY6UmMjy53dX00ih2E4Y4UvlrmmurK0eG26b-HMNNAvCGsVXHU3RcRhVoHDaOwHwU72j7bpHn9XbP3Q3jebX6KIfNbei2MiR0Wyb8RZHE-aZhRYO8_-k9G2GycTpvc-2GBsP8VHLUKKfAs2B6sW3q3ymU6M0L-cFXkZ9fHkn9ejs-sqZPhMJxtBPBxoUIUQFTgv4VXTSv914f_YkNw-EjuwbgwXMvpyr06EyfImxHoxsZkFYB-qBYHtaMxTnFsZBr6fn8Ha2JqT1hoP7Z5r5wxDu3GQhKkHw\",\"use\":\"sig\"}]}"; + let jwks = parse_jwks(jwks.as_bytes(), &OIDCProvider::Twitch).unwrap(); + let epoch_store = authority_state.epoch_store_for_testing(); + epoch_store.update_authenticator_state(&AuthenticatorStateUpdate { + epoch: 0, + round: 0, + new_active_jwks: jwks + .into_iter() + .map(|(jwk_id, jwk)| ActiveJwk { + jwk_id, + jwk, + epoch: 0, + }) + .collect(), + authenticator_obj_initial_shared_version: 1.into(), + }); + + let transfer_transaction = init_zklogin_transfer( + &authority_state, + object_id, + gas_object_id, + recipient, + sender, + pre_sign_mutations, + ephemeral_key, + zklogin, + ) + .await; + + let consensus_address = "/ip4/127.0.0.1/tcp/0/http".parse().unwrap(); + + let server = AuthorityServer::new_for_test( + "/ip4/127.0.0.1/tcp/0/http".parse().unwrap(), + authority_state.clone(), + consensus_address, + ); + let metrics = server.metrics.clone(); + + let server_handle = server.spawn_for_test().await.unwrap(); + + let client = NetworkAuthorityClient::connect(server_handle.address()) + .await + .unwrap(); + ( + object_ids, + gas_object_ids, + authority_state, + epoch_store, + transfer_transaction, + metrics, + server_handle, + client, + ) +} + +async fn init_zklogin_transfer( + authority_state: &Arc, + object_id: ObjectID, + gas_object_id: ObjectID, + recipient: SuiAddress, + sender: SuiAddress, + pre_sign_mutations: impl FnOnce(&mut TransactionData), + ephemeral_key: &Ed25519KeyPair, + zklogin: &ZkLoginInputs, +) -> sui_types::message_envelope::Envelope { + let rgp = authority_state.reference_gas_price_for_testing().unwrap(); + let object = authority_state + .get_object(&object_id) + .await + .unwrap() + .unwrap(); + let gas_object = authority_state + .get_object(&gas_object_id) + .await + .unwrap() + .unwrap(); + let object_ref = object.compute_object_reference(); + let gas_object_ref = gas_object.compute_object_reference(); + let gas_budget = rgp * TEST_ONLY_GAS_UNIT_FOR_TRANSFER; + let mut data = TransactionData::new_transfer( + recipient, + object_ref, + sender, + gas_object_ref, + gas_budget, + rgp, + ); + pre_sign_mutations(&mut data); + let mut tx = to_sender_signed_transaction(data, ephemeral_key); + let GenericSignature::Signature(signature) = + tx.data_mut_for_testing().tx_signatures_mut_for_testing()[0].clone() + else { + panic!(); + }; + let authenticator = GenericSignature::ZkLoginAuthenticator(ZkLoginAuthenticator::new( + zklogin.clone(), + 10, + signature, + )); + tx.data_mut_for_testing().tx_signatures_mut_for_testing()[0] = authenticator; + tx +} diff --git a/crates/sui-e2e-tests/tests/zklogin_tests.rs b/crates/sui-e2e-tests/tests/zklogin_tests.rs index f7067b02631a0..bd9681c5a2406 100644 --- a/crates/sui-e2e-tests/tests/zklogin_tests.rs +++ b/crates/sui-e2e-tests/tests/zklogin_tests.rs @@ -4,7 +4,9 @@ use sui_test_transaction_builder::TestTransactionBuilder; use sui_types::error::{SuiError, SuiResult}; -use sui_types::utils::{get_zklogin_user_address, make_zklogin_tx, sign_zklogin_tx}; +use sui_types::utils::{ + get_zklogin_user_address, make_zklogin_tx, sign_zklogin_tx_with_default_proof, +}; use sui_types::SUI_AUTHENTICATOR_STATE_OBJECT_ID; use test_cluster::{TestCluster, TestClusterBuilder}; @@ -129,7 +131,7 @@ async fn run_zklogin_end_to_end_test(mut test_cluster: TestCluster) { .transfer_sui(None, sender) .build(); - let (_, signed_txn, _) = sign_zklogin_tx(txn, false); + let (_, signed_txn, _) = sign_zklogin_tx_with_default_proof(txn, false); context.execute_transaction_must_succeed(signed_txn).await; diff --git a/crates/sui-types/src/base_types.rs b/crates/sui-types/src/base_types.rs index b4f511c8e2dc0..68822277fa012 100644 --- a/crates/sui-types/src/base_types.rs +++ b/crates/sui-types/src/base_types.rs @@ -46,6 +46,7 @@ use fastcrypto::encoding::{Encoding, Hex}; use fastcrypto::hash::HashFunction; use fastcrypto::traits::AllowedRng; use fastcrypto_zkp::bn254::utils::big_int_str_to_bytes; +use fastcrypto_zkp::bn254::zk_login::ZkLoginInputs; use move_binary_format::binary_views::BinaryIndexedView; use move_binary_format::file_format::SignatureToken; use move_bytecode_utils::resolve_struct; @@ -666,13 +667,22 @@ impl From<&MultiSigPublicKey> for SuiAddress { impl TryFrom<&ZkLoginAuthenticator> for SuiAddress { type Error = SuiError; fn try_from(authenticator: &ZkLoginAuthenticator) -> SuiResult { + TryFrom::try_from(&authenticator.inputs) + } +} + +/// Sui address for [struct ZkLoginAuthenticator] is defined as the black2b hash of +/// [zklogin_flag || iss_bytes_length || iss_bytes || address_seed in bytes]. +impl TryFrom<&ZkLoginInputs> for SuiAddress { + type Error = SuiError; + fn try_from(inputs: &ZkLoginInputs) -> SuiResult { let mut hasher = DefaultHash::default(); hasher.update([SignatureScheme::ZkLoginAuthenticator.flag()]); - let iss_bytes = authenticator.get_iss().as_bytes(); + let iss_bytes = inputs.get_iss().as_bytes(); hasher.update([iss_bytes.len() as u8]); hasher.update(iss_bytes); hasher.update( - big_int_str_to_bytes(authenticator.get_address_seed()) + big_int_str_to_bytes(inputs.get_address_seed()) .map_err(|_| SuiError::InvalidAddress)?, ); Ok(SuiAddress(hasher.finalize().digest)) diff --git a/crates/sui-types/src/effects/mod.rs b/crates/sui-types/src/effects/mod.rs index b0f42b20c0b68..e2457e476f095 100644 --- a/crates/sui-types/src/effects/mod.rs +++ b/crates/sui-types/src/effects/mod.rs @@ -95,6 +95,10 @@ impl Message for TransactionEffects { TransactionEffectsDigest::new(default_hash(self)) } + fn verify_user_input(&self) -> SuiResult { + Ok(()) + } + fn verify_epoch(&self, _: EpochId) -> SuiResult { // Authorities are allowed to re-sign effects from prior epochs, so we do not verify the // epoch here. diff --git a/crates/sui-types/src/error.rs b/crates/sui-types/src/error.rs index 1159c8ad8939b..e90bcdc4b65c2 100644 --- a/crates/sui-types/src/error.rs +++ b/crates/sui-types/src/error.rs @@ -331,7 +331,7 @@ pub enum SuiError { expected: String, actual: Vec, }, - #[error("Expect {actual} signer signatures but got {expected}.")] + #[error("Expect {expected} signer signatures but got {actual}.")] SignerSignatureNumberMismatch { expected: usize, actual: usize }, #[error("Value was not signed by the correct sender: {}", error)] IncorrectSigner { error: String }, diff --git a/crates/sui-types/src/message_envelope.rs b/crates/sui-types/src/message_envelope.rs index 63be718762449..f78579046193a 100644 --- a/crates/sui-types/src/message_envelope.rs +++ b/crates/sui-types/src/message_envelope.rs @@ -101,6 +101,9 @@ pub trait Message { fn digest(&self) -> Self::DigestType; + /// Perform cheap validity checks before any expensive crypto verification. + fn verify_user_input(&self) -> SuiResult; + /// Verify that the message is from the correct epoch (e.g. for CertifiedCheckpointSummary /// we verify that the checkpoint is from the same epoch as the committee signatures). fn verify_epoch(&self, epoch: EpochId) -> SuiResult; diff --git a/crates/sui-types/src/messages_checkpoint.rs b/crates/sui-types/src/messages_checkpoint.rs index 72b55ae087ae5..3856c6dc184fc 100644 --- a/crates/sui-types/src/messages_checkpoint.rs +++ b/crates/sui-types/src/messages_checkpoint.rs @@ -162,6 +162,10 @@ impl Message for CheckpointSummary { CheckpointDigest::new(default_hash(self)) } + fn verify_user_input(&self) -> SuiResult { + Ok(()) + } + fn verify_epoch(&self, epoch: EpochId) -> SuiResult { fp_ensure!( self.epoch == epoch, diff --git a/crates/sui-types/src/transaction.rs b/crates/sui-types/src/transaction.rs index 9fe2abffd56a6..3ce8037cc98d7 100644 --- a/crates/sui-types/src/transaction.rs +++ b/crates/sui-types/src/transaction.rs @@ -1768,10 +1768,8 @@ pub trait TransactionDataAPI { /// Check if the transaction is sponsored (namely gas owner != sender) fn is_sponsored_tx(&self) -> bool; - #[cfg(test)] - fn sender_mut(&mut self) -> &mut SuiAddress; + fn sender_mut_for_testing(&mut self) -> &mut SuiAddress; - #[cfg(test)] fn gas_data_mut(&mut self) -> &mut GasData; // This should be used in testing only. @@ -1908,12 +1906,10 @@ impl TransactionDataAPI for TransactionDataV1 { matches!(self.kind, TransactionKind::Genesis(_)) } - #[cfg(test)] - fn sender_mut(&mut self) -> &mut SuiAddress { + fn sender_mut_for_testing(&mut self) -> &mut SuiAddress { &mut self.sender } - #[cfg(test)] fn gas_data_mut(&mut self) -> &mut GasData { &mut self.gas_data } @@ -1960,6 +1956,10 @@ impl SenderSignedData { }]) } + pub fn inner_vec_mut_for_testing(&mut self) -> &mut Vec { + &mut self.0 + } + pub fn inner(&self) -> &SenderSignedTransaction { // assert is safe - SenderSignedTransaction::verify ensures length is 1. assert_eq!(self.0.len(), 1); @@ -2081,6 +2081,51 @@ impl Message for SenderSignedData { TransactionDigest::new(default_hash(&self.intent_message().value)) } + fn verify_user_input(&self) -> SuiResult { + fp_ensure!( + self.0.len() == 1, + SuiError::UserInputError { + error: UserInputError::Unsupported( + "SenderSignedData must contain exactly one transaction".to_string() + ) + } + ); + let tx_data = &self.intent_message().value; + fp_ensure!( + !tx_data.is_system_tx(), + SuiError::UserInputError { + error: UserInputError::Unsupported( + "SenderSignedData must not contain system transaction".to_string() + ) + } + ); + + // Verify signatures are well formed. Steps are ordered in asc complexity order + // to minimize abuse. + let signers = self.intent_message().value.signers(); + // Signature number needs to match + fp_ensure!( + self.inner().tx_signatures.len() == signers.len(), + SuiError::SignerSignatureNumberMismatch { + actual: self.inner().tx_signatures.len(), + expected: signers.len() + } + ); + + // All required signers need to be sign. + let present_sigs = self.get_signer_sig_mapping(true)?; + for s in signers { + if !present_sigs.contains_key(&s) { + return Err(SuiError::SignerSignatureAbsent { + expected: s.to_string(), + actual: present_sigs.keys().map(|s| s.to_string()).collect(), + }); + } + } + + Ok(()) + } + fn verify_epoch(&self, epoch: EpochId) -> SuiResult { for sig in &self.inner().tx_signatures { sig.verify_user_authenticator_epoch(epoch)?; @@ -2100,6 +2145,7 @@ impl AuthenticatedMessage for SenderSignedData { } Ok(()) } + fn verify_message_signature(&self, verify_params: &VerifyParams) -> SuiResult { fp_ensure!( self.0.len() == 1, diff --git a/crates/sui-types/src/unit_tests/messages_tests.rs b/crates/sui-types/src/unit_tests/messages_tests.rs index 5128a00b71afd..5071e1462b72e 100644 --- a/crates/sui-types/src/unit_tests/messages_tests.rs +++ b/crates/sui-types/src/unit_tests/messages_tests.rs @@ -937,13 +937,13 @@ fn verify_sender_signature_correctly_with_flag() { // create a sender keypair with Ed25519 let sender_kp_2 = SuiKeyPair::Ed25519(get_key_pair().1); let mut tx_data_2 = tx_data.clone(); - *tx_data_2.sender_mut() = (&sender_kp_2.public()).into(); + *tx_data_2.sender_mut_for_testing() = (&sender_kp_2.public()).into(); tx_data_2.gas_data_mut().owner = tx_data_2.sender(); // create a sender keypair with Secp256r1 let sender_kp_3 = SuiKeyPair::Secp256r1(get_key_pair().1); let mut tx_data_3 = tx_data.clone(); - *tx_data_3.sender_mut() = (&sender_kp_3.public()).into(); + *tx_data_3.sender_mut_for_testing() = (&sender_kp_3.public()).into(); tx_data_3.gas_data_mut().owner = tx_data_3.sender(); let transaction = diff --git a/crates/sui-types/src/unit_tests/utils.rs b/crates/sui-types/src/unit_tests/utils.rs index c2fddb8a758ce..9f8f459571161 100644 --- a/crates/sui-types/src/unit_tests/utils.rs +++ b/crates/sui-types/src/unit_tests/utils.rs @@ -215,7 +215,7 @@ mod zk_login { } else { make_transaction_data(get_zklogin_user_address()) }; - sign_zklogin_tx(data, legacy) + sign_zklogin_tx_with_default_proof(data, legacy) } pub fn sign_zklogin_personal_msg(data: PersonalMessage) -> (SuiAddress, GenericSignature) { @@ -228,15 +228,28 @@ mod zk_login { (address, authenticator) } - pub fn sign_zklogin_tx( + pub fn sign_zklogin_tx_with_default_proof( data: TransactionData, legacy: bool, ) -> (SuiAddress, Transaction, GenericSignature) { - // Sign the user transaction with the user's ephemeral key. + let inputs = if legacy { + get_inputs_with_bad_address_seed() + } else { + get_zklogin_inputs() + }; + + sign_zklogin_tx(&get_zklogin_user_key(), inputs, data) + } + + pub fn sign_zklogin_tx( + user_key: &SuiKeyPair, + proof: ZkLoginInputs, + data: TransactionData, + ) -> (SuiAddress, Transaction, GenericSignature) { let tx = Transaction::from_data_and_signer( - data, + data.clone(), Intent::sui_transaction(), - vec![&get_zklogin_user_key()], + vec![user_key], ); let s = match tx.inner().tx_signatures.first().unwrap() { @@ -244,29 +257,16 @@ mod zk_login { _ => panic!("Expected a signature"), }; - let inputs = if legacy { - get_inputs_with_bad_address_seed() - } else { - get_zklogin_inputs() - }; // Construct the authenticator with all user submitted components. - let authenticator = GenericSignature::ZkLoginAuthenticator(ZkLoginAuthenticator::new( - inputs, - 10, - s.clone(), - )); + let authenticator = + GenericSignature::ZkLoginAuthenticator(ZkLoginAuthenticator::new(proof, 10, s.clone())); let tx = Transaction::new(SenderSignedData::new( tx.transaction_data().clone(), Intent::sui_transaction(), vec![authenticator.clone()], )); - let addr = if legacy { - get_legacy_zklogin_user_address() - } else { - get_zklogin_user_address() - }; - (addr, tx, authenticator) + (data.execution_parts().1, tx, authenticator) } } diff --git a/crates/sui-types/src/zk_login_authenticator.rs b/crates/sui-types/src/zk_login_authenticator.rs index 22ea8e92f93bb..bc83ea19678fa 100644 --- a/crates/sui-types/src/zk_login_authenticator.rs +++ b/crates/sui-types/src/zk_login_authenticator.rs @@ -26,7 +26,7 @@ mod zk_login_authenticator_test; #[derive(Debug, Clone, JsonSchema, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct ZkLoginAuthenticator { - inputs: ZkLoginInputs, + pub inputs: ZkLoginInputs, max_epoch: EpochId, user_signature: Signature, #[serde(skip)] @@ -62,6 +62,10 @@ impl ZkLoginAuthenticator { pub fn get_iss(&self) -> &str { self.inputs.get_iss() } + + pub fn user_signature_mut_for_testing(&mut self) -> &mut Signature { + &mut self.user_signature + } } /// Necessary trait for [struct SenderSignedData].