Merge remote-tracking branch 'origin/maint-0.2.3'

Conflicts:
	src/common/crypto.c
	src/or/rendservice.c

changes/bug7352

@@ -0,0 +1,12 @@
+  o Major bugfixes:
+    - Tor tries to wipe potentially sensitive data after using it, so
+      that if some subsequent security failure exposes Tor's memory,
+      the damage will be limited. But we had a bug where the compiler
+      was eliminating these wipe operations when it decided that the
+      memory was no longer visible to a (correctly running) program,
+      hence defeating our attempt at defense in depth. We fix that
+      by using OpenSSL's OPENSSL_cleanse() operation, which a compiler
+      is unlikely to optimize away. Future versions of Tor may use
+      a less ridiculously heavy approach for this. Fixes bug 7352.
+      Reported in an article by Andrey Karpov.
+

src/common/aes.c

@@ -106,7 +106,7 @@ aes_cipher_free(aes_cnt_cipher_t *cipher)
   if (!cipher)
     return;
   EVP_CIPHER_CTX_cleanup(&cipher->evp);
-  memset(cipher, 0, sizeof(aes_cnt_cipher_t));
+  memwipe(cipher, 0, sizeof(aes_cnt_cipher_t));
   tor_free(cipher);
 }
 void
@@ -373,7 +373,7 @@ aes_cipher_free(aes_cnt_cipher_t *cipher)
   if (cipher->using_evp) {
     EVP_CIPHER_CTX_cleanup(&cipher->key.evp);
   }
-  memset(cipher, 0, sizeof(aes_cnt_cipher_t));
+  memwipe(cipher, 0, sizeof(aes_cnt_cipher_t));
   tor_free(cipher);
 }
 

src/common/compat.c

@@ -332,7 +332,7 @@ tor_munmap_file(tor_mmap_t *handle)
 {
   char *d = (char*)handle->data;
   tor_free(d);
-  memset(handle, 0, sizeof(tor_mmap_t));
+  memwipe(handle, 0, sizeof(tor_mmap_t));
   tor_free(handle);
 }
 #endif

src/common/crypto.c

@@ -444,7 +444,7 @@ crypto_cipher_free(crypto_cipher_t *env)
 
   tor_assert(env->cipher);
   aes_cipher_free(env->cipher);
-  memset(env, 0, sizeof(crypto_cipher_t));
+  memwipe(env, 0, sizeof(crypto_cipher_t));
   tor_free(env);
 }
 
