hs-v3: Set extended error if .onion is invalid

In order to achieve this, the parse_extended_hostname() had to be refactored
to return either success or failure and setting the hostname type in the given
parameter.

The reason for that is so it can detect invalid onion addresses that is having
a ".onion", the right length but just not passing validation.

That way, we can send back the prop304 ExtendedError "X'F1' Onion Service
Descriptor Is Invalid" to notify the SOCKS connection of the invalid onion
address.

Part of #30382

Signed-off-by: David Goulet <dgoulet@torproject.org>

src/core/or/connection_edge.c

@@ -1553,6 +1553,102 @@ consider_plaintext_ports(entry_connection_t *conn, uint16_t port)
   return 0;
 }
 
+/** Parse the given hostname in address. Returns true if the parsing was
+ * successful and type_out contains the type of the hostname. Else, false is
+ * returned which means it was not recognized and type_out is set to
+ * BAD_HOSTNAME.
+ *
+ * The possible recognized forms are (where true is returned):
+ *
+ *  If address is of the form "y.onion" with a well-formed handle y:
+ *     Put a NUL after y, lower-case it, and return ONION_V2_HOSTNAME or
+ *     ONION_V3_HOSTNAME depending on the HS version.
+ *
+ *  If address is of the form "x.y.onion" with a well-formed handle x:
+ *     Drop "x.", put a NUL after y, lower-case it, and return
+ *     ONION_V2_HOSTNAME or ONION_V3_HOSTNAME depending on the HS version.
+ *
+ * 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":
+ *     Put a NUL after y and return EXIT_HOSTNAME.
+ *
+ * Otherwise:
+ *     Return NORMAL_HOSTNAME and change nothing.
+ */
+STATIC bool
+parse_extended_hostname(char *address, hostname_type_t *type_out)
+{
+  char *s;
+  char *q;
+  char query[HS_SERVICE_ADDR_LEN_BASE32+1];
+
+  s = strrchr(address,'.');
+  if (!s) {
+    *type_out = NORMAL_HOSTNAME; /* no dot, thus normal */
+    goto success;
+  }
+  if (!strcmp(s+1,"exit")) {
+    *s = 0; /* NUL-terminate it */
+    *type_out = EXIT_HOSTNAME; /* .exit */
+    goto success;
+  }
+  if (strcmp(s+1,"onion")) {
+    *type_out = NORMAL_HOSTNAME; /* neither .exit nor .onion, thus normal */
+    goto success;
+  }
+
+  /* so it is .onion */
+  *s = 0; /* NUL-terminate it */
+  /* locate a 'sub-domain' component, in order to remove it */
+  q = strrchr(address, '.');
+  if (q == address) {
+    *type_out = BAD_HOSTNAME;
+    goto failed; /* reject sub-domain, as DNS does */
+  }
+  q = (NULL == q) ? address : q + 1;
+  if (strlcpy(query, q, HS_SERVICE_ADDR_LEN_BASE32+1) >=
+      HS_SERVICE_ADDR_LEN_BASE32+1) {
+    *type_out = BAD_HOSTNAME;
+    goto failed;
+  }
+  if (q != address) {
+    memmove(address, q, strlen(q) + 1 /* also get \0 */);
+  }
+  /* v2 onion address check. */
+  if (strlen(query) == REND_SERVICE_ID_LEN_BASE32) {
+    *type_out = ONION_V2_HOSTNAME;
+    if (rend_valid_v2_service_id(query)) {
+      goto success;
+    }
+    goto failed;
+  }
+
+  /* v3 onion address check. */
+  if (strlen(query) == HS_SERVICE_ADDR_LEN_BASE32) {
+    *type_out = ONION_V3_HOSTNAME;
+    if (hs_address_is_valid(query)) {
+      goto success;
+    }
+    goto failed;
+  }
+
+  /* Reaching this point, nothing was recognized. */
+  *type_out = BAD_HOSTNAME;
+  goto failed;
+
+ success:
+  return true;
+ failed:
+  /* otherwise, return to previous state and return 0 */
+  *s = '.';
+  log_warn(LD_APP, "Invalid %shostname %s; rejecting",
+      (*type_out == (ONION_V2_HOSTNAME || ONION_V3_HOSTNAME) ? "onion " : ""),
+      safe_str_client(address));
+  return false;
+}
+
 /** How many times do we try connecting with an exit configured via
  * TrackHostExits before concluding that it won't work any more and trying a
  * different one? */
@@ -2020,16 +2116,15 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
   const int automap = rr.automap;
   const addressmap_entry_source_t exit_source = rr.exit_source;
 
