Skip to content

Support RSA PKCS#1 signatures with absent parameters #346

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 7, 2025
Merged

Support RSA PKCS#1 signatures with absent parameters #346

merged 1 commit into from
May 7, 2025

Conversation

ctz
Copy link
Member

@ctz ctz commented May 7, 2025 •
edited
Loading

RFC4055 says on sha256WithRSAEncryption and company:

When any of these four object identifiers appears within an
AlgorithmIdentifier, the parameters MUST be NULL. Implementations
MUST accept the parameters being absent as well as present.

The latter clause is implemented in this commit.

The algorithm identifiers live here rather than pki-types, as they must not be used to originate new certificates. Putting them in pki-types would mean they appear in a public API, whereas here they are kept private and restricted to verifying existing signatures.

--

For ecosystem background, go entirely ignores parameters for these algorithms: https://cs.opensource.google/go/go/+/refs/tags/go1.24.3:src/crypto/x509/x509.go;l=425-431 (which is strictly weaker than the RFC requires; go would seemingly be happy if the parameters were a UTF8STRING containing the complete works of Shakespeare.)

re rustls/rustls#2448

@codecov Codecov
Copy link

codecov bot commented May 7, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.91%. Comparing base (0ac75b1) to head (1178545).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #346   +/-   ##
=======================================
  Coverage   97.91%   97.91%           
=======================================
  Files          20       20           
  Lines        4470     4470           
=======================================
  Hits         4377     4377           
  Misses         93       93

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ctz ctz force-pushed the jbp-rsa-absent-algid-params branch from 727e1fd to d8649d3 Compare May 7, 2025 13:03
djc
djc approved these changes May 7, 2025
View reviewed changes
@djc
Copy link
Member

djc commented May 7, 2025

The algorithm identifiers live here rather than pki-types, as they must not be used to originate new certificates. Putting them in pki-types would mean they appear in a public API, whereas here they are kept private and restricted to verifying existing signatures.

They're still pub API, right? I don't see a very meaningful difference...

@cpu
Copy link
Member

cpu commented May 7, 2025

They're still pub API, right?

They're in a non-pub mod and not re-exported from lib.rs though AFAICT:

webpki/src/lib.rs

Lines 91 to 104 in 0ac75b1

#[cfg(feature = "ring")]
/// Signature verification algorithm implementations using the *ring* crypto library.
pub mod ring {
pub use super::ring_algs::{
ECDSA_P256_SHA256, ECDSA_P256_SHA384, ECDSA_P384_SHA256, ECDSA_P384_SHA384, ED25519,
};
#[cfg(feature = "alloc")]
pub use super::ring_algs::{
RSA_PKCS1_2048_8192_SHA256, RSA_PKCS1_2048_8192_SHA384, RSA_PKCS1_2048_8192_SHA512,
RSA_PKCS1_3072_8192_SHA384, RSA_PSS_2048_8192_SHA256_LEGACY_KEY,
RSA_PSS_2048_8192_SHA384_LEGACY_KEY, RSA_PSS_2048_8192_SHA512_LEGACY_KEY,
};
}

cpu
cpu approved these changes May 7, 2025
View reviewed changes
@djc
Copy link
Member

djc commented May 7, 2025

They're still pub API, right?

They're in a non-pub mod and not re-exported from lib.rs though AFAICT:

webpki/src/lib.rs

Lines 91 to 104 in 0ac75b1

#[cfg(feature = "ring")]
/// Signature verification algorithm implementations using the *ring* crypto library.
pub mod ring {
pub use super::ring_algs::{
ECDSA_P256_SHA256, ECDSA_P256_SHA384, ECDSA_P384_SHA256, ECDSA_P384_SHA384, ED25519,
};
#[cfg(feature = "alloc")]
pub use super::ring_algs::{
RSA_PKCS1_2048_8192_SHA256, RSA_PKCS1_2048_8192_SHA384, RSA_PKCS1_2048_8192_SHA512,
RSA_PKCS1_3072_8192_SHA384, RSA_PSS_2048_8192_SHA256_LEGACY_KEY,
RSA_PSS_2048_8192_SHA384_LEGACY_KEY, RSA_PSS_2048_8192_SHA512_LEGACY_KEY,
};
}

I thought we relied on unreachable_pub to warn against this sort of thing...

@ctz
Copy link
Member Author

ctz commented May 7, 2025 •
edited
Loading

They're still pub API, right? I don't see a very meaningful difference...

They're pub API from this crate in the form of an impl SignatureVerificationAlgorithm, and you can get the value out via signature_alg_id() there I guess. But at least they don't appear in https://docs.rs/rustls-pki-types/latest/rustls_pki_types/alg_id/index.html ...

@ctz
Copy link
Member Author

ctz commented May 7, 2025

I thought we relied on unreachable_pub to warn against this sort of thing...

This commit exports them in the usual place.

image

@cpu
Copy link
Member

cpu commented May 7, 2025

This commit exports them in the usual place.

Oops, I was looking at tip-of-main lib.rs.

@ctz
Support RSA PKCS#1 signatures with absent parameters
1178545 
RFC4055 says on sha256WithRSAEncryption and company:

>   When any of these four object identifiers appears within an
>   AlgorithmIdentifier, the parameters MUST be NULL.  Implementations
>   MUST accept the parameters being absent as well as present.

The latter clause is implemented in this commit.

The algorithm identifiers live here rather than pki-types, as they
_must not_ be used to originate new certificates.  Putting them in
pki-types would mean they appear in a public API, whereas here they
are kept private and restricted to verifying existing signatures.
@ctz ctz force-pushed the jbp-rsa-absent-algid-params branch from d8649d3 to 1178545 Compare May 7, 2025 14:11
@ctz ctz added this pull request to the merge queue May 7, 2025
Merged via the queue into main with commit 16abda1 May 7, 2025
54 of 64 checks passed
@ctz ctz deleted the jbp-rsa-absent-algid-params branch May 7, 2025 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djc djc djc approved these changes

@cpu cpu cpu approved these changes

+1 more reviewer

@stephane-dubois stephane-dubois stephane-dubois left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ctz @djc @cpu @stephane-dubois