From 7ae4467d38d32e3c2b65b04264347d93ab6fa746 Mon Sep 17 00:00:00 2001 From: Dmytro Kozhevin Date: Thu, 29 Jun 2023 16:50:51 -0400 Subject: [PATCH] Expiration-related fixes (#899) * Don't allow using signatures that expire too far in the future. This prevents users from being able to execute potentially replayable transactions. * Modify the expiration ledger formula to be consistent with the lifetime. Before that the entry would live 1 ledger longer than the number of ledgers that it has been bumped for. * Make nonce expiration ledger to be at least min expiration ledger. * Make allowance expiration logic consistent with bumps. --- soroban-env-host/src/auth.rs | 26 +++++++- soroban-env-host/src/host.rs | 16 +++-- .../src/native_contract/token/allowance.rs | 8 ++- soroban-env-host/src/test/auth.rs | 6 ++ soroban-env-host/src/test/token.rs | 60 ++++++++++++++++++- 5 files changed, 105 insertions(+), 11 deletions(-) diff --git a/soroban-env-host/src/auth.rs b/soroban-env-host/src/auth.rs index d1d6533fb..41644a7f0 100644 --- a/soroban-env-host/src/auth.rs +++ b/soroban-env-host/src/auth.rs @@ -1172,12 +1172,32 @@ impl AccountAuthorizationTracker { } self.need_nonce = false; if let Some((nonce, expiration_ledger)) = &self.nonce { - if host.with_ledger_info(|li| Ok(li.sequence_number))? > *expiration_ledger { + let (ledger_seq, max_entry_expiration) = + host.with_ledger_info(|li| Ok((li.sequence_number, li.max_entry_expiration)))?; + if ledger_seq > *expiration_ledger { return Err(host.err( ScErrorType::Auth, ScErrorCode::InvalidInput, "signature has expired", - &[], + &[ + ledger_seq.try_into_val(host)?, + expiration_ledger.try_into_val(host)?, + ], + )); + } + if *expiration_ledger + > ledger_seq + .saturating_sub(1) + .saturating_add(max_entry_expiration) + { + return Err(host.err( + ScErrorType::Auth, + ScErrorCode::InvalidInput, + "signature expiration is too late", + &[ + (ledger_seq + max_entry_expiration).try_into_val(host)?, + expiration_ledger.try_into_val(host)?, + ], )); } if let Some(addr) = self.address { @@ -1340,6 +1360,8 @@ impl Host { nonce_key_scval.metered_clone(self.budget_ref())?, xdr::ContractDataDurability::Temporary, ); + let expiration_ledger = expiration_ledger + .max(self.get_min_expiration_ledger(xdr::ContractDataDurability::Temporary)?); self.with_mut_storage(|storage| { if storage.has(&nonce_key, self.budget_ref())? { return Err(self.err( diff --git a/soroban-env-host/src/host.rs b/soroban-env-host/src/host.rs index c67d537c7..0bbc69c5d 100644 --- a/soroban-env-host/src/host.rs +++ b/soroban-env-host/src/host.rs @@ -2031,8 +2031,12 @@ impl VmCallerEnv for Host { .borrow_mut() .touch_key(&key, self.as_budget())?; - let min_expiration = - self.with_ledger_info(|li| Ok(li.sequence_number.saturating_add(min.into())))?; + let min_expiration = self.with_ledger_info(|li| { + Ok(li + .sequence_number + .saturating_sub(1) + .saturating_add(min.into())) + })?; self.0.expiration_bumps.borrow_mut().metered_push( self, LedgerBump { @@ -2048,8 +2052,12 @@ impl VmCallerEnv for Host { _vmcaller: &mut VmCaller, min: U32Val, ) -> Result { - let min_expiration = - self.with_ledger_info(|li| Ok(li.sequence_number.saturating_add(min.into())))?; + let min_expiration = self.with_ledger_info(|li| { + Ok(li + .sequence_number + .saturating_sub(1) + .saturating_add(min.into())) + })?; let contract_id = self.get_current_contract_id_internal()?; let key = self.contract_instance_ledger_key(&contract_id)?; diff --git a/soroban-env-host/src/native_contract/token/allowance.rs b/soroban-env-host/src/native_contract/token/allowance.rs index 8e0030f82..d9b300e3d 100644 --- a/soroban-env-host/src/native_contract/token/allowance.rs +++ b/soroban-env-host/src/native_contract/token/allowance.rs @@ -35,7 +35,11 @@ pub fn write_allowance( // validates the expiration and then returns the ledger seq // The expiration can be less than ledger seq if clearing an allowance let ledger_seq = e.with_ledger_info(|li| { - if expiration > li.sequence_number.saturating_add(li.max_entry_expiration) { + if expiration + > li.sequence_number + .saturating_sub(1) + .saturating_add(li.max_entry_expiration) + { Err(err!( e, ContractError::AllowanceError, @@ -93,7 +97,7 @@ pub fn write_allowance( e.bump_contract_data( key.try_into_val(e)?, StorageType::Temporary, - (expiration - ledger_seq).into(), + (expiration - ledger_seq + 1).into(), )?; } } diff --git a/soroban-env-host/src/test/auth.rs b/soroban-env-host/src/test/auth.rs index 3253074ac..7e71c4c7f 100644 --- a/soroban-env-host/src/test/auth.rs +++ b/soroban-env-host/src/test/auth.rs @@ -91,6 +91,12 @@ impl AuthTest { // to respect the budget limit. host.as_budget().reset_unlimited(); host.enable_debug(); + + host.with_mut_ledger_info(|li| { + li.sequence_number = 100; + li.max_entry_expiration = 10000; + }) + .unwrap(); let mut accounts = vec![]; for _ in 0..signer_cnt { accounts.push(generate_keypair()); diff --git a/soroban-env-host/src/test/token.rs b/soroban-env-host/src/test/token.rs index b65dd1221..e9f4593e1 100644 --- a/soroban-env-host/src/test/token.rs +++ b/soroban-env-host/src/test/token.rs @@ -57,7 +57,7 @@ impl TokenTest { base_reserve: 5_000_000, min_persistent_entry_expiration: 4096, min_temp_entry_expiration: 16, - max_entry_expiration: 6312000, + max_entry_expiration: 10000, }); Self { host, @@ -1658,14 +1658,68 @@ fn test_auth_rejected_for_incorrect_nonce() { let args = host_vec![&test.host, user.address(&test.host), 100_i128]; - // Correct call to consume nonce. + // Expired nonce authorize_single_invocation_with_nonce( &test.host, &admin, &token.address, "mint", args.clone(), - Some((12345, 1000)), + Some((12345, 122)), + ); + assert!(test + .host + .call( + token.address.clone().into(), + Symbol::try_from_small_str("mint").unwrap(), + args.clone().into(), + ) + .is_err()); + + // Too early for nonce + authorize_single_invocation_with_nonce( + &test.host, + &admin, + &token.address, + "mint", + args.clone(), + Some((12345, 123 + 10000)), + ); + assert!(test + .host + .call( + token.address.clone().into(), + Symbol::try_from_small_str("mint").unwrap(), + args.clone().into(), + ) + .is_err()); + + // Correct call to consume nonce that expires on the current ledger. + authorize_single_invocation_with_nonce( + &test.host, + &admin, + &token.address, + "mint", + args.clone(), + Some((12345, 123)), + ); + test.host + .call( + token.address.clone().into(), + Symbol::try_from_small_str("mint").unwrap(), + args.clone().into(), + ) + .unwrap(); + + // Correct call to consume nonce that expires at the maximum possible entry + // lifetime. + authorize_single_invocation_with_nonce( + &test.host, + &admin, + &token.address, + "mint", + args.clone(), + Some((123456, 123 + 10000 - 1)), ); test.host .call(