[tor-commits] [tor/master] Extract the cryptographic parts of crypt_path_t and or_circuit_t.

commit 5ecad6c95d5e8e9a0abe5c86b5f8f066cc7a8f1c Author: Nick Mathewson nickm@torproject.org Date: Sat Mar 17 10:59:15 2018 -0400

Extract the cryptographic parts of crypt_path_t and or_circuit_t.

Additionally, this change extracts the functions that created and freed these elements.

These structures had common "forward&reverse stream&digest" elements, but they were initialized and freed through cpath objects, and different parts of the code depended on them. Now all that code is extacted, and kept in relay_crypto.c --- src/or/circuitbuild.c | 77 ++----------------------- src/or/circuitlist.c | 19 ++----- src/or/or.h | 36 ++++++------ src/or/relay_crypto.c | 136 ++++++++++++++++++++++++++++++++++++++++++--- src/or/relay_crypto.h | 8 +++ src/test/bench.c | 13 ++--- src/test/test_hs_client.c | 16 +++--- src/test/test_hs_service.c | 8 +-- 8 files changed, 177 insertions(+), 136 deletions(-)

diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index b3b543348..82c3699c4 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -56,6 +56,7 @@ #include "onion_fast.h" #include "policies.h" #include "relay.h" +#include "relay_crypto.h" #include "rendcommon.h" #include "rephist.h" #include "router.h" @@ -1336,69 +1337,10 @@ circuit_init_cpath_crypto(crypt_path_t *cpath, const char *key_data, size_t key_data_len, int reverse, int is_hs_v3) { - crypto_digest_t *tmp_digest; - crypto_cipher_t *tmp_crypto; - size_t digest_len = 0; - size_t cipher_key_len = 0;

tor_assert(cpath); - tor_assert(key_data); - tor_assert(!(cpath->f_crypto || cpath->b_crypto || - cpath->f_digest || cpath->b_digest)); - - /* Basic key size validation */ - if (is_hs_v3 && BUG(key_data_len != HS_NTOR_KEY_EXPANSION_KDF_OUT_LEN)) { - return -1; - } else if (!is_hs_v3 && BUG(key_data_len != CPATH_KEY_MATERIAL_LEN)) { - return -1; - } - - /* If we are using this cpath for next gen onion services use SHA3-256, - otherwise use good ol' SHA1 */ - if (is_hs_v3) { - digest_len = DIGEST256_LEN; - cipher_key_len = CIPHER256_KEY_LEN; - cpath->f_digest = crypto_digest256_new(DIGEST_SHA3_256); - cpath->b_digest = crypto_digest256_new(DIGEST_SHA3_256); - } else { - digest_len = DIGEST_LEN; - cipher_key_len = CIPHER_KEY_LEN; - cpath->f_digest = crypto_digest_new(); - cpath->b_digest = crypto_digest_new(); - } - - tor_assert(digest_len != 0); - tor_assert(cipher_key_len != 0); - const int cipher_key_bits = (int) cipher_key_len * 8; - - crypto_digest_add_bytes(cpath->f_digest, key_data, digest_len); - crypto_digest_add_bytes(cpath->b_digest, key_data+digest_len, digest_len); - - cpath->f_crypto = crypto_cipher_new_with_bits(key_data+(2*digest_len), - cipher_key_bits); - if (!cpath->f_crypto) { - log_warn(LD_BUG,"Forward cipher initialization failed."); - return -1; - } - - cpath->b_crypto = crypto_cipher_new_with_bits( - key_data+(2*digest_len)+cipher_key_len, - cipher_key_bits); - if (!cpath->b_crypto) { - log_warn(LD_BUG,"Backward cipher initialization failed."); - return -1; - } - - if (reverse) { - tmp_digest = cpath->f_digest; - cpath->f_digest = cpath->b_digest; - cpath->b_digest = tmp_digest; - tmp_crypto = cpath->f_crypto; - cpath->f_crypto = cpath->b_crypto; - cpath->b_crypto = tmp_crypto; - } - - return 0; + return relay_crypto_init(&cpath->crypto, key_data, key_data_len, reverse, + is_hs_v3); }

