Merge branch 'maint-0.2.9' into release-0.2.9

8 files changed, 135 insertions, 55 deletions

diff --git a/changes/bug20484 b/changes/bug20484
new file mode 100644
index 0000000000..9a0b95cb39
--- /dev/null
+++ b/changes/bug20484
@@ -0,0 +1,5 @@
+  o Minor bugfixes (single onion services):
+    - Start correctly when creating a single onion service in a
+      directory that did not previously exist. Fixes bug 20484; bugfix on
+      0.2.9.3-alpha.
+
diff --git a/changes/bug20529 b/changes/bug20529
new file mode 100644
index 0000000000..276be5b2b6
--- /dev/null
+++ b/changes/bug20529
@@ -0,0 +1,4 @@
+  o Minor bugfixes (hidden services):
+    - When configuring hidden services, check every hidden service directory's
+      permissions. Previously, we only checked the last hidden service.
+      Fixes bug 20529; bugfix on 13942 commit 85bfad1 in 0.2.6.2-alpha.
diff --git a/doc/tor.1.txt b/doc/tor.1.txt
index b502e447d0..e279460f83 100644
--- a/doc/tor.1.txt
+++ b/doc/tor.1.txt
@@ -2898,6 +2898,10 @@ __HiddenServiceDirectory__**/client_keys**::
     Authorization data for a hidden service that is only accessible by
     authorized clients.
 
+__HiddenServiceDirectory__**/onion_service_non_anonymous**::
+    This file is present if a hidden service key was created in
+    **HiddenServiceNonAnonymousMode**.
+
 SEE ALSO
 --------
 **torsocks**(1), **torify**(1) +
diff --git a/src/common/util.c b/src/common/util.c
index 9162967907..a7bce2ea6c 100644
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -2191,11 +2191,13 @@ file_status(const char *fname)
   }
 }
 
-/** Check whether <b>dirname</b> exists and is private.  If yes return 0.  If
- * it does not exist, and <b>check</b>&CPD_CREATE is set, try to create it
- * and return 0 on success. If it does not exist, and
- * <b>check</b>&CPD_CHECK, and we think we can create it, return 0.  Else
- * return -1.  If CPD_GROUP_OK is set, then it's okay if the directory
+/** Check whether <b>dirname</b> exists and is private.  If yes return 0.
+ * If <b>dirname</b> does not exist:
+ *  - if <b>check</b>&CPD_CREATE, try to create it and return 0 on success.
+ *  - if <b>check</b>&CPD_CHECK, and we think we can create it, return 0.
+ *  - if <b>check</b>&CPD_CHECK is false, and the directory exists, return 0.
+ *  - otherwise, return -1.
+ * If CPD_GROUP_OK is set, then it's okay if the directory
  * is group-readable, but in all cases we create the directory mode 0700.
  * If CPD_GROUP_READ is set, existing directory behaves as CPD_GROUP_OK and
  * if the directory is created it will use mode 0750 with group read
diff --git a/src/or/rendservice.c b/src/or/rendservice.c
index 3afe880554..56dbacdaf7 100644
--- a/src/or/rendservice.c
+++ b/src/or/rendservice.c
@@ -72,6 +72,10 @@ static ssize_t rend_service_parse_intro_for_v3(
     size_t plaintext_len,
     char **err_msg_out);
 
+static int rend_service_check_private_dir(const or_options_t *options,
+                                          const rend_service_t *s,
+                                          int create);
+
 /** Represents the mapping from a virtual port of a rendezvous service to
  * a real port on some IP.
  */
