diff options
| -rw-r--r-- | src/feature/relay/circuitbuild_relay.c | 50 | ||||
| -rw-r--r-- | 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 05146f1b67..080781f719 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 9322480844..a8d6323e40 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)); |