Make ControlSocketsGroupWritable work with User.

Original message from bug3393:

check_private_dir() to ensure that ControlSocketsGroupWritable is
safe to use. Unfortunately, check_private_dir() only checks against
the currently running user… which can be root until privileges are
dropped to the user and group configured by the User config option.

The attached patch fixes the issue by adding a new effective_user
argument to check_private_dir() and updating the callers. It might
not be the best way to fix the issue, but it did in my tests.

(Code by lunar; changelog by nickm)

diff --git a/src/common/util.c b/src/common/util.c
index 6f323dd20c04341bf4818ff56acb21e942839a43..36aa9ccef8d3544cb3bafaef8886648c7577b4ba 100644 (file)
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -1677,15 +1677,20 @@ file_status(const char *fname)
  * 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.
+ * When effective_user is not NULL, check permissions against the given user and
+ * its primary group.
  */
 int
-check_private_dir(const char *dirname, cpd_check_t check)
+check_private_dir(const char *dirname, cpd_check_t check, const char *effective_user)
 {
   int r;
   struct stat st;
   char *f;
 #ifndef MS_WINDOWS
   int mask;
+  struct passwd *pw = NULL;
+  uid_t running_uid;
+  gid_t running_gid;
 #endif
 
   tor_assert(dirname);
@@ -1724,33 +1729,47 @@ check_private_dir(const char *dirname, cpd_check_t check)
     return -1;
   }
 #ifndef MS_WINDOWS