-  /* Now, we parse the address to see if it's an .onion or .exit or
-   * other special address.
-   */
-  const hostname_type_t addresstype = parse_extended_hostname(socks->address);
-
   /* Now see whether the hostname is bogus.  This could happen because of an
    * onion hostname whose format we don't recognize. */
-  if (addresstype == BAD_HOSTNAME) {
+  hostname_type_t addresstype;
+  if (!parse_extended_hostname(socks->address, &addresstype)) {
     control_event_client_status(LOG_WARN, "SOCKS_BAD_HOSTNAME HOSTNAME=%s",
                                 escaped(socks->address));
+    if (addresstype == ONION_V3_HOSTNAME) {
+      conn->socks_request->socks_extended_error_code = SOCKS5_HS_IS_INVALID;
+    }
     connection_mark_unattached_ap(conn, END_STREAM_REASON_TORPROTOCOL);
     return -1;
   }
@@ -4312,68 +4407,6 @@ connection_ap_can_use_exit(const entry_connection_t *conn,
   return 1;
 }
 
-/** If address is of the form "y.onion" with a well-formed handle y:
- *     Put a NUL after y, lower-case it, and return ONION_V2_HOSTNAME or
- *     ONION_V3_HOSTNAME depending on the HS version.
- *
- *  If address is of the form "x.y.onion" with a well-formed handle x:
- *     Drop "x.", put a NUL after y, lower-case it, and return
- *     ONION_V2_HOSTNAME or ONION_V3_HOSTNAME depending on the HS version.
- *
- * 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":
- *     Put a NUL after y and return EXIT_HOSTNAME.
- *
- * Otherwise:
- *     Return NORMAL_HOSTNAME and change nothing.
- */
-hostname_type_t
-parse_extended_hostname(char *address)
-{
-    char *s;
-    char *q;
-    char query[HS_SERVICE_ADDR_LEN_BASE32+1];
-
-    s = strrchr(address,'.');
-    if (!s)
-      return NORMAL_HOSTNAME; /* no dot, thus normal */
-    if (!strcmp(s+1,"exit")) {
-      *s = 0; /* NUL-terminate it */
-      return EXIT_HOSTNAME; /* .exit */
-    }
-    if (strcmp(s+1,"onion"))
-      return NORMAL_HOSTNAME; /* neither .exit nor .onion, thus normal */
-
-    /* so it is .onion */
-    *s = 0; /* NUL-terminate it */
-    /* locate a 'sub-domain' component, in order to remove it */
-    q = strrchr(address, '.');
-    if (q == address) {
-      goto failed; /* reject sub-domain, as DNS does */
-    }
-    q = (NULL == q) ? address : q + 1;
-    if (strlcpy(query, q, HS_SERVICE_ADDR_LEN_BASE32+1) >=
-        HS_SERVICE_ADDR_LEN_BASE32+1)
-      goto failed;
-    if (q != address) {
-      memmove(address, q, strlen(q) + 1 /* also get \0 */);
-    }
-    if (rend_valid_v2_service_id(query)) {
-      return ONION_V2_HOSTNAME; /* success */
-    }
-    if (hs_address_is_valid(query)) {
-      return ONION_V3_HOSTNAME;
-    }
- failed:
-    /* otherwise, return to previous state and return 0 */
-    *s = '.';
-    log_warn(LD_APP, "Invalid onion hostname %s; rejecting",
-             safe_str_client(address));
-    return BAD_HOSTNAME;
-}
-
 /** Return true iff the (possibly NULL) <b>alen</b>-byte chunk of memory at
  * <b>a</b> is equal to the (possibly NULL) <b>blen</b>-byte chunk of memory
  * at <b>b</b>. */

src/core/or/connection_edge.h

@@ -71,6 +71,15 @@ entry_connection_t *EDGE_TO_ENTRY_CONN(edge_connection_t *);
 #define connection_mark_unattached_ap(conn, endreason)                  \
   connection_mark_unattached_ap_((conn), (endreason), __LINE__, SHORT_FILE__)
 
+/** Possible return values for parse_extended_hostname. */
+typedef enum hostname_type_t {
+  BAD_HOSTNAME,
+  EXIT_HOSTNAME,
+  NORMAL_HOSTNAME,
+  ONION_V2_HOSTNAME,
+  ONION_V3_HOSTNAME,
+} hostname_type_t;
+
 MOCK_DECL(void,connection_mark_unattached_ap_,
           (entry_connection_t *conn, int endreason,
            int line, const char *file));
@@ -155,13 +164,6 @@ int connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
                                                origin_circuit_t *circ,
                                                crypt_path_t *cpath);
 