/** A "created" cell <b>reply</b> came back to us on circuit <b>circ</b>. @@ -1521,7 +1463,6 @@ onionskin_answer(or_circuit_t *circ, const uint8_t *rend_circ_nonce) { cell_t cell; - crypt_path_t *tmp_cpath;

tor_assert(keys_len == CPATH_KEY_MATERIAL_LEN);

@@ -1532,25 +1473,15 @@ onionskin_answer(or_circuit_t *circ, } cell.circ_id = circ->p_circ_id;

- tmp_cpath = tor_malloc_zero(sizeof(crypt_path_t)); - tmp_cpath->magic = CRYPT_PATH_MAGIC; - circuit_set_state(TO_CIRCUIT(circ), CIRCUIT_STATE_OPEN);

log_debug(LD_CIRC,"init digest forward 0x%.8x, backward 0x%.8x.", (unsigned int)get_uint32(keys), (unsigned int)get_uint32(keys+20)); - if (circuit_init_cpath_crypto(tmp_cpath, keys, keys_len, 0, 0)<0) { + if (relay_crypto_init(&circ->crypto, keys, keys_len, 0, 0)<0) { log_warn(LD_BUG,"Circuit initialization failed"); - tor_free(tmp_cpath); return -1; } - circ->n_digest = tmp_cpath->f_digest; - circ->n_crypto = tmp_cpath->f_crypto; - circ->p_digest = tmp_cpath->b_digest; - circ->p_crypto = tmp_cpath->b_crypto; - tmp_cpath->magic = 0; - tor_free(tmp_cpath);

memcpy(circ->rend_circ_nonce, rend_circ_nonce, DIGEST_LEN);

diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 29ad9a8ee..2373bfb84 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -76,6 +76,7 @@ #include "onion_fast.h" #include "policies.h" #include "relay.h" +#include "relay_crypto.h" #include "rendclient.h" #include "rendcommon.h" #include "rephist.h" @@ -1084,10 +1085,7 @@ circuit_free_(circuit_t *circ)

should_free = (ocirc->workqueue_entry == NULL);

- crypto_cipher_free(ocirc->p_crypto); - crypto_digest_free(ocirc->p_digest); - crypto_cipher_free(ocirc->n_crypto); - crypto_digest_free(ocirc->n_digest); + relay_crypto_clear(&ocirc->crypto);

if (ocirc->rend_splice) { or_circuit_t *other = ocirc->rend_splice; @@ -1227,10 +1225,7 @@ circuit_free_cpath_node(crypt_path_t *victim) if (!victim) return;

- crypto_cipher_free(victim->f_crypto); - crypto_cipher_free(victim->b_crypto); - crypto_digest_free(victim->f_digest); - crypto_digest_free(victim->b_digest); + relay_crypto_clear(&victim->crypto); onion_handshake_state_release(&victim->handshake_state); crypto_dh_free(victim->rend_dh_handshake_state); extend_info_free(victim->extend_info); @@ -2588,8 +2583,7 @@ assert_cpath_layer_ok(const crypt_path_t *cp) switch (cp->state) { case CPATH_STATE_OPEN: - tor_assert(cp->f_crypto); - tor_assert(cp->b_crypto); + relay_crypto_assert_ok(&cp->crypto); /* fall through */ case CPATH_STATE_CLOSED: /*XXXX Assert that there's no handshake_state either. */ @@ -2679,10 +2673,7 @@ assert_circuit_ok,(const circuit_t *c)) c->state == CIRCUIT_STATE_GUARD_WAIT) { tor_assert(!c->n_chan_create_cell); if (or_circ) { - tor_assert(or_circ->n_crypto); - tor_assert(or_circ->p_crypto); - tor_assert(or_circ->n_digest); - tor_assert(or_circ->p_digest); + relay_crypto_assert_ok(&or_circ->crypto); } } if (c->state == CIRCUIT_STATE_CHAN_WAIT && !c->marked_for_close) { diff --git a/src/or/or.h b/src/or/or.h index 045cdd9e1..3e61e1bef 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2899,11 +2899,7 @@ typedef struct { } u; } onion_handshake_state_t;

