Harden digest context deserialization

The deserialization functions for SHA2 and SHA3 digest contexts did not
sufficiently validate the incoming data. Corruption in transmission or
on saved disk data could cause a out-of-bounds memory access if buffer
sizes did not match expected values.

Add sanity checks to the SHA2 and SHA3 deserialization functions to validate
buffer-related fields before they are used. The serialization format for these
digests has been changed to place these critical fields early in the stream to
enable this validation.

Additionally, add a note to the EVP_DigestInit man page to warn users that
deserialization should only be performed on trusted data. The checks we
implement are not meant to address processing of untrusted data
maliciously crafted by an attacker.

Application that need to store data or transmit it through untrusted
media SHOULD implement proper encryption and message authentication
on their own using things like CMS or other appropriate secure message
containers.

These check have been added also to quiet a bit security researchers
that try to find any way to claim CVE bounties even in completely
unlikely or invalid scenarios.

Signed-off-by: Simo Sorce <simo@redhat.com>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from https://github.com/openssl/openssl/pull/29404)

diff --git a/doc/man3/EVP_DigestInit.pod b/doc/man3/EVP_DigestInit.pod
index 14d4f7fa9c1a2bd2c621871a076e4df29a40211a..54a9a92f7ce258b89996e0cf75b7348f4e971c80 100644 (file)
--- a/doc/man3/EVP_DigestInit.pod
+++ b/doc/man3/EVP_DigestInit.pod
@@ -340,6 +340,11 @@ different OpenSSL version and does not guarantee interoperability between
 different providers. Some providers may not allow export/import across
 process boundaries.
 
+NOTE: Applications must guarantee that only trusted data is used during
+deserialization. The deserialization operation is not built to address
+adversarial data compromise, and only basic checks to protect from simple
+mistakes are implemented.
+
 =item EVP_MD_CTX_dup()
 
 Can be used to duplicate the message digest state from I<in>.  This is useful

