Merge branch 'tor-github/pr/1309'

changes/ticket31675

@@ -0,0 +1,3 @@
+  o Code simplification and refactoring:
+    - Refactor the microdescs_parse_from_string() function into smaller
+      pieces, for better comprehensibility.  Closes ticket 31675.

scripts/maint/practracker/exceptions.txt

@@ -227,7 +227,6 @@ problem function-size /src/feature/dirclient/dirclient.c:handle_response_fetch_c
 problem function-size /src/feature/dircommon/consdiff.c:gen_ed_diff() 203
 problem function-size /src/feature/dircommon/consdiff.c:apply_ed_diff() 158
 problem function-size /src/feature/dirparse/authcert_parse.c:authority_cert_parse_from_string() 181
-problem function-size /src/feature/dirparse/microdesc_parse.c:microdescs_parse_from_string() 166
 problem function-size /src/feature/dirparse/ns_parse.c:routerstatus_parse_entry_from_string() 280
 problem function-size /src/feature/dirparse/ns_parse.c:networkstatus_verify_bw_weights() 389
 problem function-size /src/feature/dirparse/ns_parse.c:networkstatus_parse_vote_from_string() 635

src/feature/dirparse/microdesc_parse.c

@@ -98,6 +98,184 @@ policy_is_reject_star_or_null(struct short_policy_t *policy)
   return !policy || short_policy_is_reject_star(policy);
 }
 
