Skip to content

Check final SHA buffer blocksize #5998

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
gojimmypi wants to merge 2 commits into from
Closed

Check final SHA buffer blocksize #5998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b220fc5
check final SHA buffer blocksize
gojimmypi Jan 22, 2023
b6f989f
don't check for SHA buffLen < 0 to appease some compilers
gojimmypi Jan 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions wolfcrypt/src/sha.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -719,6 +719,14 @@ int wc_ShaFinal(wc_Sha* sha, byte* hash)
return BAD_FUNC_ARG;
}

#ifndef WC_NO_HARDEN
Copy link
Contributor

@dgarske dgarske Jan 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move all this down to where the 0x80 is set. Use BUFFER_E as the failure. Inline with the error check on the update. Please fix other algos to use this too.

% git diff
diff --git a/wolfcrypt/src/sha.c b/wolfcrypt/src/sha.c
index bcfd1005d..3037d7cab 100644
--- a/wolfcrypt/src/sha.c
+++ b/wolfcrypt/src/sha.c
@@ -719,8 +719,6 @@ int wc_ShaFinal(wc_Sha* sha, byte* hash)
         return BAD_FUNC_ARG;
     }

-    local = (byte*)sha->buffer;
-
 #ifdef WOLF_CRYPTO_CB
     if (sha->devId != INVALID_DEVID) {
         ret = wc_CryptoCb_ShaHash(sha, NULL, 0, hash);
@@ -737,6 +735,11 @@ int wc_ShaFinal(wc_Sha* sha, byte* hash)
     }
 #endif /* WOLFSSL_ASYNC_CRYPT */

+    if (sha->buffLen > WC_SHA_BLOCK_SIZE - 1) {
+        return BUFFER_E;
+    }
+
+    local = (byte*)sha->buffer;
     local[sha->buffLen++] = 0x80;  /* add 1 */

     /* pad with zeros */

Copy link
Contributor

@dgarske dgarske Jan 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just remove the #ifndef WC_NO_HARDEN

/* We'll add a 0x80 byte at the end,
** so make sure we have appropriate buffer length. */
if (sha->buffLen > WC_SHA_BLOCK_SIZE - 1) {
return BAD_FUNC_ARG;
Copy link
Contributor

@SparkiDev SparkiDev Jan 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an internal error.
It shouldn't happen when the implementation is working appropriately.
The best error code I've found for this kind of issue is BAD_STATE_E.

}
#endif

local = (byte*)sha->buffer;

#ifdef WOLF_CRYPTO_CB
Expand Down
8 changes: 8 additions & 0 deletions wolfcrypt/src/sha256.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1243,6 +1243,14 @@ static int InitSha256(wc_Sha256* sha256)
return BAD_FUNC_ARG;
}

#ifndef WC_NO_HARDEN
/* We'll add a 0x80 byte at the end,
** so make sure we have appropriate buffer length. */
if (sha256->buffLen > WC_SHA256_BLOCK_SIZE - 1) {
return BAD_FUNC_ARG;
}
#endif

local = (byte*)sha256->buffer;
local[sha256->buffLen++] = 0x80; /* add 1 */

Expand Down
8 changes: 8 additions & 0 deletions wolfcrypt/src/sha512.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -966,6 +966,14 @@ static WC_INLINE int Sha512Final(wc_Sha512* sha512)
return BAD_FUNC_ARG;
}

#ifndef WC_NO_HARDEN
/* We'll add a 0x80 byte at the end,
** so make sure we have appropriate buffer length. */
if (sha512->buffLen > WC_SHA512_BLOCK_SIZE - 1) {
return BAD_FUNC_ARG;
}
#endif

local = (byte*)sha512->buffer;

local[sha512->buffLen++] = 0x80; /* add 1 */
Expand Down