-/** Possible return values for parse_extended_hostname. */
-typedef enum hostname_type_t {
-  NORMAL_HOSTNAME, ONION_V2_HOSTNAME, ONION_V3_HOSTNAME,
-  EXIT_HOSTNAME, BAD_HOSTNAME
-} hostname_type_t;
-hostname_type_t parse_extended_hostname(char *address);
-
 #if defined(HAVE_NET_IF_H) && defined(HAVE_NET_PFVAR_H)
 int get_pf_socket(void);
 #endif
@@ -219,6 +221,8 @@ void half_edge_free_(struct half_edge_t *he);
 
 #ifdef CONNECTION_EDGE_PRIVATE
 
+STATIC bool parse_extended_hostname(char *address, hostname_type_t *type_out);
+
 /** A parsed BEGIN or BEGIN_DIR cell */
 typedef struct begin_cell_t {
   /** The address the client has asked us to connect to, or NULL if this is

src/test/test_hs_common.c

@@ -6,6 +6,7 @@
  * \brief Test hidden service common functionalities.
  */
 
+#define CONNECTION_EDGE_PRIVATE
 #define HS_COMMON_PRIVATE
 #define HS_CLIENT_PRIVATE
 #define HS_SERVICE_PRIVATE
@@ -778,6 +779,7 @@ static void
 test_parse_extended_hostname(void *arg)
 {
   (void) arg;
+  hostname_type_t type;
 
   char address1[] = "fooaddress.onion";
   char address2[] = "aaaaaaaaaaaaaaaa.onion";
@@ -788,21 +790,42 @@ test_parse_extended_hostname(void *arg)
   char address7[] = ".abcdefghijklmnop.onion";
   char address8[] =
     "www.25njqamcweflpvkl73j4szahhihoc4xt3ktcgjnpaingr5yhkenl5sid.onion";
+  char address9[] =
+    "www.15njqamcweflpvkl73j4szahhihoc4xt3ktcgjnpaingr5yhkenl5sid.onion";
 
-  tt_assert(BAD_HOSTNAME == parse_extended_hostname(address1));
-  tt_assert(ONION_V2_HOSTNAME == parse_extended_hostname(address2));
+  tt_assert(!parse_extended_hostname(address1, &type));
+  tt_int_op(type, OP_EQ, BAD_HOSTNAME);
+
+  tt_assert(parse_extended_hostname(address2, &type));
+  tt_int_op(type, OP_EQ, ONION_V2_HOSTNAME);
   tt_str_op(address2, OP_EQ, "aaaaaaaaaaaaaaaa");
-  tt_assert(EXIT_HOSTNAME == parse_extended_hostname(address3));
-  tt_assert(NORMAL_HOSTNAME == parse_extended_hostname(address4));
-  tt_assert(ONION_V2_HOSTNAME == parse_extended_hostname(address5));
+
+  tt_assert(parse_extended_hostname(address3, &type));
+  tt_int_op(type, OP_EQ, EXIT_HOSTNAME);
+
+  tt_assert(parse_extended_hostname(address4, &type));
+  tt_int_op(type, OP_EQ, NORMAL_HOSTNAME);
+
+  tt_assert(parse_extended_hostname(address5, &type));
+  tt_int_op(type, OP_EQ, ONION_V2_HOSTNAME);
   tt_str_op(address5, OP_EQ, "abcdefghijklmnop");
-  tt_assert(ONION_V2_HOSTNAME == parse_extended_hostname(address6));
+
+  tt_assert(parse_extended_hostname(address6, &type));
+  tt_int_op(type, OP_EQ, ONION_V2_HOSTNAME);
   tt_str_op(address6, OP_EQ, "abcdefghijklmnop");
-  tt_assert(BAD_HOSTNAME == parse_extended_hostname(address7));
-  tt_assert(ONION_V3_HOSTNAME == parse_extended_hostname(address8));
+
+  tt_assert(!parse_extended_hostname(address7, &type));
+  tt_int_op(type, OP_EQ, BAD_HOSTNAME);
+
+  tt_assert(parse_extended_hostname(address8, &type));
+  tt_int_op(type, OP_EQ, ONION_V3_HOSTNAME);
   tt_str_op(address8, OP_EQ,
             "25njqamcweflpvkl73j4szahhihoc4xt3ktcgjnpaingr5yhkenl5sid");
 
+  /* Invalid v3 address. */
+  tt_assert(!parse_extended_hostname(address9, &type));
+  tt_int_op(type, OP_EQ, ONION_V3_HOSTNAME);
+
  done: ;
 }