-/** Holds accounting information for a single step in the layered encryption - * performed by a circuit. Used only at the client edge of a circuit. */ -typedef struct crypt_path_t { - uint32_t magic; - +typedef struct relay_crypto_t { /* crypto environments */ /** Encryption key and counter for cells heading towards the OR at this * step. */ @@ -2917,6 +2913,17 @@ typedef struct crypt_path_t { /** Digest state for cells heading away from the OR at this step. */ crypto_digest_t *b_digest;

+} relay_crypto_t; + +/** Holds accounting information for a single step in the layered encryption + * performed by a circuit. Used only at the client edge of a circuit. */ +typedef struct crypt_path_t { + uint32_t magic; + + /** Cryptographic state used for encrypting and authenticating relay + * cells to and from this hop. */ + relay_crypto_t crypto; + /** Current state of the handshake as performed with the OR at this * step. */ onion_handshake_state_t handshake_state; @@ -3482,21 +3489,10 @@ typedef struct or_circuit_t { /** Linked list of Exit streams associated with this circuit that are * still being resolved. */ edge_connection_t *resolving_streams; - /** The cipher used by intermediate hops for cells heading toward the - * OP. */ - crypto_cipher_t *p_crypto; - /** The cipher used by intermediate hops for cells heading away from - * the OP. */ - crypto_cipher_t *n_crypto; - - /** The integrity-checking digest used by intermediate hops, for - * cells packaged here and heading towards the OP. - */ - crypto_digest_t *p_digest; - /** The integrity-checking digest used by intermediate hops, for - * cells packaged at the OP and arriving here. - */ - crypto_digest_t *n_digest; + + /** Cryptographic state used for encrypting and authenticating relay + * cells to and from this hop. */ + relay_crypto_t crypto;

/** Points to spliced circuit if purpose is REND_ESTABLISHED, and circuit * is not marked for close. */ diff --git a/src/or/relay_crypto.c b/src/or/relay_crypto.c index e0992a301..9d61ecd42 100644 --- a/src/or/relay_crypto.c +++ b/src/or/relay_crypto.c @@ -6,6 +6,7 @@

#include "or.h" #include "config.h" +#include "hs_ntor.h" // for HS_NTOR_KEY_EXPANSION_KDF_OUT_LEN #include "relay_crypto.h" #include "relay.h"

@@ -126,12 +127,12 @@ relay_decrypt_cell(circuit_t *circ, cell_t *cell, tor_assert(thishop);

/* decrypt one layer */ - relay_crypt_one_payload(thishop->b_crypto, cell->payload); + relay_crypt_one_payload(thishop->crypto.b_crypto, cell->payload);

relay_header_unpack(&rh, cell->payload); if (rh.recognized == 0) { /* it's possibly recognized. have to check digest to be sure. */ - if (relay_digest_matches(thishop->b_digest, cell)) { + if (relay_digest_matches(thishop->crypto.b_digest, cell)) { *recognized = 1; *layer_hint = thishop; return 0; @@ -144,18 +145,20 @@ relay_decrypt_cell(circuit_t *circ, cell_t *cell, "Incoming cell at client not recognized. Closing."); return -1; } else { + relay_crypto_t *crypto = &TO_OR_CIRCUIT(circ)->crypto; /* We're in the middle. Encrypt one layer. */ - relay_crypt_one_payload(TO_OR_CIRCUIT(circ)->p_crypto, cell->payload); + relay_crypt_one_payload(crypto->b_crypto, cell->payload); } } else /* cell_direction == CELL_DIRECTION_OUT */ { /* We're in the middle. Decrypt one layer. */ + relay_crypto_t *crypto = &TO_OR_CIRCUIT(circ)->crypto;

- relay_crypt_one_payload(TO_OR_CIRCUIT(circ)->n_crypto, cell->payload); + relay_crypt_one_payload(crypto->f_crypto, cell->payload);

relay_header_unpack(&rh, cell->payload); if (rh.recognized == 0) { /* it's possibly recognized. have to check digest to be sure. */ - if (relay_digest_matches(TO_OR_CIRCUIT(circ)->n_digest, cell)) { + if (relay_digest_matches(crypto->f_digest, cell)) { *recognized = 1; return 0; } @@ -174,14 +177,14 @@ relay_encrypt_cell_outbound(cell_t *cell, crypt_path_t *layer_hint) { crypt_path_t *thishop; /* counter for repeated crypts */ - relay_set_digest(layer_hint->f_digest, cell); + relay_set_digest(layer_hint->crypto.f_digest, cell);

thishop = layer_hint; /* moving from farthest to nearest hop */ do { tor_assert(thishop); log_debug(LD_OR,"encrypting a layer of the relay cell."); - relay_crypt_one_payload(thishop->f_crypto, cell->payload); + relay_crypt_one_payload(thishop->crypto.f_crypto, cell->payload);

thishop = thishop->prev; } while (thishop != circ->cpath->prev); @@ -195,8 +198,123 @@ void relay_encrypt_cell_inbound(cell_t *cell, or_circuit_t *or_circ) { - relay_set_digest(or_circ->p_digest, cell); + relay_set_digest(or_circ->crypto.b_digest, cell); /* encrypt one layer */ - relay_crypt_one_payload(or_circ->p_crypto, cell->payload); + relay_crypt_one_payload(or_circ->crypto.b_crypto, cell->payload); +} + +/** + * Release all storage held inside <b>crypto</b>, but do not free + * <b>crypto</b> itself: it lives inside another object. + */ +void +relay_crypto_clear(relay_crypto_t *crypto) +{ + if (BUG(!crypto)) + return; + crypto_cipher_free(crypto->f_crypto); + crypto_cipher_free(crypto->b_crypto); + crypto_digest_free(crypto->f_digest); + crypto_digest_free(crypto->b_digest); +} + +/** Initialize <b>crypto</b> from the key material in key_data. + * + * If <b>is_hs_v3</b> is set, this cpath will be used for next gen hidden + * service circuits and <b>key_data</b> must be at least + * HS_NTOR_KEY_EXPANSION_KDF_OUT_LEN bytes in length. + * + * If <b>is_hs_v3</b> is not set, key_data must contain CPATH_KEY_MATERIAL_LEN + * bytes, which are used as follows: + * - 20 to initialize f_digest + * - 20 to initialize b_digest + * - 16 to key f_crypto + * - 16 to key b_crypto + * + * (If 'reverse' is true, then f_XX and b_XX are swapped.) + * + * Return 0 if init was successful, else -1 if it failed. + */ +int +relay_crypto_init(relay_crypto_t *crypto, + const char *key_data, size_t key_data_len, + int reverse, int is_hs_v3) +{ + crypto_digest_t *tmp_digest; + crypto_cipher_t *tmp_crypto; + size_t digest_len = 0; + size_t cipher_key_len = 0; + + tor_assert(crypto); + tor_assert(key_data); + tor_assert(!(crypto->f_crypto || crypto->b_crypto || + crypto->f_digest || crypto->b_digest)); + + /* Basic key size validation */ + if (is_hs_v3 && BUG(key_data_len != HS_NTOR_KEY_EXPANSION_KDF_OUT_LEN)) { + goto err; + } else if (!is_hs_v3 && BUG(key_data_len != CPATH_KEY_MATERIAL_LEN)) { + goto err; + } + + /* If we are using this crypto for next gen onion services use SHA3-256, + otherwise use good ol' SHA1 */ + if (is_hs_v3) { + digest_len = DIGEST256_LEN; + cipher_key_len = CIPHER256_KEY_LEN; + crypto->f_digest = crypto_digest256_new(DIGEST_SHA3_256); + crypto->b_digest = crypto_digest256_new(DIGEST_SHA3_256); + } else { + digest_len = DIGEST_LEN; + cipher_key_len = CIPHER_KEY_LEN; + crypto->f_digest = crypto_digest_new(); + crypto->b_digest = crypto_digest_new(); + } + + tor_assert(digest_len != 0); + tor_assert(cipher_key_len != 0); + const int cipher_key_bits = (int) cipher_key_len * 8; + + crypto_digest_add_bytes(crypto->f_digest, key_data, digest_len); + crypto_digest_add_bytes(crypto->b_digest, key_data+digest_len, digest_len); + + crypto->f_crypto = crypto_cipher_new_with_bits(key_data+(2*digest_len), + cipher_key_bits); + if (!crypto->f_crypto) { + log_warn(LD_BUG,"Forward cipher initialization failed."); + goto err; + } + + crypto->b_crypto = crypto_cipher_new_with_bits( + key_data+(2*digest_len)+cipher_key_len, + cipher_key_bits); + if (!crypto->b_crypto) { + log_warn(LD_BUG,"Backward cipher initialization failed."); + goto err; + } + + if (reverse) { + tmp_digest = crypto->f_digest; + crypto->f_digest = crypto->b_digest; + crypto->b_digest = tmp_digest; + tmp_crypto = crypto->f_crypto; + crypto->f_crypto = crypto->b_crypto; + crypto->b_crypto = tmp_crypto; + } + + return 0; + err: + relay_crypto_clear(crypto); + return -1; +} + +/** Assert that <b>crypto</b> is valid and set. */ +void +relay_crypto_assert_ok(const relay_crypto_t *crypto) +{ + tor_assert(crypto->f_crypto); + tor_assert(crypto->b_crypto); + tor_assert(crypto->f_digest); + tor_assert(crypto->b_digest); }

diff --git a/src/or/relay_crypto.h b/src/or/relay_crypto.h index 8d0917941..66ae02cee 100644 --- a/src/or/relay_crypto.h +++ b/src/or/relay_crypto.h @@ -12,6 +12,10 @@ #ifndef TOR_RELAY_CRYPTO_H #define TOR_RELAY_CRYPTO_H

+int relay_crypto_init(relay_crypto_t *crypto, + const char *key_data, size_t key_data_len, + int reverse, int is_hs_v3); + int relay_decrypt_cell(circuit_t *circ, cell_t *cell, cell_direction_t cell_direction, crypt_path_t **layer_hint, char *recognized); @@ -19,5 +23,9 @@ void relay_encrypt_cell_outbound(cell_t *cell, origin_circuit_t *or_circ, crypt_path_t *layer_hint); void relay_encrypt_cell_inbound(cell_t *cell, or_circuit_t *or_circ);

+void relay_crypto_clear(relay_crypto_t *crypto); + +void relay_crypto_assert_ok(const relay_crypto_t *crypto); + #endif /* !defined(TOR_RELAY_CRYPTO_H) */

diff --git a/src/test/bench.c b/src/test/bench.c index 67e127311..c0d430e05 100644 --- a/src/test/bench.c +++ b/src/test/bench.c @@ -505,10 +505,10 @@ bench_cell_ops(void) char key1[CIPHER_KEY_LEN], key2[CIPHER_KEY_LEN]; crypto_rand(key1, sizeof(key1)); crypto_rand(key2, sizeof(key2)); - or_circ->p_crypto = crypto_cipher_new(key1); - or_circ->n_crypto = crypto_cipher_new(key2); - or_circ->p_digest = crypto_digest_new(); - or_circ->n_digest = crypto_digest_new(); + or_circ->crypto.f_crypto = crypto_cipher_new(key1); + or_circ->crypto.b_crypto = crypto_cipher_new(key2); + or_circ->crypto.f_digest = crypto_digest_new(); + or_circ->crypto.b_digest = crypto_digest_new();

reset_perftime();

@@ -528,10 +528,7 @@ bench_cell_ops(void) NANOCOUNT(start,end,iters*CELL_PAYLOAD_SIZE)); }

- crypto_digest_free(or_circ->p_digest); - crypto_digest_free(or_circ->n_digest); - crypto_cipher_free(or_circ->p_crypto); - crypto_cipher_free(or_circ->n_crypto); + relay_crypto_clear(&or_circ->crypto); tor_free(or_circ); tor_free(cell); } diff --git a/src/test/test_hs_client.c b/src/test/test_hs_client.c index 7ee7210bc..58e12abca 100644 --- a/src/test/test_hs_client.c +++ b/src/test/test_hs_client.c @@ -213,12 +213,12 @@ test_e2e_rend_circuit_setup_legacy(void *arg) tt_int_op(retval, OP_EQ, 1);

/* Check the digest algo */ - tt_int_op(crypto_digest_get_algorithm(or_circ->cpath->f_digest), + tt_int_op(crypto_digest_get_algorithm(or_circ->cpath->crypto.f_digest), OP_EQ, DIGEST_SHA1); - tt_int_op(crypto_digest_get_algorithm(or_circ->cpath->b_digest), + tt_int_op(crypto_digest_get_algorithm(or_circ->cpath->crypto.b_digest), OP_EQ, DIGEST_SHA1); - tt_assert(or_circ->cpath->f_crypto); - tt_assert(or_circ->cpath->b_crypto); + tt_assert(or_circ->cpath->crypto.f_crypto); + tt_assert(or_circ->cpath->crypto.b_crypto);

/* Ensure that circ purpose was changed */ tt_int_op(or_circ->base_.purpose, OP_EQ, CIRCUIT_PURPOSE_C_REND_JOINED); @@ -283,12 +283,12 @@ test_e2e_rend_circuit_setup(void *arg) tt_int_op(retval, OP_EQ, 1);

/* Check that the crypt path has prop224 algorithm parameters */ - tt_int_op(crypto_digest_get_algorithm(or_circ->cpath->f_digest), + tt_int_op(crypto_digest_get_algorithm(or_circ->cpath->crypto.f_digest), OP_EQ, DIGEST_SHA3_256); - tt_int_op(crypto_digest_get_algorithm(or_circ->cpath->b_digest), + tt_int_op(crypto_digest_get_algorithm(or_circ->cpath->crypto.b_digest), OP_EQ, DIGEST_SHA3_256); - tt_assert(or_circ->cpath->f_crypto); - tt_assert(or_circ->cpath->b_crypto); + tt_assert(or_circ->cpath->crypto.f_crypto); + tt_assert(or_circ->cpath->crypto.b_crypto);

/* Ensure that circ purpose was changed */ tt_int_op(or_circ->base_.purpose, OP_EQ, CIRCUIT_PURPOSE_C_REND_JOINED); diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c index 7fade6379..ed9f9814f 100644 --- a/src/test/test_hs_service.c +++ b/src/test/test_hs_service.c @@ -173,12 +173,12 @@ test_e2e_rend_circuit_setup(void *arg) tt_int_op(retval, OP_EQ, 1);

/* Check the digest algo */ - tt_int_op(crypto_digest_get_algorithm(or_circ->cpath->f_digest), + tt_int_op(crypto_digest_get_algorithm(or_circ->cpath->crypto.f_digest), OP_EQ, DIGEST_SHA3_256); - tt_int_op(crypto_digest_get_algorithm(or_circ->cpath->b_digest), + tt_int_op(crypto_digest_get_algorithm(or_circ->cpath->crypto.b_digest), OP_EQ, DIGEST_SHA3_256); - tt_assert(or_circ->cpath->f_crypto); - tt_assert(or_circ->cpath->b_crypto); + tt_assert(or_circ->cpath->crypto.f_crypto); + tt_assert(or_circ->cpath->crypto.b_crypto);

/* Ensure that circ purpose was changed */ tt_int_op(or_circ->base_.purpose, OP_EQ, CIRCUIT_PURPOSE_S_REND_JOINED);