Rework truncated SHA-2 to silence GCC 12 false positive warning.
GCC 12's -Wstringop-overflow flags issues in SHA224_Final, etc., because
it calls into generic code that might output a SHA-224 length or a
SHA-256 length, and the function prototype declares the array is only
sized for SHA-224.
This is a bit messy because OpenSSL's API for the truncated SHA-2 hashes
allows you to mix and match them. The output size is set by SHA224_Init
and then, originally, SHA256_Final and SHA224_Final were the same thing.
See how OpenSSL's own SHA224 function calls SHA224_Init + SHA256_Final:
https://github.com/openssl/openssl/blob/OpenSSL_1_1_1q/crypto/sha/sha256.c#L49-L61
To get the function prototype bounds to work out, we tightened this
slightly in
https://boringssl-review.googlesource.com/c/boringssl/+/47807 and added
an assert to SHA224_Final that ctx->md_len was the right size.
SHA256_Final does not have that assert yet. The assert says that mixing
SHA256_Init and SHA224_Final is a caller error.
This isn't good enough for GCC 12, which checks bounds assuming there is
no external invariant on ctx->md_len. This CL changes the behavior of
the shorter Final functions: they will now always output the length
implied by the function name. ctx->md_len only figures into an assert()
call. As we don't have the assert in the untruncated functions yet, I've
preserved their behavior, but the test run with cl/471617180 should tell
us whether apply this to all functions is feasible.
Update-Note: Truncated SHA-2 Final functions change behavior slightly,
but anyone affected by this behavior change would already have tripped
an assert() in debug builds.
Change-Id: I80fdcbe6ad76bc8713c0f2de329b958a2b35e8ae
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54246
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/fipsmodule/sha/sha256.c b/crypto/fipsmodule/sha/sha256.c
index 454b947..046f6e2 100644
--- a/crypto/fipsmodule/sha/sha256.c
+++ b/crypto/fipsmodule/sha/sha256.c
@@ -133,7 +133,7 @@
return SHA256_Update(ctx, data, len);
}
-static int sha256_final_impl(uint8_t *out, SHA256_CTX *c) {
+static int sha256_final_impl(uint8_t *out, size_t md_len, SHA256_CTX *c) {
crypto_md32_final(&sha256_block_data_order, c->h, c->data, SHA256_CBLOCK,
&c->num, c->Nh, c->Nl, /*is_big_endian=*/1);
@@ -141,12 +141,12 @@
// 'final' function can fail. SHA-512 does not have a corresponding check.
// These functions already misbehave if the caller arbitrarily mutates |c|, so
// can we assume one of |SHA256_Init| or |SHA224_Init| was used?
- if (c->md_len > SHA256_DIGEST_LENGTH) {
+ if (md_len > SHA256_DIGEST_LENGTH) {
return 0;
}
- assert(c->md_len % 4 == 0);
- const size_t out_words = c->md_len / 4;
+ assert(md_len % 4 == 0);
+ const size_t out_words = md_len / 4;
for (size_t i = 0; i < out_words; i++) {
CRYPTO_store_u32_be(out, c->h[i]);
out += 4;
@@ -162,14 +162,14 @@
// |SHA256_Final| and expects |sha->md_len| to carry the size over.
//
// TODO(davidben): Add an assert and fix code to match them up.
- return sha256_final_impl(out, c);
+ return sha256_final_impl(out, c->md_len, c);
}
int SHA224_Final(uint8_t out[SHA224_DIGEST_LENGTH], SHA256_CTX *ctx) {
- // SHA224_Init sets |ctx->md_len| to |SHA224_DIGEST_LENGTH|, so this has a
- // smaller output.
+ // This function must be paired with |SHA224_Init|, which sets |ctx->md_len|
+ // to |SHA224_DIGEST_LENGTH|.
assert(ctx->md_len == SHA224_DIGEST_LENGTH);
- return sha256_final_impl(out, ctx);
+ return sha256_final_impl(out, SHA224_DIGEST_LENGTH, ctx);
}
#ifndef SHA256_ASM
diff --git a/crypto/fipsmodule/sha/sha512.c b/crypto/fipsmodule/sha/sha512.c
index 708358e..2c7ce31 100644
--- a/crypto/fipsmodule/sha/sha512.c
+++ b/crypto/fipsmodule/sha/sha512.c
@@ -71,7 +71,7 @@
// this writing, so there is no need for a common collector/padding
// implementation yet.
-static int sha512_final_impl(uint8_t *out, SHA512_CTX *sha);
+static int sha512_final_impl(uint8_t *out, size_t md_len, SHA512_CTX *sha);
int SHA384_Init(SHA512_CTX *sha) {
sha->h[0] = UINT64_C(0xcbbb9d5dc1059ed8);
@@ -162,10 +162,10 @@
int SHA384_Final(uint8_t out[SHA384_DIGEST_LENGTH], SHA512_CTX *sha) {
- // |SHA384_Init| sets |sha->md_len| to |SHA384_DIGEST_LENGTH|, so this has a
- // smaller output.
+ // This function must be paired with |SHA384_Init|, which sets |sha->md_len|
+ // to |SHA384_DIGEST_LENGTH|.
assert(sha->md_len == SHA384_DIGEST_LENGTH);
- return sha512_final_impl(out, sha);
+ return sha512_final_impl(out, SHA384_DIGEST_LENGTH, sha);
}
int SHA384_Update(SHA512_CTX *sha, const void *data, size_t len) {
@@ -177,10 +177,10 @@
}
int SHA512_256_Final(uint8_t out[SHA512_256_DIGEST_LENGTH], SHA512_CTX *sha) {
- // |SHA512_256_Init| sets |sha->md_len| to |SHA512_256_DIGEST_LENGTH|, so this
- // has a |smaller output.
+ // This function must be paired with |SHA512_256_Init|, which sets
+ // |sha->md_len| to |SHA512_256_DIGEST_LENGTH|.
assert(sha->md_len == SHA512_256_DIGEST_LENGTH);
- return sha512_final_impl(out, sha);
+ return sha512_final_impl(out, SHA512_256_DIGEST_LENGTH, sha);
}
void SHA512_Transform(SHA512_CTX *c, const uint8_t block[SHA512_CBLOCK]) {
@@ -241,10 +241,10 @@
// |SHA512_Final| and expects |sha->md_len| to carry the size over.
//
// TODO(davidben): Add an assert and fix code to match them up.
- return sha512_final_impl(out, sha);
+ return sha512_final_impl(out, sha->md_len, sha);
}
-static int sha512_final_impl(uint8_t *out, SHA512_CTX *sha) {
+static int sha512_final_impl(uint8_t *out, size_t md_len, SHA512_CTX *sha) {
uint8_t *p = sha->p;
size_t n = sha->num;
@@ -268,8 +268,8 @@
return 0;
}
- assert(sha->md_len % 8 == 0);
- const size_t out_words = sha->md_len / 8;
+ assert(md_len % 8 == 0);
+ const size_t out_words = md_len / 8;
for (size_t i = 0; i < out_words; i++) {
CRYPTO_store_u64_be(out, sha->h[i]);
out += 8;
