Address bug fixes sent in by ZD 21534

[night1rider]

See: ZD 21534

wolfSSL_use_AltPrivateKey_Id inverted AllocDer check (e31abd8, 13af706)

The AllocDer success check is inverted. It checks == 0 instead 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 failure ret never gets updated so the caller sees false success and the X509 ref count leaks. The sibling wolfSSL_CTX_add1_chain_cert uses the correct assignment.

wolfSSL_CTX_set_tmp_dh wrong logical operator and comparison (5003c03)

The error check uses && and < 0 when 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 #endif comments say HAVE_ED25519 but they actually close WOLFSSL_SM2 && WOLFSSL_SM3 blocks. 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 FreeDer runs 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. Affects wolfSSL_SetTmpDH_buffer and wolfSSL_CTX_SetTmpDH_buffer when called with WOLFSSL_FILETYPE_ASN1.

[dgarske]

🐺 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

[dgarske]

🟡 [Medium] No regression test for wolfSSL_add1_chain_cert comparison-to-assignment fix
💡 SUGGEST test

The fix changes ret == wolfSSL_X509_up_ref(x509) to ret = wolfSSL_X509_up_ref(x509). While there is an existing test_wolfSSL_CTX_add1_chain_cert that exercises wolfSSL_add1_chain_cert, no new test was added specifically to catch this regression. The old bug was masked because wolfSSL_X509_up_ref normally returns 1 and ret was already 1, making the comparison accidentally true. A test that forces wolfSSL_X509_up_ref to fail (e.g., by corrupting the ref count mutex or testing error propagation) would guard against future regressions of this type. The PR description acknowledges the bug but no dedicated test was added, unlike the AltPrivateKey fixes which got regression tests.

Recommendation: Consider adding a targeted regression test or at minimum a comment in the existing test noting which code path validates this fix.

— Skoll automated review via openclaw

[dgarske]

🟡 [Medium] No regression test for wolfSSL_CTX_set_tmp_dh operator fix
💡 SUGGEST test

The fix changes (pSz < 0) && (gSz < 0) to (pSz <= 0) || (gSz <= 0). The old code only detected failure when both p and g encoding failed, and missed zero-length returns. No new test was added to exercise the case where exactly one of p or g fails, or where a zero-length return occurs. The existing test_wolfSSL_tmp_dh covers valid DH operations but does not specifically test the error path that was broken. Unlike the AltPrivateKey fixes, there is no regression test for this.

Recommendation: Consider adding a test that triggers the single-operand failure or zero-length case to guard against regression of the && vs || and < 0 vs <= 0 fix.

— Skoll automated review via openclaw

[dgarske]

🟡 [Medium] ws_ctx_ssl_set_tmp_dh fix could benefit from a targeted test for the ASN1 path
💡 SUGGEST test

The fix replaces a dangerous cast-away-const + zero-size AllocDer pattern with a proper sized allocation + XMEMCPY. This is a correct and important fix — the old code pointed der->buffer directly at the caller's const buf, meaning FreeDer at line 5990 would try to free memory it didn't own (undefined behavior). The new code properly allocates (word32)sz bytes and copies data in. However, the ws_ctx_ssl_set_tmp_dh function is called by wolfSSL_SetTmpDH_buffer and wolfSSL_CTX_SetTmpDH_buffer, and there is no specific test exercising the WOLFSSL_FILETYPE_ASN1 format path through these callers. A test passing raw DER-encoded DH parameters would exercise the fixed code path.

Recommendation: Consider adding a test that calls wolfSSL_SetTmpDH_buffer or wolfSSL_CTX_SetTmpDH_buffer with WOLFSSL_FILETYPE_ASN1 to exercise the fixed DER allocation path.

— Skoll automated review via openclaw