[tor-commits] [tor/master] Followup: Make authority_cert_parse_from_string() take length too

commit 04bb70199be924804708bd4ace18b28b5acdbf19 Author: Nick Mathewson nickm@torproject.org Date: Tue Sep 11 11:37:55 2018 -0400

Followup: Make authority_cert_parse_from_string() take length too --- src/feature/nodelist/routerlist.c | 3 ++- src/feature/nodelist/routerparse.c | 19 +++++++++++-------- src/feature/nodelist/routerparse.h | 1 + src/feature/relay/router.c | 2 +- src/test/test_dir.c | 12 +++++++++--- src/test/test_dir_common.c | 12 +++++++++--- src/test/test_dir_handle_get.c | 16 ++++++++++++---- src/test/test_routerlist.c | 12 +++++++++--- src/test/test_shared_random.c | 12 +++++++++--- 9 files changed, 63 insertions(+), 26 deletions(-)

diff --git a/src/feature/nodelist/routerlist.c b/src/feature/nodelist/routerlist.c index bcc5c1f07..c3c72e9c7 100644 --- a/src/feature/nodelist/routerlist.c +++ b/src/feature/nodelist/routerlist.c @@ -555,7 +555,8 @@ trusted_dirs_load_certs_from_string(const char *contents, int source, int added_trusted_cert = 0;

for (s = contents; *s; s = eos) { - authority_cert_t *cert = authority_cert_parse_from_string(s, &eos); + authority_cert_t *cert = authority_cert_parse_from_string(s, strlen(s), + &eos); cert_list_t *cl; if (!cert) { failure_code = -1; diff --git a/src/feature/nodelist/routerparse.c b/src/feature/nodelist/routerparse.c index 9abdfb614..8a5efc007 100644 --- a/src/feature/nodelist/routerparse.c +++ b/src/feature/nodelist/routerparse.c @@ -2308,7 +2308,8 @@ extrainfo_parse_entry_from_string(const char *s, const char *end, /** Parse a key certificate from <b>s</b>; point <b>end-of-string</b> to * the first character after the certificate. */ authority_cert_t * -authority_cert_parse_from_string(const char *s, const char **end_of_string) +authority_cert_parse_from_string(const char *s, size_t maxlen, + const char **end_of_string) { /** Reject any certificate at least this big; it is probably an overflow, an * attack, a bug, or some other nonsense. */ @@ -2319,24 +2320,25 @@ authority_cert_parse_from_string(const char *s, const char **end_of_string) char digest[DIGEST_LEN]; directory_token_t *tok; char fp_declared[DIGEST_LEN]; - char *eos; + const char *eos; size_t len; int found; memarea_t *area = NULL; + const char *end_of_s = s + maxlen; const char *s_dup = s;

- s = eat_whitespace(s); - eos = strstr(s, "\ndir-key-certification"); + s = eat_whitespace_eos(s, end_of_s); + eos = tor_memstr(s, end_of_s - s, "\ndir-key-certification"); if (! eos) { log_warn(LD_DIR, "No signature found on key certificate"); return NULL; } - eos = strstr(eos, "\n-----END SIGNATURE-----\n"); + eos = tor_memstr(eos, end_of_s - eos, "\n-----END SIGNATURE-----\n"); if (! eos) { log_warn(LD_DIR, "No end-of-signature found on key certificate"); return NULL; } - eos = strchr(eos+2, '\n'); + eos = memchr(eos+2, '\n', end_of_s - (eos+2)); tor_assert(eos); ++eos; len = eos - s; @@ -2353,7 +2355,7 @@ authority_cert_parse_from_string(const char *s, const char **end_of_string) log_warn(LD_DIR, "Error tokenizing key certificate"); goto err; } - if (router_get_hash_impl(s, strlen(s), digest, "dir-key-certificate-version", + if (router_get_hash_impl(s, eos-s, digest, "dir-key-certificate-version", "\ndir-key-certification", '\n', DIGEST_SHA1) < 0) goto err; tok = smartlist_get(tokens, 0); @@ -3465,7 +3467,8 @@ networkstatus_parse_vote_from_string(const char *s, "\ndir-key-certificate-version"))) goto err; ++cert; - ns->cert = authority_cert_parse_from_string(cert, &end_of_cert); + ns->cert = authority_cert_parse_from_string(cert, end_of_header - cert, + &end_of_cert); if (!ns->cert || !end_of_cert || end_of_cert > end_of_header) goto err; } diff --git a/src/feature/nodelist/routerparse.h b/src/feature/nodelist/routerparse.h index 390088c94..c60046e8e 100644 --- a/src/feature/nodelist/routerparse.h +++ b/src/feature/nodelist/routerparse.h @@ -93,6 +93,7 @@ smartlist_t *microdescs_parse_from_string(const char *s, const char *eos, smartlist_t *invalid_digests_out);

authority_cert_t *authority_cert_parse_from_string(const char *s, + size_t maxlen, const char **end_of_string); int rend_parse_v2_service_descriptor(rend_service_descriptor_t **parsed_out, char *desc_id_out, diff --git a/src/feature/relay/router.c b/src/feature/relay/router.c index 1f316ebf0..17f63e998 100644 --- a/src/feature/relay/router.c +++ b/src/feature/relay/router.c @@ -717,7 +717,7 @@ load_authority_keyset(int legacy, crypto_pk_t **key_out, fname); goto done; } - parsed = authority_cert_parse_from_string(cert, &eos); + parsed = authority_cert_parse_from_string(cert, strlen(cert), &eos); if (!parsed) { log_warn(LD_DIR, "Unable to parse certificate in %s", fname); goto done; diff --git a/src/test/test_dir.c b/src/test/test_dir.c index 0fa5c3103..078a383dd 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -2799,11 +2799,17 @@ test_a_networkstatus( MOCK(get_my_v3_authority_cert, get_my_v3_authority_cert_m);

/* Parse certificates and keys. */ - cert1 = mock_cert = authority_cert_parse_from_string(AUTHORITY_CERT_1, NULL); + cert1 = mock_cert = authority_cert_parse_from_string(AUTHORITY_CERT_1, + strlen(AUTHORITY_CERT_1), + NULL); tt_assert(cert1); - cert2 = authority_cert_parse_from_string(AUTHORITY_CERT_2, NULL); + cert2 = authority_cert_parse_from_string(AUTHORITY_CERT_2, + strlen(AUTHORITY_CERT_2), + NULL); tt_assert(cert2); - cert3 = authority_cert_parse_from_string(AUTHORITY_CERT_3, NULL); + cert3 = authority_cert_parse_from_string(AUTHORITY_CERT_3, + strlen(AUTHORITY_CERT_3), + NULL); tt_assert(cert3); sign_skey_1 = crypto_pk_new(); sign_skey_2 = crypto_pk_new(); diff --git a/src/test/test_dir_common.c b/src/test/test_dir_common.c index 6e3bb9945..63bd082ee 100644 --- a/src/test/test_dir_common.c +++ b/src/test/test_dir_common.c @@ -40,14 +40,20 @@ dir_common_authority_pk_init(authority_cert_t **cert1, { /* Parse certificates and keys. */ authority_cert_t *cert; - cert = authority_cert_parse_from_string(AUTHORITY_CERT_1, NULL); + cert = authority_cert_parse_from_string(AUTHORITY_CERT_1, + strlen(AUTHORITY_CERT_1), + NULL); tt_assert(cert); tt_assert(cert->identity_key); *cert1 = cert; tt_assert(*cert1); - *cert2 = authority_cert_parse_from_string(AUTHORITY_CERT_2, NULL); + *cert2 = authority_cert_parse_from_string(AUTHORITY_CERT_2, + strlen(AUTHORITY_CERT_2), + NULL); tt_assert(*cert2); - *cert3 = authority_cert_parse_from_string(AUTHORITY_CERT_3, NULL); + *cert3 = authority_cert_parse_from_string(AUTHORITY_CERT_3, + strlen(AUTHORITY_CERT_3), + NULL); tt_assert(*cert3); *sign_skey_1 = crypto_pk_new(); *sign_skey_2 = crypto_pk_new(); diff --git a/src/test/test_dir_handle_get.c b/src/test/test_dir_handle_get.c index 09799a0e5..ecb7c1b5f 100644 --- a/src/test/test_dir_handle_get.c +++ b/src/test/test_dir_handle_get.c @@ -1270,7 +1270,9 @@ test_dir_handle_get_server_keys_authority(void* data) size_t body_used = 0; (void) data;

- mock_cert = authority_cert_parse_from_string(TEST_CERTIFICATE, NULL); + mock_cert = authority_cert_parse_from_string(TEST_CERTIFICATE, + strlen(TEST_CERTIFICATE), + NULL);

MOCK(get_my_v3_authority_cert, get_my_v3_authority_cert_m); MOCK(connection_write_to_buf_impl_, connection_write_to_buf_mock); @@ -1420,7 +1422,9 @@ test_dir_handle_get_server_keys_sk(void* data) size_t body_used = 0; (void) data;

- mock_cert = authority_cert_parse_from_string(TEST_CERTIFICATE, NULL); + mock_cert = authority_cert_parse_from_string(TEST_CERTIFICATE, + strlen(TEST_CERTIFICATE), + NULL); MOCK(get_my_v3_authority_cert, get_my_v3_authority_cert_m); MOCK(connection_write_to_buf_impl_, connection_write_to_buf_mock);

@@ -2388,7 +2392,9 @@ test_dir_handle_get_status_vote_next_authority(void* data) routerlist_free_all(); dirvote_free_all();

- mock_cert = authority_cert_parse_from_string(TEST_CERTIFICATE, NULL); + mock_cert = authority_cert_parse_from_string(TEST_CERTIFICATE, + strlen(TEST_CERTIFICATE), + NULL);

/* create a trusted ds */ ds = trusted_dir_server_new("ds", "127.0.0.1", 9059, 9060, NULL, digest, @@ -2466,7 +2472,9 @@ test_dir_handle_get_status_vote_current_authority(void* data) routerlist_free_all(); dirvote_free_all();

- mock_cert = authority_cert_parse_from_string(TEST_CERTIFICATE, NULL); + mock_cert = authority_cert_parse_from_string(TEST_CERTIFICATE, + strlen(TEST_CERTIFICATE), + NULL);

/* create a trusted ds */ ds = trusted_dir_server_new("ds", "127.0.0.1", 9059, 9060, NULL, digest, diff --git a/src/test/test_routerlist.c b/src/test/test_routerlist.c index bf76570b3..e7c577cf6 100644 --- a/src/test/test_routerlist.c +++ b/src/test/test_routerlist.c @@ -260,7 +260,9 @@ test_router_pick_directory_server_impl(void *arg)

/* Init SR subsystem. */ MOCK(get_my_v3_authority_cert, get_my_v3_authority_cert_m); - mock_cert = authority_cert_parse_from_string(AUTHORITY_CERT_1, NULL); + mock_cert = authority_cert_parse_from_string(AUTHORITY_CERT_1, + strlen(AUTHORITY_CERT_1), + NULL); sr_init(0); UNMOCK(get_my_v3_authority_cert);

@@ -472,7 +474,9 @@ test_directory_guard_fetch_with_no_dirinfo(void *arg)

/* Initialize the SRV subsystem */ MOCK(get_my_v3_authority_cert, get_my_v3_authority_cert_m); - mock_cert = authority_cert_parse_from_string(AUTHORITY_CERT_1, NULL); + mock_cert = authority_cert_parse_from_string(AUTHORITY_CERT_1, + strlen(AUTHORITY_CERT_1), + NULL); sr_init(0); UNMOCK(get_my_v3_authority_cert);

@@ -645,7 +649,9 @@ test_skew_common(void *arg, time_t now, unsigned long *offset)

/* Initialize the SRV subsystem */ MOCK(get_my_v3_authority_cert, get_my_v3_authority_cert_m); - mock_cert = authority_cert_parse_from_string(AUTHORITY_CERT_1, NULL); + mock_cert = authority_cert_parse_from_string(AUTHORITY_CERT_1, + strlen(AUTHORITY_CERT_1), + NULL); sr_init(0); UNMOCK(get_my_v3_authority_cert);

diff --git a/src/test/test_shared_random.c b/src/test/test_shared_random.c index 70adf580a..204361364 100644 --- a/src/test/test_shared_random.c +++ b/src/test/test_shared_random.c @@ -64,7 +64,9 @@ init_authority_state(void) MOCK(get_my_v3_authority_cert, get_my_v3_authority_cert_m);

or_options_t *options = get_options_mutable(); - mock_cert = authority_cert_parse_from_string(AUTHORITY_CERT_1, NULL); + mock_cert = authority_cert_parse_from_string(AUTHORITY_CERT_1, + strlen(AUTHORITY_CERT_1), + NULL); tt_assert(mock_cert); options->AuthoritativeDir = 1; tt_int_op(load_ed_keys(options, time(NULL)), OP_GE, 0); @@ -420,7 +422,9 @@ test_sr_commit(void *arg) { /* Setup a minimal dirauth environment for this test */ or_options_t *options = get_options_mutable();

- auth_cert = authority_cert_parse_from_string(AUTHORITY_CERT_1, NULL); + auth_cert = authority_cert_parse_from_string(AUTHORITY_CERT_1, + strlen(AUTHORITY_CERT_1), + NULL); tt_assert(auth_cert);

options->AuthoritativeDir = 1; @@ -823,7 +827,9 @@ test_sr_setup_commits(void) { /* Setup a minimal dirauth environment for this test */ or_options_t *options = get_options_mutable();

- auth_cert = authority_cert_parse_from_string(AUTHORITY_CERT_1, NULL); + auth_cert = authority_cert_parse_from_string(AUTHORITY_CERT_1, + strlen(AUTHORITY_CERT_1), + NULL); tt_assert(auth_cert);

options->AuthoritativeDir = 1;