Repository navigation
Fix RSA constant-time padding and length checks - #710
Merged
Merged
Conversation
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.
tarcieri
reviewed
Sep 29, 2026
tarcieri
reviewed
Sep 29, 2026
Member
|
Overall I like the direction of this PR, thanks |
tarcieri
reviewed
Oct 1, 2026
| 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 { |
Member
There was a problem hiding this comment.
This seems value-dependent? I'm also not sure what black_box is buying you here.
Contributor
Author
There was a problem hiding this comment.
Yup agreed on both black_box was really just an optimizer hint anyway I'm gonna push soon to address this
Member
There was a problem hiding this comment.
Perhaps the function could be infallible and this could just be a debug_assert!? Then you can just add a precondition comment.
Contributor
Author
There was a problem hiding this comment.
I've worked a bit on it and chose to use Choice with a debug_assert! idk if it would be more convenient
Member
There was a problem hiding this comment.
That approach sounds fine to me. Can you push it up whenever you're ready?
8 tasks done
Member
|
Note: I created #711 as a tracking issue for constant-time problems and noted this PR addresses several of them, or at least attempts to partially address them |
Member
|
To help move some of the other constant time work along I'm going to merge this as-is, then follow up with my feedback |
tarcieri
approved these changes
Oct 12, 2026
tarcieri
added a commit
that referenced
this pull request
Oct 12, 2026
Followup to #710 which implements the suggested constant-time improvements from that thread, namely changing this function to be infallible and panicking if its preconditions aren't met, with a constant-time `debug_assert!` that overflow is not occurring. Additionally expands test coverage for these cases. cc @iamnoksio
kaidokert
added a commit
to kaidokert/RSA
that referenced
this pull request
Nov 6, 2026
* Fix RSA constant-time padding and length checks (RustCrypto#710) 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. Fork adaptation: `be_pad_into` uses checked splits and copy loops so the embedded sign binary stays panic-free, and the generic `*_pad_into` functions now pad the carrier's full-width bytes instead of trimming at `bits()`. Upstream's `to_monty` is generic over the backend, and its constant-time reduction lives in `IntoMontyForm::from_value` for `BoxedMontyForm`, replacing `rem_vartime`. The modmath backends already reduce in constant time. (cherry picked from commit 55345ae) * Make `be_pad_into` infallible (RustCrypto#715) Followup to RustCrypto#710 which implements the suggested constant-time improvements from that thread, namely changing this function to be infallible and panicking if its preconditions aren't met, with a constant-time `debug_assert!` that overflow is not occurring. Additionally expands test coverage for these cases. Fork adaptation: alloc only. The no-alloc `uint_to_*_be_pad_into` functions keep the fallible, panic-free helper, renamed `try_be_pad_into`, because the embedded sign path has no panic handler to fall back on. (cherry picked from commit 2770a5f) * Rename integer serialization functions to `i2osp*` (RustCrypto#718) The Integer-to-Octet-String Primitive (I2OSP) is defined in [RFC8017 §4.1](https://www.rfc-editor.org/rfc/rfc8017.html#section-4.1). This renames the following: - `algorithms::pad::uint_to_be_pad` => `...::i2osp` - `algorithms::pad::uint_to_zeroizing_be_pad` => `...::i2osp_zeroizing` This better reflects the algorithm descriptions from `draft-irtf-cfrg-rsa-guidance` Fork adaptation: the no-alloc variants are renamed to match, to `i2osp_into` and `i2osp_zeroizing_into`. (cherry picked from commit 7f62acf) * ct-ctgrind: describe the i2osp serialization suppressions RustCrypto#710 removed the leading-zero trim that suppression (2) described. What the sign fixture now hits is the overflow check in `try_be_pad_into` on the bytes above the modulus width, which are zero for any valid signature. The shared-symbol entries follow the helper's new name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Extract `os2ip(_zeroizing)` (RustCrypto#719) Followup to RustCrypto#718 which extracts an Octet-String-to-Integer Primitive helper function(s) as described in [RFC8017 §4.2](https://www.rfc-editor.org/rfc/rfc8017.html#section-4.2). This makes it possible to write code that looks more like the algorithmic specifications, e.g. `draft-irtf-cfrg-rsa-guidance` Fork adaptation: alloc only. The fork's PKCS#1 v1.5 encrypt goes through the generic `encrypt_into`, so only the decrypt call site changes there. (cherry picked from commit aff8b64) * RFC8017-related documentation improvements (RustCrypto#720) Adopts more RFC8017 nomenclature in the rustdoc and attempts to better clarify how various APIs relate to RFC8017 specifications and terminology Fork adaptation: the README hunk goes to UPSTREAM_README.md. (cherry picked from commit 7a8265a) * Extract `pss::decode_signature` (RustCrypto#724) Fork adaptation: the fork keeps the integer `Signature<T>` (upstream RustCrypto#722/RustCrypto#723 are not taken), so `decode_signature` takes the decoded integer and the octet length. It now also gives the alloc `verify` the range and precision check that the generic paths already had. `verify_digest` keeps delegating to the generic `verify_digest_into`. tests/rfc8017.rs was added upstream by RustCrypto#722; only the helpers that the out-of-range alias test needs are brought in. The octet-string round-trip and padded-length cases depend on RustCrypto#722's byte-string signature. (cherry picked from commit 1376805) --------- Co-authored-by: N <42706907+iamnoksio@users.noreply.github.com> Co-authored-by: Tony Arcieri <bascule@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Merged
tarcieri
added a commit
that referenced
this pull request
Nov 8, 2026
## Security - Replace `num-bigint-dig` with `crypto-bigint` for constant-time modular exponentiation (#394) - Redact private-key components from `Debug` output (#480) - Harden private-key operations following `draft-irtf-cfrg-rsa-guidance` (#710, #715, #739, #751, #752, #755, #756, #758, #770, #771, #773) - Zeroize stored secret-prime Montgomery parameters in `PrecomputedValues` (#716) - Implement PKCS#1 v1.5 implicit rejection: return deterministic synthetic plaintext on invalid padding instead of exposing a padding error and with it a sidechannel (#726, #729, #749) ## Added - PKCS#8/SPKI decoding for PSS keys, including `id-RSASSA-PSS` algorithm identifiers (#424) - `pss::VerifyingKey::salt_len` and opt-in automatic salt-length detection via `new_with_auto_salt_len` (#448, #546) - Wycheproof test coverage for PKCS#1 v1.5, PSS, and OAEP (#477, #740) - `PublicKeyParts::n_bytes` and `e_bytes` serialization helpers (#519) - Multipart signing trait implementations for PKCS#1 v1.5 and PSS (#525) - `Clone` and `core::error::Error` implementations for `Error` (#530, #694) - `PrivateKey::as_public_key` accessor (#592) - Algorithm-specific inherent methods on `PrivateKey` and `PublicKey` for OAEP encryption/decryption and PKCS#1 v1.5/PSS signing/verification, including prehash and explicit-RNG variants (#638) - `PrivateKey::from_components_with_large_exponent`, gated on `hazmat` (#648) - Constant-time validation harness using `cargo checkct`/BINSEC (#732, #733, #749, #756, #771, #773) - `PrivateKey::from_components_with_wide_primes` for unbalanced two-prime keys, which ordinary constructors now reject (#755) - `TryFrom<BitStringRef<'_>>` for PKCS#1 v1.5 and PSS signatures (#764) - `crypto_common::Generate` implementation and `Generate` re-export for generating 3072-bit keys with public exponent 65537 (#780) ## Changed - Replace `BigUint` in the public API with `BoxedUint`; return `NonZero<BoxedUint>` for the modulus and `Odd<BoxedUint>` for prime factors (#394, #735) - Use `crypto-primes` for prime generation (#394) - Use DER-based key encodings and hex-or-binary serde representations via `serdect`; extend serde support to OAEP, PKCS#1 v1.5, and PSS wrapper types (#420) - Accept arbitrary byte strings as OAEP labels (#467) - Gate PKCS#1/PKCS#8/SPKI support on the default-enabled `encoding` feature; `std` no longer implicitly enables encoding dependencies (#537, #543) - Enforce a 1024-bit minimum for key generation and default public-key construction/decoding; add `PublicKey::new_with_size_limits` and `hazmat`-gated unchecked key generation (#576, #760) - Bump `pkcs1` to v0.8; `pkcs8` to v0.11; `spki` to v0.8; `serdect` to v0.4 (#584, #684, #688, #766) - Make `Oaep` and `Pss` generic over their digest types instead of using `DynDigest` (#593) - Bump `rand_core` to v0.10, `crypto-common` to v0.2, `digest` to v0.11, and `signature` to v3 (#650, #671, #693) - Remove a redundant allocation during OAEP decryption (#659) - Improve encryption and verification performance by bounding exponentiation to the public exponent's bit length (#662) - Bump `crypto-bigint` to v0.7.10 and `crypto-primes` to v0.7 (#673, #766) - Raise the default maximum public-key modulus size to 8192 bits (#681) - Bump `sha1` and `sha2` to v0.11 (#683) - Store PKCS#1 v1.5 and PSS signatures as byte strings, preserving their encoded width (#722, #723) - MSRV 1.87; Rust 2024 edition (#731, #757) - Enable `getrandom` by default and use the system RNG for blinding in default decryption and signing paths (#739, #758) - Limit signature decoding to 1024 bytes by default; add `Signature::from_bytes_with_max_len` for explicit limits (#767) - Require the opt-in `pkcs1v15` feature for PKCS#1 v1.5 encryption and signatures (#778) - Accept fallible RNGs through `TryCryptoRng` and propagate RNG failures as errors (#779) - Rename `RsaPrivateKey` and `RsaPublicKey` to `PrivateKey` and `PublicKey`, retaining the old names as deprecated aliases (#781) ## Deprecated - `pss::BlindedSigningKey` in favor of `pss::SigningKey`, which now applies blinding by default (#758) - `PublicKey::new_with_max_size` in favor of `PublicKey::new_with_size_limits` (#760) ## Removed - `nightly` and `u64_digit` features associated with `num-bigint-dig` (#394) - Deprecated `Pkcs1v15Sign::new_raw` and `pkcs1v15` `*_with_prefix` constructors (#524) - Separate `pem` feature; PEM support is included with `encoding` (#537) - Optional `sha2` feature; SHA-2 support is now always enabled (#749) ## Fixed - Honor explicit larger-key construction limits during cryptographic operations, including OAEP decryption (#441, #595) - PKCS#1 v1.5 `SigningKey::from` and `VerifyingKey::from` now use the digest prefix consistent with `new` (#472, #498) - Check the modulus's actual bit length instead of its storage precision when enforcing size limits (#529) - Retry zero blinding factors instead of replacing them with one (#653) - Reject private-key factors less than or equal to one, even or repeated factors, and private exponents greater than or equal to the modulus (#655, #738, #772, #774) - Reject PSS salt lengths that cannot be represented by the `AlgorithmIdentifier` encoder instead of truncating them (#704) - Apply RFC 8017 encoded-message width rules for PSS, including moduli whose bit length is congruent to 1 modulo 8 (#725) - Return verification errors instead of panicking for undersized PSS encoded messages and overflowing salt lengths (#769) - Propagate CRT precomputation errors from private-key constructors (#782)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.