@@ -462,6 +466,11 @@ rend_config_services(const or_options_t *options, int validate_only)
   for (line = options->RendConfigLines; line; line = line->next) {
     if (!strcasecmp(line->key, "HiddenServiceDir")) {
       if (service) { /* register the one we just finished parsing */
+        if (rend_service_check_private_dir(options, service, 0) < 0) {
+          rend_service_free(service);
+          return -1;
+        }
+
         if (validate_only)
           rend_service_free(service);
         else
@@ -672,12 +681,7 @@ rend_config_services(const or_options_t *options, int validate_only)
     }
   }
   if (service) {
-    cpd_check_t check_opts = CPD_CHECK_MODE_ONLY|CPD_CHECK;
-    if (service->dir_group_readable) {
-      check_opts |= CPD_GROUP_READ;
-    }
-
-    if (check_private_dir(service->directory, check_opts, options->User) < 0) {
+    if (rend_service_check_private_dir(options, service, 0) < 0) {
       rend_service_free(service);
       return -1;
     }
@@ -1001,7 +1005,9 @@ service_is_single_onion_poisoned(const rend_service_t *service)
   fstatus = file_status(poison_fname);
   tor_free(poison_fname);
 
-  /* If this fname is occupied, the hidden service has been poisoned. */
+  /* If this fname is occupied, the hidden service has been poisoned.
+   * fstatus can be FN_ERROR if the service directory does not exist, in that
+   * case, there is obviously no private key. */
   if (fstatus == FN_FILE || fstatus == FN_EMPTY) {
     return 1;
   }
@@ -1017,7 +1023,9 @@ rend_service_private_key_exists(const rend_service_t *service)
   char *private_key_path = rend_service_path(service, private_key_fname);
   const file_status_t private_key_status = file_status(private_key_path);
   tor_free(private_key_path);
-  /* Only non-empty regular private key files could have been used before. */
+  /* Only non-empty regular private key files could have been used before.
+   * fstatus can be FN_ERROR if the service directory does not exist, in that
+   * case, there is obviously no private key. */
   return private_key_status == FN_FILE;
 }
 
@@ -1090,6 +1098,10 @@ poison_new_single_onion_hidden_service_dir(const rend_service_t *service)
     return -1;
   }
 
+  /* Make sure the directory exists */
+  if (rend_service_check_private_dir(get_options(), service, 1) < 0)
+    return -1;
+
   poison_fname = rend_service_sos_poison_path(service);
 
   switch (file_status(poison_fname)) {
@@ -1245,6 +1257,37 @@ rend_service_derive_key_digests(struct rend_service_t *s)
   return 0;
 }
 
+/** Make sure that the directory for <b>s</b> is private, using the config in
+ * <b>options</b>.
+ * If <b>create</b> is true:
+ *  - if the directory exists, change permissions if needed,
+ *  - if the directory does not exist, create it with the correct permissions.
+ * If <b>create</b> is false:
+ *  - if the directory exists, check permissions,
+ *  - if the directory does not exist, check if we think we can create it.
+ * Return 0 on success, -1 on failure. */
+static int
+rend_service_check_private_dir(const or_options_t *options,
+                               const rend_service_t *s,
+                               int create)
+{
+  cpd_check_t  check_opts = CPD_NONE;
+  if (create) {
+    check_opts |= CPD_CREATE;
+  } else {
+    check_opts |= CPD_CHECK_MODE_ONLY;
+    check_opts |= CPD_CHECK;
+  }
+  if (s->dir_group_readable) {
+    check_opts |= CPD_GROUP_READ;
+  }
+  /* Check/create directory */
+  if (check_private_dir(s->directory, check_opts, options->User) < 0) {
+    return -1;
+  }
+  return 0;
+}
+
 /** Load and/or generate private keys for the hidden service <b>s</b>,
  * possibly including keys for client authorization.  Return 0 on success, -1
  * on failure. */
@@ -1253,23 +1296,9 @@ rend_service_load_keys(rend_service_t *s)
 {
   char *fname = NULL;
   char buf[128];
-  cpd_check_t  check_opts = CPD_CREATE;
 
-  if (s->dir_group_readable) {
-    check_opts |= CPD_GROUP_READ;
-  }
-  /* Check/create directory */
-  if (check_private_dir(s->directory, check_opts, get_options()->User) < 0) {
+  if (rend_service_check_private_dir(get_options(), s, 1) < 0)
     goto err;
-  }
-#ifndef _WIN32
-  if (s->dir_group_readable) {
-    /* Only new dirs created get new opts, also enforce group read. */
-    if (chmod(s->directory, 0750)) {
-      log_warn(LD_FS,"Unable to make %s group-readable.", s->directory);
-    }
-  }
-#endif
 
   /* Load key */
   fname = rend_service_path(s, private_key_fname);
diff --git a/src/test/test.h b/src/test/test.h
index 770f403cee..25336ac83e 100644
--- a/src/test/test.h
+++ b/src/test/test.h
@@ -73,6 +73,7 @@
     {print_ = (I64_PRINTF_TYPE) value_;}, {}, TT_EXIT_TEST_FUNCTION)
 
 const char *get_fname(const char *name);
+const char *get_fname_rnd(const char *name);
 struct crypto_pk_t *pk_generate(int idx);
 
 #define US2_CONCAT_2__(a, b) a ## __ ## b
diff --git a/src/test/test_hs.c b/src/test/test_hs.c
index fd5ab15643..b42bc590c0 100644
--- a/src/test/test_hs.c
+++ b/src/test/test_hs.c
@@ -518,6 +518,11 @@ mock_get_options(void)
   return mock_options;
 }
 
