From 353ea62c17c379dbfcd26bcdeb22c72de382efbb Mon Sep 17 00:00:00 2001 From: Apoorv Kothari Date: Tue, 30 Sep 2025 23:14:09 -0700 Subject: [PATCH] Convert CipherCtx fns into a safe abstraction. Additional testing. --- boring/src/hmac.rs | 6 +-- boring/src/ssl/test/session_resumption.rs | 58 +++++++++++++---------- boring/src/symm.rs | 26 ++++------ 3 files changed, 45 insertions(+), 45 deletions(-) diff --git a/boring/src/hmac.rs b/boring/src/hmac.rs index 9816fb57..7e50e613 100644 --- a/boring/src/hmac.rs +++ b/boring/src/hmac.rs @@ -14,11 +14,7 @@ impl HmacCtxRef { /// Configures HmacCtx to use `md` as the hash function and `key` as the key. /// /// https://commondatastorage.googleapis.com/chromium-boringssl-docs/hmac.h.html#HMAC_Init_ex - /// - /// # Safety - /// - /// The caller must ensure HMAC_CTX has been initalized. - pub unsafe fn init(&mut self, key: &[u8], md: &MessageDigest) -> Result<(), ErrorStack> { + pub fn init(&mut self, key: &[u8], md: &MessageDigest) -> Result<(), ErrorStack> { ffi::init(); unsafe { diff --git a/boring/src/ssl/test/session_resumption.rs b/boring/src/ssl/test/session_resumption.rs index f7f481db..808abe30 100644 --- a/boring/src/ssl/test/session_resumption.rs +++ b/boring/src/ssl/test/session_resumption.rs @@ -153,6 +153,7 @@ fn test_noop_tickey_key_callback( encrypt: bool, ) -> TicketKeyCallbackResult { // These should only be used for testing purposes. + const TEST_KEY_NAME: [u8; 16] = [5; 16]; const TEST_CBC_IV: [u8; ffi::EVP_MAX_IV_LENGTH as usize] = [1; ffi::EVP_MAX_IV_LENGTH as usize]; const TEST_AES_128_CBC_KEY: [u8; 16] = [2; 16]; const TEST_HMAC_KEY: [u8; 32] = [3; 32]; @@ -161,24 +162,29 @@ fn test_noop_tickey_key_callback( let cipher = Cipher::aes_128_cbc(); if encrypt { - assert_eq!(key_name, &[0; 16]); - assert_eq!(iv, &[0; 16]); - NOOP_ENCRYPTION_CALLED_BACK.fetch_add(1, Ordering::SeqCst); + // Ensure key_name and iv are initialized and set test values. + assert_eq!(key_name, &[0; 16]); + assert_eq!(iv, &[0; 16]); + key_name.copy_from_slice(&TEST_KEY_NAME); + iv.copy_from_slice(&TEST_CBC_IV); + // Set the encryption context. - unsafe { - evp_ctx - .init_encrypt(&cipher, &TEST_AES_128_CBC_KEY, &TEST_CBC_IV) - .unwrap() - }; + evp_ctx + .init_encrypt(&cipher, &TEST_AES_128_CBC_KEY, &TEST_CBC_IV) + .unwrap(); // Set the hmac context. - unsafe { hmac_ctx.init(&TEST_HMAC_KEY, &digest).unwrap() }; + hmac_ctx.init(&TEST_HMAC_KEY, &digest).unwrap(); TicketKeyCallbackResult::Success } else { NOOP_DECRYPTION_CALLED_BACK.fetch_add(1, Ordering::SeqCst); + + // Check key_name matches. + assert_eq!(key_name, &TEST_KEY_NAME); + TicketKeyCallbackResult::Noop } } @@ -193,6 +199,7 @@ fn test_success_tickey_key_callback( encrypt: bool, ) -> TicketKeyCallbackResult { // These should only be used for testing purposes. + const TEST_KEY_NAME: [u8; 16] = [5; 16]; const TEST_CBC_IV: [u8; ffi::EVP_MAX_IV_LENGTH as usize] = [1; ffi::EVP_MAX_IV_LENGTH as usize]; const TEST_AES_128_CBC_KEY: [u8; 16] = [2; 16]; const TEST_HMAC_KEY: [u8; 32] = [3; 32]; @@ -201,31 +208,34 @@ fn test_success_tickey_key_callback( let cipher = Cipher::aes_128_cbc(); if encrypt { - assert_eq!(key_name, &[0; 16]); - assert_eq!(iv, &[0; 16]); - SUCCESS_ENCRYPTION_CALLED_BACK.fetch_add(1, Ordering::SeqCst); + // Ensure key_name and iv are initialized and set test values. + assert_eq!(key_name, &[0; 16]); + assert_eq!(iv, &[0; 16]); + key_name.copy_from_slice(&TEST_KEY_NAME); + iv.copy_from_slice(&TEST_CBC_IV); + // Set the encryption context. - unsafe { - evp_ctx - .init_encrypt(&cipher, &TEST_AES_128_CBC_KEY, &TEST_CBC_IV) - .unwrap() - }; + evp_ctx + .init_encrypt(&cipher, &TEST_AES_128_CBC_KEY, &TEST_CBC_IV) + .unwrap(); // Set the hmac context. - unsafe { hmac_ctx.init(&TEST_HMAC_KEY, &digest).unwrap() }; + hmac_ctx.init(&TEST_HMAC_KEY, &digest).unwrap(); } else { SUCCESS_DECRYPTION_CALLED_BACK.fetch_add(1, Ordering::SeqCst); + + // Check key_name matches. + assert_eq!(key_name, &TEST_KEY_NAME); + // Set the decryption context. - unsafe { - evp_ctx - .init_decrypt(&cipher, &TEST_AES_128_CBC_KEY, &TEST_CBC_IV) - .unwrap() - }; + evp_ctx + .init_decrypt(&cipher, &TEST_AES_128_CBC_KEY, iv) + .unwrap(); // Set the hmac context. - unsafe { hmac_ctx.init(&TEST_HMAC_KEY, &digest).unwrap() }; + hmac_ctx.init(&TEST_HMAC_KEY, &digest).unwrap(); } TicketKeyCallbackResult::Success diff --git a/boring/src/symm.rs b/boring/src/symm.rs index 1a2dc599..38fab76d 100644 --- a/boring/src/symm.rs +++ b/boring/src/symm.rs @@ -80,14 +80,7 @@ impl CipherCtxRef { /// Configures CipherCtx for a fresh encryption operation using `cipher`. /// /// https://commondatastorage.googleapis.com/chromium-boringssl-docs/cipher.h.html#EVP_EncryptInit_ex - /// - /// # Safety - /// - /// The caller must ensure EVP_CIPHER_CTX has been initalized. - /// - /// The caller is responsible for ensuring the length of `key` and `iv` are appropriate for the - /// chosen Cipher. - pub unsafe fn init_encrypt( + pub fn init_encrypt( &mut self, cipher: &Cipher, key: &[u8], @@ -95,6 +88,10 @@ impl CipherCtxRef { ) -> Result<(), ErrorStack> { ffi::init(); + if key.len() != cipher.key_len() { + return Err(ErrorStack::get()); + } + unsafe { cvt(ffi::EVP_EncryptInit_ex( self.as_ptr(), @@ -111,14 +108,7 @@ impl CipherCtxRef { /// Configures CipherCtx for a fresh decryption operation using `cipher`. /// /// https://commondatastorage.googleapis.com/chromium-boringssl-docs/cipher.h.html#EVP_DecryptInit_ex - /// - /// # Safety - /// - /// The caller must ensure EVP_CIPHER_CTX has been initalized. - /// - /// The caller is responsible for ensuring the length of `key` and `iv` are appropriate for the - /// chosen Cipher. - pub unsafe fn init_decrypt( + pub fn init_decrypt( &mut self, cipher: &Cipher, key: &[u8], @@ -126,6 +116,10 @@ impl CipherCtxRef { ) -> Result<(), ErrorStack> { ffi::init(); + if key.len() != cipher.key_len() { + return Err(ErrorStack::get()); + } + unsafe { cvt(ffi::EVP_DecryptInit_ex( self.as_ptr(),