commit ae22c249c3423acfb7938a36c2c8c5b2cac7fb29 Author: Nick Mathewson nickm@torproject.org Date: Wed Jul 6 12:37:52 2016 -0400
Improve test coverage a little on onion*.c --- src/or/onion.c | 15 +++++++++++++++ src/or/onion_ntor.c | 5 +++++ src/or/onion_tap.c | 12 ++++++++++++ src/test/test.c | 21 ++++++++++++++++++--- 4 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/src/or/onion.c b/src/or/onion.c index 7c7f97f..5495074 100644 --- a/src/or/onion.c +++ b/src/or/onion.c @@ -130,9 +130,12 @@ onion_pending_add(or_circuit_t *circ, create_cell_t *onionskin) time_t now = time(NULL);
if (onionskin->handshake_type > MAX_ONION_HANDSHAKE_TYPE) { + /* LCOV_EXCL_START + * We should have rejected this far before this point */ log_warn(LD_BUG, "Handshake %d out of range! Dropping.", onionskin->handshake_type); return -1; + /* LCOV_EXCL_STOP */ }
tmp = tor_malloc_zero(sizeof(onion_queue_t)); @@ -305,10 +308,13 @@ static void onion_queue_entry_remove(onion_queue_t *victim) { if (victim->handshake_type > MAX_ONION_HANDSHAKE_TYPE) { + /* LCOV_EXCL_START + * We should have rejected this far before this point */ log_warn(LD_BUG, "Handshake %d out of range! Dropping.", victim->handshake_type); /* XXX leaks */ return; + /* LCOV_EXCL_STOP */ }
TOR_TAILQ_REMOVE(&ol_list[victim->handshake_type], victim, next); @@ -391,9 +397,12 @@ onion_handshake_state_release(onion_handshake_state_t *state) state->u.ntor = NULL; break; default: + /* LCOV_EXCL_START + * This state should not even exist. */ log_warn(LD_BUG, "called with unknown handshake state type %d", (int)state->tag); tor_fragile_assert(); + /* LCOV_EXCL_STOP */ } }
@@ -441,9 +450,12 @@ onion_skin_create(int type, r = NTOR_ONIONSKIN_LEN; break; default: + /* LCOV_EXCL_START + * We should never try to create an impossible handshake type. */ log_warn(LD_BUG, "called with unknown handshake state type %d", type); tor_fragile_assert(); r = -1; + /* LCOV_EXCL_STOP */ }
if (r > 0) @@ -512,9 +524,12 @@ onion_skin_server_handshake(int type, } break; default: + /* LCOV_EXCL_START + * We should have rejected this far before this point */ log_warn(LD_BUG, "called with unknown handshake state type %d", type); tor_fragile_assert(); return -1; + /* LCOV_EXCL_STOP */ }
return r; diff --git a/src/or/onion_ntor.c b/src/or/onion_ntor.c index 33afc27..d1a268f 100644 --- a/src/or/onion_ntor.c +++ b/src/or/onion_ntor.c @@ -85,8 +85,13 @@ onion_skin_ntor_create(const uint8_t *router_id, memcpy(state->router_id, router_id, DIGEST_LEN); memcpy(&state->pubkey_B, router_key, sizeof(curve25519_public_key_t)); if (curve25519_secret_key_generate(&state->seckey_x, 0) < 0) { + /* LCOV_EXCL_START + * Secret key generation should be unable to fail when the key isn't + * marked as "extra-strong" */ + tor_assert_nonfatal_unreached(); tor_free(state); return -1; + /* LCOV_EXCL_STOP */ } curve25519_public_key_generate(&state->pubkey_X, &state->seckey_x);
diff --git a/src/or/onion_tap.c b/src/or/onion_tap.c index bfd4723..abe7793 100644 --- a/src/or/onion_tap.c +++ b/src/or/onion_tap.c @@ -74,9 +74,13 @@ onion_skin_TAP_create(crypto_pk_t *dest_router_key,
return 0; err: + /* LCOV_EXCL_START + * We only get here if RSA encryption fails or DH keygen fails. Those + * shouldn't be possible. */ memwipe(challenge, 0, sizeof(challenge)); if (dh) crypto_dh_free(dh); return -1; + /* LCOV_EXCL_STOP */ }
/** Given an encrypted DH public key as generated by onion_skin_create, @@ -130,12 +134,20 @@ onion_skin_TAP_server_handshake(
dh = crypto_dh_new(DH_TYPE_CIRCUIT); if (!dh) { + /* LCOV_EXCL_START + * Failure to allocate a DH key should be impossible. + */ log_warn(LD_BUG, "Couldn't allocate DH key"); goto err; + /* LCOV_EXCL_STOP */ } if (crypto_dh_get_public(dh, handshake_reply_out, DH_KEY_LEN)) { + /* LCOV_EXCL_START + * This can only fail if the length of the key we just allocated is too + * big. That should be impossible. */ log_info(LD_GENERAL, "crypto_dh_get_public failed."); goto err; + /* LCOV_EXCP_STOP */ }
key_material_len = DIGEST_LEN+key_out_len; diff --git a/src/test/test.c b/src/test/test.c index 3a1054d..2f18346 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -178,20 +178,26 @@ test_bad_onion_handshake(void *arg) s_buf, s_keys, 40));
/* Client: Case 1: The server sent back junk. */ + const char *msg = NULL; s_buf[64] ^= 33; tt_int_op(-1, OP_EQ, - onion_skin_TAP_client_handshake(c_dh, s_buf, c_keys, 40, NULL)); + onion_skin_TAP_client_handshake(c_dh, s_buf, c_keys, 40, &msg)); s_buf[64] ^= 33; + tt_str_op(msg, OP_EQ, "Digest DOES NOT MATCH on onion handshake. " + "Bug or attack.");
/* Let the client finish; make sure it can. */ + msg = NULL; tt_int_op(0, OP_EQ, - onion_skin_TAP_client_handshake(c_dh, s_buf, c_keys, 40, NULL)); + onion_skin_TAP_client_handshake(c_dh, s_buf, c_keys, 40, &msg)); tt_mem_op(s_keys,OP_EQ, c_keys, 40); + tt_ptr_op(msg, OP_EQ, NULL);
/* Client: Case 2: The server sent back a degenerate DH. */ memset(s_buf, 0, sizeof(s_buf)); tt_int_op(-1, OP_EQ, - onion_skin_TAP_client_handshake(c_dh, s_buf, c_keys, 40, NULL)); + onion_skin_TAP_client_handshake(c_dh, s_buf, c_keys, 40, &msg)); + tt_str_op(msg, OP_EQ, "DH computation failed.");
done: crypto_dh_free(c_dh); @@ -246,6 +252,15 @@ test_ntor_handshake(void *arg) memset(s_buf, 0, 40); tt_mem_op(c_keys,OP_NE, s_buf, 40);
+ /* Now try with a bogus server response. Zero input should trigger + * All The Problems. */ + memset(c_keys, 0, 400); + memset(s_buf, 0, NTOR_REPLY_LEN); + const char *msg = NULL; + tt_int_op(-1, OP_EQ, onion_skin_ntor_client_handshake(c_state, s_buf, + c_keys, 400, &msg)); + tt_str_op(msg, OP_EQ, "Zero output from curve25519 handshake"); + done: ntor_handshake_state_free(c_state); dimap_free(s_keymap, NULL);