Fenrir fixes#10632
Conversation
|
a85f497 to
42999fb
Compare
| word32 key[3][DES_KS_SIZE]; | ||
| word32 reg[DES_BLOCK_SIZE / sizeof(word32)]; /* for CBC mode */ | ||
| word32 tmp[DES_BLOCK_SIZE / sizeof(word32)]; /* same */ | ||
| byte keySet; /* set to 1 once a key has been configured */ |
There was a problem hiding this comment.
Please use WC_BITFIELD and :1 and put at bottom of struct. Best compat.
| typedef struct Arc4 { | ||
| byte x; | ||
| byte y; | ||
| byte keySet; /* set to 1 once a key has been configured */ |
There was a problem hiding this comment.
Same. Use WC_BITFIELD and :1 and put at bottom. Thank you
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 4 total — 1 posted, 3 skipped
1 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Info] ML-DSA small-mem key-gen zeroizes the public expansion buffer too —
wolfcrypt/src/wc_mldsa.c:8163-8168
Skipped findings
- [Medium]
Des3 struct: keySet inserted mid-struct (ABI / FIPS boundary concern) - [Medium]
supported_groups restriction not exercised end-to-end - [Low]
Arc4 struct: keySet inserted before state[] instead of at end
Review generated by Skoll
Fixes F-5378
Fixes F-5379
Fixes F-4441 and F-4442
Fixes F-4443
Fixes F-5437 and F-5438
Fixes F-4891
|
Jenkins retest this please |
|
The Arduino failure is unrelated and fixed in #10644. |
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 2 total — 2 posted, 0 skipped
2 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] Supported-curve parse test lacks server/TLS1.3 guard; fails in --disable-server non-TLS1.3 builds —
tests/api/test_tls_ext.c:1058-1100 - [Low] peerNoUncompPF reset skipped when ClientHello has no extensions block —
src/internal.c:38896-38907
Review generated by Skoll
| * relevant code path (TLSX_SupportedCurve_Parse) is selected by the | ||
| * message type passed to TLSX_Parse (client_hello => isRequest), not by | ||
| * the side of the WOLFSSL object. A client-side WOLFSSL is used purely as | ||
| * the parse vehicle because creating a server-side WOLFSSL would require a |
There was a problem hiding this comment.
🟠 [Medium] Supported-curve parse test lacks server/TLS1.3 guard; fails in --disable-server non-TLS1.3 builds
The new test calls TLSX_Parse(..., client_hello, ...) and asserts the empty-list path returns BUFFER_ERROR and the unsupported-only path records a TLSX_SUPPORTED_GROUPS node. Both depend on TLSX_SupportedCurve_Parse actually running. In src/tls.c the dispatch macro EC_PARSE is the real function only when !defined(NO_WOLFSSL_SERVER) || (defined(WOLFSSL_TLS13) && !defined(WOLFSSL_NO_SERVER_GROUPS_EXT)); otherwise #define EC_PARSE(a,b,c,d,e) 0. The test's #if gate is !defined(NO_WOLFSSL_CLIENT) && !defined(NO_TLS) && defined(HAVE_SUPPORTED_CURVES) && !defined(WOLFSSL_NO_TLS12) and does NOT require server or TLS1.3. In a --disable-server build without TLS 1.3, EC_PARSE becomes a no-op returning 0, so TLSX_Parse(emptyList) returns 0 (not BUFFER_ERROR) and no node is added — both ExpectIntEQ(..., BUFFER_ERROR) and ExpectNotNull(TLSX_Find(...)) then fail at runtime. The sibling test test_TLSX_PointFormat_uncompressed_required correctly guards on !defined(NO_WOLFSSL_SERVER) (its comment even explains PF_PARSE is a no-op without server), so the gating is inconsistent.
Fix: Add a guard so the TLS1.2 portion of the test only runs when TLSX_SupportedCurve_Parse (EC_PARSE) is the real function — mirror the EC_PARSE condition (!defined(NO_WOLFSSL_SERVER) || (defined(WOLFSSL_TLS13) && !defined(WOLFSSL_NO_SERVER_GROUPS_EXT))), matching the approach used by the point-format sibling test.
| @@ -38894,6 +38894,13 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl) | |||
| } | |||
|
|
|||
| #ifdef HAVE_TLS_EXTENSIONS | |||
There was a problem hiding this comment.
🔵 [Low] peerNoUncompPF reset skipped when ClientHello has no extensions block
The defensive reset ssl->options.peerNoUncompPF = 0; is placed inside the if ((i - begin) < helloSz) extensions-present block (and inside the TLSX_SupportExtensions(ssl) block), immediately before TLSX_Parse. If a subsequent ClientHello on the same WOLFSSL object carries no extensions block at all, the reset is not executed and a stale peerNoUncompPF=1 from an earlier handshake persists; if an ECC suite is then negotiated, the new RFC 8422 check would fire a spurious illegal_parameter abort. In practice this is not reachable for secure renegotiation (RFC 5746 requires the renegotiation_info extension, so the extensions block is always present and the reset always runs), so the stated motivation in the comment is satisfied. It is only theoretically reachable with unsafe legacy renegotiation enabled and a no-extensions ClientHello that still negotiates ECC. Flagged for robustness only.
Fix: Optionally move the reset earlier to guarantee it runs for every ClientHello regardless of whether an extensions block is present. Low priority since secure renegotiation always carries an extensions block.
Summary
This PR addresses a set of Fenrir-reported issues across wolfCrypt and the TLS layer. Each fix is accompanied by a regression test where applicable.
Fixes
Cipher key-state validation
wc_Arc4Process()now refuses to run unless a key was configured viawc_Arc4SetKey(), preventing ARC4 from silently copying plaintext to ciphertext with an all-zero keystream. preventing DES3 operations from running with uninitialized or zeroed key material.Sensitive-memory zeroization
eccToPKCS8()now zeroizes the temporary buffer holding the plaintext ECC private key on every exit path via a consolidatedgoto exitcleanup.ForceZero()the private vectors (s1, s2, t, y, …) before freeing, while skipping the public matrix A that need not be cleared.Post-quantum / hardware ports
wolfSSL_liboqsGetRandomData()to advance the output buffer between chunks and guard againstsize_t→word32truncation, avoiding overwritten output and a potential infinite loop on large requests.NitroxCheckRequests()and to pass the output-length argument with the expectedUint16*type in the RSA decrypt/verify paths, using a dedicatedUint16temporary so thentohs()conversion stays correct on big-endian hosts.TLS supported_groups / ec_point_formats handling
supported_groupsparsing so an empty named-group list is rejected as malformed and an all-unsupported-groups list still records a (now empty) restriction instead of being treated as absent.illegal_parameteralert when the client'sec_point_formatsextension omits the uncompressed format and an ECC cipher suite is actually negotiated; the check runs after suite selection (ssl->specs.kea) so a client that negotiates a non-ECC suite is not rejected.Testing
Added regression tests for ARC4, DES3, supported_groups parsing, and the
ec_point_formatsenforcement — covering both the abort path when an ECC suite is negotiated (driven through a full handshake via memio) and the complementary case where a non-ECC (DHE_RSA) suite is negotiated and the handshake proceeds.