[ed25519] Use verify_strict for signature verification in ed25519 precompile (anza-xyz#1876)

samkim-crypto · 0x0ece · web-flow · commit ddda6c30a323 · 2024-07-10T09:19:52.000+09:00
* use `verify_strict` for signature verification in ed25519 precompile

* add test

* clippy

* increase ed25519 precompile cost constant by 5%

* put ed25519 strict verification cost change under feature gate

---------

Co-authored-by: Emanuele Cesena &lt;ecesena@jumptrading.com&gt;
diff --git a/cost-model/src/block_cost_limits.rs b/cost-model/src/block_cost_limits.rs
@@ -28,6 +28,8 @@ pub const SIGNATURE_COST: u64 = COMPUTE_UNIT_TO_US_RATIO * 24;
 pub const SECP256K1_VERIFY_COST: u64 = COMPUTE_UNIT_TO_US_RATIO * 223;
 /// Number of compute units for one ed25519 signature verification.
 pub const ED25519_VERIFY_COST: u64 = COMPUTE_UNIT_TO_US_RATIO * 76;
+/// Number of compute units for one ed25519 strict signature verification.
+pub const ED25519_VERIFY_STRICT_COST: u64 = COMPUTE_UNIT_TO_US_RATIO * 80;
 /// Number of compute units for one write lock
 pub const WRITE_LOCK_UNITS: u64 = COMPUTE_UNIT_TO_US_RATIO * 10;
 /// Number of data bytes per compute units
diff --git a/cost-model/src/cost_model.rs b/cost-model/src/cost_model.rs
@@ -40,7 +40,7 @@ impl CostModel {
         } else {
             let mut tx_cost = UsageCostDetails::new_with_default_capacity();
 
-            Self::get_signature_cost(&mut tx_cost, transaction);
+            Self::get_signature_cost(&mut tx_cost, transaction, feature_set);
             Self::get_write_lock_cost(&mut tx_cost, transaction, feature_set);
             Self::get_transaction_cost(&mut tx_cost, transaction, feature_set);
             tx_cost.allocated_accounts_data_size =
@@ -66,7 +66,7 @@ impl CostModel {
         } else {
             let mut tx_cost = UsageCostDetails::new_with_default_capacity();
 
-            Self::get_signature_cost(&mut tx_cost, transaction);
+            Self::get_signature_cost(&mut tx_cost, transaction, feature_set);
             Self::get_write_lock_cost(&mut tx_cost, transaction, feature_set);
             Self::get_instructions_data_cost(&mut tx_cost, transaction);
             tx_cost.allocated_accounts_data_size =
@@ -82,13 +82,25 @@ impl CostModel {
         }
     }
 
-    fn get_signature_cost(tx_cost: &mut UsageCostDetails, transaction: &SanitizedTransaction) {
+    fn get_signature_cost(
+        tx_cost: &mut UsageCostDetails,
+        transaction: &SanitizedTransaction,
+        feature_set: &FeatureSet,
+    ) {
         let signatures_count_detail = transaction.message().get_signature_details();
         tx_cost.num_transaction_signatures = signatures_count_detail.num_transaction_signatures();
         tx_cost.num_secp256k1_instruction_signatures =
             signatures_count_detail.num_secp256k1_instruction_signatures();
         tx_cost.num_ed25519_instruction_signatures =
             signatures_count_detail.num_ed25519_instruction_signatures();
+
+        let ed25519_verify_cost =
+            if feature_set.is_active(&feature_set::ed25519_precompile_verify_strict::id()) {
+                ED25519_VERIFY_STRICT_COST
+            } else {
+                ED25519_VERIFY_COST
+            };
+
         tx_cost.signature_cost = signatures_count_detail
             .num_transaction_signatures()
             .saturating_mul(SIGNATURE_COST)
@@ -100,7 +112,7 @@ impl CostModel {
             .saturating_add(
                 signatures_count_detail
                     .num_ed25519_instruction_signatures()
-                    .saturating_mul(ED25519_VERIFY_COST),
+                    .saturating_mul(ed25519_verify_cost),
             );
     }
 
diff --git a/sdk/src/ed25519_instruction.rs b/sdk/src/ed25519_instruction.rs
@@ -5,7 +5,11 @@
 #![cfg(feature = "full")]
 
 use {
-    crate::{feature_set::FeatureSet, instruction::Instruction, precompiles::PrecompileError},
+    crate::{
+        feature_set::{ed25519_precompile_verify_strict, FeatureSet},
+        instruction::Instruction,
+        precompiles::PrecompileError,
+    },
     bytemuck::bytes_of,
     bytemuck_derive::{Pod, Zeroable},
     ed25519_dalek::{ed25519::signature::Signature, Signer, Verifier},
@@ -86,7 +90,7 @@ pub fn new_ed25519_instruction(keypair: &ed25519_dalek::Keypair, message: &[u8])
 pub fn verify(
     data: &[u8],
     instruction_datas: &[&[u8]],
-    _feature_set: &FeatureSet,
+    feature_set: &FeatureSet,
 ) -> Result<(), PrecompileError> {
     if data.len() < SIGNATURE_OFFSETS_START {
         return Err(PrecompileError::InvalidInstructionDataSize);
@@ -145,9 +149,15 @@ pub fn verify(
             offsets.message_data_size as usize,
         )?;
 
-        publickey
-            .verify(message, &signature)
-            .map_err(|_| PrecompileError::InvalidSignature)?;
+        if feature_set.is_active(&ed25519_precompile_verify_strict::id()) {
+            publickey
+                .verify_strict(message, &signature)
+                .map_err(|_| PrecompileError::InvalidSignature)?;
+        } else {
+            publickey
+                .verify(message, &signature)
+                .map_err(|_| PrecompileError::InvalidSignature)?;
+        }
     }
     Ok(())
 }
@@ -189,9 +199,64 @@ pub mod test {
             signature::{Keypair, Signer},
             transaction::Transaction,
         },
+        hex,
         rand0_7::{thread_rng, Rng},
     };
 
+    pub fn new_ed25519_instruction_raw(
+        pubkey: &[u8],
+        signature: &[u8],
+        message: &[u8],
+    ) -> Instruction {
+        assert_eq!(pubkey.len(), PUBKEY_SERIALIZED_SIZE);
+        assert_eq!(signature.len(), SIGNATURE_SERIALIZED_SIZE);
+
+        let mut instruction_data = Vec::with_capacity(
+            DATA_START
+                .saturating_add(SIGNATURE_SERIALIZED_SIZE)
+                .saturating_add(PUBKEY_SERIALIZED_SIZE)
+                .saturating_add(message.len()),
+        );
+
+        let num_signatures: u8 = 1;
+        let public_key_offset = DATA_START;
+        let signature_offset = public_key_offset.saturating_add(PUBKEY_SERIALIZED_SIZE);
+        let message_data_offset = signature_offset.saturating_add(SIGNATURE_SERIALIZED_SIZE);
+
+        // add padding byte so that offset structure is aligned
+        instruction_data.extend_from_slice(bytes_of(&[num_signatures, 0]));
+
+        let offsets = Ed25519SignatureOffsets {
+            signature_offset: signature_offset as u16,
+            signature_instruction_index: u16::MAX,
+            public_key_offset: public_key_offset as u16,
+            public_key_instruction_index: u16::MAX,
+            message_data_offset: message_data_offset as u16,
+            message_data_size: message.len() as u16,
+            message_instruction_index: u16::MAX,
+        };
+
+        instruction_data.extend_from_slice(bytes_of(&offsets));
+
+        debug_assert_eq!(instruction_data.len(), public_key_offset);
+
+        instruction_data.extend_from_slice(pubkey);
+
+        debug_assert_eq!(instruction_data.len(), signature_offset);
+
+        instruction_data.extend_from_slice(signature);
+
+        debug_assert_eq!(instruction_data.len(), message_data_offset);
+
+        instruction_data.extend_from_slice(message);
+
+        Instruction {
+            program_id: solana_sdk::ed25519_program::id(),
+            accounts: vec![],
+            data: instruction_data,
+        }
+    }
+
     fn test_case(
         num_signatures: u16,
         offsets: &Ed25519SignatureOffsets,
@@ -380,4 +445,50 @@ pub mod test {
         );
         assert!(tx.verify_precompiles(&feature_set).is_err());
     }
+
+    #[test]
+    fn test_ed25519_malleability() {
+        solana_logger::setup();
+        let mint_keypair = Keypair::new();
+
+        // sig created via ed25519_dalek: both pass
+        let privkey = ed25519_dalek::Keypair::generate(&mut thread_rng());
+        let message_arr = b"hello";
+        let instruction = new_ed25519_instruction(&privkey, message_arr);
+        let tx = Transaction::new_signed_with_payer(
+            &[instruction.clone()],
+            Some(&mint_keypair.pubkey()),
+            &[&mint_keypair],
+            Hash::default(),
+        );
+
+        let feature_set = FeatureSet::default();
+        assert!(tx.verify_precompiles(&feature_set).is_ok());
+
+        let feature_set = FeatureSet::all_enabled();
+        assert!(tx.verify_precompiles(&feature_set).is_ok());
+
+        // malleable sig: verify_strict does NOT pass
+        // for example, test number 5:
+        // https://github.com/C2SP/CCTV/tree/main/ed25519
+        // R has low order (in fact R == 0)
+        let pubkey =
+            &hex::decode("10eb7c3acfb2bed3e0d6ab89bf5a3d6afddd1176ce4812e38d9fd485058fdb1f")
+                .unwrap();
+        let signature = &hex::decode("00000000000000000000000000000000000000000000000000000000000000009472a69cd9a701a50d130ed52189e2455b23767db52cacb8716fb896ffeeac09").unwrap();
+        let message = b"ed25519vectors 3";
+        let instruction = new_ed25519_instruction_raw(pubkey, signature, message);
+        let tx = Transaction::new_signed_with_payer(
+            &[instruction.clone()],
+            Some(&mint_keypair.pubkey()),
+            &[&mint_keypair],
+            Hash::default(),
+        );
+
+        let feature_set = FeatureSet::default();
+        assert!(tx.verify_precompiles(&feature_set).is_ok());
+
+        let feature_set = FeatureSet::all_enabled();
+        assert!(tx.verify_precompiles(&feature_set).is_err()); // verify_strict does NOT pass
+    }
 }
diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs
@@ -837,6 +837,10 @@ pub mod move_stake_and_move_lamports_ixs {
     solana_sdk::declare_id!("7bTK6Jis8Xpfrs8ZoUfiMDPazTcdPcTWheZFJTA5Z6X4");
 }
 
+pub mod ed25519_precompile_verify_strict {
+    solana_sdk::declare_id!("ed9tNscbWLYBooxWA7FE2B5KHWs8A6sxfY8EzezEcoo");
+}
+
 lazy_static! {
     /// Map of feature identifiers to user-visible description
     pub static ref FEATURE_NAMES: HashMap<Pubkey, &'static str> = [
@@ -1041,6 +1045,7 @@ lazy_static! {
         (zk_elgamal_proof_program_enabled::id(), "Enable ZkElGamalProof program SIMD-0153"),
         (verify_retransmitter_signature::id(), "Verify retransmitter signature #1840"),
         (move_stake_and_move_lamports_ixs::id(), "Enable MoveStake and MoveLamports stake program instructions #1610"),
+        (ed25519_precompile_verify_strict::id(), "Use strict verification in ed25519 precompile SIMD-0152"),
         /*************** ADD NEW FEATURES HERE ***************/
     ]
     .iter()
