Address bug fixes sent in by ZD 21534#10168
Merged
SparkiDev merged 9 commits intowolfSSL:masterfrom Apr 14, 2026
Merged
Conversation
dgarske
reviewed
Apr 9, 2026
Contributor
dgarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 3 total — 3 posted, 0 skipped
Posted findings
- [Medium] No regression test for wolfSSL_add1_chain_cert comparison-to-assignment fix —
src/ssl_load.c:5205 - [Medium] No regression test for wolfSSL_CTX_set_tmp_dh operator fix —
src/ssl_load.c:5867 - [Medium] ws_ctx_ssl_set_tmp_dh fix could benefit from a targeted test for the ASN1 path —
src/ssl_load.c:5932-5936
Review generated by Skoll via openclaw
bd90bb2 to
307d3ce
Compare
Contributor
Author
|
Jenkins retest this please |
dgarske
approved these changes
Apr 13, 2026
… label allocation
…instead of comparing against it, matching wolfSSL_CTX_add1_chain_cert
… to catch single-param failure and zero-length, matching wolfSSL_set_tmp_dh.
…ltPublicKey, Fix #endif comments closing WOLFSSL_SM2/SM3 blocks, not HAVE_ED25519
… actual size and copy data instead of pointing at caller's const buffer, which caused FreeDer to free non-owned memory.
Contributor
Author
|
rebased and forced to pull in #10202 to fix CI issues |
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10168
Scan targets checked: wolfssl-bugs, wolfssl-compliance, wolfssl-consttime, wolfssl-defaults, wolfssl-mutation, wolfssl-proptest, wolfssl-src, wolfssl-zeroize
No new issues found in the changed files. ✅
SparkiDev
approved these changes
Apr 14, 2026
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
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.
See: ZD 21534
wolfSSL_use_AltPrivateKey_Id inverted AllocDer check (e31abd8, 13af706)
The AllocDer success check is inverted. It checks
== 0instead of!= 0, so it treats successful allocations as failures and misses real allocation failures, leading to a NULL pointer write. The CTX level function has the correct check. A regression test was added.wolfSSL_use_AltPrivateKey_Label inverted AllocDer check (ae3c00f, cf5da7b)
Same inverted AllocDer check in the Label variant. The CTX level function has the correct check. A regression test was added.
wolfSSL_add1_chain_cert comparison instead of assignment (939f978)
ret == wolfSSL_X509_up_ref(x509)is a comparison when it should be an assignment (ret =). The happy path works by accident because both sides equal 1, but on failureretnever gets updated so the caller sees false success and the X509 ref count leaks. The siblingwolfSSL_CTX_add1_chain_certuses the correct assignment.wolfSSL_CTX_set_tmp_dh wrong logical operator and comparison (5003c03)
The error check uses
&&and< 0when it should use||and<= 0. This means it only catches the error when both p and g encoding fail, and it misses zero length returns. Three other instances of this same check in the file all use||and<= 0.ProcessBufferCertPublicKey wrong endif comments (b067686)
Two
#endifcomments sayHAVE_ED25519but they actually closeWOLFSSL_SM2 && WOLFSSL_SM3blocks. Copy paste error, no runtime impact.ws_ctx_ssl_set_tmp_dh cast away const (7d38e9c)
The DER path allocates an empty DerBuffer and points its buffer directly at the caller's const data, casting away const. When
FreeDerruns later it tries to free a pointer it doesn't own, which is undefined behavior. Fixed by allocating the DerBuffer with the actual size and copying the data in. AffectswolfSSL_SetTmpDH_bufferandwolfSSL_CTX_SetTmpDH_bufferwhen called withWOLFSSL_FILETYPE_ASN1.