-  if (st.st_uid != getuid()) {
+  if (effective_user) {
+    /* Lookup the user and group information, if we have a problem, bail out. */
+    pw = getpwnam(effective_user);
+    if (pw == NULL) {
+      log_warn(LD_CONFIG, "Error setting configured user: %s not found", effective_user);
+      return -1;
+    }
+    running_uid = pw->pw_uid;
+    running_gid = pw->pw_gid;
+  } else {
+    running_uid = getuid();
+    running_gid = getgid();
+  }
+
+  if (st.st_uid != running_uid) {
     struct passwd *pw = NULL;
     char *process_ownername = NULL;
 
-    pw = getpwuid(getuid());
+    pw = getpwuid(running_uid);
     process_ownername = pw ? tor_strdup(pw->pw_name) : tor_strdup("<unknown>");
 
     pw = getpwuid(st.st_uid);
 
     log_warn(LD_FS, "%s is not owned by this user (%s, %d) but by "
         "%s (%d). Perhaps you are running Tor as the wrong user?",
-                         dirname, process_ownername, (int)getuid(),
+                         dirname, process_ownername, (int)running_uid,
                          pw ? pw->pw_name : "<unknown>", (int)st.st_uid);
 
     tor_free(process_ownername);
     return -1;
   }
-  if ((check & CPD_GROUP_OK) && st.st_gid != getgid()) {
+  if ((check & CPD_GROUP_OK) && st.st_gid != running_gid) {
     struct group *gr;
     char *process_groupname = NULL;
-    gr = getgrgid(getgid());
+    gr = getgrgid(running_gid);
     process_groupname = gr ? tor_strdup(gr->gr_name) : tor_strdup("<unknown>");
     gr = getgrgid(st.st_gid);
 
     log_warn(LD_FS, "%s is not owned by this group (%s, %d) but by group "
              "%s (%d).  Are you running Tor as the wrong user?",
-             dirname, process_groupname, (int)getgid(),
+             dirname, process_groupname, (int)running_gid,
              gr ?  gr->gr_name : "<unknown>", (int)st.st_gid);
 
     tor_free(process_groupname);

diff --git a/src/common/util.h b/src/common/util.h
index d657db674e92f4a2cc9ada1185867b0c065ee78d..b9db25ca736ada654ade3ae2d94eb09c4295f470 100644 (file)
--- a/src/common/util.h
+++ b/src/common/util.h
@@ -292,7 +292,8 @@ typedef unsigned int cpd_check_t;
 #define CPD_CHECK 2
 #define CPD_GROUP_OK 4
 #define CPD_CHECK_MODE_ONLY 8
-int check_private_dir(const char *dirname, cpd_check_t check);
+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)
 typedef struct open_file_t open_file_t;

diff --git a/src/or/config.c b/src/or/config.c
index 44cecf353be4c3499c9753d54ff166e4489abcf5..8ab23a3b806d3bea129ca635fe306070084f232c 100644 (file)
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -1025,7 +1025,8 @@ options_act_reversible(or_options_t *old_options, char **msg)
 
   /* Ensure data directory is private; create if possible. */
   if (check_private_dir(options->DataDirectory,
-                        running_tor ? CPD_CREATE : CPD_CHECK)<0) {
+                        running_tor ? CPD_CREATE : CPD_CHECK,
+                        options->User)<0) {
     tor_asprintf(msg,
               "Couldn't access/create private data directory \"%s\"",
               options->DataDirectory);
@@ -1038,7 +1039,8 @@ options_act_reversible(or_options_t *old_options, char **msg)
     char *fn = tor_malloc(len);
     tor_snprintf(fn, len, "%s"PATH_SEPARATOR"cached-status",
                  options->DataDirectory);
-    if (check_private_dir(fn, running_tor ? CPD_CREATE : CPD_CHECK) < 0) {
+    if (check_private_dir(fn, running_tor ? CPD_CREATE : CPD_CHECK,
+                          options->User) < 0) {
       tor_asprintf(msg,
                 "Couldn't access/create private data directory \"%s\"", fn);
       tor_free(fn);

diff --git a/src/or/connection.c b/src/or/connection.c
index 3f4ca1db4b837678a00056e1314909ef146e1f02..a9e3a74ed6dd08f54f6fc98afced727d9866d38c 100644 (file)
--- a/src/or/connection.c
+++ b/src/or/connection.c
@@ -867,7 +867,7 @@ check_location_for_unix_socket(or_options_t *options, const char *path)
   if (options->ControlSocketsGroupWritable)
     flags |= CPD_GROUP_OK;
 
-  if (check_private_dir(p, flags) < 0) {
+  if (check_private_dir(p, flags, options->User) < 0) {
     char *escpath, *escdir;
     escpath = esc_for_log(path);
     escdir = esc_for_log(p);

diff --git a/src/or/geoip.c b/src/or/geoip.c
index 5bb2410a750d141dd33ad7736f47cada012a909b..c621ea81832db383e2c9604a9a010ac584d1b5a6 100644 (file)
--- a/src/or/geoip.c
+++ b/src/or/geoip.c
@@ -970,7 +970,7 @@ geoip_dirreq_stats_write(time_t now)
   geoip_remove_old_clients(start_of_dirreq_stats_interval);
 
   statsdir = get_datadir_fname("stats");
-  if (check_private_dir(statsdir, CPD_CREATE) < 0)
+  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0)
     goto done;
   filename = get_datadir_fname2("stats", "dirreq-stats");
   data_v2 = geoip_get_client_history(GEOIP_CLIENT_NETWORKSTATUS_V2);
@@ -1209,7 +1209,7 @@ geoip_bridge_stats_write(time_t now)
 
   /* Write it to disk. */
   statsdir = get_datadir_fname("stats");
-  if (check_private_dir(statsdir, CPD_CREATE) < 0)
+  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0)
     goto done;
   filename = get_datadir_fname2("stats", "bridge-stats");
 
@@ -1304,7 +1304,7 @@ geoip_entry_stats_write(time_t now)
   geoip_remove_old_clients(start_of_entry_stats_interval);
 
   statsdir = get_datadir_fname("stats");
-  if (check_private_dir(statsdir, CPD_CREATE) < 0)
+  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0)
     goto done;
   filename = get_datadir_fname2("stats", "entry-stats");
   data = geoip_get_client_history(GEOIP_CLIENT_CONNECT);

diff --git a/src/or/rendservice.c b/src/or/rendservice.c
index a10e43fead71ac80bd047e438b92d5602be421ca..d9a936471efa78af600930f1ce773027d41031e0 100644 (file)
--- a/src/or/rendservice.c
+++ b/src/or/rendservice.c
@@ -569,7 +569,7 @@ rend_service_load_keys(void)
              s->directory);
 
     /* Check/create directory */
-    if (check_private_dir(s->directory, CPD_CREATE) < 0)
+    if (check_private_dir(s->directory, CPD_CREATE, get_options()->User) < 0)
       return -1;
 
     /* Load key */

diff --git a/src/or/rephist.c b/src/or/rephist.c
index 54593a06c3470cf25898eb2196e4fb488b1eff31..b7341f3c0d0a424d06fe952391965d058094a34e 100644 (file)
--- a/src/or/rephist.c
+++ b/src/or/rephist.c
@@ -2307,7 +2307,7 @@ rep_hist_exit_stats_write(time_t now)
 
   /* Try to write to disk. */
   statsdir = get_datadir_fname("stats");
-  if (check_private_dir(statsdir, CPD_CREATE) < 0) {
+  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) {
     log_warn(LD_HIST, "Unable to create stats/ directory!");
     goto done;
   }
@@ -2497,7 +2497,7 @@ rep_hist_buffer_stats_write(time_t now)
   smartlist_clear(circuits_for_buffer_stats);
   /* write to file */
   statsdir = get_datadir_fname("stats");
-  if (check_private_dir(statsdir, CPD_CREATE) < 0)
+  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0)
     goto done;
   filename = get_datadir_fname2("stats", "buffer-stats");
   out = start_writing_to_stdio_file(filename, OPEN_FLAGS_APPEND,

diff --git a/src/or/router.c b/src/or/router.c
index 68e29bb4c8ab4273fbc632855fe1ca0829332471..2165e6ea90c211586c066578b589ecc2c54cff8a 100644 (file)
--- a/src/or/router.c
+++ b/src/or/router.c
@@ -533,12 +533,12 @@ init_keys(void)
     return 0;
   }
   /* Make sure DataDirectory exists, and is private. */
-  if (check_private_dir(options->DataDirectory, CPD_CREATE)) {
+  if (check_private_dir(options->DataDirectory, CPD_CREATE, options->User)) {
     return -1;
   }
   /* Check the key directory. */
   keydir = get_datadir_fname("keys");
-  if (check_private_dir(keydir, CPD_CREATE)) {
+  if (check_private_dir(keydir, CPD_CREATE, options->User)) {
     tor_free(keydir);
     return -1;
   }