From 6ead7347976bec04a2b65c2364462180f7d80da4 Mon Sep 17 00:00:00 2001 From: Philip Robinson Date: Fri, 2 Oct 2026 12:00:15 +0100 Subject: [PATCH 1/3] Fixes for AI security scan comments Manually import subtles constant time eq and gt/lt and used in padding impl --- Cargo.lock | 2 +- Cargo.toml | 2 +- README.md | 28 +++- src/constant_time.rs | 309 ++++++++++++++++++++++++++++++++++++++ src/ctaes_ffi.rs | 4 +- src/error.rs | 4 + src/lib.rs | 138 ++++++++++------- src/padding.rs | 172 ++++++++++++++++----- tests/decrypt_buffers.rs | 72 +++++++++ tests/padding_lengths.rs | 57 +++++++ tests/pkcs7_validation.rs | 116 ++++++++++++++ tests/tests.rs | 119 +++++++++++++-- 12 files changed, 908 insertions(+), 115 deletions(-) create mode 100644 src/constant_time.rs create mode 100644 tests/decrypt_buffers.rs create mode 100644 tests/padding_lengths.rs create mode 100644 tests/pkcs7_validation.rs diff --git a/Cargo.lock b/Cargo.lock index b79cb94..7102053 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -19,7 +19,7 @@ dependencies = [ [[package]] name = "ctaes-rs" -version = "0.1.2" +version = "0.2.0" dependencies = [ "cc", "hex-conservative", diff --git a/Cargo.toml b/Cargo.toml index e08edd8..b2996ae 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -2,7 +2,7 @@ name = "ctaes-rs" version = "0.1.2" authors = ["Philip Robinson "] -license = "MIT" +license = "MIT AND BSD-3-Clause" homepage = "https://github.com/ElementsProject/rust-ctaes/" repository = "https://github.com/ElementsProject/rust-ctaes/" description = "Rust bindings and API for CTAES a constant-time AES implementation from Bitcoin Core https://github.com/bitcoin-core/ctaes" diff --git a/README.md b/README.md index ff6e13f..37b6cbf 100644 --- a/README.md +++ b/README.md @@ -13,8 +13,30 @@ buffers. Zero Padding and PKCS7 padding implementations are provided Run `cargo doc --nodeps` to generate the documentation of the library. # Note -This is a low-level encryption library and should be employed in conjunction with a message -authentication scheme to avoid chosen-ciphertext and chosen-plaintext attacks. +This is a low-level, unauthenticated encryption library. Callers MUST authenticate +the IV, ciphertext, and relevant protocol metadata before decrypting or unpadding. +Use encrypt-then-MAC with independent encryption and MAC keys, or use an +authenticated-encryption library. Reject invalid tags before calling CBC decrypt; +never expose padding results for unauthenticated ciphertext. + +Strict padding checks and constant-time byte comparisons do not eliminate a +padding oracle: success versus failure still reveals padding validity. + +For AES padding `Padding::unpad` uses a block size of 16 by default. For any other block size +use `Pkcs7::unpad_with_block_size(buffer, BLOCK_SIZE)`. + +CBC instances retain their chaining state across calls. CBC `encrypt` and +`decrypt` require a mutable instance. Use separate instances with the same key +and initial IV to encrypt and decrypt a message. Calls on one instance process +successive block-aligned chunks of that message; they do not start new messages. +For each independent message, construct a new instance with a fresh, +unpredictable IV. # Compatibility -This crate requires Rust 1.63.0 or later. \ No newline at end of file +This crate requires Rust 1.63.0 or later. + +# License +This crate is licensed under the MIT license (see `COPYING`), except for +`src/constant_time.rs`, which contains code derived from the +[subtle](https://github.com/dalek-cryptography/subtle) crate and is licensed under +the BSD-3-Clause license reproduced at the top of that file. diff --git a/src/constant_time.rs b/src/constant_time.rs new file mode 100644 index 0000000..0b021ab --- /dev/null +++ b/src/constant_time.rs @@ -0,0 +1,309 @@ +// SPDX-License-Identifier: BSD-3-Clause +// +// Derived from the `subtle` crate v2.6.1: https://github.com/dalek-cryptography/subtle +// +// Copyright (c) 2016-2017 Isis Agora Lovecruft, Henry de Valence. All rights reserved. +// Copyright (c) 2016-2024 Isis Agora Lovecruft. All rights reserved. +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// 1. Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// +// 2. Redistributions in binary form must reproduce the above copyright +// notice, this list of conditions and the following disclaimer in the +// documentation and/or other materials provided with the distribution. +// +// 3. Neither the name of the copyright holder nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS +// IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED +// TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +// PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED +// TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR +// PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF +// LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING +// NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS +// SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +use std::ops::{BitAnd, BitAndAssign, BitOr, BitXor, Not}; + +/// The `Choice` struct represents a choice for use in conditional assignment. +/// +/// It is a wrapper around a `u8`, which should have the value either `1` (true) +/// or `0` (false). +/// +/// The conversion from `u8` to `Choice` passes the value through an optimization +/// barrier, as a best-effort attempt to prevent the compiler from inferring that +/// the `Choice` value is a boolean. This strategy is based on Tim Maclean's +/// [work on `rust-timing-shield`][rust-timing-shield], which attempts to provide +/// a more comprehensive approach for preventing software side-channels in Rust +/// code. +/// +/// The `Choice` struct implements operators for AND, OR, XOR, and NOT, to allow +/// combining `Choice` values. These operations do not short-circuit. +/// +/// [rust-timing-shield]: +/// https://www.chosenplaintext.ca/open-source/rust-timing-shield/security +#[derive(Copy, Clone, Debug)] +pub struct Choice(u8); + +impl Choice { + /// Unwrap the `Choice` wrapper to reveal the underlying `u8`. + /// + /// # Note + /// + /// This function only exists as an **escape hatch** for the rare case + /// where it's not possible to use one of the `subtle`-provided + /// trait impls. + /// + /// **To convert a `Choice` to a `bool`, use the `From` implementation instead.** + #[inline] + pub fn unwrap_u8(&self) -> u8 { + self.0 + } +} + +impl From for bool { + /// Convert the `Choice` wrapper into a `bool`, depending on whether + /// the underlying `u8` was a `0` or a `1`. + /// + /// # Note + /// + /// This function exists to avoid having higher-level cryptographic protocol + /// implementations duplicating this pattern. + /// + /// The intended use case for this conversion is at the _end_ of a + /// higher-level primitive implementation: for example, in checking a keyed + /// MAC, where the verification should happen in constant-time (and thus use + /// a `Choice`) but it is safe to return a `bool` at the end of the + /// verification. + #[inline] + fn from(source: Choice) -> bool { + debug_assert!((source.0 == 0u8) | (source.0 == 1u8)); + source.0 != 0 + } +} + +impl BitAnd for Choice { + type Output = Choice; + #[inline] + fn bitand(self, rhs: Choice) -> Choice { + (self.0 & rhs.0).into() + } +} + +impl BitAndAssign for Choice { + #[inline] + fn bitand_assign(&mut self, rhs: Choice) { + *self = *self & rhs; + } +} + +impl BitOr for Choice { + type Output = Choice; + #[inline] + fn bitor(self, rhs: Choice) -> Choice { + (self.0 | rhs.0).into() + } +} + +impl BitXor for Choice { + type Output = Choice; + #[inline] + fn bitxor(self, rhs: Choice) -> Choice { + (self.0 ^ rhs.0).into() + } +} + +impl Not for Choice { + type Output = Choice; + #[inline] + fn not(self) -> Choice { + (1u8 & (!self.0)).into() + } +} + +/// This function is a best-effort attempt to prevent the compiler from knowing +/// anything about the value of the returned `u8`, other than its type. +/// +/// Because we want to support stable Rust, we don't have access to inline +/// assembly or test::black_box, so we use the fact that volatile values will +/// never be elided to register values. +/// +/// Note: Rust's notion of "volatile" is subject to change over time. While this +/// code may break in a non-destructive way in the future, “constant-time” code +/// is a continually moving target, and this is better than doing nothing. +#[inline(never)] +fn black_box(input: T) -> T { + unsafe { + // Optimization barrier + // + // SAFETY: + // - &input is not NULL because we own input; + // - input is Copy and always live; + // - input is always properly aligned. + core::ptr::read_volatile(&input) + } +} + +impl From for Choice { + #[inline] + fn from(input: u8) -> Choice { + debug_assert!((input == 0u8) | (input == 1u8)); + + // Our goal is to prevent the compiler from inferring that the value held inside the + // resulting `Choice` struct is really a `bool` instead of a `u8`. + Choice(black_box(input)) + } +} + +/// An `Eq`-like trait that produces a `Choice` instead of a `bool`. +// #[inline] is specified on these function prototypes to signify that they +#[allow(unused_attributes)] // should be in the actual implementation +pub trait ConstantTimeEq { + /// Determine if two items are equal. + /// + /// The `ct_eq` function should execute in constant time. + /// + /// # Returns + /// + /// * `Choice(1u8)` if `self == other`; + /// * `Choice(0u8)` if `self != other`. + #[inline] + #[allow(unused_attributes)] + fn ct_eq(&self, other: &Self) -> Choice; +} + +impl ConstantTimeEq for Choice { + #[inline] + fn ct_eq(&self, rhs: &Choice) -> Choice { + !(*self ^ *rhs) + } +} + +/// Given the bit-width `$bit_width` and the corresponding primitive +/// unsigned and signed types `$t_u` and `$t_i` respectively, generate +/// an `ConstantTimeEq` implementation. +macro_rules! generate_integer_equal { + ($t_u:ty, $t_i:ty, $bit_width:expr) => { + impl ConstantTimeEq for $t_u { + #[inline] + fn ct_eq(&self, other: &$t_u) -> Choice { + // x == 0 if and only if self == other + let x: $t_u = self ^ other; + + // If x == 0, then x and -x are both equal to zero; + // otherwise, one or both will have its high bit set. + let y: $t_u = (x | x.wrapping_neg()) >> ($bit_width - 1); + + // Result is the opposite of the high bit (now shifted to low). + ((y ^ (1 as $t_u)) as u8).into() + } + } + impl ConstantTimeEq for $t_i { + #[inline] + fn ct_eq(&self, other: &$t_i) -> Choice { + // Bitcast to unsigned and call that implementation. + (*self as $t_u).ct_eq(&(*other as $t_u)) + } + } + }; +} + +generate_integer_equal!(u8, i8, 8); +generate_integer_equal!(u16, i16, 16); +generate_integer_equal!(u32, i32, 32); +generate_integer_equal!(u64, i64, 64); + +/// A type which can be compared in some manner and be determined to be greater +/// than another of the same type. +pub trait ConstantTimeGreater { + /// Determine whether `self > other`. + /// + /// The bitwise-NOT of the return value of this function should be usable to + /// determine if `self <= other`. + /// + /// This function should execute in constant time. + /// + /// # Returns + /// + /// A `Choice` with a set bit if `self > other`, and with no set bits + /// otherwise. + fn ct_gt(&self, other: &Self) -> Choice; +} + +macro_rules! generate_unsigned_integer_greater { + ($t_u: ty, $bit_width: expr) => { + impl ConstantTimeGreater for $t_u { + /// Returns Choice::from(1) iff x > y, and Choice::from(0) iff x <= y. + /// + /// # Note + /// + /// This algoritm would also work for signed integers if we first + /// flip the top bit, e.g. `let x: u8 = x ^ 0x80`, etc. + #[inline] + fn ct_gt(&self, other: &$t_u) -> Choice { + let gtb = self & !other; // All the bits in self that are greater than their corresponding bits in other. + let mut ltb = !self & other; // All the bits in self that are less than their corresponding bits in other. + let mut pow = 1; + + // Less-than operator is okay here because it's dependent on the bit-width. + while pow < $bit_width { + ltb |= ltb >> pow; // Bit-smear the highest set bit to the right. + pow += pow; + } + let mut bit = gtb & !ltb; // Select the highest set bit. + let mut pow = 1; + + while pow < $bit_width { + bit |= bit >> pow; // Shift it to the right until we end up with either 0 or 1. + pow += pow; + } + // XXX We should possibly do the above flattening to 0 or 1 in the + // Choice constructor rather than making it a debug error? + Choice::from((bit & 1) as u8) + } + } + }; +} + +generate_unsigned_integer_greater!(u8, 8); +generate_unsigned_integer_greater!(u16, 16); +generate_unsigned_integer_greater!(u32, 32); +generate_unsigned_integer_greater!(u64, 64); + +/// A type which can be compared in some manner and be determined to be less +/// than another of the same type. +pub trait ConstantTimeLess: ConstantTimeEq + ConstantTimeGreater { + /// Determine whether `self < other`. + /// + /// The bitwise-NOT of the return value of this function should be usable to + /// determine if `self >= other`. + /// + /// A default implementation is provided and implemented for the unsigned + /// integer types. + /// + /// This function should execute in constant time. + /// + /// # Returns + /// + /// A `Choice` with a set bit if `self < other`, and with no set bits + /// otherwise. + + #[inline] + fn ct_lt(&self, other: &Self) -> Choice { + !self.ct_gt(other) & !self.ct_eq(other) + } +} + +impl ConstantTimeLess for u8 {} +impl ConstantTimeLess for u16 {} +impl ConstantTimeLess for u32 {} +impl ConstantTimeLess for u64 {} diff --git a/src/ctaes_ffi.rs b/src/ctaes_ffi.rs index 2d208d9..caf5321 100644 --- a/src/ctaes_ffi.rs +++ b/src/ctaes_ffi.rs @@ -193,6 +193,6 @@ pub trait FfiAesCipher { /// A sealed trait defining the interface of an AES-CBC cipher pub trait FfiAesCbcCipher: Sized { - fn ffi_decrypt(&self, num_blocks: usize, plaintext: &mut [u8], ciphertext: &[u8]); - fn ffi_encrypt(&self, num_blocks: usize, ciphertext: &mut [u8], plaintext: &[u8]); + fn ffi_decrypt(&mut self, num_blocks: usize, plaintext: &mut [u8], ciphertext: &[u8]); + fn ffi_encrypt(&mut self, num_blocks: usize, ciphertext: &mut [u8], plaintext: &[u8]); } diff --git a/src/error.rs b/src/error.rs index 28e9324..3772086 100644 --- a/src/error.rs +++ b/src/error.rs @@ -15,6 +15,10 @@ pub enum Error { NonBlockSizeAlignedBuffer, #[error("Output buffer is not big enough to hold the result of processing input")] InsufficientBufferSize, + #[error("Invalid padding block size")] + InvalidBlockSize, + #[error("Padded buffer length exceeds usize::MAX")] + PaddedLengthOverflow, #[error("Buffer not large enough to accomodate padded buffer")] PaddedBufferTooSmall, #[error("Unable to unpad buffer: {0}")] diff --git a/src/lib.rs b/src/lib.rs index 29599e3..abfe420 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -12,10 +12,17 @@ //! The crate also provides a Padding utility implementation to help the user prepare, pad and unpad //! buffers. Zero Padding and PKCS7 padding implementations are provided //! +//! # Security +//! +//! CBC provides no authentication. Callers MUST authenticate the IV and ciphertext +//! (and any protocol metadata) before decryption or unpadding. Use encrypt-then-MAC +//! with independent encryption and MAC keys, or an authenticated-encryption library. +//! Never expose padding results for unauthenticated input; even constant-time +//! padding validation still reveals validity through its return value. +//! //! # Examples //! //! ``` -//! extern crate hex_conservative; //! use hex_conservative::FromHex; //! use ctaes_rs::{Padding, Pkcs7, AesCbcBlockCipher, Aes128Cbc}; //! @@ -23,18 +30,19 @@ //! let iv = <[u8; 16]>::from_hex("000102030405060708090a0b0c0d0e0f").unwrap(); //! let message = >::from_hex("6bc1bee22e409f96e93d7e117393172aae2d8a571e03ac9c9eb76fac45af8e5130c81c46a35ce411e5fbc1191a0a52eff69f2445df4f9b17ad2b417be66c3710").unwrap(); //! -//! let padded_buffer_length = Pkcs7::padded_buffer_length(message.len(), 16); +//! let padded_buffer_length = Pkcs7::padded_buffer_length(message.len(), 16).unwrap(); //! let mut plaintext = vec![0u8; padded_buffer_length]; //! plaintext[0..message.len()].copy_from_slice(message.as_slice()); //! Pkcs7::pad(plaintext.as_mut_slice(), message.len(), 16).unwrap(); //! let mut ciphertext = vec![0u8; padded_buffer_length]; //! -//! let cipher = Aes128Cbc::new(key.as_slice(), iv.as_slice()).unwrap(); +//! let mut cipher = Aes128Cbc::new(key.as_slice(), iv.as_slice()).unwrap(); //! cipher.encrypt(plaintext.as_slice(), ciphertext.as_mut_slice()).unwrap(); //! //! let mut deciphered = vec![0u8; padded_buffer_length]; -//! cipher.decrypt(ciphertext.as_slice(), deciphered.as_mut_slice()).unwrap(); -//! let unpadded_result = Pkcs7::unpad(deciphered.as_slice()).unwrap(); +//! let mut decipher = Aes128Cbc::new(key.as_slice(), iv.as_slice()).unwrap(); +//! decipher.decrypt(ciphertext.as_slice(), deciphered.as_mut_slice()).unwrap(); +//! let unpadded_result = Pkcs7::unpad_with_block_size(deciphered.as_slice(), 16).unwrap(); //! assert_eq!(message.as_slice(), unpadded_result); //! ``` //! @@ -42,6 +50,7 @@ extern crate thiserror; extern crate zeroize; +mod constant_time; #[allow(non_snake_case)] mod ctaes_ffi; mod error; @@ -80,6 +89,9 @@ pub trait AesBlockCipher: FfiAesCipher { } /// Decrypt the contents of `ciphertext` and place the result in the `plaintext` out parameter + /// + /// `ciphertext` must contain whole AES blocks. `plaintext` must be at least as long + /// as `ciphertext`; any remaining bytes in `plaintext` are left unchanged. fn decrypt(&self, ciphertext: &[u8], plaintext: &mut [u8]) -> Result<(), Error> { if ciphertext.len() % AES_BLOCK_SIZE != 0 { return Err(Error::NonBlockSizeAlignedBuffer); @@ -222,8 +234,21 @@ impl Aes256 { impl AesBlockCipher for Aes256 {} /// Trait that implements the common `encrypt` and `decrypt` methods for all AES-CBC ciphers +/// +/// This is unauthenticated encryption. Callers MUST verify a MAC over the IV, +/// ciphertext, and relevant metadata before decrypting or unpadding. +/// +/// Each instance holds one CBC chain. Successful calls advance its IV to the last +/// ciphertext block; empty inputs and rejected calls leave the chain unchanged. +/// Use separate instances initialized with the same key and IV for encryption +/// and decryption of a message. For each independent message, create a new +/// instance with a fresh, unpredictable IV; continuing a chain is for chunks of +/// the same message only. pub trait AesCbcBlockCipher: FfiAesCbcCipher { - fn encrypt(&self, plaintext: &[u8], ciphertext: &mut [u8]) -> Result<(), Error> { + /// Encrypt whole blocks, advancing the CBC chain. + /// + /// Output must be at least as long as input; trailing output bytes are unchanged. + fn encrypt(&mut self, plaintext: &[u8], ciphertext: &mut [u8]) -> Result<(), Error> { if plaintext.len() % AES_BLOCK_SIZE != 0 { return Err(Error::NonBlockSizeAlignedBuffer); } @@ -237,7 +262,11 @@ pub trait AesCbcBlockCipher: FfiAesCbcCipher { Ok(()) } - fn decrypt(&self, ciphertext: &[u8], plaintext: &mut [u8]) -> Result<(), Error> { + /// Decrypt the contents of `ciphertext` and place the result in `plaintext`. + /// + /// `ciphertext` must contain whole AES blocks. `plaintext` must be at least as long + /// as `ciphertext`; any remaining bytes in `plaintext` are left unchanged. + fn decrypt(&mut self, ciphertext: &[u8], plaintext: &mut [u8]) -> Result<(), Error> { if ciphertext.len() % AES_BLOCK_SIZE != 0 { return Err(Error::NonBlockSizeAlignedBuffer); } @@ -253,13 +282,12 @@ pub trait AesCbcBlockCipher: FfiAesCbcCipher { } /// 128-bit AES-CBC cipher -pub struct Aes128Cbc<'a> { - key: &'a [u8], - iv: &'a [u8], +pub struct Aes128Cbc { + context: AES128_CBC_ctx, } -impl<'a> Aes128Cbc<'a> { - pub fn new(key: &'a [u8], iv: &'a [u8]) -> Result { +impl Aes128Cbc { + pub fn new(key: &[u8], iv: &[u8]) -> Result { if key.len() != AES128_KEY_LENGTH { return Err(Error::KeyIncorrectLength(AES128_KEY_LENGTH)); } @@ -267,17 +295,19 @@ impl<'a> Aes128Cbc<'a> { return Err(Error::IvIncorrectLength); } - Ok(Self { key, iv }) + let mut context = AES128_CBC_ctx::default(); + unsafe { + AES128_CBC_init(&mut context, key.as_ptr(), iv.as_ptr()); + } + Ok(Self { context }) } } -impl FfiAesCbcCipher for Aes128Cbc<'_> { - fn ffi_decrypt(&self, num_blocks: usize, plaintext: &mut [u8], ciphertext: &[u8]) { - let mut context = AES128_CBC_ctx::default(); +impl FfiAesCbcCipher for Aes128Cbc { + fn ffi_decrypt(&mut self, num_blocks: usize, plaintext: &mut [u8], ciphertext: &[u8]) { unsafe { - AES128_CBC_init(&mut context, self.key.as_ptr(), self.iv.as_ptr()); AES128_CBC_decrypt( - &mut context, + &mut self.context, num_blocks, plaintext.as_mut_ptr(), ciphertext.as_ptr(), @@ -285,12 +315,10 @@ impl FfiAesCbcCipher for Aes128Cbc<'_> { } } - fn ffi_encrypt(&self, num_blocks: usize, ciphertext: &mut [u8], plaintext: &[u8]) { - let mut context = AES128_CBC_ctx::default(); + fn ffi_encrypt(&mut self, num_blocks: usize, ciphertext: &mut [u8], plaintext: &[u8]) { unsafe { - AES128_CBC_init(&mut context, self.key.as_ptr(), self.iv.as_ptr()); AES128_CBC_encrypt( - &mut context, + &mut self.context, num_blocks, ciphertext.as_mut_ptr(), plaintext.as_ptr(), @@ -299,16 +327,15 @@ impl FfiAesCbcCipher for Aes128Cbc<'_> { } } -impl AesCbcBlockCipher for Aes128Cbc<'_> {} +impl AesCbcBlockCipher for Aes128Cbc {} /// 192-bit AES-CBC cipher -pub struct Aes192Cbc<'a> { - key: &'a [u8], - iv: &'a [u8], +pub struct Aes192Cbc { + context: AES192_CBC_ctx, } -impl<'a> Aes192Cbc<'a> { - pub fn new(key: &'a [u8], iv: &'a [u8]) -> Result { +impl Aes192Cbc { + pub fn new(key: &[u8], iv: &[u8]) -> Result { if key.len() != AES192_KEY_LENGTH { return Err(Error::KeyIncorrectLength(AES192_KEY_LENGTH)); } @@ -316,17 +343,19 @@ impl<'a> Aes192Cbc<'a> { return Err(Error::IvIncorrectLength); } - Ok(Self { key, iv }) + let mut context = AES192_CBC_ctx::default(); + unsafe { + AES192_CBC_init(&mut context, key.as_ptr(), iv.as_ptr()); + } + Ok(Self { context }) } } -impl FfiAesCbcCipher for Aes192Cbc<'_> { - fn ffi_decrypt(&self, num_blocks: usize, plaintext: &mut [u8], ciphertext: &[u8]) { - let mut context = AES192_CBC_ctx::default(); +impl FfiAesCbcCipher for Aes192Cbc { + fn ffi_decrypt(&mut self, num_blocks: usize, plaintext: &mut [u8], ciphertext: &[u8]) { unsafe { - AES192_CBC_init(&mut context, self.key.as_ptr(), self.iv.as_ptr()); AES192_CBC_decrypt( - &mut context, + &mut self.context, num_blocks, plaintext.as_mut_ptr(), ciphertext.as_ptr(), @@ -334,12 +363,10 @@ impl FfiAesCbcCipher for Aes192Cbc<'_> { } } - fn ffi_encrypt(&self, num_blocks: usize, ciphertext: &mut [u8], plaintext: &[u8]) { - let mut context = AES192_CBC_ctx::default(); + fn ffi_encrypt(&mut self, num_blocks: usize, ciphertext: &mut [u8], plaintext: &[u8]) { unsafe { - AES192_CBC_init(&mut context, self.key.as_ptr(), self.iv.as_ptr()); AES192_CBC_encrypt( - &mut context, + &mut self.context, num_blocks, ciphertext.as_mut_ptr(), plaintext.as_ptr(), @@ -348,16 +375,15 @@ impl FfiAesCbcCipher for Aes192Cbc<'_> { } } -impl AesCbcBlockCipher for Aes192Cbc<'_> {} +impl AesCbcBlockCipher for Aes192Cbc {} /// 256-bit AES-CBC cipher -pub struct Aes256Cbc<'a> { - key: &'a [u8], - iv: &'a [u8], +pub struct Aes256Cbc { + context: AES256_CBC_ctx, } -impl<'a> Aes256Cbc<'a> { - pub fn new(key: &'a [u8], iv: &'a [u8]) -> Result { +impl Aes256Cbc { + pub fn new(key: &[u8], iv: &[u8]) -> Result { if key.len() != AES256_KEY_LENGTH { return Err(Error::KeyIncorrectLength(AES256_KEY_LENGTH)); } @@ -365,17 +391,19 @@ impl<'a> Aes256Cbc<'a> { return Err(Error::IvIncorrectLength); } - Ok(Self { key, iv }) + let mut context = AES256_CBC_ctx::default(); + unsafe { + AES256_CBC_init(&mut context, key.as_ptr(), iv.as_ptr()); + } + Ok(Self { context }) } } -impl FfiAesCbcCipher for Aes256Cbc<'_> { - fn ffi_decrypt(&self, num_blocks: usize, plaintext: &mut [u8], ciphertext: &[u8]) { - let mut context = AES256_CBC_ctx::default(); +impl FfiAesCbcCipher for Aes256Cbc { + fn ffi_decrypt(&mut self, num_blocks: usize, plaintext: &mut [u8], ciphertext: &[u8]) { unsafe { - AES256_CBC_init(&mut context, self.key.as_ptr(), self.iv.as_ptr()); AES256_CBC_decrypt( - &mut context, + &mut self.context, num_blocks, plaintext.as_mut_ptr(), ciphertext.as_ptr(), @@ -383,12 +411,10 @@ impl FfiAesCbcCipher for Aes256Cbc<'_> { } } - fn ffi_encrypt(&self, num_blocks: usize, ciphertext: &mut [u8], plaintext: &[u8]) { - let mut context = AES256_CBC_ctx::default(); + fn ffi_encrypt(&mut self, num_blocks: usize, ciphertext: &mut [u8], plaintext: &[u8]) { unsafe { - AES256_CBC_init(&mut context, self.key.as_ptr(), self.iv.as_ptr()); AES256_CBC_encrypt( - &mut context, + &mut self.context, num_blocks, ciphertext.as_mut_ptr(), plaintext.as_ptr(), @@ -397,7 +423,7 @@ impl FfiAesCbcCipher for Aes256Cbc<'_> { } } -impl AesCbcBlockCipher for Aes256Cbc<'_> {} +impl AesCbcBlockCipher for Aes256Cbc {} #[cfg(test)] mod test { @@ -467,7 +493,7 @@ mod test { Ok(()) )); - let cipher = Aes128Cbc::new([0u8; 16].as_slice(), [0u8; 16].as_slice()).unwrap(); + let mut cipher = Aes128Cbc::new([0u8; 16].as_slice(), [0u8; 16].as_slice()).unwrap(); assert!(matches!( cipher.encrypt([0u8; 33].as_slice(), [0u8; 33].as_mut_slice()), Err(Error::NonBlockSizeAlignedBuffer) diff --git a/src/padding.rs b/src/padding.rs index b613766..949e542 100644 --- a/src/padding.rs +++ b/src/padding.rs @@ -2,13 +2,14 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or https://opensource.org/licenses/mit-license.php. -use crate::Error; +use crate::constant_time::{ConstantTimeEq, ConstantTimeLess}; +use crate::{Error, AES_BLOCK_SIZE}; /// Trait defining interface for a Padding implementation pub trait Padding { /// Given a length of data and block_size return the minimum size of a buffer required to hold /// the padded data - fn padded_buffer_length(data_length: usize, block_size: usize) -> usize; + fn padded_buffer_length(data_length: usize, block_size: usize) -> Result; /// Take a buffer containing data of specified length and based on the blocksize apply the padding /// to the buffer. The returned slice will reference the slice of padded data in the buffer fn pad(buffer: &mut [u8], data_length: usize, block_size: usize) -> Result<&[u8], Error>; @@ -23,16 +24,17 @@ pub trait Padding { pub enum ZeroPadding {} impl Padding for ZeroPadding { - fn padded_buffer_length(data_length: usize, block_size: usize) -> usize { - if data_length % block_size == 0 { - data_length - } else { - block_size + block_size * (data_length / block_size) + fn padded_buffer_length(data_length: usize, block_size: usize) -> Result { + if block_size == 0 { + return Err(Error::InvalidBlockSize); } + let remainder = data_length % block_size; + let padding_length = if remainder == 0 { 0 } else { block_size - remainder }; + data_length.checked_add(padding_length).ok_or(Error::PaddedLengthOverflow) } fn pad(buffer: &mut [u8], data_length: usize, block_size: usize) -> Result<&[u8], Error> { - let padded_length = Self::padded_buffer_length(data_length, block_size); + let padded_length = Self::padded_buffer_length(data_length, block_size)?; if buffer.len() < padded_length { return Err(Error::PaddedBufferTooSmall); } @@ -44,28 +46,85 @@ impl Padding for ZeroPadding { Ok(&buffer[0..padded_length]) } + /// Remove trailing zero padding. Empty input returns an unpadding error. fn unpad(buffer: &[u8]) -> Result<&[u8], Error> { - let mut n = buffer.len() - 1; - while n > 0 { - if buffer[n] != 0 { - break; - } - n -= 1; + if buffer.is_empty() { + return Err(Error::UnpadError("Buffer to unpad is empty".to_string())); } - Ok(&buffer[0..=n]) + + let mut end = buffer.len(); + while end > 0 && buffer[end - 1] == 0 { + end -= 1; + } + Ok(&buffer[..end]) } } -/// Implementation of the PKCS7 padding scheme +/// Implementation of the PKCS7 padding scheme. +/// +/// Padding validation does not authenticate a message. Verify its MAC before +/// decrypting or unpadding, and never expose padding validity for unauthenticated +/// input. `Padding::unpad` enforces the AES block size (16 bytes); use +/// `unpad_with_block_size` for other block sizes. pub enum Pkcs7 {} +impl Pkcs7 { + /// Validate PKCS7 padding, including block alignment and padding length bounds. + /// + /// For AES, pass 16. The block size must be in 1..=255. + /// All bytes in the final block are checked using constant-time comparisons + /// before returning padding validity. This is not a substitute for a MAC. + pub fn unpad_with_block_size(buffer: &[u8], block_size: usize) -> Result<&[u8], Error> { + if block_size == 0 + || block_size > 255 + || buffer.is_empty() + || buffer.len() % block_size != 0 + { + return Err(Self::invalid_padding()); + } + Self::validate_suffix(buffer, block_size) + } + + fn invalid_padding() -> Error { + Error::UnpadError("Invalid PKCS7 padding".to_string()) + } + + fn validate_suffix(buffer: &[u8], scan_length: usize) -> Result<&[u8], Error> { + if buffer.is_empty() { + return Err(Self::invalid_padding()); + } + let padding_length = buffer[buffer.len() - 1]; + let pad = u16::from(padding_length); + // The padding length must be nonzero and must not exceed the scan length. + let mut valid = !padding_length.ct_eq(&0) & !(scan_length as u16).ct_lt(&pad); + + // The scan length and addresses depend only on public buffer/block sizes. + // Accumulate all mismatches without branching on decrypted byte values. + for (offset, byte) in buffer.iter().rev().take(scan_length).enumerate() { + let is_padding = (offset as u16).ct_lt(&pad); + valid &= !is_padding | byte.ct_eq(&padding_length); + } + + // Only the final validity decision and returned length depend on padding. + if bool::from(valid) { + Ok(&buffer[..buffer.len() - padding_length as usize]) + } else { + Err(Self::invalid_padding()) + } + } +} + impl Padding for Pkcs7 { - fn padded_buffer_length(data_length: usize, block_size: usize) -> usize { - block_size + block_size * (data_length / block_size) + fn padded_buffer_length(data_length: usize, block_size: usize) -> Result { + if block_size == 0 || block_size > 255 { + return Err(Error::InvalidBlockSize); + } + let padding_length = block_size - data_length % block_size; + data_length.checked_add(padding_length).ok_or(Error::PaddedLengthOverflow) } fn pad(buffer: &mut [u8], data_length: usize, block_size: usize) -> Result<&[u8], Error> { - let padded_length = Self::padded_buffer_length(data_length, block_size); + let padded_length = Self::padded_buffer_length(data_length, block_size)?; if buffer.len() < padded_length { return Err(Error::PaddedBufferTooSmall); } @@ -76,21 +135,12 @@ impl Padding for Pkcs7 { Ok(&buffer[0..padded_length]) } + /// Validate and remove PKCS7 padding for AES's 16-byte block size. + /// + /// Requires nonempty, block-aligned input and 1..=16 matching padding bytes. + /// For data padded with another block size, use `unpad_with_block_size`. fn unpad(buffer: &[u8]) -> Result<&[u8], Error> { - let padding_length = match buffer.last() { - None => return Err(Error::UnpadError("Buffer to unpad is empty".to_string())), - Some(l) => *l, - }; - if buffer.len() < padding_length as usize { - return Err(Error::UnpadError( - "Buffer smaller than PKCS7 recorded padded length".to_string(), - )); - } - if padding_length == 0 { - return Err(Error::UnpadError("PKCS7 padding length can't be zero".to_string())); - } - - Ok(&buffer[0..buffer.len() - padding_length as usize]) + Self::unpad_with_block_size(buffer, AES_BLOCK_SIZE) } } @@ -99,11 +149,35 @@ mod test { use crate::padding::{Padding, Pkcs7, ZeroPadding}; use crate::Error; + #[test] + fn test_zero_padding_unpad_empty() { + assert!(matches!(ZeroPadding::unpad(&[]), Err(Error::UnpadError(_)))); + } + + #[test] + fn test_zero_padding_unpad_boundaries() { + for length in 1..=32 { + let mut buffer = vec![0; length]; + assert_eq!(ZeroPadding::unpad(&buffer).unwrap(), &[]); + + // Preserve the last nonzero byte at every possible position, + // including the first byte and a buffer with no trailing padding. + for position in 0..length { + buffer[position] = 0x42; + assert_eq!(ZeroPadding::unpad(&buffer).unwrap(), &buffer[..position + 1]); + buffer[position] = 0; + } + } + } + #[test] fn test_zero_padding() { const BLOCK_SIZE: usize = 16; - assert_eq!(ZeroPadding::padded_buffer_length(20, BLOCK_SIZE), 2 * BLOCK_SIZE); - assert_eq!(ZeroPadding::padded_buffer_length(2 * BLOCK_SIZE, BLOCK_SIZE), 2 * BLOCK_SIZE); + assert_eq!(ZeroPadding::padded_buffer_length(20, BLOCK_SIZE).unwrap(), 2 * BLOCK_SIZE); + assert_eq!( + ZeroPadding::padded_buffer_length(2 * BLOCK_SIZE, BLOCK_SIZE).unwrap(), + 2 * BLOCK_SIZE + ); const DATA_LENGTH: usize = 22; let mut buffer = [2u8; 2 * BLOCK_SIZE + 1]; @@ -131,11 +205,35 @@ mod test { )); } + #[test] + fn test_pkcs7_unpad_invalid() { + let is_invalid = |buffer: &[u8]| matches!(Pkcs7::unpad(buffer), Err(Error::UnpadError(_))); + let mut buffer = [0u8; 16]; + // Zero padding length + assert!(is_invalid(&buffer)); + // Padding length larger than block size + buffer[15] = 17; + assert!(is_invalid(&buffer)); + // Mismatched padding byte + buffer[12..].fill(4); + buffer[12] = 3; + assert!(is_invalid(&buffer)); + // Valid padding of every length + for padding_length in 1..=16u8 { + buffer.fill(0xaa); + buffer[16 - padding_length as usize..].fill(padding_length); + assert_eq!(Pkcs7::unpad(&buffer).unwrap().len(), 16 - padding_length as usize); + } + } + #[test] fn test_pkcs7_padding() { const BLOCK_SIZE: usize = 16; - assert_eq!(Pkcs7::padded_buffer_length(20, BLOCK_SIZE), 2 * BLOCK_SIZE); - assert_eq!(Pkcs7::padded_buffer_length(2 * BLOCK_SIZE, BLOCK_SIZE), 3 * BLOCK_SIZE); + assert_eq!(Pkcs7::padded_buffer_length(20, BLOCK_SIZE).unwrap(), 2 * BLOCK_SIZE); + assert_eq!( + Pkcs7::padded_buffer_length(2 * BLOCK_SIZE, BLOCK_SIZE).unwrap(), + 3 * BLOCK_SIZE + ); const DATA_LENGTH: usize = 22; let mut buffer = [2u8; 2 * BLOCK_SIZE + 1]; diff --git a/tests/decrypt_buffers.rs b/tests/decrypt_buffers.rs new file mode 100644 index 0000000..d971bab --- /dev/null +++ b/tests/decrypt_buffers.rs @@ -0,0 +1,72 @@ +use ctaes_rs::{ + Aes128, Aes128Cbc, Aes192, Aes192Cbc, Aes256, Aes256Cbc, AesBlockCipher, AesCbcBlockCipher, + Error, AES_BLOCK_SIZE, +}; + +fn check_decrypt_buffers(encrypt: E, decrypt: D) +where + E: Fn(&[u8], &mut [u8]) -> Result<(), Error>, + D: Fn(&[u8], &mut [u8]) -> Result<(), Error>, +{ + const SENTINEL: u8 = 0xa5; + + // Include empty input, the reported one-block input with four blocks of + // output space, and multiple blocks. Extra output need not be block aligned. + for &input_len in &[0, AES_BLOCK_SIZE, 2 * AES_BLOCK_SIZE] { + let expected: Vec = (0..input_len).map(|i| i as u8).collect(); + let mut ciphertext = vec![0; input_len]; + encrypt(&expected, &mut ciphertext).unwrap(); + + for &extra in &[0, 1, AES_BLOCK_SIZE, 3 * AES_BLOCK_SIZE, 1024] { + let mut plaintext = vec![SENTINEL; input_len + extra]; + decrypt(&ciphertext, &mut plaintext).unwrap(); + assert_eq!(&plaintext[..input_len], expected.as_slice()); + assert!(plaintext[input_len..].iter().all(|&b| b == SENTINEL)); + } + } + + // XF-05: reject every partial-block length regardless of output capacity, + // including a one-byte ciphertext with 16-byte and 1024-byte output buffers. + for input_len in 1..2 * AES_BLOCK_SIZE { + if input_len % AES_BLOCK_SIZE == 0 { + continue; + } + let ciphertext = vec![0xaa; input_len]; + let rounded_len = (input_len / AES_BLOCK_SIZE + 1) * AES_BLOCK_SIZE; + for &output_len in &[rounded_len, rounded_len + 1, 1024] { + let mut plaintext = vec![SENTINEL; output_len]; + assert!(matches!( + decrypt(&ciphertext, &mut plaintext), + Err(Error::NonBlockSizeAlignedBuffer) + )); + assert!(plaintext.iter().all(|&b| b == SENTINEL)); + } + } + + // Reject undersized output without writing to it, regardless of alignment. + let ciphertext = [0; 2 * AES_BLOCK_SIZE]; + for &output_len in &[0, 1, AES_BLOCK_SIZE, 2 * AES_BLOCK_SIZE - 1] { + let mut plaintext = vec![SENTINEL; output_len]; + assert!(matches!(decrypt(&ciphertext, &mut plaintext), Err(Error::InsufficientBufferSize))); + assert!(plaintext.iter().all(|&b| b == SENTINEL)); + } +} + +macro_rules! decrypt_buffer_test { + ($name:ident, $cipher:expr) => { + #[test] + fn $name() { + check_decrypt_buffers( + |plaintext, ciphertext| $cipher.unwrap().encrypt(plaintext, ciphertext), + |ciphertext, plaintext| $cipher.unwrap().decrypt(ciphertext, plaintext), + ); + } + }; +} + +decrypt_buffer_test!(aes128_decrypt_buffers, Aes128::new(&[0; 16])); +decrypt_buffer_test!(aes192_decrypt_buffers, Aes192::new(&[0; 24])); +decrypt_buffer_test!(aes256_decrypt_buffers, Aes256::new(&[0; 32])); +decrypt_buffer_test!(aes128_cbc_decrypt_buffers, Aes128Cbc::new(&[0; 16], &[0; 16])); +decrypt_buffer_test!(aes192_cbc_decrypt_buffers, Aes192Cbc::new(&[0; 24], &[0; 16])); +decrypt_buffer_test!(aes256_cbc_decrypt_buffers, Aes256Cbc::new(&[0; 32], &[0; 16])); diff --git a/tests/padding_lengths.rs b/tests/padding_lengths.rs new file mode 100644 index 0000000..225aabb --- /dev/null +++ b/tests/padding_lengths.rs @@ -0,0 +1,57 @@ +use ctaes_rs::{Error, Padding, Pkcs7, ZeroPadding}; + +fn check_zero_block_size() { + for &length in &[0, 1, 16, usize::MAX] { + assert!(matches!(P::padded_buffer_length(length, 0), Err(Error::InvalidBlockSize))); + let mut buffer = [0xa5; 32]; + assert!(matches!(P::pad(&mut buffer, length, 0), Err(Error::InvalidBlockSize))); + assert_eq!(buffer, [0xa5; 32]); + } +} + +#[test] +fn zero_block_size_returns_errors() { + check_zero_block_size::(); + check_zero_block_size::(); +} + +#[test] +fn pkcs7_rejects_unrepresentable_block_sizes() { + for &size in &[256, usize::MAX] { + assert!(matches!(Pkcs7::padded_buffer_length(0, size), Err(Error::InvalidBlockSize))); + let mut buffer = [0xa5; 512]; + assert!(matches!(Pkcs7::pad(&mut buffer, 0, size), Err(Error::InvalidBlockSize))); + assert_eq!(buffer, [0xa5; 512]); + } +} + +fn check_overflow() { + assert!(matches!(P::padded_buffer_length(usize::MAX, 16), Err(Error::PaddedLengthOverflow))); + let mut buffer = [0xa5; 16]; + assert!(matches!(P::pad(&mut buffer, usize::MAX, 16), Err(Error::PaddedLengthOverflow))); + assert_eq!(buffer, [0xa5; 16]); +} + +#[test] +fn padded_length_overflow_returns_errors() { + check_overflow::(); + check_overflow::(); + // Zero padding need not grow an already aligned buffer. + assert_eq!(ZeroPadding::padded_buffer_length(usize::MAX, 1).unwrap(), usize::MAX); + assert!(matches!(Pkcs7::padded_buffer_length(usize::MAX, 1), Err(Error::PaddedLengthOverflow))); +} + +#[test] +fn valid_lengths_keep_padding_semantics() { + for size in 1..=255 { + for length in 0..=2 * size { + let zero = ZeroPadding::padded_buffer_length(length, size).unwrap(); + let pkcs7 = Pkcs7::padded_buffer_length(length, size).unwrap(); + assert_eq!(zero % size, 0); + assert!(zero >= length && zero - length < size); + assert_eq!(pkcs7 % size, 0); + assert!(pkcs7 > length && pkcs7 - length <= size); + } + } + assert_eq!(ZeroPadding::padded_buffer_length(1, usize::MAX).unwrap(), usize::MAX); +} diff --git a/tests/pkcs7_validation.rs b/tests/pkcs7_validation.rs new file mode 100644 index 0000000..2cdad66 --- /dev/null +++ b/tests/pkcs7_validation.rs @@ -0,0 +1,116 @@ +use ctaes_rs::{Error, Padding, Pkcs7}; + +fn assert_invalid(result: Result<&[u8], Error>) { + match result { + Err(Error::UnpadError(message)) => assert_eq!(message, "Invalid PKCS7 padding"), + other => panic!("expected uniform padding error, got {:?}", other), + } +} + +#[test] +fn aes_padding_lengths_and_corrupted_suffixes() { + for padding in 1..=16 { + let mut buffer = [0x42; 32]; + buffer[32 - padding..].fill(padding as u8); + assert_eq!(Pkcs7::unpad_with_block_size(&buffer, 16).unwrap(), &buffer[..32 - padding]); + assert_eq!(Pkcs7::unpad(&buffer).unwrap(), &buffer[..32 - padding]); + + // Corrupt each claimed padding byte except the length byte. A changed + // length byte can describe a different, legitimately valid suffix. + for position in 32 - padding..31 { + for replacement in 0..=255u8 { + if replacement == padding as u8 { + continue; + } + let mut corrupted = buffer; + corrupted[position] = replacement; + assert_invalid(Pkcs7::unpad_with_block_size(&corrupted, 16)); + assert_invalid(Pkcs7::unpad(&corrupted)); + } + } + } +} + +#[test] +fn invalid_lengths_and_block_sizes() { + assert_invalid(Pkcs7::unpad(&[])); + assert_invalid(Pkcs7::unpad(&[0])); + assert_invalid(Pkcs7::unpad(&[2])); + assert_invalid(Pkcs7::unpad_with_block_size(&[], 16)); + for size in &[0, 256, usize::MAX] { + assert_invalid(Pkcs7::unpad_with_block_size(&[16; 16], *size)); + } + for length in 1..32 { + if length % 16 != 0 { + assert_invalid(Pkcs7::unpad_with_block_size(&vec![1; length], 16)); + } + } + for last in 0..=255u8 { + if last == 0 || last > 16 { + let mut buffer = [0; 32]; + buffer[31] = last; + assert_invalid(Pkcs7::unpad_with_block_size(&buffer, 16)); + } + } + // Even a matching suffix is invalid when it exceeds the AES block size. + assert_invalid(Pkcs7::unpad_with_block_size(&[32; 32], 16)); +} + +#[test] +fn generic_block_sizes_and_full_padding_blocks() { + for size in 1..=255 { + let mut buffer = vec![0x42; 2 * size]; + Pkcs7::pad(&mut buffer, size, size).unwrap(); + assert_eq!(Pkcs7::unpad_with_block_size(&buffer, size).unwrap(), &buffer[..size]); + if size == 16 { + assert_eq!(Pkcs7::unpad(&buffer).unwrap(), &buffer[..size]); + } + assert_eq!(Pkcs7::unpad_with_block_size(&buffer[size..], size).unwrap(), &[]); + } +} + +#[test] +fn maximum_padding_checks_entire_suffix() { + let mut buffer = [0x42; 510]; + buffer[255..].fill(255); + for position in 255..509 { + buffer[position] = 0; + assert_invalid(Pkcs7::unpad_with_block_size(&buffer, 255)); + buffer[position] = 255; + } +} + +#[test] +fn data_before_padding_is_preserved() { + // Padding validates only the suffix, not the authenticity or meaning of data. + for padding in 1..=16 { + let mut buffer = [0; 32]; + buffer[32 - padding..].fill(padding as u8); + for value in 0..=255u8 { + buffer[31 - padding] = value; + assert_eq!(Pkcs7::unpad(&buffer).unwrap(), &buffer[..32 - padding]); + assert_eq!(Pkcs7::unpad_with_block_size(&buffer, 16).unwrap(), &buffer[..32 - padding]); + } + } +} + +#[test] +fn default_unpad_rejects_padding_larger_than_aes_block() { + assert_invalid(Pkcs7::unpad(&[32; 32])); + // Matching suffix bytes must not bypass the AES padding-length limit. + for padding in 17..=255 { + let mut buffer = [0x42; 256]; + buffer[256 - padding..].fill(padding as u8); + assert_invalid(Pkcs7::unpad(&buffer)); + } +} + +#[test] +fn default_unpad_requires_aes_block_alignment() { + for length in 1..32 { + if length % 16 != 0 { + assert_invalid(Pkcs7::unpad(&vec![1; length])); + } + } + assert_eq!(Pkcs7::unpad(&[16; 16]).unwrap(), &[]); +} diff --git a/tests/tests.rs b/tests/tests.rs index 9bc51e9..2425381 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -1,8 +1,8 @@ /********************************************************************* -* Copyright (c) 2023 Blockstream * -* Distributed under the MIT software license, see the accompanying * -* file COPYING or https://opensource.org/licenses/mit-license.php. * -**********************************************************************/ + * Copyright (c) 2023 Blockstream * + * Distributed under the MIT software license, see the accompanying * + * file COPYING or https://opensource.org/licenses/mit-license.php. * + **********************************************************************/ extern crate hex_conservative; @@ -186,32 +186,121 @@ fn test_aes_cbc_ciphers() { for test_vector in aes_cbc_test_vectors() { match test_vector.key.len() { 16 => { - let cipher = - Aes128Cbc::new(test_vector.key.as_slice(), test_vector.iv.as_slice()).unwrap(); - test_cbc_cipher(cipher, &test_vector.plaintext, &test_vector.ciphertext); + test_cbc_cipher( + || Aes128Cbc::new(&test_vector.key, &test_vector.iv).unwrap(), + &test_vector.plaintext, + &test_vector.ciphertext, + ); } 24 => { - let cipher = - Aes192Cbc::new(test_vector.key.as_slice(), test_vector.iv.as_slice()).unwrap(); - test_cbc_cipher(cipher, &test_vector.plaintext, &test_vector.ciphertext); + test_cbc_cipher( + || Aes192Cbc::new(&test_vector.key, &test_vector.iv).unwrap(), + &test_vector.plaintext, + &test_vector.ciphertext, + ); } 32 => { - let cipher = - Aes256Cbc::new(test_vector.key.as_slice(), test_vector.iv.as_slice()).unwrap(); - test_cbc_cipher(cipher, &test_vector.plaintext, &test_vector.ciphertext); + test_cbc_cipher( + || Aes256Cbc::new(&test_vector.key, &test_vector.iv).unwrap(), + &test_vector.plaintext, + &test_vector.ciphertext, + ); } _ => panic!("Invalid key length"), }; } } -fn test_cbc_cipher(cipher: T, plaintext: &Vec, ciphertext: &Vec) { +fn test_cbc_cipher T>( + new: F, + plaintext: &Vec, + ciphertext: &Vec, +) { + check_cbc_stream(&new, plaintext, ciphertext); + let mut cipher = new(); + let mut decipher = new(); let mut ciphered = vec![0u8; plaintext.len()]; let mut deciphered = vec![0u8; plaintext.len()]; cipher.encrypt(plaintext.as_slice(), ciphered.as_mut_slice()).unwrap(); assert_eq!(ciphered.as_slice(), ciphertext.as_slice()); - cipher.decrypt(ciphered.as_slice(), deciphered.as_mut_slice()).unwrap(); + decipher.decrypt(ciphered.as_slice(), deciphered.as_mut_slice()).unwrap(); assert_eq!(deciphered.as_slice(), plaintext.as_slice()); } + +// Exercise every block boundary against the known-answer vectors above, including +// multi-block calls whose next IV must be the last (not first) ciphertext block. +fn check_cbc_stream T>( + new: F, + plaintext: &[u8], + ciphertext: &[u8], +) { + // Process two chunks at every block boundary, including empty end chunks. + for split in (0..=plaintext.len()).step_by(16) { + let mut encryptor = new(); + let mut decryptor = new(); + let mut encrypted = vec![0; plaintext.len()]; + let mut decrypted = vec![0; plaintext.len()]; + // Each direction keeps its own chain across both calls. + for &(start, end) in &[(0, split), (split, plaintext.len())] { + // Neither empty nor invalid calls may alter the chain or output. + for cipher in &mut [&mut encryptor, &mut decryptor] { + let mut untouched = [0xa5; 17]; + cipher.encrypt(&[], &mut untouched).unwrap(); + cipher.decrypt(&[], &mut untouched).unwrap(); + assert!(matches!( + cipher.encrypt(&[0; 1], &mut untouched), + Err(ctaes_rs::Error::NonBlockSizeAlignedBuffer) + )); + assert!(matches!( + cipher.decrypt(&[0; 1], &mut untouched), + Err(ctaes_rs::Error::NonBlockSizeAlignedBuffer) + )); + assert!(matches!( + cipher.encrypt(&[0; 32], &mut untouched), + Err(ctaes_rs::Error::InsufficientBufferSize) + )); + assert!(matches!( + cipher.decrypt(&[0; 32], &mut untouched), + Err(ctaes_rs::Error::InsufficientBufferSize) + )); + assert_eq!(untouched, [0xa5; 17]); + } + // Extra output bytes stay untouched and must not affect IV advancement. + let mut output = vec![0xa5; end - start + 17]; + encryptor.encrypt(&plaintext[start..end], &mut output).unwrap(); + encrypted[start..end].copy_from_slice(&output[..end - start]); + assert!(output[end - start..].iter().all(|&b| b == 0xa5)); + decryptor.decrypt(&ciphertext[start..end], &mut output).unwrap(); + decrypted[start..end].copy_from_slice(&output[..end - start]); + assert!(output[end - start..].iter().all(|&b| b == 0xa5)); + } + // Chunked results must match the known-answer vectors. + assert_eq!(encrypted, ciphertext); + assert_eq!(decrypted, plaintext); + } + + // Verify chaining over several consecutive single-block calls. + let mut encryptor = new(); + let mut decryptor = new(); + for (plain, encrypted) in plaintext.chunks(16).zip(ciphertext.chunks(16)) { + let mut output = [0; 16]; + encryptor.encrypt(plain, &mut output).unwrap(); + assert_eq!(output.as_ref(), encrypted); + decryptor.decrypt(encrypted, &mut output).unwrap(); + assert_eq!(output.as_ref(), plain); + } + + // Identical plaintext blocks must match one-shot encryption across calls, + // while producing different ciphertext blocks for this test input. + let repeated = [0x42; 32]; + let mut expected = [0; 32]; + new().encrypt(&repeated, &mut expected).unwrap(); + let mut encryptor = new(); + let mut actual = [0; 32]; + encryptor.encrypt(&repeated[..16], &mut actual[..16]).unwrap(); + encryptor.encrypt(&repeated[16..], &mut actual[16..]).unwrap(); + assert_eq!(actual, expected); + assert_ne!(actual[..16], actual[16..]); +} From 032adc5bad343ec610ab16e91f63b1d8737c3ed2 Mon Sep 17 00:00:00 2001 From: Philip Robinson Date: Fri, 2 Oct 2026 16:34:50 +0100 Subject: [PATCH 2/3] Bump version to 0.2.0 --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index b2996ae..4e66594 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "ctaes-rs" -version = "0.1.2" +version = "0.2.0" authors = ["Philip Robinson "] license = "MIT AND BSD-3-Clause" homepage = "https://github.com/ElementsProject/rust-ctaes/" From 2e2aee46a12f2ee50ba0e4de7990b87f51e466aa Mon Sep 17 00:00:00 2001 From: Philip Robinson Date: Fri, 30 Oct 2026 14:39:42 +0000 Subject: [PATCH 3/3] fix MSRV github action setup --- .github/workflows/rust.yml | 30 ++++++++++++++++++++---------- Cargo.toml | 1 + 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index f6bd199..e56e095 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -42,24 +42,34 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - + + # cargo-valgrind's dependencies no longer build on the MSRV, so install + # the tool with stable and only use 1.63.0 for building/testing the crate. + - name: "Select stable toolchain for tooling" + uses: dtolnay/rust-toolchain@stable + + - name: Setup + run: | + sudo apt-get update && sudo apt-get install -y valgrind + cargo install cargo-valgrind + + # Use stable cargo's MSRV-aware resolver (driven by `rust-version` in + # Cargo.toml) to pick dependency versions that build on 1.63.0. + - name: "Generate MSRV-compatible lockfile" + run: cargo generate-lockfile + env: + CARGO_RESOLVER_INCOMPATIBLE_RUST_VERSIONS: fallback + - name: "Select toolchain" uses: dtolnay/rust-toolchain@1.63.0 - name: "Rust Version" run: rustc --version - - name: Setup - run: | - sudo apt-get update && sudo apt-get install -y valgrind - cargo install cargo-valgrind --version 2.0.0 - - name: Run tests run: | - rm Cargo.lock - cargo +1.63.0 generate-lockfile - cargo valgrind test - cargo test + cargo valgrind test --locked + cargo test --locked - name: Upload artifacts uses: actions/upload-artifact@v4 diff --git a/Cargo.toml b/Cargo.toml index 4e66594..39f1666 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,6 +9,7 @@ description = "Rust bindings and API for CTAES a constant-time AES implementatio keywords = [ "crypto", "bitcoin", "aes" ] readme = "README.md" edition = "2021" +rust-version = "1.63" [dependencies] thiserror = "2"