Merge branch 'bug2972' into maint-0.2.2

11 files changed, 203 insertions, 14 deletions

diff --git a/changes/bug2792_checkdir b/changes/bug2792_checkdir
new file mode 100644
index 0000000000..10de1deb2d
--- /dev/null
+++ b/changes/bug2792_checkdir
@@ -0,0 +1,8 @@
+  o Minor features:
+    - Tor now refuses to create a ControlSocket in a directory that is
+      world-readable (or group-readable if ControlSocketsGroupWritable
+      is 0).  This is necessary because some operating systems do not
+      check the permissions on an AF_UNIX socket when programs try to
+      connect to it.  Checking permissions on the directory holding
+      the socket, however, seems to work everywhere.
+
diff --git a/changes/bug2972 b/changes/bug2972
new file mode 100644
index 0000000000..26afcca421
--- /dev/null
+++ b/changes/bug2972
@@ -0,0 +1,5 @@
+  o Minor features:
+    - Allow ControlSockets to be group-writable when the
+      ControlSocksGroupWritable configuration option is turned on. Patch
+      by Jérémy Bobbio; implements ticket 2972.
+
diff --git a/doc/tor.1.txt b/doc/tor.1.txt
index d95d764c67..1815a8d963 100644
--- a/doc/tor.1.txt
+++ b/doc/tor.1.txt
@@ -167,6 +167,11 @@ Other options can be specified either on the command-line (--option
     Like ControlPort, but listens on a Unix domain socket, rather than a TCP
     socket. (Unix and Unix-like systems only.)
 
+**ControlSocketsGroupWritable** **0**|**1**::
+    If this option is set to 0, don't allow the filesystem group to read and
+    write unix sockets (e.g. ControlSocket). If the option is set to 1, make
+    the control socket readable and writable by the default GID. (Default: 0)
+
 **HashedControlPassword** __hashed_password__::
     Don't allow any connections on the control port except when the other
     process knows the password whose one-way hash is __hashed_password__. You
diff --git a/src/common/compat.c b/src/common/compat.c
index ea7f9d7efc..fc066da681 100644
--- a/src/common/compat.c
+++ b/src/common/compat.c
@@ -1467,6 +1467,45 @@ get_user_homedir(const char *username)
 }
 #endif
 
+/** Modify <b>fname</b> to contain the name of the directory */
+int
+get_parent_directory(char *fname)
+{
+  char *cp;
+  int at_end = 1;
+  tor_assert(fname);
+#ifdef MS_WINDOWS
+  /* If we start with, say, c:, then don't consider that the start of the path
+   */
+  if (fname[0] && fname[1] == ':') {
+    fname += 2;
+  }
+#endif
+  /* Now we want to remove all path-separators at the end of the string,
+   * and to remove the end of the string starting with the path separator
+   * before the last non-path-separator.  In perl, this would be
+   *   s#[/]*$##; s#/[^/]*$##;
+   * on a unixy platform.
+   */
+  cp = fname + strlen(fname);
+  at_end = 1;
+  while (--cp > fname) {
+    int is_sep = (*cp == '/'
+#ifdef MS_WINDOWS
+                  || *cp == '\\'
+#endif
+                  );
+    if (is_sep) {
+      *cp = '\0';
+      if (! at_end)
+        return 0;
+    } else {
+      at_end = 0;
+    }
+  }
+  return -1;
+}
+
 /** Set *addr to the IP address (in dotted-quad notation) stored in c.
  * Return 1 on success, 0 if c is badly formatted.  (Like inet_aton(c,addr),
  * but works on Windows and Solaris.)
diff --git a/src/common/compat.h b/src/common/compat.h
index af795ffba9..eff51ab30c 100644
--- a/src/common/compat.h
+++ b/src/common/compat.h
@@ -552,6 +552,8 @@ int switch_id(const char *user);
 char *get_user_homedir(const char *username);
 #endif
 
+int get_parent_directory(char *fname);
+
 int spawn_func(void (*func)(void *), void *data);
 void spawn_exit(void) ATTR_NORETURN;
 
diff --git a/src/common/util.c b/src/common/util.c
index 86f4141674..1bb116b212 100644
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -30,6 +30,7 @@
 #else
 #include <dirent.h>
 #include <pwd.h>
+#include <grp.h>
 #endif
 
 /* math.h needs this on Linux */
@@ -1664,17 +1665,25 @@ file_status(const char *fname)
     return FN_ERROR;
 }
 
-/** Check whether dirname exists and is private.  If yes return 0.  If
- * it does not exist, and check==CPD_CREATE is set, try to create it
+/** 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
- * check==CPD_CHECK, and we think we can create it, return 0.  Else
- * return -1. */
+ * <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.
+ */
 int
 check_private_dir(const char *dirname, cpd_check_t check)
 {
   int r;
   struct stat st;
   char *f;
+#ifndef MS_WINDOWS
+  int mask;
+#endif
+
   tor_assert(dirname);
   f = tor_strdup(dirname);
   clean_name_for_stat(f);
@@ -1686,10 +1695,7 @@ check_private_dir(const char *dirname, cpd_check_t check)
                strerror(errno));
       return -1;
     }
