From 00ddf99d7771e87e7df384b4f2782a10d7516bf5 Mon Sep 17 00:00:00 2001 From: CleverWild Date: Mon, 22 Jun 2026 17:53:34 +0200 Subject: [PATCH] fix!: protect evm_wallet integrity and bind key ciphertext to wallet address MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three independent failures allowed an offline attacker with DB write access to sign transactions using a different wallet's private key: 1. evm_wallet had no HMAC envelope — aead_encrypted_id could be swapped silently. 2. AEAD used a static tag as AAD — any valid ciphertext decrypted as any wallet key. 3. No post-decryption check that the derived address matched the requested wallet. Fix: sign_entity covers (address, aead_encrypted_id) in a single transaction; CreateNew/Decrypt take caller-provided AAD (wallet address bytes); after decryption signer.address() is verified against the requested wallet address. --- .../arbiter-server/src/actors/evm/mod.rs | 101 ++++++++++++++++-- .../arbiter-server/src/actors/vault/mod.rs | 14 ++- .../arbiter-server/tests/vault/concurrency.rs | 7 +- .../arbiter-server/tests/vault/lifecycle.rs | 16 +-- .../arbiter-server/tests/vault/storage.rs | 24 +++-- 5 files changed, 127 insertions(+), 35 deletions(-) diff --git a/server/crates/arbiter-server/src/actors/evm/mod.rs b/server/crates/arbiter-server/src/actors/evm/mod.rs index 06e672c..c2ea4f9 100644 --- a/server/crates/arbiter-server/src/actors/evm/mod.rs +++ b/server/crates/arbiter-server/src/actors/evm/mod.rs @@ -1,6 +1,6 @@ use crate::{ actors::vault::{CreateNew, Decrypt, Vault}, - crypto::integrity, + crypto::integrity::{self, Integrable}, db::{ DatabaseError, DatabasePool, models::{self}, @@ -25,14 +25,35 @@ use diesel::{ use diesel_async::RunQueryDsl; use kameo::{Actor, actor::ActorRef, messages}; use rand::{SeedableRng, rng, rngs::StdRng}; +use tracing::error; pub use crate::evm::safe_signer; +/// Integrity guard that binds a wallet's encrypted key ID to its Ethereum address. +/// Both fields are included in the HMAC — swapping `aead_encrypted_id` in the DB +/// invalidates the envelope MAC, and the AEAD ciphertext is also bound to `address` +/// as AAD, so decryption fails too. +#[derive(arbiter_macros::Hashable)] +struct EvmWalletIntegrity { + aead_encrypted_id: i32, + address: Address, +} + +impl Integrable for EvmWalletIntegrity { + const KIND: &'static str = "evm_wallet"; +} + #[derive(Debug, thiserror::Error)] pub enum SignTransactionError { #[error("Wallet not found")] WalletNotFound, + #[error("Decrypted key does not match requested wallet address")] + KeyAddressMismatch, + + #[error("Internal signing error")] + Internal, + #[error("Database error: {0}")] Database(#[from] DatabaseError), @@ -64,6 +85,12 @@ pub enum Error { Integrity(#[from] integrity::Error), } +impl From for Error { + fn from(e: diesel::result::Error) -> Self { + Self::Database(DatabaseError::from(e)) + } +} + #[derive(Actor)] pub struct EvmActor { pub vault: ActorRef, @@ -97,20 +124,39 @@ impl EvmActor { let aead_id: i32 = self .vault - .ask(CreateNew { plaintext }) + .ask(CreateNew { + plaintext, + aad: address.as_slice().to_vec(), + }) .await .map_err(|_| Error::VaultSend)?; let mut conn = self.db.get().await.map_err(DatabaseError::from)?; - let wallet_id = insert_into(schema::evm_wallet::table) - .values(&models::NewEvmWallet { - address: address.as_slice().to_vec(), - aead_encrypted_id: aead_id, + let wallet_id = conn + .exclusive_transaction(async |conn| { + let wallet_id: i32 = insert_into(schema::evm_wallet::table) + .values(&models::NewEvmWallet { + address: address.as_slice().to_vec(), + aead_encrypted_id: aead_id, + }) + .returning(schema::evm_wallet::id) + .get_result(conn) + .await + .map_err(DatabaseError::from) + .map_err(Error::Database)?; + + integrity::sign_entity( + conn, + &self.vault, + &EvmWalletIntegrity { address, aead_encrypted_id: aead_id }, + wallet_id, + ) + .await + .map_err(Error::Integrity)?; + + Ok::(wallet_id) }) - .returning(schema::evm_wallet::id) - .get_result(&mut conn) - .await - .map_err(DatabaseError::from)?; + .await?; Ok((wallet_id, address)) } @@ -241,16 +287,51 @@ impl EvmActor { .ok_or(SignTransactionError::WalletNotFound)?; drop(conn); + let mut conn = self.db.get().await.map_err(DatabaseError::from)?; + let attestation = integrity::verify_entity( + &mut conn, + &self.vault, + &EvmWalletIntegrity { + address: wallet_address, + aead_encrypted_id: wallet.aead_encrypted_id, + }, + wallet.id, + ) + .await + .map_err(|e| { + error!(?e, wallet_id = wallet.id, "EVM wallet integrity check failed"); + SignTransactionError::Internal + })?; + drop(conn); + + if attestation != integrity::AttestationStatus::Attested { + error!( + wallet_id = wallet.id, + "EVM wallet integrity unavailable; refusing to sign" + ); + return Err(SignTransactionError::Internal); + } + let raw_key: SafeCell> = self .vault .ask(Decrypt { aead_id: wallet.aead_encrypted_id, + aad: wallet.address.clone(), }) .await .map_err(|_| SignTransactionError::VaultSend)?; let signer = safe_signer::SafeSigner::from_cell(raw_key)?; + if signer.address() != wallet_address { + error!( + expected = %wallet_address, + actual = %signer.address(), + "Decrypted private key address does not match requested wallet" + ); + return Err(SignTransactionError::KeyAddressMismatch); + } + self.engine .evaluate_transaction(wallet_access, transaction.clone(), RunKind::Execution) .await?; diff --git a/server/crates/arbiter-server/src/actors/vault/mod.rs b/server/crates/arbiter-server/src/actors/vault/mod.rs index b995891..ec4c9fc 100644 --- a/server/crates/arbiter-server/src/actors/vault/mod.rs +++ b/server/crates/arbiter-server/src/actors/vault/mod.rs @@ -298,8 +298,10 @@ impl Vault { Ok(()) } + /// Decrypts an AEAD entry. The `aad` must match the value used at encryption time; + /// a mismatch causes authentication failure, preventing cross-wallet key swaps. #[message] - pub async fn decrypt(&mut self, aead_id: i32) -> Result>, Error> { + pub async fn decrypt(&mut self, aead_id: i32, aad: Vec) -> Result>, Error> { let Unsealed { root_key, .. } = Self::expect_unsealed(&mut self.state)?; let row: models::AeadEncrypted = { @@ -321,13 +323,15 @@ impl Vault { Error::BrokenDatabase })?; let mut output = SafeCell::new(row.ciphertext); - root_key.decrypt_in_place(&nonce, v1::TAG, &mut output)?; + root_key.decrypt_in_place(&nonce, &aad, &mut output)?; Ok(output) } + /// Creates a new `aead_encrypted` entry and returns its ID. + /// The `aad` is bound into the ciphertext and must be reproduced exactly at decryption time. // Creates new `aead_encrypted` entry in the database and returns it's ID #[message] - pub async fn create_new(&mut self, mut plaintext: SafeCell>) -> Result { + pub async fn create_new(&mut self, mut plaintext: SafeCell>, aad: Vec) -> Result { let Unsealed { root_key, root_key_history_id, @@ -339,7 +343,7 @@ impl Vault { let mut ciphertext_buffer = plaintext.write(); let ciphertext_buffer: &mut Vec = ciphertext_buffer.as_mut(); - root_key.encrypt_in_place(&nonce, v1::TAG, &mut *ciphertext_buffer)?; + root_key.encrypt_in_place(&nonce, &aad, &mut *ciphertext_buffer)?; let ciphertext = std::mem::take(ciphertext_buffer); @@ -474,7 +478,7 @@ mod tests { assert_eq!(root_row.data_encryption_nonce, n2.to_vec()); let id = actor - .create_new(SafeCell::new(b"post-interleave".to_vec())) + .create_new(SafeCell::new(b"post-interleave".to_vec()), b"test-aad".to_vec()) .await .unwrap(); let row: models::AeadEncrypted = schema::aead_encrypted::table diff --git a/server/crates/arbiter-server/tests/vault/concurrency.rs b/server/crates/arbiter-server/tests/vault/concurrency.rs index ee84f4a..ee77d19 100644 --- a/server/crates/arbiter-server/tests/vault/concurrency.rs +++ b/server/crates/arbiter-server/tests/vault/concurrency.rs @@ -14,6 +14,8 @@ use kameo::actor::{ActorRef, Spawn as _}; use std::collections::{HashMap, HashSet}; use tokio::task::JoinSet; +const TEST_AAD: &[u8] = b"test-aad"; + async fn write_concurrently( actor: ActorRef, prefix: &'static str, @@ -27,6 +29,7 @@ async fn write_concurrently( let id = actor .ask(CreateNew { plaintext: SafeCell::new(plaintext.clone()), + aad: TEST_AAD.to_vec(), }) .await .unwrap(); @@ -120,7 +123,7 @@ async fn insert_failure_does_not_create_partial_row() { drop(conn); let err = actor - .create_new(SafeCell::new(b"should fail".to_vec())) + .create_new(SafeCell::new(b"should fail".to_vec()), TEST_AAD.to_vec()) .await .unwrap_err(); assert!(matches!(err, Error::DatabaseTransaction(_))); @@ -171,7 +174,7 @@ async fn decrypt_roundtrip_after_high_concurrency() { .unwrap(); for (id, plaintext) in expected { - let mut decrypted = decryptor.decrypt(id).await.unwrap(); + let mut decrypted = decryptor.decrypt(id, TEST_AAD.to_vec()).await.unwrap(); assert_eq!(*decrypted.read(), plaintext); } } diff --git a/server/crates/arbiter-server/tests/vault/lifecycle.rs b/server/crates/arbiter-server/tests/vault/lifecycle.rs index c4ee7da..c7096ee 100644 --- a/server/crates/arbiter-server/tests/vault/lifecycle.rs +++ b/server/crates/arbiter-server/tests/vault/lifecycle.rs @@ -12,6 +12,8 @@ use arbiter_server::{ use diesel::{QueryDsl, SelectableHelper}; use diesel_async::RunQueryDsl; +const TEST_AAD: &[u8] = b"test-aad"; + #[tokio::test] #[test_log::test] async fn bootstrap() { @@ -57,7 +59,7 @@ async fn create_new_before_bootstrap_fails() { .unwrap(); let err = actor - .create_new(SafeCell::new(b"data".to_vec())) + .create_new(SafeCell::new(b"data".to_vec()), TEST_AAD.to_vec()) .await .unwrap_err(); assert!(matches!(err, Error::NotBootstrapped)); @@ -71,7 +73,7 @@ async fn decrypt_before_bootstrap_fails() { .await .unwrap(); - let err = actor.decrypt(1).await.unwrap_err(); + let err = actor.decrypt(1, TEST_AAD.to_vec()).await.unwrap_err(); assert!(matches!(err, Error::NotBootstrapped)); } @@ -85,7 +87,7 @@ async fn new_restores_sealed_state() { let mut actor2 = Vault::new(db, GlobalActors::spawn_message_bus()) .await .unwrap(); - let err = actor2.decrypt(1).await.unwrap_err(); + let err = actor2.decrypt(1, TEST_AAD.to_vec()).await.unwrap_err(); assert!(matches!(err, Error::Sealed)); } @@ -97,7 +99,7 @@ async fn unseal_correct_password() { let plaintext = b"survive a restart"; let aead_id = actor - .create_new(SafeCell::new(plaintext.to_vec())) + .create_new(SafeCell::new(plaintext.to_vec()), TEST_AAD.to_vec()) .await .unwrap(); drop(actor); @@ -108,7 +110,7 @@ async fn unseal_correct_password() { let seal_key = SafeCell::new(b"test-seal-key".to_vec()); actor.try_unseal(seal_key).await.unwrap(); - let mut decrypted = actor.decrypt(aead_id).await.unwrap(); + let mut decrypted = actor.decrypt(aead_id, TEST_AAD.to_vec()).await.unwrap(); assert_eq!(*decrypted.read(), plaintext); } @@ -120,7 +122,7 @@ async fn unseal_wrong_then_correct_password() { let plaintext = b"important data"; let aead_id = actor - .create_new(SafeCell::new(plaintext.to_vec())) + .create_new(SafeCell::new(plaintext.to_vec()), TEST_AAD.to_vec()) .await .unwrap(); drop(actor); @@ -136,6 +138,6 @@ async fn unseal_wrong_then_correct_password() { let good_key = SafeCell::new(b"test-seal-key".to_vec()); actor.try_unseal(good_key).await.unwrap(); - let mut decrypted = actor.decrypt(aead_id).await.unwrap(); + let mut decrypted = actor.decrypt(aead_id, TEST_AAD.to_vec()).await.unwrap(); assert_eq!(*decrypted.read(), plaintext); } diff --git a/server/crates/arbiter-server/tests/vault/storage.rs b/server/crates/arbiter-server/tests/vault/storage.rs index c1bd321..ed1cc8a 100644 --- a/server/crates/arbiter-server/tests/vault/storage.rs +++ b/server/crates/arbiter-server/tests/vault/storage.rs @@ -10,6 +10,8 @@ use diesel::{ExpressionMethods as _, QueryDsl, SelectableHelper, dsl::update}; use diesel_async::RunQueryDsl; use std::collections::HashSet; +const TEST_AAD: &[u8] = b"test-aad"; + #[tokio::test] #[test_log::test] async fn create_decrypt_roundtrip() { @@ -18,11 +20,11 @@ async fn create_decrypt_roundtrip() { let plaintext = b"hello arbiter"; let aead_id = actor - .create_new(SafeCell::new(plaintext.to_vec())) + .create_new(SafeCell::new(plaintext.to_vec()), TEST_AAD.to_vec()) .await .unwrap(); - let mut decrypted = actor.decrypt(aead_id).await.unwrap(); + let mut decrypted = actor.decrypt(aead_id, TEST_AAD.to_vec()).await.unwrap(); assert_eq!(*decrypted.read(), plaintext); } @@ -32,7 +34,7 @@ async fn decrypt_nonexistent_returns_not_found() { let db = db::create_test_pool().await; let mut actor = common::bootstrapped_vault(&db).await; - let err = actor.decrypt(9999).await.unwrap_err(); + let err = actor.decrypt(9999, TEST_AAD.to_vec()).await.unwrap_err(); assert!(matches!(err, Error::NotFound)); } @@ -44,11 +46,11 @@ async fn ciphertext_differs_across_entries() { let plaintext = b"same content"; let id1 = actor - .create_new(SafeCell::new(plaintext.to_vec())) + .create_new(SafeCell::new(plaintext.to_vec()), TEST_AAD.to_vec()) .await .unwrap(); let id2 = actor - .create_new(SafeCell::new(plaintext.to_vec())) + .create_new(SafeCell::new(plaintext.to_vec()), TEST_AAD.to_vec()) .await .unwrap(); @@ -68,8 +70,8 @@ async fn ciphertext_differs_across_entries() { assert_ne!(row1.ciphertext, row2.ciphertext); - let mut d1 = actor.decrypt(id1).await.unwrap(); - let mut d2 = actor.decrypt(id2).await.unwrap(); + let mut d1 = actor.decrypt(id1, TEST_AAD.to_vec()).await.unwrap(); + let mut d2 = actor.decrypt(id2, TEST_AAD.to_vec()).await.unwrap(); assert_eq!(*d1.read(), plaintext); assert_eq!(*d2.read(), plaintext); } @@ -83,7 +85,7 @@ async fn nonce_never_reused() { let n = 5; for i in 0..n { actor - .create_new(SafeCell::new(format!("secret {i}").into_bytes())) + .create_new(SafeCell::new(format!("secret {i}").into_bytes()), TEST_AAD.to_vec()) .await .unwrap(); } @@ -137,7 +139,7 @@ async fn broken_db_nonce_format_fails_closed() { drop(conn); let err = actor - .create_new(SafeCell::new(b"must fail".to_vec())) + .create_new(SafeCell::new(b"must fail".to_vec()), TEST_AAD.to_vec()) .await .unwrap_err(); assert!(matches!(err, Error::BrokenDatabase)); @@ -145,7 +147,7 @@ async fn broken_db_nonce_format_fails_closed() { let db = db::create_test_pool().await; let mut actor = common::bootstrapped_vault(&db).await; let id = actor - .create_new(SafeCell::new(b"decrypt target".to_vec())) + .create_new(SafeCell::new(b"decrypt target".to_vec()), TEST_AAD.to_vec()) .await .unwrap(); let mut conn = db.get().await.unwrap(); @@ -156,6 +158,6 @@ async fn broken_db_nonce_format_fails_closed() { .unwrap(); drop(conn); - let err = actor.decrypt(id).await.unwrap_err(); + let err = actor.decrypt(id, TEST_AAD.to_vec()).await.unwrap_err(); assert!(matches!(err, Error::BrokenDatabase)); }