rsa: keep RSA-PSS keys without parameters unrestricted - #483
rsa: keep RSA-PSS keys without parameters unrestricted#483yosuke-wolfssl wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
It changes security- and interoperability-critical DER parsing/encoding paths for RSA-PSS keys, so a final human review is warranted even with strong tests.
Pull request overview
This PR fixes RSA-PSS key decode/encode behavior so that keys whose AlgorithmIdentifier omits RSASSA-PSS-params remain unrestricted (rather than being implicitly pinned to RFC 8017 defaults like SHA-1), and ensures the encoded AlgorithmIdentifier round-trips byte-identically for those keys.
Changes:
- Update RSA-PSS parameter detection/decoding to distinguish “params absent” vs “params present” and only apply defaults when params are actually present (
src/wp_rsa_kmgmt.c). - Split key
AlgorithmIdentifierencoding so unrestricted RSA-PSS keys encode as a bare OID (no injected params), while signature alg-ids still carry digest/MGF/salt (src/wp_rsa_kmgmt.c). - Fix PSS restriction handling by initializing/copying
minSaltLenwith the intended “unrestricted” sentinel and add unit tests covering unrestricted PSS keys and signature alg-id parity with OpenSSL (src/wp_rsa_sig.c,test/test_rsa.c,test/unit.*).
File summaries
| File | Description |
|---|---|
| test/unit.h | Adds the new unit test prototype to the test registry header. |
| test/unit.c | Registers the new RSA-PSS “no params” unit test in the test list. |
| test/test_rsa.c | Adds DER fixtures and new test logic for unrestricted RSA-PSS keys and PSS signature AlgorithmIdentifier A/B vs OpenSSL. |
| src/wp_rsa_sig.c | Initializes and propagates minSaltLen so unrestricted PSS keys don’t incorrectly trigger restriction checks. |
| src/wp_rsa_kmgmt.c | Corrects RSA-PSS param presence detection and adjusts key AlgorithmIdentifier encoding for unrestricted keys. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
858bc1f to
3464657
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #483
Scan targets checked: wolfprovider-bugs, wolfprovider-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
- wp_rsa_pss_get_params() takes the AlgorithmIdentifier's length octet as the end of its content, reads a SEQUENCE after the OID as parameters present and nothing at all as absent, and rejects any other tag. The defaults, the hash, MGF and salt blocks and rsa->pssDefSet apply only when parameters are present. The BIT STRING step-over is removed. - wp_rsa_pss_encode_key_alg_id() encodes the OID alone for a key with no PSS restrictions, else defers to wp_rsa_pss_encode_alg_id(); the six call sites in the SPKI and PKCS#8 encoders use it. rsa_pss_oid moves to file scope and both encoders compute their length delta with signed operands. - wp_rsa_ctx_new() sets minSaltLen to -1 and wp_rsa_ctx_dup() copies it. For a key with no PSS restrictions wp_rsa_signverify_init() uses minSaltLen -1, and saltLen RSA_PSS_SALTLEN_AUTO where RSA_PSS_SALTLEN_AUTO_DIGEST_MAX is unavailable. - test_rsa.c gains rsa_pss_key_der_2048_pkcs8_noparams and test_rsa_pss_no_params(), covering an empty parameters SEQUENCE restricting the key and a duplicated context keeping the salt length restriction; test_rsa_sig_alg_id() compares the PSS signature AlgorithmIdentifier against OpenSSL. Issue: F-12483
3464657 to
cd35f24
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #483
Scan targets checked: wolfprovider-bugs, wolfprovider-src
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
Problem
An RSA-PSS
SubjectPublicKeyInfoor PKCS#8PrivateKeyInfowhoseAlgorithmIdentifiercarries noRSASSA-PSS-paramsis unrestricted: anysupported digest may be used with it.
wp_rsa_pss_get_params()had no way totell "parameters absent" from "parameters present" — it never learned where the
AlgorithmIdentifierended — so it installed the RFC 8017 defaults and setpssDefSetunconditionally. An unrestricted key decoded as restricted toSHA-1/MGF1-SHA-1/salt 20.
d2i_PUBKEY_exon absent-params SPKId2i_AutoPrivateKey_exon absent-params PKCS#8i2d_PUBKEYof such a keyRSASSA-PSS-paramsValid signatures over another supported digest were rejected after decoding.
Fix (
src/wp_rsa_kmgmt.c)algIdEndtakes theAlgorithmIdentifierSEQUENCE's own length octet asthe end of its content, making presence decidable. This replaces a walk that
stepped over the SPKI
BIT STRINGand landed insideRSAPublicKey, matched noparameter tag, and still reported success.
wp_rsa_pss_encode_key_alg_id()encodes the OID alone for an unrestrictedkey. Key encoders use it; the signature
AlgorithmIdentifierkeeps carrying thedigest, MGF and salt the signature was actually made with.
ossl_rsa_param_decode().In
src/wp_rsa_sig.c,minSaltLennow uses-1for "unrestricted" — thesentinel
wp_rsa_setup_md()already tested for but which was never assigned, sothe restriction check fired for every PSS key.
Empty parameters (
30 00) still decode as restricted to SHA-1, per the ASN.1defaults. Closes f-12483.
Tests
test_rsa_pss_no_params— decodes both formats, checks the default digestis SHA256 and no salt length is reported, signs and verifies under SHA-256/384/512
in both directions against OpenSSL, asserts byte-exact round-trips, and confirms a
restricted key is still pinned.
test_rsa_sig_alg_id— now A/Bs the PSS signatureAlgorithmIdentifieragainst OpenSSL across three digests.
Verification
evppkey_rsa.txtunrestricted PSS vector.test_rsa_pss_no_paramsfails; reverting theencoder split makes the alg-id test report 13 bytes against OpenSSL's 67.