commit bad1181b5d1bef55e060d64a9d4ef9278619495b Author: teor <teor@torproject.org> Date: Wed Apr 15 11:34:12 2020 +1000 relay/circuitbuild: Consider IPv6-only extends valid Allow extend cells with IPv6-only link specifiers. Warn and fail if both IPv4 and IPv6 are invalid. Also warn if the IPv4 or IPv6 addresses are unexpectedly internal, but continue with the valid address. Part of 33817. --- src/feature/relay/circuitbuild_relay.c | 50 +++++++++---- src/test/test_circuitbuild.c | 125 ++++++++++++++++++++++++++++++++- 2 files changed, 161 insertions(+), 14 deletions(-) diff --git a/src/feature/relay/circuitbuild_relay.c b/src/feature/relay/circuitbuild_relay.c index 05146f1b6..080781f71 100644 --- a/src/feature/relay/circuitbuild_relay.c +++ b/src/feature/relay/circuitbuild_relay.c @@ -123,11 +123,14 @@ circuit_extend_add_ed25519_helper(struct extend_cell_t *ec) * and are allowed by the current ExtendAllowPrivateAddresses config. * * If they are valid, return 0. - * Otherwise, if they are invalid, log a warning at <b>log_level</b>, - * and return -1. + * Otherwise, if they are invalid, return -1. + * If <b>log_zero_addrs</b> is true, log warnings about zero addresses at + * <b>log_level</b>. If <b>log_internal_addrs</b> is true, log warnings about + * internal addresses at <b>log_level</b>. */ static int circuit_extend_addr_port_helper(const struct tor_addr_port_t *ap, + bool log_zero_addrs, bool log_internal_addrs, int log_level) { /* It's safe to print the family. But we don't want to print the address, @@ -135,19 +138,23 @@ circuit_extend_addr_port_helper(const struct tor_addr_port_t *ap, * But some internal addresses might be.)*/ if (!tor_addr_port_is_valid_ap(ap, 0)) { - log_fn(log_level, LD_PROTOCOL, - "Client asked me to extend to a zero destination port or " - "%s address '%s'.", - fmt_addr_family(&ap->addr), safe_str(fmt_addrport_ap(ap))); + if (log_zero_addrs) { + log_fn(log_level, LD_PROTOCOL, + "Client asked me to extend to a zero destination port or " + "%s address '%s'.", + fmt_addr_family(&ap->addr), safe_str(fmt_addrport_ap(ap))); + } return -1; } if (tor_addr_is_internal(&ap->addr, 0) && !get_options()->ExtendAllowPrivateAddresses) { - log_fn(log_level, LD_PROTOCOL, - "Client asked me to extend to a private %s address '%s'.", - fmt_addr_family(&ap->addr), - safe_str(fmt_and_decorate_addr(&ap->addr))); + if (log_internal_addrs) { + log_fn(log_level, LD_PROTOCOL, + "Client asked me to extend to a private %s address '%s'.", + fmt_addr_family(&ap->addr), + safe_str(fmt_and_decorate_addr(&ap->addr))); + } return -1; } @@ -174,9 +181,28 @@ circuit_extend_lspec_valid_helper(const struct extend_cell_t *ec, return -1; } - if (circuit_extend_addr_port_helper(&ec->orport_ipv4, - LOG_PROTOCOL_WARN) < 0) { + /* Check the addresses, without logging */ + const int ipv4_valid = + (circuit_extend_addr_port_helper(&ec->orport_ipv4, false, false, 0) == 0); + const int ipv6_valid = + (circuit_extend_addr_port_helper(&ec->orport_ipv6, false, false, 0) == 0); + /* We need at least one valid address */ + if (!ipv4_valid && !ipv6_valid) { + /* Now, log the invalid addresses at protocol warning level */ + circuit_extend_addr_port_helper(&ec->orport_ipv4, true, true, + LOG_PROTOCOL_WARN); + circuit_extend_addr_port_helper(&ec->orport_ipv6, true, true, + LOG_PROTOCOL_WARN); + /* And fail */ return -1; + } else if (!ipv4_valid) { + /* Always log unexpected internal addresses, but go on to use the other + * valid address */ + circuit_extend_addr_port_helper(&ec->orport_ipv4, false, true, + LOG_PROTOCOL_WARN); + } else if (!ipv6_valid) { + circuit_extend_addr_port_helper(&ec->orport_ipv6, false, true, + LOG_PROTOCOL_WARN); } IF_BUG_ONCE(circ->magic != OR_CIRCUIT_MAGIC) { diff --git a/src/test/test_circuitbuild.c b/src/test/test_circuitbuild.c index 932248084..a8d6323e4 100644 --- a/src/test/test_circuitbuild.c +++ b/src/test/test_circuitbuild.c @@ -474,6 +474,9 @@ mock_get_options(void) #define PUBLIC_IPV4 "1.2.3.4" #define INTERNAL_IPV4 "0.0.0.1" +#define PUBLIC_IPV6 "1234::cdef" +#define INTERNAL_IPV6 "::1" + #define VALID_PORT 0x1234 /* Test the different cases in circuit_extend_lspec_valid_helper(). */ @@ -519,7 +522,7 @@ test_circuit_extend_lspec_valid(void *arg) tor_end_capture_bugs_(); mock_clean_saved_logs(); - /* IPv4 addr or port are 0, these should fail */ + /* IPv4 and IPv6 addr and port are all zero, this should fail */ tt_int_op(circuit_extend_lspec_valid_helper(ec, circ), OP_EQ, -1); expect_log_msg("Client asked me to extend to a zero destination port " "or unspecified address '[scrubbed]'.\n"); @@ -527,35 +530,103 @@ test_circuit_extend_lspec_valid(void *arg) /* Now ask for the actual address in the logs */ fake_options->SafeLogging_ = SAFELOG_SCRUB_NONE; + + /* IPv4 port is 0, IPv6 addr and port are both zero, this should fail */ tor_addr_parse(&ec->orport_ipv4.addr, PUBLIC_IPV4); tt_int_op(circuit_extend_lspec_valid_helper(ec, circ), OP_EQ, -1); expect_log_msg("Client asked me to extend to a zero destination port " "or IPv4 address '1.2.3.4:0'.\n"); mock_clean_saved_logs(); - tor_addr_make_null(&ec->orport_ipv4.addr, AF_INET); + tor_addr_port_make_null_ap(&ec->orport_ipv4, AF_INET); + tor_addr_port_make_null_ap(&ec->orport_ipv6, AF_INET6); + /* IPv4 addr is 0, IPv6 addr and port are both zero, this should fail */ ec->orport_ipv4.port = VALID_PORT; tt_int_op(circuit_extend_lspec_valid_helper(ec, circ), OP_EQ, -1); expect_log_msg("Client asked me to extend to a zero destination port " "or IPv4 address '0.0.0.0:4660'.\n"); mock_clean_saved_logs(); ec->orport_ipv4.port = 0; + tor_addr_port_make_null_ap(&ec->orport_ipv4, AF_INET); + tor_addr_port_make_null_ap(&ec->orport_ipv6, AF_INET6); /* IPv4 addr is internal, and port is valid. + * (IPv6 addr and port are both zero.) + * Result depends on ExtendAllowPrivateAddresses. */ + tor_addr_parse(&ec->orport_ipv4.addr, INTERNAL_IPV4); + ec->orport_ipv4.port = VALID_PORT; + + fake_options->ExtendAllowPrivateAddresses = 0; + tt_int_op(circuit_extend_lspec_valid_helper(ec, circ), OP_EQ, -1); + expect_log_msg("Client asked me to extend " + "to a private IPv4 address '0.0.0.1'.\n"); + mock_clean_saved_logs(); + fake_options->ExtendAllowPrivateAddresses = 0; + tor_addr_port_make_null_ap(&ec->orport_ipv4, AF_INET); + tor_addr_port_make_null_ap(&ec->orport_ipv6, AF_INET6); + + /* Now do the same tests, but for IPv6 */ + + /* IPv6 port is 0, IPv4 addr and port are both zero, this should fail */ + tor_addr_parse(&ec->orport_ipv6.addr, PUBLIC_IPV6); + tt_int_op(circuit_extend_lspec_valid_helper(ec, circ), OP_EQ, -1); + expect_log_msg("Client asked me to extend to a zero destination port " + "or IPv6 address '[1234::cdef]:0'.\n"); + mock_clean_saved_logs(); + tor_addr_port_make_null_ap(&ec->orport_ipv4, AF_INET); + tor_addr_port_make_null_ap(&ec->orport_ipv6, AF_INET6); + + /* IPv6 addr is 0, IPv4 addr and port are both zero, this should fail */ + ec->orport_ipv6.port = VALID_PORT; + tt_int_op(circuit_extend_lspec_valid_helper(ec, circ), OP_EQ, -1); + expect_log_msg("Client asked me to extend to a zero destination port " + "or IPv6 address '[::]:4660'.\n"); + mock_clean_saved_logs(); + ec->orport_ipv4.port = 0; + tor_addr_port_make_null_ap(&ec->orport_ipv4, AF_INET); + tor_addr_port_make_null_ap(&ec->orport_ipv6, AF_INET6); + + /* IPv6 addr is internal, and port is valid. + * (IPv4 addr and port are both zero.) + * Result depends on ExtendAllowPrivateAddresses. */ + tor_addr_parse(&ec->orport_ipv6.addr, INTERNAL_IPV6); + ec->orport_ipv6.port = VALID_PORT; + + fake_options->ExtendAllowPrivateAddresses = 0; + tt_int_op(circuit_extend_lspec_valid_helper(ec, circ), OP_EQ, -1); + expect_log_msg("Client asked me to extend " + "to a private IPv6 address '[::1]'.\n"); + mock_clean_saved_logs(); + fake_options->ExtendAllowPrivateAddresses = 0; + tor_addr_port_make_null_ap(&ec->orport_ipv4, AF_INET); + tor_addr_port_make_null_ap(&ec->orport_ipv6, AF_INET6); + + /* Both addresses are internal. * Result depends on ExtendAllowPrivateAddresses. */ tor_addr_parse(&ec->orport_ipv4.addr, INTERNAL_IPV4); ec->orport_ipv4.port = VALID_PORT; + tor_addr_parse(&ec->orport_ipv6.addr, INTERNAL_IPV6); + ec->orport_ipv6.port = VALID_PORT; fake_options->ExtendAllowPrivateAddresses = 0; tt_int_op(circuit_extend_lspec_valid_helper(ec, circ), OP_EQ, -1); expect_log_msg("Client asked me to extend " "to a private IPv4 address '0.0.0.1'.\n"); + expect_log_msg("Client asked me to extend " + "to a private IPv6 address '[::1]'.\n"); mock_clean_saved_logs(); fake_options->ExtendAllowPrivateAddresses = 0; + tor_addr_port_make_null_ap(&ec->orport_ipv4, AF_INET); + tor_addr_port_make_null_ap(&ec->orport_ipv6, AF_INET6); /* If we pass the private address check, but don't have the right * OR circuit magic number, we trigger another bug */ + tor_addr_parse(&ec->orport_ipv4.addr, INTERNAL_IPV4); + ec->orport_ipv4.port = VALID_PORT; + tor_addr_parse(&ec->orport_ipv6.addr, INTERNAL_IPV6); + ec->orport_ipv6.port = VALID_PORT; fake_options->ExtendAllowPrivateAddresses = 1; + tor_capture_bugs_(1); tt_int_op(circuit_extend_lspec_valid_helper(ec, circ), OP_EQ, -1); tt_int_op(smartlist_len(tor_get_captured_bug_log_()), OP_EQ, 1); @@ -564,6 +635,21 @@ test_circuit_extend_lspec_valid(void *arg) tor_end_capture_bugs_(); mock_clean_saved_logs(); fake_options->ExtendAllowPrivateAddresses = 0; + tor_addr_port_make_null_ap(&ec->orport_ipv4, AF_INET); + tor_addr_port_make_null_ap(&ec->orport_ipv6, AF_INET6); + + /* Fail again, but this time only set an IPv4 address. */ + tor_addr_parse(&ec->orport_ipv4.addr, INTERNAL_IPV4); + ec->orport_ipv4.port = VALID_PORT; + fake_options->ExtendAllowPrivateAddresses = 1; + tor_capture_bugs_(1); + tt_int_op(circuit_extend_lspec_valid_helper(ec, circ), OP_EQ, -1); + /* Since we're using IF_BUG_ONCE(), expect 0-1 bug logs */ + tt_int_op(smartlist_len(tor_get_captured_bug_log_()), OP_GE, 0); + tt_int_op(smartlist_len(tor_get_captured_bug_log_()), OP_LE, 1); + tor_end_capture_bugs_(); + mock_clean_saved_logs(); + fake_options->ExtendAllowPrivateAddresses = 0; /* Now set the right magic */ or_circ->base_.magic = OR_CIRCUIT_MAGIC; @@ -625,6 +711,41 @@ test_circuit_extend_lspec_valid(void *arg) tt_int_op(circuit_extend_lspec_valid_helper(ec, circ), OP_EQ, 0); mock_clean_saved_logs(); + /* Now let's check that we warn, but succeed, when only one address is + * private */ + tor_addr_parse(&ec->orport_ipv4.addr, INTERNAL_IPV4); + ec->orport_ipv4.port = VALID_PORT; + tor_addr_parse(&ec->orport_ipv6.addr, PUBLIC_IPV6); + ec->orport_ipv6.port = VALID_PORT; + fake_options->ExtendAllowPrivateAddresses = 0; + + tt_int_op(circuit_extend_lspec_valid_helper(ec, circ), OP_EQ, 0); + expect_log_msg("Client asked me to extend " + "to a private IPv4 address '0.0.0.1'.\n"); + mock_clean_saved_logs(); + tor_addr_port_make_null_ap(&ec->orport_ipv4, AF_INET); + tor_addr_port_make_null_ap(&ec->orport_ipv6, AF_INET6); + + /* Now with private IPv6 */ + tor_addr_parse(&ec->orport_ipv4.addr, PUBLIC_IPV4); + ec->orport_ipv4.port = VALID_PORT; + tor_addr_parse(&ec->orport_ipv6.addr, INTERNAL_IPV6); + ec->orport_ipv6.port = VALID_PORT; + fake_options->ExtendAllowPrivateAddresses = 0; + + tt_int_op(circuit_extend_lspec_valid_helper(ec, circ), OP_EQ, 0); + expect_log_msg("Client asked me to extend " + "to a private IPv6 address '[::1]'.\n"); + mock_clean_saved_logs(); + tor_addr_port_make_null_ap(&ec->orport_ipv4, AF_INET); + tor_addr_port_make_null_ap(&ec->orport_ipv6, AF_INET6); + + /* Now reset to public IPv4 and IPv6 */ + tor_addr_parse(&ec->orport_ipv4.addr, PUBLIC_IPV4); + ec->orport_ipv4.port = VALID_PORT; + tor_addr_parse(&ec->orport_ipv6.addr, PUBLIC_IPV6); + ec->orport_ipv6.port = VALID_PORT; + /* Fail on matching non-zero identities */ memset(&ec->ed_pubkey, 0xEE, sizeof(ec->ed_pubkey)); memset(&p_chan->ed25519_identity, 0xEE, sizeof(p_chan->ed25519_identity));