From e2375e139ee410538a3a77e8921b49cc8e727bb0 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 27 May 2015 01:32:26 -0400 Subject: [PATCH] Low-level hash 'final' functions cannot fail. The SHA-2 family has some exceptions, but they're all programmer errors and should be documented as such. (Are the failure cases even necessary?) Change-Id: I00bd0a9450cff78d8caac479817fbd8d3de872b8 Reviewed-on: https://boringssl-review.googlesource.com/4953 Reviewed-by: Adam Langley --- crypto/sha/sha256.c | 5 ++++- crypto/sha/sha512.c | 6 +++++- include/openssl/sha.h | 12 ++++++++---- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/crypto/sha/sha256.c b/crypto/sha/sha256.c index 39ce52eb..8276bbb5 100644 --- a/crypto/sha/sha256.c +++ b/crypto/sha/sha256.c @@ -144,7 +144,10 @@ int SHA224_Final(uint8_t *md, SHA256_CTX *ctx) { * to truncate to amount of bytes not divisible by 4. I bet not, but if it is, * then default: case shall be extended. For reference. Idea behind separate * cases for pre-defined lenghts is to let the compiler decide if it's - * appropriate to unroll small loops. */ + * appropriate to unroll small loops. + * + * TODO(davidben): The small |md_len| case is one of the few places a low-level + * hash 'final' function can fail. This should never happen. */ #define HASH_MAKE_STRING(c, s) \ do { \ uint32_t ll; \ diff --git a/crypto/sha/sha512.c b/crypto/sha/sha512.c index b68d5a2d..57c96ab9 100644 --- a/crypto/sha/sha512.c +++ b/crypto/sha/sha512.c @@ -276,7 +276,9 @@ int SHA512_Final(uint8_t *md, SHA512_CTX *sha) { sha512_block_data_order(sha, p, 1); - if (md == 0) { + if (md == NULL) { + /* TODO(davidben): This NULL check is absent in other low-level hash 'final' + * functions and is one of the few places one can fail. */ return 0; } @@ -312,6 +314,8 @@ int SHA512_Final(uint8_t *md, SHA512_CTX *sha) { break; /* ... as well as make sure md_len is not abused. */ default: + /* TODO(davidben): This bad |md_len| case is one of the few places a + * low-level hash 'final' function can fail. This should never happen. */ return 0; } diff --git a/include/openssl/sha.h b/include/openssl/sha.h index 0e37c45a..ac2ab758 100644 --- a/include/openssl/sha.h +++ b/include/openssl/sha.h @@ -120,7 +120,8 @@ OPENSSL_EXPORT int SHA224_Init(SHA256_CTX *sha); OPENSSL_EXPORT int SHA224_Update(SHA256_CTX *sha, const void *data, size_t len); /* SHA224_Final adds the final padding to |sha| and writes the resulting digest - * to |md|, which must have at least |SHA224_DIGEST_LENGTH| bytes of space. */ + * to |md|, which must have at least |SHA224_DIGEST_LENGTH| bytes of space. It + * returns one on success and zero on programmer error. */ OPENSSL_EXPORT int SHA224_Final(uint8_t *md, SHA256_CTX *sha); /* SHA224 writes the digest of |len| bytes from |data| to |out| and returns @@ -144,7 +145,8 @@ OPENSSL_EXPORT int SHA256_Init(SHA256_CTX *sha); OPENSSL_EXPORT int SHA256_Update(SHA256_CTX *sha, const void *data, size_t len); /* SHA256_Final adds the final padding to |sha| and writes the resulting digest - * to |md|, which must have at least |SHA256_DIGEST_LENGTH| bytes of space. */ + * to |md|, which must have at least |SHA256_DIGEST_LENGTH| bytes of space. It + * returns one on success and zero on programmer error. */ OPENSSL_EXPORT int SHA256_Final(uint8_t *md, SHA256_CTX *sha); /* SHA256 writes the digest of |len| bytes from |data| to |out| and returns @@ -179,7 +181,8 @@ OPENSSL_EXPORT int SHA384_Init(SHA512_CTX *sha); OPENSSL_EXPORT int SHA384_Update(SHA512_CTX *sha, const void *data, size_t len); /* SHA384_Final adds the final padding to |sha| and writes the resulting digest - * to |md|, which must have at least |SHA384_DIGEST_LENGTH| bytes of space. */ + * to |md|, which must have at least |SHA384_DIGEST_LENGTH| bytes of space. It + * returns one on success and zero on programmer error. */ OPENSSL_EXPORT int SHA384_Final(uint8_t *md, SHA512_CTX *sha); /* SHA384 writes the digest of |len| bytes from |data| to |out| and returns @@ -207,7 +210,8 @@ OPENSSL_EXPORT int SHA512_Init(SHA512_CTX *sha); OPENSSL_EXPORT int SHA512_Update(SHA512_CTX *sha, const void *data, size_t len); /* SHA512_Final adds the final padding to |sha| and writes the resulting digest - * to |md|, which must have at least |SHA512_DIGEST_LENGTH| bytes of space. */ + * to |md|, which must have at least |SHA512_DIGEST_LENGTH| bytes of space. It + * returns one on success and zero on programmer error. */ OPENSSL_EXPORT int SHA512_Final(uint8_t *md, SHA512_CTX *sha); /* SHA512 writes the digest of |len| bytes from |data| to |out| and returns