Merge remote-tracking branch 'teor/bug13111-empty-key-files-fn-empty'

changes/bug13111-generate-keys-on-empty-file

@@ -0,0 +1,23 @@
+  o Minor bugfixes (file handling):
+    - Stop failing when key files are zero-length. Instead, generate new
+      keys, and overwrite the empty key files.
+      Fixes bug 13111. Patch by "teor".
+    - Stop generating a fresh .old RSA key file when the .old file is missing.
+    - Avoid overwriting .old key files with empty key files.
+    - Stop crashing when a NULL filename is passed to file_status().
+      Fixed as part of bug 13111. Patches by "teor".
+
+  o Minor enhancements (file handling):
+    - Skip loading zero-length extra info store, router store, stats, state,
+      and key files.
+    - Return FN_ERROR when a zero-length filename is passed to file_status().
+      Fixed as part of bug 13111. Patches by "teor".
+
+  o Minor enhancements (testing):
+    - Test that tor does not fail when key files are zero-length.
+      Check that tor generates new keys, and overwrites the empty key files.
+    - Test that tor generates new keys when keys are missing (existing
+      behaviour).
+    - Test that tor does not overwrite key files that already contain data
+      (existing behaviour).
+      Tests bug 13111. Patch by "teor".

changes/bug14001-clang-warning

@@ -0,0 +1,6 @@
+  o Minor bugfixes:
+    - The address of an array in the middle of a structure will
+      always be non-NULL. clang recognises this and complains.
+      Disable the tautologous and redundant check to silence
+      this warning.
+      Fixes bug 14001.

src/common/compat.c

@@ -823,6 +823,7 @@ replace_file(const char *from, const char *to)
     case FN_NOENT:
       break;
     case FN_FILE:
+    case FN_EMPTY:
       if (unlink(to)) return -1;
       break;
     case FN_ERROR:

src/common/util.c

@@ -2018,15 +2018,24 @@ clean_name_for_stat(char *name)
 #endif
 }
 
-/** Return FN_ERROR if filename can't be read, FN_NOENT if it doesn't
- * exist, FN_FILE if it is a regular file, or FN_DIR if it's a
- * directory.  On FN_ERROR, sets errno. */
+/** Return:
+ * FN_ERROR if filename can't be read, is NULL, or is zero-length,
+ * FN_NOENT if it doesn't exist,
+ * FN_FILE if it is a non-empty regular file, or a FIFO on unix-like systems,
+ * FN_EMPTY for zero-byte regular files,
+ * FN_DIR if it's a directory, and
+ * FN_ERROR for any other file type.
+ * On FN_ERROR and FN_NOENT, sets errno.  (errno is not set when FN_ERROR
+ * is returned due to an unhandled file type.) */
 file_status_t
 file_status(const char *fname)
 {
   struct stat st;
   char *f;
   int r;
+  if (!fname || strlen(fname) == 0) {
+    return FN_ERROR;
+  }
   f = tor_strdup(fname);
   clean_name_for_stat(f);
   log_debug(LD_FS, "stat()ing %s", f);
@@ -2038,16 +2047,23 @@ file_status(const char *fname)
     }
     return FN_ERROR;
   }
-  if (st.st_mode & S_IFDIR)
+  if (st.st_mode & S_IFDIR) {
     return FN_DIR;
-  else if (st.st_mode & S_IFREG)
-    return FN_FILE;
+  } else if (st.st_mode & S_IFREG) {
+    if (st.st_size > 0) {
+      return FN_FILE;
+    } else if (st.st_size == 0) {
+      return FN_EMPTY;
+    } else {
+      return FN_ERROR;
+    }
 #ifndef _WIN32
-  else if (st.st_mode & S_IFIFO)
+  } else if (st.st_mode & S_IFIFO) {
     return FN_FILE;
 #endif
-  else
+  } else {
     return FN_ERROR;
+  }
 }
 
 /** Check whether <b>dirname</b> exists and is private.  If yes return 0.  If

src/common/util.h

@@ -342,7 +342,7 @@ enum stream_status get_string_from_pipe(FILE *stream, char *buf, size_t count);
 
 /** Return values from file_status(); see that function's documentation
  * for details. */
-typedef enum { FN_ERROR, FN_NOENT, FN_FILE, FN_DIR } file_status_t;
+typedef enum { FN_ERROR, FN_NOENT, FN_FILE, FN_DIR, FN_EMPTY } file_status_t;
 file_status_t file_status(const char *filename);
 
 /** Possible behaviors for check_private_dir() on encountering a nonexistent

src/or/config.c

@@ -4161,17 +4161,24 @@ find_torrc_filename(config_line_t *cmd_arg,
   if (*using_default_fname) {
     /* didn't find one, try CONFDIR */
     const char *dflt = get_default_conf_file(defaults_file);
