Merge remote-tracking branch 'meejah/ticket-11291-extra-utests'

Conflicts:
	src/or/config.c

doc/tor.1.txt

@@ -2066,6 +2066,12 @@ The following options are used to configure a hidden service.
     service descriptors to the directory servers. This information  is also
     uploaded whenever it changes. (Default: 1 hour)
 
+[[HiddenServiceDirGroupReadable]] **HiddenServiceDirGroupReadable** **0**|**1**::
+    If this option is set to 1, allow the filesystem group to read the
+    hidden service directory and hostname file. If the option is set to 0,
+    only owner is able to read the hidden service directory. (Default: 0)
+    Has no effect on Windows.
+
 TESTING NETWORK OPTIONS
 -----------------------
 

src/common/util.c

@@ -1995,8 +1995,12 @@ file_status(const char *fname)
  * <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
  * is group-readable, but in all cases we create the directory mode 0700.
- * If CPD_CHECK_MODE_ONLY is set, then we don't alter the directory permissions
- * if they are too permissive: we just return -1.
+ * 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
+ * permission. Group read privileges also assume execute permission
+ * as norm for directories. If CPD_CHECK_MODE_ONLY is set, then we don't
+ * alter the directory permissions if they are too permissive:
+ * we just return -1.
  * When effective_user is not NULL, check permissions against the given user
  * and its primary group.
  */
