From eecc44dab8ad98246b2c4dbedf977113f1874f77 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 7 Feb 2008 16:10:33 +0000 Subject: [PATCH] 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 | 6 +++++ src/common/aes.c | 38 ++++++++++++++++++++++++++++++- src/common/aes.h | 1 + src/common/crypto.c | 25 ++++++++++++++++++-- src/common/crypto.h | 1 + src/or/circuitlist.c | 7 ++++-- src/or/connection.c | 9 +++++++- src/or/onion.c | 54 ++++++++++++++++++++++++++++---------------- src/or/relay.c | 9 ++------ 9 files changed, 117 insertions(+), 33 deletions(-) diff --git a/ChangeLog b/ChangeLog index 73d91cb917..9db8a9a231 100644 --- a/ChangeLog +++ b/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 diff --git a/src/common/aes.c b/src/common/aes.c index 6dc4896988..cd8317dd11 100644 --- a/src/common/aes.c +++ b/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 len bytes from input, storing the results in place. + * Uses the key in cipher, and advances the counter by len 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 cipher to the 16-bit big-endian value * in iv. */ void diff --git a/src/common/aes.h b/src/common/aes.h index b4b90187cf..e03fae6e87 100644 --- a/src/common/aes.h +++ b/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 diff --git a/src/common/crypto.c b/src/common/crypto.c index 73367d691c..ea68e3cfdf 100644 --- a/src/common/crypto.c +++ b/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 fromlen @@ -1156,6 +1160,16 @@ crypto_cipher_decrypt(crypto_cipher_env_t *env, char *to, return 0; } +/** Encrypt len bytes on from using the cipher in env; + * 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 fromlen bytes (at least 1) from from with the key in * cipher to the buffer in to of length * tolen. tolen must be at least fromlen 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); } diff --git a/src/common/crypto.h b/src/common/crypto.h index 45debcb40c..e02e3df323 100644 --- a/src/common/crypto.h +++ b/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, diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 67770ffd5a..13dd97fb76 100644 --- a/src/or/circuitlist.c +++ b/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); } diff --git a/src/or/connection.c b/src/or/connection.c index bb42ecf936..2dc0a8ae8a 100644 --- a/src/or/connection.c +++ b/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); } diff --git a/src/or/onion.c b/src/or/onion.c index 281d011b9d..abbadcbf0c 100644 --- a/src/or/onion.c +++ b/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. */ diff --git a/src/or/relay.c b/src/or/relay.c index 31f264b637..576d6e2cf1 100644 --- a/src/or/relay.c +++ b/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; }