-    if (check == CPD_NONE) {
-      log_warn(LD_FS, "Directory %s does not exist.", dirname);
-      return -1;
-    } else if (check == CPD_CREATE) {
+    if (check & CPD_CREATE) {
       log_info(LD_GENERAL, "Creating directory %s", dirname);
 #if defined (MS_WINDOWS) && !defined (WINCE)
       r = mkdir(dirname);
@@ -1701,6 +1707,9 @@ check_private_dir(const char *dirname, cpd_check_t check)
             strerror(errno));
         return -1;
       }
+    } else if (!(check & CPD_CHECK)) {
+      log_warn(LD_FS, "Directory %s does not exist.", dirname);
+      return -1;
     }
     /* XXXX In the case where check==CPD_CHECK, we should look at the
      * parent directory a little harder. */
@@ -1728,9 +1737,38 @@ check_private_dir(const char *dirname, cpd_check_t check)
     tor_free(process_ownername);
     return -1;
   }
-  if (st.st_mode & 0077) {
+  if ((check & CPD_GROUP_OK) && st.st_gid != getgid()) {
+    struct group *gr;
+    char *process_groupname = NULL;
+    gr = getgrgid(getgid());
+    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(),
+             gr ?  gr->gr_name : "<unknown>", (int)st.st_gid);
+
+    tor_free(process_groupname);
+    return -1;
+  }
+  if (check & CPD_GROUP_OK) {
+    mask = 0027;
+  } else {
+    mask = 0077;
+  }
+  if (st.st_mode & mask) {
+    unsigned new_mode;
+    if (check & CPD_CHECK_MODE_ONLY) {
+      log_warn(LD_FS, "Permissions on directory %s are too permissive.",
+               dirname);
+      return -1;
+    }
     log_warn(LD_FS, "Fixing permissions on directory %s", dirname);
-    if (chmod(dirname, 0700)) {
+    new_mode = st.st_mode;
+    new_mode |= 0700; /* Owner should have rwx */
+    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));
       return -1;
diff --git a/src/common/util.h b/src/common/util.h
index 961b5875ad..f32709accd 100644
--- a/src/common/util.h
+++ b/src/common/util.h
@@ -286,7 +286,12 @@ file_status_t file_status(const char *filename);
 
 /** Possible behaviors for check_private_dir() on encountering a nonexistent
  * directory; see that function's documentation for details. */
-typedef enum { CPD_NONE, CPD_CREATE, CPD_CHECK } cpd_check_t;
+typedef unsigned int cpd_check_t;
+#define CPD_NONE 0
+#define CPD_CREATE 1
+#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);
 #define OPEN_FLAGS_REPLACE (O_WRONLY|O_CREAT|O_TRUNC)
 #define OPEN_FLAGS_APPEND (O_WRONLY|O_CREAT|O_APPEND)
diff --git a/src/or/config.c b/src/or/config.c
index 6a2742d954..614fc48c3e 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -209,6 +209,7 @@ static config_var_t _option_vars[] = {
   V(ControlPortFileGroupReadable,BOOL,     "0"),
   V(ControlPortWriteToFile,      FILENAME, NULL),
   V(ControlSocket,               LINELIST, NULL),
+  V(ControlSocketsGroupWritable,    BOOL,     "0"),
   V(CookieAuthentication,        BOOL,     "0"),
   V(CookieAuthFileGroupReadable, BOOL,     "0"),
   V(CookieAuthFile,              STRING,   NULL),
@@ -952,9 +953,15 @@ options_act_reversible(or_options_t *old_options, char **msg)
   }
 
 #ifndef HAVE_SYS_UN_H
-  if (options->ControlSocket) {
-    *msg = tor_strdup("Unix domain sockets (ControlSocket) not supported"
-                      " on this OS/with this build.");
+  if (options->ControlSocket || options->ControlSocketsGroupWritable) {
+    *msg = tor_strdup("Unix domain sockets (ControlSocket) not supported "
+                      "on this OS/with this build.");
+    goto rollback;
+  }
+#else
+  if (options->ControlSocketsGroupWritable && !options->ControlSocket) {
+    *msg = tor_strdup("Setting ControlSocketGroupWritable without setting"
+                      "a ControlSocket makes no sense.");
     goto rollback;
   }
 #endif
diff --git a/src/or/connection.c b/src/or/connection.c
index 01b533d9b5..b7d6fe408d 100644
--- a/src/or/connection.c
+++ b/src/or/connection.c
@@ -853,6 +853,43 @@ warn_too_many_conns(void)
   }
 }
 