+/**
+ * Return a human-readable description of a given saved_location_t.
+ * Never returns NULL.
+ **/
+static const char *
+saved_location_to_string(saved_location_t where)
+{
+  const char *location;
+  switch (where) {
+    case SAVED_NOWHERE:
+      location = "download or generated string";
+      break;
+    case SAVED_IN_CACHE:
+      location = "cache";
+      break;
+    case SAVED_IN_JOURNAL:
+      location = "journal";
+      break;
+    default:
+      location = "unknown location";
+      break;
+  }
+  return location;
+}
+
+/**
+ * Given a microdescriptor stored in <b>where</b> which starts at <b>s</b>,
+ * which ends at <b>start_of_next_microdescriptor</b>, and which is located
+ * within a larger document beginning at <b>start</b>: Fill in the body,
+ * bodylen, bodylen, saved_location, off, and digest fields of <b>md</b> as
+ * appropriate.
+ *
+ * The body field will be an alias within <b>s</b> if <b>saved_location</b>
+ * is SAVED_IN_CACHE, and will be copied into body and nul-terminated
+ * otherwise.
+ **/
+static int
+microdesc_extract_body(microdesc_t *md,
+                       const char *start,
+                       const char *s, const char *start_of_next_microdesc,
+                       saved_location_t where)
+{
+  const bool copy_body = (where != SAVED_IN_CACHE);
+
+  const char *cp = tor_memstr(s, start_of_next_microdesc-s, "onion-key");
+
+  const bool no_onion_key = (cp == NULL);
+  if (no_onion_key) {
+    cp = s; /* So that we have *some* junk to put in the body */
+  }
+
+  md->bodylen = start_of_next_microdesc - cp;
+  md->saved_location = where;
+  if (copy_body)
+    md->body = tor_memdup_nulterm(cp, md->bodylen);
+  else
+    md->body = (char*)cp;
+  md->off = cp - start;
+
+  crypto_digest256(md->digest, md->body, md->bodylen, DIGEST_SHA256);
+
+  return no_onion_key ? -1 : 0;
+}
+
+/**
+ * Parse a microdescriptor which begins at <b>s</b> and ends at
+ * <b>start_of_next_microdesc.  Store its fields into <b>md</b>.  Use
+ * <b>where</b> for generating log information.  If <b>allow_annotations</b>
+ * is true, then one or more annotations may precede the microdescriptor body
+ * proper.  Use <b>area</b> for memory management, clearing it when done.
+ *
+ * On success, return 0; otherwise return -1.
+ **/
+static int
+microdesc_parse_fields(microdesc_t *md,
+                       memarea_t *area,
+                       const char *s, const char *start_of_next_microdesc,
+                       int allow_annotations,
+                       saved_location_t where)
+{
+  smartlist_t *tokens = smartlist_new();
+  int rv = -1;
+  int flags = allow_annotations ? TS_ANNOTATIONS_OK : 0;
+  directory_token_t *tok;
+
+  if (tokenize_string(area, s, start_of_next_microdesc, tokens,
+                      microdesc_token_table, flags)) {
+    log_warn(LD_DIR, "Unparseable microdescriptor found in %s",
+             saved_location_to_string(where));
+    goto err;
+  }
+
+  if ((tok = find_opt_by_keyword(tokens, A_LAST_LISTED))) {
+    if (parse_iso_time(tok->args[0], &md->last_listed)) {
+      log_warn(LD_DIR, "Bad last-listed time in microdescriptor");
+      goto err;
+    }
+  }
+
+  tok = find_by_keyword(tokens, K_ONION_KEY);
+  if (!crypto_pk_public_exponent_ok(tok->key)) {
+    log_warn(LD_DIR,
+             "Relay's onion key had invalid exponent.");
+    goto err;
+  }
+  md->onion_pkey = tor_memdup(tok->object_body, tok->object_size);
+  md->onion_pkey_len = tok->object_size;
+  crypto_pk_free(tok->key);
+
+  if ((tok = find_opt_by_keyword(tokens, K_ONION_KEY_NTOR))) {
+    curve25519_public_key_t k;
+    tor_assert(tok->n_args >= 1);
+    if (curve25519_public_from_base64(&k, tok->args[0]) < 0) {
+      log_warn(LD_DIR, "Bogus ntor-onion-key in microdesc");
+      goto err;
+    }
+    md->onion_curve25519_pkey =
+      tor_memdup(&k, sizeof(curve25519_public_key_t));
+  }
+
+  smartlist_t *id_lines = find_all_by_keyword(tokens, K_ID);
+  if (id_lines) {
+    SMARTLIST_FOREACH_BEGIN(id_lines, directory_token_t *, t) {
+      tor_assert(t->n_args >= 2);
+      if (!strcmp(t->args[0], "ed25519")) {
+        if (md->ed25519_identity_pkey) {
+          log_warn(LD_DIR, "Extra ed25519 key in microdesc");
+          smartlist_free(id_lines);
+          goto err;
+        }
+        ed25519_public_key_t k;
+        if (ed25519_public_from_base64(&k, t->args[1])<0) {
+          log_warn(LD_DIR, "Bogus ed25519 key in microdesc");
+          smartlist_free(id_lines);
+          goto err;
+        }
+        md->ed25519_identity_pkey = tor_memdup(&k, sizeof(k));
+      }
+    } SMARTLIST_FOREACH_END(t);
+    smartlist_free(id_lines);
+  }
+
+  {
+    smartlist_t *a_lines = find_all_by_keyword(tokens, K_A);
+    if (a_lines) {
+      find_single_ipv6_orport(a_lines, &md->ipv6_addr, &md->ipv6_orport);
+      smartlist_free(a_lines);
+    }
+  }
+
+  if ((tok = find_opt_by_keyword(tokens, K_FAMILY))) {
+    md->family = nodefamily_parse(tok->args[0],
+                                  NULL,
+                                  NF_WARN_MALFORMED);
+  }
+
+  if ((tok = find_opt_by_keyword(tokens, K_P))) {
+    md->exit_policy = parse_short_policy(tok->args[0]);
+  }
+  if ((tok = find_opt_by_keyword(tokens, K_P6))) {
+    md->ipv6_exit_policy = parse_short_policy(tok->args[0]);
+  }
+
+  if (policy_is_reject_star_or_null(md->exit_policy) &&
+      policy_is_reject_star_or_null(md->ipv6_exit_policy)) {
+    md->policy_is_reject_star = 1;
+  }
+
+  rv = 0;
+ err:
+
+  SMARTLIST_FOREACH(tokens, directory_token_t *, t, token_clear(t));
+  memarea_clear(area);
+  smartlist_free(tokens);
+
+  return rv;
+}
+
 /** Parse as many microdescriptors as are found from the string starting at
  * <b>s</b> and ending at <b>eos</b>.  If allow_annotations is set, read any
  * annotations we recognize and ignore ones we don't.
@@ -115,16 +293,11 @@ microdescs_parse_from_string(const char *s, const char *eos,
                              saved_location_t where,
                              smartlist_t *invalid_digests_out)
 {
-  smartlist_t *tokens;
   smartlist_t *result;
   microdesc_t *md = NULL;
   memarea_t *area;
   const char *start = s;
   const char *start_of_next_microdesc;
-  int flags = allow_annotations ? TS_ANNOTATIONS_OK : 0;
-  const int copy_body = (where != SAVED_IN_CACHE);
-
-  directory_token_t *tok;
 
   if (!eos)
     eos = s + strlen(s);
@@ -132,156 +305,47 @@ microdescs_parse_from_string(const char *s, const char *eos,
   s = eat_whitespace_eos(s, eos);
   area = memarea_new();
   result = smartlist_new();
-  tokens = smartlist_new();
 
   while (s < eos) {
-    int okay = 0;
+   bool okay = false;
 
     start_of_next_microdesc = find_start_of_next_microdesc(s, eos);
     if (!start_of_next_microdesc)
       start_of_next_microdesc = eos;
 
     md = tor_malloc_zero(sizeof(microdesc_t));
+    uint8_t md_digest[DIGEST256_LEN];
     {
-      const char *cp = tor_memstr(s, start_of_next_microdesc-s,
-                                  "onion-key");
-      const int no_onion_key = (cp == NULL);
-      if (no_onion_key) {
-        cp = s; /* So that we have *some* junk to put in the body */
-      }
+      const bool body_not_found =
+        microdesc_extract_body(md, start, s,
+                               start_of_next_microdesc,
+                               where) < 0;
 
