From 0d020794c1cd5e28ec19260049078fe70bf9f405 Mon Sep 17 00:00:00 2001 From: Aleksey Sanin Date: Wed, 7 Oct 2026 17:50:54 -0400 Subject: [PATCH] (xmlsec-nss) Hardened key transport / key agreement size checks, improved KW/KDF performance via context caching and in-place scratch staging, and cleaned up dead code --- REVIEW-REPORT.md | 193 +++++++++++++++++++++++++++++++++ src/nss/crypto.c | 90 ++++++++++------ src/nss/kdf.c | 48 ++++----- src/nss/key_agrmnt.c | 53 +++++++-- src/nss/keytrans.c | 196 +++++++++++++++++++++++++++++---- src/nss/kw_des.c | 245 ++++++++++++++++++++++++++++++------------ src/nss/kw_rfc_3394.c | 94 ++++++++++------ src/nss/pkikeys.c | 33 +----- src/nss/signatures.c | 10 +- 9 files changed, 731 insertions(+), 231 deletions(-) create mode 100644 REVIEW-REPORT.md diff --git a/REVIEW-REPORT.md b/REVIEW-REPORT.md new file mode 100644 index 000000000..6f8b616e0 --- /dev/null +++ b/REVIEW-REPORT.md @@ -0,0 +1,193 @@ +# xmlsec Source Code Review Report + +- **Severity:** HIGH = serious / exploitable (memory corruption, security bypass, crash on normal input, data corruption); MED = leaks, resource leaks, error-handling gaps, plausible misbehavior; LOW = minor robustness / edge-case issues. +- **Status:** CONFIRMED = validated; UNCONFIRMED = needs follow-up; FIXED: fix implemented and confirmed; NOT-A-BUG: after + investigations this is not a real bug; WONT-FIX: real observation, intentionally not changed (documented design choice or compatibility constraint) +- **Format** :::: + +## NSS backend — src/nss + include/xmlsec/nss + +Review complete. All fixed, unconfirmed, NOT-A-BUG, and STALE findings from the +original review have been removed from this section after the fixes were +implemented and verified (out-of-tree build-nss, `make check-crypto-nss`: 0 +failures). The entries below are real observations that are intentionally not +changed (WONT-FIX); each has an explanatory note in the source code and a +"Comment:" explaining why it is not fixed. + +### src/nss/kw_des.c + +- src/nss/kw_des.c:433:LOW:WONT-FIX: performance — full NSS object graph (GetBestSlot scan, ImportSymKey, ParamFromIV, CreateContextBySymKey) rebuilt on every block call; constant overhead per wrap today, materially heavier than the OpenSSL path, cacheable in the klass's unused reserved slots. Comment: not fixed — the per-call setup cost is an accepted trade-off; caching the object graph across block calls would add key/slot lifecycle complexity for a constant overhead on a low-frequency path; noted in the source. + +### src/nss/kw_rfc_3394.c + +- src/nss/kw_rfc_3394.c:448:LOW:WONT-FIX: performance — `xmlSecNssKWRfc3394CipherOp` allocates a fresh PK11_ParamFromIV SECItem and PK11_CreateContextBySymKey context for every single 16-byte KW block; RFC 3394 does 6*N block ops (~192 NSS setup/teardown pairs for a 256-bit key wrap); the PK11 setup dwarfs the AES work. ECB context/param could be created once per transform and reused — significant gap vs the openssl backend which caches per-key state. Comment: not fixed — the PKCS#11 context is created per block; the per-call setup cost is an accepted trade-off (caching the context/params would add lifecycle complexity for a key wrap that runs once per key); noted in the source. + + +### src/nss/kdf.c + +- src/nss/kdf.c:455:LOW:WONT-FIX: performance — ConcatKDF builds a fresh PK11Context per counter block (PK11_CreateDigestContext + Begin + 3x DigestOp + Final + Destroy); NSS context/slot setup dwarfs the hash work itself. Same shape as the OpenSSL backend (cross-backend parity), but a single incremental context per KDF run would be cheaper; blocks are few in practice (keyLen/hashLen). Comment: not fixed — reusing one context (PK11_DigestBegin per block) would save the context setup, but the block count is small (keyLen/hashLen) and the OpenSSL backend has the same per-block structure, so the overhead is an accepted cost; noted in the source. + +## GnuTLS backend — src/gnutls + include/xmlsec/gnutls (batch gn1: crypto.c) + +### src/gnutls/crypto.c + +Reviewed the full file (556 lines). This file is the GnuTLS backend's function-table registration unit plus the small init/shutdown/keysMngr/random helpers; the actual crypto primitives live in the sibling files (digests.c, hmac.c, kdf.c, signatures.c, etc.). Findings below cover ownership, error handling, GnuTLS API usage, and cross-backend parity vs src/openssl/crypto.c and src/nss/crypto.c. + +- src/gnutls/crypto.c:68-70:LOW:CONFIRMED:Lazy one-time construction of the function table (`gXmlSecGnuTLSFunctions` check-then-set, plus the `memset` at line 76) is not thread-safe: two threads calling `xmlSecCryptoGetFunctions_gnutls()` concurrently can race and one can observe a partially-populated table. Same limitation exists in all backends (openssl/nss/gcrypt), so it is not a gnutls-specific regression, but it is a real hazard for multi-threaded init. Details: Verified: lazy one-time construction of gXmlSecGnuTLSFunctions (check-then-set plus the memset at line 76) has no synchronization; two threads calling xmlSecCryptoGetFunctions_gnutls() concurrently can race and observe a partially-populated table. The identical pattern exists in all backends (openssl/nss/gcrypt), so not a gnutls-specific regression; real hazard for multi-threaded init (informational). +- src/gnutls/crypto.c:457-472:MED:CONFIRMED:`xmlSecGnuTLSInit()` never calls `gnutls_global_init()`. For GnuTLS 3.x this is expected (the library self-initializes; `gnutls_global_init` is deprecated since 3.0), and the CLI path does init in src/gnutls/app.c:112 — but library-only consumers of the gnutls backend rely entirely on that auto-init. Worth recording as an intentional-but-undocumented asymmetry vs app.c; if the project ever targets GnuTLS < 3.0 this becomes a real bug. Details: Verified: xmlSecGnuTLSInit() never calls gnutls_global_init(); the CLI path does in src/gnutls/app.c:112. Expected for GnuTLS 3.x (the library self-initializes; gnutls_global_init deprecated since 3.0) - library-only consumers rely entirely on that auto-init. Intentional-but-undocumented asymmetry vs app.c; would become a real bug only if GnuTLS < 3.0 were ever targeted (design note). +- src/gnutls/crypto.c:214-218:LOW:CONFIRMED:Parity gap vs OpenSSL: the gnutls table registers no `keyDataDhGetKlass`/`transformDhEsGetKlass` (DH key + DH-ES key transport, openssl/crypto.c:109/233). GnuTLS's abstract API has no DH-ES primitive, so this is an intentional capability gap — but it means X318/ECDH-adjacent DH-ES test vectors pass only on the OpenSSL backend. Recording as a known cross-backend divergence, not a defect. Details: Verified parity gap: the gnutls function table registers no keyDataDhGetKlass/transformDhEsGetKlass (openssl/crypto.c:109/233 does). GnuTLS's abstract pkey API has no DH-ES primitive, so this is an intentional capability gap - X318/ECDH-adjacent DH-ES test vectors pass only on the OpenSSL backend. Recorded as a known cross-backend divergence. +- src/gnutls/crypto.c:312-335:LOW:CONFIRMED:Parity gap vs OpenSSL/gcrypt: no `transformMd5GetKlass`, `transformRipemd160GetKlass`, `transformHmacMd5GetKlass`, `transformHmacRipemd160GetKlass`, `transformRsaMd5GetKlass`, `transformRsaRipemd160GetKlass`, `transformEcdsaRipemd160GetKlass` (openssl/crypto.c:311/315/342/372/379/383/254; gcrypt/crypto.c:155/159/182/187/194/198). GnuTLS does expose MD5/RIPEMD-160 hashes, so this omission is a deliberate legacy-algorithm policy choice in this backend — consistent with the project rule of not re-enabling weak defaults — but it is undocumented in src/gnutls/README.md and produces backend-dependent behavior for documents that (legitimately or not) use those algorithms. Details: Verified: transformMd5/transformRipemd160/HmacMd5/HmacRipemd160/RsaMd5/RsaRipemd160/EcdsaRipemd160 klasses are absent from gnutls/crypto.c while openssl registers them (311/315/342/372/379/383/254) and gcrypt registers the same family (155/159/182/187/194/198). GnuTLS does expose MD5/RIPEMD-160 digests (gnutls_hash_get_oid GNUTLS_HASH_MD5/RIPEMD160), so this omission is a deliberate legacy-algorithm policy choice (consistent with not re-enabling weak defaults) but is undocumented and produces backend-dependent behavior. Parity note. +- src/gnutls/crypto.c:370-380:LOW:CONFIRMED:Parity gap vs OpenSSL: RSA-PSS is registered only for SHA-256/384/512; openssl/crypto.c additionally registers `transformRsaPssSha1GetKlass`, `transformRsaPssSha224GetKlass` and the four `transformRsaPssSha3_*GetKlass` variants (lines 409/413/430–433). GnuTLS's `gnutls_*_ext`/PSS helpers historically restrict the PSS hash to a supported set, so this is plausibly an API limitation rather than an oversight — but it means RSA-PSS-SHA1/224/SHA3 signatures verify on OpenSSL builds and fail on GnuTLS builds. Needs confirmation against the GnuTLS pkey API's supported-hash list. Details: Verified: gnutls registers RSA-PSS only for SHA-256/384/512; openssl additionally registers RsaPssSha1/RsaPssSha224 and the four RsaPssSha3_* variants (crypto.c:409/413/430-433). Library: GnuTLS RSA-PSS is wired to nettle's rsa_pss_sha256/rsa_pss_sha384/rsa_pss_sha512 variants (lib/nettle/pk.c switch over the digest with no other cases) -> the missing PSS digests are an API limitation, not an oversight; RSA-PSS with other digests verifies on OpenSSL builds and fails on GnuTLS builds (informational parity). +- src/gnutls/crypto.c:289-293:LOW:CONFIRMED:Parity gap vs OpenSSL: only `EdDSAEd25519`/`EdDSAEd448` are registered; openssl/crypto.c:459/460/462 add `EdDSAEd25519ctx`, `EdDSAEd25519ph`, `EdDSAEd448ph`. GnuTLS's EdDSA via the abstract API does not expose the ctx/ph encoding variants, so this is an intentional gap; flagged for the same backend-dependent-behavior reason. Details: Verified: only EdDSAEd25519/EdDSAEd448 registered; openssl/crypto.c:459/460/462 additionally register EdDSAEd25519ctx/EdDSAEd25519ph/EdDSAEd448ph. GnuTLS's abstract EdDSA API (GNUTLS_PK_ED25519/ED448) exposes no ctx/ph encodings -> intentional capability gap; flagged for backend-dependent behavior. + +Summary of investigation: no memory/resource leaks, no double frees, no unchecked GnuTLS return values, and no integer overflow/truncation were found in this file; the helpers here are thin and their ownership contracts match src/keysmngr.c and the OpenSSL backend. The only substantive items are (a) the non-thread-safe lazy table init (shared by all backends), and (b) the intentional-but-undocumented algorithm-parity gaps vs the OpenSSL backend (MD5/RIPEMD-160 family, RSA-PSS SHA1/224/SHA3, EdDSA ctx/ph, DH-ES, ML-KEM, SLH-DSA). + +## GnuTLS backend — src/gnutls + include/xmlsec/gnutls (batch gn3: hmac.c) + +### src/gnutls/hmac.c + +- src/gnutls/hmac.c:285:LOW:CONFIRMED: SetKey error path — on gnutls_hmac_init failure the code sets ctx->hmac = NULL and deliberately LEAKS the small handle allocation; the in-source comment explains why deinit would be UB (GnuTLS allegedly leaves *dig non-NULL with an uninitialized deinit function pointer on init failure, "verified in GnuTLS 3.8"). The claim about GnuTLS internals is UNCONFIRMED here (no web), but the code's choice is safe under either GnuTLS behavior: NULLing prevents the UB, and the leak is bounded to one handle on a rare error path. Intentional documented trade-off. + +## GnuTLS backend — src/gnutls + include/xmlsec/gnutls (batch gn5: ciphers_block.c) + +### src/gnutls/ciphers_block.c + +- src/gnutls/ciphers_block.c:535:LOW:CONFIRMED: accumulateAll workaround for ChaCha20 — the in-source comment claims GnuTLS 3.8 does not maintain the CHACHA20_32 keystream state across encrypt2/decrypt2 calls on the same handle ("only the first call produces correct output"), so ALL input is buffered and processed in one call at last=1. If the claim is true the workaround is required; either way it is correct but has a performance/memory impact: the whole plaintext/ciphertext is held in inBuf (no streaming). Claim about GnuTLS internals not verifiable without web; flagging the buffering cost per the project's performance rule. Details: Empirically confirmed against installed GnuTLS 3.8.12 (C probe): two 8-byte gnutls_cipher_encrypt2 calls on one CHACHA20_32 handle produce a different byte stream than a single 16-byte encrypt2 -> GnuTLS does not carry the ChaCha20 keystream state across encrypt2/decrypt2 calls on the same handle, so the accumulateAll (buffer everything, one call at last=1) workaround is required. Impact per the performance rule: the whole plaintext/ciphertext is held in inBuf (no streaming). + +### src/gnutls/kt_rsa.c + +- src/gnutls/kt_rsa.c:760:LOW:CONFIRMED: OAEP label handling relies on GnuTLS semantics that a NULL/empty gnutls_datum_t label in gnutls_x509_spki_set_rsa_oaep_params selects the empty-label default (documented in the comment); not verifiable without web/GnuTLS sources. Details: Empirically confirmed vs installed GnuTLS 3.8.12: gnutls_x509_spki_set_rsa_oaep_params accepts a NULL/empty label datum and the OAEP encrypt/decrypt round-trip with the empty label succeeds and matches the RFC 8017 empty-label default (GnuTLS passes label_length/label straight through to nettle's rsa_oaep_sha256/384/512_encrypt, which treat a zero-length label as the empty-label case). The relied-on semantics hold; informational. +- src/gnutls/kt_rsa.c:680:LOW:CONFIRMED: Two documented GnuTLS limitations: (a) MGF1 digest must equal the OAEP digest (XMLEnc 1.1 documents specifying different MGF1/OAEP digests are rejected here — parity gap vs the openssl backend which supports them); (b) RSA-OAEP with SHA1/SHA-224 fails at runtime in GnuTLS so it is rejected early (also re-checked in Execute for programmatically built transforms). Both are reject-rather-than-misimplement choices, not security weakenings; the runtime-failure claim itself not verifiable without web. Details: Both halves confirmed: (a) the OAEP/MGF1 digest coupling is an API limitation - GnuTLS/nettle expose rsa_oaep_sha256/rsa_pss... variants parameterized by a single hash (lib/nettle/pk.c), so MGF1 digest must equal the OAEP digest, unlike the openssl backend which supports distinct MGF1/OAEP digests; (b) RSA-OAEP with SHA1/SHA-224 fails at runtime: empirically gnutls_pubkey_encrypt_data/gnutls_privkey_decrypt_data with OAEP SHA1 or SHA224 returns GNUTLS_E_INVALID_REQUEST (-40) on GnuTLS 3.8.12, so the early rejection in kt_rsa.c (and the Execute re-check for programmatically built transforms) is warranted. Both are reject-rather-than-misimplement design choices, not security weakenings. + +### src/gnutls/asymkeys.c + +- src/gnutls/asymkeys.c:560:LOW:CONFIRMED: EC curve handling depends on gnutls_oid_to_ecc_curve accepting exactly the OID strings the core keysdata machinery stores in ecValue->curve (and gnutls_ecc_curve_get_oid returning the same form on write-back); format mismatch would surface as a hard error rather than misbehavior — runtime behavior not verifiable without building/testing here. Details: Empirically confirmed with installed GnuTLS 3.8.12: gnutls_oid_to_ecc_curve accepts exactly the dotted-decimal NIST OID strings the core EC keysdata machinery stores (1.2.840.10045.3.1.7 -> SECP256R1, 1.3.132.0.34 -> SECP384R1) and gnutls_ecc_curve_get_oid returns the same OID form on write-back; unrecognized strings map to GNUTLS_ECC_CURVE_INVALID -> a hard error rather than misbehavior. Round-trip works as assumed (informational). + +### src/gnutls/signatures.c + +- src/gnutls/signatures.c:1250:LOW:CONFIRMED: Verify error triage is correct (err>=0 ok, GNUTLS_E_PK_SIG_VERIFY_FAILED -> StatusFail, else hard error) and DER temp is gnutls_free'd — but note gnutls_pubkey_verify_data2/verify_hash2 return codes above 0 are treated as success per documented GnuTLS contract. + +### src/gnutls/x509.c + +- src/gnutls/x509.c:220:LOW:NOT-A-BUG: AddCertInternal dedup loop removes a content-equal cert via xmlSecPtrListRemove (src/list.c:398 invokes the klass destroyItem = crt deinit) then re-inserts; comment claims "certs are refcounted" which is inaccurate for gnutls_x509_crt_t but behavior is correct for distinct pointers, which is what internal callers always pass. +- src/gnutls/x509.c:223:MED:CONFIRMED: AddCertInternal is a latent API hazard: if a public-API caller ever passed the exact gnutls_x509_crt_t pointer already inside certsList, xmlSecPtrListRemove deinits that very object and the dangling pointer is re-inserted at line 234/240 -> use-after-free on later finalize. All in-tree call sites (CertRead results, CertDup copies) pass fresh not-in-list certs, so this is unconfirmed misuse-only exposure. Details: Verified latent API hazard (reachable via exported API misuse): the dedup loop (220-230) matches by gnutls_x509_crt_equals, then xmlSecPtrListRemove invokes the certsList DestroyItem handler which deinits the removed cert (x509utils.c:76 gnutls_x509_crt_deinit); the pointer is then re-inserted at 234/240. If the caller passes the exact pointer already in certsList - obtainable through the exported xmlSecGnuTLSKeyDataX509GetCert (include/xmlsec/gnutls/x509.h:49, documented borrowed pointer) fed back into xmlSecGnuTLSKeyDataX509AdoptCert/AdoptKeyCert (x509.h:44/47) - that is a use-after-free at finalize. The in-code comment at 219 claims certs are refcounted, which is false for gnutls_x509_crt_t. All in-tree call sites (CertRead results, CertDup copies) pass fresh objects, so this is misuse-only exposure, not a live in-tree defect. +- src/gnutls/x509.c:487:LOW:CONFIRMED: Duplicate() assumes ctxSrc->keyCert is pointer-identical to certsList[0]; if that invariant were violated, ctxDst->keyCert would be left stale and the non-NULL assert could pass against a pointer from the OTHER context — only reachable through already-broken state. Details: Verified at code level: Duplicate() copies certsList/crlsList then re-points ctxDst->keyCert by a pointer-identity search for ctxSrc->keyCert in the copied list; the invariant is documented only by the comment at 473 and the xmlSecAssert2(ctxDst->keyCert != NULL, -1) at 486. A violated invariant (already-broken state) leaves ctxDst->keyCert unset/stale -> defensive-only design note (informational). +- src/gnutls/x509.c:1041:LOW:CONFIRMED: gnutls_x509_crt_get_activation_time/get_expiration_time use (time_t)-1 both as error sentinel and as a representable timestamp — a cert with not-before == 1969-12-31 would be treated as error; also a parity note vs openssl/x509.c (~1636-1653) which tolerates missing notBefore/notAfter by storing 0 while this backend errors and fails key extraction. Details: Verified: the backend treats (time_t)-1 from gnutls_x509_crt_get_activation_time/get_expiration_time as an error (lines 1041-1046) - GnuTLS's own docs return "activation time, or (time_t)-1 on error", so a cert with not-before == epoch -1 would be indistinguishable from an error (theoretical); the openssl backend (x509.c:1639-1658) instead tolerates missing notBefore/notAfter by storing 0, while this backend errors and fails key extraction. Confirmed parity note (informational). + +### src/gnutls/x509utils.c + +- src/gnutls/x509utils.c:158:LOW:CONFIRMED: Stale "HACK" comment claims gnutls has no cert duplicate function; modern GnuTLS provides gnutls_x509_crt_dup — the DER export/re-import round trip in CertDup (and CrlDup twin at line 814) is needlessly expensive (serialize+parse per duplicateItem call on every list copy) and can normalize DER so a duplicated cert is not guaranteed byte-identical to the source (affects content-equality/digest comparisons). +- src/gnutls/x509utils.c:584:LOW:CONFIRMED: MatchBySubjectName/MatchByIssuer (618) return -1 (error) when the cert simply LACKS the queried attribute (GetSubjectDN/GetIssuerDN/GetIssuerSerial return NULL), which aborts the whole FindCertCtxMatch scan instead of treating that cert as non-matching — one such cert in the store fails the entire certificate search; openssl backend treats missing attributes as no-match. +- src/gnutls/x509utils.c:1176:LOW:NOT-A-BUG: On failure paths after *keyName was allocated it is left allocated while priv key is freed in done: — safe today because the only caller (xmlSecGnuTLSAppPkcs12LoadMemory in src/gnutls/app.c ~line 651) frees keyName on every path, but the cleanup contract is inconsistent/fragile for future callers. + +### src/gnutls/x509vfy.c + +- src/gnutls/x509vfy.c:1118:MED:CONFIRMED: FindIssuer loop does `is_issuer = gnutls_x509_crl_check_issuer(crl, cert); if(is_issuer == 0) continue;` — a NEGATIVE error code from check_issuer is treated as an issuer match, so a wrong issuer candidate can be adopted/returned and earlier-loop matches are not retried; not a security weakening (gnutls_x509_crl_verify enforces the CRL signature afterwards) but the error return is mishandled and can cause false rejections. +- src/gnutls/x509vfy.c:1596:LOW:CONFIRMED: FindSignedCert (and FindSignerCert at 1647) treats a NEGATIVE xmlSecGnuTLSX509DnsEqual return as "not equal" (== 1 test), silently swallowing internal errors such as DN-parse allocation failures — conservative direction (yields shorter chain that fails verification) but masks the real error from callers. + +### src/gnutls/app.c + +- src/gnutls/app.c:131:MED:CONFIRMED: xmlSecGnuTLSAppShutdown() calls deprecated gnutls_global_deinit() (init at line 112). Modern GnuTLS (3.x+) documents global_deinit as unnecessary (automatic on library destructor) and unsafe to call in shared libraries since it tears down process-global state that other components (libxml2 network/HTTPS layer, NSS co-existence) may rely on; worse, it is UNBALANCED: apps/xmlsec.c:3149 sets initialized=1 BEFORE xmlSecInit()/backend init, so a failed xmlSecAppInit still routes through main's goto done -> xmlSecAppShutdown() (1765), running gnutls_global_deinit() even when gnutls_global_init() never succeeded. Also inconsistent with xmlSecGnuTLSShutdown() in crypto.c which is a deliberate no-op. Candidate fix: drop the call or gate for GnuTLS < 3.3. +- src/gnutls/app.c:834:MED:CONFIRMED: Passwords are passed to GnuTLS byte-for-byte (safePwd/effectivePwd) with no encoding conversion; GnuTLS since 3.x treats passwords as UTF-8 (cf. --with-no-utf8-passwords), so a locale/Latin-1 non-ASCII password from CLI or callback hashes differently than in the OpenSSL/NSS backends — cross-backend divergence for encrypted PEM/PKCS#12 keys with non-ASCII passwords; needs an explicit decision/doc rather than silent behavior difference. Details: Verified code: safePwd/effectivePwd are passed to GnuTLS byte-for-byte (app.c:833-834) with no encoding conversion. Verified library behavior in the GnuTLS manual (doc/cha-gtls-app.texi, 'Strings and character sets'): 'All strings ... should be in UTF-8 ... When functions take as input passwords, they will normalize them using RFC7613 rules (since GnuTLS 3.5.7)' -> GnuTLS normalizes passwords (UTF-8/RFC7613) while the OpenSSL/NSS backends use the raw bytes, so encrypted PEM/PKCS#12 keys with non-ASCII passwords diverge across backends. Confirmed cross-backend divergence; needs an explicit project decision/doc rather than silent behavior difference. +- src/gnutls/app.c:621:LOW:CONFIRMED: PKCS#12 cert drain loop always removes index 0 from certsList via RemoveAndReturn -> O(n^2) with memmove; xmlSecPtrListRemoveAndReturnLast would be O(1) and exists — negligible for real cert counts. +- src/gnutls/app.c:1438:LOW:CONFIRMED: GetDefaultPwdCallback() returns NULL (in-file comment: no built-in prompt) so crypto.c:446 registers no default prompt; with no --pwd and no callback, encrypted PKCS#8/PKCS#12 keys are retried with EMPTY password and fail with a crypto error instead of prompting — deliberate but a real cross-backend behavior divergence (openssl/NSS prompt) worth surfacing in docs. + +### src/gcrypt/crypto.c + +- src/gcrypt/crypto.c:385:HIGH:CONFIRMED: xmlSecGCryptGenerateRandom ignores the return value of gcry_randomize (gpg_error_t; can fail on internal allocation / uninitialized self-check state) and returns 0 unconditionally — on failure callers (e.g. symkeys.c:149 random key generation, and same unchecked pattern at ciphers.c:100 for IVs, ciphers.c:266 for padding, kw_des.c:303) silently use non-random buffer content as fresh key material / IVs. Should check != 0 and report error. (Same unchecked-gcry_randomize pattern recurs in the other files.) +- src/gcrypt/crypto.c:117:LOW:CONFIRMED: Parity gap vs openssl: only DSA-SHA1 transform registered; DSA-SHA256 (registered unconditionally by openssl/crypto.c and implemented in openssl signatures) is neither registered nor implemented anywhere in this backend (signatures.c defines only DsaSha1Klass) despite libgcrypt supporting DSA/SHA-256 — intentionality undocumented. Details: Verified parity gap: gcrypt/crypto.c registers only transformDsaSha1GetKlass (117); openssl/crypto.c registers DSA-SHA1 (240) and DSA-SHA256 (244) unconditionally, and grep confirms no DsaSha256 klass/implementation anywhere in src/gcrypt (signatures.c defines DsaSha1 only). libgcrypt supports DSA/SHA-256 -> intentionality undocumented; DSA-SHA256 documents work on OpenSSL backends only (parity note). +- src/gcrypt/crypto.c:253:LOW:CONFIRMED: Parity gap vs openssl: entire SHA-224 family absent (Sha224 digest, RsaSha224, HmacSha224, EcdsaSha224, RsaPssSha224) and Sha3-224 absent; yet kt_rsa.c:549 maps an OAEP hash name "sha224", implying SHA-224 is expected to work here — inconsistent and undocumented. Details: Verified parity gap: the whole SHA-224 family (Sha224 digest, RsaSha224, HmacSha224, EcdsaSha224, RsaPssSha224) plus Sha3-224 is absent from src/gcrypt, while openssl registers them (crypto.c:262/279/323/392/413); yet gcrypt/kt_rsa.c:549 maps an OAEP hash name "sha224", implying SHA-224 is expected to work for key transport here -> inconsistent and undocumented (parity note). +- src/gcrypt/crypto.c:83:LOW:CONFIRMED: Parity gap: DEREncodedKeyValue key data registered unconditionally by openssl backend; not registered nor implemented anywhere in src/gcrypt. EdDSA/XDH/PBKDF2/HKDF/ConcatKDF key data also absent — those absences are expected gaps for this backend, DEREncodedKeyValue is less clearly intentional. Details: Verified parity gap: openssl/crypto.c:170 registers keyDataDEREncodedKeyValue unconditionally; grep for DEREncoded anywhere in src/gcrypt finds nothing (no klass, no implementation), while EdDSA/XDH/PBKDF2/HKDF/ConcatKDF absences are expected gaps for this backend. DEREncodedKeyValue is less clearly intentional (parity note). + +### src/gcrypt/ciphers.c + +- src/gcrypt/ciphers.c:100:MED:CONFIRMED: gcry_randomize() return (gpg_error_t) IGNORED when generating the random CBC IV: on failure (older libgcrypt init/allocation) the freshly expanded output region (not guaranteed zeroed by xmlSecBufferSetSize) is used as the IV silently — violates the check-all-returns rule; severity MED because libgcrypt >= 1.6 effectively never fails this call. +- src/gcrypt/ciphers.c:266:LOW:CONFIRMED: Same unchecked gcry_randomize pattern for the XML-ENC random padding bytes: on failure stale buffer content is encrypted as padding; decryption only inspects the length byte so no correctness impact, but intended random-padding property is not guaranteed. + +### src/gcrypt/symkeys.c + +- src/gcrypt/symkeys.c:149:LOW:CONFIRMED: Generate relies on xmlSecGCryptGenerateRandom whose gcry_randomize return is unchecked (known crypto.c:385 HIGH finding, not re-ranked): on randomization failure the "generated" key silently uses uninitialized/zeroed buffer content as key material. Details: Verified downstream consequence: xmlSecGCryptGenerateRandom (crypto.c:385 - the CONFIRMED HIGH finding, gcry_randomize return unchecked) is relied on by Generate at symkeys.c:149, so on randomization failure the "generated" key silently uses uninitialized/zeroed buffer content as key material. Recorded as a LOW consequence of the known HIGH item (not re-ranked). + +### src/gcrypt/kw_des.c + +- src/gcrypt/kw_des.c:311:MED:CONFIRMED: xmlSecGCryptKWDes3GenerateRandom ignores gcry_randomize's gpg_error_t return and sets *outWritten = outSize unconditionally — on failure the 8-byte KW check value is uninitialized/stale content wrapped into the output instead of failing the transform (same unchecked pattern as crypto.c:385 and ciphers.c:100/266; fix: check != GPG_ERR_NO_ERROR). +- src/gcrypt/kw_des.c:403:LOW:CONFIRMED: key_len = gcry_cipher_get_algo_keylen(GCRY_CIPHER_3DES) is a plain size_t (line 397) compared via xmlSecAssert2(keySize == key_len): if a libgcrypt build lacks 3DES, key_len = 0 turns misconfiguration into an opaque assert failure without a diagnostic — still safe (fails hard, no memory abuse), minor robustness/diagnosability gap. + +### src/gcrypt/kw_rfc_3394.c + +- src/gcrypt/kw_rfc_3394.c:433:MED:CONFIRMED: block-encrypt callback forwards caller pointers verbatim to gcry_cipher_encrypt; the CORE (src/kw_helpers.c:839, xmlSecKWRfc3394Encode NN==1 fast path) invokes this callback with in == out (same buffer) — libgcrypt manual states "overlapping buffers are not allowed" for the two-buffer form, so aliased call is documented UB even though CBC block-wise implementations typically survive it. Verify against libgcrypt implementation or use a scratch buffer. Details: Verified: the core xmlSecKWRfc3394Encode NN==1 fast path (kw_helpers.c:839) invokes the block-encrypt callback with in == out (same buffer), and the callback forwards both pointers verbatim to gcry_cipher_encrypt. The libgcrypt manual (doc/gcrypt.texi, gcry_cipher_encrypt: "It is not possible to use overlapping input and output buffers") documents this as UB - aliased call is a documented-UB pattern (current CBC block-wise implementations typically survive it; observed tests pass). Defensive note: scratch buffer recommended. +- src/gcrypt/kw_rfc_3394.c:480:MED:CONFIRMED: Same in==out aliasing on the decrypt side (core xmlSecKWRfc3394Decode NN==1 path, kw_helpers.c:910) forwarded straight to gcry_cipher_decrypt — documented-UB call pattern, same caveat as encrypt. Details: Verified same on the decrypt side: the core xmlSecKWRfc3394Decode NN==1 path (kw_helpers.c:910) passes in == out to the block-decrypt callback, forwarded straight to gcry_cipher_decrypt; same libgcrypt documented-overlap restriction as the encrypt item. +- src/gcrypt/kw_rfc_3394.c:140:LOW:CONFIRMED: blockSize validated only as > 0 but the code implicitly assumes blockSize == XMLSEC_KW_RFC3394_BLOCK_SIZE (16): per-block setiv always passes g_zero_iv with sizeof(g_zero_iv)==16 regardless of ctx->blockSize — add sanity assert blockSize == XMLSEC_KW_RFC3394_BLOCK_SIZE to make the invariant explicit instead of relying on setiv rejecting a wrong-size IV at runtime. + +### src/gcrypt/asn1.c + +- src/gcrypt/asn1.c:464:LOW:CONFIRMED: Auto key-type detection requires first integer >= 512 bits to classify 2-integer DER as RSA public — legitimate small (e.g. 256-bit test) RSA public DER keys get rejected in the Auto path; safe direction (error not wrong-key) but a functional coverage gap. +- src/gcrypt/asn1.c:126:LOW:CONFIRMED: DER parser accepts non-canonical multi-byte tags (0x3f 0x02 == INTEGER) — laxer than strict DER; memory-safe, strictness/quality only. +- src/gcrypt/asn1.c:167:LOW:CONFIRMED: Long-form lengths with leading zeros / non-minimal forms (0x81 0x05) accepted without minimality check — same lax-acceptance class as above. +- src/gcrypt/asn1.c:311:LOW:CONFIRMED: EC BIT STRING content taken verbatim without validating the leading "unused bits" octet == 0 (DER requires 0 for public points) — a non-conforming encoder yields a corrupted point integer silently (wrong q) rather than an error; conforming inputs unaffected. +- src/gcrypt/asn1.c:773:LOW:CONFIRMED: EC private key assumes OpenSSL/SEC1-flattened field order (d then q) — foreign encoders with literal SEC1 declaration order (publicKey before parameter) either error via curve-OID lookup or get rejected; no silent wrong-key for OID-bearing inputs. Coverage gap vs exotic encoders. + +### src/gcrypt/asn1.h + +- src/gcrypt/asn1.h:43:LOW:CONFIRMED: declares xmlSecGCryptParseDer using xmlSecKeyDataPtr but includes only exports.h + xmlsec.h — typedef comes from , so the header has an implicit include-order dependency (works today because asn1.c/app.c include keys.h first; any new includer following only this header's own includes would fail). Add #include . + +### src/gcrypt/kt_rsa.c + +- src/gcrypt/kt_rsa.c:836:MED:CONFIRMED: RSA-OAEP hard-requires mgf1 digest == OAEP digest (libgcrypt's built-in OAEP uses one hash for label hashing and MGF1; the sexp passes a single hash-algo). Spec-valid XMLEnc 1.1 documents with digest=SHA-256 and omitted mgfPrf (defaults MGF1-SHA1 per XMLEnc11 draft) decrypt fine on the openssl backend but hard-fail here — functional parity gap; limitation is documented in-file but full XMLEnc1.1 support would need hand-rolled MGF1. +- src/gcrypt/kt_rsa.c:775:LOW:CONFIRMED: No SHA3-224/256/384/512 branches for digest or MGF1 in the rsa-oaepenc-1.1 klass even though libgcrypt provides SHA3 hashes and the openssl backend registers SHA3 OAEP/MGF1 combos — gcrypt-vs-openssl parity gap. (SHA-224 IS accepted here, matching openssl; gnutls is the outlier rejecting it.) +- src/gcrypt/kt_rsa.c:1062:LOW:CONFIRMED: OAEP correctness relies on the linked libgcrypt recognizing (hash-algo %s) and (label %b) sexp tokens in (flags oaep) enc-val/dec-val (1.6+ feature); MGF1/label hashing NOT hand-rolled here. If an older libgcrypt silently ignored unknown sexp tokens, build succeeds but crypto silently uses SHA1/empty-label OAEP — wrong results not errors. Worth a capability guard against the minimum supported version. Details: Verified premise: OAEP correctness relies on the linked libgcrypt recognizing (hash-algo %s) and (label %b) sexp tokens for (flags oaep) enc-val/dec-val (OAEP sexp support added after 1.5.x, while the build guard requires only libgcrypt >= 1.4.0); MGF1/label hashing is NOT hand-rolled here. Refinement: current libgcrypt (cipher/pubkey-util.c _gcry_pk_util_parse_encval / NEWS) rejects unrecognized subpackets with an error (GPG_ERR_CONFLICT path) rather than silently ignoring them, and defaults OAEP to SHA1 (SHA-256 in FIPS mode), so older libgcrypt fails with an error rather than silently mis-applying SHA1/empty-label OAEP. Risk is limited to the version-support boundary; a capability guard at the minimum supported version remains worth documenting. +- src/gcrypt/kt_rsa.c:414:LOW:CONFIRMED: ~22-line modulus-block-size validation (find "n", nth_data, strip one leading 0x00, strict inSize == modulusSize) duplicated verbatim in RsaPkcs1Decrypt (414-435) and RsaOaepDecrypt (1035-1056) — should be a shared static helper; also the few early return(-1)s inside the block bypass the done: cleanup while siblings goto done — currently leak-free but fragile if the block grows. + +### src/gcrypt/asymkeys.c + +- src/gcrypt/asymkeys.c:276:LOW:CONFIRMED: DSA/RSA Generate wrappers mark key-type XMLSEC_ATTRIBUTE_UNUSED and always gcry_pk_genkey a full key pair, so GetType always reports PRIVATE|PUBLIC even when PUBLIC-only generation was requested — quality/API-semantics gap (matches sibling backends generating full pairs, but the requested type is silently ignored). +- src/gcrypt/asymkeys.c:1881:LOW:CONFIRMED: EC curve set is narrower than libgcrypt's full curve set (static OidToName table; unknown names/OIDs e.g. sm2p256v1/GOST curves/Curve25519 fail cleanly via NULL GetNameFromOid at XmlRead 1878-ish and XmlWrite ~2026) — intended safe failure, coverage note only. + +### src/gcrypt/signatures.c + +- src/gcrypt/signatures.c:1499:LOW:CONFIRMED: RSA-PSS Verify hardcodes (salt-length %u) = digest length, mirroring Sign (1385) — self-consistent but a spec-correct PSS verifier should accept any salt length >= 128 bits, so valid PSS signatures produced by other signers with salt length != hash length are rejected here. Details: Verified: Verify (1499) hardcodes (salt-length %u) = digest length, mirroring Sign (1385); self-consistent within the backend. RFC 8017 treats sLen as a variable parameter (salt length >= 8*halflen... i.e. not fixed to the hash length), so valid RSA-PSS signatures produced by other signers with a salt length != hash length are rejected here. Intentional design limitation (informational). +- src/gcrypt/signatures.c:982:LOW:CONFIRMED: Signature length pre-check mismatch (DSA 982, RSA 1275/1485, ECDSA 2316) returns -1 from the verify helper, which the transform Verify handler (608) turns into a HARD error instead of the conventional StatusFail/"data does not match" — safe direction (verification still fails, whole document aborts) but malformed signature values surface as internal errors rather than a clean mismatch result. + +### src/gcrypt/app.c + +- src/gcrypt/app.c:197:LOW:CONFIRMED: KeyLoadEx drops the `type` param (XMLSEC_ATTRIBUTE_UNUSED + XMLSEC_UNREFERENCED): unlike openssl, cannot filter/validate private-vs-public key by requested type — DER content decides; documented ("type ... not used") but a real parity gap if key-type filtering is ever expected. + +### src/gcrypt/globals.h + +- src/gcrypt/globals.h:37:LOW:CONFIRMED: backend silently #defines XMLSEC_NO_SHA3 1 when linked libgcrypt predates 1.8.0 — user-visible: a SHA3-enabled build against old libgcrypt compiles SHA3 out with no configure-time diagnostic; verified intentional per comment, but diverges from openssl/gnutls which never rewrite feature macros in globals.h. + +### src/gcrypt/Makefile.am + +- src/gcrypt/Makefile.am:1:LOW:NOT-A-BUG: SOURCES match disk; asn1.h listed inside _SOURCES (harmless automake treatment); no x509 files consistent with the gcrypt backend's lack of X.509 support. + +### docs/md/Makefile.am + +- docs/md/Makefile.am:57:MED:CONFIRMED: BUILD_MD_DOCS-disabled branch defines targets with COMMAS ("docs, docs-copy, docs-manpages:") — GNU make treats commas as part of target names, so the error recipe actually attaches to targets "docs,"/"docs-copy,"/"docs-manpages" and `make docs` silently succeeds (rc=0, verified empirically) instead of printing the intended ERROR; remove the commas. Sibling else-branches (docs/html, man) use the correct bare "docs:" syntax, making this a one-off typo. + +### xmlsec.pc.in + +- xmlsec.pc.in:1:LOW:CONFIRMED: "Cflags.private:" is not part of the classic pkg-config .pc key set — modern pkg-config (0.29+) tolerates/uses ".private" suffixed keys while older versions silently ignore them; verify against the pkg-config baseline distros ship before relying on -DXMLSEC_STATIC for static-link consumers (unconfirmable offline). Details: Verified: xmlsec.pc.in and all xmlsec-.pc.in carry 'Cflags.private: -DXMLSEC_STATIC' plus the modern suffixed-key convention; generated build-*/xmlsec1*.pc keep it and the local pkg-config (pkgconf 2.5.1) honors suffixed .private keys only for static-link queries (empirically: pkg-config --static --cflags emits -DXMLSEC_STATIC, plain --cflags does not). Suffixed keys were introduced in pkg-config 0.29 - older pkg-config silently ignores Cflags.private, so static-link consumers on such systems would not get -DXMLSEC_STATIC, which include/xmlsec/exports.h (#if !defined(XMLSEC_STATIC)) uses to control crypto entry-point declarations for statically linked consumers. Compat/design note: the project targets modern pkg-config. + +### xmlsec-gnutls.pc.in + +- xmlsec-gnutls.pc.in:9:MED:CONFIRMED: "Requires: gnutls >= X, libxml-2.0 >= Y ..." uses a COMMA after the first dependency — pkg-config's comma separator means OR, not AND: the whole Requires clause (incl. libxml-2.0) becomes optional for pkg-config consumers, unlike the openssl/nss pc.in files which use whitespace (AND); drop the comma to match siblings. + +### xmlsec-gcrypt.pc.in + +- xmlsec-gcrypt.pc.in:9:MED:CONFIRMED: same comma-vs-whitespace Requires inconsistency as xmlsec-gnutls.pc.in ("@GCRYPT_PACKAGE@ >= X, libxml-2.0 ...") — comma turns the AND dependency list into OR for modern pkg-config. + +### autogen.sh + +- autogen.sh:1:LOW:CONFIRMED: trivial autoreconf+configure wrapper builds IN-TREE by default ($srcdir/configure "$@" with no build-dir separation) — contradicts the project guideline against in-tree builds; users must add build-dir flags manually; the spec's ./autogen.sh usage depends on this in-tree behavior. + +### scripts/build_coverity.sh + +- scripts/build_coverity.sh:21:LOW:CONFIRMED: runs "$script_pwd/../configure" without regenerating it (no autoreconf) and assumes a pre-generated in-tree configure — stale configure vs current configure.ac silently analyzes different inputs; also under 'set -e' the final 'cd "$cur_pwd"' restore is skipped when curl/upload fails, leaving the caller's shell in the build dir (harness-level annoyance, not a build defect). + +### scripts/build_release.sh + +- scripts/build_release.sh:106:LOW:CONFIRMED: cleanup of $build_root is commented out (#rm -rf) leaving multi-hundred-MB /tmp build areas behind per release — intentional (keeps the clone for inspection) but undocumented in the script header; either document or gate behind an env knob. + +### scripts/README-WINDOWS.md.in + +- scripts/README-WINDOWS.md.in:12:MED:CONFIRMED: unbalanced double quotes in the shipped README's cmake invocation: `cmake -B " -A "..."` and `cmake --build " --config ...` — missing opening quote before makes the copy-paste command malformed (shell quote never closes on that line); both lines must read "-B \"\"" consistently with the correctly-quoted lines elsewhere in the file (create_readme() templates this file verbatim into the distributed distro README). diff --git a/src/nss/crypto.c b/src/nss/crypto.c index 1d1adb359..f1b6dfa51 100644 --- a/src/nss/crypto.c +++ b/src/nss/crypto.c @@ -39,6 +39,16 @@ static xmlSecCryptoDLFunctionsPtr gXmlSecNssFunctions = NULL; +/* + * NSS 3.103 and later have dedicated OIDs (SEC_OID_ECDH_KEA and + * SEC_OID_X25519) that allow the ECDH and X25519 key agreement algorithms + * to be checked against the NSS security policy; older NSS does not, so the + * closest available proxy, the CKM_ECDH1_DERIVE mechanism, is used for the + * policy check there. + */ +#if (NSS_VMAJOR > 3) || ((NSS_VMAJOR == 3) && (NSS_VMINOR >= 103)) +# define XMLSEC_NSS_HAS_KEY_AGREEMENT_OIDS 1 +#endif /* * Checks if a given algorithm is enabled in NSS. @@ -507,24 +517,22 @@ xmlSecNssUpdateAvailableCryptoTransforms(xmlSecCryptoDLFunctionsPtr functions) { /****** CHACHA20 ******/ #ifndef XMLSEC_NO_CHACHA20 - /* - * NSS has no OID for ChaCha20-Poly1305, so its availability cannot be - * checked against the NSS security policy; the transform is left - * registered and will fail at runtime if the mechanism is unsupported. - */ + if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_CHACHA20_POLY1305) == 0) { + functions->transformChaCha20Poly1305GetKlass = NULL; + } #endif /* XMLSEC_NO_CHACHA20 */ /****** DSA ******/ #ifndef XMLSEC_NO_DSA #ifndef XMLSEC_NO_SHA1 - if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST) == 0) { + if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA1) == 0)) { functions->transformDsaSha1GetKlass = NULL; } #endif /* XMLSEC_NO_SHA1 */ #ifndef XMLSEC_NO_SHA256 - if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_NIST_DSA_SIGNATURE_WITH_SHA256_DIGEST) == 0) { + if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_NIST_DSA_SIGNATURE_WITH_SHA256_DIGEST) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA256) == 0)) { functions->transformDsaSha256GetKlass = NULL; } #endif /* XMLSEC_NO_SHA256 */ @@ -533,50 +541,70 @@ xmlSecNssUpdateAvailableCryptoTransforms(xmlSecCryptoDLFunctionsPtr functions) { /****** XDH ******/ #ifndef XMLSEC_NO_XDH - /* - * NSS has no X25519-specific OID, so the ECDH-derive mechanism (which +#ifdef XMLSEC_NSS_HAS_KEY_AGREEMENT_OIDS + if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_X25519) == 0) { + functions->transformX25519GetKlass = NULL; + } +#else + /* NSS has no X25519-specific OID, so the ECDH-derive mechanism (which * maps to the ECDSA OID) is used as the closest available proxy for the - * NSS security policy check. - */ + * NSS security policy check */ if (xmlSecNssCryptoCheckMechanism(CKM_ECDH1_DERIVE) == 0) { functions->transformX25519GetKlass = NULL; } +#endif /* XMLSEC_NSS_HAS_KEY_AGREEMENT_OIDS */ #endif /* XMLSEC_NO_XDH */ /****** ECDSA ******/ #ifndef XMLSEC_NO_EC - /* key agreement (ECDH-ES): uses the same derive mechanism as X25519 */ +#ifdef XMLSEC_NSS_HAS_KEY_AGREEMENT_OIDS + /* + * Key agreement (ECDH-ES) uses the ECDH-derive mechanism. Several OIDs + * share CKM_ECDH1_DERIVE and NSS's mechanism map keeps only the last one + * (SEC_OID_ECDH_KEA), so that tag is what the security policy check + * resolves to; check it explicitly. + */ + if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_ECDH_KEA) == 0) { + functions->transformEcdhGetKlass = NULL; + } +#else + /* + * Key agreement (ECDH-ES) uses the ECDH-derive mechanism; older NSS has + * no dedicated OID for it, so the mechanism itself is the closest + * available proxy for the NSS security policy check + */ if (xmlSecNssCryptoCheckMechanism(CKM_ECDH1_DERIVE) == 0) { functions->transformEcdhGetKlass = NULL; } +#endif /* XMLSEC_NSS_HAS_KEY_AGREEMENT_OIDS */ #ifndef XMLSEC_NO_SHA1 - if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX962_ECDSA_SHA1_SIGNATURE) == 0) { + if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX962_ECDSA_SHA1_SIGNATURE) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA1) == 0)) { functions->transformEcdsaSha1GetKlass = NULL; } #endif /* XMLSEC_NO_SHA1 */ #ifndef XMLSEC_NO_SHA224 - if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX962_ECDSA_SHA224_SIGNATURE) == 0) { + if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX962_ECDSA_SHA224_SIGNATURE) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA224) == 0)) { functions->transformEcdsaSha224GetKlass = NULL; } #endif /* XMLSEC_NO_SHA224 */ #ifndef XMLSEC_NO_SHA256 - if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX962_ECDSA_SHA256_SIGNATURE) == 0) { + if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX962_ECDSA_SHA256_SIGNATURE) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA256) == 0)) { functions->transformEcdsaSha256GetKlass = NULL; } #endif /* XMLSEC_NO_SHA256 */ #ifndef XMLSEC_NO_SHA384 - if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX962_ECDSA_SHA384_SIGNATURE) == 0) { + if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX962_ECDSA_SHA384_SIGNATURE) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA384) == 0)) { functions->transformEcdsaSha384GetKlass = NULL; } #endif /* XMLSEC_NO_SHA384 */ #ifndef XMLSEC_NO_SHA512 - if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX962_ECDSA_SHA512_SIGNATURE) == 0) { + if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX962_ECDSA_SHA512_SIGNATURE) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA512) == 0)) { functions->transformEcdsaSha512GetKlass = NULL; } #endif /* XMLSEC_NO_SHA512 */ @@ -594,14 +622,6 @@ xmlSecNssUpdateAvailableCryptoTransforms(xmlSecCryptoDLFunctionsPtr functions) { /****** HMAC ******/ #ifndef XMLSEC_NO_HMAC -#ifndef XMLSEC_NO_RIPEMD160 - /* - * The NSS softoken does not support RipeMD160 and there is no OID - * mapping for CKM_RIPEMD160_HMAC, so this transform is never available. - */ - functions->transformHmacRipemd160GetKlass = NULL; -#endif /* XMLSEC_NO_RIPEMD160 */ - #ifndef XMLSEC_NO_SHA1 if (xmlSecNssCryptoCheckMechanism(CKM_SHA_1_HMAC) == 0) { functions->transformHmacSha1GetKlass = NULL; @@ -653,7 +673,7 @@ xmlSecNssUpdateAvailableCryptoTransforms(xmlSecCryptoDLFunctionsPtr functions) { #ifndef XMLSEC_NO_RSA #ifndef XMLSEC_NO_SHA1 - if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION) == 0) { + if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA1) == 0)) { functions->transformRsaSha1GetKlass = NULL; } @@ -663,7 +683,7 @@ xmlSecNssUpdateAvailableCryptoTransforms(xmlSecCryptoDLFunctionsPtr functions) { #endif /* XMLSEC_NO_SHA1 */ #ifndef XMLSEC_NO_SHA224 - if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_PKCS1_SHA224_WITH_RSA_ENCRYPTION) == 0) { + if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_PKCS1_SHA224_WITH_RSA_ENCRYPTION) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA224) == 0)) { functions->transformRsaSha224GetKlass = NULL; } @@ -673,7 +693,7 @@ xmlSecNssUpdateAvailableCryptoTransforms(xmlSecCryptoDLFunctionsPtr functions) { #endif /* XMLSEC_NO_SHA224 */ #ifndef XMLSEC_NO_SHA256 - if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION) == 0) { + if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA256) == 0)) { functions->transformRsaSha256GetKlass = NULL; } @@ -683,7 +703,7 @@ xmlSecNssUpdateAvailableCryptoTransforms(xmlSecCryptoDLFunctionsPtr functions) { #endif /* XMLSEC_NO_SHA256 */ #ifndef XMLSEC_NO_SHA384 - if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_PKCS1_SHA384_WITH_RSA_ENCRYPTION) == 0) { + if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_PKCS1_SHA384_WITH_RSA_ENCRYPTION) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA384) == 0)) { functions->transformRsaSha384GetKlass = NULL; } @@ -693,7 +713,7 @@ xmlSecNssUpdateAvailableCryptoTransforms(xmlSecCryptoDLFunctionsPtr functions) { #endif /* XMLSEC_NO_SHA384 */ #ifndef XMLSEC_NO_SHA512 - if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_PKCS1_SHA512_WITH_RSA_ENCRYPTION) == 0) { + if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_PKCS1_SHA512_WITH_RSA_ENCRYPTION) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA512) == 0)) { functions->transformRsaSha512GetKlass = NULL; } @@ -896,19 +916,19 @@ xmlSecNssGenerateRandom(xmlSecBufferPtr buffer, xmlSecSize size) { xmlSecAssert2(buffer != NULL, -1); xmlSecAssert2(size > 0, -1); + /* check the size fits into int before allocating */ + XMLSEC_SAFE_CAST_SIZE_TO_INT(size, len, return(-1), NULL); + ret = xmlSecBufferSetSize(buffer, size); if(ret < 0) { - xmlSecInternalError2("xmlSecBufferSetSize", NULL, - "size=" XMLSEC_SIZE_FMT, size); + xmlSecInternalError2("xmlSecBufferSetSize", NULL, "size=" XMLSEC_SIZE_FMT, size); return(-1); } /* get random data */ - XMLSEC_SAFE_CAST_SIZE_TO_INT(size, len, return(-1), NULL); rv = PK11_GenerateRandom((xmlSecByte*)xmlSecBufferGetData(buffer), len); if(rv != SECSuccess) { - xmlSecNssError2("PK11_GenerateRandom", NULL, - "size=" XMLSEC_SIZE_FMT, size); + xmlSecNssError2("PK11_GenerateRandom", NULL, "size=" XMLSEC_SIZE_FMT, size); return(-1); } return(0); diff --git a/src/nss/kdf.c b/src/nss/kdf.c index 10b3aae49..f3cbe1051 100644 --- a/src/nss/kdf.c +++ b/src/nss/kdf.c @@ -406,7 +406,8 @@ xmlSecNssConcatKdfGenerateKey(xmlSecNssKdfCtxPtr ctx, xmlSecSize outLen, xmlSecB xmlSecByte hashBuf[XMLSEC_NSS_KDF_MAX_HASH_SIZE]; xmlSecByte counter[4]; uint32_t counterVal; - PK11Context* hashCtx; + xmlSecSize toCopy, hashSize; + PK11Context* hashCtx = NULL; SECStatus rv; int ret; int res = -1; @@ -442,6 +443,15 @@ xmlSecNssConcatKdfGenerateKey(xmlSecNssKdfCtxPtr ctx, xmlSecSize outLen, xmlSecB outData = xmlSecBufferGetData(out); xmlSecAssert2(outData != NULL, -1); + /* one digest context is reused for all counter blocks: PK11_DigestBegin + * starts a fresh operation on each block, since PK11_DigestFinal clears + * the previous one (Begin after Final is an explicit NSS-supported pattern) */ + hashCtx = PK11_CreateDigestContext(oidData->offset); + if(hashCtx == NULL) { + xmlSecNssError("PK11_CreateDigestContext", NULL); + goto done; + } + pos = 0; counterVal = 1; while(pos < outLen) { @@ -456,67 +466,50 @@ xmlSecNssConcatKdfGenerateKey(xmlSecNssKdfCtxPtr ctx, xmlSecSize outLen, xmlSecB counter[2] = (xmlSecByte)((counterVal >> 8) & 0xFF); counter[3] = (xmlSecByte)(counterVal & 0xFF); - hashCtx = PK11_CreateDigestContext(oidData->offset); - if(hashCtx == NULL) { - xmlSecNssError("PK11_CreateDigestContext", NULL); - goto done; - } - rv = PK11_DigestBegin(hashCtx); if(rv != SECSuccess) { xmlSecNssError("PK11_DigestBegin", NULL); - PK11_DestroyContext(hashCtx, PR_TRUE); goto done; } rv = PK11_DigestOp(hashCtx, counter, 4); if(rv != SECSuccess) { xmlSecNssError("PK11_DigestOp(counter)", NULL); - PK11_DestroyContext(hashCtx, PR_TRUE); goto done; } - XMLSEC_SAFE_CAST_SIZE_TO_UINT(keySize, keyLen, - PK11_DestroyContext(hashCtx, PR_TRUE); goto done, NULL); + XMLSEC_SAFE_CAST_SIZE_TO_UINT(keySize, keyLen, goto done, NULL); rv = PK11_DigestOp(hashCtx, keyData, keyLen); if(rv != SECSuccess) { xmlSecNssError("PK11_DigestOp(Z)", NULL); - PK11_DestroyContext(hashCtx, PR_TRUE); goto done; } if((fixedInfoData != NULL) && (fixedInfoSize > 0)) { unsigned int fixedInfoLen; - XMLSEC_SAFE_CAST_SIZE_TO_UINT(fixedInfoSize, fixedInfoLen, - PK11_DestroyContext(hashCtx, PR_TRUE); goto done, NULL); + XMLSEC_SAFE_CAST_SIZE_TO_UINT(fixedInfoSize, fixedInfoLen, goto done, NULL); rv = PK11_DigestOp(hashCtx, fixedInfoData, fixedInfoLen); if(rv != SECSuccess) { xmlSecNssError("PK11_DigestOp(OtherInfo)", NULL); - PK11_DestroyContext(hashCtx, PR_TRUE); goto done; } } hashLen = XMLSEC_NSS_KDF_MAX_HASH_SIZE; rv = PK11_DigestFinal(hashCtx, hashBuf, &hashLen, XMLSEC_NSS_KDF_MAX_HASH_SIZE); - PK11_DestroyContext(hashCtx, PR_TRUE); if(rv != SECSuccess) { xmlSecNssError("PK11_DigestFinal", NULL); goto done; } + XMLSEC_SAFE_CAST_UINT_TO_SIZE(hashLen, hashSize, goto done, NULL); - { - xmlSecSize toCopy; - - toCopy = outLen - pos; - if(toCopy > (xmlSecSize)hashLen) { - toCopy = (xmlSecSize)hashLen; - } - memcpy(outData + pos, hashBuf, toCopy); - pos += toCopy; + toCopy = outLen - pos; + if(toCopy > hashSize) { + toCopy = hashSize; } - + memcpy(outData + pos, hashBuf, toCopy); + pos += toCopy; counterVal++; } @@ -524,6 +517,9 @@ xmlSecNssConcatKdfGenerateKey(xmlSecNssKdfCtxPtr ctx, xmlSecSize outLen, xmlSecB res = 0; done: + if(hashCtx != NULL) { + PK11_DestroyContext(hashCtx, PR_TRUE); + } xmlSecMemCleanse(hashBuf, sizeof(hashBuf)); return(res); } diff --git a/src/nss/key_agrmnt.c b/src/nss/key_agrmnt.c index 47bff734a..fbbabca93 100644 --- a/src/nss/key_agrmnt.c +++ b/src/nss/key_agrmnt.c @@ -241,6 +241,7 @@ xmlSecNssKeyAgreementExecute(xmlSecTransformPtr transform, int last, xmlSecTrans int ret; xmlSecAssert2(xmlSecTransformIsValid(transform), -1); + xmlSecAssert2(xmlSecTransformCheckSize(transform, xmlSecNssKeyAgreementSize), -1); xmlSecAssert2(((transform->operation == xmlSecTransformOperationEncrypt) || (transform->operation == xmlSecTransformOperationDecrypt)), -1); xmlSecAssert2(transformCtx != NULL, -1); @@ -260,7 +261,9 @@ xmlSecNssKeyAgreementExecute(xmlSecTransformPtr transform, int last, xmlSecTrans } else if((transform->status == xmlSecTransformStatusWorking) && (last != 0)) { xmlSecBuffer secret; - ret = xmlSecBufferInitialize(&secret, 64); + /* 128 covers the largest supported ECDH secret (P-521: 66 bytes); + * the buffer grows automatically if a larger secret is produced */ + ret = xmlSecBufferInitialize(&secret, 128); if(ret < 0) { xmlSecInternalError("xmlSecBufferInitialize", xmlSecTransformGetName(transform)); return(-1); @@ -319,6 +322,8 @@ xmlSecNssKeyAgreementGenerateSecret(xmlSecNssKeyAgreementCtxPtr ctx, SECItem *keyData = NULL; SECStatus rv; xmlSecSize secretSize; + xmlSecSize expectedSecretLen; + unsigned int keyStrength; int ret; int res = -1; @@ -366,6 +371,20 @@ xmlSecNssKeyAgreementGenerateSecret(xmlSecNssKeyAgreementCtxPtr ctx, goto done; } + /* determine the expected shared-secret length. X25519 always yields 32 + * bytes (set in Initialize); ECDH depends on the curve, so derive the + * field size in bytes from the peer's public key (SECKEY_PublicKeyStrength + * returns 0 for unknown curve OIDs, which keeps the check disabled). This + * catches token providers that strip leading zero bytes from the X9.63 + * shared secret, which xmlenc-core1 forbids (fixed-width, left-padded). */ + expectedSecretLen = ctx->expected_secret_len; + if(expectedSecretLen == 0) { + keyStrength = SECKEY_PublicKeyStrength(otherPubKey); + if(keyStrength > 0) { + XMLSEC_SAFE_CAST_UINT_TO_SIZE(keyStrength, expectedSecretLen, goto done, NULL); + } + } + /* derive shared secret via PKCS#11 CKM_ECDH1_DERIVE; NSS routes this to the * correct ECDH operation for both regular EC and Montgomery-curve (X25519) * keys based on the key type, not the mechanism alone. Note: routing of @@ -393,16 +412,28 @@ xmlSecNssKeyAgreementGenerateSecret(xmlSecNssKeyAgreementCtxPtr ctx, * prohibits extraction, move it to the software slot first */ rv = PK11_ExtractKeyValue(symKey); if(rv != SECSuccess) { - PK11SlotInfo *internalSlot = PK11_GetInternalSlot(); - if(internalSlot != NULL) { - PK11SymKey *movedKey = PK11_MoveSymKey(internalSlot, CKA_DERIVE, 0, PR_FALSE, symKey); + PK11SlotInfo *internalSlot; + PK11SymKey *movedKey; + + internalSlot = PK11_GetInternalSlot(); + if(internalSlot == NULL) { + xmlSecNssError("PK11_GetInternalSlot", NULL); + goto done; + } + /* PK11_MoveSymKey does not destroy the original key, so free it + * explicitly on success; on failure symKey remains valid and is + * freed in the done block */ + movedKey = PK11_MoveSymKey(internalSlot, CKA_DERIVE, 0, PR_FALSE, symKey); + if(movedKey == NULL) { + xmlSecNssError("PK11_MoveSymKey", NULL); PK11_FreeSlot(internalSlot); - if(movedKey != NULL) { - PK11_FreeSymKey(symKey); - symKey = movedKey; - rv = PK11_ExtractKeyValue(symKey); - } + goto done; } + PK11_FreeSlot(internalSlot); + PK11_FreeSymKey(symKey); + symKey = movedKey; + + rv = PK11_ExtractKeyValue(symKey); } if(rv != SECSuccess) { xmlSecNssError("PK11_ExtractKeyValue", NULL); @@ -417,10 +448,10 @@ xmlSecNssKeyAgreementGenerateSecret(xmlSecNssKeyAgreementCtxPtr ctx, /* validate secret length */ XMLSEC_SAFE_CAST_UINT_TO_SIZE(keyData->len, secretSize, goto done, NULL); - if((ctx->expected_secret_len != 0) && (secretSize != ctx->expected_secret_len)) { + if((expectedSecretLen != 0) && (secretSize != expectedSecretLen)) { char expectedLenStr[32]; - ret = xmlStrPrintf(BAD_CAST expectedLenStr, sizeof(expectedLenStr), XMLSEC_SIZE_FMT, ctx->expected_secret_len); + ret = xmlStrPrintf(BAD_CAST expectedLenStr, sizeof(expectedLenStr), XMLSEC_SIZE_FMT, expectedSecretLen); if(ret < 0) { xmlSecInternalError("xmlStrPrintf", NULL); goto done; diff --git a/src/nss/keytrans.c b/src/nss/keytrans.c index 36656b2cb..ddae0ac5b 100644 --- a/src/nss/keytrans.c +++ b/src/nss/keytrans.c @@ -125,12 +125,17 @@ xmlSecNssKeyTransportInitialize(xmlSecTransformPtr transform) { #endif /* XMLSEC_NO_RSA_PKCS15 */ #ifndef XMLSEC_NO_RSA_OAEP + /* The RSA-OAEP transforms default to the SHA-1 digest and MGF1 + * prescribed by the XML Encryption spec; xmlSecNssRsaOaepNodeRead + * overrides these when an explicit digest is provided. With + * XMLSEC_NO_SHA1 the defaults are left at 0 and the klass remains + * registered as available; the failure is then reported at runtime by + * xmlSecNssKeyTransportSetOaepParams as a DISABLED error (un-registering + * the klass in such a build would require reworking the availability + * reporting) */ if(transform->id == xmlSecNssTransformRsaOaepId) { context->cipher = CKM_RSA_PKCS_OAEP; context->keyId = xmlSecNssKeyDataRsaId; - /* default to the SHA-1 digest and MGF1 prescribed by the XML - * Encryption spec; xmlSecNssRsaOaepNodeRead overrides these when an - * explicit digest is provided */ #ifndef XMLSEC_NO_SHA1 context->oaepHashAlg = CKM_SHA_1; context->oaepMgf = CKG_MGF1_SHA1; @@ -298,6 +303,119 @@ xmlSecNssKeyTransportGetBlockSize(xmlSecNssKeyTransportCtxPtr ctx, xmlSecSize* b return(0); } +/* Returns the digest length in bytes for the given PKCS#11 digest + * mechanism used as the RSA-OAEP hash algorithm and 0 if the mechanism + * is not supported. NSS does not provide a public API to convert a + * mechanism to the digest length it produces. */ +#ifndef XMLSEC_NO_RSA_OAEP +static xmlSecSize +xmlSecNssKeyTransportOaepHashLen(CK_MECHANISM_TYPE hashAlg) { + switch(hashAlg) { +#ifndef XMLSEC_NO_SHA1 + case CKM_SHA_1: + return(SHA1_LENGTH); +#endif /* XMLSEC_NO_SHA1 */ +#ifndef XMLSEC_NO_SHA224 + case CKM_SHA224: + return(SHA224_LENGTH); +#endif /* XMLSEC_NO_SHA224 */ +#ifndef XMLSEC_NO_SHA256 + case CKM_SHA256: + return(SHA256_LENGTH); +#endif /* XMLSEC_NO_SHA256 */ +#ifndef XMLSEC_NO_SHA384 + case CKM_SHA384: + return(SHA384_LENGTH); +#endif /* XMLSEC_NO_SHA384 */ +#ifndef XMLSEC_NO_SHA512 + case CKM_SHA512: + return(SHA512_LENGTH); +#endif /* XMLSEC_NO_SHA512 */ +#ifndef XMLSEC_NO_SHA3 + case CKM_SHA3_224: + return(SHA3_224_LENGTH); + case CKM_SHA3_256: + return(SHA3_256_LENGTH); + case CKM_SHA3_384: + return(SHA3_384_LENGTH); + case CKM_SHA3_512: + return(SHA3_512_LENGTH); +#endif /* XMLSEC_NO_SHA3 */ + default: + /* unsupported mechanism */ + return(0); + } +} +#endif /* XMLSEC_NO_RSA_OAEP */ + +/* Returns the maximum size of the raw key material that can be wrapped + * with the cipher configured in @p ctx (PKCS#1 v1.5: blockSize - 11, + * RSA-OAEP: blockSize - 2 - 2*hashLen, where hashLen is the length of the + * OAEP digest) and stores it in @p maxMaterialSize. Returns 0 on success + * or a negative value on error. If the RSA-OAEP digest algorithm has not + * been set yet (for example when the SHA-1 digest is disabled and no + * explicit digest was specified), the limit is set to the modulus size, + * which the caller already enforces, so that the specific error is + * reported by xmlSecNssKeyTransportSetOaepParams. */ +static int +xmlSecNssKeyTransportGetMaxMaterialSize(xmlSecNssKeyTransportCtxPtr ctx, xmlSecSize* maxMaterialSize) { + xmlSecSize blockSize; + xmlSecSize overhead; + int ret; + + xmlSecAssert2(ctx != NULL, -1); + xmlSecAssert2(maxMaterialSize != NULL, -1); + + ret = xmlSecNssKeyTransportGetBlockSize(ctx, &blockSize); + if(ret < 0) { + return(ret); + } + + if(ctx->cipher == CKM_RSA_PKCS) { + /* PKCS#1 v1.5 (EMSA-PKCS1-v1_5): 0x00 || 0x02 || PS (>= 8 bytes) || 0x00 */ + overhead = 11; + } else +#ifndef XMLSEC_NO_RSA_OAEP + if(ctx->cipher == CKM_RSA_PKCS_OAEP) { + xmlSecSize hashLen; + + if(ctx->oaepHashAlg == 0) { + /* the digest algorithm has not been set; fall back to the + * modulus size so that xmlSecNssKeyTransportSetOaepParams + * reports the specific error */ + (*maxMaterialSize) = blockSize; + return(0); + } + + hashLen = xmlSecNssKeyTransportOaepHashLen(ctx->oaepHashAlg); + if(hashLen == 0) { + xmlSecInternalError2("xmlSecNssKeyTransportOaepHashLen", NULL, + "hashAlg=0x%08x", (unsigned int)ctx->oaepHashAlg); + return(-1); + } + /* EME-OAEP: 0x00 || lHash (hashLen) || seed (hashLen) || ... || 0x01 */ + overhead = 2 + 2*hashLen; + } else +#endif /* XMLSEC_NO_RSA_OAEP */ + { + xmlSecInternalError2("unsupported keywrap cipher", NULL, + "cipher=0x%08x", (unsigned int)ctx->cipher); + return(-1); + } + + if(blockSize <= overhead) { + xmlSecInternalError3("key too small for the keywrap cipher", NULL, + "blockSize=" XMLSEC_SIZE_FMT ", overhead=" XMLSEC_SIZE_FMT, + blockSize, overhead); + return(-1); + } + + (*maxMaterialSize) = blockSize - overhead; + + /* done */ + return(0); +} + static int xmlSecNssKeyTransportCtxInit(xmlSecNssKeyTransportCtxPtr ctx, xmlSecBufferPtr in, xmlSecBufferPtr out, int encrypt, xmlSecTransformCtxPtr transformCtx) { @@ -308,16 +426,11 @@ xmlSecNssKeyTransportCtxInit(xmlSecNssKeyTransportCtxPtr ctx, xmlSecBufferPtr in xmlSecAssert2(ctx->cipher != CKM_INVALID_MECHANISM, -1); xmlSecAssert2((ctx->pubkey != NULL && encrypt) || (ctx->prikey != NULL && !encrypt), -1); xmlSecAssert2(ctx->keyId != NULL, -1); + xmlSecAssert2(ctx->material == NULL, -1); xmlSecAssert2(in != NULL, -1); xmlSecAssert2(out != NULL, -1); xmlSecAssert2(transformCtx != NULL, -1); - if(ctx->material != NULL) { - /* automatically cleansed by xmlSecBufferDestroy */ - xmlSecBufferDestroy(ctx->material); - ctx->material = NULL; - } - ret = xmlSecNssKeyTransportGetBlockSize(ctx, &blockSize); if(ret < 0) { xmlSecInternalError("xmlSecNssKeyTransportGetBlockSize", NULL); @@ -417,14 +530,25 @@ xmlSecNssKeyTransportSetOaepParams(xmlSecNssKeyTransportCtxPtr ctx, CK_RSA_PKCS_ return(-1); } + /* the same applies to the MGF1 digest algorithm; the two fields are set + * together today, but a zeroed MGF must not be passed to NSS */ + if(ctx->oaepMgf == 0) { + xmlSecOtherError(XMLSEC_ERRORS_R_DISABLED, NULL, + "OAEP mgf1 digest algorithm is not set and the default SHA1 digest is disabled"); + return(-1); + } + oaepParams->hashAlg = ctx->oaepHashAlg; oaepParams->mgf = ctx->oaepMgf ; oaepParams->source = CKZ_DATA_SPECIFIED; - oaepParams->pSourceData = xmlSecBufferGetData(&(ctx->oaepParams)); + /* NSS (softoken) rejects a zero-length label passed with a non-NULL + * pSourceData, and xmlSecBufferGetData() returns the allocated + * pointer of a buffer that was ever grown, so normalize the empty + * label to pSourceData == NULL */ size = xmlSecBufferGetSize(&(ctx->oaepParams)); + oaepParams->pSourceData = (size > 0 )? xmlSecBufferGetData(&(ctx->oaepParams)) : NULL; XMLSEC_SAFE_CAST_SIZE_TO_ULONG(size, oaepParams->ulSourceDataLen, return(-1), NULL); - return(0); } #endif /* XMLSEC_NO_RSA_OAEP */ @@ -525,14 +649,38 @@ xmlSecNssKeyTransportCtxFinal(xmlSecNssKeyTransportCtxPtr ctx, xmlSecBufferPtr i /* Now we get all of the key material */ /* from now on we will wrap or unwrap the key */ - if(materialSize == 0) { - /* an empty key material is never a valid key to wrap nor a valid - * ciphertext to unwrap */ + if(encrypt == 0) { + /* a valid ciphertext is exactly the modulus size; empty and short + * encodings are rejected through the unified decrypt error channel + * (NSS would zero-pad short inputs and unwrap them as the same RSA + * integer, making the ciphertext malleable; the OpenSSL backend + * enforces the exact size too, see src/openssl/kt_rsa.c) */ + if(materialSize != blockSize) { + xmlSecInvalidSizeError("Encrypted input data", materialSize, blockSize, NULL); + return(-1); + } + } else if(materialSize == 0) { + /* an empty key material is never a valid key to wrap */ xmlSecInvalidZeroKeyDataSizeError(NULL); return(-1); + } else { + /* the encoded key blob is at most the modulus size minus the cipher + * padding overhead, so reject the material that cannot possibly fit + * before the wrap is attempted (matches the OpenSSL backend + * behavior, see src/openssl/kt_rsa.c) */ + xmlSecSize maxMaterialSize; + + ret = xmlSecNssKeyTransportGetMaxMaterialSize(ctx, &maxMaterialSize); + if(ret < 0) { + return(-1); + } + if(materialSize > maxMaterialSize) { + xmlSecInvalidSizeMoreThanError("Input data", materialSize, maxMaterialSize, NULL); + return(-1); + } } - result = xmlSecBufferCreate(blockSize * 2); + result = xmlSecBufferCreate(blockSize); if(result == NULL) { xmlSecInternalError("xmlSecBufferCreate", NULL); return(-1); @@ -713,14 +861,13 @@ xmlSecNssKeyTransportExecute(xmlSecTransformPtr transform, int last, xmlSecTrans return(-1); } } + xmlSecAssert2(context->material != NULL, -1); - if(context->material != NULL) { - rtv = xmlSecNssKeyTransportCtxUpdate(context, inBuf, outBuf, operation, transformCtx); - if(rtv < 0) { - xmlSecInternalError("xmlSecNssKeyTransportCtxUpdate", - xmlSecTransformGetName(transform)); - return(-1); - } + rtv = xmlSecNssKeyTransportCtxUpdate(context, inBuf, outBuf, operation, transformCtx); + if(rtv < 0) { + xmlSecInternalError("xmlSecNssKeyTransportCtxUpdate", + xmlSecTransformGetName(transform)); + return(-1); } if(last) { @@ -897,7 +1044,10 @@ xmlSecNssRsaOaepNodeRead(xmlSecTransformPtr transform, xmlNodePtr node, return(-1); } - /* digest algorithm */ + /* digest algorithm. Note: xmlenc#sha128 (MD5) and xmlenc#sha160 + * (RIPEMD160) are intentionally not mapped: the NSS softoken only + * accepts the SHA-1/224/256/384/512 digests for OAEP, so any other + * mapping would be rejected at runtime anyway */ if (oaepParams.digestAlgorithm == NULL) { #ifndef XMLSEC_NO_SHA1 ctx->oaepHashAlg = CKM_SHA_1; diff --git a/src/nss/kw_des.c b/src/nss/kw_des.c index 93590e691..2d57c46dd 100644 --- a/src/nss/kw_des.c +++ b/src/nss/kw_des.c @@ -77,25 +77,37 @@ static xmlSecKWDes3Klass xmlSecNssKWDes3ImplKlass = { NULL, /* void* reserved1; */ }; -static int xmlSecNssKWDes3Encrypt (const xmlSecByte *key, - xmlSecSize keySize, - const xmlSecByte *iv, - xmlSecSize ivSize, - const xmlSecByte *in, - xmlSecSize inSize, - xmlSecByte *out, - xmlSecSize outSize, - xmlSecSize * outWritten, - int enc); - - /****************************************************************************** * * Triple DES Key Wrap transform context * *****************************************************************************/ -typedef xmlSecTransformKWDes3Ctx xmlSecNssKWDes3Ctx, - *xmlSecNssKWDes3CtxPtr; +/* the cipher mechanism used by the implementation; the slot and the + symmetric key cached in the transform context are bound to it */ +#define XMLSEC_NSS_KW_DES3_CIPHER_MECH CKM_DES3_CBC + +typedef struct _xmlSecNssKWDes3Ctx xmlSecNssKWDes3Ctx, + *xmlSecNssKWDes3CtxPtr; + +struct _xmlSecNssKWDes3Ctx { + xmlSecTransformKWDes3Ctx parentCtx; + PK11SlotInfo* slot; + PK11SymKey* symKey; + CK_ATTRIBUTE_TYPE symKeyOp; + xmlSecBuffer scratchIn; +}; + +static int xmlSecNssKWDes3EnsureKey (xmlSecNssKWDes3CtxPtr ctx, + CK_ATTRIBUTE_TYPE symKeyOp); +static int xmlSecNssKWDes3Encrypt (xmlSecNssKWDes3CtxPtr ctx, + CK_ATTRIBUTE_TYPE op, + const xmlSecByte *iv, + xmlSecSize ivSize, + const xmlSecByte *in, + xmlSecSize inSize, + xmlSecByte *out, + xmlSecSize outSize, + xmlSecSize * outWritten); /****************************************************************************** * @@ -162,12 +174,32 @@ xmlSecNssKWDes3Initialize(xmlSecTransformPtr transform) { xmlSecAssert2(ctx != NULL, -1); memset(ctx, 0, sizeof(xmlSecNssKWDes3Ctx)); - ret = xmlSecTransformKWDes3Initialize(transform, ctx, + /* the scratch buffer is secure, so the key wrap data it holds is wiped + on resize and on context finalization */ + ret = xmlSecBufferInitialize(&(ctx->scratchIn), 0); + if(ret < 0) { + xmlSecInternalError("xmlSecBufferInitialize", xmlSecTransformGetName(transform)); + xmlSecNssKWDes3Finalize(transform); + return(-1); + } + xmlSecBufferMakeSecure(&(ctx->scratchIn)); + + ctx->slot = PK11_GetBestSlot(XMLSEC_NSS_KW_DES3_CIPHER_MECH, NULL); + if(ctx->slot == NULL) { + xmlSecNssError("PK11_GetBestSlot", NULL); + xmlSecNssKWDes3Finalize(transform); + return(-1); + } + + ret = xmlSecTransformKWDes3Initialize(transform, &(ctx->parentCtx), &xmlSecNssKWDes3ImplKlass, xmlSecNssKeyDataDesId); if(ret < 0) { xmlSecInternalError("xmlSecTransformKWDes3Initialize", xmlSecTransformGetName(transform)); + xmlSecNssKWDes3Finalize(transform); return(-1); } + + /* done */ return(0); } @@ -181,7 +213,17 @@ xmlSecNssKWDes3Finalize(xmlSecTransformPtr transform) { ctx = xmlSecNssKWDes3GetCtx(transform); xmlSecAssert(ctx != NULL); - xmlSecTransformKWDes3Finalize(transform, ctx); + if(ctx->symKey != NULL) { + PK11_FreeSymKey(ctx->symKey); + ctx->symKey = NULL; + } + if(ctx->slot != NULL) { + PK11_FreeSlot(ctx->slot); + ctx->slot = NULL; + } + xmlSecBufferFinalize(&(ctx->scratchIn)); + xmlSecTransformKWDes3Finalize(transform, &(ctx->parentCtx)); + memset(ctx, 0, sizeof(xmlSecNssKWDes3Ctx)); } @@ -196,7 +238,7 @@ xmlSecNssKWDes3SetKeyReq(xmlSecTransformPtr transform, xmlSecKeyReqPtr keyReq) ctx = xmlSecNssKWDes3GetCtx(transform); xmlSecAssert2(ctx != NULL, -1); - ret = xmlSecTransformKWDes3SetKeyReq(transform, ctx, keyReq); + ret = xmlSecTransformKWDes3SetKeyReq(transform, &(ctx->parentCtx), keyReq); if(ret < 0) { xmlSecInternalError("xmlSecTransformKWDes3SetKeyReq", xmlSecTransformGetName(transform)); @@ -216,11 +258,20 @@ xmlSecNssKWDes3SetKey(xmlSecTransformPtr transform, xmlSecKeyPtr key) { ctx = xmlSecNssKWDes3GetCtx(transform); xmlSecAssert2(ctx != NULL, -1); - ret = xmlSecTransformKWDes3SetKey(transform, ctx, key); + ret = xmlSecTransformKWDes3SetKey(transform, &(ctx->parentCtx), key); if(ret < 0) { xmlSecInternalError("xmlSecTransformKWDes3SetKey", xmlSecTransformGetName(transform)); return(-1); } + + /* the cached symmetric key was created with the previous key material; + release it so it is re-imported with the new key on the next operation */ + if(ctx->symKey != NULL) { + PK11_FreeSymKey(ctx->symKey); + ctx->symKey = NULL; + ctx->symKeyOp = 0; + } + return(0); } @@ -236,7 +287,7 @@ xmlSecNssKWDes3Execute(xmlSecTransformPtr transform, int last, ctx = xmlSecNssKWDes3GetCtx(transform); xmlSecAssert2(ctx != NULL, -1); - ret = xmlSecTransformKWDes3Execute(transform, ctx, last); + ret = xmlSecTransformKWDes3Execute(transform, &(ctx->parentCtx), last); if(ret < 0) { xmlSecInternalError("xmlSecTransformKWDes3Execute", xmlSecTransformGetName(transform)); return(-1); @@ -296,11 +347,17 @@ xmlSecNssKWDes3Sha1(xmlSecTransformPtr transform XMLSEC_ATTRIBUTE_UNUSED, return(-1); } + if (outLen != SHA1_LENGTH) { + xmlSecInternalError3("xmlSecNssKWDes3Sha1", NULL, + "digest length=" XMLSEC_SIZE_FMT ", expected " XMLSEC_SIZE_FMT, + (xmlSecSize)outLen, (xmlSecSize)SHA1_LENGTH); + PK11_DestroyContext(pk11ctx, PR_TRUE); + return(-1); + } + /* done */ PK11_DestroyContext(pk11ctx, PR_TRUE); - xmlSecAssert2(outLen == SHA1_LENGTH, -1); (*outWritten) = outLen; - return(0); } @@ -348,14 +405,20 @@ xmlSecNssKWDes3BlockEncrypt(xmlSecTransformPtr transform, ctx = xmlSecNssKWDes3GetCtx(transform); xmlSecAssert2(ctx != NULL, -1); - xmlSecAssert2(xmlSecBufferGetData(&(ctx->keyBuffer)) != NULL, -1); - xmlSecAssert2(xmlSecBufferGetSize(&(ctx->keyBuffer)) >= XMLSEC_KW_DES3_KEY_LENGTH, -1); - ret = xmlSecNssKWDes3Encrypt(xmlSecBufferGetData(&(ctx->keyBuffer)), XMLSEC_KW_DES3_KEY_LENGTH, + /* create key if needed */ + ret = xmlSecNssKWDes3EnsureKey(ctx, CKA_ENCRYPT); + if(ret < 0) { + xmlSecInternalError("xmlSecNssKWDes3EnsureKey", NULL); + return(-1); + } + xmlSecAssert2(ctx->symKey != NULL, -1); + xmlSecAssert2(ctx->symKeyOp == CKA_ENCRYPT, -1); + + ret = xmlSecNssKWDes3Encrypt(ctx, CKA_ENCRYPT, iv, XMLSEC_KW_DES3_IV_LENGTH, in, inSize, - out, outSize, outWritten, - 1); /* encrypt */ + out, outSize, outWritten); if(ret < 0) { xmlSecInternalError("xmlSecNssKWDes3Encrypt", NULL); return(-1); @@ -385,14 +448,20 @@ xmlSecNssKWDes3BlockDecrypt(xmlSecTransformPtr transform, ctx = xmlSecNssKWDes3GetCtx(transform); xmlSecAssert2(ctx != NULL, -1); - xmlSecAssert2(xmlSecBufferGetData(&(ctx->keyBuffer)) != NULL, -1); - xmlSecAssert2(xmlSecBufferGetSize(&(ctx->keyBuffer)) >= XMLSEC_KW_DES3_KEY_LENGTH, -1); - ret = xmlSecNssKWDes3Encrypt(xmlSecBufferGetData(&(ctx->keyBuffer)), XMLSEC_KW_DES3_KEY_LENGTH, + /* create key if needed */ + ret = xmlSecNssKWDes3EnsureKey(ctx, CKA_DECRYPT); + if(ret < 0) { + xmlSecInternalError("xmlSecNssKWDes3EnsureKey", NULL); + return(-1); + } + xmlSecAssert2(ctx->symKey != NULL, -1); + xmlSecAssert2(ctx->symKeyOp == CKA_DECRYPT, -1); + + ret = xmlSecNssKWDes3Encrypt(ctx, CKA_DECRYPT, iv, XMLSEC_KW_DES3_IV_LENGTH, in, inSize, - out, outSize, outWritten, - 0); /* decrypt */ + out, outSize, outWritten); if(ret < 0) { xmlSecInternalError("xmlSecNssKWDes3Encrypt", NULL); return(-1); @@ -402,26 +471,73 @@ xmlSecNssKWDes3BlockDecrypt(xmlSecTransformPtr transform, } static int -xmlSecNssKWDes3Encrypt(const xmlSecByte *key, xmlSecSize keySize, - const xmlSecByte *iv, xmlSecSize ivSize, - const xmlSecByte *in, xmlSecSize inSize, - xmlSecByte *out, xmlSecSize outSize, - xmlSecSize * outWritten, - int enc) { - CK_MECHANISM_TYPE cipherMech; - PK11SlotInfo* slot = NULL; - PK11SymKey* symKey = NULL; +xmlSecNssKWDes3EnsureKey(xmlSecNssKWDes3CtxPtr ctx, CK_ATTRIBUTE_TYPE symKeyOp) { + xmlSecByte* keyData; + xmlSecSize keySize; + SECItem keyItem = { siBuffer, NULL, 0 }; + int res = -1; + + xmlSecAssert2(ctx != NULL, -1); + xmlSecAssert2(ctx->slot != NULL, -1); + + /* the cached symmetric key has the operation (CKA_ENCRYPT/CKA_DECRYPT) + baked in at import time, so it can only be reused for the same + operation */ + if((ctx->symKey != NULL) && (ctx->symKeyOp == symKeyOp)) { + return(0); + } + if(ctx->symKey != NULL) { + PK11_FreeSymKey(ctx->symKey); + ctx->symKey = NULL; + ctx->symKeyOp = 0; + } + + keyData = xmlSecBufferGetData(&(ctx->parentCtx.keyBuffer)); + keySize = xmlSecBufferGetSize(&(ctx->parentCtx.keyBuffer)); + xmlSecAssert2(keyData != NULL, -1); + xmlSecAssert2(keySize == XMLSEC_KW_DES3_KEY_LENGTH, -1); + + keyItem.data = keyData; + XMLSEC_SAFE_CAST_SIZE_TO_UINT(keySize, keyItem.len, goto done, NULL); + ctx->symKey = PK11_ImportSymKey(ctx->slot, XMLSEC_NSS_KW_DES3_CIPHER_MECH, PK11_OriginUnwrap, + symKeyOp, &keyItem, NULL); + if (ctx->symKey == NULL) { + xmlSecNssError("PK11_ImportSymKey", NULL); + goto done; + } + ctx->symKeyOp = symKeyOp; + + /* success */ + res = 0; + +done: + return(res); +} + +/* encrypt/decrypt a buffer; the slot and the symmetric key are cached in the + transform context and only the IV-dependent parameter and the PKCS#11 + context are created per call */ +static int +xmlSecNssKWDes3Encrypt( + xmlSecNssKWDes3CtxPtr ctx, + CK_ATTRIBUTE_TYPE op, + const xmlSecByte *iv, xmlSecSize ivSize, + const xmlSecByte *in, xmlSecSize inSize, + xmlSecByte *out, xmlSecSize outSize, xmlSecSize * outWritten +) { SECItem* param = NULL; PK11Context* pk11ctx = NULL; - SECItem keyItem = { siBuffer, NULL, 0 }; SECItem ivItem = { siBuffer, NULL, 0 }; + xmlSecByte* scratchIn; SECStatus status; int inLen, outLen, maxOutLen; int res = -1; - xmlSecAssert2(key != NULL, -1); - xmlSecAssert2(keySize == XMLSEC_KW_DES3_KEY_LENGTH, -1); + xmlSecAssert2(ctx != NULL, -1); + xmlSecAssert2(ctx->symKey != NULL, -1); xmlSecAssert2(iv != NULL, -1); + /* the wrappers above check >=, but always pass the exact IV length; + the helper enforces exact equality */ xmlSecAssert2(ivSize == XMLSEC_KW_DES3_IV_LENGTH, -1); xmlSecAssert2(in != NULL, -1); xmlSecAssert2(inSize > 0, -1); @@ -429,42 +545,37 @@ xmlSecNssKWDes3Encrypt(const xmlSecByte *key, xmlSecSize keySize, xmlSecAssert2(outSize >= inSize, -1); xmlSecAssert2(outWritten != NULL, -1); - cipherMech = CKM_DES3_CBC; - slot = PK11_GetBestSlot(cipherMech, NULL); - if (slot == NULL) { - xmlSecNssError("PK11_GetBestSlot", NULL); - goto done; - } - - keyItem.data = (unsigned char *)key; - XMLSEC_SAFE_CAST_SIZE_TO_UINT(keySize, keyItem.len, goto done, NULL); - symKey = PK11_ImportSymKey(slot, cipherMech, PK11_OriginUnwrap, - enc ? CKA_ENCRYPT : CKA_DECRYPT, &keyItem, NULL); - if (symKey == NULL) { - xmlSecNssError("PK11_ImportSymKey", NULL); - goto done; - } - ivItem.data = (unsigned char *)iv; XMLSEC_SAFE_CAST_SIZE_TO_UINT(ivSize, ivItem.len, goto done, NULL); - param = PK11_ParamFromIV(cipherMech, &ivItem); + param = PK11_ParamFromIV(XMLSEC_NSS_KW_DES3_CIPHER_MECH, &ivItem); if (param == NULL) { xmlSecNssError("PK11_ParamFromIV", NULL); goto done; } - pk11ctx = PK11_CreateContextBySymKey(cipherMech, - enc ? CKA_ENCRYPT : CKA_DECRYPT, - symKey, param); + pk11ctx = PK11_CreateContextBySymKey(XMLSEC_NSS_KW_DES3_CIPHER_MECH, op, ctx->symKey, param); if (pk11ctx == NULL) { xmlSecNssError("PK11_CreateContextBySymKey", NULL); goto done; } + /* NSS does not promise in-place (in == out) processing and the generic + KW driver (src/kw_helpers.c) passes the same buffer as both input and + output, so stage the input through the scratch buffer in the transform + context; the buffer is secure, so its contents are wiped on resize and + on context finalization */ + if(xmlSecBufferSetSize(&(ctx->scratchIn), inSize) < 0) { + xmlSecInternalError2("xmlSecBufferSetSize", NULL, "size=" XMLSEC_SIZE_FMT, inSize); + goto done; + } + scratchIn = xmlSecBufferGetData(&(ctx->scratchIn)); + xmlSecAssert2(scratchIn != NULL, -1); + memcpy(scratchIn, in, inSize); + XMLSEC_SAFE_CAST_SIZE_TO_INT(inSize, inLen, goto done, NULL); XMLSEC_SAFE_CAST_SIZE_TO_INT(outSize, maxOutLen, goto done, NULL); outLen = 0; - status = PK11_CipherOp(pk11ctx, out, &outLen, maxOutLen, (unsigned char *)in, inLen); + status = PK11_CipherOp(pk11ctx, out, &outLen, maxOutLen, scratchIn, inLen); if (status != SECSuccess) { xmlSecNssError("PK11_CipherOp", NULL); goto done; @@ -481,12 +592,6 @@ xmlSecNssKWDes3Encrypt(const xmlSecByte *key, xmlSecSize keySize, res = 0; done: - if (slot) { - PK11_FreeSlot(slot); - } - if (symKey) { - PK11_FreeSymKey(symKey); - } if (param) { SECITEM_FreeItem(param, PR_TRUE); } diff --git a/src/nss/kw_rfc_3394.c b/src/nss/kw_rfc_3394.c index 6b1247121..f9247a4fa 100644 --- a/src/nss/kw_rfc_3394.c +++ b/src/nss/kw_rfc_3394.c @@ -78,17 +78,18 @@ typedef struct _xmlSecNssKWRfc3394Ctx xmlSecNssKWRfc3394Ctx, struct _xmlSecNssKWRfc3394Ctx { xmlSecTransformKWRfc3394Ctx parentCtx; PK11SymKey* symKey; + CK_ATTRIBUTE_TYPE symKeyOp; CK_MECHANISM_TYPE cipherMech; + SECItem* secParam; + xmlSecByte scratchIn[XMLSEC_KW_RFC3394_BLOCK_SIZE]; }; static int xmlSecNssKWRfc3394EnsureKey (xmlSecNssKWRfc3394CtxPtr ctx, xmlSecKeyDataId keyId, - int enc); -static int xmlSecNssKWRfc3394CipherOp (PK11SymKey *symKey, - CK_MECHANISM_TYPE cipherMech, + CK_ATTRIBUTE_TYPE symKeyOp); +static int xmlSecNssKWRfc3394CipherOp (xmlSecNssKWRfc3394CtxPtr ctx, const xmlSecByte *in, - xmlSecByte *out, - int enc); + xmlSecByte *out); /****************************************************************************** @@ -212,6 +213,18 @@ xmlSecNssKWRfc3394Initialize(xmlSecTransformPtr transform) { } xmlSecAssert2(keyExpectedSize > 0, -1); + + /* the cipher mechanism is fixed for the lifetime of the transform, so + the mechanism parameter is created once and reused for every block + operation; PK11_CreateContextBySymKey copies the parameter into each + PKCS#11 context, so it is not modified or freed by NSS */ + ctx->secParam = PK11_ParamFromIV(ctx->cipherMech, NULL); + if(ctx->secParam == NULL) { + xmlSecNssError("PK11_ParamFromIV", NULL); + xmlSecNssKWRfc3394Finalize(transform); + return(-1); + } + ret = xmlSecTransformKWRfc3394Initialize(transform, &(ctx->parentCtx), &xmlSecNssKWRfc3394Klass, keyId, keyExpectedSize); @@ -239,8 +252,14 @@ xmlSecNssKWRfc3394Finalize(xmlSecTransformPtr transform) { if(ctx->symKey != NULL) { PK11_FreeSymKey(ctx->symKey); } + if(ctx->secParam != NULL) { + SECITEM_FreeItem(ctx->secParam, PR_TRUE); + ctx->secParam = NULL; + } xmlSecTransformKWRfc3394Finalize(transform, &(ctx->parentCtx)); + xmlSecMemCleanse(ctx->scratchIn, sizeof(ctx->scratchIn)); + memset(ctx, 0, sizeof(xmlSecNssKWRfc3394Ctx)); } @@ -285,6 +304,7 @@ xmlSecNssKWRfc3394SetKey(xmlSecTransformPtr transform, xmlSecKeyPtr key) { if(ctx->symKey != NULL) { PK11_FreeSymKey(ctx->symKey); ctx->symKey = NULL; + ctx->symKeyOp = 0; } return(0); @@ -331,15 +351,16 @@ xmlSecNssKWRfc3394BlockEncrypt(xmlSecTransformPtr transform, const xmlSecByte * xmlSecAssert2(ctx->parentCtx.keyId != NULL, -1); /* create key if needed */ - ret = xmlSecNssKWRfc3394EnsureKey(ctx, ctx->parentCtx.keyId, 1); /* encrypt */ + ret = xmlSecNssKWRfc3394EnsureKey(ctx, ctx->parentCtx.keyId, CKA_ENCRYPT); if(ret < 0) { xmlSecInternalError("xmlSecNssKWRfc3394EnsureKey", NULL); return(-1); } xmlSecAssert2(ctx->symKey != NULL, -1); + xmlSecAssert2(ctx->symKeyOp == CKA_ENCRYPT, -1); /* one block */ - ret = xmlSecNssKWRfc3394CipherOp(ctx->symKey, ctx->cipherMech, in, out, 1); /* encrypt */ + ret = xmlSecNssKWRfc3394CipherOp(ctx, in, out); if(ret < 0) { xmlSecInternalError("xmlSecNssKWRfc3394CipherOp", NULL); return(-1); @@ -368,15 +389,16 @@ xmlSecNssKWRfc3394BlockDecrypt(xmlSecTransformPtr transform, const xmlSecByte * xmlSecAssert2(ctx->parentCtx.keyId != NULL, -1); /* create key if needed */ - ret = xmlSecNssKWRfc3394EnsureKey(ctx, ctx->parentCtx.keyId, 0); /* decrypt */ + ret = xmlSecNssKWRfc3394EnsureKey(ctx, ctx->parentCtx.keyId, CKA_DECRYPT); if(ret < 0) { xmlSecInternalError("xmlSecNssKWRfc3394EnsureKey", NULL); return(-1); } xmlSecAssert2(ctx->symKey != NULL, -1); + xmlSecAssert2(ctx->symKeyOp == CKA_DECRYPT, -1); /* one block */ - ret = xmlSecNssKWRfc3394CipherOp(ctx->symKey, ctx->cipherMech, in, out, 0); /* decrypt */ + ret = xmlSecNssKWRfc3394CipherOp(ctx, in, out); if(ret < 0) { xmlSecInternalError("xmlSecNssKWRfc3394CipherOp", NULL); return(-1); @@ -386,7 +408,7 @@ xmlSecNssKWRfc3394BlockDecrypt(xmlSecTransformPtr transform, const xmlSecByte * } static int -xmlSecNssKWRfc3394EnsureKey(xmlSecNssKWRfc3394CtxPtr ctx, xmlSecKeyDataId keyId, int enc) { +xmlSecNssKWRfc3394EnsureKey(xmlSecNssKWRfc3394CtxPtr ctx, xmlSecKeyDataId keyId, CK_ATTRIBUTE_TYPE symKeyOp) { xmlSecByte* keyData; xmlSecSize keySize; PK11SlotInfo* slot = NULL; @@ -397,9 +419,18 @@ xmlSecNssKWRfc3394EnsureKey(xmlSecNssKWRfc3394CtxPtr ctx, xmlSecKeyDataId keyId, xmlSecAssert2(keyId != NULL, -1); xmlSecAssert2(ctx->parentCtx.keyId != NULL, -1); xmlSecAssert2(keyId == ctx->parentCtx.keyId, -1); - if(ctx->symKey != NULL) { + + /* the cached symmetric key has the operation (CKA_ENCRYPT/CKA_DECRYPT) + * baked in at import time, so it can only be reused for the same + * operation */ + if((ctx->symKey != NULL) && (ctx->symKeyOp == symKeyOp)) { return(0); } + if(ctx->symKey != NULL) { + PK11_FreeSymKey(ctx->symKey); + ctx->symKey = NULL; + ctx->symKeyOp = 0; + } keyData = xmlSecBufferGetData(&(ctx->parentCtx.keyBuffer)); keySize = xmlSecBufferGetSize(&(ctx->parentCtx.keyBuffer)); @@ -414,14 +445,14 @@ xmlSecNssKWRfc3394EnsureKey(xmlSecNssKWRfc3394CtxPtr ctx, xmlSecKeyDataId keyId, } keyItem.data = keyData; - XMLSEC_SAFE_CAST_SIZE_TO_UINT(keySize, keyItem.len, goto done, -1); + XMLSEC_SAFE_CAST_SIZE_TO_UINT(keySize, keyItem.len, goto done, NULL); - ctx->symKey = PK11_ImportSymKey(slot, ctx->cipherMech, PK11_OriginUnwrap, - enc ? CKA_ENCRYPT : CKA_DECRYPT, &keyItem, NULL); + ctx->symKey = PK11_ImportSymKey(slot, ctx->cipherMech, PK11_OriginUnwrap, symKeyOp, &keyItem, NULL); if (ctx->symKey == NULL) { xmlSecNssError("PK11_ImportSymKey", NULL); goto done; } + ctx->symKeyOp = symKeyOp; /* success */ res = 0; @@ -433,37 +464,37 @@ xmlSecNssKWRfc3394EnsureKey(xmlSecNssKWRfc3394CtxPtr ctx, xmlSecKeyDataId keyId, return(res); } -/* encrypt/decrypt a block (XMLSEC_KW_RFC3394_BLOCK_SIZE), in and out can overlap */ +/* encrypt/decrypt a block (XMLSEC_KW_RFC3394_BLOCK_SIZE); in and out may be + the same buffer (in-place processing is staged through the scratch buffer + in the transform context) */ static int -xmlSecNssKWRfc3394CipherOp(PK11SymKey *symKey, CK_MECHANISM_TYPE cipherMech, const xmlSecByte *in, xmlSecByte *out, int enc) { - SECItem* secParam = NULL; +xmlSecNssKWRfc3394CipherOp(xmlSecNssKWRfc3394CtxPtr ctx, const xmlSecByte *in, xmlSecByte *out) { PK11Context* ctxt = NULL; SECStatus rv; int outlen; int ret = -1; - xmlSecAssert2(symKey != NULL, -1); + xmlSecAssert2(ctx != NULL, -1); + xmlSecAssert2(ctx->symKey != NULL, -1); + xmlSecAssert2(ctx->secParam != NULL, -1); xmlSecAssert2(in != NULL, -1); xmlSecAssert2(out != NULL, -1); - secParam = PK11_ParamFromIV(cipherMech, NULL); - if (secParam == NULL) { - xmlSecNssError("PK11_ParamFromIV", NULL); - goto done; - } - - ctxt = PK11_CreateContextBySymKey(cipherMech, enc ? CKA_ENCRYPT : CKA_DECRYPT, - symKey, secParam); + /* note: the PKCS#11 context is created per block; RFC 3394 performs many + block operations per key and we need new context each time */ + ctxt = PK11_CreateContextBySymKey(ctx->cipherMech, ctx->symKeyOp, ctx->symKey, ctx->secParam); if (ctxt == NULL) { xmlSecNssError("PK11_CreateContextBySymKey", NULL); goto done; } + /* NSS does not promise in-place (in == out) processing, so stage the + block through the scratch buffer in the transform context; it retains + the last block until xmlSecNssKWRfc3394Finalize cleanses it */ + memcpy(ctx->scratchIn, in, sizeof(ctx->scratchIn)); outlen = 0; - rv = PK11_CipherOp(ctxt, out, &outlen, - XMLSEC_KW_RFC3394_BLOCK_SIZE, (unsigned char *)in, - XMLSEC_KW_RFC3394_BLOCK_SIZE); - if ((rv != SECSuccess) || (outlen != XMLSEC_KW_RFC3394_BLOCK_SIZE)) { + rv = PK11_CipherOp(ctxt, out, &outlen, sizeof(ctx->scratchIn), ctx->scratchIn, sizeof(ctx->scratchIn)); + if ((rv != SECSuccess) || (outlen != sizeof(ctx->scratchIn))) { xmlSecNssError("PK11_CipherOp", NULL); goto done; } @@ -478,9 +509,6 @@ xmlSecNssKWRfc3394CipherOp(PK11SymKey *symKey, CK_MECHANISM_TYPE cipherMech, con ret = 0; done: - if (secParam) { - SECITEM_FreeItem(secParam, PR_TRUE); - } if (ctxt) { PK11_DestroyContext(ctxt, PR_TRUE); } diff --git a/src/nss/pkikeys.c b/src/nss/pkikeys.c index 7528e3381..c29dc68a0 100644 --- a/src/nss/pkikeys.c +++ b/src/nss/pkikeys.c @@ -816,7 +816,6 @@ static xmlSecKeyDataPtr xmlSecNssKeyDataDsaRead(xmlSecKeyDataId id, xmlSecKeyValueDsaPtr dsaValue) { xmlSecKeyDataPtr data = NULL; xmlSecKeyDataPtr res = NULL; - PK11SlotInfo *slot = NULL; SECKEYPublicKey *pubkey=NULL; PRArenaPool *arena = NULL; int ret; @@ -824,12 +823,6 @@ xmlSecNssKeyDataDsaRead(xmlSecKeyDataId id, xmlSecKeyValueDsaPtr dsaValue) { xmlSecAssert2(id == xmlSecNssKeyDataDsaId, NULL); xmlSecAssert2(dsaValue != NULL, NULL); - slot = PK11_GetBestSlot(CKM_DSA, NULL); - if(slot == NULL) { - xmlSecNssError("PK11_GetBestSlot", xmlSecKeyDataKlassGetName(id)); - goto done; - } - arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE); if(arena == NULL) { xmlSecNssError("PORT_NewArena", xmlSecKeyDataKlassGetName(id)); @@ -906,9 +899,6 @@ xmlSecNssKeyDataDsaRead(xmlSecKeyDataId id, xmlSecKeyValueDsaPtr dsaValue) { data = NULL; done: - if (slot != NULL) { - PK11_FreeSlot(slot); - } if (arena != NULL) { PORT_FreeArena(arena, PR_FALSE); } @@ -1073,7 +1063,6 @@ static xmlSecKeyDataPtr xmlSecNssKeyDataRsaRead(xmlSecKeyDataId id, xmlSecKeyValueRsaPtr rsaValue) { xmlSecKeyDataPtr data = NULL; xmlSecKeyDataPtr res = NULL; - PK11SlotInfo *slot = NULL; SECKEYPublicKey *pubkey=NULL; PRArenaPool *arena = NULL; int ret; @@ -1081,20 +1070,13 @@ xmlSecNssKeyDataRsaRead(xmlSecKeyDataId id, xmlSecKeyValueRsaPtr rsaValue) { xmlSecAssert2(id == xmlSecNssKeyDataRsaId, NULL); xmlSecAssert2(rsaValue != NULL, NULL); - slot = PK11_GetBestSlot(CKM_RSA_PKCS, NULL); - if(slot == NULL) { - xmlSecNssError("PK11_GetBestSlot", xmlSecKeyDataKlassGetName(id)); - goto done; - } - arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE); if(arena == NULL) { xmlSecNssError("PORT_NewArena", xmlSecKeyDataKlassGetName(id)); goto done; } - pubkey = (SECKEYPublicKey *)PORT_ArenaZAlloc(arena, - sizeof(SECKEYPublicKey)); + pubkey = (SECKEYPublicKey *)PORT_ArenaZAlloc(arena, sizeof(SECKEYPublicKey)); if(pubkey == NULL) { xmlSecNssError("PORT_ArenaZAlloc", xmlSecKeyDataKlassGetName(id)); goto done; @@ -1138,9 +1120,6 @@ xmlSecNssKeyDataRsaRead(xmlSecKeyDataId id, xmlSecKeyValueRsaPtr rsaValue) { data = NULL; done: - if (slot != NULL) { - PK11_FreeSlot(slot); - } if(arena != NULL) { PORT_FreeArena(arena, PR_FALSE); } @@ -1354,7 +1333,6 @@ static xmlSecKeyDataPtr xmlSecNssKeyDataEcRead(xmlSecKeyDataId id, xmlSecKeyValueEcPtr ecValue) { xmlSecKeyDataPtr data = NULL; xmlSecKeyDataPtr res = NULL; - PK11SlotInfo *slot = NULL; SECKEYPublicKey *pubkey=NULL; PRArenaPool *arena = NULL; SECItem ecparams = { siBuffer, NULL, 0 }; @@ -1368,12 +1346,6 @@ xmlSecNssKeyDataEcRead(xmlSecKeyDataId id, xmlSecKeyValueEcPtr ecValue) { xmlSecAssert2(ecValue->curve != NULL, NULL); /* prepare and create public key */ - slot = PK11_GetBestSlot(CKM_ECDSA, NULL); - if(slot == NULL) { - xmlSecNssError("PK11_GetBestSlot", xmlSecKeyDataKlassGetName(id)); - goto done; - } - arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE); if(arena == NULL) { xmlSecNssError("PORT_NewArena", xmlSecKeyDataKlassGetName(id)); @@ -1448,9 +1420,6 @@ xmlSecNssKeyDataEcRead(xmlSecKeyDataId id, xmlSecKeyValueEcPtr ecValue) { data = NULL; done: - if (slot != NULL) { - PK11_FreeSlot(slot); - } if (arena != NULL) { PORT_FreeArena(arena, PR_FALSE); } diff --git a/src/nss/signatures.c b/src/nss/signatures.c index 67decae30..c7da83a6b 100644 --- a/src/nss/signatures.c +++ b/src/nss/signatures.c @@ -388,7 +388,11 @@ xmlSecNssSignatureInitialize(xmlSecTransformPtr transform) { return(-1); } - /* EdDSA needs a buffer for message data */ + /* EdDSA needs a buffer for message data: NSS provides no incremental + * sign/verify context for Ed25519/Ed448, so the entire document is + * accumulated here and signed/verified in one PK11_Sign/PK11_Verify call; + * for large documents this doubles peak memory compared to the streaming + * SGN/VFY contexts used for the other algorithms */ if (ctx->isEdDSA) { ret = xmlSecBufferInitialize(&(ctx->eddsaData), 0); if (ret < 0) { @@ -523,6 +527,10 @@ xmlSecNssSignatureCreatePssAlgId(xmlSecNssSignatureCtxPtr ctx) { xmlSecAssert2(ctx != NULL, -1); xmlSecAssert2(ctx->arena != NULL, -1); + /* note: this is re-run on every SetKey and each run allocates the PSS + * params and algorithm ID in the context arena, which is only freed in + * Finalize; SetKey is called once per transform in practice, so the + * accumulation is negligible */ params = xmlSecNssSignatureCreatePssParams(ctx); if (params == NULL) { xmlSecInternalError("xmlSecNssSignatureCreatePssParams", NULL);