Guard SHA client hash block-transfer loops to propagate errors

[jackctj117]

This pull request makes a small but important change to the SHA hash processing functions in src/wh_client_crypto.c. The update ensures that the block processing loops for SHA-224, SHA-256, SHA-384, and SHA-512 only continue if there have been no errors so far, improving error handling and preventing further processing after a failure.

Improved error handling in SHA hash functions:

• Updated the block processing loops in wh_Client_Sha224, wh_Client_Sha256, wh_Client_Sha384, and wh_Client_Sha512 to check that ret == 0 before each iteration, ensuring the loop exits immediately if an error occurs. [1] [2] [3] [4]

[Copilot]

Pull request overview

This PR aims to improve error propagation in the client-side SHA block-processing paths by stopping block-transfer loops as soon as an error occurs, for SHA-224/256/384/512 in src/wh_client_crypto.c.

Changes:

• Add ret == 0 as a guard condition to the full-block processing while loops in wh_Client_Sha224, wh_Client_Sha256, wh_Client_Sha384, and wh_Client_Sha512.

Comments suppressed due to low confidence (4)

src/wh_client_crypto.c:4714

• Same issue as SHA256: after adding ret == 0 to the full-block loop condition, an error can leave more than one block remaining, and the subsequent “copy remaining data into the buffer” logic can append multiple blocks into sha224->buffer (single-block capacity). Please ensure no buffering occurs when ret != 0, or cap buffering to one block / return immediately on error.

        while (ret == 0 && (inLen - i) >= WC_SHA224_BLOCK_SIZE) {
            memcpy(sha224BufferBytes, in + i, WC_SHA224_BLOCK_SIZE);
            ret = _xferSha224BlockAndUpdateDigest(ctx, sha224, 0);
            i += WC_SHA224_BLOCK_SIZE;
        }

src/wh_client_crypto.c:4995

• After this change, if _xferSha384BlockAndUpdateDigest fails, the full-block loop stops but the function still proceeds to buffer the remaining input bytes. Because the remaining input may include multiple full blocks, this can overflow sha384->buffer (which only holds one block) and corrupt memory. Suggest returning immediately when ret != 0, or guarding/capping the remainder-buffering logic.

        while (ret == 0 && (inLen - i) >= WC_SHA384_BLOCK_SIZE) {
            memcpy(sha384BufferBytes, in + i, WC_SHA384_BLOCK_SIZE);
            ret = _xferSha384BlockAndUpdateDigest(ctx, sha384, 0);
            i += WC_SHA384_BLOCK_SIZE;
        }

src/wh_client_crypto.c:5275

• With ret == 0 added to the full-block loop, a transfer error can leave inLen - i containing multiple blocks. The later remainder-copy loop will then copy all remaining bytes into sha512->buffer (one-block sized), which can overflow and corrupt memory. Please skip buffering on error or otherwise ensure at most one block is retained before returning.

        while (ret == 0 && (inLen - i) >= WC_SHA512_BLOCK_SIZE) {
            memcpy(sha512BufferBytes, in + i, WC_SHA512_BLOCK_SIZE);
            ret = _xferSha512BlockAndUpdateDigest(ctx, sha512, 0);
            i += WC_SHA512_BLOCK_SIZE;
        }

src/wh_client_crypto.c:4424

• With the new ret == 0 guard, the loop can exit early on error while inLen - i may still contain multiple full blocks. The remainder-buffering loop later in this function will then copy all remaining bytes into sha256->buffer (one-block sized), which can overflow sha256BufferBytes and corrupt memory. Consider returning immediately on error or guarding the remainder-copy path with ret == 0 (or otherwise ensuring at most one block is buffered).

        while (ret == 0 && (inLen - i) >= WC_SHA256_BLOCK_SIZE) {
            memcpy(sha256BufferBytes, in + i, WC_SHA256_BLOCK_SIZE);
            ret = _xferSha256BlockAndUpdateDigest(ctx, sha256, 0);
            i += WC_SHA256_BLOCK_SIZE;
        }

---

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

You can also share your feedback on Copilot code review. Take the survey.

[bigbrett]

This only adds an early exit on error path for one of the multiple operations requested. So it would early exit some block transfers but not others. In the case of a one-shot SHA, where both input and output data are present, it would stop processing input data, but continue with the finalize. The fix is incomplete.