From 3d0d2a511c13d6d24be73c8651374c4d7db99379 Mon Sep 17 00:00:00 2001 From: cypherpunks Date: Wed, 11 Nov 2015 11:50:09 +0100 Subject: [PATCH 1/6] Fix buffer over-reads in the directory tests The tests pass empty digest strings to the dir_server_new function which copies it into a directory server structure. The copy operation expects the digest strings to be DIGEST_LEN characters long. Because the length of the empty digest strings are lower than DIGEST_LEN, the copy operation reads outside the digest strings which leads to buffer over-reads. The issue is resolved by using character arrays with a size of DIGEST_LEN. Patch on 4ff08bb5811ddfe554e597d129ec48a774364480. --- src/test/test_dir_handle_get.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/test/test_dir_handle_get.c b/src/test/test_dir_handle_get.c index 2e5a50a2f6..be003df2c0 100644 --- a/src/test/test_dir_handle_get.c +++ b/src/test/test_dir_handle_get.c @@ -1231,6 +1231,7 @@ test_dir_handle_get_server_keys_all(void* data) char *header = NULL; char *body = NULL; size_t body_used = 0; + const char digest[DIGEST_LEN] = ""; dir_server_t *ds = NULL; (void) data; @@ -1241,7 +1242,7 @@ test_dir_handle_get_server_keys_all(void* data) routerlist_free_all(); /* create a trusted ds */ - ds = trusted_dir_server_new("ds", "127.0.0.1", 9059, 9060, "", NULL, + ds = trusted_dir_server_new("ds", "127.0.0.1", 9059, 9060, digest, NULL, V3_DIRINFO, 1.0); tt_assert(ds); dir_server_add(ds); @@ -1390,6 +1391,7 @@ test_dir_handle_get_server_keys_fp(void* data) char *body = NULL; size_t body_used = 0; dir_server_t *ds = NULL; + const char digest[DIGEST_LEN] = ""; (void) data; MOCK(connection_write_to_buf_impl_, connection_write_to_buf_mock); @@ -1398,7 +1400,7 @@ test_dir_handle_get_server_keys_fp(void* data) routerlist_free_all(); /* create a trusted ds */ - ds = trusted_dir_server_new("ds", "127.0.0.1", 9059, 9060, "", NULL, + ds = trusted_dir_server_new("ds", "127.0.0.1", 9059, 9060, digest, NULL, V3_DIRINFO, 1.0); tt_assert(ds); dir_server_add(ds); @@ -1543,6 +1545,7 @@ test_dir_handle_get_server_keys_fpsk(void* data) char *body = NULL; size_t body_used = 0; dir_server_t *ds = NULL; + const char digest[DIGEST_LEN] = ""; (void) data; MOCK(connection_write_to_buf_impl_, connection_write_to_buf_mock); @@ -1551,7 +1554,7 @@ test_dir_handle_get_server_keys_fpsk(void* data) routerlist_free_all(); /* create a trusted ds */ - ds = trusted_dir_server_new("ds", "127.0.0.1", 9059, 9060, "", NULL, + ds = trusted_dir_server_new("ds", "127.0.0.1", 9059, 9060, digest, NULL, V3_DIRINFO, 1.0); tt_assert(ds); @@ -1600,13 +1603,14 @@ test_dir_handle_get_server_keys_busy(void* data) dir_connection_t *conn = NULL; char *header = NULL; dir_server_t *ds = NULL; + const char digest[DIGEST_LEN] = ""; (void) data; clear_dir_servers(); routerlist_free_all(); /* create a trusted ds */ - ds = trusted_dir_server_new("ds", "127.0.0.1", 9059, 9060, "", NULL, + ds = trusted_dir_server_new("ds", "127.0.0.1", 9059, 9060, digest, NULL, V3_DIRINFO, 1.0); tt_assert(ds); @@ -1994,13 +1998,14 @@ test_dir_handle_get_status_vote_d(void* data) char *header = NULL, *body = NULL; size_t body_used = 0; dir_server_t *ds = NULL; + const char digest[DIGEST_LEN] = ""; (void) data; clear_dir_servers(); dirvote_free_all(); /* create a trusted ds */ - ds = trusted_dir_server_new("ds", "127.0.0.1", 9059, 9060, "", NULL, + ds = trusted_dir_server_new("ds", "127.0.0.1", 9059, 9060, digest, NULL, V3_DIRINFO, 1.0); tt_assert(ds); dir_server_add(ds); @@ -2338,6 +2343,7 @@ test_dir_handle_get_status_vote_next_authority(void* data) int status_out = 0; size_t body_used = 0; dir_server_t *ds = NULL; + const char digest[DIGEST_LEN] = ""; (void) data; clear_dir_servers(); @@ -2347,7 +2353,7 @@ test_dir_handle_get_status_vote_next_authority(void* data) mock_cert = authority_cert_parse_from_string(TEST_CERTIFICATE, NULL); /* create a trusted ds */ - ds = trusted_dir_server_new("ds", "127.0.0.1", 9059, 9060, "", NULL, + ds = trusted_dir_server_new("ds", "127.0.0.1", 9059, 9060, digest, NULL, V3_DIRINFO, 1.0); tt_assert(ds); dir_server_add(ds); @@ -2413,6 +2419,7 @@ test_dir_handle_get_status_vote_current_authority(void* data) const char *msg_out = NULL; int status_out = 0; size_t body_used = 0; + const char digest[DIGEST_LEN] = ""; dir_server_t *ds = NULL; (void) data; @@ -2424,7 +2431,7 @@ test_dir_handle_get_status_vote_current_authority(void* data) mock_cert = authority_cert_parse_from_string(TEST_CERTIFICATE, NULL); /* create a trusted ds */ - ds = trusted_dir_server_new("ds", "127.0.0.1", 9059, 9060, "", NULL, + ds = trusted_dir_server_new("ds", "127.0.0.1", 9059, 9060, digest, NULL, V3_DIRINFO, 1.0); tt_assert(ds); dir_server_add(ds); From c94aa4573ab571af233e83f539844f3ccdd9fc2b Mon Sep 17 00:00:00 2001 From: cypherpunks Date: Wed, 11 Nov 2015 14:47:35 +0100 Subject: [PATCH 2/6] Fix buffer over-reads in the rendcache tests The hidden service descriptor cache (rendcache) tests use digest maps which expect keys to have a length of DIGEST_LEN. Because the tests use key strings with a length lower than DIGEST_LEN, the internal copy operation reads outside the key strings which leads to buffer over-reads. The issue is resolved by using character arrays with a size of DIGEST_LEN. Patch on ade5005853c17b3ae5923c194680442e0f86db4d. --- src/test/test_rendcache.c | 49 +++++++++++++++++++++++---------------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/src/test/test_rendcache.c b/src/test/test_rendcache.c index 92adf01273..958c83aaa9 100644 --- a/src/test/test_rendcache.c +++ b/src/test/test_rendcache.c @@ -767,28 +767,31 @@ test_rend_cache_failure_intro_lookup(void *data) rend_cache_failure_t *failure; rend_cache_failure_intro_t *ip; rend_cache_failure_intro_t *entry; + const char key_ip_one[DIGEST_LEN] = "ip1"; + const char key_ip_two[DIGEST_LEN] = "ip2"; + const char key_foo[DIGEST_LEN] = "foo1"; rend_cache_init(); failure = rend_cache_failure_entry_new(); ip = rend_cache_failure_intro_entry_new(INTRO_POINT_FAILURE_TIMEOUT); - digestmap_set(failure->intro_failures, "ip1", ip); + digestmap_set(failure->intro_failures, key_ip_one, ip); strmap_set_lc(rend_cache_failure, "foo1", failure); // Test not found - ret = cache_failure_intro_lookup((const uint8_t *)"foo1", "foo2", NULL); + ret = cache_failure_intro_lookup((const uint8_t *) key_foo, "foo2", NULL); tt_int_op(ret, OP_EQ, 0); // Test found with no intro failures in it - ret = cache_failure_intro_lookup((const uint8_t *)"ip2", "foo1", NULL); + ret = cache_failure_intro_lookup((const uint8_t *) key_ip_two, "foo1", NULL); tt_int_op(ret, OP_EQ, 0); // Test found - ret = cache_failure_intro_lookup((const uint8_t *)"ip1", "foo1", NULL); + ret = cache_failure_intro_lookup((const uint8_t *) key_ip_one, "foo1", NULL); tt_int_op(ret, OP_EQ, 1); // Test found and asking for entry - cache_failure_intro_lookup((const uint8_t *)"ip1", "foo1", &entry); + cache_failure_intro_lookup((const uint8_t *) key_ip_one, "foo1", &entry); tt_assert(entry); tt_assert(entry == ip); @@ -892,6 +895,9 @@ test_rend_cache_failure_clean(void *data) rend_cache_failure_t *failure; rend_cache_failure_intro_t *ip_one, *ip_two; + const char key_one[DIGEST_LEN] = "ip1"; + const char key_two[DIGEST_LEN] = "ip2"; + (void)data; rend_cache_init(); @@ -909,7 +915,7 @@ test_rend_cache_failure_clean(void *data) // Test with one new intro point failure = rend_cache_failure_entry_new(); ip_one = rend_cache_failure_intro_entry_new(INTRO_POINT_FAILURE_TIMEOUT); - digestmap_set(failure->intro_failures, "ip1", ip_one); + digestmap_set(failure->intro_failures, key_one, ip_one); strmap_set_lc(rend_cache_failure, "foo1", failure); rend_cache_failure_clean(time(NULL)); tt_int_op(strmap_size(rend_cache_failure), OP_EQ, 1); @@ -919,7 +925,7 @@ test_rend_cache_failure_clean(void *data) failure = rend_cache_failure_entry_new(); ip_one = rend_cache_failure_intro_entry_new(INTRO_POINT_FAILURE_TIMEOUT); ip_one->created_ts = time(NULL) - 7*60; - digestmap_set(failure->intro_failures, "ip1", ip_one); + digestmap_set(failure->intro_failures, key_one, ip_one); strmap_set_lc(rend_cache_failure, "foo1", failure); rend_cache_failure_clean(time(NULL)); tt_int_op(strmap_size(rend_cache_failure), OP_EQ, 0); @@ -929,10 +935,10 @@ test_rend_cache_failure_clean(void *data) failure = rend_cache_failure_entry_new(); ip_one = rend_cache_failure_intro_entry_new(INTRO_POINT_FAILURE_TIMEOUT); ip_one->created_ts = time(NULL) - 7*60; - digestmap_set(failure->intro_failures, "ip1", ip_one); + digestmap_set(failure->intro_failures, key_one, ip_one); ip_two = rend_cache_failure_intro_entry_new(INTRO_POINT_FAILURE_TIMEOUT); ip_two->created_ts = time(NULL) - 2*60; - digestmap_set(failure->intro_failures, "ip2", ip_two); + digestmap_set(failure->intro_failures, key_two, ip_two); strmap_set_lc(rend_cache_failure, "foo1", failure); rend_cache_failure_clean(time(NULL)); tt_int_op(strmap_size(rend_cache_failure), OP_EQ, 1); @@ -1051,25 +1057,26 @@ test_rend_cache_failure_intro_add(void *data) (void)data; rend_cache_failure_t *fail_entry; rend_cache_failure_intro_t *entry; + const char identity[DIGEST_LEN] = "foo1"; rend_cache_init(); // Adds non-existing entry - cache_failure_intro_add((const uint8_t *)"foo1", "foo2", + cache_failure_intro_add((const uint8_t *) identity, "foo2", INTRO_POINT_FAILURE_TIMEOUT); fail_entry = strmap_get_lc(rend_cache_failure, "foo2"); tt_assert(fail_entry); tt_int_op(digestmap_size(fail_entry->intro_failures), OP_EQ, 1); - entry = digestmap_get(fail_entry->intro_failures, "foo1"); + entry = digestmap_get(fail_entry->intro_failures, identity); tt_assert(entry); // Adds existing entry - cache_failure_intro_add((const uint8_t *)"foo1", "foo2", + cache_failure_intro_add((const uint8_t *) identity, "foo2", INTRO_POINT_FAILURE_TIMEOUT); fail_entry = strmap_get_lc(rend_cache_failure, "foo2"); tt_assert(fail_entry); tt_int_op(digestmap_size(fail_entry->intro_failures), OP_EQ, 1); - entry = digestmap_get(fail_entry->intro_failures, "foo1"); + entry = digestmap_get(fail_entry->intro_failures, identity); tt_assert(entry); done: @@ -1082,22 +1089,23 @@ test_rend_cache_intro_failure_note(void *data) (void)data; rend_cache_failure_t *fail_entry; rend_cache_failure_intro_t *entry; + const char key[DIGEST_LEN] = "foo1"; rend_cache_init(); // Test not found rend_cache_intro_failure_note(INTRO_POINT_FAILURE_TIMEOUT, - (const uint8_t *)"foo1", "foo2"); + (const uint8_t *) key, "foo2"); fail_entry = strmap_get_lc(rend_cache_failure, "foo2"); tt_assert(fail_entry); tt_int_op(digestmap_size(fail_entry->intro_failures), OP_EQ, 1); - entry = digestmap_get(fail_entry->intro_failures, "foo1"); + entry = digestmap_get(fail_entry->intro_failures, key); tt_assert(entry); tt_int_op(entry->failure_type, OP_EQ, INTRO_POINT_FAILURE_TIMEOUT); // Test found rend_cache_intro_failure_note(INTRO_POINT_FAILURE_UNREACHABLE, - (const uint8_t *)"foo1", "foo2"); + (const uint8_t *) key, "foo2"); tt_int_op(entry->failure_type, OP_EQ, INTRO_POINT_FAILURE_UNREACHABLE); done: @@ -1121,6 +1129,7 @@ test_rend_cache_clean_v2_descs_as_dir(void *data) time_t now; rend_service_descriptor_t *desc; now = time(NULL); + const char key[DIGEST_LEN] = "abcde"; (void)data; @@ -1138,7 +1147,7 @@ test_rend_cache_clean_v2_descs_as_dir(void *data) desc->timestamp = now; desc->pk = pk_generate(0); e->parsed = desc; - digestmap_set(rend_cache_v2_dir, "abcde", e); + digestmap_set(rend_cache_v2_dir, key, e); hid_serv_responsible_for_desc_id_response = 1; rend_cache_clean_v2_descs_as_dir(now, 0); @@ -1157,7 +1166,7 @@ test_rend_cache_clean_v2_descs_as_dir(void *data) desc->timestamp = now; desc->pk = pk_generate(0); e->parsed = desc; - digestmap_set(rend_cache_v2_dir, "abcde", e); + digestmap_set(rend_cache_v2_dir, key, e); hid_serv_responsible_for_desc_id_response = 0; rend_cache_clean_v2_descs_as_dir(now, 0); @@ -1170,7 +1179,7 @@ test_rend_cache_clean_v2_descs_as_dir(void *data) desc->timestamp = now; desc->pk = pk_generate(0); e->parsed = desc; - digestmap_set(rend_cache_v2_dir, "abcde", e); + digestmap_set(rend_cache_v2_dir, key, e); hid_serv_responsible_for_desc_id_response = 1; rend_cache_clean_v2_descs_as_dir(now, 0); @@ -1183,7 +1192,7 @@ test_rend_cache_clean_v2_descs_as_dir(void *data) desc->timestamp = now; desc->pk = pk_generate(0); e->parsed = desc; - digestmap_set(rend_cache_v2_dir, "abcde", e); + digestmap_set(rend_cache_v2_dir, key, e); hid_serv_responsible_for_desc_id_response = 1; rend_cache_clean_v2_descs_as_dir(now, 20000); From 0a97a3095b6af3554d92a46f4108061a53cc4dd2 Mon Sep 17 00:00:00 2001 From: cypherpunks Date: Wed, 11 Nov 2015 15:05:47 +0100 Subject: [PATCH 3/6] Remove unnecessary casting --- src/test/test_rendcache.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/test_rendcache.c b/src/test/test_rendcache.c index 958c83aaa9..77796994b4 100644 --- a/src/test/test_rendcache.c +++ b/src/test/test_rendcache.c @@ -1265,7 +1265,7 @@ test_rend_cache_validate_intro_point_failure(void *data) rend_service_descriptor_t *desc = NULL; char *service_id = NULL; rend_intro_point_t *intro = NULL; - const uint8_t *identity = NULL; + const char *identity = NULL; rend_cache_failure_t *failure; rend_cache_failure_intro_t *ip; @@ -1275,11 +1275,11 @@ test_rend_cache_validate_intro_point_failure(void *data) desc->timestamp = time(NULL) + RECENT_TIME; intro = (rend_intro_point_t *)smartlist_get(desc->intro_nodes, 0); - identity = (uint8_t *) intro->extend_info->identity_digest; + identity = intro->extend_info->identity_digest; failure = rend_cache_failure_entry_new(); ip = rend_cache_failure_intro_entry_new(INTRO_POINT_FAILURE_TIMEOUT); - digestmap_set(failure->intro_failures, (char *)identity, ip); + digestmap_set(failure->intro_failures, identity, ip); strmap_set_lc(rend_cache_failure, service_id, failure); // Test when we have an intro point in our cache From 3dcb7320cf8cb458c60a5fe6d5d51dbaaf67f7f0 Mon Sep 17 00:00:00 2001 From: cypherpunks Date: Mon, 16 Nov 2015 15:12:44 +0100 Subject: [PATCH 4/6] Add changes file for 17776 --- changes/bug17776 | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changes/bug17776 diff --git a/changes/bug17776 b/changes/bug17776 new file mode 100644 index 0000000000..a949625baa --- /dev/null +++ b/changes/bug17776 @@ -0,0 +1,6 @@ + o Minor bugfixes (tests): + - Fix buffer over-reads in the directory tests. Fixes bug 17776; not in any + released version of Tor. + - Fix buffer over-reads in the rendcache tests. Fixes bug 17776; not in any + released version of Tor. + From fbdd32ebe9021d34dbe314a9cb24480ff3a9c9b1 Mon Sep 17 00:00:00 2001 From: cypherpunks Date: Wed, 9 Dec 2015 13:07:35 +0100 Subject: [PATCH 5/6] Mention the expected length of the digests Some functions that use digest maps did not mention that the digests are expected to have DIGEST_LEN bytes. This lead to buffer over-reads in the past. --- src/or/rendcache.c | 6 +++--- src/or/routerlist.c | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/or/rendcache.c b/src/or/rendcache.c index 790e0c246d..c69671e289 100644 --- a/src/or/rendcache.c +++ b/src/or/rendcache.c @@ -321,9 +321,9 @@ rend_cache_failure_purge(void) } /** Lookup the rend failure cache using a relay identity digest in - * identity and service ID service_id. If found, the intro - * failure is set in intro_entry else it stays untouched. Return 1 - * iff found else 0. */ + * identity which has DIGEST_LEN bytes and service ID service_id + * which is a null-terminated string. If found, the intro failure is set in + * intro_entry else it stays untouched. Return 1 iff found else 0. */ STATIC int cache_failure_intro_lookup(const uint8_t *identity, const char *service_id, rend_cache_failure_intro_t **intro_entry) diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 08911b96a7..814551521e 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -4034,9 +4034,9 @@ router_exit_policy_rejects_all(const routerinfo_t *router) } /** Create an directory server at address:port, with OR identity - * key digest. If address is NULL, add ourself. If - * is_authority, this is a directory authority. Return the new - * directory server entry on success or NULL on failure. */ + * key digest which has DIGEST_LEN bytes. If address is NULL, + * add ourself. If is_authority, this is a directory authority. Return + * the new directory server entry on success or NULL on failure. */ static dir_server_t * dir_server_new(int is_authority, const char *nickname, From c76059ec9bc8ac99096b253fc4af1119f26102eb Mon Sep 17 00:00:00 2001 From: cypherpunks Date: Wed, 9 Dec 2015 13:12:45 +0100 Subject: [PATCH 6/6] Assert that the directory server digest is given This prevents a possible crash when memory is copied from a pointer to NULL. --- src/or/routerlist.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 814551521e..5e7906475f 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -4051,6 +4051,8 @@ dir_server_new(int is_authority, uint32_t a; char *hostname_ = NULL; + tor_assert(digest); + if (weight < 0) return NULL;