@@ -544,7 +544,7 @@ crypto_pk_read_private_key_from_filename(crypto_pk_t *env,
 
   /* Try to parse it. */
   r = crypto_pk_read_private_key_from_string(env, contents, -1);
-  memset(contents, 0, strlen(contents));
+  memwipe(contents, 0, strlen(contents));
   tor_free(contents);
   if (r)
     return -1; /* read_private_key_from_string already warned, so we don't.*/
@@ -686,7 +686,7 @@ crypto_pk_write_private_key_to_filename(crypto_pk_t *env,
   s[len]='\0';
   r = write_str_to_file(fname, s, 0);
   BIO_free(bio);
-  memset(s, 0, strlen(s));
+  memwipe(s, 0, strlen(s));
   tor_free(s);
   return r;
 }
@@ -1012,7 +1012,7 @@ crypto_pk_private_sign_digest(crypto_pk_t *env, char *to, size_t tolen,
   if (crypto_digest(digest,from,fromlen)<0)
     return -1;
   r = crypto_pk_private_sign(env,to,tolen,digest,DIGEST_LEN);
-  memset(digest, 0, sizeof(digest));
+  memwipe(digest, 0, sizeof(digest));
   return r;
 }
 
@@ -1076,14 +1076,14 @@ crypto_pk_public_hybrid_encrypt(crypto_pk_t *env,
                             from+pkeylen-overhead-CIPHER_KEY_LEN, symlen);
 
   if (r<0) goto err;
-  memset(buf, 0, pkeylen);
+  memwipe(buf, 0, pkeylen);
   tor_free(buf);
   crypto_cipher_free(cipher);
   tor_assert(outlen+symlen < INT_MAX);
   return (int)(outlen + symlen);
  err:
 
-  memset(buf, 0, pkeylen);
+  memwipe(buf, 0, pkeylen);
   tor_free(buf);
   crypto_cipher_free(cipher);
   return -1;
@@ -1134,13 +1134,13 @@ crypto_pk_private_hybrid_decrypt(crypto_pk_t *env,
   r = crypto_cipher_decrypt(cipher, to+outlen, from+pkeylen, fromlen-pkeylen);
   if (r<0)
     goto err;
-  memset(buf,0,pkeylen);
+  memwipe(buf,0,pkeylen);
   tor_free(buf);
   crypto_cipher_free(cipher);
   tor_assert(outlen + fromlen < INT_MAX);
   return (int)(outlen + (fromlen-pkeylen));
  err:
-  memset(buf,0,pkeylen);
+  memwipe(buf,0,pkeylen);
   tor_free(buf);
   crypto_cipher_free(cipher);
   return -1;
@@ -1540,7 +1540,7 @@ crypto_digest_free(crypto_digest_t *digest)
 {
   if (!digest)
     return;
-  memset(digest, 0, sizeof(crypto_digest_t));
+  memwipe(digest, 0, sizeof(crypto_digest_t));
   tor_free(digest);
 }
 
@@ -1602,7 +1602,7 @@ crypto_digest_get_digest(crypto_digest_t *digest,
       break;
   }
   memcpy(out, r, out_len);
-  memset(r, 0, sizeof(r));
+  memwipe(r, 0, sizeof(r));
 }
 
 /** Allocate and return a new digest object with the same state as
@@ -2187,7 +2187,7 @@ crypto_dh_compute_secret(int severity, crypto_dh_t *dh,
   if (pubkey_bn)
     BN_free(pubkey_bn);
   if (secret_tmp) {
-    memset(secret_tmp, 0, secret_tmp_len);
+    memwipe(secret_tmp, 0, secret_tmp_len);
     tor_free(secret_tmp);
   }
   if (result < 0)
@@ -2222,15 +2222,15 @@ crypto_expand_key_material(const char *key_in, size_t key_in_len,
       goto err;
     memcpy(cp, digest, MIN(DIGEST_LEN, key_out_len-(cp-key_out)));
   }
-  memset(tmp, 0, key_in_len+1);
+  memwipe(tmp, 0, key_in_len+1);
   tor_free(tmp);
-  memset(digest, 0, sizeof(digest));
+  memwipe(digest, 0, sizeof(digest));
   return 0;
 
  err:
-  memset(tmp, 0, key_in_len+1);
+  memwipe(tmp, 0, key_in_len+1);
   tor_free(tmp);
-  memset(digest, 0, sizeof(digest));
+  memwipe(digest, 0, sizeof(digest));
   return -1;
 }
 
@@ -2320,7 +2320,7 @@ crypto_seed_rng(int startup)
     return rand_poll_status ? 0 : -1;
   }
   RAND_seed(buf, sizeof(buf));
-  memset(buf, 0, sizeof(buf));
+  memwipe(buf, 0, sizeof(buf));
   seed_weak_rng();
   return 0;
 #else
@@ -2337,7 +2337,7 @@ crypto_seed_rng(int startup)
       return -1;
     }
     RAND_seed(buf, (int)sizeof(buf));
-    memset(buf, 0, sizeof(buf));
+    memwipe(buf, 0, sizeof(buf));
     seed_weak_rng();
     return 0;
   }
@@ -2820,7 +2820,7 @@ base32_decode(char *dest, size_t destlen, const char *src, size_t srclen)
     }
   }
 
-  memset(tmp, 0, srclen);
+  memwipe(tmp, 0, srclen);
   tor_free(tmp);
   tmp = NULL;
   return 0;
@@ -2865,11 +2865,54 @@ 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, tmplen);
+  memwipe(tmp, 0, tmplen);
   tor_free(tmp);
   crypto_digest_free(d);
 }
 
+/**
+ * Destroy the <b>sz</b> bytes of data stored at <b>mem</b>, setting them to
+ * the value <b>byte</b>.
+ *
+ * This function is preferable to memset, since many compilers will happily
+ * optimize out memset() when they can convince themselves that the data being
+ * cleared will never be read.
+ *
+ * Right now, our convention is to use this function when we are wiping data
+ * that's about to become inaccessible, such as stack buffers that are about
+ * to go out of scope or structures that are about to get freed.  (In
+ * practice, it appears that the compilers we're currently using will optimize
+ * out the memset()s for stack-allocated buffers, but not those for
+ * about-to-be-freed structures. That could change, though, so we're being
+ * wary.)  If there are live reads for the data, then you can just use
+ * memset().
+ */
+void
+memwipe(void *mem, uint8_t byte, size_t sz)
+{
+  /* Because whole-program-optimization exists, we may not be able to just
+   * have this function call "memset".  A smart compiler could inline it, then
+   * eliminate dead memsets, and declare itself to be clever. */
+
+  /* This is a slow and ugly function from OpenSSL that fills 'mem' with junk
+   * based on the pointer value, then uses that junk to update a global
+   * variable.  It's an elaborate ruse to trick the compiler into not
+   * optimizing out the "wipe this memory" code.  Read it if you like zany
+   * programming tricks! In later versions of Tor, we should look for better
+   * not-optimized-out memory wiping stuff. */
+  OPENSSL_cleanse(mem, sz);
+  /* Just in case some caller of memwipe() is relying on getting a buffer
+   * filled with a particular value, fill the buffer.
+   *
+   * If this function gets inlined, this memset might get eliminated, but
+   * that's okay: We only care about this particular memset in the case where
+   * the caller should have been using memset(), and the memset() wouldn't get
+   * eliminated.  In other words, this is here so that we won't break anything
+   * if somebody accidentally calls memwipe() instead of memset().
+   **/
+  memset(mem, byte, sz);
+}
+
 #ifdef TOR_IS_MULTITHREADED
 /** Helper: OpenSSL uses this callback to manipulate mutexes. */
 static void

src/common/crypto.h

@@ -273,6 +273,9 @@ int digest256_from_base64(char *digest, const char *d64);
 void secret_to_key(char *key_out, size_t key_out_len, const char *secret,
                    size_t secret_len, const char *s2k_specifier);
 
+/** OpenSSL-based utility functions. */
+void memwipe(void *mem, uint8_t byte, size_t sz);
+
 #ifdef CRYPTO_PRIVATE
 /* Prototypes for private functions only used by tortls.c, crypto.c, and the
  * unit tests. */

src/common/mempool.c

@@ -8,6 +8,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include "torint.h"
+#include "crypto.h"
 #define MEMPOOL_PRIVATE
 #include "mempool.h"
 
@@ -509,7 +510,7 @@ mp_pool_destroy(mp_pool_t *pool)
   destroy_chunks(pool->empty_chunks);
   destroy_chunks(pool->used_chunks);
   destroy_chunks(pool->full_chunks);
-  memset(pool, 0xe0, sizeof(mp_pool_t));
+  memwipe(pool, 0xe0, sizeof(mp_pool_t));
   FREE(pool);
 }
 

