An attempt at bug3940 and making AllowDotExit 0 work with MapAddress

This time, I follow grarpamp's suggestion and move the check for
.exit+AllowDotExit 0 to the top of connection_ap_rewrite_and_attach,
before any rewriting occurs.  This way, .exit addresses are
forbidden as they arrive from a socks connection or a DNSPort
request, and not otherwise.

It _is_ a little more complicated than that, though.  We need to
treat any .exit addresses whose source is TrackHostExits as meaning
that we can retry without that exit.  We also need to treat any
.exit address that comes from an AutomapHostsOnResolve operation as
user-provided (and thus forbidden if AllowDotExits==0), so that
transitioning from AllowDotExits==1 to AllowDotExits==0 will
actually turn off automapped .exit addresses.

6 files changed, 98 insertions, 65 deletions

diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c
index 5ef56a63b0..b15e135d11 100644
--- a/src/or/connection_edge.c
+++ b/src/or/connection_edge.c
@@ -1095,13 +1095,17 @@ addressmap_match_superdomains(char *address)
  * address changed; false otherwise.  Set *<b>expires_out</b> to the
  * expiry time of the result, or to <b>time_max</b> if the result does
  * not expire.
+ *
+ * DOCDOC exit_source_out;
  */
 int
-addressmap_rewrite(char *address, size_t maxlen, time_t *expires_out)
+addressmap_rewrite(char *address, size_t maxlen, time_t *expires_out,
+                   addressmap_entry_source_t *exit_source_out)
 {
   addressmap_entry_t *ent;
   int rewrites;
   time_t expires = TIME_MAX;
+  addressmap_entry_source_t exit_source = ADDRMAPSRC_NONE;
 
   for (rewrites = 0; rewrites < 16; rewrites++) {
     int exact_match = 0;
@@ -1117,9 +1121,7 @@ addressmap_rewrite(char *address, size_t maxlen, time_t *expires_out)
         /* This is a rule like *.example.com example.com, and we just got
          * "example.com" */
         tor_free(addr_orig);
-        if (expires_out)
-          *expires_out = expires;
-        return rewrites > 0;
+        goto done;
       }
 
       exact_match = 1;
@@ -1127,9 +1129,7 @@ addressmap_rewrite(char *address, size_t maxlen, time_t *expires_out)
 
     if (!ent || !ent->new_address) {
       tor_free(addr_orig);
-      if (expires_out)
-        *expires_out = expires;
-      return (rewrites > 0); /* done, no rewrite needed */
+      goto done;
     }
 
     if (ent->dst_wildcard && !exact_match) {
@@ -1139,6 +1139,12 @@ addressmap_rewrite(char *address, size_t maxlen, time_t *expires_out)
       strlcpy(address, ent->new_address, maxlen);
     }
 
+    if (!strcmpend(address, ".exit") &&
+        strcmpend(addr_orig, ".exit") &&
+        exit_source == ADDRMAPSRC_NONE) {
+      exit_source = ent->source;
+    }
+
     log_info(LD_APP, "Addressmap: rewriting %s to %s",
              addr_orig, escaped_safe_str_client(address));
     if (ent->expires > 1 && ent->expires < expires)
@@ -1149,9 +1155,13 @@ addressmap_rewrite(char *address, size_t maxlen, time_t *expires_out)
            "Loop detected: we've rewritten %s 16 times! Using it as-is.",
            escaped_safe_str_client(address));
   /* it's fine to rewrite a rewrite, but don't loop forever */
+
+ done:
+  if (exit_source_out)
+    *exit_source_out = exit_source;
   if (expires_out)
     *expires_out = TIME_MAX;
-  return 1;
+  return (rewrites > 0);
 }
 
 /** If we have a cached reverse DNS entry for the address stored in the
@@ -1782,11 +1792,9 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
   int automap = 0;
   char orig_address[MAX_SOCKS_ADDR_LEN];
   time_t map_expires = TIME_MAX;
-  /* This will be set to true iff the address starts out as a non-.exit
-     address, and we remap it to one because of an entry in the addressmap. */
-  int remapped_to_exit = 0;
   time_t now = time(NULL);
   connection_t *base_conn = ENTRY_TO_CONN(conn);
