From d8cfa2ef4e6d57f6dd4a33e5b3cfb1a2a12fc4be Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 16 Dec 2013 13:00:15 -0500 Subject: [PATCH 1/4] Avoid free()ing from an mmap on corrupted microdesc cache The 'body' field of a microdesc_t holds a strdup()'d value if the microdesc's saved_location field is SAVED_IN_JOURNAL or SAVED_NOWHERE, and holds a pointer to the middle of an mmap if the microdesc is SAVED_IN_CACHE. But we weren't setting that field until a while after we parsed the microdescriptor, which left an interval where microdesc_free() would try to free() the middle of the mmap(). This patch also includes a regression test. This is a fix for #10409; bugfix on 0.2.2.6-alpha. --- changes/bug10409 | 3 +++ src/or/dirvote.c | 3 ++- src/or/microdesc.c | 3 +-- src/or/routerparse.c | 13 ++++++++--- src/or/routerparse.h | 2 +- src/test/test_microdesc.c | 45 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 62 insertions(+), 7 deletions(-) create mode 100644 changes/bug10409 diff --git a/changes/bug10409 b/changes/bug10409 new file mode 100644 index 000000000..5ef5ae29d --- /dev/null +++ b/changes/bug10409 @@ -0,0 +1,3 @@ + o Minor bugfixes: + - Avoid a crash bug when starting with a corrupted microdescriptor + cache file. Fix for bug 10406; bugfix on 0.2.2.6-alpha. diff --git a/src/or/dirvote.c b/src/or/dirvote.c index 144859ae0..ab2225cf0 100644 --- a/src/or/dirvote.c +++ b/src/or/dirvote.c @@ -3538,7 +3538,8 @@ dirvote_create_microdescriptor(const routerinfo_t *ri) { smartlist_t *lst = microdescs_parse_from_string(output, - output+strlen(output), 0, 1); + output+strlen(output), 0, + SAVED_NOWHERE); if (smartlist_len(lst) != 1) { log_warn(LD_DIR, "We generated a microdescriptor we couldn't parse."); SMARTLIST_FOREACH(lst, microdesc_t *, md, microdesc_free(md)); diff --git a/src/or/microdesc.c b/src/or/microdesc.c index b4d22c1c6..6f9134cf2 100644 --- a/src/or/microdesc.c +++ b/src/or/microdesc.c @@ -149,11 +149,10 @@ microdescs_add_to_cache(microdesc_cache_t *cache, { smartlist_t *descriptors, *added; const int allow_annotations = (where != SAVED_NOWHERE); - const int copy_body = (where != SAVED_IN_CACHE); descriptors = microdescs_parse_from_string(s, eos, allow_annotations, - copy_body); + where); if (listed_at > 0) { SMARTLIST_FOREACH(descriptors, microdesc_t *, md, md->last_listed = listed_at); diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 299d07d37..52f57ec59 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -4355,12 +4355,17 @@ find_start_of_next_microdesc(const char *s, const char *eos) /** Parse as many microdescriptors as are found from the string starting at * s and ending at eos. If allow_annotations is set, read any - * annotations we recognize and ignore ones we don't. If copy_body is - * true, then strdup the bodies of the microdescriptors. Return all newly + * annotations we recognize and ignore ones we don't. + * + * If saved_location isn't SAVED_IN_CACHE, make a local copy of each + * descriptor in the body field of each microdesc_t. + * + * Return all newly * parsed microdescriptors in a newly allocated smartlist_t. */ smartlist_t * microdescs_parse_from_string(const char *s, const char *eos, - int allow_annotations, int copy_body) + int allow_annotations, + saved_location_t where) { smartlist_t *tokens; smartlist_t *result; @@ -4369,6 +4374,7 @@ microdescs_parse_from_string(const char *s, const char *eos, const char *start = s; const char *start_of_next_microdesc; int flags = allow_annotations ? TS_ANNOTATIONS_OK : 0; + const int copy_body = (where != SAVED_IN_CACHE); directory_token_t *tok; @@ -4398,6 +4404,7 @@ microdescs_parse_from_string(const char *s, const char *eos, tor_assert(cp); md->bodylen = start_of_next_microdesc - cp; + md->saved_location = where; if (copy_body) md->body = tor_strndup(cp, md->bodylen); else diff --git a/src/or/routerparse.h b/src/or/routerparse.h index c6382a7f6..e357f18b8 100644 --- a/src/or/routerparse.h +++ b/src/or/routerparse.h @@ -64,7 +64,7 @@ ns_detached_signatures_t *networkstatus_parse_detached_signatures( smartlist_t *microdescs_parse_from_string(const char *s, const char *eos, int allow_annotations, - int copy_body); + saved_location_t where); authority_cert_t *authority_cert_parse_from_string(const char *s, const char **end_of_string); diff --git a/src/test/test_microdesc.c b/src/test/test_microdesc.c index 89c578f4a..828e33431 100644 --- a/src/test/test_microdesc.c +++ b/src/test/test_microdesc.c @@ -226,8 +226,53 @@ test_md_cache(void *data) tor_free(fn); } +static const char truncated_md[] = + "@last-listed 2013-08-08 19:02:59\n" + "onion-key\n" + "-----BEGIN RSA PUBLIC KEY-----\n" + "MIGJAoGBAM91vLFNaM+gGhnRIdz2Cm/Kl7Xz0cOobIdVzhS3cKUJfk867hCuTipS\n" + "NveLBzNopvgXKruAAzEj3cACxk6Q8lv5UWOGCD1UolkgsWSE62RBjap44g+oc9J1\n" + "RI9968xOTZw0VaBQg9giEILNXl0djoikQ+5tQRUvLDDa67gpa5Q1AgMBAAE=\n" + "-----END RSA PUBLIC KEY-----\n" + "family @\n"; + +static void +test_md_cache_broken(void *data) +{ + or_options_t *options; + char *fn=NULL; + microdesc_cache_t *mc = NULL; + + (void)data; + + options = get_options_mutable(); + tt_assert(options); + options->DataDirectory = tor_strdup(get_fname("md_datadir_test2")); + +#ifdef _WIN32 + tt_int_op(0, ==, mkdir(options->DataDirectory)); +#else + tt_int_op(0, ==, mkdir(options->DataDirectory, 0700)); +#endif + + tor_asprintf(&fn, "%s"PATH_SEPARATOR"cached-microdescs", + options->DataDirectory); + + write_str_to_file(fn, truncated_md, 1); + + mc = get_microdesc_cache(); + tt_assert(mc); + + done: + if (options) + tor_free(options->DataDirectory); + tor_free(fn); + microdesc_free_all(); +} + struct testcase_t microdesc_tests[] = { { "cache", test_md_cache, TT_FORK, NULL, NULL }, + { "broken_cache", test_md_cache_broken, TT_FORK, NULL, NULL }, END_OF_TESTCASES }; From 46b3b6208de3e5a5b87b4a33387d18343bc3c851 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 17 Dec 2013 13:12:52 -0500 Subject: [PATCH 2/4] Avoid double-free on failure to dump_descriptor() a cached md This is a fix for 10423, which was introducd in caa0d15c in 0.2.4.13-alpha. Spotted by bobnomnom. --- changes/bug10423 | 4 ++++ src/or/microdesc.c | 33 ++++++++++++++++++++++----------- 2 files changed, 26 insertions(+), 11 deletions(-) create mode 100644 changes/bug10423 diff --git a/changes/bug10423 b/changes/bug10423 new file mode 100644 index 000000000..493b7b15e --- /dev/null +++ b/changes/bug10423 @@ -0,0 +1,4 @@ + o Minor bugfixes: + - If we fail to dump a previously cached microdescriptor to disk, avoid + freeing duplicate data later on. Fix for bug 10423; bugfix on + 0.2.4.13-alpha. Spotted by "bobnomnom". diff --git a/src/or/microdesc.c b/src/or/microdesc.c index 8b5581f4a..18d26c98a 100644 --- a/src/or/microdesc.c +++ b/src/or/microdesc.c @@ -407,6 +407,26 @@ should_rebuild_md_cache(microdesc_cache_t *cache) return 0; } +/** + * Mark md as having no body, and release any storage previously held + * by its body. + */ +static void +microdesc_wipe_body(microdesc_t *md) +{ + if (!md) + return; + + if (md->saved_location != SAVED_IN_CACHE) + tor_free(md->body); + + md->off = 0; + md->saved_location = SAVED_NOWHERE; + md->body = NULL; + md->bodylen = 0; + md->no_save = 1; +} + /** Regenerate the main cache file for cache, clear the journal file, * and update every microdesc_t in the cache with pointers to its new * location. If force is true, do this unconditionally. If @@ -455,12 +475,7 @@ microdesc_cache_rebuild(microdesc_cache_t *cache, int force) size = dump_microdescriptor(fd, md, &annotation_len); if (size < 0) { - if (md->saved_location != SAVED_IN_CACHE) - tor_free(md->body); - md->saved_location = SAVED_NOWHERE; - md->off = 0; - md->bodylen = 0; - md->no_save = 1; + microdesc_wipe_body(md); /* rewind, in case it was a partial write. */ tor_fd_setpos(fd, off); @@ -497,11 +512,7 @@ microdesc_cache_rebuild(microdesc_cache_t *cache, int force) HT_FOREACH(mdp, microdesc_map, &cache->map) { microdesc_t *md = *mdp; if (md->saved_location == SAVED_IN_CACHE) { - md->off = 0; - md->saved_location = SAVED_NOWHERE; - md->body = NULL; - md->bodylen = 0; - md->no_save = 1; + microdesc_wipe_body(md); } } return -1; From 7b87003957530427eadce36ed03b4645b481a335 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 18 Dec 2013 11:49:44 -0500 Subject: [PATCH 3/4] Never allow OpenSSL engines to replace the RAND_SSLeay method This fixes bug 10402, where the rdrand engine would use the rdrand instruction, not as an additional entropy source, but as a replacement for the entire userspace PRNG. That's obviously stupid: even if you don't think that RDRAND is a likely security risk, the right response to an alleged new alleged entropy source is never to throw away all previously used entropy sources. Thanks to coderman and rl1987 for diagnosing and tracking this down. --- changes/bug10402 | 11 +++++++++++ src/common/crypto.c | 13 ++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-) create mode 100644 changes/bug10402 diff --git a/changes/bug10402 b/changes/bug10402 new file mode 100644 index 000000000..eac00bdc6 --- /dev/null +++ b/changes/bug10402 @@ -0,0 +1,11 @@ + o Major bugfixes: + - Do not allow OpenSSL engines to replace the PRNG, even when + HardwareAccel is set. The only default builtin PRNG engine uses + the Intel RDRAND instruction to replace the entire PRNG, and + ignores all attempts to seed it with more entropy. That's + cryptographically stupid: the right response to a new alleged + entropy source is never to discard all previously used entropy + sources. Fixes bug 10402; works around behavior introduced in + OpenSSL 1.0.0. Diagnosis and investigation thanks to "coderman" + and "rl1987". + diff --git a/src/common/crypto.c b/src/common/crypto.c index 0ababeaea..940a756f6 100644 --- a/src/common/crypto.c +++ b/src/common/crypto.c @@ -169,8 +169,8 @@ log_engine(const char *fn, ENGINE *e) const char *name, *id; name = ENGINE_get_name(e); id = ENGINE_get_id(e); - log_notice(LD_CRYPTO, "Using OpenSSL engine %s [%s] for %s", - name?name:"?", id?id:"?", fn); + log_notice(LD_CRYPTO, "Default OpenSSL engine for %s is %s [%s]", + fn, name?name:"?", id?id:"?"); } else { log_info(LD_CRYPTO, "Using default implementation for %s", fn); } @@ -288,7 +288,7 @@ crypto_global_init(int useAccel, const char *accelName, const char *accelDir) } log_engine("RSA", ENGINE_get_default_RSA()); log_engine("DH", ENGINE_get_default_DH()); - log_engine("RAND", ENGINE_get_default_RAND()); + log_engine("RAND (which we will not use)", ENGINE_get_default_RAND()); log_engine("SHA1", ENGINE_get_digest_engine(NID_sha1)); log_engine("3DES", ENGINE_get_cipher_engine(NID_des_ede3_ecb)); log_engine("AES", ENGINE_get_cipher_engine(NID_aes_128_ecb)); @@ -297,6 +297,13 @@ crypto_global_init(int useAccel, const char *accelName, const char *accelDir) log_info(LD_CRYPTO, "NOT using OpenSSL engine support."); } + if (RAND_get_rand_method() != RAND_SSLeay()) { + log_notice(LD_CRYPTO, "It appears that one of our engines has provided " + "a replacement the OpenSSL RNG. Resetting it to the default " + "implementation."); + RAND_set_rand_method(RAND_SSLeay()); + } + evaluate_evp_for_aes(-1); evaluate_ctr_for_aes(); From b5d13d11c90cb94193b6071e8c525f75cc77b861 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 21 Dec 2013 10:15:09 -0500 Subject: [PATCH 4/4] Fix a logic error in circuit_stream_is_being_handled. When I introduced the unusable_for_new_circuits flag in 62fb209d837f3f551, I had a spurious ! in the circuit_stream_is_being_handled loop. This made us decide that non-unusable circuits (that is, usable ones) were the ones to avoid, and caused it to launch a bunch of extra circuits. Fixes bug 10456; bugfix on 0.2.4.12-alpha. --- changes/bug10456 | 6 ++++++ src/or/circuituse.c | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 changes/bug10456 diff --git a/changes/bug10456 b/changes/bug10456 new file mode 100644 index 000000000..fb3b92fcd --- /dev/null +++ b/changes/bug10456 @@ -0,0 +1,6 @@ + o Major bugfixes: + - Avoid launching spurious extra circuits when a stream is pending. + This fixes a bug where any circuit that _wasn't_ unusable for new + streams would be treated as if it were, causing extra circuits to + be launched. Fixes bug 10456; bugfix on 0.2.4.12-alpha. + diff --git a/src/or/circuituse.c b/src/or/circuituse.c index 25997ebdb..598469198 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -828,7 +828,7 @@ circuit_stream_is_being_handled(entry_connection_t *conn, cpath_build_state_t *build_state = origin_circ->build_state; if (build_state->is_internal || build_state->onehop_tunnel) continue; - if (!origin_circ->unusable_for_new_conns) + if (origin_circ->unusable_for_new_conns) continue; exitnode = build_state_get_exit_node(build_state);