Various fixes in internal.c

[embhorn]

Description

Size checking and code quality issues from report

Fixes zd21594

Testing

None

Checklist

• added tests
• updated/added doxygen
• updated appropriate READMEs
• Updated manual and documentation

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #10224

Scan targets checked: wolfssl-bugs, wolfssl-compliance, wolfssl-consttime, wolfssl-defaults, wolfssl-mutation, wolfssl-proptest, wolfssl-src, wolfssl-zeroize

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #10224

Scan targets checked: wolfssl-bugs, wolfssl-compliance, wolfssl-consttime, wolfssl-defaults, wolfssl-mutation, wolfssl-proptest, wolfssl-src, wolfssl-zeroize

Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

src/internal.c:1

• AddPSKtoPreMasterSecret is declared as returning int, but this introduces a bare return; which is invalid in standard C for non-void functions (and may fail compilation depending on warnings-as-errors). Return an appropriate negative error code here (consistent with surrounding error returns in this function), and ensure the caller path handles it.
  src/internal.c:1
• This bounds check can overflow if idx is large (e.g., near UINT32_MAX), causing the addition to wrap and potentially bypass the check. Prefer an overflow-safe form like comparing len - idx against the required size (after ensuring idx <= len) to guarantee correctness.
  src/internal.c:1
• Using ((unsigned int)-1) as a max-value sentinel is less clear than using UINT_MAX from <limits.h>. Switching to UINT_MAX improves readability and avoids relying on a cast-from--1 idiom.
  src/internal.c:1
• inSz - idx is evaluated with the native types of inSz/idx and then compared against an explicitly unsigned LHS. Even though earlier checks likely ensure idx <= inSz, this mixed signed/unsigned arithmetic can still produce compiler warnings and is easy to regress. Consider casting the RHS to the same unsigned type explicitly (or rewriting as a remaining-bytes variable) to keep the comparison type-safe and clearer.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

MemBrowse Memory Report

No memory changes detected for:

• gcc-arm-cortex-m4- gcc-arm-cortex-m4-baremetal- gcc-arm-cortex-m4-min-ecc- gcc-arm-cortex-m4-tls12

[ColtonWilley]

Jenkins retest this please

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #10224

Scan targets checked: wolfssl-bugs, wolfssl-compliance, wolfssl-consttime, wolfssl-defaults, wolfssl-mutation, wolfssl-proptest, wolfssl-src, wolfssl-zeroize

Findings: 7
7 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

[wolfSSL-Fenrir-bot]

🔴 [High] Inverted preprocessor guard compiled out RSA-PSS signature verification in DHE key exchange
Category: Fail-open behavior

The original code used #ifndef WC_RSA_PSS to guard the rsa_pss_sa_algo case in the server-side signature algorithm self-check during DHE SendServerKeyExchange. This inverted guard meant that when WC_RSA_PSS was defined (the standard configuration for RSA-PSS support), the rsa_pss_sa_algo case was compiled out of the switch statement. Consequently, if the server negotiated RSA-PSS as the signature algorithm for DHE key exchange, the self-check would not match the expected algorithm case, effectively bypassing the signature algorithm verification. The PR's own test (test_tls12_dhe_rsa_pss_sigalg) confirms the handshake did not fail — demonstrating fail-open behavior where the missing check was silently skipped rather than causing a verification error. The fix correctly changes #ifndef to #ifdef and also adds the missing rsa_pss_pss_algo case.

