r17963@catbus: nickm | 2008-02-07 10:14:25 -0500

 Be more thorough about memory poisoning and clearing.  Add an in-place version of aes_crypt in order to remove a memcpy from relay_crypt_one_payload.


svn:r13414

ChangeLog

@@ -13,6 +13,10 @@ Changes in version 0.2.0.19-alpha - 2008-02-??
     - Give more descriptive well-formedness errors for out-of-range
       hidden service descriptor/protocol versions.
 
+  o Minor features (security):
+    - Be slightly more paranoid about overwriting sensitive memory on free,
+      as a defensive programming tactic to ensure forward secrecy.
+
   o Deprecated features (controller):
     - The status/version/num-versioning and status/version/num-concurring
       GETINFO options are no longer useful in the V3 directory protocol:
@@ -59,6 +63,8 @@ Changes in version 0.2.0.19-alpha - 2008-02-??
       from a CREATE cell that we are waiting for a cpuworker to be
       assigned" and "onionskin from an EXTEND cell that we are going to
       send to an OR as soon as we are connected".
+    - Add an in-place version of aes_crypt so that we can avoid doing a
+      needless memcpy() call on each cell payload.
 
 
 Changes in version 0.2.0.18-alpha - 2008-01-25

src/common/aes.c

@@ -289,7 +289,7 @@ aes_crypt(aes_cnt_cipher_t *cipher, const char *input, size_t len,
    * of alignmement, using a bigger buffer, and so on. Not till after 0.1.2.x,
    * though. */
   int c = cipher->pos;
-  if (!len) return;
+  if (PREDICT_UNLIKELY(!len)) return;
 
   while (1) {
     do {
@@ -312,6 +312,42 @@ aes_crypt(aes_cnt_cipher_t *cipher, const char *input, size_t len,
   }
 }
 
+/** Encrypt <b>len</b> bytes from <b>input</b>, storing the results in place.
+ * Uses the key in <b>cipher</b>, and advances the counter by <b>len</b> bytes
+ * as it encrypts.
+ */
+void
+aes_crypt_inplace(aes_cnt_cipher_t *cipher, char *data, size_t len)
+{
+
+  /* XXXX This function is up to 5% of our runtime in some profiles;
+   * we should look into unrolling some of the loops; taking advantage
+   * of alignmement, using a bigger buffer, and so on. Not till after 0.1.2.x,
+   * though. */
+  int c = cipher->pos;
+  if (PREDICT_UNLIKELY(!len)) return;
+
+  while (1) {
+    do {
+      if (len-- == 0) { cipher->pos = c; return; }
+      *(data++) ^= cipher->buf[c];
+    } while (++c != 16);
+    cipher->pos = c = 0;
+    if (PREDICT_UNLIKELY(! ++COUNTER(cipher, 0))) {
+      if (PREDICT_UNLIKELY(! ++COUNTER(cipher, 1))) {
+        if (PREDICT_UNLIKELY(! ++COUNTER(cipher, 2))) {
+          ++COUNTER(cipher, 3);
+          UPDATE_CTR_BUF(cipher, 3);
+        }
+        UPDATE_CTR_BUF(cipher, 2);
+      }
+      UPDATE_CTR_BUF(cipher, 1);
+    }
+    UPDATE_CTR_BUF(cipher, 0);
+    _aes_fill_buf(cipher);
+  }
+}
+
 /** Reset the 128-bit counter of <b>cipher</b> to the 16-bit big-endian value
  * in <b>iv</b>. */
 void

src/common/aes.h

@@ -25,6 +25,7 @@ void aes_free_cipher(aes_cnt_cipher_t *cipher);
 void aes_set_key(aes_cnt_cipher_t *cipher, const char *key, int key_bits);
 void aes_crypt(aes_cnt_cipher_t *cipher, const char *input, size_t len,
                char *output);
+void aes_crypt_inplace(aes_cnt_cipher_t *cipher, char *data, size_t len);
 void aes_set_iv(aes_cnt_cipher_t *cipher, const char *iv);
 
 #endif

src/common/crypto.c

@@ -380,6 +380,7 @@ crypto_free_cipher_env(crypto_cipher_env_t *env)
 
   tor_assert(env->cipher);
   aes_free_cipher(env->cipher);
+  memset(env, 0, sizeof(crypto_cipher_env_t));
   tor_free(env);
 }
 
@@ -775,10 +776,13 @@ int
 crypto_pk_private_sign_digest(crypto_pk_env_t *env, char *to,
                               const char *from, size_t fromlen)
 {
+  int r;
   char digest[DIGEST_LEN];
   if (crypto_digest(digest,from,fromlen)<0)
     return -1;
-  return crypto_pk_private_sign(env,to,digest,DIGEST_LEN);
+  r = crypto_pk_private_sign(env,to,digest,DIGEST_LEN);
+  memset(digest, 0, sizeof(digest));
+  return r;
 }
 
 /** Perform a hybrid (public/secret) encryption on <b>fromlen</b>
@@ -1156,6 +1160,16 @@ crypto_cipher_decrypt(crypto_cipher_env_t *env, char *to,
   return 0;
 }
 
+/** Encrypt <b>len</b> bytes on <b>from</b> using the cipher in <b>env</b>;
+ * on success, return 0.  On failure, return -1.
+ */
+int
+crypto_cipher_crypt_inplace(crypto_cipher_env_t *env, char *buf, size_t len)
+{
+  aes_crypt_inplace(env->cipher, buf, len);
+  return 0;
+}
+
 /** Encrypt <b>fromlen</b> bytes (at least 1) from <b>from</b> with the key in
  * <b>cipher</b> to the buffer in <b>to</b> of length
  * <b>tolen</b>. <b>tolen</b> must be at least <b>fromlen</b> plus
@@ -1250,6 +1264,7 @@ crypto_new_digest_env(void)
 void
 crypto_free_digest_env(crypto_digest_env_t *digest)
 {
+  memset(digest, 0, sizeof(crypto_digest_env_t));
   tor_free(digest);
 }
 
@@ -1286,6 +1301,7 @@ crypto_digest_get_digest(crypto_digest_env_t *digest,
   memcpy(&tmpctx, &digest->d, sizeof(SHA_CTX));
   SHA1_Final(r, &tmpctx);
   memcpy(out, r, out_len);
+  memset(r, 0, sizeof(r));
 }
 
 /** Allocate and return a new digest object with the same state as
@@ -1588,11 +1604,13 @@ crypto_expand_key_material(const char *key_in, size_t key_in_len,
   }
   memset(tmp, 0, key_in_len+1);
   tor_free(tmp);
+  memset(digest, 0, sizeof(digest));
   return 0;
 
  err:
   memset(tmp, 0, key_in_len+1);
   tor_free(tmp);
+  memset(digest, 0, sizeof(digest));
   return -1;
 }
 
@@ -1668,6 +1686,7 @@ crypto_seed_rng(void)
     return rand_poll_status ? 0 : -1;
   }
   RAND_seed(buf, sizeof(buf));
+  memset(buf, 0, sizeof(buf));
   return 0;
 #else
   for (i = 0; filenames[i]; ++i) {
@@ -1682,6 +1701,7 @@ crypto_seed_rng(void)
       return -1;
     }
     RAND_seed(buf, sizeof(buf));
+    memset(buf, 0, sizeof(buf));
     return 0;
   }
 
@@ -1863,7 +1883,6 @@ base64_decode(char *dest, size_t destlen, const char *src, size_t srclen)
   ret += len;
   return ret;
 #else
-  #define ACC32
   const char *eos = src+srclen;
   uint32_t n=0;
   int n_idx=0;
@@ -2056,6 +2075,7 @@ base32_decode(char *dest, size_t destlen, const char *src, size_t srclen)
     }
   }
 
+  memset(tmp, 0, srclen);
   tor_free(tmp);
   tmp = NULL;
   return 0;
@@ -2099,6 +2119,7 @@ secret_to_key(char *key_out, size_t key_out_len, const char *secret,
     }
   }
   crypto_digest_get_digest(d, key_out, key_out_len);
+  memset(tmp, 0, 8+secret_len);
   tor_free(tmp);
   crypto_free_digest_env(d);
 }

src/common/crypto.h

@@ -128,6 +128,7 @@ int crypto_cipher_encrypt(crypto_cipher_env_t *env, char *to,
                           const char *from, size_t fromlen);
 int crypto_cipher_decrypt(crypto_cipher_env_t *env, char *to,
                           const char *from, size_t fromlen);
+int crypto_cipher_crypt_inplace(crypto_cipher_env_t *env, char *d, size_t len);
 
 int crypto_cipher_encrypt_with_iv(crypto_cipher_env_t *env,
                                   char *to, size_t tolen,

src/or/circuitlist.c

@@ -378,10 +378,12 @@ static void
 circuit_free(circuit_t *circ)
 {
   void *mem;
+  size_t memlen;
   tor_assert(circ);
   if (CIRCUIT_IS_ORIGIN(circ)) {
     origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ);
     mem = ocirc;
+    memlen = sizeof(origin_circuit_t);
     tor_assert(circ->magic == ORIGIN_CIRCUIT_MAGIC);
     if (ocirc->build_state) {
       if (ocirc->build_state->chosen_exit)
@@ -398,6 +400,7 @@ circuit_free(circuit_t *circ)
   } else {
     or_circuit_t *ocirc = TO_OR_CIRCUIT(circ);
     mem = ocirc;
+    memlen = sizeof(or_circuit_t);
     tor_assert(circ->magic == OR_CIRCUIT_MAGIC);
 
     if (ocirc->p_crypto)
@@ -432,7 +435,7 @@ circuit_free(circuit_t *circ)
    * "active" checks will be violated. */
   cell_queue_clear(&circ->n_conn_cells);
 
-  memset(circ, 0xAA, sizeof(circuit_t)); /* poison memory */
+  memset(circ, 0xAA, memlen); /* poison memory */
   tor_free(mem);
 }
 
