From 9b7cdd23deb5830cce794ce6e901b82fa094dd9f Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 28 Feb 2019 11:22:20 +1000 Subject: [PATCH 01/28] doc: update the man page and sample torrc for ExitRelay We changed the default of ExitRelay in #21530 in 0.3.5.1-alpha, but forgot to update the documentation. Closes 29612. --- doc/tor.1.txt | 13 +++++++------ src/config/torrc.sample.in | 21 ++++++++++++++++----- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/doc/tor.1.txt b/doc/tor.1.txt index 573fdf221a..60997ad16e 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -1849,15 +1849,16 @@ is non-zero): [[ExitRelay]] **ExitRelay** **0**|**1**|**auto**:: Tells Tor whether to run as an exit relay. If Tor is running as a non-bridge server, and ExitRelay is set to 1, then Tor allows traffic to - exit according to the ExitPolicy option (or the default ExitPolicy if - none is specified). + + exit according to the ExitPolicy option, the ReducedExitPolicy option, + or the default ExitPolicy (if no other exit policy option is specified). + + If ExitRelay is set to 0, no traffic is allowed to - exit, and the ExitPolicy option is ignored. + + exit, and the ExitPolicy and ReducedExitPolicy options are ignored. + + - If ExitRelay is set to "auto", then Tor behaves as if it were set to 1, but - warns the user if this would cause traffic to exit. In a future version, - the default value will be 0. (Default: auto) + If ExitRelay is set to "auto", then Tor checks the ExitPolicy and + ReducedExitPolicy options. If either is set, Tor behaves as if ExitRelay + were set to 1. If neither exit policy option is set, Tor behaves as if + ExitRelay were set to 0. (Default: auto) [[ExitPolicy]] **ExitPolicy** __policy__,__policy__,__...__:: Set an exit policy for this server. Each policy is of the form diff --git a/src/config/torrc.sample.in b/src/config/torrc.sample.in index 8d56b0896b..c2ae707e93 100644 --- a/src/config/torrc.sample.in +++ b/src/config/torrc.sample.in @@ -1,5 +1,5 @@ ## Configuration file for a typical Tor user -## Last updated 22 December 2017 for Tor 0.3.2.8-rc. +## Last updated 28 February 2019 for Tor 0.3.5.1-alpha. ## (may or may not work for much older or much newer versions of Tor.) ## ## Lines that begin with "## " try to explain what's going on. Lines @@ -172,14 +172,25 @@ ## Note: do not use MyFamily on bridge relays. #MyFamily $keyid,$keyid,... -## Uncomment this if you do *not* want your relay to allow any exit traffic. -## (Relays allow exit traffic by default.) -#ExitRelay 0 +## Uncomment this if you want your relay to be an exit, with the default +## exit policy (or whatever exit policy you set below). +## (If ReducedExitPolicy or ExitPolicy are set, relays are exits. +## If neither exit policy option is set, relays are non-exits.) +#ExitRelay 1 ## Uncomment this if you want your relay to allow IPv6 exit traffic. -## (Relays only allow IPv4 exit traffic by default.) +## You must also set ExitRelay, ReducedExitPolicy, or ExitPolicy to make your +## relay into an exit. +## (Relays do not allow any exit traffic by default.) #IPv6Exit 1 +## Uncomment this if you want your relay to be an exit, with a reduced set +## of exit ports. +#ReducedExitPolicy 1 + +## Uncomment these lines if you want your relay to be an exit, with the +## specified set of exit IPs and ports. +## ## A comma-separated list of exit policies. They're considered first ## to last, and the first match wins. ## From 669ec64325fde2f145b1798ee04c5fb74313b0e8 Mon Sep 17 00:00:00 2001 From: rl1987 Date: Wed, 20 Mar 2019 19:44:54 +0200 Subject: [PATCH 02/28] Fix CID 1444119 Let's use the same function exit point for BUG() codepath that we're using for every other exit condition. That way, we're not forgetting to clean up the memarea. --- changes/cid1444119 | 3 +++ src/or/consdiff.c | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 changes/cid1444119 diff --git a/changes/cid1444119 b/changes/cid1444119 new file mode 100644 index 0000000000..bb6854e66f --- /dev/null +++ b/changes/cid1444119 @@ -0,0 +1,3 @@ + o Minor bugfixes (C correctness): + - Fix an unlikely memory leak in consensus_diff_apply(). Fixes bug 29824; + bugfix on 0.3.1.1-alpha. This is Coverity warning CID 1444119. diff --git a/src/or/consdiff.c b/src/or/consdiff.c index deaf465fe7..1b90dfe99e 100644 --- a/src/or/consdiff.c +++ b/src/or/consdiff.c @@ -1385,7 +1385,7 @@ consensus_diff_apply(const char *consensus, r1 = consensus_compute_digest_as_signed(consensus, &d1); if (BUG(r1 < 0)) - return NULL; // LCOV_EXCL_LINE + goto done; lines1 = smartlist_new(); lines2 = smartlist_new(); From 680fd3f8fb7157432398a3552ee9c98c72bd7397 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 29 Mar 2019 13:38:14 -0400 Subject: [PATCH 03/28] NSS: Log an error message when SSL_ExportKeyingMaterial() fails Diagnostic for 29241. --- changes/29241_diagnostic | 4 ++++ src/lib/tls/tortls_nss.c | 8 ++++++++ 2 files changed, 12 insertions(+) create mode 100644 changes/29241_diagnostic diff --git a/changes/29241_diagnostic b/changes/29241_diagnostic new file mode 100644 index 0000000000..1e38654957 --- /dev/null +++ b/changes/29241_diagnostic @@ -0,0 +1,4 @@ + o Minor features (NSS, diagnostic): + - Try to log an error from NSS (if there is any) and a more useful + description of our situation if we are using NSS and a call to + SSL_ExportKeyingMaterial() fails. Diagnostic for ticket 29241. diff --git a/src/lib/tls/tortls_nss.c b/src/lib/tls/tortls_nss.c index 00c4af0e97..4e107fae7b 100644 --- a/src/lib/tls/tortls_nss.c +++ b/src/lib/tls/tortls_nss.c @@ -726,10 +726,18 @@ tor_tls_export_key_material,(tor_tls_t *tls, uint8_t *secrets_out, tor_assert(context_len <= UINT_MAX); SECStatus s; + /* Make sure that the error code is set here, so that we can be sure that + * any error code set after a failure was in fact caused by + * SSL_ExportKeyingMaterial. */ + PR_SetError(PR_UNKNOWN_ERROR, 0); s = SSL_ExportKeyingMaterial(tls->ssl, label, (unsigned)strlen(label), PR_TRUE, context, (unsigned)context_len, secrets_out, DIGEST256_LEN); + if (s != SECSuccess) { + tls_log_errors(tls, LOG_WARN, LD_CRYPTO, + "exporting key material for a TLS handshake"); + } return (s == SECSuccess) ? 0 : -1; } From 5cb94cbf9d89804ea37a2f1e68d354a86edb223e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 29 Mar 2019 13:38:48 -0400 Subject: [PATCH 04/28] NSS: disable TLS1.2 SHA-384 ciphersuites. In current NSS versions, these ciphersuites don't work with SSL_ExportKeyingMaterial(), which was causing relays to fail when they tried to negotiate the v3 link protocol authentication. Fixes bug 29241; bugfix on 0.4.0.1-alpha. --- changes/bug29241 | 6 ++++++ src/lib/tls/tortls_nss.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 changes/bug29241 diff --git a/changes/bug29241 b/changes/bug29241 new file mode 100644 index 0000000000..13951d1162 --- /dev/null +++ b/changes/bug29241 @@ -0,0 +1,6 @@ + o Major bugfixes (NSS, relay): + - When running with NSS, disable TLS 1.2 ciphersuites that use SHA384 + for their PRF. Due to an NSS bug, the TLS key exporters for these + ciphersuites don't work -- which caused relays to fail to handshake + with one another when these ciphersuites were enabled. + Fixes bug 29241; bugfix on 0.4.0.1-alpha. diff --git a/src/lib/tls/tortls_nss.c b/src/lib/tls/tortls_nss.c index 4e107fae7b..3c62e98df1 100644 --- a/src/lib/tls/tortls_nss.c +++ b/src/lib/tls/tortls_nss.c @@ -152,6 +152,32 @@ we_like_auth_type(SSLAuthType at) } } +/** + * Return true iff this ciphersuite will be hit by a mozilla bug 1312976, + * which makes TLS key exporters not work with TLS 1.2 non-SHA256 + * ciphersuites. + **/ +static bool +ciphersuite_has_nss_export_bug(const SSLCipherSuiteInfo *info) +{ + /* For more information on the bug, see + https://bugzilla.mozilla.org/show_bug.cgi?id=1312976 */ + + /* This bug only exists in TLS 1.2. */ + if (info->authType == ssl_auth_tls13_any) + return false; + + /* Sadly, there's no way to get this information from the + * CipherSuiteInfo object itself other than by looking at the + * name. */ + if (strstr(info->cipherSuiteName, "_SHA384") || + strstr(info->cipherSuiteName, "_SHA512")) { + return true; + } + + return false; +} + tor_tls_context_t * tor_tls_context_new(crypto_pk_t *identity, unsigned int key_lifetime, unsigned flags, int is_client) @@ -256,6 +282,12 @@ tor_tls_context_new(crypto_pk_t *identity, !we_like_mac_algorithm(info.macAlgorithm) || !we_like_auth_type(info.authType)/* Requires NSS 3.24 */; + if (ciphersuite_has_nss_export_bug(&info)) { + /* SSL_ExportKeyingMaterial will fail; we can't use this cipher. + */ + disable = 1; + } + s = SSL_CipherPrefSet(ctx->ctx, ciphers[i], disable ? PR_FALSE : PR_TRUE); if (s != SECSuccess) From 4dd96f74444b0eeb8b3b2c36f6f129edddd464d4 Mon Sep 17 00:00:00 2001 From: teor Date: Sat, 6 Apr 2019 11:07:20 +1000 Subject: [PATCH 05/28] changes: Ticket 29241 is actually a bug on NSS in 0.3.5.1-alpha --- changes/bug29241 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changes/bug29241 b/changes/bug29241 index 13951d1162..7f25e154d1 100644 --- a/changes/bug29241 +++ b/changes/bug29241 @@ -3,4 +3,4 @@ for their PRF. Due to an NSS bug, the TLS key exporters for these ciphersuites don't work -- which caused relays to fail to handshake with one another when these ciphersuites were enabled. - Fixes bug 29241; bugfix on 0.4.0.1-alpha. + Fixes bug 29241; bugfix on 0.3.5.1-alpha. From 9ce0bdd22636a332399d65f280915e899a24b69b Mon Sep 17 00:00:00 2001 From: Tobias Stoeckmann Date: Sun, 31 Mar 2019 12:27:55 +0200 Subject: [PATCH 06/28] Prevent double free on huge files with 32 bit. The function compat_getdelim_ is used for tor_getline if tor is compiled on a system that lacks getline and getdelim. These systems should be very rare, considering that getdelim is POSIX. If this system is further a 32 bit architecture, it is possible to trigger a double free with huge files. If bufsiz has been already increased to 2 GB, the next chunk would be 4 GB in size, which wraps around to 0 due to 32 bit limitations. A realloc(*buf, 0) could be imagined as "free(*buf); return malloc(0);" which therefore could return NULL. The code in question considers that an error, but will keep the value of *buf pointing to already freed memory. The caller of tor_getline() would free the pointer again, therefore leading to a double free. This code can only be triggered in dirserv_read_measured_bandwidths with a huge measured bandwith list file on a system that actually allows to reach 2 GB of space through realloc. It is not possible to trigger this on Linux with glibc or other major *BSD systems even on unit tests, because these systems cannot reach so much memory due to memory fragmentation. This patch is effectively based on the penetration test report of cure53 for curl available at https://cure53.de/pentest-report_curl.pdf and explained under section "CRL-01-007 Double-free in aprintf() via unsafe size_t multiplication (Medium)". --- src/ext/getdelim.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ext/getdelim.c b/src/ext/getdelim.c index 8254103ff9..1c29baffd9 100644 --- a/src/ext/getdelim.c +++ b/src/ext/getdelim.c @@ -67,7 +67,8 @@ compat_getdelim_(char **buf, size_t *bufsiz, int delimiter, FILE *fp) char *nbuf; size_t nbufsiz = *bufsiz * 2; ssize_t d = ptr - *buf; - if ((nbuf = raw_realloc(*buf, nbufsiz)) == NULL) + if (nbufsiz < *bufsiz || + (nbuf = raw_realloc(*buf, nbufsiz)) == NULL) return -1; *buf = nbuf; *bufsiz = nbufsiz; From 2cdc6b2005d2ad09b44cf9a455a70f258e7f6fca Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Tue, 9 Apr 2019 17:30:14 +0300 Subject: [PATCH 07/28] Add changes file for #30040. --- changes/bug30040 | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 changes/bug30040 diff --git a/changes/bug30040 b/changes/bug30040 new file mode 100644 index 0000000000..7d80528a10 --- /dev/null +++ b/changes/bug30040 @@ -0,0 +1,9 @@ + o Minor bugfixes (security): + - Fix a potential double free bug when reading huge bandwidth files. The + issue is not exploitable in the current Tor network because the + vulnerable code is only reached when directory authorities read bandwidth + files, but bandwidth files come from a trusted source (usually the + authorities themselves). Furthermore, the issue is only exploitable in + rare (non-POSIX) 32-bit architectures which are not used by any of the + current authorities. Fixes bug 30040; bugfix on 0.3.5.1-alpha. Bug found + and fixed by Tobias Stoeckmann. From 85ff6f9114eb2d492d88ed55d01a69f7bafb10be Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 11 Apr 2019 18:18:14 -0400 Subject: [PATCH 08/28] Fix a memory leak on failure to create keys directory. Fixes bug 30148, which is also CID 1437429 and CID 1437454. Bugfix on 0.3.3.1-alpha, when separate key directories became a thing. --- changes/bug30148 | 4 ++++ src/or/routerkeys.c | 3 +-- 2 files changed, 5 insertions(+), 2 deletions(-) create mode 100644 changes/bug30148 diff --git a/changes/bug30148 b/changes/bug30148 new file mode 100644 index 0000000000..7d0257e3fe --- /dev/null +++ b/changes/bug30148 @@ -0,0 +1,4 @@ + o Minor bugfixes (memory leak): + - Avoid a minor memory leak that could occur on relays when + creating a keys directory failed. Fixes bug 30148; bugfix on + 0.3.3.1-alpha. diff --git a/src/or/routerkeys.c b/src/or/routerkeys.c index 43460da8cc..dd19d28d05 100644 --- a/src/or/routerkeys.c +++ b/src/or/routerkeys.c @@ -816,7 +816,7 @@ load_ed_keys(const or_options_t *options, time_t now) /* Check/Create the key directory */ if (create_keys_directory(options) < 0) - return -1; + goto err; char *fname; if (options->master_key_fname) { @@ -1403,4 +1403,3 @@ routerkeys_free_all(void) rsa_ed_crosscert = NULL; // redundant rsa_ed_crosscert_len = 0; } - From 802ac8ad618a0674c26bb973a2277e07b20f8c7b Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 15 Apr 2019 14:16:23 -0400 Subject: [PATCH 09/28] Use a tor_abort_() wrapper in our util_bug.h macros Previously, our use of abort() would break anywhere that we didn't include stdlib.h. This was especially troublesome in case where tor_assert_nonfatal() was used with ALL_BUGS_ARE_FATAL, since that one seldom gets tested. As an alternative, we could have just made this header include stdlib.h. But that seems bloaty. Fixes bug 30189; bugfix on 0.3.4.1-alpha. --- changes/bug30189 | 4 ++++ src/lib/log/util_bug.c | 14 ++++++++++++++ src/lib/log/util_bug.h | 8 +++++--- 3 files changed, 23 insertions(+), 3 deletions(-) create mode 100644 changes/bug30189 diff --git a/changes/bug30189 b/changes/bug30189 new file mode 100644 index 0000000000..f8c932a5f9 --- /dev/null +++ b/changes/bug30189 @@ -0,0 +1,4 @@ + o Minor bugfixes (compilation, unusual configuration): + - Avoid failures when building with ALL_BUGS_ARE_FAILED due to + missing declarations of abort(), and prevent other such failures + in the future. Fixes bug 30189; bugfix on 0.3.4.1-alpha. diff --git a/src/lib/log/util_bug.c b/src/lib/log/util_bug.c index f42d2d2ab4..c65a91ae9e 100644 --- a/src/lib/log/util_bug.c +++ b/src/lib/log/util_bug.c @@ -19,6 +19,7 @@ #include "lib/string/printf.h" #include +#include #ifdef TOR_UNIT_TESTS static void (*failed_assertion_cb)(void) = NULL; @@ -120,6 +121,19 @@ tor_bug_occurred_(const char *fname, unsigned int line, #endif } +/** + * Call the abort() function to kill the current process with a fatal + * error. + * + * (This is a separate function so that we declare it in util_bug.h without + * including stdlib in all the users of util_bug.h) + **/ +void +tor_abort_(void) +{ + abort(); +} + #ifdef _WIN32 /** Take a filename and return a pointer to its final element. This * function is called on __FILE__ to fix a MSVC nit where __FILE__ diff --git a/src/lib/log/util_bug.h b/src/lib/log/util_bug.h index 18d40bbf39..2a4d68127e 100644 --- a/src/lib/log/util_bug.h +++ b/src/lib/log/util_bug.h @@ -99,7 +99,7 @@ if (ASSERT_PREDICT_LIKELY_(expr)) { \ } else { \ tor_assertion_failed_(SHORT_FILE__, __LINE__, __func__, #expr); \ - abort(); \ + tor_abort_(); \ } STMT_END #endif /* defined(TOR_UNIT_TESTS) && defined(DISABLE_ASSERTS_IN_UNIT_TESTS) */ @@ -107,7 +107,7 @@ STMT_BEGIN { \ tor_assertion_failed_(SHORT_FILE__, __LINE__, __func__, \ "line should be unreached"); \ - abort(); \ + tor_abort_(); \ } STMT_END /* Non-fatal bug assertions. The "unreached" variants mean "this line should @@ -141,7 +141,7 @@ #define BUG(cond) \ (ASSERT_PREDICT_UNLIKELY_(cond) ? \ (tor_assertion_failed_(SHORT_FILE__,__LINE__,__func__,"!("#cond")"), \ - abort(), 1) \ + tor_abort_(), 1) \ : 0) #elif defined(TOR_UNIT_TESTS) && defined(DISABLE_ASSERTS_IN_UNIT_TESTS) #define tor_assert_nonfatal_unreached() STMT_NIL @@ -226,6 +226,8 @@ void tor_bug_occurred_(const char *fname, unsigned int line, const char *func, const char *expr, int once); +void tor_abort_(void) ATTR_NORETURN; + #ifdef _WIN32 #define SHORT_FILE__ (tor_fix_source_file(__FILE__)) const char *tor_fix_source_file(const char *fname); From 8c4e68438dd680b6b36f9b337b12d098b08db9d8 Mon Sep 17 00:00:00 2001 From: "Bernhard M. Wiedemann" Date: Mon, 15 Apr 2019 13:57:24 +0200 Subject: [PATCH 10/28] Do not warn about compatible OpenSSL upgrades When releasing OpenSSL patch-level maintenance updates, we do not want to rebuild binaries using it. And since they guarantee ABI stability, we do not have to. Without this patch, warning messages were produced that confused users: https://bugzilla.opensuse.org/show_bug.cgi?id=1129411 Fixes bug 30190; bugfix on 0.2.4.2-alpha commit 7607ad2bec Signed-off-by: Bernhard M. Wiedemann --- changes/bug30190 | 3 +++ src/lib/crypt_ops/crypto_openssl_mgt.c | 8 ++++++++ 2 files changed, 11 insertions(+) create mode 100644 changes/bug30190 diff --git a/changes/bug30190 b/changes/bug30190 new file mode 100644 index 0000000000..e2352c3b9c --- /dev/null +++ b/changes/bug30190 @@ -0,0 +1,3 @@ + o Minor bugfixes (lib): + do not log a warning for OpenSSL versions that should be compatible + Fixes bug 30190; bugfix on 0.2.4.2-alpha diff --git a/src/lib/crypt_ops/crypto_openssl_mgt.c b/src/lib/crypt_ops/crypto_openssl_mgt.c index 60e4ea795e..c97815f9a4 100644 --- a/src/lib/crypt_ops/crypto_openssl_mgt.c +++ b/src/lib/crypt_ops/crypto_openssl_mgt.c @@ -213,6 +213,14 @@ crypto_openssl_early_init(void) !strcmp(version_str, OPENSSL_VERSION_TEXT)) { log_info(LD_CRYPTO, "OpenSSL version matches version from headers " "(%lx: %s).", version_num, version_str); + } else if ((version_num & 0xffff0000) == + (OPENSSL_VERSION_NUMBER & 0xffff0000)) { + log_notice(LD_CRYPTO, + "We compiled with OpenSSL %lx: %s and we " + "are running with OpenSSL %lx: %s. " + "These two versions should be binary compatible.", + (unsigned long)OPENSSL_VERSION_NUMBER, OPENSSL_VERSION_TEXT, + version_num, version_str); } else { log_warn(LD_CRYPTO, "OpenSSL version from headers does not match the " "version we're running with. If you get weird crashes, that " From 0ab4dc7ef7c6468c1d0d87a7cbc834217214f16b Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 29 Apr 2019 13:43:50 -0400 Subject: [PATCH 11/28] Move bandwidth-file-headers line to appear in the correct vote section Fixes bug 30316; bugfix on 0.3.5.1-alpha. --- changes/bug30316 | 4 ++++ src/feature/dirauth/dirvote.c | 7 +++---- 2 files changed, 7 insertions(+), 4 deletions(-) create mode 100644 changes/bug30316 diff --git a/changes/bug30316 b/changes/bug30316 new file mode 100644 index 0000000000..3e396318ad --- /dev/null +++ b/changes/bug30316 @@ -0,0 +1,4 @@ + o Minor bugfixes (directory authority): + - Move the "bandwidth-file-headers" line in directory authority votes + so that it conforms to dir-spec.txt. Fixes bug 30316; bugfix on + 0.3.5.1-alpha. diff --git a/src/feature/dirauth/dirvote.c b/src/feature/dirauth/dirvote.c index 5e426b0f86..af8b3dc207 100644 --- a/src/feature/dirauth/dirvote.c +++ b/src/feature/dirauth/dirvote.c @@ -322,10 +322,10 @@ format_networkstatus_vote(crypto_pk_t *private_signing_key, "known-flags %s\n" "flag-thresholds %s\n" "params %s\n" + "%s" /* bandwidth file headers */ "dir-source %s %s %s %s %d %d\n" "contact %s\n" "%s" /* shared randomness information */ - "%s" /* bandwidth file headers */ , v3_ns->type == NS_TYPE_VOTE ? "vote" : "opinion", methods, @@ -338,13 +338,12 @@ format_networkstatus_vote(crypto_pk_t *private_signing_key, flags, flag_thresholds, params, + bw_headers_line ? bw_headers_line : "", voter->nickname, fingerprint, voter->address, fmt_addr32(addr), voter->dir_port, voter->or_port, voter->contact, shared_random_vote_str ? - shared_random_vote_str : "", - bw_headers_line ? - bw_headers_line : ""); + shared_random_vote_str : ""); tor_free(params); tor_free(flags); From cbcc570ff4a8e03fc08bfd020d6b4c7365d4d137 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 9 May 2019 12:45:17 -0400 Subject: [PATCH 12/28] hs: Remove usage of HS_INTRO_ACK_STATUS_CANT_RELAY The INTRODUCE1 trunnel definition file doesn't support that value so it can not be used else it leads to an assert on the intro point side if ever tried. Fortunately, it was impossible to reach that code path. Part of #30454 Signed-off-by: David Goulet --- src/or/hs_intropoint.c | 5 +++-- src/or/hs_intropoint.h | 1 - 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/or/hs_intropoint.c b/src/or/hs_intropoint.c index ee79109ca9..e0ab27bd36 100644 --- a/src/or/hs_intropoint.c +++ b/src/or/hs_intropoint.c @@ -483,8 +483,9 @@ handle_introduce1(or_circuit_t *client_circ, const uint8_t *request, RELAY_COMMAND_INTRODUCE2, (char *) request, request_len, NULL)) { log_warn(LD_PROTOCOL, "Unable to send INTRODUCE2 cell to the service."); - /* Inform the client that we can't relay the cell. */ - status = HS_INTRO_ACK_STATUS_CANT_RELAY; + /* Inform the client that we can't relay the cell. Use the unknown ID + * status code since it means that we do not know the service. */ + status = HS_INTRO_ACK_STATUS_UNKNOWN_ID; goto send_ack; } diff --git a/src/or/hs_intropoint.h b/src/or/hs_intropoint.h index 749d1530e1..014f9339a6 100644 --- a/src/or/hs_intropoint.h +++ b/src/or/hs_intropoint.h @@ -24,7 +24,6 @@ typedef enum { HS_INTRO_ACK_STATUS_SUCCESS = 0x0000, HS_INTRO_ACK_STATUS_UNKNOWN_ID = 0x0001, HS_INTRO_ACK_STATUS_BAD_FORMAT = 0x0002, - HS_INTRO_ACK_STATUS_CANT_RELAY = 0x0003, } hs_intro_ack_status_t; /* Object containing introduction point common data between the service and From 1b16fcb70cb9507184fe7316396c93c1daa12674 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 9 May 2019 11:26:13 -0400 Subject: [PATCH 13/28] Add a --list-modules command Closes ticket 30452. --- changes/bug30452 | 3 +++ doc/tor.1.txt | 4 ++++ src/app/config/config.c | 13 +++++++++++++ src/feature/dirauth/authmode.h | 4 ++++ 4 files changed, 24 insertions(+) create mode 100644 changes/bug30452 diff --git a/changes/bug30452 b/changes/bug30452 new file mode 100644 index 0000000000..2bb401d87d --- /dev/null +++ b/changes/bug30452 @@ -0,0 +1,3 @@ + o Minor features (compile-time modules): + - Add a --list-modules command to print a list of which compile-time + modules are enabled. Closes ticket 30452. diff --git a/doc/tor.1.txt b/doc/tor.1.txt index 975a198182..f46929897b 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -88,6 +88,10 @@ COMMAND-LINE OPTIONS List all valid options that are scheduled to become obsolete in a future version. (This is a warning, not a promise.) +[[opt-list-modules]] **--list-modules**:: + For each optional module, list whether or not it has been compiled + into Tor. (Any module not listed is not optional in this version of Tor.) + [[opt-version]] **--version**:: Display Tor version and exit. diff --git a/src/app/config/config.c b/src/app/config/config.c index 2a504d3065..7ccd76e7f0 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -2474,6 +2474,7 @@ static const struct { { "--quiet", TAKES_NO_ARGUMENT }, { "--hush", TAKES_NO_ARGUMENT }, { "--version", TAKES_NO_ARGUMENT }, + { "--list-modules", TAKES_NO_ARGUMENT }, { "--library-versions", TAKES_NO_ARGUMENT }, { "-h", TAKES_NO_ARGUMENT }, { "--help", TAKES_NO_ARGUMENT }, @@ -2695,6 +2696,13 @@ list_deprecated_options(void) } } +/** Print all compile-time modules and their enabled/disabled status. */ +static void +list_enabled_modules(void) +{ + printf("%s: %s\n", "dirauth", have_module_dirauth() ? "yes" : "no"); +} + /** Last value actually set by resolve_my_address. */ static uint32_t last_resolved_addr = 0; @@ -5198,6 +5206,11 @@ options_init_from_torrc(int argc, char **argv) return 1; } + if (config_line_find(cmdline_only_options, "--list-modules")) { + list_enabled_modules(); + return 1; + } + if (config_line_find(cmdline_only_options, "--library-versions")) { printf("Tor version %s. \n", get_version()); printf("Library versions\tCompiled\t\tRuntime\n"); diff --git a/src/feature/dirauth/authmode.h b/src/feature/dirauth/authmode.h index 40a89c7397..876a1f947b 100644 --- a/src/feature/dirauth/authmode.h +++ b/src/feature/dirauth/authmode.h @@ -27,6 +27,8 @@ authdir_mode_v3(const or_options_t *options) return authdir_mode(options) && options->V3AuthoritativeDir != 0; } +#define have_module_dirauth() (1) + #else /* HAVE_MODULE_DIRAUTH */ #define authdir_mode(options) (((void)(options)),0) @@ -37,6 +39,8 @@ authdir_mode_v3(const or_options_t *options) #define authdir_mode_bridge(options) (((void)(options)),0) #define authdir_mode_v3(options) (((void)(options)),0) +#define have_module_dirauth() (0) + #endif /* HAVE_MODULE_DIRAUTH */ #endif /* TOR_MODE_H */ From 0c451b31d2694c96ec20145e3b0e9b22cd54819d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 14 May 2019 19:55:35 -0400 Subject: [PATCH 14/28] Make --list-modules imply --hush --- src/app/main/main.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/app/main/main.c b/src/app/main/main.c index c45c87d8d2..67f2181cd5 100644 --- a/src/app/main/main.c +++ b/src/app/main/main.c @@ -577,6 +577,7 @@ tor_init(int argc, char *argv[]) if (!strcmp(cl->key, "--version") || !strcmp(cl->key, "--digests") || !strcmp(cl->key, "--list-torrc-options") || !strcmp(cl->key, "--library-versions") || + !strcmp(cl->key, "--list-modules") || !strcmp(cl->key, "--hash-password") || !strcmp(cl->key, "-h") || !strcmp(cl->key, "--help")) { if (quiet < 1) From 37aae23945c45c459091b38699130a14466d33ef Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 17 May 2019 10:03:41 -0400 Subject: [PATCH 15/28] OOM-purge the DNS cache one part at a time Previously we purged it in 1-hour increments -- but one-hour is the maximum TTL for the cache! Now we do it in 25%-TTL increments. Fixes bug 29617; bugfix on 0.3.5.1-alpha. --- changes/ticket29617 | 4 ++++ src/feature/relay/dns.c | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 changes/ticket29617 diff --git a/changes/ticket29617 b/changes/ticket29617 new file mode 100644 index 0000000000..4d50ea9627 --- /dev/null +++ b/changes/ticket29617 @@ -0,0 +1,4 @@ + o Minor bugfixes (out-of-memory handler): + - When purging the DNS cache because of an out-of-memory condition, + try purging just the older entries at first. Previously, we would + purge the whole thing. Fixes bug 29617; bugfix on 0.3.5.1-alpha. diff --git a/src/feature/relay/dns.c b/src/feature/relay/dns.c index d3660c47ef..cc9f4cf490 100644 --- a/src/feature/relay/dns.c +++ b/src/feature/relay/dns.c @@ -2130,7 +2130,8 @@ dns_cache_handle_oom(time_t now, size_t min_remove_bytes) current_size -= bytes_removed; total_bytes_removed += bytes_removed; - time_inc += 3600; /* Increase time_inc by 1 hour. */ + /* Increase time_inc by a reasonable fraction. */ + time_inc += (MAX_DNS_TTL_AT_EXIT / 4); } while (total_bytes_removed < min_remove_bytes); return total_bytes_removed; From 590d97bc1065bdb93c70695c91de9c548ef802af Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 9 May 2019 12:55:52 -0400 Subject: [PATCH 16/28] hs: Define INTRODUCE_ACK status code in trunnel Remove the hs_intro_ack_status_t enum and move the value into trunnel. Only use these values from now on in the intro point code. Interestingly enough, the client side also re-define these values in hs_cell.h with the hs_cell_introd_ack_status_t enum. Next commit will fix that and force to use the trunnel ABI. Part of #30454 Signed-off-by: David Goulet --- src/or/hs_intropoint.c | 16 ++++++++-------- src/or/hs_intropoint.h | 7 ------- src/trunnel/hs/cell_introduce1.c | 13 +++++++------ src/trunnel/hs/cell_introduce1.h | 3 +++ src/trunnel/hs/cell_introduce1.trunnel | 9 ++++++++- 5 files changed, 26 insertions(+), 22 deletions(-) diff --git a/src/or/hs_intropoint.c b/src/or/hs_intropoint.c index e0ab27bd36..472e4afe98 100644 --- a/src/or/hs_intropoint.c +++ b/src/or/hs_intropoint.c @@ -336,7 +336,7 @@ hs_intro_received_establish_intro(or_circuit_t *circ, const uint8_t *request, * Return 0 on success else a negative value on error which will close the * circuit. */ static int -send_introduce_ack_cell(or_circuit_t *circ, hs_intro_ack_status_t status) +send_introduce_ack_cell(or_circuit_t *circ, uint16_t status) { int ret = -1; uint8_t *encoded_cell = NULL; @@ -433,7 +433,7 @@ handle_introduce1(or_circuit_t *client_circ, const uint8_t *request, int ret = -1; or_circuit_t *service_circ; trn_cell_introduce1_t *parsed_cell; - hs_intro_ack_status_t status = HS_INTRO_ACK_STATUS_SUCCESS; + uint16_t status = TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS; tor_assert(client_circ); tor_assert(request); @@ -448,14 +448,14 @@ handle_introduce1(or_circuit_t *client_circ, const uint8_t *request, "Rejecting %s INTRODUCE1 cell. Responding with NACK.", cell_size == -1 ? "invalid" : "truncated"); /* Inform client that the INTRODUCE1 has a bad format. */ - status = HS_INTRO_ACK_STATUS_BAD_FORMAT; + status = TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT; goto send_ack; } /* Once parsed validate the cell format. */ if (validate_introduce1_parsed_cell(parsed_cell) < 0) { /* Inform client that the INTRODUCE1 has bad format. */ - status = HS_INTRO_ACK_STATUS_BAD_FORMAT; + status = TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT; goto send_ack; } @@ -472,7 +472,7 @@ handle_introduce1(or_circuit_t *client_circ, const uint8_t *request, "Responding with NACK.", safe_str(b64_key), client_circ->p_circ_id); /* Inform the client that we don't know the requested service ID. */ - status = HS_INTRO_ACK_STATUS_UNKNOWN_ID; + status = TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID; goto send_ack; } } @@ -485,12 +485,12 @@ handle_introduce1(or_circuit_t *client_circ, const uint8_t *request, log_warn(LD_PROTOCOL, "Unable to send INTRODUCE2 cell to the service."); /* Inform the client that we can't relay the cell. Use the unknown ID * status code since it means that we do not know the service. */ - status = HS_INTRO_ACK_STATUS_UNKNOWN_ID; + status = TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID; goto send_ack; } /* Success! Send an INTRODUCE_ACK success status onto the client circuit. */ - status = HS_INTRO_ACK_STATUS_SUCCESS; + status = TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS; ret = 0; send_ack: @@ -501,7 +501,7 @@ handle_introduce1(or_circuit_t *client_circ, const uint8_t *request, /* Circuit has been closed on failure of transmission. */ goto done; } - if (status != HS_INTRO_ACK_STATUS_SUCCESS) { + if (status != TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS) { /* We just sent a NACK that is a non success status code so close the * circuit because it's not useful to keep it open. Remember, a client can * only send one INTRODUCE1 cell on a circuit. */ diff --git a/src/or/hs_intropoint.h b/src/or/hs_intropoint.h index 014f9339a6..1c2cc564ab 100644 --- a/src/or/hs_intropoint.h +++ b/src/or/hs_intropoint.h @@ -19,13 +19,6 @@ typedef enum { HS_INTRO_AUTH_KEY_TYPE_ED25519 = 0x02, } hs_intro_auth_key_type_t; -/* INTRODUCE_ACK status code. */ -typedef enum { - HS_INTRO_ACK_STATUS_SUCCESS = 0x0000, - HS_INTRO_ACK_STATUS_UNKNOWN_ID = 0x0001, - HS_INTRO_ACK_STATUS_BAD_FORMAT = 0x0002, -} hs_intro_ack_status_t; - /* Object containing introduction point common data between the service and * the client side. */ typedef struct hs_intropoint_t { diff --git a/src/trunnel/hs/cell_introduce1.c b/src/trunnel/hs/cell_introduce1.c index 358b355cda..b93add64c8 100644 --- a/src/trunnel/hs/cell_introduce1.c +++ b/src/trunnel/hs/cell_introduce1.c @@ -519,6 +519,7 @@ trn_cell_introduce_ack_new(void) trn_cell_introduce_ack_t *val = trunnel_calloc(1, sizeof(trn_cell_introduce_ack_t)); if (NULL == val) return NULL; + val->status = TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT; return val; } @@ -550,7 +551,7 @@ trn_cell_introduce_ack_get_status(const trn_cell_introduce_ack_t *inp) int trn_cell_introduce_ack_set_status(trn_cell_introduce_ack_t *inp, uint16_t val) { - if (! ((val == 0 || val == 1 || val == 2))) { + if (! ((val == TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT || val == TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS || val == TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID))) { TRUNNEL_SET_ERROR_CODE(inp); return -1; } @@ -587,7 +588,7 @@ trn_cell_introduce_ack_check(const trn_cell_introduce_ack_t *obj) return "Object was NULL"; if (obj->trunnel_error_code_) return "A set function failed on this object"; - if (! (obj->status == 0 || obj->status == 1 || obj->status == 2)) + if (! (obj->status == TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT || obj->status == TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS || obj->status == TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID)) return "Integer out of bounds"; { const char *msg; @@ -606,7 +607,7 @@ trn_cell_introduce_ack_encoded_len(const trn_cell_introduce_ack_t *obj) return -1; - /* Length of u16 status IN [0, 1, 2] */ + /* Length of u16 status IN [TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT, TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS, TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID] */ result += 2; /* Length of struct trn_cell_extension extensions */ @@ -638,7 +639,7 @@ trn_cell_introduce_ack_encode(uint8_t *output, const size_t avail, const trn_cel trunnel_assert(encoded_len >= 0); #endif - /* Encode u16 status IN [0, 1, 2] */ + /* Encode u16 status IN [TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT, TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS, TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID] */ trunnel_assert(written <= avail); if (avail - written < 2) goto truncated; @@ -687,11 +688,11 @@ trn_cell_introduce_ack_parse_into(trn_cell_introduce_ack_t *obj, const uint8_t * ssize_t result = 0; (void)result; - /* Parse u16 status IN [0, 1, 2] */ + /* Parse u16 status IN [TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT, TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS, TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID] */ CHECK_REMAINING(2, truncated); obj->status = trunnel_ntohs(trunnel_get_uint16(ptr)); remaining -= 2; ptr += 2; - if (! (obj->status == 0 || obj->status == 1 || obj->status == 2)) + if (! (obj->status == TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT || obj->status == TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS || obj->status == TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID)) goto fail; /* Parse struct trn_cell_extension extensions */ diff --git a/src/trunnel/hs/cell_introduce1.h b/src/trunnel/hs/cell_introduce1.h index fa218adc6d..1bec014b6f 100644 --- a/src/trunnel/hs/cell_introduce1.h +++ b/src/trunnel/hs/cell_introduce1.h @@ -12,6 +12,9 @@ struct trn_cell_extension_st; struct link_specifier_st; #define TRUNNEL_SHA1_LEN 20 #define TRUNNEL_REND_COOKIE_LEN 20 +#define TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS 0 +#define TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID 1 +#define TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT 2 #if !defined(TRUNNEL_OPAQUE) && !defined(TRUNNEL_OPAQUE_TRN_CELL_INTRODUCE1) struct trn_cell_introduce1_st { uint8_t legacy_key_id[TRUNNEL_SHA1_LEN]; diff --git a/src/trunnel/hs/cell_introduce1.trunnel b/src/trunnel/hs/cell_introduce1.trunnel index 574382b163..6d924058f9 100644 --- a/src/trunnel/hs/cell_introduce1.trunnel +++ b/src/trunnel/hs/cell_introduce1.trunnel @@ -12,6 +12,11 @@ extern struct link_specifier; const TRUNNEL_SHA1_LEN = 20; const TRUNNEL_REND_COOKIE_LEN = 20; +/* Introduce ACK status code. */ +const TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS = 0x0000; +const TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID = 0x0001; +const TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT = 0x0002; + /* INTRODUCE1 payload. See details in section 3.2.1. */ struct trn_cell_introduce1 { /* Always zeroed. MUST be checked explicitly by the caller. */ @@ -32,7 +37,9 @@ struct trn_cell_introduce1 { /* INTRODUCE_ACK payload. See details in section 3.2.2. */ struct trn_cell_introduce_ack { /* Status of introduction. */ - u16 status IN [0x0000, 0x0001, 0x0002]; + u16 status IN [TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS, + TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID, + TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT]; /* Extension(s). Reserved fields. */ struct trn_cell_extension extensions; From dcc1d8d15bf11ca1c4e2760bbc47d5fa3df3814d Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 9 May 2019 13:01:15 -0400 Subject: [PATCH 17/28] hs: Get rid of duplicate hs_cell_introd_ack_status_t This enum was the exact same as hs_intro_ack_status_t that was removed at the previous commit. It was used client side when parsing the INTRODUCE_ACK cell. Now, the entire code dealing with the INTRODUCE_ACK cell (both sending and receiving) have been modified to all use the same ABI defined in the trunnel introduce1 file. Finally, the client will default to the normal behavior when receiving an unknown NACK status code which is to note down that we've failed and re-extend to the next intro point. This way, unknown status code won't trigger a different behavior client side. Part of #30454. Signed-off-by: David Goulet --- src/or/hs_cell.c | 4 ++-- src/or/hs_cell.h | 8 -------- src/or/hs_client.c | 18 +++++++++--------- 3 files changed, 11 insertions(+), 19 deletions(-) diff --git a/src/or/hs_cell.c b/src/or/hs_cell.c index 03273a44f9..ba80653a73 100644 --- a/src/or/hs_cell.c +++ b/src/or/hs_cell.c @@ -881,9 +881,9 @@ hs_cell_parse_introduce_ack(const uint8_t *payload, size_t payload_len) * do a special case. */ if (payload_len <= 1) { if (payload_len == 0) { - ret = HS_CELL_INTRO_ACK_SUCCESS; + ret = TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS; } else { - ret = HS_CELL_INTRO_ACK_FAILURE; + ret = TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID; } goto end; } diff --git a/src/or/hs_cell.h b/src/or/hs_cell.h index 958dde4ffc..ed4af3c5af 100644 --- a/src/or/hs_cell.h +++ b/src/or/hs_cell.h @@ -16,14 +16,6 @@ * 3.2.2 of the specification). Below this value, the cell must be padded. */ #define HS_CELL_INTRODUCE1_MIN_SIZE 246 -/* Status code of an INTRODUCE_ACK cell. */ -typedef enum { - HS_CELL_INTRO_ACK_SUCCESS = 0x0000, /* Cell relayed to service. */ - HS_CELL_INTRO_ACK_FAILURE = 0x0001, /* Service ID not recognized */ - HS_CELL_INTRO_ACK_BADFMT = 0x0002, /* Bad message format */ - HS_CELL_INTRO_ACK_NORELAY = 0x0003, /* Can't relay cell to service */ -} hs_cell_introd_ack_status_t; - /* Onion key type found in the INTRODUCE1 cell. */ typedef enum { HS_CELL_ONION_KEY_TYPE_NTOR = 1, diff --git a/src/or/hs_client.c b/src/or/hs_client.c index 8ecefc1209..7d44952e4d 100644 --- a/src/or/hs_client.c +++ b/src/or/hs_client.c @@ -35,6 +35,8 @@ #include "router.h" #include "routerset.h" +#include "trunnel/hs/cell_introduce1.h" + /* Return a human-readable string for the client fetch status code. */ static const char * fetch_status_to_string(hs_client_fetch_status_t status) @@ -1018,23 +1020,21 @@ handle_introduce_ack(origin_circuit_t *circ, const uint8_t *payload, status = hs_cell_parse_introduce_ack(payload, payload_len); switch (status) { - case HS_CELL_INTRO_ACK_SUCCESS: + case TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS: ret = 0; handle_introduce_ack_success(circ); goto end; - case HS_CELL_INTRO_ACK_FAILURE: - case HS_CELL_INTRO_ACK_BADFMT: - case HS_CELL_INTRO_ACK_NORELAY: + case TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID: + case TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT: + /* It is possible that the intro point can send us an unknown status code + * for the NACK that we do not know about like a new code for instance. + * Just fallthrough so we can note down the NACK and re-extend. */ + default: handle_introduce_ack_bad(circ, status); /* We are going to see if we have to close the circuits (IP and RP) or we * can re-extend to a new intro point. */ ret = close_or_reextend_intro_circ(circ); break; - default: - log_info(LD_PROTOCOL, "Unknown INTRODUCE_ACK status code %u from %s", - status, - safe_str_client(extend_info_describe(circ->build_state->chosen_exit))); - break; } end: From 79cfe2ddd7c030e679104588c7e4842c2ff6b239 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 9 May 2019 13:09:44 -0400 Subject: [PATCH 18/28] hs: Remove hs_intro_auth_key_type_t enum Like the previous commit about the INTRODUCE_ACK status code, change all auth key type to use the one defined in the trunnel file. Standardize the use of these auth type to a common ABI. Part of #30454 Signed-off-by: David Goulet --- src/or/hs_cell.c | 5 +++-- src/or/hs_intropoint.c | 10 +++++----- src/or/hs_intropoint.h | 7 ------- src/test/test_hs_cell.c | 2 +- src/test/test_hs_intropoint.c | 4 ++-- src/trunnel/hs/cell_introduce1.c | 13 +++++++------ src/trunnel/hs/cell_introduce1.h | 3 +++ src/trunnel/hs/cell_introduce1.trunnel | 9 ++++++++- 8 files changed, 29 insertions(+), 24 deletions(-) diff --git a/src/or/hs_cell.c b/src/or/hs_cell.c index ba80653a73..7bbefe8fd0 100644 --- a/src/or/hs_cell.c +++ b/src/or/hs_cell.c @@ -441,7 +441,8 @@ introduce1_set_auth_key(trn_cell_introduce1_t *cell, tor_assert(cell); tor_assert(data); /* There is only one possible type for a non legacy cell. */ - trn_cell_introduce1_set_auth_key_type(cell, HS_INTRO_AUTH_KEY_TYPE_ED25519); + trn_cell_introduce1_set_auth_key_type(cell, + TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_ED25519); trn_cell_introduce1_set_auth_key_len(cell, ED25519_PUBKEY_LEN); trn_cell_introduce1_setlen_auth_key(cell, ED25519_PUBKEY_LEN); memcpy(trn_cell_introduce1_getarray_auth_key(cell), @@ -514,7 +515,7 @@ hs_cell_build_establish_intro(const char *circ_nonce, /* Set AUTH_KEY_TYPE: 2 means ed25519 */ trn_cell_establish_intro_set_auth_key_type(cell, - HS_INTRO_AUTH_KEY_TYPE_ED25519); + TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_ED25519); /* Set AUTH_KEY and AUTH_KEY_LEN field. Must also set byte-length of * AUTH_KEY to match */ diff --git a/src/or/hs_intropoint.c b/src/or/hs_intropoint.c index 472e4afe98..a12a3210cb 100644 --- a/src/or/hs_intropoint.c +++ b/src/or/hs_intropoint.c @@ -75,7 +75,7 @@ verify_establish_intro_cell(const trn_cell_establish_intro_t *cell, /* We only reach this function if the first byte of the cell is 0x02 which * means that auth_key_type is of ed25519 type, hence this check should * always pass. See hs_intro_received_establish_intro(). */ - if (BUG(cell->auth_key_type != HS_INTRO_AUTH_KEY_TYPE_ED25519)) { + if (BUG(cell->auth_key_type != TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_ED25519)) { return -1; } @@ -315,10 +315,10 @@ hs_intro_received_establish_intro(or_circuit_t *circ, const uint8_t *request, * ESTABLISH_INTRO and pass it to the appropriate cell handler */ const uint8_t first_byte = request[0]; switch (first_byte) { - case HS_INTRO_AUTH_KEY_TYPE_LEGACY0: - case HS_INTRO_AUTH_KEY_TYPE_LEGACY1: + case TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY0: + case TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY1: return rend_mid_establish_intro_legacy(circ, request, request_len); - case HS_INTRO_AUTH_KEY_TYPE_ED25519: + case TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_ED25519: return handle_establish_intro(circ, request, request_len); default: log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, @@ -396,7 +396,7 @@ validate_introduce1_parsed_cell(const trn_cell_introduce1_t *cell) /* The auth key of an INTRODUCE1 should be of type ed25519 thus leading to a * known fixed length as well. */ if (trn_cell_introduce1_get_auth_key_type(cell) != - HS_INTRO_AUTH_KEY_TYPE_ED25519) { + TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_ED25519) { log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, "Rejecting invalid INTRODUCE1 cell auth key type. " "Responding with NACK."); diff --git a/src/or/hs_intropoint.h b/src/or/hs_intropoint.h index 1c2cc564ab..1891bffacc 100644 --- a/src/or/hs_intropoint.h +++ b/src/or/hs_intropoint.h @@ -12,13 +12,6 @@ #include "crypto_curve25519.h" #include "torcert.h" -/* Authentication key type in an ESTABLISH_INTRO cell. */ -typedef enum { - HS_INTRO_AUTH_KEY_TYPE_LEGACY0 = 0x00, - HS_INTRO_AUTH_KEY_TYPE_LEGACY1 = 0x01, - HS_INTRO_AUTH_KEY_TYPE_ED25519 = 0x02, -} hs_intro_auth_key_type_t; - /* Object containing introduction point common data between the service and * the client side. */ typedef struct hs_intropoint_t { diff --git a/src/test/test_hs_cell.c b/src/test/test_hs_cell.c index 8e15184c2a..f754068a01 100644 --- a/src/test/test_hs_cell.c +++ b/src/test/test_hs_cell.c @@ -50,7 +50,7 @@ test_gen_establish_intro_cell(void *arg) /* Check the contents of the cell */ { /* First byte is the auth key type: make sure its correct */ - tt_int_op(buf[0], OP_EQ, HS_INTRO_AUTH_KEY_TYPE_ED25519); + tt_int_op(buf[0], OP_EQ, TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_ED25519); /* Next two bytes is auth key len */ tt_int_op(ntohs(get_uint16(buf+1)), OP_EQ, ED25519_PUBKEY_LEN); /* Skip to the number of extensions: no extensions */ diff --git a/src/test/test_hs_intropoint.c b/src/test/test_hs_intropoint.c index faa14d9015..f348a076fb 100644 --- a/src/test/test_hs_intropoint.c +++ b/src/test/test_hs_intropoint.c @@ -138,7 +138,7 @@ helper_create_introduce1_cell(void) { size_t auth_key_len = sizeof(auth_key_kp.pubkey); trn_cell_introduce1_set_auth_key_type(cell, - HS_INTRO_AUTH_KEY_TYPE_ED25519); + TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_ED25519); trn_cell_introduce1_set_auth_key_len(cell, auth_key_len); trn_cell_introduce1_setlen_auth_key(cell, auth_key_len); uint8_t *auth_key_ptr = trn_cell_introduce1_getarray_auth_key(cell); @@ -749,7 +749,7 @@ test_introduce1_validation(void *arg) ret = validate_introduce1_parsed_cell(cell); tt_int_op(ret, OP_EQ, -1); /* Reset is to correct value and make sure it's correct. */ - cell->auth_key_type = HS_INTRO_AUTH_KEY_TYPE_ED25519; + cell->auth_key_type = TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_ED25519; ret = validate_introduce1_parsed_cell(cell); tt_int_op(ret, OP_EQ, 0); diff --git a/src/trunnel/hs/cell_introduce1.c b/src/trunnel/hs/cell_introduce1.c index b93add64c8..1fdb954534 100644 --- a/src/trunnel/hs/cell_introduce1.c +++ b/src/trunnel/hs/cell_introduce1.c @@ -50,6 +50,7 @@ trn_cell_introduce1_new(void) trn_cell_introduce1_t *val = trunnel_calloc(1, sizeof(trn_cell_introduce1_t)); if (NULL == val) return NULL; + val->auth_key_type = TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_ED25519; return val; } @@ -121,7 +122,7 @@ trn_cell_introduce1_get_auth_key_type(const trn_cell_introduce1_t *inp) int trn_cell_introduce1_set_auth_key_type(trn_cell_introduce1_t *inp, uint8_t val) { - if (! ((val == 0 || val == 1 || val == 2))) { + if (! ((val == TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_ED25519 || val == TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY0 || val == TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY1))) { TRUNNEL_SET_ERROR_CODE(inp); return -1; } @@ -295,7 +296,7 @@ trn_cell_introduce1_check(const trn_cell_introduce1_t *obj) return "Object was NULL"; if (obj->trunnel_error_code_) return "A set function failed on this object"; - if (! (obj->auth_key_type == 0 || obj->auth_key_type == 1 || obj->auth_key_type == 2)) + if (! (obj->auth_key_type == TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_ED25519 || obj->auth_key_type == TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY0 || obj->auth_key_type == TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY1)) return "Integer out of bounds"; if (TRUNNEL_DYNARRAY_LEN(&obj->auth_key) != obj->auth_key_len) return "Length mismatch for auth_key"; @@ -319,7 +320,7 @@ trn_cell_introduce1_encoded_len(const trn_cell_introduce1_t *obj) /* Length of u8 legacy_key_id[TRUNNEL_SHA1_LEN] */ result += TRUNNEL_SHA1_LEN; - /* Length of u8 auth_key_type IN [0, 1, 2] */ + /* Length of u8 auth_key_type IN [TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_ED25519, TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY0, TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY1] */ result += 1; /* Length of u16 auth_key_len */ @@ -367,7 +368,7 @@ trn_cell_introduce1_encode(uint8_t *output, const size_t avail, const trn_cell_i memcpy(ptr, obj->legacy_key_id, TRUNNEL_SHA1_LEN); written += TRUNNEL_SHA1_LEN; ptr += TRUNNEL_SHA1_LEN; - /* Encode u8 auth_key_type IN [0, 1, 2] */ + /* Encode u8 auth_key_type IN [TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_ED25519, TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY0, TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY1] */ trunnel_assert(written <= avail); if (avail - written < 1) goto truncated; @@ -451,11 +452,11 @@ trn_cell_introduce1_parse_into(trn_cell_introduce1_t *obj, const uint8_t *input, memcpy(obj->legacy_key_id, ptr, TRUNNEL_SHA1_LEN); remaining -= TRUNNEL_SHA1_LEN; ptr += TRUNNEL_SHA1_LEN; - /* Parse u8 auth_key_type IN [0, 1, 2] */ + /* Parse u8 auth_key_type IN [TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_ED25519, TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY0, TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY1] */ CHECK_REMAINING(1, truncated); obj->auth_key_type = (trunnel_get_uint8(ptr)); remaining -= 1; ptr += 1; - if (! (obj->auth_key_type == 0 || obj->auth_key_type == 1 || obj->auth_key_type == 2)) + if (! (obj->auth_key_type == TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_ED25519 || obj->auth_key_type == TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY0 || obj->auth_key_type == TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY1)) goto fail; /* Parse u16 auth_key_len */ diff --git a/src/trunnel/hs/cell_introduce1.h b/src/trunnel/hs/cell_introduce1.h index 1bec014b6f..c94c7d5075 100644 --- a/src/trunnel/hs/cell_introduce1.h +++ b/src/trunnel/hs/cell_introduce1.h @@ -15,6 +15,9 @@ struct link_specifier_st; #define TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS 0 #define TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID 1 #define TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT 2 +#define TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY0 0 +#define TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY1 1 +#define TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_ED25519 2 #if !defined(TRUNNEL_OPAQUE) && !defined(TRUNNEL_OPAQUE_TRN_CELL_INTRODUCE1) struct trn_cell_introduce1_st { uint8_t legacy_key_id[TRUNNEL_SHA1_LEN]; diff --git a/src/trunnel/hs/cell_introduce1.trunnel b/src/trunnel/hs/cell_introduce1.trunnel index 6d924058f9..69da2c1136 100644 --- a/src/trunnel/hs/cell_introduce1.trunnel +++ b/src/trunnel/hs/cell_introduce1.trunnel @@ -17,13 +17,20 @@ const TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS = 0x0000; const TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID = 0x0001; const TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT = 0x0002; +/* Authentication key type. */ +const TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY0 = 0x00; +const TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY1 = 0x01; +const TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_ED25519 = 0x02; + /* INTRODUCE1 payload. See details in section 3.2.1. */ struct trn_cell_introduce1 { /* Always zeroed. MUST be checked explicitly by the caller. */ u8 legacy_key_id[TRUNNEL_SHA1_LEN]; /* Authentication key material. */ - u8 auth_key_type IN [0x00, 0x01, 0x02]; + u8 auth_key_type IN [TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY0, + TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY1, + TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_ED25519]; u16 auth_key_len; u8 auth_key[auth_key_len]; From 9f52b875184098aef91bfdebcfd272a60b3b879e Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 9 May 2019 13:23:14 -0400 Subject: [PATCH 19/28] hs: Add changes file for #30454 Signed-off-by: David Goulet --- changes/ticket30454 | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 changes/ticket30454 diff --git a/changes/ticket30454 b/changes/ticket30454 new file mode 100644 index 0000000000..77c45d0feb --- /dev/null +++ b/changes/ticket30454 @@ -0,0 +1,10 @@ + o Major bugfixes (hidden service v3): + - An intro point could try to send an INTRODUCE_ACK with a status code + that it wasn't able to encode leading to a hard assert() of the relay. + Fortunately, that specific code path can not be reached thus this issue + can't be triggered. We've consolidated the ABI values into trunnel now. + Fixes bug 30454; bugfix on 0.3.0.1-alpha. + - HSv3 client will now be able to properly handle unknown status code from + a INTRODUCE_ACK cell (nack) even if they do not know it. The NACK + behavior will stay the same. This will allow us to extend status code if + we want in the future without breaking the normal client behavior. From 7d3e904a27ac714c33f9b1e14b4d1d18d4042147 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 15 May 2019 15:27:27 -0400 Subject: [PATCH 20/28] trunnel: Remove INTRODUCE1 status code IN statement We want to support parsing a cell with unknown status code so we are forward compatible. Part of #30454 Signed-off-by: David Goulet --- src/trunnel/hs/cell_introduce1.c | 18 +++--------------- src/trunnel/hs/cell_introduce1.trunnel | 4 +--- 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/src/trunnel/hs/cell_introduce1.c b/src/trunnel/hs/cell_introduce1.c index 1fdb954534..cebfa28673 100644 --- a/src/trunnel/hs/cell_introduce1.c +++ b/src/trunnel/hs/cell_introduce1.c @@ -520,7 +520,6 @@ trn_cell_introduce_ack_new(void) trn_cell_introduce_ack_t *val = trunnel_calloc(1, sizeof(trn_cell_introduce_ack_t)); if (NULL == val) return NULL; - val->status = TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT; return val; } @@ -552,10 +551,6 @@ trn_cell_introduce_ack_get_status(const trn_cell_introduce_ack_t *inp) int trn_cell_introduce_ack_set_status(trn_cell_introduce_ack_t *inp, uint16_t val) { - if (! ((val == TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT || val == TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS || val == TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID))) { - TRUNNEL_SET_ERROR_CODE(inp); - return -1; - } inp->status = val; return 0; } @@ -589,8 +584,6 @@ trn_cell_introduce_ack_check(const trn_cell_introduce_ack_t *obj) return "Object was NULL"; if (obj->trunnel_error_code_) return "A set function failed on this object"; - if (! (obj->status == TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT || obj->status == TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS || obj->status == TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID)) - return "Integer out of bounds"; { const char *msg; if (NULL != (msg = trn_cell_extension_check(obj->extensions))) @@ -608,7 +601,7 @@ trn_cell_introduce_ack_encoded_len(const trn_cell_introduce_ack_t *obj) return -1; - /* Length of u16 status IN [TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT, TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS, TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID] */ + /* Length of u16 status */ result += 2; /* Length of struct trn_cell_extension extensions */ @@ -640,7 +633,7 @@ trn_cell_introduce_ack_encode(uint8_t *output, const size_t avail, const trn_cel trunnel_assert(encoded_len >= 0); #endif - /* Encode u16 status IN [TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT, TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS, TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID] */ + /* Encode u16 status */ trunnel_assert(written <= avail); if (avail - written < 2) goto truncated; @@ -689,12 +682,10 @@ trn_cell_introduce_ack_parse_into(trn_cell_introduce_ack_t *obj, const uint8_t * ssize_t result = 0; (void)result; - /* Parse u16 status IN [TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT, TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS, TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID] */ + /* Parse u16 status */ CHECK_REMAINING(2, truncated); obj->status = trunnel_ntohs(trunnel_get_uint16(ptr)); remaining -= 2; ptr += 2; - if (! (obj->status == TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT || obj->status == TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS || obj->status == TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID)) - goto fail; /* Parse struct trn_cell_extension extensions */ result = trn_cell_extension_parse(&obj->extensions, ptr, remaining); @@ -710,9 +701,6 @@ trn_cell_introduce_ack_parse_into(trn_cell_introduce_ack_t *obj, const uint8_t * relay_fail: trunnel_assert(result < 0); return result; - fail: - result = -1; - return result; } ssize_t diff --git a/src/trunnel/hs/cell_introduce1.trunnel b/src/trunnel/hs/cell_introduce1.trunnel index 69da2c1136..bbb091d2b8 100644 --- a/src/trunnel/hs/cell_introduce1.trunnel +++ b/src/trunnel/hs/cell_introduce1.trunnel @@ -44,9 +44,7 @@ struct trn_cell_introduce1 { /* INTRODUCE_ACK payload. See details in section 3.2.2. */ struct trn_cell_introduce_ack { /* Status of introduction. */ - u16 status IN [TRUNNEL_HS_INTRO_ACK_STATUS_SUCCESS, - TRUNNEL_HS_INTRO_ACK_STATUS_UNKNOWN_ID, - TRUNNEL_HS_INTRO_ACK_STATUS_BAD_FORMAT]; + u16 status; /* Extension(s). Reserved fields. */ struct trn_cell_extension extensions; From 56908c6f1c63d32bef7011c811976e24156f17d3 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 15 May 2019 15:30:29 -0400 Subject: [PATCH 21/28] hs: Remove hs_cell_onion_key_type_t enum Unify this with the trunnel ABI so we don't duplicate. Part of #30454 Signed-off-by: David Goulet --- src/or/hs_cell.c | 7 ++++--- src/or/hs_cell.h | 5 ----- src/trunnel/hs/cell_introduce1.c | 14 +++++++------- src/trunnel/hs/cell_introduce1.h | 1 + src/trunnel/hs/cell_introduce1.trunnel | 5 ++++- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/or/hs_cell.c b/src/or/hs_cell.c index 7bbefe8fd0..7b7e9379c3 100644 --- a/src/or/hs_cell.c +++ b/src/or/hs_cell.c @@ -160,11 +160,12 @@ parse_introduce2_encrypted(const uint8_t *decrypted_data, } if (trn_cell_introduce_encrypted_get_onion_key_type(enc_cell) != - HS_CELL_ONION_KEY_TYPE_NTOR) { + TRUNNEL_HS_INTRO_ONION_KEY_TYPE_NTOR) { log_info(LD_REND, "INTRODUCE2 onion key type is invalid. Got %u but " "expected %u on circuit %u for service %s", trn_cell_introduce_encrypted_get_onion_key_type(enc_cell), - HS_CELL_ONION_KEY_TYPE_NTOR, TO_CIRCUIT(circ)->n_circ_id, + TRUNNEL_HS_INTRO_ONION_KEY_TYPE_NTOR, + TO_CIRCUIT(circ)->n_circ_id, safe_str_client(service->onion_address)); goto err; } @@ -257,7 +258,7 @@ introduce1_set_encrypted_onion_key(trn_cell_introduce_encrypted_t *cell, tor_assert(onion_pk); /* There is only one possible key type for a non legacy cell. */ trn_cell_introduce_encrypted_set_onion_key_type(cell, - HS_CELL_ONION_KEY_TYPE_NTOR); + TRUNNEL_HS_INTRO_ONION_KEY_TYPE_NTOR); trn_cell_introduce_encrypted_set_onion_key_len(cell, CURVE25519_PUBKEY_LEN); trn_cell_introduce_encrypted_setlen_onion_key(cell, CURVE25519_PUBKEY_LEN); memcpy(trn_cell_introduce_encrypted_getarray_onion_key(cell), onion_pk, diff --git a/src/or/hs_cell.h b/src/or/hs_cell.h index ed4af3c5af..49b29a43bf 100644 --- a/src/or/hs_cell.h +++ b/src/or/hs_cell.h @@ -16,11 +16,6 @@ * 3.2.2 of the specification). Below this value, the cell must be padded. */ #define HS_CELL_INTRODUCE1_MIN_SIZE 246 -/* Onion key type found in the INTRODUCE1 cell. */ -typedef enum { - HS_CELL_ONION_KEY_TYPE_NTOR = 1, -} hs_cell_onion_key_type_t; - /* This data structure contains data that we need to build an INTRODUCE1 cell * used by the INTRODUCE1 build function. */ typedef struct hs_cell_introduce1_data_t { diff --git a/src/trunnel/hs/cell_introduce1.c b/src/trunnel/hs/cell_introduce1.c index cebfa28673..53b3d299f2 100644 --- a/src/trunnel/hs/cell_introduce1.c +++ b/src/trunnel/hs/cell_introduce1.c @@ -723,7 +723,7 @@ trn_cell_introduce_encrypted_new(void) trn_cell_introduce_encrypted_t *val = trunnel_calloc(1, sizeof(trn_cell_introduce_encrypted_t)); if (NULL == val) return NULL; - val->onion_key_type = 1; + val->onion_key_type = TRUNNEL_HS_INTRO_ONION_KEY_TYPE_NTOR; return val; } @@ -827,7 +827,7 @@ trn_cell_introduce_encrypted_get_onion_key_type(const trn_cell_introduce_encrypt int trn_cell_introduce_encrypted_set_onion_key_type(trn_cell_introduce_encrypted_t *inp, uint8_t val) { - if (! ((val == 1))) { + if (! ((val == TRUNNEL_HS_INTRO_ONION_KEY_TYPE_NTOR))) { TRUNNEL_SET_ERROR_CODE(inp); return -1; } @@ -1069,7 +1069,7 @@ trn_cell_introduce_encrypted_check(const trn_cell_introduce_encrypted_t *obj) if (NULL != (msg = trn_cell_extension_check(obj->extensions))) return msg; } - if (! (obj->onion_key_type == 1)) + if (! (obj->onion_key_type == TRUNNEL_HS_INTRO_ONION_KEY_TYPE_NTOR)) return "Integer out of bounds"; if (TRUNNEL_DYNARRAY_LEN(&obj->onion_key) != obj->onion_key_len) return "Length mismatch for onion_key"; @@ -1102,7 +1102,7 @@ trn_cell_introduce_encrypted_encoded_len(const trn_cell_introduce_encrypted_t *o /* Length of struct trn_cell_extension extensions */ result += trn_cell_extension_encoded_len(obj->extensions); - /* Length of u8 onion_key_type IN [1] */ + /* Length of u8 onion_key_type IN [TRUNNEL_HS_INTRO_ONION_KEY_TYPE_NTOR] */ result += 1; /* Length of u16 onion_key_len */ @@ -1166,7 +1166,7 @@ trn_cell_introduce_encrypted_encode(uint8_t *output, const size_t avail, const t goto fail; /* XXXXXXX !*/ written += result; ptr += result; - /* Encode u8 onion_key_type IN [1] */ + /* Encode u8 onion_key_type IN [TRUNNEL_HS_INTRO_ONION_KEY_TYPE_NTOR] */ trunnel_assert(written <= avail); if (avail - written < 1) goto truncated; @@ -1270,11 +1270,11 @@ trn_cell_introduce_encrypted_parse_into(trn_cell_introduce_encrypted_t *obj, con trunnel_assert((size_t)result <= remaining); remaining -= result; ptr += result; - /* Parse u8 onion_key_type IN [1] */ + /* Parse u8 onion_key_type IN [TRUNNEL_HS_INTRO_ONION_KEY_TYPE_NTOR] */ CHECK_REMAINING(1, truncated); obj->onion_key_type = (trunnel_get_uint8(ptr)); remaining -= 1; ptr += 1; - if (! (obj->onion_key_type == 1)) + if (! (obj->onion_key_type == TRUNNEL_HS_INTRO_ONION_KEY_TYPE_NTOR)) goto fail; /* Parse u16 onion_key_len */ diff --git a/src/trunnel/hs/cell_introduce1.h b/src/trunnel/hs/cell_introduce1.h index c94c7d5075..986a531ca7 100644 --- a/src/trunnel/hs/cell_introduce1.h +++ b/src/trunnel/hs/cell_introduce1.h @@ -18,6 +18,7 @@ struct link_specifier_st; #define TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY0 0 #define TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY1 1 #define TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_ED25519 2 +#define TRUNNEL_HS_INTRO_ONION_KEY_TYPE_NTOR 1 #if !defined(TRUNNEL_OPAQUE) && !defined(TRUNNEL_OPAQUE_TRN_CELL_INTRODUCE1) struct trn_cell_introduce1_st { uint8_t legacy_key_id[TRUNNEL_SHA1_LEN]; diff --git a/src/trunnel/hs/cell_introduce1.trunnel b/src/trunnel/hs/cell_introduce1.trunnel index bbb091d2b8..5911c695a2 100644 --- a/src/trunnel/hs/cell_introduce1.trunnel +++ b/src/trunnel/hs/cell_introduce1.trunnel @@ -22,6 +22,9 @@ const TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY0 = 0x00; const TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_LEGACY1 = 0x01; const TRUNNEL_HS_INTRO_AUTH_KEY_TYPE_ED25519 = 0x02; +/* Onion key type. */ +const TRUNNEL_HS_INTRO_ONION_KEY_TYPE_NTOR = 0x01; + /* INTRODUCE1 payload. See details in section 3.2.1. */ struct trn_cell_introduce1 { /* Always zeroed. MUST be checked explicitly by the caller. */ @@ -59,7 +62,7 @@ struct trn_cell_introduce_encrypted { struct trn_cell_extension extensions; /* Onion key material. */ - u8 onion_key_type IN [0x01]; + u8 onion_key_type IN [TRUNNEL_HS_INTRO_ONION_KEY_TYPE_NTOR]; u16 onion_key_len; u8 onion_key[onion_key_len]; From 5cbd71b977f1c3ae0b9dc0f9e63094941ece015c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 29 May 2019 11:00:09 -0400 Subject: [PATCH 22/28] Make get_proxy_type() connection-specific Previously, we were looking at our global settings to see what kind of proxy we had. But doing this would sometimes give us the wrong results when we had ClientTransportPlugin configured but we weren't using it for a particular connection. In several places in the code, we had added checks to see if we were _really_ using a PT or whether we were using a socks proxy, but we had forgotten to do so in at least once case. Instead, since every time we call this function we are asking about a single connection, it is probably best just to make this function connection-specific. Fixes bug 29670; bugfix on 0.2.6.2-alpha. --- changes/bug29670 | 4 ++++ src/core/mainloop/connection.c | 33 +++++++++++++++++++++------------ 2 files changed, 25 insertions(+), 12 deletions(-) create mode 100644 changes/bug29670 diff --git a/changes/bug29670 b/changes/bug29670 new file mode 100644 index 0000000000..00b0c33327 --- /dev/null +++ b/changes/bug29670 @@ -0,0 +1,4 @@ + o Minor bugfixes (configuration, proxies): + - Fix a bug that prevented us from supporting SOCKS5 proxies that want + authentication along with configued (but unused!) + ClientTransportPlugins. Fixes bug 29670; bugfix on 0.2.6.1-alpha. diff --git a/src/core/mainloop/connection.c b/src/core/mainloop/connection.c index 7b8dc7f364..29ef26ca5a 100644 --- a/src/core/mainloop/connection.c +++ b/src/core/mainloop/connection.c @@ -182,7 +182,7 @@ static const char *connection_proxy_state_to_string(int state); static int connection_read_https_proxy_response(connection_t *conn); static void connection_send_socks5_connect(connection_t *conn); static const char *proxy_type_to_string(int proxy_type); -static int get_proxy_type(void); +static int conn_get_proxy_type(const connection_t *conn); const tor_addr_t *conn_get_outbound_address(sa_family_t family, const or_options_t *options, unsigned int conn_type); static void reenable_blocked_connection_init(const or_options_t *options); @@ -2260,18 +2260,27 @@ connection_proxy_state_to_string(int state) return states[state]; } -/** Returns the global proxy type used by tor. Use this function for - * logging or high-level purposes, don't use it to fill the +/** Returns the proxy type used by tor for a single connection, for + * logging or high-level purposes. Don't use it to fill the * proxy_type field of or_connection_t; use the actual proxy * protocol instead.*/ static int -get_proxy_type(void) +conn_get_proxy_type(const connection_t *conn) { const or_options_t *options = get_options(); - if (options->ClientTransportPlugin) - return PROXY_PLUGGABLE; - else if (options->HTTPSProxy) + if (options->ClientTransportPlugin) { + /* If we have plugins configured *and* this addr/port is a known bridge + * with a transport, then we should be PROXY_PLUGGABLE. */ + const transport_t *transport = NULL; + int r; + r = get_transport_by_bridge_addrport(&conn->addr, conn->port, &transport); + if (r == 0 && transport) + return PROXY_PLUGGABLE; + } + + /* In all other cases, we're using a global proxy. */ + if (options->HTTPSProxy) return PROXY_CONNECT; else if (options->Socks4Proxy) return PROXY_SOCKS4; @@ -2358,7 +2367,7 @@ connection_proxy_connect(connection_t *conn, int type) arguments to transmit. If we do, compress all arguments to a single string in 'socks_args_string': */ - if (get_proxy_type() == PROXY_PLUGGABLE) { + if (conn_get_proxy_type(conn) == PROXY_PLUGGABLE) { socks_args_string = pt_get_socks_args_for_proxy_addrport(&conn->addr, conn->port); if (socks_args_string) @@ -2418,7 +2427,7 @@ connection_proxy_connect(connection_t *conn, int type) Socks5ProxyUsername or if we want to pass arguments to our pluggable transport proxy: */ if ((options->Socks5ProxyUsername) || - (get_proxy_type() == PROXY_PLUGGABLE && + (conn_get_proxy_type(conn) == PROXY_PLUGGABLE && (get_socks_args_by_bridge_addrport(&conn->addr, conn->port)))) { /* number of auth methods */ buf[1] = 2; @@ -2611,16 +2620,16 @@ connection_read_proxy_handshake(connection_t *conn) const char *user, *pass; char *socks_args_string = NULL; - if (get_proxy_type() == PROXY_PLUGGABLE) { + if (conn_get_proxy_type(conn) == PROXY_PLUGGABLE) { socks_args_string = pt_get_socks_args_for_proxy_addrport(&conn->addr, conn->port); if (!socks_args_string) { - log_warn(LD_NET, "Could not create SOCKS args string."); + log_warn(LD_NET, "Could not create SOCKS args string for PT."); ret = -1; break; } - log_debug(LD_NET, "SOCKS5 arguments: %s", socks_args_string); + log_debug(LD_NET, "PT SOCKS5 arguments: %s", socks_args_string); tor_assert(strlen(socks_args_string) > 0); tor_assert(strlen(socks_args_string) <= MAX_SOCKS5_AUTH_SIZE_TOTAL); From 70b85358afd0a8d4291489f9e9f5654ab2d53371 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 31 May 2019 08:26:10 -0400 Subject: [PATCH 23/28] Fix a logic error in deciding whether to accept SessionGroup= Fixes bug 22619; bugfix on 0.2.7.2-alpha --- changes/bug22619 | 3 +++ src/app/config/config.c | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 changes/bug22619 diff --git a/changes/bug22619 b/changes/bug22619 new file mode 100644 index 0000000000..9c71996f5b --- /dev/null +++ b/changes/bug22619 @@ -0,0 +1,3 @@ + o Minor bugfixes (circuit isolation): + - Fix a logic error that prevented the SessionGroup sub-option from + being accepted. Fixes bug 22619; bugfix on 0.2.7.2-alpha. diff --git a/src/app/config/config.c b/src/app/config/config.c index 2a504d3065..3525597591 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -7080,7 +7080,7 @@ parse_port_config(smartlist_t *out, if (!strcasecmpstart(elt, "SessionGroup=")) { int group = (int)tor_parse_long(elt+strlen("SessionGroup="), 10, 0, INT_MAX, &ok, NULL); - if (!ok || !allow_no_stream_options) { + if (!ok || allow_no_stream_options) { log_warn(LD_CONFIG, "Invalid %sPort option '%s'", portname, escaped(elt)); goto err; From 3c3158f1826826d9b4e8841bc67855cca0fc883b Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 31 May 2019 09:03:16 -0400 Subject: [PATCH 24/28] Fix some tests for CL_PORT_NO_STREAM_OPTIONS The comment in the tests was correct: this option _was_ inverted wrt SessionGroup=. --- src/test/test_config.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/test/test_config.c b/src/test/test_config.c index c342d8cca4..0de6b12919 100644 --- a/src/test/test_config.c +++ b/src/test/test_config.c @@ -4568,16 +4568,14 @@ test_config_parse_port_config__ports__ports_given(void *data) "127.0.0.44", 0, CL_PORT_NO_STREAM_OPTIONS); tt_int_op(ret, OP_EQ, -1); - // TODO: this seems wrong. Shouldn't it be the other way around? - // Potential bug. - // Test failure for a SessionGroup argument with valid value but with stream - // options allowed + // Test failure for a SessionGroup argument with valid value but with no + // stream options allowed config_free_lines(config_port_invalid); config_port_invalid = NULL; SMARTLIST_FOREACH(slout,port_cfg_t *,pf,port_cfg_free(pf)); smartlist_clear(slout); config_port_invalid = mock_config_line("DNSPort", "42 SessionGroup=123"); ret = parse_port_config(slout, config_port_invalid, "DNS", 0, - "127.0.0.44", 0, 0); + "127.0.0.44", 0, CL_PORT_NO_STREAM_OPTIONS); tt_int_op(ret, OP_EQ, -1); // Test failure for more than one SessionGroup argument @@ -4587,7 +4585,7 @@ test_config_parse_port_config__ports__ports_given(void *data) config_port_invalid = mock_config_line("DNSPort", "42 SessionGroup=123 " "SessionGroup=321"); ret = parse_port_config(slout, config_port_invalid, "DNS", 0, - "127.0.0.44", 0, CL_PORT_NO_STREAM_OPTIONS); + "127.0.0.44", 0, 0); tt_int_op(ret, OP_EQ, -1); // Test success with a sessiongroup options @@ -4596,7 +4594,7 @@ test_config_parse_port_config__ports__ports_given(void *data) smartlist_clear(slout); config_port_valid = mock_config_line("DNSPort", "42 SessionGroup=1111122"); ret = parse_port_config(slout, config_port_valid, "DNS", 0, - "127.0.0.44", 0, CL_PORT_NO_STREAM_OPTIONS); + "127.0.0.44", 0, 0); tt_int_op(ret, OP_EQ, 0); tt_int_op(smartlist_len(slout), OP_EQ, 1); port_cfg = (port_cfg_t *)smartlist_get(slout, 0); From 5b3c8865842f3b12d3ea35db19e8feb135bb46dc Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 31 May 2019 11:46:58 -0400 Subject: [PATCH 25/28] Consider dir info to have changed when the bridges change Otherwise, we won't realize that we haven't got enough bridge information to build circuits. Part of a fix for ticket 29875. --- src/app/config/config.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index 2a504d3065..7f6657db3c 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -2412,7 +2412,8 @@ options_act(const or_options_t *old_options) if (!bool_eq(directory_fetches_dir_info_early(options), directory_fetches_dir_info_early(old_options)) || !bool_eq(directory_fetches_dir_info_later(options), - directory_fetches_dir_info_later(old_options))) { + directory_fetches_dir_info_later(old_options)) || + !config_lines_eq(old_options->Bridges, options->Bridges)) { /* Make sure update_router_have_minimum_dir_info() gets called. */ router_dir_info_changed(); /* We might need to download a new consensus status later or sooner than From 8015979eeba2c8b10fb058318628debf228fb023 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 31 May 2019 12:19:29 -0400 Subject: [PATCH 26/28] num_bridges_usable(): only count configured bridges. When this function was implemented, it counted all the entry guards in the bridge set. But this included previously configured bridges, as well as currently configured ones! Instead, only count the _filtered_ bridges (ones that are configured and possibly reachable) as maybe usable. Fixes bug 29875; bugfix on 0.3.0.1-alpha. --- src/feature/client/entrynodes.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/feature/client/entrynodes.c b/src/feature/client/entrynodes.c index e543289ce0..15ec830594 100644 --- a/src/feature/client/entrynodes.c +++ b/src/feature/client/entrynodes.c @@ -3300,6 +3300,9 @@ num_bridges_usable,(int use_maybe_reachable)) } SMARTLIST_FOREACH_BEGIN(gs->sampled_entry_guards, entry_guard_t *, guard) { + /* Not a bridge, or not one we are configured to be able to use. */ + if (! guard->is_filtered_guard) + continue; /* Definitely not usable */ if (guard->is_reachable == GUARD_REACHABLE_NO) continue; From c09e7a4e715d8266517896cfbc8fc80f19feba85 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 31 May 2019 12:39:52 -0400 Subject: [PATCH 27/28] changes file for bug 29875. --- changes/bug29875 | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 changes/bug29875 diff --git a/changes/bug29875 b/changes/bug29875 new file mode 100644 index 0000000000..58a1c871cd --- /dev/null +++ b/changes/bug29875 @@ -0,0 +1,11 @@ + o Major bugfixes (bridges): + - Do not count previously configured working bridges towards our total of + working bridges. Previously, when Tor's list of bridges changed, it + would think that the old bridges were still usable, and delay fetching + router descriptors for the new ones. Fixes part of bug 29875; bugfix + on 0.3.0.1-alpha. + - Consider our directory information to have changed when our list of + bridges changes. Previously, Tor would not re-compute the status of its + directory information when bridges changed, and therefore would not + realize that it was no longer able to build circuits. Fixes part of bug + 29875. From a6399da598101d8046cbf7ab2d80051fd8820e2f Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 5 Jun 2019 11:50:44 -0700 Subject: [PATCH 28/28] Bug 29034: Cleanup hs circuitmap when purpose changes. Leave the other rend and hs_ident data around until circuit free, since code may still try to inspect it after marking the circuit for close. The circuitmap is the important thing to clean up, since repurposed intropoints must be removed from this map to ensure validity. --- changes/bug29034 | 5 +++++ src/core/or/circuituse.c | 6 ++++++ 2 files changed, 11 insertions(+) create mode 100644 changes/bug29034 diff --git a/changes/bug29034 b/changes/bug29034 new file mode 100644 index 0000000000..e7aa9af00b --- /dev/null +++ b/changes/bug29034 @@ -0,0 +1,5 @@ + o Major bugfixes (Onion service reachability): + - Properly clean up the introduction point map when circuits change purpose + from onion service circuits to pathbias, measurement, or other circuit types. + This should fix some service-side instances of introduction point failure. + Fixes bug 29034; bugfix on 0.3.2.1-alpha. diff --git a/src/core/or/circuituse.c b/src/core/or/circuituse.c index 02bfa15fb3..000a7c36da 100644 --- a/src/core/or/circuituse.c +++ b/src/core/or/circuituse.c @@ -3066,6 +3066,12 @@ circuit_change_purpose(circuit_t *circ, uint8_t new_purpose) circ->purpose, circuit_purpose_to_string(new_purpose), new_purpose); + + /* Take specific actions if we are repurposing a hidden service circuit. */ + if (circuit_purpose_is_hidden_service(circ->purpose) && + !circuit_purpose_is_hidden_service(new_purpose)) { + hs_circ_cleanup(circ); + } } old_purpose = circ->purpose;