commit a24ab549e6ae246b391155d7bed3790ac0e07de2
author David Benjamin <davidben@google.com> Thu Mar 25 15:26:25 2021 -0400
committer Adam Langley <agl@google.com> Mon Mar 29 16:27:23 2021 +0000
tree 36f6f3eec37f463f7cae40c09a5b04ce80be6431
parent 139adff9b27eaf0bdaac664ec4c9a7db2fe3f920

Use an unsized helper for truncated SHA-512 variants.

Although it is strictly fine to call SHA512_Final in SHA384_Final
(array sizes in C parameters are purely decorational, according to the
language), GCC 11 reportedly checks now and gets upset about the size
mismatch. Use an unsized helper function so all our code matches the
specified bounds.

Unfortunately, the bounds in all the functions are a bit misleading
because SHA512_Final really outputs based on sha->md_len (which Init
function you called) rather than which Final function. I've fixed this
places within a library where we mismatched and added asserts to the
smaller functions. SHA512_Final is assert-less because I've seen lots of
code use SHA384_Init / SHA512_Update / SHA512_Final.

This doesn't fix the SHA256 variant since that is generated by a pile of
macros in a multiply-included file. This is probably a good opportunity
to make that code less macro-heavy.

Update-Note: There is a small chance the asserts will trip something,
but hopefully not since I've left SHA512_Final alone.

Bug: 402
Change-Id: I4c9d579a63ee0a0dea103c19ef219c13bb9aa62c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46405
Reviewed-by: Adam Langley <agl@google.com>

crypto/fipsmodule/digest/digests.c

diff --git a/crypto/fipsmodule/digest/digests.c b/crypto/fipsmodule/digest/digests.c
index 16daeba..f006ebb 100644
--- a/crypto/fipsmodule/digest/digests.c
+++ b/crypto/fipsmodule/digest/digests.c
@@ -247,13 +247,21 @@
   CHECK(SHA512_256_Init(ctx->md_data));
 }
 
+static void sha512_256_update(EVP_MD_CTX *ctx, const void *data, size_t count) {
+  CHECK(SHA512_256_Update(ctx->md_data, data, count));
+}
+
+static void sha512_256_final(EVP_MD_CTX *ctx, uint8_t *md) {
+  CHECK(SHA512_256_Final(md, ctx->md_data));
+}
+
 DEFINE_METHOD_FUNCTION(EVP_MD, EVP_sha512_256) {
   out->type = NID_sha512_256;
   out->md_size = SHA512_256_DIGEST_LENGTH;
   out->flags = 0;
   out->init = sha512_256_init;
-  out->update = sha512_update;
-  out->final = sha512_final;
+  out->update = sha512_256_update;
+  out->final = sha512_256_final;
   out->block_size = 128;
   out->ctx_size = sizeof(SHA512_CTX);
 }

crypto/fipsmodule/sha/sha512.c

diff --git a/crypto/fipsmodule/sha/sha512.c b/crypto/fipsmodule/sha/sha512.c
index fd02574..ba86c1e 100644
--- a/crypto/fipsmodule/sha/sha512.c
+++ b/crypto/fipsmodule/sha/sha512.c
@@ -70,6 +70,8 @@
 // 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);
+
 int SHA384_Init(SHA512_CTX *sha) {
   sha->h[0] = UINT64_C(0xcbbb9d5dc1059ed8);
   sha->h[1] = UINT64_C(0x629a292a367cd507);
@@ -146,8 +148,8 @@
                     uint8_t out[SHA512_256_DIGEST_LENGTH]) {
   SHA512_CTX ctx;
   SHA512_256_Init(&ctx);
-  SHA512_Update(&ctx, data, len);
-  SHA512_Final(out, &ctx);
+  SHA512_256_Update(&ctx, data, len);
+  SHA512_256_Final(out, &ctx);
   OPENSSL_cleanse(&ctx, sizeof(ctx));
   return out;
 }
@@ -161,7 +163,8 @@
 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.
-  return SHA512_Final(out, sha);
+  assert(sha->md_len == SHA384_DIGEST_LENGTH);
+  return sha512_final_impl(out, sha);
 }
 
 int SHA384_Update(SHA512_CTX *sha, const void *data, size_t len) {
@@ -172,11 +175,11 @@
   return SHA512_Update(sha, data, len);
 }
 
-int SHA512_256_Final(uint8_t out[SHA512_256_DIGEST_LENGTH],
-                                    SHA512_CTX *sha) {
+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.
-  return SHA512_Final(out, sha);
+  assert(sha->md_len == SHA512_256_DIGEST_LENGTH);
+  return sha512_final_impl(out, sha);
 }
 
 void SHA512_Transform(SHA512_CTX *c, const uint8_t block[SHA512_CBLOCK]) {
@@ -232,6 +235,15 @@
 }
 
 int SHA512_Final(uint8_t out[SHA512_DIGEST_LENGTH], SHA512_CTX *sha) {
+  // Ideally we would assert |sha->md_len| is |SHA512_DIGEST_LENGTH| to match
+  // the size hint, but calling code often pairs |SHA384_Init| with
+  // |SHA512_Final| and expects |sha->md_len| to carry the over.
+  //
+  // TODO(davidben): Add an assert and fix code to match them up.
+  return sha512_final_impl(out, sha);
+}
+
+static int sha512_final_impl(uint8_t *out, SHA512_CTX *sha) {
   uint8_t *p = sha->p;
   size_t n = sha->num;