@@ -499,7 +502,7 @@ circuit_free_cpath_node(crypt_path_t *victim)
   if (victim->extend_info)
     extend_info_free(victim->extend_info);
 
-  victim->magic = 0xDEADBEEFu;
+  memset(victim, 0xBB, sizeof(crypt_path_t)); /* poison memory */
   tor_free(victim);
 }
 

src/or/connection.c

@@ -266,27 +266,33 @@ static void
 _connection_free(connection_t *conn)
 {
   void *mem;
+  size_t memlen;
   switch (conn->type) {
     case CONN_TYPE_OR:
       tor_assert(conn->magic == OR_CONNECTION_MAGIC);
       mem = TO_OR_CONN(conn);
+      memlen = sizeof(or_connection_t);
       break;
     case CONN_TYPE_AP:
     case CONN_TYPE_EXIT:
       tor_assert(conn->magic == EDGE_CONNECTION_MAGIC);
       mem = TO_EDGE_CONN(conn);
+      memlen = sizeof(edge_connection_t);
       break;
     case CONN_TYPE_DIR:
       tor_assert(conn->magic == DIR_CONNECTION_MAGIC);
       mem = TO_DIR_CONN(conn);
+      memlen = sizeof(dir_connection_t);
       break;
     case CONN_TYPE_CONTROL:
       tor_assert(conn->magic == CONTROL_CONNECTION_MAGIC);
       mem = TO_CONTROL_CONN(conn);
+      memlen = sizeof(control_connection_t);
       break;
     default:
       tor_assert(conn->magic == BASE_CONNECTION_MAGIC);
       mem = conn;
+      memlen = sizeof(connection_t);
       break;
   }
 
@@ -331,6 +337,7 @@ _connection_free(connection_t *conn)
   if (CONN_IS_EDGE(conn)) {
     edge_connection_t *edge_conn = TO_EDGE_CONN(conn);
     tor_free(edge_conn->chosen_exit_name);
+    memset(edge_conn->socks_request, 0xcc, sizeof(socks_request_t));
     tor_free(edge_conn->socks_request);
   }
   if (conn->type == CONN_TYPE_CONTROL) {
@@ -365,7 +372,7 @@ _connection_free(connection_t *conn)
     connection_or_remove_from_identity_map(TO_OR_CONN(conn));
   }
 
-  memset(conn, 0xAA, sizeof(connection_t)); /* poison memory */
+  memset(conn, 0xAA, memlen); /* poison memory */
   tor_free(mem);
 }
 