src/common/tortls.c

@@ -713,7 +713,7 @@ tor_cert_free(tor_cert_t *cert)
   if (cert->cert)
     X509_free(cert->cert);
   tor_free(cert->encoded);
-  memset(cert, 0x03, sizeof(*cert));
+  memwipe(cert, 0x03, sizeof(*cert));
   tor_free(cert);
 }
 
@@ -2450,7 +2450,7 @@ tor_tls_get_tlssecrets(tor_tls_t *tls, uint8_t *secrets_out)
                      (char*)tls->ssl->session->master_key,
                      tls->ssl->session->master_key_length,
                      buf, len);
-  memset(buf, 0, sizeof(buf));
+  memwipe(buf, 0, sizeof(buf));
   return 0;
 }
 

src/common/util.c

@@ -4014,7 +4014,7 @@ tor_process_handle_destroy(process_handle_t *process_handle,
     fclose(process_handle->stderr_handle);
 #endif
 
-  memset(process_handle, 0x0f, sizeof(process_handle_t));
+  memwipe(process_handle, 0x0f, sizeof(process_handle_t));
   tor_free(process_handle);
 }
 

src/or/buffers.c

@@ -1544,14 +1544,14 @@ socks_request_free(socks_request_t *req)
   if (!req)
     return;
   if (req->username) {
-    memset(req->username, 0x10, req->usernamelen);
+    memwipe(req->username, 0x10, req->usernamelen);
     tor_free(req->username);
   }
   if (req->password) {
-    memset(req->password, 0x04, req->passwordlen);
+    memwipe(req->password, 0x04, req->passwordlen);
     tor_free(req->password);
   }
