From b8bee85734e8948939e686fbfdfeb803c18970f8 Mon Sep 17 00:00:00 2001 From: "N." Date: Sun, 27 Sep 2026 10:06:53 +0200 Subject: [PATCH 1/2] Fix RSA constant-time padding and length checks This change removes variable-time padding behavior by writing into fixed-width buffers instead of slicing based on secret leading-zero counts. It also enforces the RFC 8017 ciphertext-length check before PKCS#1 v1.5 decryption and avoids variable-time Montgomery reduction on private-key paths. The patch adds regression tests covering padding edge cases, PKCS#1 v1.5 length rejection, and Montgomery reduction correctness. --- src/algorithms/pad.rs | 84 +++++++++++++++++++++++--------------- src/algorithms/pkcs1v15.rs | 12 +++--- src/algorithms/rsa.rs | 76 ++++++++++++++++++++++++++++++---- src/pkcs1v15.rs | 29 +++++++++++-- src/pss.rs | 6 +-- 5 files changed, 152 insertions(+), 55 deletions(-) diff --git a/src/algorithms/pad.rs b/src/algorithms/pad.rs index c84ab127..faf90f81 100644 --- a/src/algorithms/pad.rs +++ b/src/algorithms/pad.rs @@ -2,62 +2,78 @@ use alloc::vec::Vec; use crypto_bigint::BoxedUint; -use zeroize::Zeroizing; +use zeroize::{Zeroize, Zeroizing}; use crate::errors::{Error, Result}; -/// Returns a new vector of the given length, with 0s left padded. +/// Writes the big-endian encoding of `bytes` into a buffer of exactly `padded_len` bytes +/// +/// Constant-time with respect to the *value*: the only branches depend on the public lengths +/// The previous implementation sliced at `leading_zeros() / 8`, so the amount of data copied (and +/// whether an error was returned) depended on how many leading zero bytes the secret plaintext +/// had, which is exactly the signal the Marvin attack measures #[inline] -fn left_pad(input: &[u8], padded_len: usize) -> Result> { - if input.len() > padded_len { - return Err(Error::InvalidPadLen); +fn be_pad_into(bytes: &[u8], out: &mut [u8]) -> Result<()> { + let padded_len = out.len(); + if bytes.len() >= padded_len { + let (hi, lo) = bytes.split_at(bytes.len() - padded_len); + let overflow = hi.iter().fold(0u8, |acc, b| acc | b); + out.copy_from_slice(lo); + if core::hint::black_box(overflow) != 0 { + out.zeroize(); + return Err(Error::InvalidPadLen); + } + } else { + let (zero, tail) = out.split_at_mut(padded_len - bytes.len()); + zero.fill(0); + tail.copy_from_slice(bytes); } - - let mut out = vec![0u8; padded_len]; - out[padded_len - input.len()..].copy_from_slice(input); - Ok(out) + Ok(()) } /// Converts input to the new vector of the given length, using BE and with 0s left padded. -/// In some cases BoxedUint might already have leading zeroes, this function removes them -/// before padding again. #[inline] pub(crate) fn uint_to_be_pad(input: BoxedUint, padded_len: usize) -> Result> { - let leading_zeros = input.leading_zeros() as usize / 8; - left_pad(&input.to_be_bytes()[leading_zeros..], padded_len) + let mut out = vec![0u8; padded_len]; + be_pad_into(&input.to_be_bytes(), &mut out)?; + Ok(out) } /// Converts input to the new vector of the given length, using BE and with 0s left padded. -/// In some cases BoxedUint might already have leading zeroes, this function removes them -/// before padding again. +/// +/// For secret values: every intermediate buffer, including the returned one, is zeroized on #[inline] -pub(crate) fn uint_to_zeroizing_be_pad(input: BoxedUint, padded_len: usize) -> Result> { - let leading_zeros = input.leading_zeros() as usize / 8; - - let m = Zeroizing::new(input); - let m = Zeroizing::new(m.to_be_bytes()); - - left_pad(&m[leading_zeros..], padded_len) +pub(crate) fn uint_to_zeroizing_be_pad( + input: BoxedUint, + padded_len: usize, +) -> Result>> { + let input = Zeroizing::new(input); + let bytes = Zeroizing::new(input.to_be_bytes()); + let mut out = Zeroizing::new(vec![0u8; padded_len]); + be_pad_into(&bytes, &mut out)?; + Ok(out) } #[cfg(test)] mod tests { use super::*; #[test] - fn test_left_pad() { - const INPUT_LEN: usize = 3; - let input = vec![0u8; INPUT_LEN]; + fn be_pad() { + let x = BoxedUint::from(0x0102u64); + let p = uint_to_be_pad(x.clone(), 16).unwrap(); + assert_eq!(&p[..14], &[0u8; 14]); + assert_eq!(&p[14..], &[1, 2]); + + let p = uint_to_zeroizing_be_pad(x.clone(), 3).unwrap(); + assert_eq!(&p[..], &[0, 1, 2]); - // input len < padded len - let padded = left_pad(&input, INPUT_LEN + 1).unwrap(); - assert_eq!(padded.len(), INPUT_LEN + 1); + let p = uint_to_be_pad(x.clone(), 8).unwrap(); + assert_eq!(&p[..], &[0, 0, 0, 0, 0, 0, 1, 2]); - // input len == padded len - let padded = left_pad(&input, INPUT_LEN).unwrap(); - assert_eq!(padded.len(), INPUT_LEN); + assert!(uint_to_be_pad(x.clone(), 1).is_err()); + assert!(uint_to_zeroizing_be_pad(x, 1).is_err()); - // input len > padded len - let padded = left_pad(&input, INPUT_LEN - 1); - assert!(padded.is_err()); + let p = uint_to_be_pad(BoxedUint::zero_with_precision(128), 4).unwrap(); + assert_eq!(&p[..], &[0u8; 4]); } } diff --git a/src/algorithms/pkcs1v15.rs b/src/algorithms/pkcs1v15.rs index 346d7947..00219269 100644 --- a/src/algorithms/pkcs1v15.rs +++ b/src/algorithms/pkcs1v15.rs @@ -67,13 +67,13 @@ where /// forge signatures as if they had the private key. See /// `decrypt_session_key` for a way of solving this problem. #[inline] -pub(crate) fn pkcs1v15_encrypt_unpad(em: Vec, k: usize) -> Result> { - let (valid, out, index) = decrypt_inner(em, k)?; +pub(crate) fn pkcs1v15_encrypt_unpad(em: &[u8], k: usize) -> Result> { + let (valid, index) = decrypt_inner(em, k)?; if valid == 0 { return Err(Error::Decryption); } - Ok(out[index as usize..].to_vec()) + Ok(em[index as usize..].to_vec()) } /// Removes the PKCS1v15 padding It returns one or zero in valid that indicates whether the @@ -82,8 +82,8 @@ pub(crate) fn pkcs1v15_encrypt_unpad(em: Vec, k: usize) -> Result> { /// in order to maintain constant memory access patterns. If the plaintext was /// valid then index contains the index of the original message in em. #[inline] -fn decrypt_inner(em: Vec, k: usize) -> Result<(u8, Vec, u32)> { - if k < 11 { +fn decrypt_inner(em: &[u8], k: usize) -> Result<(u8, u32)> { + if k < 11 && em.len() != k { return Err(Error::Decryption); } @@ -113,7 +113,7 @@ fn decrypt_inner(em: Vec, k: usize) -> Result<(u8, Vec, u32)> { let valid = first_byte_is_zero & second_byte_is_two & !looking_for_index & valid_ps; index = u32::ct_select(&0, &(index + 1), valid); - Ok((valid.to_u8(), em, index)) + Ok((valid.to_u8(), index)) } #[inline] diff --git a/src/algorithms/rsa.rs b/src/algorithms/rsa.rs index 9c7a2567..19f064f6 100644 --- a/src/algorithms/rsa.rs +++ b/src/algorithms/rsa.rs @@ -231,28 +231,43 @@ fn unblind(m: &BoxedUint, unblinder: &BoxedUint, n_params: &BoxedMontyParams) -> } /// Computes `base.pow_mod(exp, n)` with precomputed `n_params`. +/// +/// Constant-time with respect to `base` and `exp` fn pow_mod_params(base: &BoxedUint, exp: &BoxedUint, n_params: &BoxedMontyParams) -> BoxedUint { - let base = reduce_vartime(base, n_params); - base.pow(exp).retrieve() + to_monty(base, n_params).pow(exp).retrieve() } /// Computes `base.pow_mod(exp, n)` with a bounded exponent and precomputed `n_params`. /// -/// The exponent bit length `exp_bits` may be leaked in the time pattern. +/// Constant-time with respect to `base`. The exponent bit length `exp_bits` may be leaked in the +/// time pattern fn pow_mod_params_vartime_exp_bits( base: &BoxedUint, exp: &BoxedUint, exp_bits: u32, n_params: &BoxedMontyParams, ) -> BoxedUint { - let base = reduce_vartime(base, n_params); - base.pow_bounded_exp(exp, exp_bits).retrieve() + to_monty(base, n_params) + .pow_bounded_exp(exp, exp_bits) + .retrieve() } -fn reduce_vartime(n: &BoxedUint, p: &BoxedMontyParams) -> BoxedMontyForm { - let modulus = p.modulus().as_nz_ref().clone(); - let n_reduced = n.rem_vartime(&modulus).resize_unchecked(p.bits_precision()); - BoxedMontyForm::new(n_reduced, p) +// Converts `x` into Montgomery form modulo `p` in constant time with respect to `x` +/// +/// This is on the private-key path: `rsa_decrypt_and_check` re-encrypts the unblinded plaintext +/// to detect CRT faults, and the non-CRT decrypt path feeds the ciphertext through here. A +/// variable-time remainder at this point leaks information about the plaintext (Marvin attack, +/// CVE-2023-49092 / RUSTSEC-2023-0071) +fn to_monty(x: &BoxedUint, p: &BoxedMontyParams) -> BoxedMontyForm { + let bits = p.bits_precision(); + let x = if x.bits_precision() <= bits { + // Montgomery conversion computes REDC(x * R^2 mod N). For any x < R = 2^bits the product + // is < R * N, so REDC already yields a fully reduced result and no division is needed + x.resize_unchecked(bits) + } else { + x.rem(p.modulus().as_nz_ref()).resize_unchecked(bits) + }; + BoxedMontyForm::new(x, p) } /// The following (deterministic) algorithm also recovers the prime factors `p` and `q` of a modulus `n`, given the @@ -458,4 +473,47 @@ mod tests { assert_eq!(p, p1); assert_eq!(q, q1); } + + #[test] + fn to_monty_matches_reference_reduction() { + use crypto_bigint::{Odd, Resize}; + + let n = Odd::new( + BoxedUint::from_be_hex( + concat!( + "d397b84d98a4c26138ed1b695a8106ead91d553bf06041b62d3fdc50a041e222", + "b8f4529689c1b82c5e71554f5dd69fa2f4b6158cf0dbeb57811a0fc327e1f28e", + "74fe74d3bc166c1eabdc1b8b57b934ca8be5b00b4f29975bcc99acaf415b59bb", + "28a6782bb41a2c3c2976b3c18dbadef62f00c6bb226640095096c0cc60d22fe7", + "ef987d75c6a81b10d96bf292028af110dc7cc1bbc43d22adab379a0cd5d8078c", + "c780ff5cd6209dea34c922cf784f7717e428d75b5aec8ff30e5f0141510766e2", + "e0ab8d473c84e8710b2b98227c3db095337ad3452f19e2b9bfbccdd8148abf67", + "76fa552775e6e75956e45229ae5a9c46949bab1e622f0e48f56524a84ed3483b" + ), + 2048, + ) + .unwrap(), + ) + .unwrap(); + let params = BoxedMontyParams::new(n.clone()); + let nz = n.as_nz_ref(); + + let candidates = [ + BoxedUint::zero_with_precision(2048), + BoxedUint::one_with_precision(2048), + n.as_ref() - &BoxedUint::one(), + n.as_ref().clone(), + n.as_ref().wrapping_add(BoxedUint::one()), + BoxedUint::max(2048), + BoxedUint::from(12345u64), + BoxedUint::max(4096), + BoxedUint::max(2048) + .resize(4096) + .wrapping_add(BoxedUint::one()), + ]; + for x in candidates { + let expected = x.rem_vartime(nz).resize_unchecked(2048); + assert_eq!(to_monty(&x, ¶ms).retrieve(), expected); + } + } } diff --git a/src/pkcs1v15.rs b/src/pkcs1v15.rs index 92ebee18..5ed03f82 100644 --- a/src/pkcs1v15.rs +++ b/src/pkcs1v15.rs @@ -176,11 +176,16 @@ fn decrypt( ) -> Result> { key::check_public(priv_key)?; + // RFC 8017 § 7.2.2 step 1 + if ciphertext.len() != priv_key.size() { + return Err(Error::Decryption); + } + let ciphertext = BoxedUint::from_be_slice(ciphertext, priv_key.n_bits_precision())?; let em = rsa_decrypt_and_check(priv_key, rng, &ciphertext)?; let em = uint_to_zeroizing_be_pad(em, priv_key.size())?; - pkcs1v15_encrypt_unpad(em, priv_key.size()) + pkcs1v15_encrypt_unpad(&em, priv_key.size()) } /// Calculates the signature of hashed using @@ -206,7 +211,8 @@ fn sign( let em = pkcs1v15_sign_pad(prefix, hashed, priv_key.size())?; let em = BoxedUint::from_be_slice(&em, priv_key.n_bits_precision())?; - uint_to_zeroizing_be_pad(rsa_decrypt_and_check(priv_key, rng, &em)?, priv_key.size()) + // The signature itself is public; only the computation is secret + uint_to_be_pad(rsa_decrypt_and_check(priv_key, rng, &em)?, priv_key.size()) } /// Verifies an RSA PKCS#1 v1.5 signature. @@ -386,6 +392,23 @@ mod tests { assert_eq!(out, plaintext.as_bytes()); } + #[test] + fn test_decrypt_pkcs1v15_rejects_wrong_length() { + let priv_key = get_private_key(); + let ct = Base64::decode_vec( + "arReP9DJtEVyV2Dg3dDp4c/PSk1O6lxkoJ8HcFupoRorBZG+7+1fDAwT1olNddFnQMjmkb8vxwmNMoTAT/BFjQ==", + ) + .unwrap(); + assert!(priv_key.decrypt(Pkcs1v15Encrypt, &ct).is_ok()); + + // RFC 8017 § 7.2.2 step 1: a ciphertext shorter than `k`, even if numerically valid, + // must be rejected + assert!(priv_key.decrypt(Pkcs1v15Encrypt, &ct[1..]).is_err()); + let mut long = vec![0u8]; + long.extend_from_slice(&ct); + assert!(priv_key.decrypt(Pkcs1v15Encrypt, &long).is_err()); + } + #[test] fn test_encrypt_decrypt_pkcs1v15_traits() { let mut rng = ChaCha8Rng::from_seed([42; 32]); @@ -500,7 +523,7 @@ mod tests { #[rstest] #[case( - "Test.\n", + "Test.\n", hex!( "a4f3fa6ea93bcdd0c57be020c1193ecbfd6f200a3d95c409769b029578fa0e33" "6ad9a347600e40d3ae823b8c7e6bad88cc07c1d54c3a1523cbbb6d58efc362ae" diff --git a/src/pss.rs b/src/pss.rs index 363989a0..7073c540 100644 --- a/src/pss.rs +++ b/src/pss.rs @@ -26,7 +26,7 @@ use crypto_bigint::BoxedUint; use digest::{Digest, FixedOutputReset}; use rand_core::TryCryptoRng; -use crate::algorithms::pad::{uint_to_be_pad, uint_to_zeroizing_be_pad}; +use crate::algorithms::pad::uint_to_be_pad; use crate::algorithms::pss::*; use crate::algorithms::rsa::{rsa_decrypt_and_check, rsa_encrypt}; use crate::errors::{Error, Result}; @@ -243,7 +243,7 @@ where let em = BoxedUint::from_be_slice(&em, priv_key.n_bits_precision())?; let raw = rsa_decrypt_and_check(priv_key, blind_rng, &em)?; - uint_to_zeroizing_be_pad(raw, priv_key.size()) + uint_to_be_pad(raw, priv_key.size()) } fn sign_pss_with_salt_digest( @@ -260,7 +260,7 @@ where let em = emsa_pss_encode_digest::(hashed, em_bits as _, salt)?; let em = BoxedUint::from_be_slice(&em, priv_key.n_bits_precision())?; - uint_to_zeroizing_be_pad( + uint_to_be_pad( rsa_decrypt_and_check(priv_key, blind_rng, &em)?, priv_key.size(), ) From 43fa427ab73c02a25c664ca60b4a60249378d3dd Mon Sep 17 00:00:00 2001 From: "N." Date: Tue, 29 Sep 2026 19:59:39 +0200 Subject: [PATCH 2/2] apply suggestion and correct logical operator --- src/algorithms/pad.rs | 2 +- src/algorithms/pkcs1v15.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/algorithms/pad.rs b/src/algorithms/pad.rs index faf90f81..d5c68bbc 100644 --- a/src/algorithms/pad.rs +++ b/src/algorithms/pad.rs @@ -41,7 +41,7 @@ pub(crate) fn uint_to_be_pad(input: BoxedUint, padded_len: usize) -> Result Result> { /// valid then index contains the index of the original message in em. #[inline] fn decrypt_inner(em: &[u8], k: usize) -> Result<(u8, u32)> { - if k < 11 && em.len() != k { + if k < 11 || em.len() != k { return Err(Error::Decryption); }