Conversation
… buffer EchCalcAcceptance() hashes the message from its handshake header and reads the confirmation bytes at an offset into it, so its input must start at that header. DoTls13ServerHello() instead passed the buffer it had been handed together with an index measured from that buffer's base. The two coincide only when the ServerHello is the first, unfragmented handshake message of its record; otherwise the trailing hash length it computes is wrong. Derive the message start as input + args->begin - headerSz, the adjustment HashInput() already makes, and convert both the server-random offset and the HelloRetryRequest confBuf offset to be relative to it. That holds on every path reaching here: a record buffer, a reassembled TLS message, and the DTLS 1.3 reassembly path, which writes the handshake header immediately before the assembled message. The adjusted value goes into a local rather than back into args->acceptOffset, so re-entering TLS_ASYNC_FINALIZE gives the same result. EchCalcAcceptance() additionally rejects offsets that do not lie inside the message, so a future caller mistake cannot produce a negative hash length again. The server-side caller, EchWriteAcceptance(), already passes header-relative values and is unaffected. Behaviour change: a fragmented ServerHello or HelloRetryRequest no longer spuriously rejects ECH. A ServerHello coalesced behind a HelloRetryRequest that accepted ECH now fails with INVALID_PARAMETER, which is the RFC 9849 section 6.1.5 outcome. Tests: test_tls13_ech_hrr_coalesced_server_hello and test_tls13_ech_fragmented_server_hello, both failing before the fix and both needing HAVE_ECH, HAVE_SNI and the manual memio dependencies.
In padded decrypt mode the function held two kinds of pending state at once: the most recent decrypted block in ctx->lastBlock for the padding check in Final, and a partial input block in ctx->buf. Nothing kept the two mutually exclusive, so a call that completed the partial block and carried further full blocks could write more than the inl + block_size that EVP_DecryptUpdate promises, which is what our own doxygen example for the function relies on. The library's own callers decrypt in single Update calls and are unaffected. A stored block is now output as soon as more input arrives, and the buffer completion path stores one only when the call ends on a block boundary, which is what OpenSSL does. Every call therefore stays within inl + block_size, the flush in the full blocks path becomes dead and is removed, and Final still sees either lastUsed or a non-zero bufUsed for truncated input. The same path also stored a block completed from ctx->buf when padding is disabled, where Final only checks that nothing is buffered and never looks at lastBlock, so that plaintext was silently dropped. The store is skipped with WOLFSSL_EVP_CIPH_NO_PADDING set, and Final now returns a block that was stored before padding was turned off. Behaviour change: which Update call returns a given block moves one call earlier for input that is not fed in block multiples, matching OpenSSL, so the three chunked decrypt checks in wolfcrypt/test/test.c are updated. Two of the three had no body on the if and had never checked anything. out must be at least inl + block size and must not overlap in; the doxygen documented neither and sized its example buffer at exactly inl. Strict in-place operation is not supported and never was. Tests: test_evp_cipher_update_chunked_bound, which decrypts a 64 byte AES-128-CBC ciphertext in every three way split against a buffer of exactly inl + block size followed by a canary, and test_evp_cipher_update_no_padding_buffered.
In the WC_PKCS7_AUTHENV_ATRBEND state the streaming decoder guaranteed only MAX_LENGTH_SZ + ASN_TAG_SZ bytes before copying the authTag out of pkiMsg. Its re-buffer guard compared the tag size plus the OCTET STRING header against pkiMsgSz, the size of the whole buffer, instead of the bytes left after the header, so for a real message it never fired. Both explicit BUFFER_E bounds checks in front of the copy were compiled only under NO_PKCS7_STREAM, and no configure option defines that macro. Measure the re-buffer guard against pkiMsgSz - localIdx and, after a successful wc_PKCS7_AddDataToStream(), restore localIdx from the header offset and refresh pkiMsgSz, the idiom wc_PKCS7_DecodeEnvelopedData() already uses for encryptedContent. Make the pre-copy BUFFER_E check unconditional so the copy cannot exceed the available input whatever the streaming logic decides, and drop the weaker NO_PKCS7_STREAM-only check it subsumes. Two adjacent defects in the same state go with it. The state prologue never restored encOID, unlike WC_PKCS7_AUTHENV_5 and _6, so the AES-GCM and AES-CCM tag size gates below it compared against 0 on every re-entry, and the bound above makes that re-entry the normal streaming path rather than a rare corner. The re-buffer call also returned directly, so an error from it skipped the shared handler that releases the AAD and resets the stream; it now breaks into that handler, which already passes WC_PKCS7_WANT_READ_E through untouched. Behaviour change: input ending inside the tag returns WC_PKCS7_WANT_READ_E when streaming and BUFFER_E under NO_PKCS7_STREAM. A chunked delivery whose read boundary falls inside the tag now completes once the rest arrives; previously it failed with AES_GCM_AUTH_E. test_wc_PKCS7_DecodeAuthEnvelopedData_truncated was compiled only under NO_PKCS7_STREAM, so the default streaming decoder was never exercised, and it decoded from an oversized stack buffer. It now runs in streaming builds, decodes from exact sized heap copies, and sweeps truncations into the tag as well as the existing cut inside encryptedContent.
DoServerHello() checked only that the ServerHello's cipher suite appeared in the client's own list, never that it is usable at the negotiated version. VerifyServerSuite() has exactly that check on the server, and the client had no equivalent. A resuming ClientHello is pinned to the cached session's version by wolfSSL_set_session() but still carries the full configured suite list, so a downgrade-capable client resuming a TLS 1.2 session by session ID offers TLS 1.3-only suites inside a TLS 1.2 hello. With HAVE_NULL_CIPHER built, TLS_SHA384_SHA384 is in the default client list without any application opt-in. Selecting such a suite gives SetCipherSpecs() sizes whose TLS 1.2 key block does not fit key_dig, on a resumption path that derives keys before the peer is authenticated. Reject the suite in SetCipherSpecs(), where the specs become authoritative: specs.kea == any_kea marks exactly the suites VerifyServerSuite() rejects, so one check covers both DoServerHello() paths as well as the DTLS key-import API and the sniffer. Every other caller already runs with ssl->version at TLS 1.3 when a TLS 1.3 suite is legitimate. DeriveTlsKeys() additionally returns BUFFER_E for a key block larger than its buffer. wc_PRF_TLS() keeps no MAX_PRF_DIG bound because wolfSSL_export_keying_material() and the EAP key export legitimately ask for more. Stop offering the suites in the first place: SendClientHello() is reached only below TLS 1.3, since SendTls13ClientHello() takes over above it, so filter TLS 1.3-only suites out of the cipher_suites it writes and fail with SUITES_ERROR if nothing is left. VerifyServerSuite() already carried IsTls13CipherSuite(), a second copy of the same predicate; the two are merged into IsTls13OnlySuite(), keeping the HAVE_NULL_CIPHER and SM4 guards, so a future TLS 1.3 suite has one list to be added to. SetCipherSpecs() zeroes ssl->specs before returning UNSUPPORTED_SUITE. GetCipherSpec() has already written the rejected suite's key, hash and IV sizes there, and ssl->options.tls, ssl->hmac and the DTLS fields are not updated on that path, so a caller that tolerated the failure would run TLS 1.2 with a TLS 1.3 key block. Behaviour change: a TLS 1.2 or DTLS 1.2 peer that picks a TLS 1.3 suite now fails with UNSUPPORTED_SUITE at ServerHello rather than BAD_KEA_TYPE_E or a Finished failure. Tests: test_tls12_server_hello_tls13_suite rewrites the suite of a resuming ServerHello and expects UNSUPPORTED_SUITE; test_tls12_client_hello_no_tls13_suites checks that the resuming ClientHello drops the TLS 1.3 suites the non-resuming one offers, and still resumes.
EccSpecifiedECDomainDecode() took the curve size straight from the DER length of the prime INTEGER with no upper bound, and wc_EccPublicKeyDecode() then installed that curve on the caller's key. Every fixed buffer in ecc.c is dimensioned for MAX_ECC_BYTES, so a curve larger than that left the export and encode paths padding a coordinate outside its buffer, with both the offset and the bytes written taken from the key. One of those paths reaches it through the library's own allocation, with no output buffer from the caller at all. Reject the parameters where the size is taken from input, which is where the static ecc_sets[] table establishes the same invariant for every named curve. wc_ecc_set_custom_curve() applies the bound too, so a curve installed through the API cannot exceed the fixed buffers either, and the WOLFSSL_ECC_CURVE_STATIC branch bounds the a/b/order lengths, which are hex-encoded into MAX_ECC_STRING arrays from their own unbounded DER lengths. wc_ecc_set_custom_curve() rejects a non-positive size as well, matching the decoder: size 0 let a one byte 0x04 satisfy the importer's length check and yield a degenerate public key. Both reject paths log. Behaviour change: explicit-parameter EC keys and certificates whose prime is longer than MAX_ECC_BYTES (66 bytes without SAKKE, 128 with it) now fail to decode with ASN_PARSE_E instead of being accepted with a dp the rest of the library cannot handle safely. Every standard prime curve up to P-521 still decodes. Known limitation in WOLFSSL_ECC_CURVE_STATIC builds: the order bound is MAX_ECC_BYTES, but the Koblitz curves carry an order one byte wider than their prime, so in a build whose MAX_ECC_BYTES equals that prime size an explicit parameter key for one of them is now refused. MAX_ECC_STRING cannot hold its hex form, which is what the unbounded DataToHexString() overflowed before, so this fails closed where it used to corrupt the struct. Test: test_wc_EccPublicKeyDecode_explicit_curve_size, which builds the SubjectPublicKeyInfo at run time so the control prime tracks MAX_ECC_BYTES in whatever configuration it is built for.
|
wolfSSL-Fenrir-bot
previously requested changes
Sep 17, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11484
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-src, wolfssl-bugs
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
_ecc_import_x963_ex2() took the ordinate width from the input length and never compared it with the curve, so a SubjectPublicKeyInfo naming one curve but carrying wider ordinates became a key on that curve holding oversized ordinates. wc_ecc_set_curve() bounds the size only by ECC_MAXSIZE and, when given a curve id, selects the curve by id without comparing sizes. The untrusted-import validation did not catch it in SP builds: sp_ecc_is_point_256/384/521() copy only the low limbs out of the mp_int, so the truncated point is on the curve and the import succeeds, where the generic wc_ecc_is_point() returns ECC_OUT_OF_RANGE_E. Every caller of the importer is affected, including wc_EccPublicKeyDecode(), the TLS 1.2 and TLS 1.3 key exchange, PKCS#7 KARI, HPKE and the compat d2i_* functions; none of them compares the wire length with the curve size first. wolfSSL_EC_POINT_point2hex() then placed the x-ordinate at an offset derived from the curve size minus the ordinate size, which goes negative once an ordinate is wider than the curve. The compressed form and the y ordinate degenerate the same way. _ecc_import_x963_ex2() now requires the ordinate size to equal key->dp->size, returning ECC_BAD_ARG_E otherwise. This is the check wc_ecc_import_point_der_ex() already has, placed in the shared importer so it covers every caller and every math back end. wolfSSL_EC_POINT_point2hex() refuses ordinates larger than the curve before computing any offset, mirroring wc_ecc_export_point_der(), whose guard wolfSSL_EC_POINT_point2oct() already inherits, and picks up the NULL ordinate check that its new bound would otherwise be the first to dereference past. sp_ecc_is_point_256/384/521/1024() in all eight SP back ends gain the mp_count_bits() and modulus comparison that the neighbouring sp_ecc_check_key_*() already carries, so SP builds no longer accept a same-length non-canonical encoding that every other back end rejects. Both functions also gain the mp_isneg() test the generic implementation has: sp_<n>_from_mp() ignores the sign, so a negative ordinate was read as its magnitude. sp_ecc_check_key_*() lacked that check before this change as well. The sp_*.c files are generator output; the change was made in wolfSSL/scripts sp/ecc.rb and the files regenerated, and the result is byte for byte what the modified generator produces. Behaviour change: X9.63 encodings shorter than the curve size are rejected as well. They are non-canonical under SEC 1 section 2.3.4 and RFC 5480 and OpenSSL rejects them too. With ECC_CURVE_DEF the size-based curve lookup still resolves exact-size encodings as before. Tests: test_wc_ecc_import_x963_oversized and test_wc_ecc_import_x963_non_canonical cover the importer, test_wolfSSL_EC_POINT_point2hex_oversized covers the sink, and all three fail on the unfixed tree. The SP MC/DC drivers gain the new bit-length and sign branches and an operand exactly the field's width but not below the modulus, without which the new range compare was only ever reached with the bit-length guard already true. Those rows now assert ECC_OUT_OF_RANGE_E rather than discarding the result, and report a failure separately from a skipped row; removing either guard from a back end makes them fail.
wolfSSL-Fenrir-bot
dismissed
their stale review
September 17, 2026 15:37
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11484
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-src, wolfssl-bugs
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Six independent robustness fixes, kept as separate commits. Each adds a missing bound or corrects an offset calculation, and each ships a regression test.
Changes
tls13: the ECH acceptance confirmation is computed from the handshake header rather than from the buffer the message arrived in. The two differ for a fragmented or coalesced ServerHello.evp: the padded decrypt path no longer holds a stored block and a partial input block at the same time, keepingEVP_CipherUpdateoutput within the documentedinl + block size.EVP_CipherFinal()also returns a stored block when padding is disabled, where it was previously dropped.pkcs7: the AuthEnvelopedData authTag copy is bounded by the input remaining after the OCTET STRING header, and its bounds check is no longer compiled out in the default build. Two adjacent issues in the same state go with it:encOIDwas not restored on re-entry, and the re-buffer call bypassed the shared error handler.tls: the client rejects a ServerHello cipher suite that is not usable at the negotiated version, which is the checkVerifyServerSuite()already performs on the server, and stops offering TLS 1.3-only suites below TLS 1.3.ecc: explicit EC domain parameters, and curves installed throughwc_ecc_set_custom_curve(), are bounded byMAX_ECC_BYTES, which is what the fixed buffers inecc.care dimensioned for.ecc: X9.63 ordinates must match the curve size,wolfSSL_EC_POINT_point2hex()refuses ordinates wider than the curve, andsp_ecc_is_point_*()in all eight SP back ends range checks its input so the back ends agree on which points are acceptable.The
sp_*.cfiles are generator output based on https://github.com/wolfSSL/scripts/pull/685.Behavioral changes
ECC_BAD_ARG_E; they are non-canonical and OpenSSL rejects them too, so a conforming peer is unaffected.ECC_OUT_OF_RANGE_Efor ordinates that are not less than the field prime, as the other math back ends already did.EVP_DecryptUpdate()emits a completed block as soon as more input arrives rather than withholding it until Final, sooutmust hold at leastinl + EVP_CIPHER_CTX_block_size()bytes and must not overlapin. The doxygen block and its example are corrected.MAX_ECC_BYTES(66 bytes, 128 with SAKKE) fail withASN_PARSE_E; every standard prime curve up to P-521 still decodes. InWOLFSSL_ECC_CURVE_STATICbuilds this also refuses the Koblitz curves, whose order is a byte wider than their prime, in a build sized exactly to that prime.UNSUPPORTED_SUITEat ServerHello.WC_PKCS7_WANT_READ_Ewhen streaming andBUFFER_Eotherwise. A chunked delivery split inside the tag now completes once the rest arrives.