crash in md_rand.c when MD_Init fails

[bernd-edlinger]

Hi,

I tried to test out of memory handling with openssl_1.0.2j.

There is a crash in RAND_bytes, when MD_Init fails,
a later MD_Update crashes with null pointer.

Not sure if return code of MD_Update and MD_Final need to be
handled as well, at least with SHA1 there is no crash if
that is ignored.

possible, but probably incomplete fix follows.

[bernd-edlinger]

Hrmm..

attached a readable patch file (hopefully)

rand-patch.diff.txt

[richsalz]

@bernd-edlinger , Is this a trivial change? If not, we'll need a CLA from you.

[richsalz]

@bernd-edlinger , Is this a trivial change? If not, we'll need a CLA from you.

[bernd-edlinger]

It is probably trivial, only checking return codes.
But it is not meant to be checked in as is.
It is only a hint, what might be done to fix it.
I have a copyright assignment with the FSF already, (I contribute to gcc occasionally).

[levitte]

FYI, we are not the FSF.

[bernd-edlinger]

OK understood.
Maybe I should do the paperwork now.
Please give me instructions how to file a CLA.

[levitte]

Certainly! Please go here: https://www.openssl.org/policies/cla.html

[bernd-edlinger]

OK this will take a while, because it needs to be signed by my company.

in the meantime, here is a somewhat better patch:

--- crypto/rand/md_rand.c.orig  2016-09-26 11:49:07.000000000 +0200
+++ crypto/rand/md_rand.c       2016-12-15 18:53:04.159408650 +0100
@@ -266,17 +266,21 @@ static void ssleay_rand_add(const void *
         j = (num - i);
         j = (j > MD_DIGEST_LENGTH) ? MD_DIGEST_LENGTH : j;

-        MD_Init(&m);
-        MD_Update(&m, local_md, MD_DIGEST_LENGTH);
+        if (!MD_Init(&m) ||
+            !MD_Update(&m, local_md, MD_DIGEST_LENGTH))
+           goto err;
         k = (st_idx + j) - STATE_SIZE;
         if (k > 0) {
-            MD_Update(&m, &(state[st_idx]), j - k);
-            MD_Update(&m, &(state[0]), k);
+            if (!MD_Update(&m, &(state[st_idx]), j - k) ||
+                !MD_Update(&m, &(state[0]), k))
+                goto err;
         } else
-            MD_Update(&m, &(state[st_idx]), j);
+            if (!MD_Update(&m, &(state[st_idx]), j))
+                goto err;

         /* DO NOT REMOVE THE FOLLOWING CALL TO MD_Update()! */
-        MD_Update(&m, buf, j);
+        if (!MD_Update(&m, buf, j))
+            goto err;
         /*
          * We know that line may cause programs such as purify and valgrind
          * to complain about use of uninitialized data.  The problem is not,
@@ -285,8 +289,9 @@ static void ssleay_rand_add(const void *
          * insecure keys.
          */

-        MD_Update(&m, (unsigned char *)&(md_c[0]), sizeof(md_c));
-        MD_Final(&m, local_md);
+        if (!MD_Update(&m, (unsigned char *)&(md_c[0]), sizeof(md_c)) ||
+            !MD_Final(&m, local_md))
+            goto err;
         md_c[1]++;

         buf = (const char *)buf + j;
@@ -305,7 +310,6 @@ static void ssleay_rand_add(const void *
                 st_idx = 0;
         }
     }
-    EVP_MD_CTX_cleanup(&m);

     if (!do_not_lock)
         CRYPTO_w_lock(CRYPTO_LOCK_RAND);
@@ -326,6 +330,9 @@ static void ssleay_rand_add(const void *
 #if !defined(OPENSSL_THREADS) && !defined(OPENSSL_SYS_WIN32)
     assert(md_c[1] == md_count[1]);
 #endif
+
+ err:
+    EVP_MD_CTX_cleanup(&m);
 }

 static void ssleay_rand_seed(const void *buf, int num)
