commit 942543253a30b8231c46eeaeb44f7ba174152113 Author: Nick Mathewson nickm@torproject.org Date: Thu Jan 16 19:52:01 2020 -0500

Use time-invariant conditional memcpy to make onionbalance loop safer --- src/feature/hs/hs_cell.c | 51 +++++++++++++++++++----------------------------- 1 file changed, 20 insertions(+), 31 deletions(-)

diff --git a/src/feature/hs/hs_cell.c b/src/feature/hs/hs_cell.c index 2b8345383..c82ffd647 100644 --- a/src/feature/hs/hs_cell.c +++ b/src/feature/hs/hs_cell.c @@ -776,20 +776,20 @@ get_introduce2_keys_and_verify_mac(hs_cell_introduce2_data_t *data, if (intro_keys == NULL) { log_info(LD_REND, "Invalid INTRODUCE2 encrypted data. Unable to " "compute key material"); - goto err; + return NULL; + } + + /* Make sure we are not about to underflow. */ + if (BUG(encrypted_section_len < DIGEST256_LEN)) { + return NULL; }

/* Validate MAC from the cell and our computed key material. The MAC field * in the cell is at the end of the encrypted section. */ - int found_idx = -1; + intro_keys_result = tor_malloc_zero(sizeof(*intro_keys_result)); for (int i = 0; i < data->n_subcredentials; ++i) { uint8_t mac[DIGEST256_LEN];

- /* Make sure we are not about to underflow. */ - if (encrypted_section_len < sizeof(mac)) { - goto err; - } - /* The MAC field is at the very end of the ENCRYPTED section. */ size_t mac_offset = encrypted_section_len - sizeof(mac); /* Compute the MAC. Use the entire encoded payload with a length up to the @@ -800,33 +800,22 @@ get_introduce2_keys_and_verify_mac(hs_cell_introduce2_data_t *data, intro_keys[i].mac_key, sizeof(intro_keys[i].mac_key), mac, sizeof(mac)); - if (tor_memeq(mac, encrypted_section + mac_offset, sizeof(mac))) { - found_idx = i; - break; - } - } - - if (found_idx == -1) { - log_info(LD_REND, "Invalid MAC validation for INTRODUCE2 cell"); - goto err; + /* Time-invariant conditional copy: if the MAC is what we expected, then + * set intro_keys_result to intro_keys[i]. Otherwise, don't: but don't + * leak which one it was! */ + bool equal = tor_memeq(mac, encrypted_section + mac_offset, sizeof(mac)); + memcpy_if_true_timei(equal, intro_keys_result, &intro_keys[i], + sizeof(*intro_keys_result)); }

- /* We found a match! */ - if (data->n_subcredentials == 1) { - /* There was only one; steal it. */ - intro_keys_result = intro_keys; - intro_keys = NULL; - } else { - /* Copy out the one we wanted. */ - intro_keys_result = tor_memdup(&intro_keys[found_idx], - sizeof(hs_ntor_intro_cell_keys_t)); - } + /* We no longer need intro_keys. */ + memwipe(intro_keys, 0, + sizeof(hs_ntor_intro_cell_keys_t) * data->n_subcredentials); + tor_free(intro_keys);

- err: - if (intro_keys) { - memwipe(intro_keys, 0, - sizeof(hs_ntor_intro_cell_keys_t) * data->n_subcredentials); - tor_free(intro_keys); + if (safe_mem_is_zero(intro_keys_result, sizeof(*intro_keys_result))) { + log_info(LD_REND, "Invalid MAC validation for INTRODUCE2 cell"); + tor_free(intro_keys_result); /* sets intro_keys_result to NULL */ }

return intro_keys_result;