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.

changes/bug3940_redux

@@ -0,0 +1,5 @@
+  o Major bugfixes:
+    - Change the AllowDotExit rules so they should actually work.
+      We now enforce AllowDotExit only immediately after receiving
+      an address via SOCKS or DNSPort: other sources are free to provide
+      .exit addresses after the resolution occurs.

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 */

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);

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 ***************************/

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 ||

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);

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:
   ;