+/* arg can't be 0 (the test fails) or 2 (the test is skipped) */
+#define CREATE_HS_DIR_NONE ((intptr_t)0x04)
+#define CREATE_HS_DIR1     ((intptr_t)0x08)
+#define CREATE_HS_DIR2     ((intptr_t)0x10)
+
 /* Test that single onion poisoning works. */
 static void
 test_single_onion_poisoning(void *arg)
@@ -528,15 +533,15 @@ test_single_onion_poisoning(void *arg)
   MOCK(get_options, mock_get_options);
 
   int ret = -1;
-  mock_options->DataDirectory = tor_strdup(get_fname("test_data_dir"));
+  intptr_t create_dir_mask = (intptr_t)arg;
+  /* Get directories with a random suffix so we can repeat the tests */
+  mock_options->DataDirectory = tor_strdup(get_fname_rnd("test_data_dir"));
   rend_service_t *service_1 = tor_malloc_zero(sizeof(rend_service_t));
-  char *dir1 = tor_strdup(get_fname("test_hs_dir1"));
+  char *dir1 = tor_strdup(get_fname_rnd("test_hs_dir1"));
   rend_service_t *service_2 = tor_malloc_zero(sizeof(rend_service_t));
-  char *dir2 = tor_strdup(get_fname("test_hs_dir2"));
+  char *dir2 = tor_strdup(get_fname_rnd("test_hs_dir2"));
   smartlist_t *services = smartlist_new();
 
-  (void) arg;
-
   /* No services, no problem! */
   mock_options->HiddenServiceSingleHopMode = 0;
   mock_options->HiddenServiceNonAnonymousMode = 0;
@@ -549,22 +554,20 @@ test_single_onion_poisoning(void *arg)
   ret = rend_service_list_verify_single_onion_poison(services, mock_options);
   tt_assert(ret == 0);
 
-  /* Create directories for both services */
-
-#ifdef _WIN32
-  ret = mkdir(mock_options->DataDirectory);
-  tt_assert(ret == 0);
-  ret = mkdir(dir1);
-  tt_assert(ret == 0);
-  ret = mkdir(dir2);
-#else
-  ret = mkdir(mock_options->DataDirectory, 0700);
-  tt_assert(ret == 0);
-  ret = mkdir(dir1, 0700);
-  tt_assert(ret == 0);
-  ret = mkdir(dir2, 0700);
-#endif
+  /* Create the data directory, and, if the correct bit in arg is set,
+   * create a directory for that service.
+   * The data directory is required for the lockfile, which is used when
+   * loading keys. */
+  ret = check_private_dir(mock_options->DataDirectory, CPD_CREATE, NULL);
   tt_assert(ret == 0);
