When allocating a crypto_digest_t, allocate no more bytes than needed

Previously we would allocate as many bytes as we'd need for a
keccak--even when we were only calculating SHA1.

Closes ticket 17796.

1 files changed, 52 insertions, 20 deletions

diff --git a/src/common/crypto.c b/src/common/crypto.c
index a11ca79c04..265d0650e2 100644
--- a/src/common/crypto.c
+++ b/src/common/crypto.c
@@ -1736,23 +1736,57 @@ crypto_digest_algorithm_get_length(digest_algorithm_t alg)
 
 /** Intermediate information about the digest of a stream of data. */
 struct crypto_digest_t {
+  digest_algorithm_t algorithm; /**< Which algorithm is in use? */
+   /** State for the digest we're using.  Only one member of the
+    * union is usable, depending on the value of <b>algorithm</b>. Note also
+    * that space for other members might not even be allocated!
+    */
   union {
     SHA_CTX sha1; /**< state for SHA1 */
     SHA256_CTX sha2; /**< state for SHA256 */
     SHA512_CTX sha512; /**< state for SHA512 */
     keccak_state sha3; /**< state for SHA3-[256,512] */
-  } d; /**< State for the digest we're using.  Only one member of the
-        * union is usable, depending on the value of <b>algorithm</b>. */
-  digest_algorithm_bitfield_t algorithm : 8; /**< Which algorithm is in use? */
+  } d;
 };
 
+/**
+ * Return the number of bytes we need to malloc in order to get a
+ * crypto_digest_t for <b>alg</b>, or the number of bytes we need to wipe
+ * when we free one.
+ */
+static size_t
+crypto_digest_alloc_bytes(digest_algorithm_t alg)
+{
+  /* Helper: returns the number of bytes in the 'f' field of 'st' */
+#define STRUCT_FIELD_SIZE(st, f) (sizeof( ((st*)0)->f ))
+  /* Gives the length of crypto_digest_t through the end of the field 'd' */
+#define END_OF_FIELD(f) (STRUCT_OFFSET(crypto_digest_t, f) + \
+                         STRUCT_FIELD_SIZE(crypto_digest_t, f))
+  switch (alg) {
+    case DIGEST_SHA1:
+      return END_OF_FIELD(d.sha1);
+    case DIGEST_SHA256:
+      return END_OF_FIELD(d.sha2);
+    case DIGEST_SHA512:
+      return END_OF_FIELD(d.sha512);
+    case DIGEST_SHA3_256:
+    case DIGEST_SHA3_512:
+      return END_OF_FIELD(d.sha3);
+    default:
+      tor_assert(0);
+      return 0;
+  }
+#undef END_OF_FIELD
+#undef STRUCT_FIELD_SIZE
+}
+
 /** Allocate and return a new digest object to compute SHA1 digests.
  */
 crypto_digest_t *
 crypto_digest_new(void)
 {
   crypto_digest_t *r;
-  r = tor_malloc(sizeof(crypto_digest_t));
+  r = tor_malloc(crypto_digest_alloc_bytes(DIGEST_SHA1));
   SHA1_Init(&r->d.sha1);
   r->algorithm = DIGEST_SHA1;
   return r;
@@ -1765,7 +1799,7 @@ crypto_digest256_new(digest_algorithm_t algorithm)
 {
   crypto_digest_t *r;
   tor_assert(algorithm == DIGEST_SHA256 || algorithm == DIGEST_SHA3_256);
-  r = tor_malloc(sizeof(crypto_digest_t));
+  r = tor_malloc(crypto_digest_alloc_bytes(algorithm));
   if (algorithm == DIGEST_SHA256)
     SHA256_Init(&r->d.sha2);
   else
@@ -1781,7 +1815,7 @@ crypto_digest512_new(digest_algorithm_t algorithm)
 {
   crypto_digest_t *r;
   tor_assert(algorithm == DIGEST_SHA512 || algorithm == DIGEST_SHA3_512);
-  r = tor_malloc(sizeof(crypto_digest_t));
+  r = tor_malloc(crypto_digest_alloc_bytes(algorithm));
   if (algorithm == DIGEST_SHA512)
     SHA512_Init(&r->d.sha512);
   else
@@ -1797,7 +1831,8 @@ crypto_digest_free(crypto_digest_t *digest)
 {
   if (!digest)
     return;
-  memwipe(digest, 0, sizeof(crypto_digest_t));
+  size_t bytes = crypto_digest_alloc_bytes(digest->algorithm);
+  memwipe(digest, 0, bytes);
   tor_free(digest);
 }
 
@@ -1856,8 +1891,9 @@ crypto_digest_get_digest(crypto_digest_t *digest,
     return;
   }
 
+  const size_t alloc_bytes = crypto_digest_alloc_bytes(digest->algorithm);
   /* memcpy into a temporary ctx, since SHA*_Final clears the context */
-  memcpy(&tmpenv, digest, sizeof(crypto_digest_t));
+  memcpy(&tmpenv, digest, alloc_bytes);
   switch (digest->algorithm) {
     case DIGEST_SHA1:
       SHA1_Final(r, &tmpenv.d.sha1);
@@ -1873,12 +1909,7 @@ crypto_digest_get_digest(crypto_digest_t *digest,
       log_warn(LD_BUG, "Handling unexpected algorithm %d", digest->algorithm);
       tor_assert(0); /* This is fatal, because it should never happen. */
     default:
-      log_warn(LD_BUG, "Called with unknown algorithm %d", digest->algorithm);
-      /* If fragile_assert is not enabled, then we should at least not
-       * leak anything. */
-      memwipe(r, 0xff, sizeof(r));
-      memwipe(&tmpenv, 0, sizeof(crypto_digest_t));
-      tor_fragile_assert();
+      tor_assert(0); /* Unreachable. */
       break;
   }
   memcpy(out, r, out_len);
@@ -1891,15 +1922,14 @@ crypto_digest_get_digest(crypto_digest_t *digest,
 crypto_digest_t *
 crypto_digest_dup(const crypto_digest_t *digest)
 {
-  crypto_digest_t *r;
   tor_assert(digest);
-  r = tor_malloc(sizeof(crypto_digest_t));
-  memcpy(r,digest,sizeof(crypto_digest_t));
-  return r;
+  const size_t alloc_bytes = crypto_digest_alloc_bytes(digest->algorithm);
+  return tor_memdup(digest, alloc_bytes);
 }
 
 /** Replace the state of the digest object <b>into</b> with the state
- * of the digest object <b>from</b>.
+ * of the digest object <b>from</b>.  Requires that 'into' and 'from'
+ * have the same digest type.
  */
 void
 crypto_digest_assign(crypto_digest_t *into,
@@ -1907,7 +1937,9 @@ crypto_digest_assign(crypto_digest_t *into,
 {
   tor_assert(into);
   tor_assert(from);
-  memcpy(into,from,sizeof(crypto_digest_t));
+  tor_assert(into->algorithm == from->algorithm);
+  const size_t alloc_bytes = crypto_digest_alloc_bytes(from->algorithm);
+  memcpy(into,from,alloc_bytes);
 }
 
 /** Given a list of strings in <b>lst</b>, set the <b>len_out</b>-byte digest