-    if (dflt && file_status(dflt) == FN_FILE) {
+    file_status_t st = file_status(dflt);
+    if (dflt && (st == FN_FILE || st == FN_EMPTY)) {
       fname = tor_strdup(dflt);
     } else {
 #ifndef _WIN32
       char *fn = NULL;
-      if (!defaults_file)
+      if (!defaults_file) {
         fn = expand_filename("~/.torrc");
-      if (fn && file_status(fn) == FN_FILE) {
-        fname = fn;
+      }
+      if (fn) {
+        file_status_t hmst = file_status(fn);
+        if (hmst == FN_FILE || hmst == FN_EMPTY) {
+          fname = fn;
+        } else {
+          tor_free(fn);
+          fname = tor_strdup(dflt);
+        }
       } else {
-        tor_free(fn);
         fname = tor_strdup(dflt);
       }
 #else
@@ -4207,7 +4214,8 @@ load_torrc_from_disk(config_line_t *cmd_arg, int defaults_file)
   *fname_var = fname;
 
   /* Open config file */
-  if (file_status(fname) != FN_FILE ||
+  file_status_t st = file_status(fname);
+  if (!(st == FN_FILE || st == FN_EMPTY) ||
       !(cf = read_file_to_str(fname,0,NULL))) {
     if (using_default_torrc == 1 || ignore_missing_torrc) {
       if (!defaults_file)
@@ -6466,7 +6474,9 @@ write_configuration_file(const char *fname, const or_options_t *options)
   tor_assert(fname);
 
   switch (file_status(fname)) {
+    /* create backups of old config files, even if they're empty */
     case FN_FILE:
+    case FN_EMPTY:
       old_val = read_file_to_str(fname, 0, NULL);
       if (!old_val || strcmpstart(old_val, GENERATED_FILE_PREFIX)) {
         rename_old = 1;

src/or/router.c

@@ -313,6 +313,7 @@ rotate_onion_key(void)
   time_t now;
   fname = get_datadir_fname2("keys", "secret_onion_key");
   fname_prev = get_datadir_fname2("keys", "secret_onion_key.old");
+  /* There isn't much point replacing an old key with an empty file */
   if (file_status(fname) == FN_FILE) {
     if (replace_file(fname, fname_prev))
       goto error;
@@ -335,6 +336,7 @@ rotate_onion_key(void)
   fname_prev = get_datadir_fname2("keys", "secret_onion_key_ntor.old");
   if (curve25519_keypair_generate(&new_curve25519_keypair, 1) < 0)
     goto error;
+  /* There isn't much point replacing an old key with an empty file */
   if (file_status(fname) == FN_FILE) {
     if (replace_file(fname, fname_prev))
       goto error;
@@ -411,7 +413,11 @@ init_key_from_file(const char *fname, int generate, int severity,
     case FN_ERROR:
       tor_log(severity, LD_FS,"Can't read key from \"%s\"", fname);
       goto error;
+    /* treat empty key files as if the file doesn't exist, and,
+     * if generate is set, replace the empty file in
+     * crypto_pk_write_private_key_to_filename() */
     case FN_NOENT:
+    case FN_EMPTY:
       if (generate) {
         if (!have_lockfile()) {
           if (try_locking(get_options(), 0)<0) {
@@ -464,10 +470,10 @@ init_key_from_file(const char *fname, int generate, int severity,
 }
 
 /** Load a curve25519 keypair from the file <b>fname</b>, writing it into
- * <b>keys_out</b>.  If the file isn't found and <b>generate</b> is true,
- * create a new keypair and write it into the file.  If there are errors, log
- * them at level <b>severity</b>. Generate files using <b>tag</b> in their
- * ASCII wrapper. */
+ * <b>keys_out</b>.  If the file isn't found, or is empty, and <b>generate</b>
+ * is true, create a new keypair and write it into the file.  If there are
+ * errors, log them at level <b>severity</b>. Generate files using <b>tag</b>
+ * in their ASCII wrapper. */
 static int
 init_curve25519_keypair_from_file(curve25519_keypair_t *keys_out,
                                   const char *fname,
@@ -480,7 +486,10 @@ init_curve25519_keypair_from_file(curve25519_keypair_t *keys_out,
     case FN_ERROR:
       tor_log(severity, LD_FS,"Can't read key from \"%s\"", fname);
       goto error;
+    /* treat empty key files as if the file doesn't exist, and, if generate
+     * is set, replace the empty file in curve25519_keypair_write_to_file() */
     case FN_NOENT:
+    case FN_EMPTY:
       if (generate) {
         if (!have_lockfile()) {
           if (try_locking(get_options(), 0)<0) {
@@ -880,7 +889,9 @@ init_keys(void)
 
   keydir = get_datadir_fname2("keys", "secret_onion_key.old");
   if (!lastonionkey && file_status(keydir) == FN_FILE) {
-    prkey = init_key_from_file(keydir, 1, LOG_ERR, 0); /* XXXX Why 1? */
+    /* Load keys from non-empty files only.
+     * Missing old keys won't be replaced with freshly generated keys. */
+    prkey = init_key_from_file(keydir, 0, LOG_ERR, 0);
     if (prkey)
       lastonionkey = prkey;
   }
@@ -901,6 +912,8 @@ init_keys(void)
                            last_curve25519_onion_key.pubkey.public_key,
                         CURVE25519_PUBKEY_LEN) &&
         file_status(keydir) == FN_FILE) {
+      /* Load keys from non-empty files only.
+       * Missing old keys won't be replaced with freshly generated keys. */
       init_curve25519_keypair_from_file(&last_curve25519_onion_key,
                                         keydir, 0, LOG_ERR, "onion");
     }
@@ -2577,8 +2590,9 @@ router_has_orport(const routerinfo_t *router, const tor_addr_port_t *orport)
  * <b>end_line</b>, ensure that its timestamp is not more than 25 hours in
  * the past or more than 1 hour in the future with respect to <b>now</b>,
  * and write the file contents starting with that line to *<b>out</b>.
- * Return 1 for success, 0 if the file does not exist, or -1 if the file
- * does not contain a line matching these criteria or other failure. */
+ * Return 1 for success, 0 if the file does not exist or is empty, or -1
+ * if the file does not contain a line matching these criteria or other
+ * failure. */
 static int
 load_stats_file(const char *filename, const char *end_line, time_t now,
                 char **out)
@@ -2612,7 +2626,9 @@ load_stats_file(const char *filename, const char *end_line, time_t now,
      notfound:
       tor_free(contents);
       break;
+    /* treat empty stats files as if the file doesn't exist */
     case FN_NOENT:
+    case FN_EMPTY:
       r = 0;
       break;
     case FN_ERROR:

src/or/routerlist.c

@@ -1206,6 +1206,7 @@ router_reload_router_list_impl(desc_store_t *store)
 
   tor_free(fname);
   fname = get_datadir_fname_suffix(store->fname_base, ".new");
+  /* don't load empty files - we wouldn't get any data, even if we tried */
   if (file_status(fname) == FN_FILE)
     contents = read_file_to_str(fname, RFTS_BIN|RFTS_IGNORE_MISSING, &st);
   if (contents) {

src/or/statefile.c

@@ -323,7 +323,10 @@ or_state_load(void)
         goto done;
       }
       break;
+    /* treat empty state files as if the file doesn't exist, and generate
+     * a new state file, overwriting the empty file in or_state_save() */
     case FN_NOENT:
+    case FN_EMPTY:
       break;
     case FN_ERROR:
     case FN_DIR:

src/test/include.am

@@ -122,9 +122,11 @@ if USEPYTHON
 	./src/test/test-bt-cl assert | $(PYTHON) $(top_srcdir)/src/test/bt_test.py
 	./src/test/test-bt-cl crash | $(PYTHON) $(top_srcdir)/src/test/bt_test.py
 endif
+	./src/test/zero_length_keys.sh
 
 EXTRA_DIST += \
 	src/test/bt_test.py \
 	src/test/ntor_ref.py \
 	src/test/slownacl_curve25519.py \
-	src/test/test_cmdline_args.py
+	src/test/test_cmdline_args.py \
+	src/test/zero_length_keys.sh

src/test/zero_length_keys.sh

@@ -0,0 +1,113 @@
+#!/bin/sh
+# Check that tor regenerates keys when key files are zero-length
+# Test for bug #13111 - Tor fails to start if onion keys are zero length
+#
+# Usage:
+#  ./zero_length_keys.sh
+#    Run all the tests below
+#  ./zero_length_keys.sh -z
+#    Check tor will launch and regenerate zero-length keys
+#  ./zero_length_keys.sh -d
+#    Check tor regenerates deleted keys (existing behaviour)
+#  ./zero_length_keys.sh -e
+#    Check tor does not overwrite existing keys (existing behaviour)
+#
+# Exit Statuses:
+#  -2: test failed - tor did not generate the key files on first run
+#  -1: a command failed - the test could not be completed
+#   0: test succeeded - tor regenerated/kept the files
+#   1: test failed - tor did not regenerate/keep the files
+#
+
+if [ $# -lt 1 ]; then
+  echo "Testing that tor correctly handles zero-length keys"
+  "$0" -z && "$0" -d && "$0" -e
+  exit $?
+fi
+
+export DATA_DIR=`mktemp -d -t tor_zero_length_keys`
+TOR="src/or/tor --hush --DisableNetwork 1 --ShutdownWaitLength 0 --ORPort 12345"
+
+if [ -s "$DATA_DIR"/keys/secret_id_key -a -s "$DATA_DIR"/keys/secret_onion_key -a -s "$DATA_DIR"/keys/secret_onion_key_ntor ]; then
+  echo "Failure: Previous tor keys present in tor data directory"
+  exit -1
+else
+  echo "Generating initial tor keys"
+  $TOR --DataDirectory "$DATA_DIR" --PidFile "$DATA_DIR"/pid &
+  TOR_PID=$!
+  # generate SIGTERM, hopefully after the keys have been regenerated
+  sleep 5
+  kill $TOR_PID
+  wait $TOR_PID
+
+  # tor must successfully generate non-zero-length key files
+  if [ -s "$DATA_DIR"/keys/secret_id_key -a -s "$DATA_DIR"/keys/secret_onion_key -a -s "$DATA_DIR"/keys/secret_onion_key_ntor ]; then
+    true #echo "tor generated the initial key files"
+  else
+    echo "Failure: tor failed to generate the initial key files"
+    exit -2
+  fi
+fi
+
+#ls -lh  "$DATA_DIR"/keys/ || exit -1
+
+# backup and keep/delete/create zero-length files for the keys
+
+FILE_DESC="keeps existing"
+# make a backup
+cp -r "$DATA_DIR"/keys "$DATA_DIR"/keys.old
+
+# delete keys for -d or -z
+if [ "$1" != "-e" ]; then
+  FILE_DESC="regenerates deleted"
+  rm "$DATA_DIR"/keys/secret_id_key || exit -1
+  rm "$DATA_DIR"/keys/secret_onion_key || exit -1
+  rm "$DATA_DIR"/keys/secret_onion_key_ntor || exit -1
+fi
+
+# create empty files for -z
+if [ "$1" == "-z" ]; then
+  FILE_DESC="regenerates zero-length"
+  touch "$DATA_DIR"/keys/secret_id_key || exit -1
+  touch "$DATA_DIR"/keys/secret_onion_key || exit -1
+  touch "$DATA_DIR"/keys/secret_onion_key_ntor || exit -1
+fi
+
+echo "Running tor again to check if it $FILE_DESC keys"
+$TOR --DataDirectory "$DATA_DIR" --PidFile "$DATA_DIR"/pid &
+TOR_PID=$!
+# generate SIGTERM, hopefully after the keys have been regenerated
+sleep 5
+kill $TOR_PID
+wait $TOR_PID
+
+#ls -lh "$DATA_DIR"/keys/ || exit -1
+
+# tor must always have non-zero-length key files
+if [ -s "$DATA_DIR"/keys/secret_id_key -a -s "$DATA_DIR"/keys/secret_onion_key -a -s "$DATA_DIR"/keys/secret_onion_key_ntor ]; then
+  # check if the keys are different to the old ones
+  diff -q -r "$DATA_DIR"/keys "$DATA_DIR"/keys.old > /dev/null
+  SAME_KEYS=$?
+  # if we're not testing existing keys,
+  # the current keys should be different to the old ones
+  if [ "$1" != "-e" ]; then
+    if [ $SAME_KEYS -ne 0 ]; then
+      echo "Success: test that tor $FILE_DESC key files: different keys"
+      exit 0
+    else
+      echo "Failure: test that tor $FILE_DESC key files: same keys"
+      exit 1
+    fi
+  else #[ "$1" == "-e" ]; then
+    if [ $SAME_KEYS -eq 0 ]; then
+      echo "Success: test that tor $FILE_DESC key files: same keys"
+      exit 0
+    else
+      echo "Failure: test that tor $FILE_DESC key files: different keys"
+      exit 1
+    fi
+  fi
+else
+  echo "Failure: test that tor $FILE_DESC key files: no key files"
+  exit 1
+fi