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

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(&registry);
     let cache = VerifiedDigestCache::<CertificateDigest>::new(
         metrics.certificate_signatures_cache_hits.clone(),
+        metrics.certificate_signatures_cache_misses.clone(),
         metrics.certificate_signatures_cache_evictions.clone(),
     );
 

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;

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 {

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<CertifiedTransaction>,
     ) -> Result<tonic::Response<HandleCertificateResponseV2>, 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<CertifiedTransaction>,
     ) -> Result<tonic::Response<HandleCertificateResponse>, tonic::Status> {
+        request.get_ref().verify_user_input()?;
         self.handle_certificate_v2(request)
             .await
             .map(|v| tonic::Response::new(v.into_inner().into()))

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<D> {
     inner: RwLock<LruCache<D, ()>>,
     cache_hits_counter: IntCounter,
+    cache_misses_counter: IntCounter,
     cache_evictions_counter: IntCounter,
 }
 
 impl<D: Hash + Eq + Copy> VerifiedDigestCache<D> {
-    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<D: Hash + Eq + Copy> VerifiedDigestCache<D> {
             self.cache_hits_counter.inc();
             true
         } else {
+            self.cache_misses_counter.inc();
             false
         }
     }
@@ -587,4 +608,9 @@ impl<D: Hash + Eq + Copy> VerifiedDigestCache<D> {
         }
         Ok(())
     }
+
+    pub fn clear(&self) {
+        let mut inner = self.inner.write();
+        inner.clear();
+    }
 }

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)