diff --git a/providers/implementations/digests/sha2_prov.c b/providers/implementations/digests/sha2_prov.c
index 31a6b8545028c62fd9ca61ff1280174baf47b2af..c75b6d9b3f18d23b5c413f292e823f7ab166a580 100644 (file)
--- a/providers/implementations/digests/sha2_prov.c
+++ b/providers/implementations/digests/sha2_prov.c
@@ -58,10 +58,10 @@ static const unsigned char sha256magic[] = "SHA256v1";
     (                                                 \
         SHA256MAGIC_LEN /* magic */                   \
         + sizeof(uint32_t) /* c->md_len */            \
+        + sizeof(uint32_t) /* c->num */               \
         + sizeof(uint32_t) * 8 /* c->h */             \
         + sizeof(uint32_t) * 2 /* c->Nl + c->Nh */    \
         + sizeof(uint32_t) * SHA_LBLOCK /* c->data */ \
-        + sizeof(uint32_t) /* c->num */               \
     )
 
 static int SHA256_Serialize(SHA256_CTX *c, unsigned char *out,
@@ -90,6 +90,9 @@ static int SHA256_Serialize(SHA256_CTX *c, unsigned char *out,
     /* md_len */
     p = OPENSSL_store_u32_le(p, c->md_len);
 
+    /* num */
+    p = OPENSSL_store_u32_le(p, c->num);
+
     /* h */
     for (i = 0; i < sizeof(c->h) / sizeof(SHA_LONG); i++)
         p = OPENSSL_store_u32_le(p, c->h[i]);
@@ -102,15 +105,17 @@ static int SHA256_Serialize(SHA256_CTX *c, unsigned char *out,
     for (i = 0; i < SHA_LBLOCK; i++)
         p = OPENSSL_store_u32_le(p, c->data[i]);
 
-    /* num */
-    p = OPENSSL_store_u32_le(p, c->num);
-
     if (outlen != NULL)
         *outlen = SHA256_SERIALIZATION_LEN;
 
     return 1;
 }
 
+/*
+ * This function only performs basic input sanity checks and is not
+ * built to handle malicious input data. Only trusted input should be
+ * fed to this function
+ */
 static int SHA256_Deserialize(SHA256_CTX *c, const unsigned char *in,
     size_t inlen)
 {
@@ -133,6 +138,12 @@ static int SHA256_Deserialize(SHA256_CTX *c, const unsigned char *in,
         return 0;
     }
 
+    /* num check */
+    p = OPENSSL_load_u32_le(&val, p);
+    if (val >= sizeof(c->data))
+        return 0;
+    c->num = (unsigned int)val;
+
     /* h */
     for (i = 0; i < (sizeof(c->h) / sizeof(SHA_LONG)); i++) {
         p = OPENSSL_load_u32_le(&val, p);
@@ -151,10 +162,6 @@ static int SHA256_Deserialize(SHA256_CTX *c, const unsigned char *in,
         c->data[i] = (SHA_LONG)val;
     }
 
-    /* num */
-    p = OPENSSL_load_u32_le(&val, p);
-    c->num = (unsigned int)val;
-
     return 1;
 }
 
@@ -164,10 +171,10 @@ static const unsigned char sha512magic[] = "SHA512v1";
     (                                              \
         SHA512MAGIC_LEN /* magic */                \
         + sizeof(uint32_t) /* c->md_len */         \
+        + sizeof(uint32_t) /* c->num */            \
         + sizeof(uint64_t) * 8 /* c->h */          \
         + sizeof(uint64_t) * 2 /* c->Nl + c->Nh */ \
         + SHA512_CBLOCK /* c->u.d/c->u.p */        \
-        + sizeof(uint32_t) /* c->num */            \
     )
 
 static int SHA512_Serialize(SHA512_CTX *c, unsigned char *out,
@@ -196,6 +203,9 @@ static int SHA512_Serialize(SHA512_CTX *c, unsigned char *out,
     /* md_len */
     p = OPENSSL_store_u32_le(p, c->md_len);
 
+    /* num */
+    p = OPENSSL_store_u32_le(p, c->num);
+
     /* h */
     for (i = 0; i < sizeof(c->h) / sizeof(SHA_LONG64); i++)
         p = OPENSSL_store_u64_le(p, c->h[i]);
@@ -208,15 +218,17 @@ static int SHA512_Serialize(SHA512_CTX *c, unsigned char *out,
     memcpy(p, c->u.p, SHA512_CBLOCK);
     p += SHA512_CBLOCK;
 
-    /* num */
-    p = OPENSSL_store_u32_le(p, c->num);
-
     if (outlen != NULL)
         *outlen = SHA512_SERIALIZATION_LEN;
 
     return 1;
 }
 
+/*
+ * This function only performs basic input sanity checks and is not
+ * built to handle malicious input data. Only trusted input should be
+ * fed to this function
+ */
 static int SHA512_Deserialize(SHA512_CTX *c, const unsigned char *in,
     size_t inlen)
 {
@@ -239,6 +251,12 @@ static int SHA512_Deserialize(SHA512_CTX *c, const unsigned char *in,
     if ((unsigned int)val32 != c->md_len)
         return 0;
 
+    /* num check */
+    p = OPENSSL_load_u32_le(&val32, p);
+    if (val32 >= sizeof(c->u.d))
+        return 0;
+    c->num = (unsigned int)val32;
+
     /* h */
     for (i = 0; i < (sizeof(c->h) / sizeof(SHA_LONG64)); i++) {
         p = OPENSSL_load_u64_le(&val, p);
@@ -255,10 +273,6 @@ static int SHA512_Deserialize(SHA512_CTX *c, const unsigned char *in,
     memcpy(c->u.p, p, SHA512_CBLOCK);
     p += SHA512_CBLOCK;
 
-    /* num */
-    p = OPENSSL_load_u32_le(&val32, p);
-    c->num = (unsigned int)val32;
-
     return 1;
 }
 

diff --git a/providers/implementations/digests/sha3_prov.c b/providers/implementations/digests/sha3_prov.c
index 39afa0e15c821cfae2c2c59a7a982e744f8823ee..5072cd8e79deaab6a31b61a2b199e9f358d68f63 100644 (file)
--- a/providers/implementations/digests/sha3_prov.c
+++ b/providers/implementations/digests/sha3_prov.c
@@ -586,8 +586,8 @@ static const unsigned char keccakmagic[] = "KECCAKv1";
         KECCAKMAGIC_LEN /* magic string */                                           \
         + sizeof(uint64_t) /* impl-ID */                                             \
         + sizeof(uint64_t) /* c->md_size */                                          \
-        + (sizeof(uint64_t) * 5 * 5) /* c->A */                                      \
         + (sizeof(uint64_t) * 4) /* c->block_size, c->bufsz, c->pad, c->xof_state */ \
+        + (sizeof(uint64_t) * 5 * 5) /* c->A */                                      \
         + (KECCAK1600_WIDTH / 8 - 32) /* c->buf */                                   \
     )
 
@@ -618,17 +618,17 @@ static int KECCAK_Serialize(KECCAK1600_CTX *c, int impl_id,
     p = OPENSSL_store_u64_le(p, impl_id);
     p = OPENSSL_store_u64_le(p, c->md_size);
 
+    p = OPENSSL_store_u64_le(p, c->block_size);
+    p = OPENSSL_store_u64_le(p, c->bufsz);
+    p = OPENSSL_store_u64_le(p, c->pad);
+    p = OPENSSL_store_u64_le(p, c->xof_state);
+
     /* A matrix */
     for (i = 0; i < 5; i++) {
         for (j = 0; j < 5; j++)
             p = OPENSSL_store_u64_le(p, c->A[i][j]);
     }
 
-    p = OPENSSL_store_u64_le(p, c->block_size);
-    p = OPENSSL_store_u64_le(p, c->bufsz);
-    p = OPENSSL_store_u64_le(p, c->pad);
-    p = OPENSSL_store_u64_le(p, c->xof_state);
-
     if (outlen != NULL)
         *outlen = KECCAK_SERIALIZATION_LEN;
 
@@ -638,6 +638,11 @@ static int KECCAK_Serialize(KECCAK1600_CTX *c, int impl_id,
     return 1;
 }
 
+/*
+ * This function only performs basic input sanity checks and is not
+ * built to handle malicious input data. Only trusted input should be
+ * fed to this function
+ */
 static int KECCAK_Deserialize(KECCAK1600_CTX *c, int impl_id,
     const unsigned char *input, size_t len)
 {
@@ -664,6 +669,21 @@ static int KECCAK_Deserialize(KECCAK1600_CTX *c, int impl_id,
     if (val != (uint64_t)c->md_size)
         return 0;
 
+    /* check that block_size is congruent with the initialized value */
+    p = OPENSSL_load_u64_le(&val, p);
+    if (val != c->block_size)
+        return 0;
+    /* check that bufsz does not exceed block_size */
+    p = OPENSSL_load_u64_le(&val, p);
+    if (val > c->block_size)
+        return 0;
+    c->bufsz = (size_t)val;
+    p = OPENSSL_load_u64_le(&val, p);
+    if (val != c->pad)
+        return 0;
+    p = OPENSSL_load_u64_le(&val, p);
+    c->xof_state = (int)val;
+
     /* A matrix */
     for (i = 0; i < 5; i++) {
         for (j = 0; j < 5; j++) {
@@ -672,15 +692,6 @@ static int KECCAK_Deserialize(KECCAK1600_CTX *c, int impl_id,
         }
     }
 
-    p = OPENSSL_load_u64_le(&val, p);
-    c->block_size = (size_t)val;
-    p = OPENSSL_load_u64_le(&val, p);
-    c->bufsz = (size_t)val;
-    p = OPENSSL_load_u64_le(&val, p);
-    c->pad = (unsigned char)val;
-    p = OPENSSL_load_u64_le(&val, p);
-    c->xof_state = (int)val;
-
     /* buf */
     memcpy(c->buf, p, sizeof(c->buf));