[tor-commits] [tor/master] crypto: Change crypto_mac_sha3_256 to use the key length in the construction

commit 118691cd47e53521319cdcbf994f29ecca3db4d1 Author: David Goulet dgoulet@torproject.org Date: Mon Dec 12 16:45:28 2016 -0500

crypto: Change crypto_mac_sha3_256 to use the key length in the construction

Signed-off-by: David Goulet dgoulet@torproject.org --- src/common/crypto.c | 25 ++++++++++++++++--------- src/common/crypto.h | 7 ++++--- src/or/hs_intropoint.c | 18 +++++++++--------- src/or/hs_intropoint.h | 2 +- src/or/hs_service.c | 6 +++--- src/or/hs_service.h | 2 +- src/test/test_crypto.c | 24 +++++++++++++++--------- src/test/test_hs_intropoint.c | 29 +++++++++++++++-------------- src/test/test_hs_service.c | 8 ++++---- 9 files changed, 68 insertions(+), 53 deletions(-)

diff --git a/src/common/crypto.c b/src/common/crypto.c index e4ef52d..1b1f1f9 100644 --- a/src/common/crypto.c +++ b/src/common/crypto.c @@ -2109,25 +2109,32 @@ crypto_hmac_sha256(char *hmac_out, tor_assert(rv); }

-/** Compute an SHA3 MAC of <b>msg</b> using <b>key</b> as the key. The format - * used for our MAC is SHA3(k | m). Write the DIGEST256_LEN-byte result into - * <b>mac_out</b> of size <b>mac_out_len</b>. */ +/** Compute a MAC using SHA3-256 of <b>msg_len</b> bytes in <b>msg</b> using a + * <b>key</b> of length <b>key_len</b> and a <b>salt</b> of length + * <b>salt_len</b>. Store the result of <b>len_out</b> bytes in in + * <b>mac_out</b>. This function can't fail. */ void -crypto_mac_sha3_256(char *mac_out, size_t mac_out_len, - const char *key, size_t key_len, - const char *msg, size_t msg_len) +crypto_mac_sha3_256(uint8_t *mac_out, size_t len_out, + const uint8_t *key, size_t key_len, + const uint8_t *msg, size_t msg_len) { crypto_digest_t *digest;

+ const uint64_t key_len_netorder = tor_htonll(key_len); + tor_assert(mac_out); tor_assert(key); tor_assert(msg);

digest = crypto_digest256_new(DIGEST_SHA3_256);

- crypto_digest_add_bytes(digest, key, key_len); - crypto_digest_add_bytes(digest, msg, msg_len); - crypto_digest_get_digest(digest, mac_out, mac_out_len); + /* Order matters here that is any subsystem using this function should + * expect this very precise ordering in the MAC construction. */ + crypto_digest_add_bytes(digest, (const char *) &key_len_netorder, + sizeof(key_len_netorder)); + crypto_digest_add_bytes(digest, (const char *) key, key_len); + crypto_digest_add_bytes(digest, (const char *) msg, msg_len); + crypto_digest_get_digest(digest, (char *) mac_out, len_out); crypto_digest_free(digest); }

