Merge remote-tracking branch 'tor-github/pr/1902/head'

15 files changed, 171 insertions, 87 deletions

diff --git a/changes/ticket33788 b/changes/ticket33788
new file mode 100644
index 0000000000..236c056623
--- /dev/null
+++ b/changes/ticket33788
@@ -0,0 +1,4 @@
+  o Minor features (code safety):
+    - Check for failures of tor_inet_ntop() and tor_inet_ntoa() functions in
+      DNS and IP address processing code and adjust codepaths to make them
+      less likely to crash entire Tor instance. Resolves issue 33788.
diff --git a/src/app/config/resolve_addr.c b/src/app/config/resolve_addr.c
index 52f4efc992..9d1a8e0260 100644
--- a/src/app/config/resolve_addr.c
+++ b/src/app/config/resolve_addr.c
@@ -198,7 +198,7 @@ resolve_my_address(int warn_severity, const or_options_t *options,
   tor_addr_from_ipv4h(&myaddr,addr);
 
   addr_string = tor_dup_ip(addr);
-  if (tor_addr_is_internal(&myaddr, 0)) {
+  if (addr_string && tor_addr_is_internal(&myaddr, 0)) {
     /* make sure we're ok with publishing an internal IP */
     if (using_default_dir_authorities(options)) {
       /* if they are using the default authorities, disallow internal IPs
diff --git a/src/core/or/connection_edge.c b/src/core/or/connection_edge.c
index 5c9bf64e8e..fc77db8334 100644
--- a/src/core/or/connection_edge.c
+++ b/src/core/or/connection_edge.c
@@ -3459,8 +3459,9 @@ tell_controller_about_resolved_result(entry_connection_t *conn,
   expires = time(NULL) + ttl;
   if (answer_type == RESOLVED_TYPE_IPV4 && answer_len >= 4) {
     char *cp = tor_dup_ip(ntohl(get_uint32(answer)));
-    control_event_address_mapped(conn->socks_request->address,
-                                 cp, expires, NULL, 0);
+    if (cp)
+      control_event_address_mapped(conn->socks_request->address,
+                                   cp, expires, NULL, 0);
     tor_free(cp);
   } else if (answer_type == RESOLVED_TYPE_HOSTNAME && answer_len < 256) {
     char *cp = tor_strndup(answer, answer_len);
diff --git a/src/feature/client/circpathbias.c b/src/feature/client/circpathbias.c
index 4ac5cb8fc9..74260171fe 100644
--- a/src/feature/client/circpathbias.c
+++ b/src/feature/client/circpathbias.c
@@ -826,6 +826,11 @@ pathbias_send_usable_probe(circuit_t *circ)
   ocirc->pathbias_probe_nonce &= 0x00ffffff;
   probe_nonce = tor_dup_ip(ocirc->pathbias_probe_nonce);
 
+  if (!probe_nonce) {
+    log_err(LD_BUG, "Failed to generate nonce");
+    return -1;
+  }
+
   tor_snprintf(payload,RELAY_PAYLOAD_SIZE, "%s:25", probe_nonce);
   payload_len = (int)strlen(payload)+1;
 
diff --git a/src/feature/control/control_getinfo.c b/src/feature/control/control_getinfo.c
index b2d0c9680d..0823acbe07 100644
--- a/src/feature/control/control_getinfo.c
+++ b/src/feature/control/control_getinfo.c
@@ -137,6 +137,7 @@ getinfo_helper_misc(control_connection_t *conn, const char *question,
       return -1;
     }
     *answer = tor_dup_ip(addr);
+    tor_assert_nonfatal(*answer);
   } else if (!strcmp(question, "traffic/read")) {
     tor_asprintf(answer, "%"PRIu64, (get_bytes_read()));
   } else if (!strcmp(question, "traffic/written")) {
diff --git a/src/feature/dirauth/dirvote.c b/src/feature/dirauth/dirvote.c
index 6bf30d083b..85a23a12f6 100644
--- a/src/feature/dirauth/dirvote.c
+++ b/src/feature/dirauth/dirvote.c
@@ -322,43 +322,47 @@ format_networkstatus_vote(crypto_pk_t *private_signing_key,
       tor_free(digest_algo_b64_digest_bw_file);
     }
 
-    smartlist_add_asprintf(chunks,
-                 "network-status-version 3\n"
-                 "vote-status %s\n"
-                 "consensus-methods %s\n"
-                 "published %s\n"
-                 "valid-after %s\n"
-                 "fresh-until %s\n"
-                 "valid-until %s\n"
-                 "voting-delay %d %d\n"
-                 "%s%s" /* versions */
-                 "%s" /* protocols */
-                 "known-flags %s\n"
-                 "flag-thresholds %s\n"
-                 "params %s\n"
-                 "%s" /* bandwidth file headers */
-                 "%s" /* bandwidth file digest */
-                 "dir-source %s %s %s %s %d %d\n"
-                 "contact %s\n"
-                 "%s" /* shared randomness information */
-                 ,
-                 v3_ns->type == NS_TYPE_VOTE ? "vote" : "opinion",
-                 methods,
-                 published, va, fu, vu,
-                 v3_ns->vote_seconds, v3_ns->dist_seconds,
-                 client_versions_line,
-                 server_versions_line,
-                 protocols_lines,
-                 flags,
-                 flag_thresholds,
-                 params,
-                 bw_headers_line ? bw_headers_line : "",
-                 bw_file_digest ? bw_file_digest: "",
-                 voter->nickname, fingerprint, voter->address,
-                 fmt_addr32(addr), voter->dir_port, voter->or_port,
-                 voter->contact,
-                 shared_random_vote_str ?
-                           shared_random_vote_str : "");
+    const char *ip_str = fmt_addr32(addr);
+
+    if (ip_str[0]) {
+      smartlist_add_asprintf(chunks,
+                   "network-status-version 3\n"
+                   "vote-status %s\n"
+                   "consensus-methods %s\n"
+                   "published %s\n"
+                   "valid-after %s\n"
+                   "fresh-until %s\n"
+                   "valid-until %s\n"
+                   "voting-delay %d %d\n"
+                   "%s%s" /* versions */
+                   "%s" /* protocols */
+                   "known-flags %s\n"
+                   "flag-thresholds %s\n"
+                   "params %s\n"
+                   "%s" /* bandwidth file headers */
+                   "%s" /* bandwidth file digest */
+                   "dir-source %s %s %s %s %d %d\n"
+                   "contact %s\n"
+                   "%s" /* shared randomness information */
+                   ,
+                   v3_ns->type == NS_TYPE_VOTE ? "vote" : "opinion",
+                   methods,
+                   published, va, fu, vu,
+                   v3_ns->vote_seconds, v3_ns->dist_seconds,
+                   client_versions_line,
+                   server_versions_line,
+                   protocols_lines,
+                   flags,
+                   flag_thresholds,
+                   params,
+                   bw_headers_line ? bw_headers_line : "",
+                   bw_file_digest ? bw_file_digest: "",
+                   voter->nickname, fingerprint, voter->address,
+                   ip_str, voter->dir_port, voter->or_port,
+                   voter->contact,
+                   shared_random_vote_str ?
+                             shared_random_vote_str : "");
+    }
 
     tor_free(params);
     tor_free(flags);
@@ -368,6 +372,9 @@ format_networkstatus_vote(crypto_pk_t *private_signing_key,
     tor_free(bw_headers_line);
     tor_free(bw_file_digest);
 
+    if (ip_str[0] == '\0')
+      goto err;
+
     if (!tor_digest_is_zero(voter->legacy_id_digest)) {
       char fpbuf[HEX_DIGEST_LEN+1];
       base16_encode(fpbuf, sizeof(fpbuf), voter->legacy_id_digest, DIGEST_LEN);
@@ -4494,6 +4501,11 @@ dirserv_generate_networkstatus_vote_obj(crypto_pk_t *private_key,
     hostname = tor_dup_ip(addr);
   }
 
+  if (!hostname) {
+    log_err(LD_BUG, "Failed to determine hostname AND duplicate address");
+    return NULL;
+  }
+
   if (d_options->VersioningAuthoritativeDirectory) {
     client_versions =
       format_recommended_version_list(d_options->RecommendedClientVersions, 0);
diff --git a/src/feature/nodelist/authcert.c b/src/feature/nodelist/authcert.c
index 9c7525b6e2..97e44d53e3 100644
--- a/src/feature/nodelist/authcert.c
+++ b/src/feature/nodelist/authcert.c
@@ -464,11 +464,13 @@ trusted_dirs_load_certs_from_string(const char *contents, int source,
           (ds->addr != cert->addr ||
            ds->dir_port != cert->dir_port)) {
         char *a = tor_dup_ip(cert->addr);
-        log_notice(LD_DIR, "Updating address for directory authority %s "
-                   "from %s:%d to %s:%d based on certificate.",
-                   ds->nickname, ds->address, (int)ds->dir_port,
-                   a, cert->dir_port);
-        tor_free(a);
+        if (a) {
+          log_notice(LD_DIR, "Updating address for directory authority %s "
+                     "from %s:%d to %s:%d based on certificate.",
+                     ds->nickname, ds->address, (int)ds->dir_port,
+                     a, cert->dir_port);
+          tor_free(a);
+        }
         ds->addr = cert->addr;
         ds->dir_port = cert->dir_port;
       }
diff --git a/src/feature/nodelist/dirlist.c b/src/feature/nodelist/dirlist.c
index d7069bb0ec..33d1bfc4d0 100644
--- a/src/feature/nodelist/dirlist.c
+++ b/src/feature/nodelist/dirlist.c
@@ -358,6 +358,9 @@ trusted_dir_server_new(const char *nickname, const char *address,
     }
     if (!hostname)
       hostname = tor_dup_ip(a);
+
+    if (!hostname)
+      return NULL;
   } else {
     if (tor_lookup_hostname(address, &a)) {
       log_warn(LD_CONFIG,
diff --git a/src/feature/nodelist/fmt_routerstatus.c b/src/feature/nodelist/fmt_routerstatus.c
index 0cf4a6eeab..ca4a312639 100644
--- a/src/feature/nodelist/fmt_routerstatus.c
+++ b/src/feature/nodelist/fmt_routerstatus.c
@@ -53,6 +53,10 @@ routerstatus_format_entry(const routerstatus_t *rs, const char *version,
   char digest64[BASE64_DIGEST_LEN+1];
   smartlist_t *chunks = smartlist_new();
 
+  const char *ip_str = fmt_addr32(rs->addr);
+  if (ip_str[0] == '\0')
+    goto err;
+
   format_iso_time(published, rs->published_on);
   digest_to_base64(identity64, rs->identity_digest);
   digest_to_base64(digest64, rs->descriptor_digest);
@@ -64,7 +68,7 @@ routerstatus_format_entry(const routerstatus_t *rs, const char *version,
                    (format==NS_V3_CONSENSUS_MICRODESC)?"":digest64,
                    (format==NS_V3_CONSENSUS_MICRODESC)?"":" ",
                    published,
-                   fmt_addr32(rs->addr),
+                   ip_str,
                    (int)rs->or_port,
                    (int)rs->dir_port);
 
diff --git a/src/feature/relay/dns.c b/src/feature/relay/dns.c
index 4dcf5b7031..b83bd9b758 100644
--- a/src/feature/relay/dns.c
+++ b/src/feature/relay/dns.c
@@ -1591,12 +1591,17 @@ evdns_callback(int result, char type, int count, int ttl, void *addresses,
     } else if (type == DNS_IPv6_AAAA && count) {
       char answer_buf[TOR_ADDR_BUF_LEN];
       char *escaped_address;
+      const char *ip_str;
       struct in6_addr *addrs = addresses;
       tor_addr_from_in6(&addr, &addrs[0]);
-      tor_inet_ntop(AF_INET6, &addrs[0], answer_buf, sizeof(answer_buf));
+      ip_str = tor_inet_ntop(AF_INET6, &addrs[0], answer_buf,
+                             sizeof(answer_buf));
       escaped_address = esc_for_log(string_address);
 
-      if (answer_is_wildcarded(answer_buf)) {
+      if (BUG(ip_str == NULL)) {
+        log_warn(LD_EXIT, "tor_inet_ntop() failed!");
+        result = DNS_ERR_NOTEXIST;
+      } else if (answer_is_wildcarded(answer_buf)) {
         log_debug(LD_EXIT, "eventdns said that %s resolves to ISP-hijacked "
                   "address %s; treating as a failure.",
                   safe_str(escaped_address),
@@ -1863,6 +1868,7 @@ evdns_wildcard_check_callback(int result, char type, int count, int ttl,
                               void *addresses, void *arg)
 {
   (void)ttl;
+  const char *ip_str;
   ++n_wildcard_requests;
   if (result == DNS_ERR_NONE && count) {
     char *string_address = arg;
@@ -1872,16 +1878,22 @@ evdns_wildcard_check_callback(int result, char type, int count, int ttl,
       for (i = 0; i < count; ++i) {
         char answer_buf[INET_NTOA_BUF_LEN+1];
         struct in_addr in;
+        int ntoa_res;
         in.s_addr = addrs[i];
-        tor_inet_ntoa(&in, answer_buf, sizeof(answer_buf));
-        wildcard_increment_answer(answer_buf);
+        ntoa_res = tor_inet_ntoa(&in, answer_buf, sizeof(answer_buf));
+        tor_assert_nonfatal(ntoa_res >= 0);
+        if (ntoa_res > 0)
+          wildcard_increment_answer(answer_buf);
       }
     } else if (type == DNS_IPv6_AAAA) {
       const struct in6_addr *addrs = addresses;
       for (i = 0; i < count; ++i) {
         char answer_buf[TOR_ADDR_BUF_LEN+1];
-        tor_inet_ntop(AF_INET6, &addrs[i], answer_buf, sizeof(answer_buf));
-        wildcard_increment_answer(answer_buf);
+        ip_str = tor_inet_ntop(AF_INET6, &addrs[i], answer_buf,
+                               sizeof(answer_buf));
+        tor_assert_nonfatal(ip_str);
+        if (ip_str)
+          wildcard_increment_answer(answer_buf);
       }
     }
 
diff --git a/src/feature/relay/relay_periodic.c b/src/feature/relay/relay_periodic.c
index b751323e0d..08ad110cf6 100644
--- a/src/feature/relay/relay_periodic.c
+++ b/src/feature/relay/relay_periodic.c
@@ -203,29 +203,34 @@ reachability_warnings_callback(time_t now, const or_options_t *options)
     const routerinfo_t *me = router_get_my_routerinfo();
     if (me && !check_whether_orport_reachable(options)) {
       char *address = tor_dup_ip(me->addr);
-      log_warn(LD_CONFIG,"Your server (%s:%d) has not managed to confirm that "
-               "its ORPort is reachable. Relays do not publish descriptors "
-               "until their ORPort and DirPort are reachable. Please check "
-               "your firewalls, ports, address, /etc/hosts file, etc.",
-               address, me->or_port);
-      control_event_server_status(LOG_WARN,
-                                  "REACHABILITY_FAILED ORADDRESS=%s:%d",
-                                  address, me->or_port);
-      tor_free(address);
+      if (address) {
+        log_warn(LD_CONFIG,
+                 "Your server (%s:%d) has not managed to confirm that "
+                 "its ORPort is reachable. Relays do not publish descriptors "
+                 "until their ORPort and DirPort are reachable. Please check "
+                 "your firewalls, ports, address, /etc/hosts file, etc.",
+                 address, me->or_port);
+        control_event_server_status(LOG_WARN,
+                                    "REACHABILITY_FAILED ORADDRESS=%s:%d",
+                                    address, me->or_port);
+        tor_free(address);
+      }
     }
 
     if (me && !check_whether_dirport_reachable(options)) {
       char *address = tor_dup_ip(me->addr);
-      log_warn(LD_CONFIG,
-               "Your server (%s:%d) has not managed to confirm that its "
-               "DirPort is reachable. Relays do not publish descriptors "
-               "until their ORPort and DirPort are reachable. Please check "
-               "your firewalls, ports, address, /etc/hosts file, etc.",
-               address, me->dir_port);
-      control_event_server_status(LOG_WARN,
-                                  "REACHABILITY_FAILED DIRADDRESS=%s:%d",
-                                  address, me->dir_port);
-      tor_free(address);
+      if (address) {
+        log_warn(LD_CONFIG,
+                 "Your server (%s:%d) has not managed to confirm that its "
+                 "DirPort is reachable. Relays do not publish descriptors "
+                 "until their ORPort and DirPort are reachable. Please check "
+                 "your firewalls, ports, address, /etc/hosts file, etc.",
+                 address, me->dir_port);
+        control_event_server_status(LOG_WARN,
+                                    "REACHABILITY_FAILED DIRADDRESS=%s:%d",
+                                    address, me->dir_port);
+        tor_free(address);
+      }
     }
   }
 
diff --git a/src/feature/relay/router.c b/src/feature/relay/router.c
index 267dee8483..34d8163c36 100644
--- a/src/feature/relay/router.c
+++ b/src/feature/relay/router.c
@@ -2784,6 +2784,9 @@ router_dump_router_to_string(routerinfo_t *router,
   }
 
   address = tor_dup_ip(router->addr);
+  if (!address)
+    goto err;
+
   chunks = smartlist_new();
 
   /* Generate the easy portion of the router descriptor. */
diff --git a/src/feature/relay/selftest.c b/src/feature/relay/selftest.c
index 402ce0e3d3..18fe25b989 100644
--- a/src/feature/relay/selftest.c
+++ b/src/feature/relay/selftest.c
@@ -224,7 +224,11 @@ inform_testing_reachability(void)
   const routerinfo_t *me = router_get_my_routerinfo();
   if (!me)
     return 0;
+
   address = tor_dup_ip(me->addr);
+  if (!address)
+    return 0;
+
   control_event_server_status(LOG_NOTICE,
                               "CHECKING_REACHABILITY ORADDRESS=%s:%d",
                               address, me->or_port);
@@ -255,6 +259,10 @@ router_orport_found_reachable(void)
   const or_options_t *options = get_options();
   if (!can_reach_or_port && me) {
     char *address = tor_dup_ip(me->addr);
+
+    if (!address)
+      return;
+
     log_notice(LD_OR,"Self-testing indicates your ORPort is reachable from "
                "the outside. Excellent.%s",
                options->PublishServerDescriptor_ != NO_DIRINFO
@@ -282,6 +290,10 @@ router_dirport_found_reachable(void)
   const or_options_t *options = get_options();
   if (!can_reach_dir_port && me) {
     char *address = tor_dup_ip(me->addr);
+
+    if (!address)
+      return;
+
     log_notice(LD_DIRSERV,"Self-testing indicates your DirPort is reachable "
                "from the outside. Excellent.%s",
                options->PublishServerDescriptor_ != NO_DIRINFO
diff --git a/src/feature/rend/rendservice.c b/src/feature/rend/rendservice.c
index 9d7ff2d17f..a88c2080fd 100644
--- a/src/feature/rend/rendservice.c
+++ b/src/feature/rend/rendservice.c
@@ -3715,20 +3715,23 @@ directory_post_to_hs_dir(rend_service_descriptor_t *renddesc,
       base32_encode(desc_id_base32, sizeof(desc_id_base32),
                     desc->desc_id, DIGEST_LEN);
       hs_dir_ip = tor_dup_ip(hs_dir->addr);
-      log_info(LD_REND, "Launching upload for v2 descriptor for "
-                        "service '%s' with descriptor ID '%s' with validity "
-                        "of %d seconds to hidden service directory '%s' on "
-                        "%s:%d.",
-               safe_str_client(service_id),
-               safe_str_client(desc_id_base32),
-               seconds_valid,
-               hs_dir->nickname,
-               hs_dir_ip,
-               hs_dir->or_port);
+      if (hs_dir_ip) {
+        log_info(LD_REND, "Launching upload for v2 descriptor for "
+                          "service '%s' with descriptor ID '%s' with validity "
+                          "of %d seconds to hidden service directory '%s' on "
+                          "%s:%d.",
+                 safe_str_client(service_id),
+                 safe_str_client(desc_id_base32),
+                 seconds_valid,
+                 hs_dir->nickname,
+                 hs_dir_ip,
+                 hs_dir->or_port);
+        tor_free(hs_dir_ip);
+      }
+
       control_event_hs_descriptor_upload(service_id,
                                          hs_dir->identity_digest,
                                          desc_id_base32, NULL);
-      tor_free(hs_dir_ip);
       /* Remember successful upload to this router for next time. */
       if (!smartlist_contains_digest(successful_uploads,
                                      hs_dir->identity_digest))
diff --git a/src/lib/net/address.c b/src/lib/net/address.c
index b09c9115c4..6d46f9b955 100644
--- a/src/lib/net/address.c
+++ b/src/lib/net/address.c
@@ -1196,14 +1196,24 @@ fmt_addrport(const tor_addr_t *addr, uint16_t port)
 
 /** Like fmt_addr(), but takes <b>addr</b> as a host-order IPv4
  * addresses. Also not thread-safe, also clobbers its return buffer on
- * repeated calls. */
+ * repeated calls. Clean internal buffer and return empty string on failure. */
 const char *
 fmt_addr32(uint32_t addr)
 {
   static char buf[INET_NTOA_BUF_LEN];
   struct in_addr in;
+  int success;
+
   in.s_addr = htonl(addr);
-  tor_inet_ntoa(&in, buf, sizeof(buf));
+
+  success = tor_inet_ntoa(&in, buf, sizeof(buf));
+  tor_assertf_nonfatal(success >= 0,
+      "Failed to convert IP 0x%08X (HBO) to string", addr);
+
+  IF_BUG_ONCE(success < 0) {
+    memset(buf, 0, INET_NTOA_BUF_LEN);
+  }
+
   return buf;
 }
 
@@ -1999,17 +2009,24 @@ parse_port_range(const char *port, uint16_t *port_min_out,
 }
 
 /** Given a host-order <b>addr</b>, call tor_inet_ntop() on it
- *  and return a strdup of the resulting address.
+ *  and return a strdup of the resulting address. Return NULL if
+ *  tor_inet_ntop() fails.
  */
 char *
 tor_dup_ip(uint32_t addr)
 {
+  const char *ip_str;
   char buf[TOR_ADDR_BUF_LEN];
   struct in_addr in;
 
   in.s_addr = htonl(addr);
-  tor_inet_ntop(AF_INET, &in, buf, sizeof(buf));
-  return tor_strdup(buf);
+  ip_str = tor_inet_ntop(AF_INET, &in, buf, sizeof(buf));
+
+  tor_assertf_nonfatal(ip_str, "Failed to duplicate IP %08X", addr);
+  if (ip_str)
+    return tor_strdup(buf);
+
+  return NULL;
 }
 
 /**