-      md->bodylen = start_of_next_microdesc - cp;
-      md->saved_location = where;
-      if (copy_body)
-        md->body = tor_memdup_nulterm(cp, md->bodylen);
-      else
-        md->body = (char*)cp;
-      md->off = cp - start;
-      crypto_digest256(md->digest, md->body, md->bodylen, DIGEST_SHA256);
-      if (no_onion_key) {
+      memcpy(md_digest, md->digest, DIGEST256_LEN);
+      if (body_not_found) {
         log_fn(LOG_PROTOCOL_WARN, LD_DIR, "Malformed or truncated descriptor");
         goto next;
       }
     }
 
-    if (tokenize_string(area, s, start_of_next_microdesc, tokens,
-                        microdesc_token_table, flags)) {
-      const char *location;
-      switch (where) {
-        case SAVED_NOWHERE:
-          location = "download or generated string";
-          break;
-        case SAVED_IN_CACHE:
-          location = "cache";
-          break;
-        case SAVED_IN_JOURNAL:
-          location = "journal";
-          break;
-        default:
-          location = "unknown location";
-          break;
-      }
-      log_warn(LD_DIR, "Unparseable microdescriptor found in %s", location);
-      goto next;
-    }
-
-    if ((tok = find_opt_by_keyword(tokens, A_LAST_LISTED))) {
-      if (parse_iso_time(tok->args[0], &md->last_listed)) {
-        log_warn(LD_DIR, "Bad last-listed time in microdescriptor");
-        goto next;
-      }
-    }
-
-    tok = find_by_keyword(tokens, K_ONION_KEY);
-    if (!crypto_pk_public_exponent_ok(tok->key)) {
-      log_warn(LD_DIR,
-               "Relay's onion key had invalid exponent.");
-      goto next;
-    }
-    md->onion_pkey = tor_memdup(tok->object_body, tok->object_size);
-    md->onion_pkey_len = tok->object_size;
-    crypto_pk_free(tok->key);
-
-    if ((tok = find_opt_by_keyword(tokens, K_ONION_KEY_NTOR))) {
-      curve25519_public_key_t k;
-      tor_assert(tok->n_args >= 1);
-      if (curve25519_public_from_base64(&k, tok->args[0]) < 0) {
-        log_warn(LD_DIR, "Bogus ntor-onion-key in microdesc");
-        goto next;
-      }
-      md->onion_curve25519_pkey =
-        tor_memdup(&k, sizeof(curve25519_public_key_t));
-    }
-
-    smartlist_t *id_lines = find_all_by_keyword(tokens, K_ID);
-    if (id_lines) {
-      SMARTLIST_FOREACH_BEGIN(id_lines, directory_token_t *, t) {
-        tor_assert(t->n_args >= 2);
-        if (!strcmp(t->args[0], "ed25519")) {
-          if (md->ed25519_identity_pkey) {
-            log_warn(LD_DIR, "Extra ed25519 key in microdesc");
-            smartlist_free(id_lines);
-            goto next;
-          }
-          ed25519_public_key_t k;
-          if (ed25519_public_from_base64(&k, t->args[1])<0) {
-            log_warn(LD_DIR, "Bogus ed25519 key in microdesc");
-            smartlist_free(id_lines);
-            goto next;
-          }
-          md->ed25519_identity_pkey = tor_memdup(&k, sizeof(k));
-        }
-      } SMARTLIST_FOREACH_END(t);
-      smartlist_free(id_lines);
-    }
-
-    {
-      smartlist_t *a_lines = find_all_by_keyword(tokens, K_A);
-      if (a_lines) {
-        find_single_ipv6_orport(a_lines, &md->ipv6_addr, &md->ipv6_orport);
-        smartlist_free(a_lines);
-      }
-    }
-
-    if ((tok = find_opt_by_keyword(tokens, K_FAMILY))) {
-      md->family = nodefamily_parse(tok->args[0],
-                                    NULL,
-                                    NF_WARN_MALFORMED);
-    }
-
-    if ((tok = find_opt_by_keyword(tokens, K_P))) {
-      md->exit_policy = parse_short_policy(tok->args[0]);
-    }
-    if ((tok = find_opt_by_keyword(tokens, K_P6))) {
-      md->ipv6_exit_policy = parse_short_policy(tok->args[0]);
-    }
-
-    if (policy_is_reject_star_or_null(md->exit_policy) &&
-        policy_is_reject_star_or_null(md->ipv6_exit_policy)) {
-      md->policy_is_reject_star = 1;
+    if (microdesc_parse_fields(md, area, s, start_of_next_microdesc,
+                               allow_annotations, where) == 0) {
+      smartlist_add(result, md);
+      md = NULL; // prevent free
+      okay = true;
     }
 
-    smartlist_add(result, md);
-    okay = 1;
-
-    md = NULL;
   next:
     if (! okay && invalid_digests_out) {
       smartlist_add(invalid_digests_out,
-                    tor_memdup(md->digest, DIGEST256_LEN));
+                    tor_memdup(md_digest, DIGEST256_LEN));
     }
     microdesc_free(md);
     md = NULL;
