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>
chromium-5359
David Benjamin 2 years ago committed by Boringssl LUCI CQ
parent 19009c51bf
commit 2749466282
  1. 16
      crypto/fipsmodule/sha/sha256.c
  2. 22
      crypto/fipsmodule/sha/sha512.c

@ -133,7 +133,7 @@ int SHA224_Update(SHA256_CTX *ctx, const void *data, size_t len) {
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 @@ static int sha256_final_impl(uint8_t *out, SHA256_CTX *c) {
// '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 @@ int SHA256_Final(uint8_t out[SHA256_DIGEST_LENGTH], SHA256_CTX *c) {
// |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

@ -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 @@ static void sha512_block_data_order(uint64_t *state, const uint8_t *in,
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_Update(SHA512_CTX *sha, const void *data, size_t len) {
}
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 @@ int SHA512_Final(uint8_t out[SHA512_DIGEST_LENGTH], SHA512_CTX *sha) {
// |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 @@ static int sha512_final_impl(uint8_t *out, SHA512_CTX *sha) {
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;

Loading…
Cancel
Save