Fix RSA constant-time padding and length checks

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<Vec<u8>> {
-    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<Vec<u8>> {
-    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<Vec<u8>> {
-    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<Zeroizing<Vec<u8>>> {
+    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]);
     }
 }

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<u8>, k: usize) -> Result<Vec<u8>> {
-    let (valid, out, index) = decrypt_inner(em, k)?;
+pub(crate) fn pkcs1v15_encrypt_unpad(em: &[u8], k: usize) -> Result<Vec<u8>> {
+    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<u8>, k: usize) -> Result<Vec<u8>> {
 /// 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<u8>, k: usize) -> Result<(u8, Vec<u8>, 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<u8>, k: usize) -> Result<(u8, Vec<u8>, 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]

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, &params).retrieve(), expected);
+        }
+    }
 }

src/pkcs1v15.rs

@@ -176,11 +176,16 @@ fn decrypt<R: TryCryptoRng + ?Sized>(
 ) -> Result<Vec<u8>> {
     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<R: TryCryptoRng + ?Sized>(
     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"

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<T, D>(
@@ -260,7 +260,7 @@ where
     let em = emsa_pss_encode_digest::<D>(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(),
     )