+  addressmap_entry_source_t exit_source = ADDRMAPSRC_NONE;
 
   tor_strlower(socks->address); /* normalize it */
   strlcpy(orig_address, socks->address, sizeof(orig_address));
@@ -1794,6 +1802,16 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
             safe_str_client(socks->address),
             socks->port);
 
+  if (!strcmpend(socks->address, ".exit") && !options->AllowDotExit) {
+    log_warn(LD_APP, "The  \".exit\" notation is disabled in Tor due to "
+             "security risks. Set AllowDotExit in your torrc to enable "
+             "it (at your own risk).");
+    control_event_client_status(LOG_WARN, "SOCKS_BAD_HOSTNAME HOSTNAME=%s",
+                                escaped(socks->address));
+    connection_mark_unattached_ap(conn, END_STREAM_REASON_TORPROTOCOL);
+    return -1;
+  }
+
   if (! conn->original_dest_address)
     conn->original_dest_address = tor_strdup(conn->socks_request->address);
 
@@ -1854,16 +1872,11 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
       }
     }
   } else if (!automap) {
-    int started_without_chosen_exit = strcasecmpend(socks->address, ".exit");
     /* For address map controls, remap the address. */
     if (addressmap_rewrite(socks->address, sizeof(socks->address),
-                           &map_expires)) {
+                           &map_expires, &exit_source)) {
       control_event_stream_status(conn, STREAM_EVENT_REMAP,
                                   REMAP_STREAM_SOURCE_CACHE);
-      if (started_without_chosen_exit &&
-          !strcasecmpend(socks->address, ".exit") &&
-          map_expires < TIME_MAX)
-        remapped_to_exit = 1;
     }
   }
 
@@ -1882,8 +1895,7 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
   /* Parse the address provided by SOCKS.  Modify it in-place if it
    * specifies a hidden-service (.onion) or particular exit node (.exit).
    */