switch (ssl->options.sigAlgo)
                        {
                        #ifndef NO_RSA
                        #ifdef WC_RSA_PSS
                            case rsa_pss_sa_algo:
                            case rsa_pss_pss_algo:
                        #endif
                            case rsa_sa_algo:
                            {

Recommendation: This fix is correct. The #ifdef WC_RSA_PSS guard now properly includes the RSA-PSS signature algorithm cases when PSS support is compiled in. Verify that all other #ifdef/#ifndef WC_RSA_PSS guards in the codebase are not similarly inverted — a grep for #ifndef WC_RSA_PSS guarding rsa_pss_sa_algo cases would be prudent.

[wolfSSL-Fenrir-bot]

🟠 [Medium] Untested rsa_pss_pss_algo case added as fallthrough
Category: Surviving deletion mutations

The PR adds case rsa_pss_pss_algo: as a new fallthrough case alongside the existing rsa_pss_sa_algo case in the server-side signature algorithm self-check. The new test test_tls12_dhe_rsa_pss_sigalg only exercises rsa_pss_sa_algo (standard RSA key with PSS signature) by using "RSA-PSS+SHA256" and asserting sigAlgo == rsa_pss_sa_algo. The rsa_pss_pss_algo path (PSS-only certificate keys) requires a dedicated RSA-PSS certificate to trigger, which the test does not set up. A mutation that deletes the case rsa_pss_pss_algo: line would not be caught by any test.

#ifdef WC_RSA_PSS
    case rsa_pss_sa_algo:
    case rsa_pss_pss_algo:
#endif
    case rsa_sa_algo:

Recommendation: Add a test that performs a DHE handshake using an RSA-PSS-only certificate (OID rsassaPss) so that the server negotiates rsa_pss_pss_algo and the new case is exercised.

[wolfSSL-Fenrir-bot]

🟠 [Medium] Old-format ClientHello cumulative bounds check could be deleted without test failure
Category: Surviving deletion mutations

The PR adds a cumulative bounds check if ((word32)clSuites.suiteSz + (word32)sessionSz + (word32)randomSz > inSz - idx) return BUFFER_ERROR; to validate that the parsed length fields do not exceed the actual input buffer. This is a critical safety check for untrusted network input, but there is no test that sends a malformed old-format ClientHello with inflated length fields to exercise this path. The entire check could be deleted (mutation) with no test detecting it.

if ((word32)clSuites.suiteSz + (word32)sessionSz + (word32)randomSz
        > inSz - idx)
    return BUFFER_ERROR;

Recommendation: Add a test that crafts an old-format ClientHello with suiteSz + sessionSz + randomSz exceeding the packet length, and verify that the server returns BUFFER_ERROR.

[wolfSSL-Fenrir-bot]

🟠 [Medium] ImportCipherSpecState AES bounds and NULL checks untested
Category: Surviving deletion mutations

The PR adds two new guard checks before accessing AES state during cipher spec import: (1) a bounds check (idx + 2 * WC_AES_BLOCK_SIZE) > len and (2) a NULL check ssl->encrypt.aes == NULL || ssl->decrypt.aes == NULL. Both checks protect against buffer over-read and NULL dereference respectively. These could be deleted without any test failure since no test exercises the import path with a truncated buffer or uninitialized AES objects.

if ((idx + 2 * WC_AES_BLOCK_SIZE) > len) {
    WOLFSSL_MSG("Buffer not large enough for AES state import");
    return BUFFER_E;
}
if (ssl->encrypt.aes == NULL || ssl->decrypt.aes == NULL) {
    WOLFSSL_MSG("AES cipher objects not allocated for import");
    return BAD_STATE_E;
}

Recommendation: Add tests for the session export/import path that provide a truncated export buffer (to trigger BUFFER_E) and an SSL object without initialized AES cipher objects (to trigger BAD_STATE_E).

[wolfSSL-Fenrir-bot]

🔵 [Low] EdDSA_Update integer overflow check could be deleted without test failure
Category: Surviving deletion mutations

The PR adds if (sz < 0 || ssl->hsHashes->length < 0 || ssl->hsHashes->length > INT_MAX - sz) return BUFFER_ERROR; to prevent integer overflow when computing the allocation size ssl->hsHashes->length + sz. While this is a valid safety check, no test exercises an EdDSA handshake with message sizes near INT_MAX. The entire check could be deleted with no test detecting it.

if (sz < 0 || ssl->hsHashes->length < 0 ||
        ssl->hsHashes->length > INT_MAX - sz)
    return BUFFER_ERROR;

Recommendation: Consider adding a unit test that calls the EdDSA hash update path with a large or negative sz value to verify the overflow check.

[wolfSSL-Fenrir-bot]

🔵 [Low] ProcessPeerCerts XFREE-before-XMALLOC for Ed25519/Ed448 keys untested
Category: Surviving deletion mutations

The PR adds XFREE(ssl->buffers.peerEd25519Key.buffer, ...) and XFREE(ssl->buffers.peerEd448Key.buffer, ...) before the corresponding XMALLOC calls. This fixes a memory leak when the peer key buffer was previously allocated (e.g., during renegotiation or multiple certificate processing). However, no test exercises a scenario where these buffers are non-NULL before the allocation, so deleting these XFREE calls would produce no test failure. The same pattern applies to the Ed448 fix at line 17211.

XFREE(ssl->buffers.peerEd25519Key.buffer,
      ssl->heap, DYNAMIC_TYPE_ED25519);
ssl->buffers.peerEd25519Key.buffer =
       (byte*)XMALLOC(args->dCert->pubKeySize, ...);

Recommendation: Add a test that processes peer certificates twice (e.g., via renegotiation with Ed25519/Ed448 keys) and verify no memory leak occurs, ideally under a memory leak checker.

[wolfSSL-Fenrir-bot]

🔵 [Low] DoCertificateStatus heap parameter change from NULL to ssl->heap untested
Category: Surviving constant mutations

The PR changes WC_FREE_VAR_EX(status, NULL, ...) to WC_FREE_VAR_EX(status, ssl->heap, ...) (and similarly for single and response). This fixes heap allocator mismatch when wolfSSL is configured with a custom heap. However, most test configurations use a single default heap where NULL and ssl->heap are equivalent. A mutation reverting ssl->heap back to NULL would not be caught by standard tests.

WC_FREE_VAR_EX(status, ssl->heap, DYNAMIC_TYPE_OCSP_STATUS);
WC_FREE_VAR_EX(single, ssl->heap, DYNAMIC_TYPE_OCSP_ENTRY);
WC_FREE_VAR_EX(response, ssl->heap, DYNAMIC_TYPE_OCSP_REQUEST);

Recommendation: Add an OCSP stapling v2 test that uses a custom heap allocator (wolfSSL_CTX_SetHeap) to ensure the ssl->heap parameter is required for correct deallocation.