[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
[or-cvs] r13414: Be more thorough about memory poisoning and clearing. Add an (in tor/trunk: . src/common src/or)
Author: nickm
Date: 2008-02-07 11:10:33 -0500 (Thu, 07 Feb 2008)
New Revision: 13414
Modified:
tor/trunk/
tor/trunk/ChangeLog
tor/trunk/src/common/aes.c
tor/trunk/src/common/aes.h
tor/trunk/src/common/crypto.c
tor/trunk/src/common/crypto.h
tor/trunk/src/or/circuitlist.c
tor/trunk/src/or/connection.c
tor/trunk/src/or/onion.c
tor/trunk/src/or/relay.c
Log:
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.
Property changes on: tor/trunk
___________________________________________________________________
svk:merge ticket from /tor/trunk [r17963] on 8246c3cf-6607-4228-993b-4d95d33730f1
Modified: tor/trunk/ChangeLog
===================================================================
--- tor/trunk/ChangeLog 2008-02-07 08:06:20 UTC (rev 13413)
+++ tor/trunk/ChangeLog 2008-02-07 16:10:33 UTC (rev 13414)
@@ -13,6 +13,10 @@
- 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 @@
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
Modified: tor/trunk/src/common/aes.c
===================================================================
--- tor/trunk/src/common/aes.c 2008-02-07 08:06:20 UTC (rev 13413)
+++ tor/trunk/src/common/aes.c 2008-02-07 16:10:33 UTC (rev 13414)
@@ -289,7 +289,7 @@
* 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 @@
}
}
+/** 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
Modified: tor/trunk/src/common/aes.h
===================================================================
--- tor/trunk/src/common/aes.h 2008-02-07 08:06:20 UTC (rev 13413)
+++ tor/trunk/src/common/aes.h 2008-02-07 16:10:33 UTC (rev 13414)
@@ -25,6 +25,7 @@
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
Modified: tor/trunk/src/common/crypto.c
===================================================================
--- tor/trunk/src/common/crypto.c 2008-02-07 08:06:20 UTC (rev 13413)
+++ tor/trunk/src/common/crypto.c 2008-02-07 16:10:33 UTC (rev 13414)
@@ -380,6 +380,7 @@
tor_assert(env->cipher);
aes_free_cipher(env->cipher);
+ memset(env, 0, sizeof(crypto_cipher_env_t));
tor_free(env);
}
@@ -775,10 +776,13 @@
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 @@
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 @@
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 @@
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 @@
}
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 @@
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 @@
return -1;
}
RAND_seed(buf, sizeof(buf));
+ memset(buf, 0, sizeof(buf));
return 0;
}
@@ -1863,7 +1883,6 @@
ret += len;
return ret;
#else
- #define ACC32
const char *eos = src+srclen;
uint32_t n=0;
int n_idx=0;
@@ -2056,6 +2075,7 @@
}
}
+ memset(tmp, 0, srclen);
tor_free(tmp);
tmp = NULL;
return 0;
@@ -2099,6 +2119,7 @@
}
}
crypto_digest_get_digest(d, key_out, key_out_len);
+ memset(tmp, 0, 8+secret_len);
tor_free(tmp);
crypto_free_digest_env(d);
}
Modified: tor/trunk/src/common/crypto.h
===================================================================
--- tor/trunk/src/common/crypto.h 2008-02-07 08:06:20 UTC (rev 13413)
+++ tor/trunk/src/common/crypto.h 2008-02-07 16:10:33 UTC (rev 13414)
@@ -128,6 +128,7 @@
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,
Modified: tor/trunk/src/or/circuitlist.c
===================================================================
--- tor/trunk/src/or/circuitlist.c 2008-02-07 08:06:20 UTC (rev 13413)
+++ tor/trunk/src/or/circuitlist.c 2008-02-07 16:10:33 UTC (rev 13414)
@@ -378,10 +378,12 @@
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 @@
} 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 @@
* "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 @@
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);
}
Modified: tor/trunk/src/or/connection.c
===================================================================
--- tor/trunk/src/or/connection.c 2008-02-07 08:06:20 UTC (rev 13413)
+++ tor/trunk/src/or/connection.c 2008-02-07 16:10:33 UTC (rev 13414)
@@ -266,27 +266,33 @@
_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 @@
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_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);
}
Modified: tor/trunk/src/or/onion.c
===================================================================
--- tor/trunk/src/or/onion.c 2008-02-07 08:06:20 UTC (rev 13413)
+++ tor/trunk/src/or/onion.c 2008-02-07 16:10:33 UTC (rev 13414)
@@ -166,7 +166,7 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
{
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 @@
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 @@
}
/* 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 @@
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 @@
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 @@
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 @@
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. */
Modified: tor/trunk/src/or/relay.c
===================================================================
--- tor/trunk/src/or/relay.c 2008-02-07 08:06:20 UTC (rev 13413)
+++ tor/trunk/src/or/relay.c 2008-02-07 16:10:33 UTC (rev 13414)
@@ -117,19 +117,14 @@
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;
+ (void)encrypt_mode;
+ r = crypto_cipher_crypt_inplace(cipher, in, CELL_PAYLOAD_SIZE);
- if (encrypt_mode)
- r = crypto_cipher_encrypt(cipher, out, in, CELL_PAYLOAD_SIZE);
- else
- r = crypto_cipher_decrypt(cipher, out, in, CELL_PAYLOAD_SIZE);
-
if (r) {
log_warn(LD_BUG,"Error during relay encryption");
return -1;
}
- memcpy(in,out,CELL_PAYLOAD_SIZE);
return 0;
}