-  addresstype = parse_extended_hostname(socks->address,
-                         remapped_to_exit || options->AllowDotExit);
+  addresstype = parse_extended_hostname(socks->address);
 
   if (addresstype == BAD_HOSTNAME) {
     control_event_client_status(LOG_WARN, "SOCKS_BAD_HOSTNAME HOSTNAME=%s",
@@ -1902,14 +1914,42 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
       options->_ExcludeExitNodesUnion : options->ExcludeExitNodes;
     const node_t *node;
 
+    if (exit_source == ADDRMAPSRC_AUTOMAP && !options->AllowDotExit) {
+      /* Whoops; this one is stale.  It must have gotten added earlier,
+       * when AllowDotExit was on. */
+      log_warn(LD_APP,"Stale automapped address for '%s.exit', with "
+               "AllowDotExit disabled. Refusing.",
+               safe_str_client(socks->address));
+      control_event_client_status(LOG_WARN, "SOCKS_BAD_HOSTNAME HOSTNAME=%s",
+                                  escaped(socks->address));
+      connection_mark_unattached_ap(conn, END_STREAM_REASON_TORPROTOCOL);
+      return -1;
+    }
+
+    if (exit_source == ADDRMAPSRC_DNS ||
+        (exit_source == ADDRMAPSRC_NONE && !options->AllowDotExit)) {
+      /* It shouldn't be possible to get a .exit address from any of these
+       * sources. */
+      log_warn(LD_BUG,"Address '%s.exit', with impossible source for the .exit "
+               "part. Refusing.",
+               safe_str_client(socks->address));
+      control_event_client_status(LOG_WARN, "SOCKS_BAD_HOSTNAME HOSTNAME=%s",
+                                  escaped(socks->address));
+      connection_mark_unattached_ap(conn, END_STREAM_REASON_TORPROTOCOL);
+      return -1;
+    }
+
     tor_assert(!automap);
     if (s) {
       /* The address was of the form "(stuff).(name).exit */
       if (s[1] != '\0') {
         conn->chosen_exit_name = tor_strdup(s+1);
         node = node_get_by_nickname(conn->chosen_exit_name, 1);
-        if (remapped_to_exit) /* 5 tries before it expires the addressmap */
+
+        if (exit_source == ADDRMAPSRC_TRACKEXIT) {
+          /* We 5 tries before it expires the addressmap */
           conn->chosen_exit_retries = TRACKHOSTEXITS_RETRIES;
+        }
         *s = 0;
       } else {
         /* Oops, the address was (stuff)..exit.  That's not okay. */
@@ -3407,17 +3447,14 @@ connection_ap_can_use_exit(const entry_connection_t *conn, const node_t *exit)
  * If address is of the form "y.onion" with a badly-formed handle y:
  *     Return BAD_HOSTNAME and log a message.
  *
- * If address is of the form "y.exit" and <b>allowdotexit</b> is true:
+ * If address is of the form "y.exit":
  *     Put a NUL after y and return EXIT_HOSTNAME.
  *
- * If address is of the form "y.exit" and <b>allowdotexit</b> is false:
- *     Return BAD_HOSTNAME and log a message.
- *
  * Otherwise:
  *     Return NORMAL_HOSTNAME and change nothing.
  */
 hostname_type_t
-parse_extended_hostname(char *address, int allowdotexit)
+parse_extended_hostname(char *address)
 {
     char *s;
     char query[REND_SERVICE_ID_LEN_BASE32+1];
@@ -3426,16 +3463,8 @@ parse_extended_hostname(char *address, int allowdotexit)
     if (!s)
       return NORMAL_HOSTNAME; /* no dot, thus normal */
     if (!strcmp(s+1,"exit")) {
-      if (allowdotexit) {
-        *s = 0; /* NUL-terminate it */
-        return EXIT_HOSTNAME; /* .exit */
-      } else {
-        log_warn(LD_APP, "The \".exit\" notation is disabled in Tor due to "
-                 "security risks. Set AllowDotExit in your torrc to enable "
-                 "it.");
-        /* FFFF send a controller event too to notify Vidalia users */
-        return BAD_HOSTNAME;
-      }
+      *s = 0; /* NUL-terminate it */
+      return EXIT_HOSTNAME; /* .exit */
     }
     if (strcmp(s+1,"onion"))
       return NORMAL_HOSTNAME; /* neither .exit nor .onion, thus normal */
diff --git a/src/or/connection_edge.h b/src/or/connection_edge.h
index 47c9c45b1a..b69e476ede 100644
--- a/src/or/connection_edge.h
+++ b/src/or/connection_edge.h
@@ -74,7 +74,8 @@ void addressmap_clean(time_t now);
 void addressmap_clear_configured(void);
 void addressmap_clear_transient(void);
 void addressmap_free_all(void);
-int addressmap_rewrite(char *address, size_t maxlen, time_t *expires_out);
+int addressmap_rewrite(char *address, size_t maxlen, time_t *expires_out,
+                       addressmap_entry_source_t *exit_source_out);
 int addressmap_have_mapping(const char *address, int update_timeout);
 
 void addressmap_register(const char *address, char *new_address,
@@ -100,7 +101,7 @@ int connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
 typedef enum hostname_type_t {
   NORMAL_HOSTNAME, ONION_HOSTNAME, EXIT_HOSTNAME, BAD_HOSTNAME
 } hostname_type_t;
-hostname_type_t parse_extended_hostname(char *address, int allowdotexit);
+hostname_type_t parse_extended_hostname(char *address);
 
 #if defined(HAVE_NET_IF_H) && defined(HAVE_NET_PFVAR_H)
 int get_pf_socket(void);
diff --git a/src/or/or.h b/src/or/or.h
index b8fee649ea..f1260d187b 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -3856,6 +3856,9 @@ typedef enum {
   /** We're remapping this address because we got a DNS resolution from a
    * Tor server that told us what its value was. */
   ADDRMAPSRC_DNS,
+
+  /** DOCDOC */
+  ADDRMAPSRC_NONE
 } addressmap_entry_source_t;
 
 /********************************* control.c ***************************/
diff --git a/src/or/relay.c b/src/or/relay.c
index 38a563fece..ad98e05041 100644
--- a/src/or/relay.c
+++ b/src/or/relay.c
@@ -761,7 +761,7 @@ connection_ap_process_end_not_open(
         /* rewrite it to an IP if we learned one. */
         if (addressmap_rewrite(conn->socks_request->address,
                                sizeof(conn->socks_request->address),
-                               NULL)) {
+                               NULL, NULL)) {
           control_event_stream_status(conn, STREAM_EVENT_REMAP, 0);
         }
         if (conn->chosen_exit_optional ||
diff --git a/src/test/test.c b/src/test/test.c
index 7f196aacf4..829e806759 100644
--- a/src/test/test.c
+++ b/src/test/test.c
@@ -1287,10 +1287,10 @@ test_rend_fns(void)
   char address3[] = "fooaddress.exit";
   char address4[] = "www.torproject.org";
 
-  test_assert(BAD_HOSTNAME == parse_extended_hostname(address1, 1));
-  test_assert(ONION_HOSTNAME == parse_extended_hostname(address2, 1));
-  test_assert(EXIT_HOSTNAME == parse_extended_hostname(address3, 1));
-  test_assert(NORMAL_HOSTNAME == parse_extended_hostname(address4, 1));
+  test_assert(BAD_HOSTNAME == parse_extended_hostname(address1));
+  test_assert(ONION_HOSTNAME == parse_extended_hostname(address2));
+  test_assert(EXIT_HOSTNAME == parse_extended_hostname(address3));
+  test_assert(NORMAL_HOSTNAME == parse_extended_hostname(address4));
 
   pk1 = pk_generate(0);
   pk2 = pk_generate(1);
diff --git a/src/test/test_config.c b/src/test/test_config.c
index d8161de14c..4c77af5817 100644
--- a/src/test/test_config.c
+++ b/src/test/test_config.c
@@ -42,56 +42,56 @@ test_config_addressmap(void *arg)
 
   /* MapAddress .invalidwildcard.com .torserver.exit  - no match */
   strlcpy(address, "www.invalidwildcard.com", sizeof(address));
-  test_assert(!addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(!addressmap_rewrite(address, sizeof(address), &expires, NULL));
 
   /* MapAddress *invalidasterisk.com .torserver.exit  - no match */
   strlcpy(address, "www.invalidasterisk.com", sizeof(address));
-  test_assert(!addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(!addressmap_rewrite(address, sizeof(address), &expires, NULL));
 
   /* Where no mapping for FQDN match on top-level domain */
   /* MapAddress .google.com .torserver.exit */
   strlcpy(address, "reader.google.com", sizeof(address));
-  test_assert(addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(addressmap_rewrite(address, sizeof(address), &expires, NULL));
   test_streq(address, "reader.torserver.exit");
 
   /* MapAddress *.yahoo.com *.google.com.torserver.exit */
   strlcpy(address, "reader.yahoo.com", sizeof(address));
-  test_assert(addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(addressmap_rewrite(address, sizeof(address), &expires, NULL));
   test_streq(address, "reader.google.com.torserver.exit");
 
   /*MapAddress *.cnn.com www.cnn.com */
   strlcpy(address, "cnn.com", sizeof(address));
-  test_assert(addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(addressmap_rewrite(address, sizeof(address), &expires, NULL));
   test_streq(address, "www.cnn.com");
 
   /* MapAddress .cn.com www.cnn.com */
   strlcpy(address, "www.cn.com", sizeof(address));
-  test_assert(addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(addressmap_rewrite(address, sizeof(address), &expires, NULL));
   test_streq(address, "www.cnn.com");
 
   /* MapAddress ex.com www.cnn.com  - no match */
   strlcpy(address, "www.ex.com", sizeof(address));
-  test_assert(!addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(!addressmap_rewrite(address, sizeof(address), &expires, NULL));
 
   /* MapAddress ey.com *.cnn.com - invalid expression */
   strlcpy(address, "ey.com", sizeof(address));
-  test_assert(!addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(!addressmap_rewrite(address, sizeof(address), &expires, NULL));
 
   /* Where mapping for FQDN match on FQDN */
   strlcpy(address, "www.google.com", sizeof(address));
-  test_assert(addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(addressmap_rewrite(address, sizeof(address), &expires, NULL));
   test_streq(address, "3.3.3.3");
 
   strlcpy(address, "www.torproject.org", sizeof(address));
-  test_assert(addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(addressmap_rewrite(address, sizeof(address), &expires, NULL));
   test_streq(address, "1.1.1.1");
 
   strlcpy(address, "other.torproject.org", sizeof(address));
-  test_assert(addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(addressmap_rewrite(address, sizeof(address), &expires, NULL));
   test_streq(address, "this.torproject.org.otherserver.exit");
 
   strlcpy(address, "test.torproject.org", sizeof(address));
-  test_assert(addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(addressmap_rewrite(address, sizeof(address), &expires, NULL));
   test_streq(address, "2.2.2.2");
 
   /* Test a chain of address mappings and the order in which they were added:
@@ -100,17 +100,17 @@ test_config_addressmap(void *arg)
           "MapAddress 4.4.4.4 5.5.5.5"
   */
   strlcpy(address, "www.example.org", sizeof(address));
-  test_assert(addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(addressmap_rewrite(address, sizeof(address), &expires, NULL));
   test_streq(address, "5.5.5.5");
 
   /* Test infinite address mapping results in no change */
   strlcpy(address, "www.infiniteloop.org", sizeof(address));
-  test_assert(addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(addressmap_rewrite(address, sizeof(address), &expires, NULL));
   test_streq(address, "www.infiniteloop.org");
 
   /* Test we don't find false positives */
   strlcpy(address, "www.example.com", sizeof(address));
-  test_assert(!addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(!addressmap_rewrite(address, sizeof(address), &expires, NULL));
 
   /* Test top-level-domain matching a bit harder */
   addressmap_clear_configured();
@@ -122,23 +122,23 @@ test_config_addressmap(void *arg)
   config_register_addressmaps(get_options());
 
   strlcpy(address, "www.abc.com", sizeof(address));
-  test_assert(addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(addressmap_rewrite(address, sizeof(address), &expires, NULL));
   test_streq(address, "www.abc.torserver.exit");
 
   strlcpy(address, "www.def.com", sizeof(address));
-  test_assert(addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(addressmap_rewrite(address, sizeof(address), &expires, NULL));
   test_streq(address, "www.def.torserver.exit");
 
   strlcpy(address, "www.torproject.org", sizeof(address));
-  test_assert(addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(addressmap_rewrite(address, sizeof(address), &expires, NULL));
   test_streq(address, "1.1.1.1");
 
   strlcpy(address, "test.torproject.org", sizeof(address));
-  test_assert(addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(addressmap_rewrite(address, sizeof(address), &expires, NULL));
   test_streq(address, "1.1.1.1");
 
   strlcpy(address, "torproject.net", sizeof(address));
-  test_assert(addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(addressmap_rewrite(address, sizeof(address), &expires, NULL));
   test_streq(address, "2.2.2.2");
 
   /* We don't support '*' as a mapping directive */
@@ -148,13 +148,13 @@ test_config_addressmap(void *arg)
   config_register_addressmaps(get_options());
 
   strlcpy(address, "www.abc.com", sizeof(address));
-  test_assert(!addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(!addressmap_rewrite(address, sizeof(address), &expires, NULL));
 
   strlcpy(address, "www.def.net", sizeof(address));
-  test_assert(!addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(!addressmap_rewrite(address, sizeof(address), &expires, NULL));
 
   strlcpy(address, "www.torproject.org", sizeof(address));
-  test_assert(!addressmap_rewrite(address, sizeof(address), &expires));
+  test_assert(!addressmap_rewrite(address, sizeof(address), &expires, NULL));
 
  done:
   ;