Merge remote-tracking branch 'public/bug3940_redux'

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,22 @@ 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.
+ *
+ * If <b>exit_source_out</b> is non-null, we set it as follows.  If we the
+ * address starts out as a non-exit address, and we remap it to an .exit
+ * address at any point, then set *<b>exit_source_out</b> to the
+ * address_entry_source_t of the first such rule.  Set *<b>exit_source_out</b>
+ * to ADDRMAPSRC_NONE if there is no such rewrite.
+ *
  */
 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 +1126,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)
+        goto done;
-          *expires_out = expires;
-        return rewrites > 0;
       }
 
       exact_match = 1;
@@ -1127,9 +1134,7 @@ addressmap_rewrite(char *address, size_t maxlen, time_t *expires_out)
 
     if (!ent || !ent->new_address) {
       tor_free(addr_orig);
-      if (expires_out)
+      goto done;
-        *expires_out = expires;
-      return (rewrites > 0); /* done, no rewrite needed */
     }
 
     if (ent->dst_wildcard && !exact_match) {
@@ -1139,6 +1144,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 +1160,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 +1797,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 +1807,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 +1877,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 +1900,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,
+  addresstype = parse_extended_hostname(socks->address);
-                         remapped_to_exit || options->AllowDotExit);
 
   if (addresstype == BAD_HOSTNAME) {
     control_event_client_status(LOG_WARN, "SOCKS_BAD_HOSTNAME HOSTNAME=%s",
@@ -1902,14 +1919,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. */
@@ -3427,17 +3472,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];
@@ -3446,16 +3488,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 */
-        *s = 0; /* NUL-terminate it */
+      return EXIT_HOSTNAME; /* .exit */
-        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;
-      }
     }
     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,
@@ -101,7 +102,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

@@ -3855,6 +3855,11 @@ 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,
+
+  /** No remapping has occurred.  This isn't a possible value for an
+   * addrmap_entry_t; it's used as a null value when we need to answer "Why
+   * did this remapping happen." */
+  ADDRMAPSRC_NONE
 } addressmap_entry_source_t;
 
 /********************************* control.c ***************************/

src/or/relay.c

@@ -764,7 +764,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

@@ -1290,10 +1290,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(BAD_HOSTNAME == parse_extended_hostname(address1));
-  test_assert(ONION_HOSTNAME == parse_extended_hostname(address2, 1));
+  test_assert(ONION_HOSTNAME == parse_extended_hostname(address2));
-  test_assert(EXIT_HOSTNAME == parse_extended_hostname(address3, 1));
+  test_assert(EXIT_HOSTNAME == parse_extended_hostname(address3));
-  test_assert(NORMAL_HOSTNAME == parse_extended_hostname(address4, 1));
+  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:
   ;