src/or/onion.c

@@ -166,7 +166,7 @@ onion_skin_create(crypto_pk_env_t *dest_router_key,
                   crypto_dh_env_t **handshake_state_out,
                   char *onion_skin_out) /* ONIONSKIN_CHALLENGE_LEN bytes */
 {
-  char *challenge = NULL;
+  char challenge[DH_KEY_LEN];
   crypto_dh_env_t *dh = NULL;
   int dhbytes, pkbytes;
 
@@ -183,7 +183,6 @@ onion_skin_create(crypto_pk_env_t *dest_router_key,
   pkbytes = crypto_pk_keysize(dest_router_key);
   tor_assert(dhbytes == 128);
   tor_assert(pkbytes == 128);
-  challenge = tor_malloc_zero(DH_KEY_LEN);
 
   if (crypto_dh_get_public(dh, challenge, dhbytes))
     goto err;
@@ -211,12 +210,12 @@ onion_skin_create(crypto_pk_env_t *dest_router_key,
                                       PK_PKCS1_OAEP_PADDING, 1)<0)
     goto err;
 
-  tor_free(challenge);
+  memset(challenge, 0, sizeof(challenge));
   *handshake_state_out = dh;
 
   return 0;
  err:
-  tor_free(challenge);
+  memset(challenge, 0, sizeof(challenge));
   if (dh) crypto_dh_free(dh);
   return -1;
 }