+#ifdef HAVE_SYS_UN_H
+/** Check whether we should be willing to open an AF_UNIX socket in
+ * <b>path</b>.  Return 0 if we should go ahead and -1 if we shouldn't. */
+static int
+check_location_for_unix_socket(or_options_t *options, const char *path)
+{
+  int r = -1;
+  char *p = tor_strdup(path);
+  cpd_check_t flags = CPD_CHECK_MODE_ONLY;
+  if (get_parent_directory(p)<0)
+    goto done;
+
+  if (options->ControlSocketsGroupWritable)
+    flags |= CPD_GROUP_OK;
+
+  if (check_private_dir(p, flags) < 0) {
+    char *escpath, *escdir;
+    escpath = esc_for_log(path);
+    escdir = esc_for_log(p);
+    log_warn(LD_GENERAL, "Before Tor can create a control socket in %s, the "
+             "directory %s needs to exist, and to be accessible only by the "
+             "user%s account that is running Tor.  (On some Unix systems, "
+             "anybody who can list a socket can conect to it, so Tor is "
+             "being careful.)", escpath, escdir,
+             options->ControlSocketsGroupWritable ? " and group" : "");
+    tor_free(escpath);
+    tor_free(escdir);
+    goto done;
+  }
+
+  r = 0;
+ done:
+  tor_free(p);
+  return r;
+}
+#endif
+
 /** Bind a new non-blocking socket listening to the socket described
  * by <b>listensockaddr</b>.
  *
@@ -947,6 +984,9 @@ connection_create_listener(const struct sockaddr *listensockaddr,
      * and listeners at the same time */
     tor_assert(type == CONN_TYPE_CONTROL_LISTENER);
 
+    if (check_location_for_unix_socket(get_options(), address) < 0)
+      goto err;
+
     log_notice(LD_NET, "Opening %s on %s",
                conn_type_to_string(type), address);
 
@@ -966,6 +1006,15 @@ connection_create_listener(const struct sockaddr *listensockaddr,
                tor_socket_strerror(tor_socket_errno(s)));
       goto err;
     }
+    if (get_options()->ControlSocketsGroupWritable) {
+      /* We need to use chmod; fchmod doesn't work on sockets on all
+       * platforms. */
+      if (chmod(address, 0660) < 0) {
+        log_warn(LD_FS,"Unable to make %s group-writable.", address);
+        tor_close_socket(s);
+        goto err;
+      }
+    }
 
     if (listen(s,SOMAXCONN) < 0) {
       log_warn(LD_NET, "Could not listen on %s: %s", address,
diff --git a/src/or/or.h b/src/or/or.h
index 5647691550..b9d8319ba5 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -2445,6 +2445,7 @@ typedef struct {
   int ControlPort; /**< Port to listen on for control connections. */
   config_line_t *ControlSocket; /**< List of Unix Domain Sockets to listen on
                                  * for control connections. */
+  int ControlSocketsGroupWritable; /**< Boolean: Are control sockets g+rw? */
   int DirPort; /**< Port to listen on for directory connections. */
   int DNSPort; /**< Port to listen on for DNS requests. */
   int AssumeReachable; /**< Whether to publish our descriptor regardless. */
diff --git a/src/test/test_util.c b/src/test/test_util.c
index 0da45df499..23cd059cf7 100644
--- a/src/test/test_util.c
+++ b/src/test/test_util.c
@@ -1195,6 +1195,35 @@ test_util_listdir(void *ptr)
   }
 }
 
+static void
+test_util_parent_dir(void *ptr)
+{
+  char *cp;
+  (void)ptr;
+
+#define T(input,expect_ok,output)               \
+  do {                                          \
+    int ok;                                     \
+    cp = tor_strdup(input);                     \
+    ok = get_parent_directory(cp);              \
+    tt_int_op(ok, ==, expect_ok);               \
+    if (ok==0)                                  \
+      tt_str_op(cp, ==, output);                \
+    tor_free(cp);                               \
+  } while (0);
+
+  T("/home/wombat/knish", 0, "/home/wombat");
+  T("/home/wombat/knish/", 0, "/home/wombat");
+  T("./home/wombat/knish/", 0, "./home/wombat");
+  T("./wombat", 0, ".");
+  T("", -1, "");
+  T("/", -1, "");
+  T("////", -1, "");
+
+ done:
+  tor_free(cp);
+}
+
 #ifdef MS_WINDOWS
 static void
 test_util_load_win_lib(void *ptr)
@@ -1286,6 +1315,7 @@ struct testcase_t util_tests[] = {
   UTIL_TEST(find_str_at_start_of_line, 0),
   UTIL_TEST(asprintf, 0),
   UTIL_TEST(listdir, 0),
+  UTIL_TEST(parent_dir, 0),
 #ifdef MS_WINDOWS
   UTIL_TEST(load_win_lib, 0),
 #endif