From 5beb32d3d9a9640e515e6369d3a908e93b8895ef Mon Sep 17 00:00:00 2001 From: teor Date: Mon, 24 Jun 2019 21:19:49 +1000 Subject: [PATCH 1/2] stats: Stop removing the ed25519 signature if the extra info file is too big If the signature data was removed, but the keyword was kept, this could result in an unparseable extra info file. Fixes bug 30958; bugfix on 0.2.7.2-alpha. --- changes/bug30958 | 5 +++++ src/or/router.c | 12 +++++++----- 2 files changed, 12 insertions(+), 5 deletions(-) create mode 100644 changes/bug30958 diff --git a/changes/bug30958 b/changes/bug30958 new file mode 100644 index 0000000000..374c8e46f7 --- /dev/null +++ b/changes/bug30958 @@ -0,0 +1,5 @@ + o Minor bugfixes (statistics): + - Stop removing the ed25519 signature if the extra info file is too big. + If the signature data was removed, but the keyword was kept, this could + result in an unparseable extra info file. Fixes bug 30958; + bugfix on 0.2.7.2-alpha. diff --git a/src/or/router.c b/src/or/router.c index c416474226..3fce45115c 100644 --- a/src/or/router.c +++ b/src/or/router.c @@ -3252,11 +3252,13 @@ extrainfo_dump_to_string(char **s_out, extrainfo_t *extrainfo, while (strlen(s) > MAX_EXTRAINFO_UPLOAD_SIZE - DIROBJ_MAX_SIG_LEN) { /* So long as there are at least two chunks (one for the initial * extra-info line and one for the router-signature), we can keep removing - * things. */ - if (smartlist_len(chunks) > 2) { - /* We remove the next-to-last element (remember, len-1 is the last - element), since we need to keep the router-signature element. */ - int idx = smartlist_len(chunks) - 2; + * things. If emit_ed_sigs is true, we also keep 2 additional chunks at the + * end for the ed25519 signature. */ + const int required_chunks = emit_ed_sigs ? 4 : 2; + if (smartlist_len(chunks) > required_chunks) { + /* We remove the next-to-last or 4th-last element (remember, len-1 is the + * last element), since we need to keep the router-signature elements. */ + int idx = smartlist_len(chunks) - required_chunks; char *e = smartlist_get(chunks, idx); smartlist_del_keeporder(chunks, idx); log_warn(LD_GENERAL, "We just generated an extra-info descriptor " From c131b0763e994ea850f457319ec6d9c487760a85 Mon Sep 17 00:00:00 2001 From: teor Date: Mon, 24 Jun 2019 21:20:34 +1000 Subject: [PATCH 2/2] stats: add comments about the required chunk structure in extra info files These comments should prevent future instances of 30958. And allow a larger file in practracker. Follow up after 30958. --- scripts/maint/practracker/exceptions.txt | 2 +- src/feature/relay/router.c | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt index 4db452b897..ffe238a679 100644 --- a/scripts/maint/practracker/exceptions.txt +++ b/scripts/maint/practracker/exceptions.txt @@ -225,7 +225,7 @@ problem function-size /src/feature/nodelist/routerlist.c:update_extrainfo_downlo problem function-size /src/feature/relay/dns.c:dns_resolve_impl() 134 problem function-size /src/feature/relay/dns.c:configure_nameservers() 161 problem function-size /src/feature/relay/dns.c:evdns_callback() 109 -problem file-size /src/feature/relay/router.c 3510 +problem file-size /src/feature/relay/router.c 3522 problem include-count /src/feature/relay/router.c 56 problem function-size /src/feature/relay/router.c:init_keys() 252 problem function-size /src/feature/relay/router.c:get_my_declared_family() 114 diff --git a/src/feature/relay/router.c b/src/feature/relay/router.c index 782084caa3..51ced6289d 100644 --- a/src/feature/relay/router.c +++ b/src/feature/relay/router.c @@ -3158,6 +3158,8 @@ extrainfo_dump_to_string_header_helper( ed_cert_line = tor_strdup(""); } + /* This is the first chunk in the file. If the file is too big, other chunks + * are removed. So we must only add one chunk here. */ tor_asprintf(&pre, "extra-info %s %s\n%spublished %s\n", extrainfo->nickname, identity, ed_cert_line, @@ -3187,6 +3189,10 @@ extrainfo_dump_to_string_stats_helper(smartlist_t *chunks, char *contents = NULL; time_t now = time(NULL); + /* If the file is too big, these chunks are removed, starting with the last + * chunk. So each chunk must be a complete line, and the file must be valid + * after each chunk. */ + /* Add information about the pluggable transports we support, even if we * are not publishing statistics. This information is needed by BridgeDB * to distribute bridges. */ @@ -3269,6 +3275,8 @@ extrainfo_dump_to_string_ed_sig_helper( char buf[ED25519_SIG_BASE64_LEN+1]; int rv = -1; + /* These are two of the three final chunks in the file. If the file is too + * big, other chunks are removed. So we must only add two chunks here. */ smartlist_add_strdup(chunks, "router-sig-ed25519 "); crypto_digest_smartlist_prefix(sha256_digest, DIGEST256_LEN, ED_DESC_SIGNATURE_PREFIX, @@ -3362,6 +3370,8 @@ extrainfo_dump_to_string(char **s_out, extrainfo_t *extrainfo, goto err; } + /* This is one of the three final chunks in the file. If the file is too big, + * other chunks are removed. So we must only add one chunk here. */ smartlist_add_strdup(chunks, "router-signature\n"); s = smartlist_join_strings(chunks, "", 0, NULL);