diff --git a/src/common/crypto.h b/src/common/crypto.h index 32b6531..bf2fa06 100644 --- a/src/common/crypto.h +++ b/src/common/crypto.h @@ -255,9 +255,10 @@ void crypto_digest_assign(crypto_digest_t *into, void crypto_hmac_sha256(char *hmac_out, const char *key, size_t key_len, const char *msg, size_t msg_len); -void crypto_mac_sha3_256(char *mac_out, size_t mac_out_len, - const char *key, size_t key_len, - const char *msg, size_t msg_len); +void crypto_mac_sha3_256(uint8_t *mac_out, size_t len_out, + const uint8_t *key, size_t key_len, + const uint8_t *msg, size_t msg_len); + crypto_xof_t *crypto_xof_new(void); void crypto_xof_add_bytes(crypto_xof_t *xof, const uint8_t *data, size_t len); void crypto_xof_squeeze_bytes(crypto_xof_t *xof, uint8_t *out, size_t len); diff --git a/src/or/hs_intropoint.c b/src/or/hs_intropoint.c index fb8637b..b5f62aa 100644 --- a/src/or/hs_intropoint.c +++ b/src/or/hs_intropoint.c @@ -42,7 +42,7 @@ get_auth_key_from_establish_intro_cell(ed25519_public_key_t *auth_key_out, * given <b>circuit_key_material</b>. Return 0 on success else -1 on error. */ STATIC int verify_establish_intro_cell(const hs_cell_establish_intro_t *cell, - const char *circuit_key_material, + const uint8_t *circuit_key_material, size_t circuit_key_material_len) { /* We only reach this function if the first byte of the cell is 0x02 which @@ -62,7 +62,7 @@ verify_establish_intro_cell(const hs_cell_establish_intro_t *cell, return -1; }

- const char *msg = (char*) cell->start_cell; + const uint8_t *msg = cell->start_cell;

/* Verify the sig */ { @@ -79,7 +79,7 @@ verify_establish_intro_cell(const hs_cell_establish_intro_t *cell, ed25519_public_key_t auth_key; get_auth_key_from_establish_intro_cell(&auth_key, cell);

- const size_t sig_msg_len = (char*) (cell->end_sig_fields) - msg; + const size_t sig_msg_len = cell->end_sig_fields - msg; int sig_mismatch = ed25519_checksig_prefixed(&sig_struct, (uint8_t*) msg, sig_msg_len, ESTABLISH_INTRO_SIG_PREFIX, @@ -92,11 +92,11 @@ verify_establish_intro_cell(const hs_cell_establish_intro_t *cell,

/* Verify the MAC */ { - const size_t auth_msg_len = (char*) (cell->end_mac_fields) - msg; - char mac[DIGEST256_LEN]; + const size_t auth_msg_len = cell->end_mac_fields - msg; + uint8_t mac[DIGEST256_LEN]; crypto_mac_sha3_256(mac, sizeof(mac), - circuit_key_material, circuit_key_material_len, - msg, auth_msg_len); + circuit_key_material, circuit_key_material_len, + msg, auth_msg_len); if (tor_memneq(mac, cell->handshake_mac, sizeof(mac))) { log_warn(LD_PROTOCOL, "ESTABLISH_INTRO handshake_auth not as expected"); return -1; @@ -198,8 +198,8 @@ handle_establish_intro(or_circuit_t *circ, const uint8_t *request, }

cell_ok = verify_establish_intro_cell(parsed_cell, - circ->rend_circ_nonce, - sizeof(circ->rend_circ_nonce)); + (uint8_t *) circ->rend_circ_nonce, + sizeof(circ->rend_circ_nonce)); if (cell_ok < 0) { log_warn(LD_PROTOCOL, "Failed to verify ESTABLISH_INTRO cell."); goto err; diff --git a/src/or/hs_intropoint.h b/src/or/hs_intropoint.h index 651e2dc..b7846a4 100644 --- a/src/or/hs_intropoint.h +++ b/src/or/hs_intropoint.h @@ -28,7 +28,7 @@ int hs_intro_circuit_is_suitable(const or_circuit_t *circ);

STATIC int verify_establish_intro_cell(const hs_cell_establish_intro_t *out, - const char *circuit_key_material, + const uint8_t *circuit_key_material, size_t circuit_key_material_len);

STATIC void diff --git a/src/or/hs_service.c b/src/or/hs_service.c index 69f83cf..6f0836c 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -60,7 +60,7 @@ set_cell_extensions(hs_cell_establish_intro_t *cell) * returned cell is allocated on the heap and it's the responsibility of the * caller to free it. */ STATIC hs_cell_establish_intro_t * -generate_establish_intro_cell(const char *circuit_key_material, +generate_establish_intro_cell(const uint8_t *circuit_key_material, size_t circuit_key_material_len) { hs_cell_establish_intro_t *cell = NULL; @@ -109,7 +109,7 @@ generate_establish_intro_cell(const char *circuit_key_material, /* To calculate HANDSHAKE_AUTH, we dump the cell in bytes, and then derive the MAC from it. */ uint8_t cell_bytes_tmp[RELAY_PAYLOAD_SIZE] = {0}; - char mac[TRUNNEL_SHA3_256_LEN]; + uint8_t mac[TRUNNEL_SHA3_256_LEN];

encoded_len = hs_cell_establish_intro_encode(cell_bytes_tmp, sizeof(cell_bytes_tmp), @@ -125,7 +125,7 @@ generate_establish_intro_cell(const char *circuit_key_material, /* Calculate MAC of all fields before HANDSHAKE_AUTH */ crypto_mac_sha3_256(mac, sizeof(mac), circuit_key_material, circuit_key_material_len, - (const char*)cell_bytes_tmp, + cell_bytes_tmp, encoded_len - (ED25519_SIG_LEN + 2 + TRUNNEL_SHA3_256_LEN)); /* Write the MAC to the cell */ uint8_t *handshake_ptr = diff --git a/src/or/hs_service.h b/src/or/hs_service.h index 4a76007..a54a960 100644 --- a/src/or/hs_service.h +++ b/src/or/hs_service.h @@ -17,7 +17,7 @@ #ifdef TOR_UNIT_TESTS

STATIC hs_cell_establish_intro_t * -generate_establish_intro_cell(const char *circuit_key_material, +generate_establish_intro_cell(const uint8_t *circuit_key_material, size_t circuit_key_material_len);

STATIC ssize_t diff --git a/src/test/test_crypto.c b/src/test/test_crypto.c index 91c55d8..d66ddcc 100644 --- a/src/test/test_crypto.c +++ b/src/test/test_crypto.c @@ -1147,28 +1147,34 @@ test_crypto_mac_sha3(void *arg) const char msg[] = "i am in a library somewhere using my computer"; const char key[] = "i'm from the past talking to the future.";

- char hmac_test[DIGEST256_LEN]; + uint8_t hmac_test[DIGEST256_LEN]; char hmac_manual[DIGEST256_LEN];

(void) arg;

/* First let's use our nice HMAC-SHA3 function */ crypto_mac_sha3_256(hmac_test, sizeof(hmac_test), - key, strlen(key), - msg, strlen(msg)); + (uint8_t *) key, strlen(key), + (uint8_t *) msg, strlen(msg));

- /* Now let's try a manual H(k || m) construction */ + /* Now let's try a manual H(len(k) || k || m) construction */ { - char *key_msg_concat = NULL; + char *key_msg_concat = NULL, *all = NULL; int result; + const uint64_t key_len_netorder = tor_htonll(strlen(key)); + size_t all_len;

tor_asprintf(&key_msg_concat, "%s%s", key, msg); + all_len = sizeof(key_len_netorder) + strlen(key_msg_concat); + all = tor_malloc_zero(all_len); + memcpy(all, &key_len_netorder, sizeof(key_len_netorder)); + memcpy(all + sizeof(key_len_netorder), key_msg_concat, + strlen(key_msg_concat));

- result = crypto_digest256(hmac_manual, - key_msg_concat, strlen(key_msg_concat), - DIGEST_SHA3_256); - tt_int_op(result, ==, 0); + result = crypto_digest256(hmac_manual, all, all_len, DIGEST_SHA3_256); tor_free(key_msg_concat); + tor_free(all); + tt_int_op(result, ==, 0); }

/* Now compare the two results */ diff --git a/src/test/test_hs_intropoint.c b/src/test/test_hs_intropoint.c index 76f9dba..608988b 100644 --- a/src/test/test_hs_intropoint.c +++ b/src/test/test_hs_intropoint.c @@ -44,12 +44,12 @@ test_establish_intro_wrong_purpose(void *arg) or_circuit_t *intro_circ = or_circuit_new(0,NULL);; uint8_t cell_body[RELAY_PAYLOAD_SIZE]; ssize_t cell_len = 0; - char circuit_key_material[DIGEST_LEN] = {0}; + uint8_t circuit_key_material[DIGEST_LEN] = {0};

(void)arg;

/* Get the auth key of the intro point */ - crypto_rand(circuit_key_material, sizeof(circuit_key_material)); + crypto_rand((char *) circuit_key_material, sizeof(circuit_key_material)); memcpy(intro_circ->rend_circ_nonce, circuit_key_material, DIGEST_LEN);

/* Set a bad circuit purpose!! :) */ @@ -75,7 +75,8 @@ test_establish_intro_wrong_purpose(void *arg)

/* Prepare a circuit for accepting an ESTABLISH_INTRO cell */ static void -helper_prepare_circ_for_intro(or_circuit_t *circ, char *circuit_key_material) +helper_prepare_circ_for_intro(or_circuit_t *circ, + uint8_t *circuit_key_material) { /* Prepare the circuit for the incoming ESTABLISH_INTRO */ circuit_change_purpose(TO_CIRCUIT(circ), CIRCUIT_PURPOSE_OR); @@ -88,12 +89,12 @@ test_establish_intro_wrong_keytype(void *arg) { int retval; or_circuit_t *intro_circ = or_circuit_new(0,NULL);; - char circuit_key_material[DIGEST_LEN] = {0}; + uint8_t circuit_key_material[DIGEST_LEN] = {0};

(void)arg;

/* Get the auth key of the intro point */ - crypto_rand(circuit_key_material, sizeof(circuit_key_material)); + crypto_rand((char *) circuit_key_material, sizeof(circuit_key_material)); helper_prepare_circ_for_intro(intro_circ, circuit_key_material);

/* Receive the cell. Should fail. */ @@ -113,12 +114,12 @@ test_establish_intro_wrong_keytype2(void *arg) or_circuit_t *intro_circ = or_circuit_new(0,NULL);; uint8_t cell_body[RELAY_PAYLOAD_SIZE]; ssize_t cell_len = 0; - char circuit_key_material[DIGEST_LEN] = {0}; + uint8_t circuit_key_material[DIGEST_LEN] = {0};

(void)arg;

/* Get the auth key of the intro point */ - crypto_rand(circuit_key_material, sizeof(circuit_key_material)); + crypto_rand((char *) circuit_key_material, sizeof(circuit_key_material)); helper_prepare_circ_for_intro(intro_circ, circuit_key_material);

/* Create outgoing ESTABLISH_INTRO cell and extract its payload so that we @@ -152,12 +153,12 @@ test_establish_intro_wrong_sig(void *arg) or_circuit_t *intro_circ = or_circuit_new(0,NULL);; uint8_t cell_body[RELAY_PAYLOAD_SIZE]; ssize_t cell_len = 0; - char circuit_key_material[DIGEST_LEN] = {0}; + uint8_t circuit_key_material[DIGEST_LEN] = {0};

(void)arg;

/* Get the auth key of the intro point */ - crypto_rand(circuit_key_material, sizeof(circuit_key_material)); + crypto_rand((char *) circuit_key_material, sizeof(circuit_key_material)); helper_prepare_circ_for_intro(intro_circ, circuit_key_material);

/* Create outgoing ESTABLISH_INTRO cell and extract its payload so that we @@ -190,12 +191,12 @@ helper_establish_intro_v3(or_circuit_t *intro_circ) hs_cell_establish_intro_t *establish_intro_cell = NULL; uint8_t cell_body[RELAY_PAYLOAD_SIZE]; ssize_t cell_len = 0; - char circuit_key_material[DIGEST_LEN] = {0}; + uint8_t circuit_key_material[DIGEST_LEN] = {0};

tt_assert(intro_circ);

/* Prepare the circuit for the incoming ESTABLISH_INTRO */ - crypto_rand(circuit_key_material, sizeof(circuit_key_material)); + crypto_rand((char *) circuit_key_material, sizeof(circuit_key_material)); helper_prepare_circ_for_intro(intro_circ, circuit_key_material);

/* Create outgoing ESTABLISH_INTRO cell and extract its payload so that we @@ -224,12 +225,12 @@ helper_establish_intro_v2(or_circuit_t *intro_circ) int retval; uint8_t cell_body[RELAY_PAYLOAD_SIZE]; ssize_t cell_len = 0; - char circuit_key_material[DIGEST_LEN] = {0}; + uint8_t circuit_key_material[DIGEST_LEN] = {0};

tt_assert(intro_circ);

/* Prepare the circuit for the incoming ESTABLISH_INTRO */ - crypto_rand(circuit_key_material, sizeof(circuit_key_material)); + crypto_rand((char *) circuit_key_material, sizeof(circuit_key_material)); helper_prepare_circ_for_intro(intro_circ, circuit_key_material);

/* Send legacy establish_intro */ @@ -238,7 +239,7 @@ helper_establish_intro_v2(or_circuit_t *intro_circ) /* Use old circuit_key_material why not */ cell_len = encode_establish_intro_cell_legacy((char*)cell_body, key1, - circuit_key_material); + (char *) circuit_key_material); tt_int_op(cell_len, >, 0);

/* Receive legacy establish_intro */ diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c index 88c7ef2..195e506 100644 --- a/src/test/test_hs_service.c +++ b/src/test/test_hs_service.c @@ -24,12 +24,12 @@ test_gen_establish_intro_cell(void *arg) { (void) arg; int retval; - char circuit_key_material[DIGEST_LEN] = {0}; + uint8_t circuit_key_material[DIGEST_LEN] = {0}; uint8_t buf[RELAY_PAYLOAD_SIZE]; hs_cell_establish_intro_t *cell_out = NULL; hs_cell_establish_intro_t *cell_in = NULL;

- crypto_rand(circuit_key_material, sizeof(circuit_key_material)); + crypto_rand((char *) circuit_key_material, sizeof(circuit_key_material));

/* Create outgoing ESTABLISH_INTRO cell and extract its payload so that we attempt to parse it. */ @@ -79,11 +79,11 @@ test_gen_establish_intro_cell_bad(void *arg) { (void) arg; hs_cell_establish_intro_t *cell = NULL; - char circuit_key_material[DIGEST_LEN] = {0}; + uint8_t circuit_key_material[DIGEST_LEN] = {0};

MOCK(ed25519_sign_prefixed, mock_ed25519_sign_prefixed);

- crypto_rand(circuit_key_material, sizeof(circuit_key_material)); + crypto_rand((char *) circuit_key_material, sizeof(circuit_key_material));

setup_full_capture_of_logs(LOG_WARN); /* Easiest way to make that function fail is to mock the