Bug32588 035 tests

changes/bug32588

@@ -0,0 +1,6 @@
+  o Minor bugfixes (relays, brudges, memory safety):
+    - Stop advertising incorrect IPv6 ORPorts in relay and bridge descriptors,
+      when the IPv6 port was configured as "auto". This may be a low-severity
+      memory safety issue, because the published port number may be derived
+      from uninitialised or out-of-bounds memory reads.
+      Fixes bug 32588; bugfix on 0.2.3.9-alpha

src/app/config/config.c

@@ -809,9 +809,6 @@ static int normalize_nickname_list(config_line_t **normalized_out,
                                    char **msg);
 static char *get_bindaddr_from_transport_listen_line(const char *line,
                                                      const char *transport);
-static int parse_ports(or_options_t *options, int validate_only,
-                              char **msg_out, int *n_ports_out,
-                              int *world_writable_control_socket);
 static int check_server_ports(const smartlist_t *ports,
                               const or_options_t *options,
                               int *num_low_ports_out);
@@ -7370,7 +7367,7 @@ count_real_listeners(const smartlist_t *ports, int listenertype,
  * If <b>validate_only</b> is false, set configured_client_ports to the
  * new list of ports parsed from <b>options</b>.
  **/
-static int
+STATIC int
 parse_ports(or_options_t *options, int validate_only,
             char **msg, int *n_ports_out,
             int *world_writable_control_socket)

src/app/config/config.h

@@ -295,6 +295,10 @@ STATIC int open_and_add_file_log(const log_severity_list_t *severity,
                                  const char *fname,
                                  int truncate_log);
 
+STATIC int parse_ports(or_options_t *options, int validate_only,
+                       char **msg, int *n_ports_out,
+                       int *world_writable_control_socket);
+
 #endif /* defined(CONFIG_PRIVATE) */
 
 #endif /* !defined(TOR_CONFIG_H) */

src/feature/relay/router.c

@@ -1433,6 +1433,49 @@ router_get_advertised_or_port_by_af(const or_options_t *options,
   return port;
 }
 
+/** As router_get_advertised_or_port(), but returns the IPv6 address and
+ *  port in ipv6_ap_out, which must not be NULL. Returns a null address and
+ * zero port, if no ORPort is found. */
+void
+router_get_advertised_ipv6_or_ap(const or_options_t *options,
+                                 tor_addr_port_t *ipv6_ap_out)
+{
+  /* Bug in calling function, we can't return a sensible result, and it
+   * shouldn't use the NULL pointer once we return. */
+  tor_assert(ipv6_ap_out);
+
+  /* If there is no valid IPv6 ORPort, return a null address and port. */
+  tor_addr_make_null(&ipv6_ap_out->addr, AF_INET6);
+  ipv6_ap_out->port = 0;
+
+  const tor_addr_t *addr = get_first_advertised_addr_by_type_af(
+                                                      CONN_TYPE_OR_LISTENER,
+                                                      AF_INET6);
+  const uint16_t port = router_get_advertised_or_port_by_af(
+                                                      options,
+                                                      AF_INET6);
+
+  if (!addr || port == 0) {
+    log_info(LD_CONFIG, "There is no advertised IPv6 ORPort.");
+    return;
+  }
+
+  /* Like IPv4, if the relay is configured using the default
+   * authorities, disallow internal IPs. Otherwise, allow them. */
+  const int default_auth = using_default_dir_authorities(options);
+  if (tor_addr_is_internal(addr, 0) && default_auth) {
+    log_warn(LD_CONFIG,
+             "Unable to use configured IPv6 ORPort \"%s\" in a "
+             "descriptor. Skipping it. "
+             "Try specifying a globally reachable address explicitly.",
+             fmt_addrport(addr, port));
+    return;
+  }
+
+  tor_addr_copy(&ipv6_ap_out->addr, addr);
+  ipv6_ap_out->port = port;
+}
+
 /** Return the port that we should advertise as our DirPort;
  * this is one of three possibilities:
  * The one that is passed as <b>dirport</b> if the DirPort option is 0, or
@@ -1848,34 +1891,11 @@ router_build_fresh_descriptor(routerinfo_t **r, extrainfo_t **e)
                sizeof(curve25519_public_key_t));
 
   /* For now, at most one IPv6 or-address is being advertised. */
-  {
-    const port_cfg_t *ipv6_orport = NULL;
-    SMARTLIST_FOREACH_BEGIN(get_configured_ports(), const port_cfg_t *, p) {
-      if (p->type == CONN_TYPE_OR_LISTENER &&
-          ! p->server_cfg.no_advertise &&
-          ! p->server_cfg.bind_ipv4_only &&
-          tor_addr_family(&p->addr) == AF_INET6) {
-        /* Like IPv4, if the relay is configured using the default
-         * authorities, disallow internal IPs. Otherwise, allow them. */
-        const int default_auth = using_default_dir_authorities(options);
-        if (! tor_addr_is_internal(&p->addr, 0) || ! default_auth) {
-          ipv6_orport = p;
-          break;
-        } else {
-          char addrbuf[TOR_ADDR_BUF_LEN];
-          log_warn(LD_CONFIG,
-                   "Unable to use configured IPv6 address \"%s\" in a "
-                   "descriptor. Skipping it. "
-                   "Try specifying a globally reachable address explicitly.",
-                   tor_addr_to_str(addrbuf, &p->addr, sizeof(addrbuf), 1));
-        }
-      }
-    } SMARTLIST_FOREACH_END(p);
-    if (ipv6_orport) {
-      tor_addr_copy(&ri->ipv6_addr, &ipv6_orport->addr);
-      ri->ipv6_orport = ipv6_orport->port;
-    }
-  }
+  tor_addr_port_t ipv6_orport;
+  router_get_advertised_ipv6_or_ap(options, &ipv6_orport);
+  /* If there is no valud IPv6 ORPort, the address and port are null. */
+  tor_addr_copy(&ri->ipv6_addr, &ipv6_orport.addr);
+  ri->ipv6_orport = ipv6_orport.port;
 
   ri->identity_pkey = crypto_pk_dup_key(get_server_identity_key());
   if (BUG(crypto_pk_get_digest(ri->identity_pkey,

src/feature/relay/router.h

@@ -59,6 +59,8 @@ int init_keys_client(void);
 uint16_t router_get_active_listener_port_by_type_af(int listener_type,
                                                     sa_family_t family);
 uint16_t router_get_advertised_or_port(const or_options_t *options);
+void router_get_advertised_ipv6_or_ap(const or_options_t *options,
+                                      tor_addr_port_t *ipv6_ap_out);
 uint16_t router_get_advertised_or_port_by_af(const or_options_t *options,
                                              sa_family_t family);
 uint16_t router_get_advertised_dir_port(const or_options_t *options,

src/test/test_config.c

@@ -4860,6 +4860,64 @@ test_config_parse_port_config__ports__server_options(void *data)
   config_free_lines(config_port_valid); config_port_valid = NULL;
 }
 
+static void
+test_config_get_first_advertised(void *data)
+{
+  (void)data;
+  int r, w=0, n=0;
+  char *msg=NULL;
+  or_options_t *opts = options_new();
+  int port;
+  const tor_addr_t *addr;
+
+  // no ports are configured? We get NULL.
+  port = get_first_advertised_port_by_type_af(CONN_TYPE_OR_LISTENER,
+                                              AF_INET);
+  tt_int_op(port, OP_EQ, 0);
+  addr = get_first_advertised_addr_by_type_af(CONN_TYPE_OR_LISTENER,
+                                              AF_INET);
+  tt_ptr_op(addr, OP_EQ, NULL);
+
+  config_line_append(&opts->ORPort_lines, "ORPort", "[1234::5678]:8080");
+  config_line_append(&opts->ORPort_lines, "ORPort",
+                     "1.2.3.4:9999 noadvertise");
+  config_line_append(&opts->ORPort_lines, "ORPort",
+                     "5.6.7.8:9911 nolisten");
+
+  r = parse_ports(opts, 0, &msg, &n, &w);
+  tt_assert(r == 0);
+
+  // UNSPEC gets us nothing.
+  port = get_first_advertised_port_by_type_af(CONN_TYPE_OR_LISTENER,
+                                              AF_UNSPEC);
+  tt_int_op(port, OP_EQ, 0);
+  addr = get_first_advertised_addr_by_type_af(CONN_TYPE_OR_LISTENER,
+                                              AF_UNSPEC);
+  tt_ptr_op(addr, OP_EQ, NULL);
+
+  // Try AF_INET.
+  port = get_first_advertised_port_by_type_af(CONN_TYPE_OR_LISTENER,
+                                              AF_INET);
+  tt_int_op(port, OP_EQ, 9911);
+  addr = get_first_advertised_addr_by_type_af(CONN_TYPE_OR_LISTENER,
+                                              AF_INET);
+  tt_ptr_op(addr, OP_NE, NULL);
+  tt_str_op(fmt_addrport(addr,port), OP_EQ, "5.6.7.8:9911");
+
+  // Try AF_INET6
+  port = get_first_advertised_port_by_type_af(CONN_TYPE_OR_LISTENER,
+                                              AF_INET6);
+  tt_int_op(port, OP_EQ, 8080);
+  addr = get_first_advertised_addr_by_type_af(CONN_TYPE_OR_LISTENER,
+                                              AF_INET6);
+  tt_ptr_op(addr, OP_NE, NULL);
+  tt_str_op(fmt_addrport(addr,port), OP_EQ, "[1234::5678]:8080");
+
+ done:
+  or_options_free(opts);
+  config_free_all();
+}
+
 static void
 test_config_parse_log_severity(void *data)
 {
@@ -5904,6 +5962,7 @@ struct testcase_t config_tests[] = {
   CONFIG_TEST(parse_port_config__ports__no_ports_given, 0),
   CONFIG_TEST(parse_port_config__ports__server_options, 0),
   CONFIG_TEST(parse_port_config__ports__ports_given, 0),
+  CONFIG_TEST(get_first_advertised, TT_FORK),
   CONFIG_TEST(parse_log_severity, 0),
   CONFIG_TEST(include_limit, 0),
   CONFIG_TEST(include_does_not_exist, 0),

src/test/test_router.c

@@ -7,16 +7,22 @@
  * \brief Unittests for code in router.c
  **/
 
+#define CONFIG_PRIVATE
+#define CONNECTION_PRIVATE
 #include "core/or/or.h"
 #include "app/config/config.h"
 #include "core/mainloop/mainloop.h"
+#include "core/mainloop/connection.h"
 #include "feature/hibernate/hibernate.h"
 #include "feature/nodelist/routerinfo_st.h"
 #include "feature/nodelist/routerlist.h"
 #include "feature/relay/router.h"
 #include "feature/stats/rephist.h"
 #include "lib/crypt_ops/crypto_curve25519.h"
 #include "lib/crypt_ops/crypto_ed25519.h"
+#include "lib/encoding/confline.h"
+
+#include "core/or/listener_connection_st.h"
 
 /* Test suite stuff */
 #include "test/test.h"
@@ -231,11 +237,101 @@ test_router_check_descriptor_bandwidth_changed(void *arg)
   UNMOCK(we_are_hibernating);
 }
 
+static smartlist_t *fake_connection_array = NULL;
+static smartlist_t *
+mock_get_connection_array(void)
+{
+  return fake_connection_array;
+}
+
+static void
+test_router_get_advertised_or_port(void *arg)
+{
+  (void)arg;
+  int r, w=0, n=0;
+  char *msg=NULL;
+  or_options_t *opts = options_new();
+  listener_connection_t *listener = NULL;
+  tor_addr_port_t ipv6;
+
+  // Test one failing case of router_get_advertised_ipv6_or_ap().
+  router_get_advertised_ipv6_or_ap(opts, &ipv6);
+  tt_str_op(fmt_addrport(&ipv6.addr, ipv6.port), OP_EQ, "[::]:0");
+
+  // Set up a couple of configured ports.
+  config_line_append(&opts->ORPort_lines, "ORPort", "[1234::5678]:9999");
+  config_line_append(&opts->ORPort_lines, "ORPort", "5.6.7.8:auto");
+  r = parse_ports(opts, 0, &msg, &n, &w);
+  tt_assert(r == 0);
+
+  // There are no listeners, so the "auto" case will turn up no results.
+  tt_int_op(0, OP_EQ, router_get_advertised_or_port_by_af(opts, AF_INET));
+
+  // This will return the matching value from the configured port.
+  tt_int_op(9999, OP_EQ, router_get_advertised_or_port_by_af(opts, AF_INET6));
+
+  // Now set up a dummy listener.
+  MOCK(get_connection_array, mock_get_connection_array);
+  fake_connection_array = smartlist_new();
+  listener = listener_connection_new(CONN_TYPE_OR_LISTENER, AF_INET);
+  TO_CONN(listener)->port = 54321;
+  smartlist_add(fake_connection_array, TO_CONN(listener));
+
+  // We should get a port this time.
+  tt_int_op(54321, OP_EQ, router_get_advertised_or_port_by_af(opts, AF_INET));
+
+  // Test one succeeding case of router_get_advertised_ipv6_or_ap().
+  router_get_advertised_ipv6_or_ap(opts, &ipv6);
+  tt_str_op(fmt_addrport(&ipv6.addr, ipv6.port), OP_EQ,
+            "[1234::5678]:9999");
+
+ done:
+  or_options_free(opts);
+  config_free_all();
+  smartlist_free(fake_connection_array);
+  connection_free_minimal(TO_CONN(listener));
+  UNMOCK(get_connection_array);
+}
+
+static void
+test_router_get_advertised_or_port_localhost(void *arg)
+{
+  (void)arg;
+  int r, w=0, n=0;
+  char *msg=NULL;
+  or_options_t *opts = options_new();
+  tor_addr_port_t ipv6;
+
+  // Set up a couple of configured ports on localhost.
+  config_line_append(&opts->ORPort_lines, "ORPort", "[::1]:9999");
+  config_line_append(&opts->ORPort_lines, "ORPort", "127.0.0.1:8888");
+  r = parse_ports(opts, 0, &msg, &n, &w);
+  tt_assert(r == 0);
+
+  // We should refuse to advertise them, since we have default dirauths.
+  router_get_advertised_ipv6_or_ap(opts, &ipv6);
+  tt_str_op(fmt_addrport(&ipv6.addr, ipv6.port), OP_EQ, "[::]:0");
+
+  // Now try with a fake authority set up.
+  config_line_append(&opts->DirAuthorities, "DirAuthority",
+                     "127.0.0.1:1066 "
+                     "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA");
+
+  router_get_advertised_ipv6_or_ap(opts, &ipv6);
+  tt_str_op(fmt_addrport(&ipv6.addr, ipv6.port), OP_EQ, "[::1]:9999");
+
+ done:
+  or_options_free(opts);
+  config_free_all();
+}
+
 #define ROUTER_TEST(name, flags)                          \
   { #name, test_router_ ## name, flags, NULL, NULL }
 
 struct testcase_t router_tests[] = {
   ROUTER_TEST(check_descriptor_bandwidth_changed, TT_FORK),
   ROUTER_TEST(dump_router_to_string_no_bridge_distribution_method, TT_FORK),
+  ROUTER_TEST(get_advertised_or_port, TT_FORK),
+  ROUTER_TEST(get_advertised_or_port_localhost, TT_FORK),
   END_OF_TESTCASES
 };