-  memset(req, 0xCC, sizeof(socks_request_t));
+  memwipe(req, 0xCC, sizeof(socks_request_t));
   tor_free(req);
 }
 

src/or/circuitlist.c

@@ -637,11 +637,11 @@ circuit_free(circuit_t *circ)
 
     tor_free(ocirc->dest_address);
     if (ocirc->socks_username) {
-      memset(ocirc->socks_username, 0x12, ocirc->socks_username_len);
+      memwipe(ocirc->socks_username, 0x12, ocirc->socks_username_len);
       tor_free(ocirc->socks_username);
     }
     if (ocirc->socks_password) {
-      memset(ocirc->socks_password, 0x06, ocirc->socks_password_len);
+      memwipe(ocirc->socks_password, 0x06, ocirc->socks_password_len);
       tor_free(ocirc->socks_password);
     }
   } else {
@@ -682,7 +682,7 @@ circuit_free(circuit_t *circ)
    * "active" checks will be violated. */
   cell_queue_clear(&circ->n_chan_cells);
 
-  memset(mem, 0xAA, memlen); /* poison memory */
+  memwipe(mem, 0xAA, memlen); /* poison memory */
   tor_free(mem);
 }
 
@@ -746,7 +746,7 @@ circuit_free_cpath_node(crypt_path_t *victim)
   crypto_dh_free(victim->dh_handshake_state);
   extend_info_free(victim->extend_info);
 
-  memset(victim, 0xBB, sizeof(crypt_path_t)); /* poison memory */
+  memwipe(victim, 0xBB, sizeof(crypt_path_t)); /* poison memory */
   tor_free(victim);
 }
 

src/or/connection.c

@@ -574,7 +574,7 @@ connection_free_(connection_t *conn)
   }
 #endif
 
-  memset(mem, 0xCC, memlen); /* poison memory */
+  memwipe(mem, 0xCC, memlen); /* poison memory */
   tor_free(mem);
 }
 

src/or/connection_edge.c

@@ -3716,11 +3716,11 @@ circuit_clear_isolation(origin_circuit_t *circ)
   circ->session_group = -1;
   circ->nym_epoch = 0;
   if (circ->socks_username) {
-    memset(circ->socks_username, 0x11, circ->socks_username_len);
+    memwipe(circ->socks_username, 0x11, circ->socks_username_len);
     tor_free(circ->socks_username);
   }
   if (circ->socks_password) {
-    memset(circ->socks_password, 0x05, circ->socks_password_len);
+    memwipe(circ->socks_password, 0x05, circ->socks_password_len);
     tor_free(circ->socks_password);
   }
   circ->socks_username_len = circ->socks_password_len = 0;

src/or/connection_or.c

@@ -1719,7 +1719,7 @@ or_handshake_state_free(or_handshake_state_t *state)
   crypto_digest_free(state->digest_received);
   tor_cert_free(state->auth_cert);
   tor_cert_free(state->id_cert);