@@ -469,15 +476,18 @@ int ssleay_rand_bytes(unsigned char *buf
         /* num_ceil -= MD_DIGEST_LENGTH/2 */
         j = (num >= MD_DIGEST_LENGTH / 2) ? MD_DIGEST_LENGTH / 2 : num;
         num -= j;
-        MD_Init(&m);
+        if (!MD_Init(&m))
+           goto err;
 #ifndef GETPID_IS_MEANINGLESS
         if (curr_pid) {         /* just in the first iteration to save time */
-            MD_Update(&m, (unsigned char *)&curr_pid, sizeof curr_pid);
+            if (!MD_Update(&m, (unsigned char *)&curr_pid, sizeof curr_pid))
+                goto err;
             curr_pid = 0;
         }
 #endif
-        MD_Update(&m, local_md, MD_DIGEST_LENGTH);
-        MD_Update(&m, (unsigned char *)&(md_c[0]), sizeof(md_c));
+        if (!MD_Update(&m, local_md, MD_DIGEST_LENGTH) ||
+            !MD_Update(&m, (unsigned char *)&(md_c[0]), sizeof(md_c)))
+            goto err;

 #ifndef PURIFY                  /* purify complains */
         /*
@@ -487,16 +497,20 @@ int ssleay_rand_bytes(unsigned char *buf
          * builds it is not used: the removal of such a small source of
          * entropy has negligible impact on security.
          */
-        MD_Update(&m, buf, j);
+        if (!MD_Update(&m, buf, j))
+            goto err;
 #endif

         k = (st_idx + MD_DIGEST_LENGTH / 2) - st_num;
         if (k > 0) {
-            MD_Update(&m, &(state[st_idx]), MD_DIGEST_LENGTH / 2 - k);
-            MD_Update(&m, &(state[0]), k);
+            if (!MD_Update(&m, &(state[st_idx]), MD_DIGEST_LENGTH / 2 - k) ||
+                !MD_Update(&m, &(state[0]), k))
+                goto err;
         } else
-            MD_Update(&m, &(state[st_idx]), MD_DIGEST_LENGTH / 2);
-        MD_Final(&m, local_md);
+            if (!MD_Update(&m, &(state[st_idx]), MD_DIGEST_LENGTH / 2))
+                goto err;
+        if (!MD_Final(&m, local_md))
+            goto err;

         for (i = 0; i < MD_DIGEST_LENGTH / 2; i++) {
             /* may compete with other threads */
@@ -508,13 +522,18 @@ int ssleay_rand_bytes(unsigned char *buf
         }
     }

-    MD_Init(&m);
-    MD_Update(&m, (unsigned char *)&(md_c[0]), sizeof(md_c));
-    MD_Update(&m, local_md, MD_DIGEST_LENGTH);
+    if (!MD_Init(&m) ||
+        !MD_Update(&m, (unsigned char *)&(md_c[0]), sizeof(md_c)) ||
+        !MD_Update(&m, local_md, MD_DIGEST_LENGTH))
+        goto err;
     if (lock)
         CRYPTO_w_lock(CRYPTO_LOCK_RAND);
-    MD_Update(&m, md, MD_DIGEST_LENGTH);
-    MD_Final(&m, md);
+    if (!MD_Update(&m, md, MD_DIGEST_LENGTH) ||
+        !MD_Final(&m, md)) {
+        if (lock)
+            CRYPTO_w_unlock(CRYPTO_LOCK_RAND);
+        goto err;
+    }
     if (lock)
         CRYPTO_w_unlock(CRYPTO_LOCK_RAND);

@@ -529,6 +548,10 @@ int ssleay_rand_bytes(unsigned char *buf
                            "http://www.openssl.org/support/faq.html");
         return (0);
     }
+
+ err:
+    EVP_MD_CTX_cleanup(&m);
+    return (0);
 }

 static int ssleay_rand_nopseudo_bytes(unsigned char *buf, int num)

[bernd-edlinger]

Note I see more places where EVP_DigestInit_ex is used without error checking,
but these are probably not used in my test application.

For instance crypto/srp/srp_lib.c, ./crypto/srp/srp_vfy.c
and ssl/s3_enc.c.

And also here:
./crypto/pem/pem_sign.c:

void PEM_SignInit(EVP_MD_CTX *ctx, EVP_MD *type)
{
EVP_DigestInit_ex(ctx, type, NULL);
}

how is that supposed to work?

[richsalz]

it's a bug and we're stuck with it. some places errors just cannot be returned because they are "swallowed" by the existing functions :( We could add a new function if needed.

[bernd-edlinger]

No, fortunatley I don't need it.
BTW, in 1.1.0 it is "int PEM_SignInit(EVP_MD_CTX *ctx, EVP_MD *type)"

[ltdragonfly]

Which file defines the functions MD_Init and MD_Update?

[richsalz]

git grep MD_Init shows rand_lcl.h