+  if (create_dir_mask & CREATE_HS_DIR1) {
+    ret = check_private_dir(dir1, CPD_CREATE, NULL);
+    tt_assert(ret == 0);
+  }
+  if (create_dir_mask & CREATE_HS_DIR2) {
+    ret = check_private_dir(dir2, CPD_CREATE, NULL);
+    tt_assert(ret == 0);
+  }
 
   service_1->directory = dir1;
   service_2->directory = dir2;
@@ -694,7 +697,7 @@ test_single_onion_poisoning(void *arg)
   tt_assert(ret < 0);
 
  done:
-  /* TODO: should we delete the directories here? */
+  /* The test harness deletes the directories at exit */
   rend_service_free(service_1);
   rend_service_free(service_2);
   smartlist_free(services);
@@ -716,8 +719,14 @@ struct testcase_t hs_tests[] = {
     NULL, NULL },
   { "hs_auth_cookies", test_hs_auth_cookies, TT_FORK,
     NULL, NULL },
-  { "single_onion_poisoning", test_single_onion_poisoning, TT_FORK,
-    NULL, NULL },
+  { "single_onion_poisoning_create_dir_none", test_single_onion_poisoning,
+    TT_FORK, &passthrough_setup, (void*)(CREATE_HS_DIR_NONE) },
+  { "single_onion_poisoning_create_dir1", test_single_onion_poisoning,
+    TT_FORK, &passthrough_setup, (void*)(CREATE_HS_DIR1) },
+  { "single_onion_poisoning_create_dir2", test_single_onion_poisoning,
+    TT_FORK, &passthrough_setup, (void*)(CREATE_HS_DIR2) },
+  { "single_onion_poisoning_create_dir_both", test_single_onion_poisoning,
+    TT_FORK, &passthrough_setup, (void*)(CREATE_HS_DIR1 | CREATE_HS_DIR2) },
   END_OF_TESTCASES
 };
 
diff --git a/src/test/testing_common.c b/src/test/testing_common.c
index fd7c4e7074..9c6580f788 100644
--- a/src/test/testing_common.c
+++ b/src/test/testing_common.c
@@ -102,18 +102,41 @@ setup_directory(void)
   temp_dir_setup_in_pid = getpid();
 }
 
-/** Return a filename relative to our testing temporary directory */
-const char *
-get_fname(const char *name)
+/** Return a filename relative to our testing temporary directory, based on
+ * name and suffix. If name is NULL, return the name of the testing temporary
+ * directory. */
+static const char *
+get_fname_suffix(const char *name, const char *suffix)
 {
   static char buf[1024];
   setup_directory();
   if (!name)
     return temp_dir;
-  tor_snprintf(buf,sizeof(buf),"%s/%s",temp_dir,name);
+  tor_snprintf(buf,sizeof(buf),"%s/%s%s%s",temp_dir,name,suffix ? "_" : "",
+               suffix ? suffix : "");
   return buf;
 }
 
+/** Return a filename relative to our testing temporary directory. If name is
+ * NULL, return the name of the testing temporary directory. */
+const char *
+get_fname(const char *name)
+{
+  return get_fname_suffix(name, NULL);
+}
+
+/** Return a filename with a random suffix, relative to our testing temporary
+ * directory. If name is NULL, return the name of the testing temporary
+ * directory, without any suffix. */
+const char *
+get_fname_rnd(const char *name)
+{
+  char rnd[256], rnd32[256];
+  crypto_rand(rnd, RAND_PATH_BYTES);
+  base32_encode(rnd32, sizeof(rnd32), rnd, RAND_PATH_BYTES);
+  return get_fname_suffix(name, rnd32);
+}
+
 /* Remove a directory and all of its subdirectories */
 static void
 rm_rf(const char *dir)
@@ -217,6 +240,9 @@ free_pregenerated_keys(void)
 static void *
 passthrough_test_setup(const struct testcase_t *testcase)
 {
+  /* Make sure the passthrough doesn't unintentionally fail or skip tests */
+  tor_assert(testcase->setup_data);
+  tor_assert(testcase->setup_data != (void*)TT_SKIP);
   return testcase->setup_data;
 }
 static int