diff options
| -rw-r--r-- | changes/bug2792_checkdir | 8 | ||||
| -rw-r--r-- | changes/bug2972 | 5 | ||||
| -rw-r--r-- | doc/tor.1.txt | 5 | ||||
| -rw-r--r-- | src/common/compat.c | 39 | ||||
| -rw-r--r-- | src/common/compat.h | 2 | ||||
| -rw-r--r-- | src/common/util.c | 58 | ||||
| -rw-r--r-- | src/common/util.h | 7 | ||||
| -rw-r--r-- | src/or/config.c | 13 | ||||
| -rw-r--r-- | src/or/connection.c | 49 | ||||
| -rw-r--r-- | src/or/or.h | 1 | ||||
| -rw-r--r-- | src/test/test_util.c | 30 |
11 files changed, 203 insertions, 14 deletions
diff --git a/changes/bug2792_checkdir b/changes/bug2792_checkdir new file mode 100644 index 000000000..10de1deb2 --- /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 000000000..26afcca42 --- /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 74458ab81..7d72350eb 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 436d882f3..1d00e30c4 100644 --- a/src/common/compat.c +++ b/src/common/compat.c @@ -1522,6 +1522,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 b658ed51a..66303c481 100644 --- a/src/common/compat.h +++ b/src/common/compat.h @@ -557,6 +557,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 d2140c3f3..355b3ab34 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -31,6 +31,7 @@ #else #include <dirent.h> #include <pwd.h> +#include <grp.h> #endif /* math.h needs this on Linux */ @@ -1665,17 +1666,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); @@ -1687,10 +1696,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); @@ -1702,6 +1708,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. */ @@ -1729,9 +1738,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 974b7d6fd..e8d5de2ef 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 cc775e610..78cc4772e 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -214,6 +214,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), @@ -974,9 +975,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 099482bf7..17411e01f 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -896,6 +896,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>. * @@ -990,6 +1027,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); @@ -1009,6 +1049,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 c66271956..6c7430a4e 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2611,6 +2611,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 ee2d455b2..d338bbac9 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1208,6 +1208,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) @@ -1495,6 +1524,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 |