-  memset(state, 0xBE, sizeof(or_handshake_state_t));
+  memwipe(state, 0xBE, sizeof(or_handshake_state_t));
   tor_free(state);
 }
 
@@ -1760,7 +1760,7 @@ or_handshake_state_record_cell(or_handshake_state_t *state,
      this very often at all. */
   cell_pack(&packed, cell);
   crypto_digest_add_bytes(d, packed.body, sizeof(packed.body));
-  memset(&packed, 0, sizeof(packed));
+  memwipe(&packed, 0, sizeof(packed));
 }
 
 /** Remember that a variable-length <b>cell</b> has been transmitted (if
@@ -1795,7 +1795,7 @@ or_handshake_state_record_var_cell(or_handshake_state_t *state,
   crypto_digest_add_bytes(d, buf, sizeof(buf));
   crypto_digest_add_bytes(d, (const char *)cell->payload, cell->payload_len);
 
-  memset(buf, 0, sizeof(buf));
+  memwipe(buf, 0, sizeof(buf));
 }
 
 /** Set <b>conn</b>'s state to OR_CONN_STATE_OPEN, and tell other subsystems
@@ -2125,7 +2125,7 @@ connection_or_send_auth_challenge_cell(or_connection_t *conn)
 
   connection_or_write_var_cell_to_buf(cell, conn);
   var_cell_free(cell);
-  memset(challenge, 0, sizeof(challenge));
+  memwipe(challenge, 0, sizeof(challenge));
 
   return 0;
 }

src/or/networkstatus.c

@@ -417,7 +417,7 @@ networkstatus_vote_free(networkstatus_t *ns)
 
   digestmap_free(ns->desc_digest_map, NULL);
 
-  memset(ns, 11, sizeof(*ns));
+  memwipe(ns, 11, sizeof(*ns));
   tor_free(ns);
 }
 

src/or/onion.c

@@ -206,12 +206,12 @@ onion_skin_create(crypto_pk_t *dest_router_key,
                                       PK_PKCS1_OAEP_PADDING, 1)<0)
     goto err;
 
-  memset(challenge, 0, sizeof(challenge));
+  memwipe(challenge, 0, sizeof(challenge));
   *handshake_state_out = dh;
 
   return 0;
  err:
-  memset(challenge, 0, sizeof(challenge));
+  memwipe(challenge, 0, sizeof(challenge));
   if (dh) crypto_dh_free(dh);
   return -1;
 }
@@ -286,15 +286,15 @@ onion_skin_server_handshake(const char *onion_skin, /*ONIONSKIN_CHALLENGE_LEN*/
   /* use the rest of the key material for our shared keys, digests, etc */
   memcpy(key_out, key_material+DIGEST_LEN, key_out_len);
 
-  memset(challenge, 0, sizeof(challenge));
-  memset(key_material, 0, key_material_len);
+  memwipe(challenge, 0, sizeof(challenge));
+  memwipe(key_material, 0, key_material_len);
   tor_free(key_material);
   crypto_dh_free(dh);
   return 0;
  err:
-  memset(challenge, 0, sizeof(challenge));
+  memwipe(challenge, 0, sizeof(challenge));
   if (key_material) {
-    memset(key_material, 0, key_material_len);
+    memwipe(key_material, 0, key_material_len);
     tor_free(key_material);
   }
   if (dh) crypto_dh_free(dh);
@@ -340,11 +340,11 @@ onion_skin_client_handshake(crypto_dh_t *handshake_state,
   /* use the rest of the key material for our shared keys, digests, etc */
   memcpy(key_out, key_material+DIGEST_LEN, key_out_len);
 
-  memset(key_material, 0, key_material_len);
+  memwipe(key_material, 0, key_material_len);
   tor_free(key_material);
   return 0;
  err:
-  memset(key_material, 0, key_material_len);
+  memwipe(key_material, 0, key_material_len);
   tor_free(key_material);
   return -1;
 }
