r17933@catbus: nickm | 2008-02-05 19:54:28 -0500

Stamp out a bunch of atoi users; make more tor_parse_long() users check their outputs.


svn:r13395

ChangeLog

@@ -7,6 +7,12 @@ Changes in version 0.2.0.19-alpha - 2008-02-??
     - Reject controller commands over 1MB in length.  This keeps rogue
       processes from running us out of memory.
 
+  o Minor features (misc):
+    - Reject router descriptors with out-of-range bandwidthcapacity or
+      bandwidthburst values.
+    - Give more descriptive well-formedness errors for out-of-range
+      hidden service descriptor/protocol versions.
+
   o Deprecated features (controller):
     - The status/version/num-versioning and status/version/num-concurring
       GETINFO options are no longer useful in the V3 directory protocol:

src/or/rendservice.c

@@ -195,10 +195,10 @@ parse_port_config(const char *string)
     goto err;
   }
 
-  virtport = atoi(smartlist_get(sl,0));
+  virtport = (int)tor_parse_long(smartlist_get(sl,0), 10, 1, 65535, NULL,NULL);
-  if (virtport < 1 || virtport > 65535) {
+  if (!virtport) {
-    log_warn(LD_CONFIG, "Missing or invalid port in hidden service port "
+    log_warn(LD_CONFIG, "Missing or invalid port %s in hidden service port "
-             "configuration.");
+             "configuration", escaped(smartlist_get(sl,0)));
     goto err;
   }
 
@@ -217,9 +217,12 @@ parse_port_config(const char *string)
       realport = p?p:virtport;
     } else {
       /* No addr:port, no addr -- must be port. */
-      realport = atoi(addrport);
+      realport = (int)tor_parse_long(addrport, 10, 1, 65535, NULL, NULL);
-      if (realport < 1 || realport > 65535)
+      if (!realport) {
+        log_warn(LD_CONFIG,"Unparseable or out-of-range port %s in hidden "
+                 "service port configuration.", escaped(addrport));
         goto err;
+      }
       addr = 0x7F000001u; /* Default to 127.0.0.1 */
     }
   }