@@ -2008,7 +2012,8 @@ check_private_dir(const char *dirname, cpd_check_t check,
   struct stat st;
   char *f;
 #ifndef _WIN32
-  int mask;
+  int mask = 0;
+  int perm = 0;
   const struct passwd *pw = NULL;
   uid_t running_uid;
   gid_t running_gid;
@@ -2033,7 +2038,11 @@ check_private_dir(const char *dirname, cpd_check_t check,
 #if defined (_WIN32)
       r = mkdir(dirname);
 #else
-      r = mkdir(dirname, 0700);
+      if (check & CPD_GROUP_READ) {
+        r = mkdir(dirname, 0750);
+      } else {
+        r = mkdir(dirname, 0700);
+      }
 #endif
       if (r) {
         log_warn(LD_FS, "Error creating directory %s: %s", dirname,
@@ -2086,7 +2095,8 @@ check_private_dir(const char *dirname, cpd_check_t check,
     tor_free(process_ownername);
     return -1;
   }
-  if ((check & CPD_GROUP_OK) && st.st_gid != running_gid) {
+  if ( (check & (CPD_GROUP_OK|CPD_GROUP_READ))
+       && (st.st_gid != running_gid) ) {
     struct group *gr;
     char *process_groupname = NULL;
     gr = getgrgid(running_gid);
@@ -2101,7 +2111,7 @@ check_private_dir(const char *dirname, cpd_check_t check,
     tor_free(process_groupname);
     return -1;
   }
-  if (check & CPD_GROUP_OK) {
+  if (check & (CPD_GROUP_OK|CPD_GROUP_READ)) {
     mask = 0027;
   } else {
     mask = 0077;
@@ -2116,10 +2126,13 @@ check_private_dir(const char *dirname, cpd_check_t check,
     log_warn(LD_FS, "Fixing permissions on directory %s", dirname);
     new_mode = st.st_mode;
     new_mode |= 0700; /* Owner should have rwx */
+    if (check & CPD_GROUP_READ) {
+      new_mode |= 0050; /* Group should have rx */
+    }
     new_mode &= ~mask; /* Clear the other bits that we didn't want set...*/
     if (chmod(dirname, new_mode)) {
       log_warn(LD_FS, "Could not chmod directory %s: %s", dirname,
-          strerror(errno));
+               strerror(errno));
       return -1;
     } else {
       return 0;

src/common/util.h

@@ -347,9 +347,11 @@ typedef unsigned int cpd_check_t;
 #define CPD_CREATE 1
 #define CPD_CHECK 2
 #define CPD_GROUP_OK 4
-#define CPD_CHECK_MODE_ONLY 8
+#define CPD_GROUP_READ 8
+#define CPD_CHECK_MODE_ONLY 16
 int check_private_dir(const char *dirname, cpd_check_t check,
                       const char *effective_user);
+
 #define OPEN_FLAGS_REPLACE (O_WRONLY|O_CREAT|O_TRUNC)
 #define OPEN_FLAGS_APPEND (O_WRONLY|O_CREAT|O_APPEND)
 #define OPEN_FLAGS_DONT_REPLACE (O_CREAT|O_EXCL|O_APPEND|O_WRONLY)

src/or/config.c

@@ -262,6 +262,7 @@ static config_var_t option_vars_[] = {
   V(HashedControlPassword,       LINELIST, NULL),
   V(HidServDirectoryV2,          BOOL,     "1"),
   VAR("HiddenServiceDir",    LINELIST_S, RendConfigLines,    NULL),
+  VAR("HiddenServiceDirGroupReadable",  LINELIST_S, RendConfigLines, NULL),
   VAR("HiddenServiceOptions",LINELIST_V, RendConfigLines,    NULL),
   VAR("HiddenServicePort",   LINELIST_S, RendConfigLines,    NULL),
   VAR("HiddenServiceVersion",LINELIST_S, RendConfigLines,    NULL),

src/or/or.h

@@ -4227,6 +4227,7 @@ typedef struct {
 
   /** Should we send the timestamps that pre-023 hidden services want? */
   int Support022HiddenServices;
+
 } or_options_t;
 
 /** Persistent state for an onion router, as saved to disk. */

src/or/rendservice.c

@@ -95,6 +95,8 @@ typedef struct rend_service_port_config_t {
 typedef struct rend_service_t {
   /* Fields specified in config file */
   char *directory; /**< where in the filesystem it stores it */
+  int dir_group_readable; /**< if 1, allow group read
+                             permissions on directory */
   smartlist_t *ports; /**< List of rend_service_port_config_t */
   rend_auth_type_t auth_type; /**< Client authorization type or 0 if no client
                                * authorization is performed. */
@@ -359,6 +361,7 @@ rend_config_services(const or_options_t *options, int validate_only)
   rend_service_t *service = NULL;
   rend_service_port_config_t *portcfg;
   smartlist_t *old_service_list = NULL;
+  int ok = 0;
 
   if (!validate_only) {
     old_service_list = rend_service_list;
@@ -393,6 +396,20 @@ rend_config_services(const or_options_t *options, int validate_only)
         return -1;
       }
       smartlist_add(service->ports, portcfg);
+    } else if (!strcasecmp(line->key,
+                           "HiddenServiceDirGroupReadable")) {
+        service->dir_group_readable = (int)tor_parse_long(line->value,
+                                                          10, 0, 1, &ok, NULL);
+        if (!ok) {
+            log_warn(LD_CONFIG,
+                     "HiddenServiceDirGroupReadable should be 0 or 1, not %s",
+                     line->value);
+            rend_service_free(service);
+            return -1;
+        }
+        log_info(LD_CONFIG,
+                 "HiddenServiceDirGroupReadable=%d for %s",
+                 service->dir_group_readable, service->directory);
     } else if (!strcasecmp(line->key, "HiddenServiceAuthorizeClient")) {
       /* Parse auth type and comma-separated list of client names and add a
        * rend_authorized_client_t for each client to the service's list
@@ -513,10 +530,11 @@ rend_config_services(const or_options_t *options, int validate_only)
     }
   }
   if (service) {
-    if (validate_only)
+    if (validate_only) {
       rend_service_free(service);
-    else
+    } else {
       rend_add_service(service);
+    }
   }
 
   /* If this is a reload and there were hidden services configured before,
@@ -693,10 +711,23 @@ rend_service_load_keys(rend_service_t *s)
 {
   char fname[512];
   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, CPD_CREATE, get_options()->User) < 0)
+  if (check_private_dir(s->directory, check_opts, get_options()->User) < 0) {
     return -1;
+  }
+#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 */
   if (strlcpy(fname,s->directory,sizeof(fname)) >= sizeof(fname) ||
@@ -733,6 +764,15 @@ rend_service_load_keys(rend_service_t *s)
     memwipe(buf, 0, sizeof(buf));
     return -1;
   }
+#ifndef _WIN32
+  if (s->dir_group_readable) {
+    /* Also verify hostname file created with group read. */
+    if (chmod(fname, 0640))
+      log_warn(LD_FS,"Unable to make hidden hostname file %s group-readable.",
+               fname);
+  }
+#endif
+
   memwipe(buf, 0, sizeof(buf));
 
   /* If client authorization is configured, load or generate keys. */

src/test/Makefile.nmake

@@ -12,8 +12,8 @@ LIBS = ..\..\..\build-alpha\lib\libevent.lib \
  crypt32.lib gdi32.lib user32.lib
 
 TEST_OBJECTS = test.obj test_addr.obj test_containers.obj \
-	test_controller_events.ogj test_crypto.obj test_data.obj test_dir.obj \
-	test_microdesc.obj test_pt.obj test_util.obj test_config.obj \
+	test_controller_events.obj test_crypto.obj test_data.obj test_dir.obj \
+	test_checkdir.obj test_microdesc.obj test_pt.obj test_util.obj test_config.obj \
 	test_cell_formats.obj test_replay.obj test_introduce.obj tinytest.obj \
 	test_hs.obj
 

src/test/include.am

@@ -28,6 +28,7 @@ src_test_test_SOURCES = \
 	src/test/test_cell_queue.c \
 	src/test/test_data.c \
 	src/test/test_dir.c \
+	src/test/test_checkdir.c \
 	src/test/test_entrynodes.c \
 	src/test/test_extorport.c \
 	src/test/test_introduce.c \

src/test/test.c

@@ -1279,6 +1279,7 @@ extern struct testcase_t crypto_tests[];
 extern struct testcase_t container_tests[];
 extern struct testcase_t util_tests[];
 extern struct testcase_t dir_tests[];
+extern struct testcase_t checkdir_tests[];
 extern struct testcase_t microdesc_tests[];
 extern struct testcase_t pt_tests[];
 extern struct testcase_t config_tests[];
@@ -1316,6 +1317,7 @@ static struct testgroup_t testgroups[] = {
   { "cellfmt/", cell_format_tests },
   { "cellqueue/", cell_queue_tests },
   { "dir/", dir_tests },
+  { "checkdir/", checkdir_tests },
   { "dir/md/", microdesc_tests },
   { "pt/", pt_tests },
   { "config/", config_tests },

src/test/test_checkdir.c

@@ -0,0 +1,132 @@
+/* Copyright (c) 2014, The Tor Project, Inc. */
+/* See LICENSE for licensing information */
+
+#include "orconfig.h"
+#include "or.h"
+#include <dirent.h>
+#include "config.h"
+#include "test.h"
+#include "util.h"
+
+/** Run unit tests for private dir permission enforcement logic. */
+static void
+test_checkdir_perms(void *testdata)
+{
+  or_options_t *options = get_options_mutable();
+  const char *subdir = "test_checkdir";
+  char *testdir;
+  cpd_check_t  cpd_chkopts;
+  cpd_check_t  unix_create_opts;
+  cpd_check_t  unix_verify_optsmask;
+  struct stat st;
+
+  /* setup data directory before tests. */
+  tor_free(options->DataDirectory);
+  options->DataDirectory = tor_strdup(get_fname(subdir));
+  tt_int_op(mkdir(options->DataDirectory, 0750), ==, 0);
+
+  /* test: create new dir, no flags. */
+  testdir = get_datadir_fname("checkdir_new_none");
+  cpd_chkopts = CPD_CREATE;
+  unix_verify_optsmask = 0077;
+  tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
+  tt_int_op(0, ==, stat(testdir, &st));
+  tt_int_op(0, ==, (st.st_mode & unix_verify_optsmask));
+  tor_free(testdir);
+
+  /* test: create new dir, CPD_GROUP_OK option set. */
+  testdir = get_datadir_fname("checkdir_new_groupok");
+  cpd_chkopts = CPD_CREATE|CPD_GROUP_OK;
+  unix_verify_optsmask = 0077;
+  tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
+  tt_int_op(0, ==, stat(testdir, &st));
+  tt_int_op(0, ==, (st.st_mode & unix_verify_optsmask));
+  tor_free(testdir);
+
+  /* test: should get an error on existing dir with
+           wrong perms */
+  testdir = get_datadir_fname("checkdir_new_groupok_err");
+  tt_int_op(0, ==, mkdir(testdir, 027));
+  cpd_chkopts = CPD_CHECK_MODE_ONLY|CPD_CREATE|CPD_GROUP_OK;
+  tt_int_op(-1, ==, check_private_dir(testdir, cpd_chkopts, NULL));
+  tor_free(testdir);
+
+  /* test: create new dir, CPD_GROUP_READ option set. */
+  testdir = get_datadir_fname("checkdir_new_groupread");
+  cpd_chkopts = CPD_CREATE|CPD_GROUP_READ;
+  unix_verify_optsmask = 0027;
+  tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
+  tt_int_op(0, ==, stat(testdir, &st));
+  tt_int_op(0, ==, (st.st_mode & unix_verify_optsmask));
+  tor_free(testdir);
+
+  /* test: check existing dir created with defaults,
+            and verify with CPD_CREATE only. */
+  testdir = get_datadir_fname("checkdir_exists_none");
+  cpd_chkopts = CPD_CREATE;
+  unix_create_opts = 0700;
+  unix_verify_optsmask = 0077;
+  tt_int_op(0, ==, mkdir(testdir, unix_create_opts));
+  tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
+  tt_int_op(0, ==, stat(testdir, &st));
+  tt_int_op(0, ==, (st.st_mode & unix_verify_optsmask));
+  tor_free(testdir);
+
+  /* test: check existing dir created with defaults,
+            and verify with CPD_GROUP_OK option set. */
+  testdir = get_datadir_fname("checkdir_exists_groupok");
+  cpd_chkopts = CPD_CREATE;
+  unix_verify_optsmask = 0077;
+  tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
+  cpd_chkopts = CPD_GROUP_OK;
+  tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
+  tt_int_op(0, ==, stat(testdir, &st));
+  tt_int_op(0, ==, (st.st_mode & unix_verify_optsmask));
+  tor_free(testdir);
+
+  /* test: check existing dir created with defaults,
+            and verify with CPD_GROUP_READ option set. */
+  testdir = get_datadir_fname("checkdir_exists_groupread");
+  cpd_chkopts = CPD_CREATE;
+  unix_verify_optsmask = 0027;
+  tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
+  cpd_chkopts = CPD_GROUP_READ;
+  tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
+  tt_int_op(0, ==, stat(testdir, &st));
+  tt_int_op(0, ==, (st.st_mode & unix_verify_optsmask));
+  tor_free(testdir);
+
+  /* test: check existing dir created with CPD_GROUP_READ,
+            and verify with CPD_GROUP_OK option set. */
+  testdir = get_datadir_fname("checkdir_existsread_groupok");
+  cpd_chkopts = CPD_CREATE|CPD_GROUP_READ;
+  unix_verify_optsmask = 0027;
+  tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
+  cpd_chkopts = CPD_GROUP_OK;
+  tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
+  tt_int_op(0, ==, stat(testdir, &st));
+  tt_int_op(0, ==, (st.st_mode & unix_verify_optsmask));
+  tor_free(testdir);
+
+  /* test: check existing dir created with CPD_GROUP_READ,
+            and verify with CPD_GROUP_READ option set. */
+  testdir = get_datadir_fname("checkdir_existsread_groupread");
+  cpd_chkopts = CPD_CREATE|CPD_GROUP_READ;
+  unix_verify_optsmask = 0027;
+  tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
+  tt_int_op(0, ==, stat(testdir, &st));
+  tt_int_op(0, ==, (st.st_mode & unix_verify_optsmask));
+  tor_free(testdir);
+
+  done:
+  ;
+}
+
+#define CHECKDIR(name,flags)                              \
+  { #name, test_checkdir_##name, (flags), NULL, NULL }
+
+struct testcase_t checkdir_tests[] = {
+  CHECKDIR(perms, 0),
+  END_OF_TESTCASES
+};
+