@@ -381,8 +381,8 @@ fast_server_handshake(const uint8_t *key_in, /* DIGEST_LEN bytes */
   memcpy(key_out, out+DIGEST_LEN, key_out_len);
   r = 0;
  done:
-  memset(tmp, 0, sizeof(tmp));
-  memset(out, 0, out_len);
+  memwipe(tmp, 0, sizeof(tmp));
+  memwipe(out, 0, out_len);
   tor_free(out);
   return r;
 }
@@ -426,8 +426,8 @@ fast_client_handshake(const uint8_t *handshake_state,/*DIGEST_LEN bytes*/
   memcpy(key_out, out+DIGEST_LEN, key_out_len);
   r = 0;
  done:
-  memset(tmp, 0, sizeof(tmp));
-  memset(out, 0, out_len);
+  memwipe(tmp, 0, sizeof(tmp));
+  memwipe(out, 0, out_len);
   tor_free(out);
   return r;
 }

src/or/rendclient.c

@@ -936,10 +936,10 @@ rend_client_receive_rendezvous(origin_circuit_t *circ, const uint8_t *request,
 
   circuit_try_attaching_streams(circ);
 
-  memset(keys, 0, sizeof(keys));
+  memwipe(keys, 0, sizeof(keys));
   return 0;
  err:
-  memset(keys, 0, sizeof(keys));
+  memwipe(keys, 0, sizeof(keys));
   circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_TORPROTOCOL);
   return -1;
 }

src/or/rendservice.c

@@ -1406,13 +1406,13 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request,
   tor_free(err_msg);
 
  done:
-  memset(keys, 0, sizeof(keys));
-  memset(buf, 0, sizeof(buf));
-  memset(serviceid, 0, sizeof(serviceid));
-  memset(hexcookie, 0, sizeof(hexcookie));
-  memset(intro_key_digest, 0, sizeof(intro_key_digest));
-  memset(auth_data, 0, sizeof(auth_data));
-  memset(diffie_hellman_hash, 0, sizeof(diffie_hellman_hash));
+  memwipe(keys, 0, sizeof(keys));
+  memwipe(buf, 0, sizeof(buf));
+  memwipe(serviceid, 0, sizeof(serviceid));
+  memwipe(hexcookie, 0, sizeof(hexcookie));
+  memwipe(intro_key_digest, 0, sizeof(intro_key_digest));
+  memwipe(auth_data, 0, sizeof(auth_data));
+  memwipe(diffie_hellman_hash, 0, sizeof(diffie_hellman_hash));
 
   /* Free the parsed cell */
   if (parsed_req) {

src/or/routerparse.c

@@ -4637,7 +4637,7 @@ tor_version_parse(const char *s, tor_version_t *out)
     if (close_paren-cp > HEX_DIGEST_LEN)
       return -1;
     hexlen = (int)(close_paren-cp);
-    memset(digest, 0, sizeof(digest));
+    memwipe(digest, 0, sizeof(digest));
     if ( hexlen == 0 || (hexlen % 2) == 1)
       return -1;
     if (base16_decode(digest, hexlen/2, cp, hexlen))

src/tools/tor-gencert.c

@@ -105,7 +105,7 @@ load_passphrase(void)
   cp = memchr(buf, '\n', n);
   passphrase_len = cp-buf;
   passphrase = tor_strndup(buf, passphrase_len);
-  memset(buf, 0, sizeof(buf));
+  memwipe(buf, 0, sizeof(buf));
   return 0;
 }
 
@@ -113,7 +113,7 @@ static void
 clear_passphrase(void)
 {
   if (passphrase) {
-    memset(passphrase, 0, passphrase_len);
+    memwipe(passphrase, 0, passphrase_len);
     tor_free(passphrase);
   }
 }
@@ -191,7 +191,7 @@ parse_commandline(int argc, char **argv)
     }
   }
 
-  memset(&s, 0, sizeof(s));
+  memwipe(&s, 0, sizeof(s));
   if (verbose)
     set_log_severity_config(LOG_DEBUG, LOG_ERR, &s);
   else