@@ -300,7 +303,7 @@ rend_config_services(or_options_t *options, int validate_only)
     } else {
       smartlist_t *versions;
       char *version_str;
-      int i, version, versions_bitmask = 0;
+      int i, version, ver_ok=1, versions_bitmask = 0;
       tor_assert(!strcasecmp(line->key, "HiddenServiceVersion"));
       versions = smartlist_create();
       smartlist_split_string(versions, line->value, ",",
@@ -315,7 +318,10 @@ rend_config_services(or_options_t *options, int validate_only)
           rend_service_free(service);
           return -1;
         }
-        version = atoi(version_str);
+        version = (int)tor_parse_long(version_str, 10, 0, INT_MAX, &ver_ok,
+                                      NULL);
+        if (!ver_ok)
+          continue;
         versions_bitmask |= 1 << version;
       }
       /* If exactly one version is set, change descriptor_version to that

src/or/rephist.c

@@ -741,7 +741,7 @@ parse_possibly_bad_iso_time(const char *s, time_t *time_out)
   char b[5];
   strlcpy(b, s, sizeof(b));
   b[4] = '\0';
-  year = atoi(b);
+  year = (int)tor_parse_long(b, 10, 0, INT_MAX, NULL, NULL);
   if (year < 1970) {
     *time_out = 0;
     ++n_bogus_times;

src/or/routerparse.c

@@ -1049,6 +1049,7 @@ router_parse_entry_from_string(const char *s, const char *end,
   struct in_addr in;
   const char *start_of_annotations, *cp;
   size_t prepend_len = prepend_annotations ? strlen(prepend_annotations) : 0;
+  int ok = 1;
 
   tor_assert(!allow_annotations || !prepend_annotations);
 
@@ -1145,25 +1146,39 @@ router_parse_entry_from_string(const char *s, const char *end,
   router->addr = ntohl(in.s_addr);
 
   router->or_port =
-    (uint16_t) tor_parse_long(tok->args[2],10,0,65535,NULL,NULL);
+    (uint16_t) tor_parse_long(tok->args[2],10,0,65535,&ok,NULL);
+  if (!ok) {
+    log_warn(LD_DIR,"Invalid OR port %s", escaped(tok->args[2]));
+    goto err;
+  }
   router->dir_port =
-    (uint16_t) tor_parse_long(tok->args[4],10,0,65535,NULL,NULL);
+    (uint16_t) tor_parse_long(tok->args[4],10,0,65535,&ok,NULL);
+  if (!ok) {
+    log_warn(LD_DIR,"Invalid dir port %s", escaped(tok->args[4]));
+    goto err;
+  }
 
   tok = find_first_by_keyword(tokens, K_BANDWIDTH);
   tor_assert(tok && tok->n_args >= 3);
   router->bandwidthrate =
-    tor_parse_long(tok->args[0],10,0,INT_MAX,NULL,NULL);
+    tor_parse_long(tok->args[0],10,1,INT_MAX,&ok,NULL);
 
-  if (!router->bandwidthrate) {
+  if (!ok) {
     log_warn(LD_DIR, "bandwidthrate %s unreadable or 0. Failing.",
              escaped(tok->args[0]));
     goto err;
   }
-  router->bandwidthburst =
+  router->bandwidthburst = tor_parse_long(tok->args[1],10,0,INT_MAX,&ok,NULL);
-    tor_parse_long(tok->args[1],10,0,INT_MAX,NULL,NULL);
+  if (!ok) {
+    log_warn(LD_DIR, "Invalid bandwidthburst %s", escaped(tok->args[1]));
+    goto err;
+  }
   router->bandwidthcapacity =
-    tor_parse_long(tok->args[2],10,0,INT_MAX,NULL,NULL);
+    tor_parse_long(tok->args[2],10,0,INT_MAX,&ok,NULL);
-  /* XXXX020 we don't error-check these values? -RD */
+  if (!ok) {
+    log_warn(LD_DIR, "Invalid bandwidthcapacity %s", escaped(tok->args[1]));
+    goto err;
+  }
 
   if ((tok = find_first_by_keyword(tokens, A_PURPOSE))) {
     tor_assert(tok->n_args);
@@ -1176,7 +1191,11 @@ router_parse_entry_from_string(const char *s, const char *end,
 
   if ((tok = find_first_by_keyword(tokens, K_UPTIME))) {
     tor_assert(tok->n_args >= 1);
-    router->uptime = tor_parse_long(tok->args[0],10,0,LONG_MAX,NULL,NULL);
+    router->uptime = tor_parse_long(tok->args[0],10,0,LONG_MAX,&ok,NULL);
+    if (!ok) {
+      log_warn(LD_DIR, "Invalid uptime %s", escaped(tok->args[0]));
+      goto err;
+    }
   }
 
   if ((tok = find_first_by_keyword(tokens, K_HIBERNATING))) {
@@ -1535,7 +1554,8 @@ authority_cert_parse_from_string(const char *s, const char **end_of_string)
                                      cert->signing_key_digest);
   found = 0;
   if (old_cert) {
-    /* XXXX020 can we just compare signed_descriptor_digest ? */
+    /* XXXX We could just compare signed_descriptor_digest, but that wouldn't
+     * buy us much. */
     if (old_cert->cache_info.signed_descriptor_len == len &&
         old_cert->cache_info.signed_descriptor_body &&
         !memcmp(s, old_cert->cache_info.signed_descriptor_body, len)) {
@@ -3180,7 +3200,7 @@ rend_parse_v2_service_descriptor(rend_service_descriptor_t **parsed_out,
   smartlist_t *tokens = smartlist_create();
   directory_token_t *tok;
   char secret_id_part[DIGEST_LEN];
-  int i, version;
+  int i, version, num_ok=1;
   smartlist_t *versions;
   char public_key_hash[DIGEST_LEN];
   char test_desc_id[DIGEST_LEN];
@@ -3238,17 +3258,15 @@ rend_parse_v2_service_descriptor(rend_service_descriptor_t **parsed_out,
   tok = find_first_by_keyword(tokens, R_VERSION);
   tor_assert(tok);
   tor_assert(tok->n_args == 1);
-  result->version = atoi(tok->args[0]);
+  result->version =
-  if (result->version != 2) {
+    (int) tor_parse_long(tok->args[0], 10, 0, INT_MAX, &num_ok, NULL);
+  if (result->version != 2 || !num_ok) {
     /* If it's <2, it shouldn't be under this format.  If the number
      * is greater than 2, we bumped it because we broke backward
      * compatibility.  See how version numbers in our other formats
-     * work. -NM */
+     * work. */
-    /* That means that adding optional fields to the descriptor wouldn't
+    log_warn(LD_REND, "Unrecognized descriptor version: %s",
-     * require a new version number, but the way of verifying it's origin
+             escaped(tok->args[0]));
-     * would. Okay. -KL */
-    /* XXX020 Nick, confirm that you're happy here -RD */
-    log_warn(LD_REND, "Wrong descriptor version: %d", result->version);
     goto err;
   }
   /* Parse public key. */
@@ -3287,24 +3305,10 @@ rend_parse_v2_service_descriptor(rend_service_descriptor_t **parsed_out,
   smartlist_split_string(versions, tok->args[0], ",",
                          SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0);
   for (i = 0; i < smartlist_len(versions); i++) {
-    /* XXXX020 validate the numbers here. -NM */
+    version = (int) tor_parse_long(smartlist_get(versions, i),
-    /* As above, validating these numbers on a hidden service directory
+                                   10, 0, INT_MAX, &num_ok, NULL);
-     * might require an extension to new valid numbers at some time. But
+    if (!num_ok) /* It's a string; let's ignore it. */
-     * this would require making a distinction of hidden service direcoties
+      continue;
-     * which accept the old valid numbers and those which accept the new
-     * valid numbers. -KL */
-    /* As above, increased version numbers are for
-     * non-backward-compatible changes.  This code doesn't know how to
-     * parse a v3 descriptor, because a v3 descriptor is by definition not
-     * compatible with this code. -NM */
-    /* This refers to the permitted versions of introduction cells which might
-     * change independently from the descriptor version. If we validated the
-     * numbers here, a hidden service directory might reject a descriptor that
-     * would be understood by newer clients. Then we would need a "HSDir3" tag
-     * only to be able to use a new introduction cell version. I really think
-     * we should not validate it here. -KL */
-    /* XXX020 Nick, confirm that you're happy here -RD */
-    version = atoi(smartlist_get(versions, i));
     result->protocols |= 1 << version;
   }
   SMARTLIST_FOREACH(versions, char *, cp, tor_free(cp));
@@ -3377,7 +3381,7 @@ rend_decrypt_introduction_points(rend_service_descriptor_t *parsed,
   rend_intro_point_t *intro;
   extend_info_t *info;
   struct in_addr ip;
-  int result;
+  int result, num_ok=1;
   tor_assert(parsed);
   /** Function may only be invoked once. */
   tor_assert(!parsed->intro_nodes);
@@ -3454,13 +3458,11 @@ rend_decrypt_introduction_points(rend_service_descriptor_t *parsed,
     info->addr = ntohl(ip.s_addr);
     /* Parse onion port. */
     tok = find_first_by_keyword(tokens, R_IPO_ONION_PORT);
-    info->port = (uint16_t) atoi(tok->args[0]);
+    info->port = (uint16_t) tor_parse_long(tok->args[0],10,1,65535,
-    /* XXXX020 this next check fails with ports like 65537. -NM */
+                                           &num_ok,NULL);
-    /* No, uint16_t only allows numbers in the interval 0..65535. -KL */
+    if (!info->port || !num_ok) {
-    /* XXX020 Nick, confirm that you're happy here -RD */
+      log_warn(LD_REND, "Introduction point onion port %s is invalid",
-    if (!info->port) {
+               escaped(tok->args[0]));
-      log_warn(LD_REND, "Introduction point onion port is out of range: %d",
-               info->port);
       rend_intro_point_free(intro);
       goto err;
     }