-
-    SMARTLIST_FOREACH(tokens, directory_token_t *, t, token_clear(t));
-    memarea_clear(area);
-    smartlist_clear(tokens);
     s = start_of_next_microdesc;
   }
 
-  SMARTLIST_FOREACH(tokens, directory_token_t *, t, token_clear(t));
   memarea_drop_all(area);
-  smartlist_free(tokens);
 
   return result;
 }

src/test/test_microdesc.c

@@ -21,6 +21,7 @@
 #include "feature/nodelist/routerstatus_st.h"
 
 #include "test/test.h"
+#include "test/log_test_helpers.h"
 
 #ifdef HAVE_SYS_STAT_H
 #include <sys/stat.h>
@@ -770,6 +771,80 @@ test_md_parse(void *arg)
   tor_free(mem_op_hex_tmp);
 }
 
+static void
+test_md_parse_id_ed25519(void *arg)
+{
+  (void)arg;
+
+  /* A correct MD with an ed25519 ID ... and an unspecified ID type,
+   * which is permitted. */
+  const char GOOD_MD[] =
+    "onion-key\n"
+    "-----BEGIN RSA PUBLIC KEY-----\n"
+    "MIGJAoGBAM7uUtq5F6h63QNYIvC+4NcWaD0DjtnrOORZMkdpJhinXUOwce3cD5Dj\n"
+    "sgdN1wJpWpTQMXJ2DssfSgmOVXETP7qJuZyRprxalQhaEATMDNJA/66Ml1jSO9mZ\n"
+    "+8Xb7m/4q778lNtkSbsvMaYD2Dq6k2QQ3kMhr9z8oUtX0XA23+pfAgMBAAE=\n"
+    "-----END RSA PUBLIC KEY-----\n"
+    "id ed25519 VGhpcyBpc24ndCBhY3R1YWxseSBhIHB1YmxpYyBrZXk\n"
+    "id wumpus dodecahedron\n";
+
+  smartlist_t *mds = NULL;
+  const microdesc_t *md;
+
+  mds = microdescs_parse_from_string(GOOD_MD,
+                                     NULL, 1, SAVED_NOWHERE, NULL);
+  tt_assert(mds);
+  tt_int_op(smartlist_len(mds), OP_EQ, 1);
+  md = smartlist_get(mds, 0);
+  tt_mem_op(md->ed25519_identity_pkey, OP_EQ,
+            "This isn't actually a public key", ED25519_PUBKEY_LEN);
+  SMARTLIST_FOREACH(mds, microdesc_t *, m, microdesc_free(m));
+  smartlist_free(mds);
+
+  /* As above, but ed25519 ID key appears twice. */
+  const char DUPLICATE_KEY[] =
+    "onion-key\n"
+    "-----BEGIN RSA PUBLIC KEY-----\n"
+    "MIGJAoGBAM7uUtq5F6h63QNYIvC+4NcWaD0DjtnrOORZMkdpJhinXUOwce3cD5Dj\n"
+    "sgdN1wJpWpTQMXJ2DssfSgmOVXETP7qJuZyRprxalQhaEATMDNJA/66Ml1jSO9mZ\n"
+    "+8Xb7m/4q778lNtkSbsvMaYD2Dq6k2QQ3kMhr9z8oUtX0XA23+pfAgMBAAE=\n"
+    "-----END RSA PUBLIC KEY-----\n"
+    "id ed25519 VGhpcyBpc24ndCBhY3R1YWxseSBhIHB1YmxpYyBrZXk\n"
+    "id ed25519 VGhpcyBpc24ndCBhY3R1YWxseSBhIHB1YmxpYyBrZXk\n";
+
+  setup_capture_of_logs(LOG_WARN);
+  mds = microdescs_parse_from_string(DUPLICATE_KEY,
+                                     NULL, 1, SAVED_NOWHERE, NULL);
+  tt_assert(mds);
+  tt_int_op(smartlist_len(mds), OP_EQ, 0); // no entries.
+  expect_single_log_msg_containing("Extra ed25519 key");
+  mock_clean_saved_logs();
+  smartlist_free(mds);
+
+  /* As above, but ed25519 ID key is invalid. */
+  const char BOGUS_KEY[] =
+    "onion-key\n"
+    "-----BEGIN RSA PUBLIC KEY-----\n"
+    "MIGJAoGBAM7uUtq5F6h63QNYIvC+4NcWaD0DjtnrOORZMkdpJhinXUOwce3cD5Dj\n"
+    "sgdN1wJpWpTQMXJ2DssfSgmOVXETP7qJuZyRprxalQhaEATMDNJA/66Ml1jSO9mZ\n"
+    "+8Xb7m/4q778lNtkSbsvMaYD2Dq6k2QQ3kMhr9z8oUtX0XA23+pfAgMBAAE=\n"
+    "-----END RSA PUBLIC KEY-----\n"
+    "id ed25519 VGhpcyBpc24ndCBhY3R1YWxseSBhIHB1YmxpYyZZZZZZZZZZZ\n";
+
+  mds = microdescs_parse_from_string(BOGUS_KEY,
+                                     NULL, 1, SAVED_NOWHERE, NULL);
+  tt_assert(mds);
+  tt_int_op(smartlist_len(mds), OP_EQ, 0); // no entries.
+  expect_single_log_msg_containing("Bogus ed25519 key");
+
+ done:
+  if (mds) {
+    SMARTLIST_FOREACH(mds, microdesc_t *, m, microdesc_free(m));
+    smartlist_free(mds);
+  }
+  teardown_capture_of_logs();
+}
+
 static int mock_rgsbd_called = 0;
 static routerstatus_t *mock_rgsbd_val_a = NULL;
 static routerstatus_t *mock_rgsbd_val_b = NULL;
@@ -903,6 +978,7 @@ struct testcase_t microdesc_tests[] = {
   { "broken_cache", test_md_cache_broken, TT_FORK, NULL, NULL },
   { "generate", test_md_generate, 0, NULL, NULL },
   { "parse", test_md_parse, 0, NULL, NULL },
+  { "parse_id_ed25519", test_md_parse_id_ed25519, 0, NULL, NULL },
   { "reject_cache", test_md_reject_cache, TT_FORK, NULL, NULL },
   { "corrupt_desc", test_md_corrupt_desc, TT_FORK, NULL, NULL },
   END_OF_TESTCASES