@@ -238,6 +237,7 @@ onion_skin_server_handshake(const char *onion_skin, /*ONIONSKIN_CHALLENGE_LEN*/
   crypto_dh_env_t *dh = NULL;
   int len;
   char *key_material=NULL;
+  size_t key_material_len=0;
   int i;
   crypto_pk_env_t *k;
 
@@ -277,9 +277,10 @@ onion_skin_server_handshake(const char *onion_skin, /*ONIONSKIN_CHALLENGE_LEN*/
   puts("");
 #endif
 
-  key_material = tor_malloc(DIGEST_LEN+key_out_len);
+  key_material_len = DIGEST_LEN+key_out_len;
+  key_material = tor_malloc(key_material_len);
   len = crypto_dh_compute_secret(dh, challenge, DH_KEY_LEN,
-                                 key_material, DIGEST_LEN+key_out_len);
+                                 key_material, key_material_len);
   if (len < 0) {
     log_info(LD_GENERAL, "crypto_dh_compute_secret failed.");
     goto err;
@@ -300,11 +301,17 @@ onion_skin_server_handshake(const char *onion_skin, /*ONIONSKIN_CHALLENGE_LEN*/
   puts("");
 #endif
 
+  memset(challenge, 0, sizeof(challenge));
+  memset(key_material, 0, key_material_len);
   tor_free(key_material);
   crypto_dh_free(dh);
   return 0;
  err:
-  tor_free(key_material);
+  memset(challenge, 0, sizeof(challenge));
+  if (key_material) {
+    memset(key_material, 0, key_material_len);
+    tor_free(key_material);
+  }
   if (dh) crypto_dh_free(dh);
 
   return -1;
@@ -327,6 +334,7 @@ onion_skin_client_handshake(crypto_dh_env_t *handshake_state,
 {
   int len;
   char *key_material=NULL;
+  size_t key_material_len;
   tor_assert(crypto_dh_get_bytes(handshake_state) == DH_KEY_LEN);
 
 #ifdef DEBUG_ONION_SKINS
@@ -337,13 +345,14 @@ onion_skin_client_handshake(crypto_dh_env_t *handshake_state,
   puts("");
 #endif
 
-  key_material = tor_malloc(20+key_out_len);
+  key_material_len = DIGEST_LEN + key_out_len;
+  key_material = tor_malloc(key_material_len);
   len = crypto_dh_compute_secret(handshake_state, handshake_reply, DH_KEY_LEN,
-                                 key_material, 20+key_out_len);
+                                 key_material, key_material_len);
   if (len < 0)
     goto err;
 
-  if (memcmp(key_material, handshake_reply+DH_KEY_LEN, 20)) {
+  if (memcmp(key_material, handshake_reply+DH_KEY_LEN, DIGEST_LEN)) {
     /* H(K) does *not* match. Something fishy. */
     log_warn(LD_PROTOCOL,"Digest DOES NOT MATCH on onion handshake. "
              "Bug or attack.");
@@ -351,7 +360,7 @@ onion_skin_client_handshake(crypto_dh_env_t *handshake_state,
   }
 
   /* use the rest of the key material for our shared keys, digests, etc */
-  memcpy(key_out, key_material+20, key_out_len);
+  memcpy(key_out, key_material+DIGEST_LEN, key_out_len);
 
 #ifdef DEBUG_ONION_SKINS
   printf("Client: keys out:");
@@ -359,9 +368,11 @@ onion_skin_client_handshake(crypto_dh_env_t *handshake_state,
   puts("");
 #endif
 
+  memset(key_material, 0, key_material_len);
   tor_free(key_material);
   return 0;
  err:
+  memset(key_material, 0, key_material_len);
   tor_free(key_material);
   return -1;
 }
@@ -380,8 +391,9 @@ fast_server_handshake(const char *key_in, /* DIGEST_LEN bytes */
                       size_t key_out_len)
 {
   char tmp[DIGEST_LEN+DIGEST_LEN];
-  char *out;
+  char *out = NULL;
   size_t out_len;
+  int r = -1;
 
   if (crypto_rand(handshake_reply_out, DIGEST_LEN)<0)
     return -1;
@@ -391,15 +403,16 @@ fast_server_handshake(const char *key_in, /* DIGEST_LEN bytes */
   out_len = key_out_len+DIGEST_LEN;
   out = tor_malloc(out_len);
   if (crypto_expand_key_material(tmp, sizeof(tmp), out, out_len)) {
-    tor_free(out);
-    return -1;
+    goto done;
   }
   memcpy(handshake_reply_out+DIGEST_LEN, out, DIGEST_LEN);
   memcpy(key_out, out+DIGEST_LEN, key_out_len);
+  r = 0;
+ done:
   memset(tmp, 0, sizeof(tmp));
   memset(out, 0, out_len);
   tor_free(out);
-  return 0;
+  return r;
 }
 
 /** Implement the second half of the client side of the CREATE_FAST handshake.
@@ -423,27 +436,28 @@ fast_client_handshake(const char *handshake_state, /* DIGEST_LEN bytes */
   char tmp[DIGEST_LEN+DIGEST_LEN];
   char *out;
   size_t out_len;
+  int r;
 
   memcpy(tmp, handshake_state, DIGEST_LEN);
   memcpy(tmp+DIGEST_LEN, handshake_reply_out, DIGEST_LEN);
   out_len = key_out_len+DIGEST_LEN;
   out = tor_malloc(out_len);
   if (crypto_expand_key_material(tmp, sizeof(tmp), out, out_len)) {
-    tor_free(out);
-    return -1;
+    goto done;
   }
   if (memcmp(out, handshake_reply_out+DIGEST_LEN, DIGEST_LEN)) {
     /* H(K) does *not* match. Something fishy. */
     log_warn(LD_PROTOCOL,"Digest DOES NOT MATCH on fast handshake. "
              "Bug or attack.");
-    tor_free(out);
-    return -1;
+    goto done;
   }
   memcpy(key_out, out+DIGEST_LEN, key_out_len);
+  r = 0;
+ done:
   memset(tmp, 0, sizeof(tmp));
   memset(out, 0, out_len);
   tor_free(out);
-  return 0;
+  return r;
 }
 
 /** Remove all circuits from the pending list.  Called from tor_free_all. */

src/or/relay.c

@@ -117,19 +117,14 @@ static int
 relay_crypt_one_payload(crypto_cipher_env_t *cipher, char *in,
                         int encrypt_mode)
 {
-  char out[CELL_PAYLOAD_SIZE]; /* 'in' must be this size too */
   int r;
-
-  if (encrypt_mode)
-    r = crypto_cipher_encrypt(cipher, out, in, CELL_PAYLOAD_SIZE);
-  else
-    r = crypto_cipher_decrypt(cipher, out, in, CELL_PAYLOAD_SIZE);
+  (void)encrypt_mode;
+  r = crypto_cipher_crypt_inplace(cipher, in, CELL_PAYLOAD_SIZE);
 
   if (r) {
     log_warn(LD_BUG,"Error during relay encryption");
     return -1;
   }
-  memcpy(in,out,CELL_PAYLOAD_SIZE);
   return 0;
 }