January 2018 - tor-commits - lists.torproject.org

[tor/master] Merge remote-tracking branch 'public/ticket24849_032'
by nickm＠torproject.org 31 Jan '18

31 Jan '18

commit 80c8689be1d0b6d926752059b51966d398f7c184 Merge: 94878cf1e 75d4bd349 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Jan 31 09:38:24 2018 -0500 Merge remote-tracking branch 'public/ticket24849_032' changes/ticket24849 | 3 +++ src/or/dirvote.c | 8 +++++++- 2 files changed, 10 insertions(+), 1 deletion(-)

1 0

[tor/master] Improve log when unable to add sigs to pending consensus
by nickm＠torproject.org 31 Jan '18

31 Jan '18

commit 75d4bd34976536643b634f860b3d2c0e06d875ee Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Jan 26 14:19:59 2018 -0500 Improve log when unable to add sigs to pending consensus Closes ticket 24849. --- changes/ticket24849 | 3 +++ src/or/dirvote.c | 8 +++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/changes/ticket24849 b/changes/ticket24849 new file mode 100644 index 000000000..fd9492acb --- /dev/null +++ b/changes/ticket24849 @@ -0,0 +1,3 @@ + o Minor features (directory authority): + - When unable to add signatures to a pending consensus, log the reason + why. Closes ticket 24849. diff --git a/src/or/dirvote.c b/src/or/dirvote.c index ce82a5ef4..e57f6d46f 100644 --- a/src/or/dirvote.c +++ b/src/or/dirvote.c @@ -3551,7 +3551,13 @@ dirvote_add_signatures_to_pending_consensus( } r = networkstatus_add_detached_signatures(pc->consensus, sigs, source, severity, msg_out); - log_info(LD_DIR,"Added %d signatures to consensus.", r); + if (r >= 0) { + log_info(LD_DIR,"Added %d signatures to consensus.", r); + } else { + log_fn(LOG_PROTOCOL_WARN, LD_DIR, + "Unable to add signatures to consensus: %s", + *msg_out ? *msg_out : "(unknown)"); + } if (r >= 1) { char *new_signatures =

1 0

[tor/master] Merge remote-tracking branch 'dgoulet/ticket24902_029_05'
by nickm＠torproject.org 31 Jan '18

31 Jan '18

commit 94878cf1eaa59f4aae6bc88c93d55c4ecba1e0d9 Merge: c0447033f 1f4a73133 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Jan 31 09:35:07 2018 -0500 Merge remote-tracking branch 'dgoulet/ticket24902_029_05' src/or/dos.c | 82 ++++++++++++++++++++--------- src/or/dos.h | 2 +- src/test/test_dos.c | 148 +++++++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 205 insertions(+), 27 deletions(-)

1 0

[tor/master] test: Add unit tests for overflows and underflows in cc_stats_refill_bucket
by nickm＠torproject.org 31 Jan '18

31 Jan '18

commit 1f4a73133cf864774c017e2c50b347727519c18f Author: teor <teor2345(a)gmail.com> Date: Wed Jan 31 11:22:20 2018 +1100 test: Add unit tests for overflows and underflows in cc_stats_refill_bucket Closes #25094. Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/test/test_dos.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 146 insertions(+) diff --git a/src/test/test_dos.c b/src/test/test_dos.c index 7fe549560..071926c28 100644 --- a/src/test/test_dos.c +++ b/src/test/test_dos.c @@ -234,6 +234,152 @@ test_dos_bucket_refill(void *arg) current_circ_count += max_circuit_count; tt_uint_op(dos_stats->cc_stats.circuit_bucket, OP_EQ, current_circ_count); + /* Now send as many CREATE cells as needed to deplete our token bucket + * completely */ + for (; current_circ_count != 0; current_circ_count--) { + dos_cc_new_create_cell(chan); + } + tt_uint_op(current_circ_count, OP_EQ, 0); + tt_uint_op(dos_stats->cc_stats.circuit_bucket, OP_EQ, current_circ_count); + + /* Now use a very large time, and check that the token bucket does not have + * more than max_circs allowance, even tho we let it simmer for so long. */ + now = INT32_MAX; /* 2038? */ + update_approx_time(now); + cc_stats_refill_bucket(&dos_stats->cc_stats, addr); + current_circ_count += max_circuit_count; + tt_uint_op(dos_stats->cc_stats.circuit_bucket, OP_EQ, current_circ_count); + + /* Now send as many CREATE cells as needed to deplete our token bucket + * completely */ + for (; current_circ_count != 0; current_circ_count--) { + dos_cc_new_create_cell(chan); + } + tt_uint_op(current_circ_count, OP_EQ, 0); + tt_uint_op(dos_stats->cc_stats.circuit_bucket, OP_EQ, current_circ_count); + + /* Now use a very small time, and check that the token bucket has exactly + * the max_circs allowance, because backward clock jumps are rare. */ + now = INT32_MIN; /* 19?? */ + update_approx_time(now); + cc_stats_refill_bucket(&dos_stats->cc_stats, addr); + current_circ_count += max_circuit_count; + tt_uint_op(dos_stats->cc_stats.circuit_bucket, OP_EQ, current_circ_count); + + /* Now send as many CREATE cells as needed to deplete our token bucket + * completely */ + for (; current_circ_count != 0; current_circ_count--) { + dos_cc_new_create_cell(chan); + } + tt_uint_op(current_circ_count, OP_EQ, 0); + tt_uint_op(dos_stats->cc_stats.circuit_bucket, OP_EQ, current_circ_count); + + /* Progress time forward one sec again, refill the bucket and check that the + * refill happened correctly. */ + now += 1; + update_approx_time(now); + cc_stats_refill_bucket(&dos_stats->cc_stats, addr); + /* check refill */ + current_circ_count += circ_rate; + tt_uint_op(dos_stats->cc_stats.circuit_bucket, OP_EQ, current_circ_count); + + /* Now send as many CREATE cells as needed to deplete our token bucket + * completely */ + for (; current_circ_count != 0; current_circ_count--) { + dos_cc_new_create_cell(chan); + } + tt_uint_op(current_circ_count, OP_EQ, 0); + tt_uint_op(dos_stats->cc_stats.circuit_bucket, OP_EQ, current_circ_count); + + /* Now use a very large time (again), and check that the token bucket does + * not have more than max_circs allowance, even tho we let it simmer for so + * long. */ + now = INT32_MAX; /* 2038? */ + update_approx_time(now); + cc_stats_refill_bucket(&dos_stats->cc_stats, addr); + current_circ_count += max_circuit_count; + tt_uint_op(dos_stats->cc_stats.circuit_bucket, OP_EQ, current_circ_count); + + /* Now send as many CREATE cells as needed to deplete our token bucket + * completely */ + for (; current_circ_count != 0; current_circ_count--) { + dos_cc_new_create_cell(chan); + } + tt_uint_op(current_circ_count, OP_EQ, 0); + tt_uint_op(dos_stats->cc_stats.circuit_bucket, OP_EQ, current_circ_count); + + /* This code resets the time to zero with 32-bit time_t, which triggers the + * code that initialises the bucket. */ +#if SIZEOF_TIME_T == 8 + /* Now use a very very small time, and check that the token bucket has + * exactly the max_circs allowance, because backward clock jumps are rare. + */ + now = (time_t)INT64_MIN; /* ???? */ + update_approx_time(now); + cc_stats_refill_bucket(&dos_stats->cc_stats, addr); + current_circ_count += max_circuit_count; + tt_uint_op(dos_stats->cc_stats.circuit_bucket, OP_EQ, current_circ_count); + + /* Now send as many CREATE cells as needed to deplete our token bucket + * completely */ + for (; current_circ_count != 0; current_circ_count--) { + dos_cc_new_create_cell(chan); + } + tt_uint_op(current_circ_count, OP_EQ, 0); + tt_uint_op(dos_stats->cc_stats.circuit_bucket, OP_EQ, current_circ_count); + + /* Progress time forward one sec again, refill the bucket and check that the + * refill happened correctly. */ + now += 1; + update_approx_time(now); + cc_stats_refill_bucket(&dos_stats->cc_stats, addr); + /* check refill */ + current_circ_count += circ_rate; + tt_uint_op(dos_stats->cc_stats.circuit_bucket, OP_EQ, current_circ_count); + + /* Now send as many CREATE cells as needed to deplete our token bucket + * completely */ + for (; current_circ_count != 0; current_circ_count--) { + dos_cc_new_create_cell(chan); + } + tt_uint_op(current_circ_count, OP_EQ, 0); + tt_uint_op(dos_stats->cc_stats.circuit_bucket, OP_EQ, current_circ_count); + + /* Now use a very very small time, and check that the token bucket has + * exactly the max_circs allowance, because backward clock jumps are rare. + */ + now = (time_t)INT64_MIN; /* ???? */ + update_approx_time(now); + cc_stats_refill_bucket(&dos_stats->cc_stats, addr); + current_circ_count += max_circuit_count; + tt_uint_op(dos_stats->cc_stats.circuit_bucket, OP_EQ, current_circ_count); + + /* Now send as many CREATE cells as needed to deplete our token bucket + * completely */ + for (; current_circ_count != 0; current_circ_count--) { + dos_cc_new_create_cell(chan); + } + tt_uint_op(current_circ_count, OP_EQ, 0); + tt_uint_op(dos_stats->cc_stats.circuit_bucket, OP_EQ, current_circ_count); + + /* Now use a very very large time, and check that the token bucket does not + * have more than max_circs allowance, even tho we let it simmer for so + * long. */ + now = (time_t)INT64_MAX; /* ???? */ + update_approx_time(now); + cc_stats_refill_bucket(&dos_stats->cc_stats, addr); + current_circ_count += max_circuit_count; + tt_uint_op(dos_stats->cc_stats.circuit_bucket, OP_EQ, current_circ_count); + + /* Now send as many CREATE cells as needed to deplete our token bucket + * completely */ + for (; current_circ_count != 0; current_circ_count--) { + dos_cc_new_create_cell(chan); + } + tt_uint_op(current_circ_count, OP_EQ, 0); + tt_uint_op(dos_stats->cc_stats.circuit_bucket, OP_EQ, current_circ_count); +#endif + done: tor_free(chan); dos_free_all();

1 0

[tor/master] test: Remove a redundant round from test_dos_bucket_refill
by nickm＠torproject.org 31 Jan '18

31 Jan '18

commit b45ae1b00237f0209c4ccce777de59581fda5e39 Author: teor <teor2345(a)gmail.com> Date: Wed Jan 31 11:11:08 2018 +1100 test: Remove a redundant round from test_dos_bucket_refill This round is left over from the tenths of a second code. Part of #25094. --- src/test/test_dos.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/test_dos.c b/src/test/test_dos.c index 5a8474ad8..80abc1937 100644 --- a/src/test/test_dos.c +++ b/src/test/test_dos.c @@ -176,7 +176,7 @@ test_dos_bucket_refill(void *arg) /* Initialize DoS subsystem and get relevant limits */ dos_init(); uint32_t max_circuit_count = get_param_cc_circuit_burst(NULL); - int circ_rate = tor_lround(get_circuit_rate_per_second()); + int circ_rate = get_circuit_rate_per_second(); /* Check that the circuit rate is a positive number and smaller than the max * circuit count */ tt_int_op(circ_rate, OP_GT, 1);

1 0

[tor/master] dos: Make sure cc_stats_refill_bucket can't overflow while calculating
by nickm＠torproject.org 31 Jan '18

31 Jan '18

commit a09d5f5735abe2e1d16cf0ee9389ae096d5e7ef1 Author: teor <teor2345(a)gmail.com> Date: Wed Jan 31 11:13:17 2018 +1100 dos: Make sure cc_stats_refill_bucket can't overflow while calculating Debug log the elapsed time in cc_stats_refill_bucket Part of #25094. Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/dos.c | 82 +++++++++++++++++++++++++++++++++++++---------------- src/or/dos.h | 2 +- src/test/test_dos.c | 2 +- 3 files changed, 59 insertions(+), 27 deletions(-) diff --git a/src/or/dos.c b/src/or/dos.c index a614d1231..c221e5ecd 100644 --- a/src/or/dos.c +++ b/src/or/dos.c @@ -222,7 +222,7 @@ cc_consensus_has_changed(const networkstatus_t *ns) /** Return the number of circuits we allow per second under the current * configuration. */ -STATIC uint32_t +STATIC uint64_t get_circuit_rate_per_second(void) { return dos_cc_circuit_rate; @@ -234,31 +234,40 @@ get_circuit_rate_per_second(void) STATIC void cc_stats_refill_bucket(cc_client_stats_t *stats, const tor_addr_t *addr) { - uint32_t new_circuit_bucket_count, circuit_rate = 0, num_token; - time_t now, elapsed_time_last_refill; + uint32_t new_circuit_bucket_count; + uint64_t num_token, elapsed_time_last_refill = 0, circuit_rate = 0; + time_t now; + int64_t last_refill_ts; tor_assert(stats); tor_assert(addr); now = approx_time(); + last_refill_ts = (int64_t)stats->last_circ_bucket_refill_ts; - /* We've never filled the bucket so fill it with the maximum being the burst - * and we are done. */ - if (stats->last_circ_bucket_refill_ts == 0) { - num_token = dos_cc_circuit_burst; - goto end; + /* If less than a second has elapsed, don't add any tokens. + * Note: If a relay's clock is ever 0, any new clients won't get a refill + * until the next second. But a relay that thinks it is 1970 will never + * validate the public consensus. */ + if ((int64_t)now == last_refill_ts) { + goto done; } /* At this point, we know we might need to add token to the bucket. We'll - * first compute the circuit rate that is how many circuit are we allowed to - * do per second. */ + * first get the circuit rate that is how many circuit are we allowed to do + * per second. */ circuit_rate = get_circuit_rate_per_second(); - /* How many seconds have elapsed between now and the last refill? */ - elapsed_time_last_refill = now - stats->last_circ_bucket_refill_ts; + /* We've never filled the bucket so fill it with the maximum being the burst + * and we are done. + * Note: If a relay's clock is ever 0, all clients that were last refilled + * in that zero second will get a full refill here. */ + if (last_refill_ts == 0) { + num_token = dos_cc_circuit_burst; + goto end; + } - /* If the elapsed time is below 0 it means our clock jumped backward so in - * that case, lets be safe and fill it up to the maximum. Not filling it + /* Our clock jumped backward so fill it up to the maximum. Not filling it * could trigger a detection for a valid client. Also, if the clock jumped * negative but we didn't notice until the elapsed time became positive * again, then we potentially spent many seconds not refilling the bucket @@ -266,28 +275,51 @@ cc_stats_refill_bucket(cc_client_stats_t *stats, const tor_addr_t *addr) * until now means that no circuit creation requests came in during that * time, so the client doesn't end up punished that much from this hopefully * rare situation.*/ - if (elapsed_time_last_refill < 0) { - /* Dividing the burst by the circuit rate gives us the time span that will - * give us the maximum allowed value of token. */ - elapsed_time_last_refill = (dos_cc_circuit_burst / circuit_rate); + if ((int64_t)now < last_refill_ts) { + /* Use the maximum allowed value of token. */ + num_token = dos_cc_circuit_burst; + goto end; + } + + /* How many seconds have elapsed between now and the last refill? + * This subtraction can't underflow, because now >= last_refill_ts. + * And it can't overflow, because INT64_MAX - (-INT64_MIN) == UINT64_MAX. */ + elapsed_time_last_refill = (uint64_t)now - last_refill_ts; + + /* If the elapsed time is very large, it means our clock jumped forward. + * If the multiplication would overflow, use the maximum allowed value. */ + if (elapsed_time_last_refill > UINT32_MAX) { + num_token = dos_cc_circuit_burst; + goto end; } /* Compute how many circuits we are allowed in that time frame which we'll - * add to the bucket. This can be big but it is cap to a maximum after. */ + * add to the bucket. This can't overflow, because both multiplicands + * are less than or equal to UINT32_MAX, and num_token is uint64_t. */ num_token = elapsed_time_last_refill * circuit_rate; end: - /* We cap the bucket to the burst value else this could grow to infinity - * over time. */ - new_circuit_bucket_count = MIN(stats->circuit_bucket + num_token, - dos_cc_circuit_burst); + /* If the sum would overflow, use the maximum allowed value. */ + if (num_token > UINT32_MAX - stats->circuit_bucket) { + new_circuit_bucket_count = dos_cc_circuit_burst; + } else { + /* We cap the bucket to the burst value else this could overflow uint32_t + * over time. */ + new_circuit_bucket_count = MIN(stats->circuit_bucket + (uint32_t)num_token, + dos_cc_circuit_burst); + } + /* This function is not allowed to make the bucket count smaller */ + tor_assert_nonfatal(new_circuit_bucket_count >= stats->circuit_bucket); log_debug(LD_DOS, "DoS address %s has its circuit bucket value: %" PRIu32 - ". Filling it to %" PRIu32 ". Circuit rate is %" PRIu32, + ". Filling it to %" PRIu32 ". Circuit rate is %" PRIu64 + ". Elapsed time is %" PRIi64, fmt_addr(addr), stats->circuit_bucket, new_circuit_bucket_count, - circuit_rate); + circuit_rate, (int64_t)elapsed_time_last_refill); stats->circuit_bucket = new_circuit_bucket_count; stats->last_circ_bucket_refill_ts = now; + + done: return; } diff --git a/src/or/dos.h b/src/or/dos.h index 8695512ea..5d35a2b12 100644 --- a/src/or/dos.h +++ b/src/or/dos.h @@ -125,7 +125,7 @@ STATIC uint32_t get_param_cc_circuit_burst(const networkstatus_t *ns); STATIC uint32_t get_param_cc_min_concurrent_connection( const networkstatus_t *ns); -STATIC uint32_t get_circuit_rate_per_second(void); +STATIC uint64_t get_circuit_rate_per_second(void); STATIC void cc_stats_refill_bucket(cc_client_stats_t *stats, const tor_addr_t *addr); diff --git a/src/test/test_dos.c b/src/test/test_dos.c index 80abc1937..7fe549560 100644 --- a/src/test/test_dos.c +++ b/src/test/test_dos.c @@ -176,7 +176,7 @@ test_dos_bucket_refill(void *arg) /* Initialize DoS subsystem and get relevant limits */ dos_init(); uint32_t max_circuit_count = get_param_cc_circuit_burst(NULL); - int circ_rate = get_circuit_rate_per_second(); + uint64_t circ_rate = get_circuit_rate_per_second(); /* Check that the circuit rate is a positive number and smaller than the max * circuit count */ tt_int_op(circ_rate, OP_GT, 1);

1 0

[tor/maint-0.3.2] When a tor_cert_T check fails, log the reason why.
by nickm＠torproject.org 31 Jan '18

31 Jan '18

commit ee5c624bebbc8e823ed49fe563fd56294fb226d4 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Jan 26 13:55:25 2018 -0500 When a tor_cert_T check fails, log the reason why. Diagnostic attempt for 24972. --- changes/bug24972 | 4 ++++ src/or/hs_client.c | 6 ++++-- src/or/hs_descriptor.c | 9 ++++++--- src/or/routerkeys.c | 11 ++++++++--- src/or/torcert.c | 21 ++++++++++++++++++++- src/or/torcert.h | 1 + 6 files changed, 43 insertions(+), 9 deletions(-) diff --git a/changes/bug24972 b/changes/bug24972 new file mode 100644 index 000000000..5adf970ab --- /dev/null +++ b/changes/bug24972 @@ -0,0 +1,4 @@ + o Minor features (logging, diagnostic): + - When logging a failure to check a hidden service's certificate, + also log what the problem with the certificate was. Diagnostic + for ticket 24972. diff --git a/src/or/hs_client.c b/src/or/hs_client.c index 9ac653c72..551cf5055 100644 --- a/src/or/hs_client.c +++ b/src/or/hs_client.c @@ -1229,10 +1229,12 @@ hs_client_decode_descriptor(const char *desc_str, /* Make sure the descriptor signing key cross certifies with the computed * blinded key. Without this validation, anyone knowing the subcredential * and onion address can forge a descriptor. */ - if (tor_cert_checksig((*desc)->plaintext_data.signing_key_cert, + tor_cert_t *cert = (*desc)->plaintext_data.signing_key_cert; + if (tor_cert_checksig(cert, &blinded_pubkey, approx_time()) < 0) { log_warn(LD_GENERAL, "Descriptor signing key certificate signature " - "doesn't validate with computed blinded key."); + "doesn't validate with computed blinded key: %s", + tor_cert_describe_signature_status(cert)); goto err; } diff --git a/src/or/hs_descriptor.c b/src/or/hs_descriptor.c index 170886694..9683fca50 100644 --- a/src/or/hs_descriptor.c +++ b/src/or/hs_descriptor.c @@ -1233,7 +1233,8 @@ cert_is_valid(tor_cert_t *cert, uint8_t type, const char *log_obj_type) /* The following will not only check if the signature matches but also the * expiration date and overall validity. */ if (tor_cert_checksig(cert, &cert->signing_key, approx_time()) < 0) { - log_warn(LD_REND, "Invalid signature for %s.", log_obj_type); + log_warn(LD_REND, "Invalid signature for %s: %s", log_obj_type, + tor_cert_describe_signature_status(cert)); goto err; } @@ -1728,7 +1729,8 @@ decode_introduction_point(const hs_descriptor_t *desc, const char *start) /* Validate authentication certificate with descriptor signing key. */ if (tor_cert_checksig(ip->auth_key_cert, &desc->plaintext_data.signing_pubkey, 0) < 0) { - log_warn(LD_REND, "Invalid authentication key signature"); + log_warn(LD_REND, "Invalid authentication key signature: %s", + tor_cert_describe_signature_status(ip->auth_key_cert)); goto err; } @@ -1765,7 +1767,8 @@ decode_introduction_point(const hs_descriptor_t *desc, const char *start) } if (tor_cert_checksig(ip->enc_key_cert, &desc->plaintext_data.signing_pubkey, 0) < 0) { - log_warn(LD_REND, "Invalid encryption key signature"); + log_warn(LD_REND, "Invalid encryption key signature: %s", + tor_cert_describe_signature_status(ip->enc_key_cert)); goto err; } /* It is successfully cross certified. Flag the object. */ diff --git a/src/or/routerkeys.c b/src/or/routerkeys.c index 7295c1965..f0973044b 100644 --- a/src/or/routerkeys.c +++ b/src/or/routerkeys.c @@ -536,7 +536,8 @@ ed_key_init_from_file(const char *fname, uint32_t flags, bad_cert = 1; } else if (signing_key && tor_cert_checksig(cert, &signing_key->pubkey, now) < 0) { - tor_log(severity, LD_OR, "Can't check certificate"); + tor_log(severity, LD_OR, "Can't check certificate: %s", + tor_cert_describe_signature_status(cert)); bad_cert = 1; } else if (cert->cert_expired) { tor_log(severity, LD_OR, "Certificate is expired"); @@ -883,8 +884,12 @@ load_ed_keys(const or_options_t *options, time_t now) if (! ed25519_pubkey_eq(&sign_cert->signing_key, &id->pubkey)) FAIL("The signing cert we have was not signed with the master key " "we loaded!"); - if (tor_cert_checksig(sign_cert, &id->pubkey, 0) < 0) - FAIL("The signing cert we loaded was not signed correctly!"); + if (tor_cert_checksig(sign_cert, &id->pubkey, 0) < 0) { + log_warn(LD_OR, "The signing cert we loaded was not signed " + "correctly: %s!", + tor_cert_describe_signature_status(sign_cert)); + goto err; + } } if (want_new_signing_key && sign_signing_key_with_id) { diff --git a/src/or/torcert.c b/src/or/torcert.c index befb39d6e..212534d31 100644 --- a/src/or/torcert.c +++ b/src/or/torcert.c @@ -93,7 +93,8 @@ tor_cert_sign_impl(const ed25519_keypair_t *signing_key, if (tor_cert_checksig(torcert, &signing_key->pubkey, now) < 0) { /* LCOV_EXCL_START */ - log_warn(LD_BUG, "Generated a certificate whose signature we can't check"); + log_warn(LD_BUG, "Generated a certificate whose signature we can't " + "check: %s", tor_cert_describe_signature_status(torcert)); goto err; /* LCOV_EXCL_STOP */ } @@ -267,6 +268,24 @@ tor_cert_checksig(tor_cert_t *cert, } } +/** Return a string describing the status of the signature on cert + * + * Will always be "unchecked" unless tor_cert_checksig has been called. + */ +const char * +tor_cert_describe_signature_status(const tor_cert_t *cert) +{ + if (cert->cert_expired) { + return "expired"; + } else if (cert->sig_bad) { + return "mis-signed"; + } else if (cert->sig_ok) { + return "okay"; + } else { + return "unchecked"; + } +} + /** Return a new copy of cert */ tor_cert_t * tor_cert_dup(const tor_cert_t *cert) diff --git a/src/or/torcert.h b/src/or/torcert.h index c77ae2089..ac227db20 100644 --- a/src/or/torcert.h +++ b/src/or/torcert.h @@ -66,6 +66,7 @@ int tor_cert_get_checkable_sig(ed25519_checkable_t *checkable_out, int tor_cert_checksig(tor_cert_t *cert, const ed25519_public_key_t *pubkey, time_t now); +const char *tor_cert_describe_signature_status(const tor_cert_t *cert); tor_cert_t *tor_cert_dup(const tor_cert_t *cert); int tor_cert_eq(const tor_cert_t *cert1, const tor_cert_t *cert2);

1 0

[tor/release-0.3.2] When a tor_cert_T check fails, log the reason why.
by nickm＠torproject.org 31 Jan '18

31 Jan '18

commit ee5c624bebbc8e823ed49fe563fd56294fb226d4 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Jan 26 13:55:25 2018 -0500 When a tor_cert_T check fails, log the reason why. Diagnostic attempt for 24972. --- changes/bug24972 | 4 ++++ src/or/hs_client.c | 6 ++++-- src/or/hs_descriptor.c | 9 ++++++--- src/or/routerkeys.c | 11 ++++++++--- src/or/torcert.c | 21 ++++++++++++++++++++- src/or/torcert.h | 1 + 6 files changed, 43 insertions(+), 9 deletions(-) diff --git a/changes/bug24972 b/changes/bug24972 new file mode 100644 index 000000000..5adf970ab --- /dev/null +++ b/changes/bug24972 @@ -0,0 +1,4 @@ + o Minor features (logging, diagnostic): + - When logging a failure to check a hidden service's certificate, + also log what the problem with the certificate was. Diagnostic + for ticket 24972. diff --git a/src/or/hs_client.c b/src/or/hs_client.c index 9ac653c72..551cf5055 100644 --- a/src/or/hs_client.c +++ b/src/or/hs_client.c @@ -1229,10 +1229,12 @@ hs_client_decode_descriptor(const char *desc_str, /* Make sure the descriptor signing key cross certifies with the computed * blinded key. Without this validation, anyone knowing the subcredential * and onion address can forge a descriptor. */ - if (tor_cert_checksig((*desc)->plaintext_data.signing_key_cert, + tor_cert_t *cert = (*desc)->plaintext_data.signing_key_cert; + if (tor_cert_checksig(cert, &blinded_pubkey, approx_time()) < 0) { log_warn(LD_GENERAL, "Descriptor signing key certificate signature " - "doesn't validate with computed blinded key."); + "doesn't validate with computed blinded key: %s", + tor_cert_describe_signature_status(cert)); goto err; } diff --git a/src/or/hs_descriptor.c b/src/or/hs_descriptor.c index 170886694..9683fca50 100644 --- a/src/or/hs_descriptor.c +++ b/src/or/hs_descriptor.c @@ -1233,7 +1233,8 @@ cert_is_valid(tor_cert_t *cert, uint8_t type, const char *log_obj_type) /* The following will not only check if the signature matches but also the * expiration date and overall validity. */ if (tor_cert_checksig(cert, &cert->signing_key, approx_time()) < 0) { - log_warn(LD_REND, "Invalid signature for %s.", log_obj_type); + log_warn(LD_REND, "Invalid signature for %s: %s", log_obj_type, + tor_cert_describe_signature_status(cert)); goto err; } @@ -1728,7 +1729,8 @@ decode_introduction_point(const hs_descriptor_t *desc, const char *start) /* Validate authentication certificate with descriptor signing key. */ if (tor_cert_checksig(ip->auth_key_cert, &desc->plaintext_data.signing_pubkey, 0) < 0) { - log_warn(LD_REND, "Invalid authentication key signature"); + log_warn(LD_REND, "Invalid authentication key signature: %s", + tor_cert_describe_signature_status(ip->auth_key_cert)); goto err; } @@ -1765,7 +1767,8 @@ decode_introduction_point(const hs_descriptor_t *desc, const char *start) } if (tor_cert_checksig(ip->enc_key_cert, &desc->plaintext_data.signing_pubkey, 0) < 0) { - log_warn(LD_REND, "Invalid encryption key signature"); + log_warn(LD_REND, "Invalid encryption key signature: %s", + tor_cert_describe_signature_status(ip->enc_key_cert)); goto err; } /* It is successfully cross certified. Flag the object. */ diff --git a/src/or/routerkeys.c b/src/or/routerkeys.c index 7295c1965..f0973044b 100644 --- a/src/or/routerkeys.c +++ b/src/or/routerkeys.c @@ -536,7 +536,8 @@ ed_key_init_from_file(const char *fname, uint32_t flags, bad_cert = 1; } else if (signing_key && tor_cert_checksig(cert, &signing_key->pubkey, now) < 0) { - tor_log(severity, LD_OR, "Can't check certificate"); + tor_log(severity, LD_OR, "Can't check certificate: %s", + tor_cert_describe_signature_status(cert)); bad_cert = 1; } else if (cert->cert_expired) { tor_log(severity, LD_OR, "Certificate is expired"); @@ -883,8 +884,12 @@ load_ed_keys(const or_options_t *options, time_t now) if (! ed25519_pubkey_eq(&sign_cert->signing_key, &id->pubkey)) FAIL("The signing cert we have was not signed with the master key " "we loaded!"); - if (tor_cert_checksig(sign_cert, &id->pubkey, 0) < 0) - FAIL("The signing cert we loaded was not signed correctly!"); + if (tor_cert_checksig(sign_cert, &id->pubkey, 0) < 0) { + log_warn(LD_OR, "The signing cert we loaded was not signed " + "correctly: %s!", + tor_cert_describe_signature_status(sign_cert)); + goto err; + } } if (want_new_signing_key && sign_signing_key_with_id) { diff --git a/src/or/torcert.c b/src/or/torcert.c index befb39d6e..212534d31 100644 --- a/src/or/torcert.c +++ b/src/or/torcert.c @@ -93,7 +93,8 @@ tor_cert_sign_impl(const ed25519_keypair_t *signing_key, if (tor_cert_checksig(torcert, &signing_key->pubkey, now) < 0) { /* LCOV_EXCL_START */ - log_warn(LD_BUG, "Generated a certificate whose signature we can't check"); + log_warn(LD_BUG, "Generated a certificate whose signature we can't " + "check: %s", tor_cert_describe_signature_status(torcert)); goto err; /* LCOV_EXCL_STOP */ } @@ -267,6 +268,24 @@ tor_cert_checksig(tor_cert_t *cert, } } +/** Return a string describing the status of the signature on cert + * + * Will always be "unchecked" unless tor_cert_checksig has been called. + */ +const char * +tor_cert_describe_signature_status(const tor_cert_t *cert) +{ + if (cert->cert_expired) { + return "expired"; + } else if (cert->sig_bad) { + return "mis-signed"; + } else if (cert->sig_ok) { + return "okay"; + } else { + return "unchecked"; + } +} + /** Return a new copy of cert */ tor_cert_t * tor_cert_dup(const tor_cert_t *cert) diff --git a/src/or/torcert.h b/src/or/torcert.h index c77ae2089..ac227db20 100644 --- a/src/or/torcert.h +++ b/src/or/torcert.h @@ -66,6 +66,7 @@ int tor_cert_get_checkable_sig(ed25519_checkable_t *checkable_out, int tor_cert_checksig(tor_cert_t *cert, const ed25519_public_key_t *pubkey, time_t now); +const char *tor_cert_describe_signature_status(const tor_cert_t *cert); tor_cert_t *tor_cert_dup(const tor_cert_t *cert); int tor_cert_eq(const tor_cert_t *cert1, const tor_cert_t *cert2);

1 0

[tor/master] When a tor_cert_T check fails, log the reason why.
by nickm＠torproject.org 31 Jan '18

31 Jan '18

commit ee5c624bebbc8e823ed49fe563fd56294fb226d4 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Jan 26 13:55:25 2018 -0500 When a tor_cert_T check fails, log the reason why. Diagnostic attempt for 24972. --- changes/bug24972 | 4 ++++ src/or/hs_client.c | 6 ++++-- src/or/hs_descriptor.c | 9 ++++++--- src/or/routerkeys.c | 11 ++++++++--- src/or/torcert.c | 21 ++++++++++++++++++++- src/or/torcert.h | 1 + 6 files changed, 43 insertions(+), 9 deletions(-) diff --git a/changes/bug24972 b/changes/bug24972 new file mode 100644 index 000000000..5adf970ab --- /dev/null +++ b/changes/bug24972 @@ -0,0 +1,4 @@ + o Minor features (logging, diagnostic): + - When logging a failure to check a hidden service's certificate, + also log what the problem with the certificate was. Diagnostic + for ticket 24972. diff --git a/src/or/hs_client.c b/src/or/hs_client.c index 9ac653c72..551cf5055 100644 --- a/src/or/hs_client.c +++ b/src/or/hs_client.c @@ -1229,10 +1229,12 @@ hs_client_decode_descriptor(const char *desc_str, /* Make sure the descriptor signing key cross certifies with the computed * blinded key. Without this validation, anyone knowing the subcredential * and onion address can forge a descriptor. */ - if (tor_cert_checksig((*desc)->plaintext_data.signing_key_cert, + tor_cert_t *cert = (*desc)->plaintext_data.signing_key_cert; + if (tor_cert_checksig(cert, &blinded_pubkey, approx_time()) < 0) { log_warn(LD_GENERAL, "Descriptor signing key certificate signature " - "doesn't validate with computed blinded key."); + "doesn't validate with computed blinded key: %s", + tor_cert_describe_signature_status(cert)); goto err; } diff --git a/src/or/hs_descriptor.c b/src/or/hs_descriptor.c index 170886694..9683fca50 100644 --- a/src/or/hs_descriptor.c +++ b/src/or/hs_descriptor.c @@ -1233,7 +1233,8 @@ cert_is_valid(tor_cert_t *cert, uint8_t type, const char *log_obj_type) /* The following will not only check if the signature matches but also the * expiration date and overall validity. */ if (tor_cert_checksig(cert, &cert->signing_key, approx_time()) < 0) { - log_warn(LD_REND, "Invalid signature for %s.", log_obj_type); + log_warn(LD_REND, "Invalid signature for %s: %s", log_obj_type, + tor_cert_describe_signature_status(cert)); goto err; } @@ -1728,7 +1729,8 @@ decode_introduction_point(const hs_descriptor_t *desc, const char *start) /* Validate authentication certificate with descriptor signing key. */ if (tor_cert_checksig(ip->auth_key_cert, &desc->plaintext_data.signing_pubkey, 0) < 0) { - log_warn(LD_REND, "Invalid authentication key signature"); + log_warn(LD_REND, "Invalid authentication key signature: %s", + tor_cert_describe_signature_status(ip->auth_key_cert)); goto err; } @@ -1765,7 +1767,8 @@ decode_introduction_point(const hs_descriptor_t *desc, const char *start) } if (tor_cert_checksig(ip->enc_key_cert, &desc->plaintext_data.signing_pubkey, 0) < 0) { - log_warn(LD_REND, "Invalid encryption key signature"); + log_warn(LD_REND, "Invalid encryption key signature: %s", + tor_cert_describe_signature_status(ip->enc_key_cert)); goto err; } /* It is successfully cross certified. Flag the object. */ diff --git a/src/or/routerkeys.c b/src/or/routerkeys.c index 7295c1965..f0973044b 100644 --- a/src/or/routerkeys.c +++ b/src/or/routerkeys.c @@ -536,7 +536,8 @@ ed_key_init_from_file(const char *fname, uint32_t flags, bad_cert = 1; } else if (signing_key && tor_cert_checksig(cert, &signing_key->pubkey, now) < 0) { - tor_log(severity, LD_OR, "Can't check certificate"); + tor_log(severity, LD_OR, "Can't check certificate: %s", + tor_cert_describe_signature_status(cert)); bad_cert = 1; } else if (cert->cert_expired) { tor_log(severity, LD_OR, "Certificate is expired"); @@ -883,8 +884,12 @@ load_ed_keys(const or_options_t *options, time_t now) if (! ed25519_pubkey_eq(&sign_cert->signing_key, &id->pubkey)) FAIL("The signing cert we have was not signed with the master key " "we loaded!"); - if (tor_cert_checksig(sign_cert, &id->pubkey, 0) < 0) - FAIL("The signing cert we loaded was not signed correctly!"); + if (tor_cert_checksig(sign_cert, &id->pubkey, 0) < 0) { + log_warn(LD_OR, "The signing cert we loaded was not signed " + "correctly: %s!", + tor_cert_describe_signature_status(sign_cert)); + goto err; + } } if (want_new_signing_key && sign_signing_key_with_id) { diff --git a/src/or/torcert.c b/src/or/torcert.c index befb39d6e..212534d31 100644 --- a/src/or/torcert.c +++ b/src/or/torcert.c @@ -93,7 +93,8 @@ tor_cert_sign_impl(const ed25519_keypair_t *signing_key, if (tor_cert_checksig(torcert, &signing_key->pubkey, now) < 0) { /* LCOV_EXCL_START */ - log_warn(LD_BUG, "Generated a certificate whose signature we can't check"); + log_warn(LD_BUG, "Generated a certificate whose signature we can't " + "check: %s", tor_cert_describe_signature_status(torcert)); goto err; /* LCOV_EXCL_STOP */ } @@ -267,6 +268,24 @@ tor_cert_checksig(tor_cert_t *cert, } } +/** Return a string describing the status of the signature on cert + * + * Will always be "unchecked" unless tor_cert_checksig has been called. + */ +const char * +tor_cert_describe_signature_status(const tor_cert_t *cert) +{ + if (cert->cert_expired) { + return "expired"; + } else if (cert->sig_bad) { + return "mis-signed"; + } else if (cert->sig_ok) { + return "okay"; + } else { + return "unchecked"; + } +} + /** Return a new copy of cert */ tor_cert_t * tor_cert_dup(const tor_cert_t *cert) diff --git a/src/or/torcert.h b/src/or/torcert.h index c77ae2089..ac227db20 100644 --- a/src/or/torcert.h +++ b/src/or/torcert.h @@ -66,6 +66,7 @@ int tor_cert_get_checkable_sig(ed25519_checkable_t *checkable_out, int tor_cert_checksig(tor_cert_t *cert, const ed25519_public_key_t *pubkey, time_t now); +const char *tor_cert_describe_signature_status(const tor_cert_t *cert); tor_cert_t *tor_cert_dup(const tor_cert_t *cert); int tor_cert_eq(const tor_cert_t *cert1, const tor_cert_t *cert2);

1 0

[tor/master] Merge branch 'maint-0.3.2'
by nickm＠torproject.org 31 Jan '18

31 Jan '18

commit c0447033f5e1032be379b9b78d9085f71fd51bd6 Merge: d2ae1bfcb ee5c624be Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Jan 31 09:19:55 2018 -0500 Merge branch 'maint-0.3.2' changes/bug24972 | 4 ++++ src/or/hs_client.c | 6 ++++-- src/or/hs_descriptor.c | 9 ++++++--- src/or/routerkeys.c | 11 ++++++++--- src/or/torcert.c | 21 ++++++++++++++++++++- src/or/torcert.h | 1 + 6 files changed, 43 insertions(+), 9 deletions(-)

1 0