commit a30d143228b4211fd24093c244117e07e9409de5 Author: Nick Mathewson <nickm@torproject.org> Date: Tue Nov 19 11:59:21 2019 -0500 Make KeyDirectory's GroupReadable behave the same as CacheDirectory's. In #26913 we solved a bug where CacheDirectoryGroupReadable would override DataDirectoryGroupReadable when the two directories are the same. We never did the same for KeyDirectory, though, because that's a rare setting. Now that I'm testing this code, though, fixing this issue seems fine. Fixes bug #27992; bugfix on 0.3.3.1-alpha. --- changes/ticket27992 | 5 ++++ doc/tor.1.txt | 8 +++--- src/app/config/config.c | 65 ++++++++++++++++++++++++++++++++------------- src/app/config/config.h | 2 ++ src/test/test_options_act.c | 6 +---- 5 files changed, 59 insertions(+), 27 deletions(-) diff --git a/changes/ticket27992 b/changes/ticket27992 new file mode 100644 index 000000000..9329a7891 --- /dev/null +++ b/changes/ticket27992 @@ -0,0 +1,5 @@ + o Minor bugfixes (configuration): + - When creating a KeyDirectory with the same location as the + DataDirectory (not recommended), respect the DataDirectory's + group-readable setting if one has not been set for the KeyDirectory. + Fixes bug 27992; bugfix on 0.3.3.1-alpha. diff --git a/doc/tor.1.txt b/doc/tor.1.txt index ed9efb6fc..4cbfa01a0 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -2589,10 +2589,12 @@ is non-zero): running. (Default: the "keys" subdirectory of DataDirectory.) -[[KeyDirectoryGroupReadable]] **KeyDirectoryGroupReadable** **0**|**1**:: +[[KeyDirectoryGroupReadable]] **KeyDirectoryGroupReadable** **0**|**1**|**auto**:: If this option is set to 0, don't allow the filesystem group to read the - KeywDirectory. If the option is set to 1, make the KeyDirectory readable - by the default GID. (Default: 0) + KeyDirectory. If the option is set to 1, make the KeyDirectory readable + by the default GID. If the option is "auto", then we use the + setting for DataDirectoryGroupReadable when the KeyDirectory is the + same as the DataDirectory, and 0 otherwise. (Default: auto) [[RephistTrackTime]] **RephistTrackTime** __N__ **seconds**|**minutes**|**hours**|**days**|**weeks**:: Tells an authority, or other node tracking node reliability and history, diff --git a/src/app/config/config.c b/src/app/config/config.c index edab684d7..e0a334c79 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -540,7 +540,7 @@ static const config_var_t option_vars_[] = { V(Socks5ProxyUsername, STRING, NULL), V(Socks5ProxyPassword, STRING, NULL), VAR_IMMUTABLE("KeyDirectory", FILENAME, KeyDirectory_option, NULL), - V(KeyDirectoryGroupReadable, BOOL, "0"), + V(KeyDirectoryGroupReadable, AUTOBOOL, "auto"), VAR_D("HSLayer2Nodes", ROUTERSET, HSLayer2Nodes, NULL), VAR_D("HSLayer3Nodes", ROUTERSET, HSLayer3Nodes, NULL), V(KeepalivePeriod, INTERVAL, "5 minutes"), @@ -1516,10 +1516,38 @@ options_switch_id(char **msg_out) } /** + * Helper. Given a data directory (<b>datadir</b>) and another directory + * (<b>subdir</b>) with respective group-writable permissions + * <b>datadir_gr</b> and <b>subdir_gr</b>, compute whether the subdir should + * be group-writeable. + **/ +static int +compute_group_readable_flag(const char *datadir, + const char *subdir, + int datadir_gr, + int subdir_gr) +{ + if (subdir_gr != -1) { + /* The user specified a default for "subdir", so we always obey it. */ + return subdir_gr; + } + + /* The user left the subdir_gr option on "auto." */ + if (0 == strcmp(subdir, datadir)) { + /* The directories are the same, so we use the group-readable flag from + * the datadirectory */ + return datadir_gr; + } else { + /* The directores are different, so we default to "not group-readable" */ + return 0; + } +} + +/** * Create our DataDirectory, CacheDirectory, and KeyDirectory, and * set their permissions correctly. */ -static int +STATIC int options_create_directories(char **msg_out) { const or_options_t *options = get_options(); @@ -1536,30 +1564,29 @@ options_create_directories(char **msg_out) msg_out) < 0) { return -1; } + + /* We need to handle the group-readable flag for the cache directory and key + * directory specially, since they may be the same as the data directory */ + const int key_dir_group_readable = compute_group_readable_flag( + options->DataDirectory, + options->KeyDirectory, + options->DataDirectoryGroupReadable, + options->KeyDirectoryGroupReadable); + if (check_and_create_data_directory(running_tor /* create */, options->KeyDirectory, - options->KeyDirectoryGroupReadable, + key_dir_group_readable, options->User, msg_out) < 0) { return -1; } - /* We need to handle the group-readable flag for the cache directory - * specially, since the directory defaults to being the same as the - * DataDirectory. */ - int cache_dir_group_readable; - if (options->CacheDirectoryGroupReadable != -1) { - /* If the user specified a value, use their setting */ - cache_dir_group_readable = options->CacheDirectoryGroupReadable; - } else if (!strcmp(options->CacheDirectory, options->DataDirectory)) { - /* If the user left the value as "auto", and the cache is the same as the - * datadirectory, use the datadirectory setting. - */ - cache_dir_group_readable = options->DataDirectoryGroupReadable; - } else { - /* Otherwise, "auto" means "not group readable". */ - cache_dir_group_readable = 0; - } + const int cache_dir_group_readable = compute_group_readable_flag( + options->DataDirectory, + options->CacheDirectory, + options->DataDirectoryGroupReadable, + options->CacheDirectoryGroupReadable); + if (check_and_create_data_directory(running_tor /* create */, options->CacheDirectory, cache_dir_group_readable, diff --git a/src/app/config/config.h b/src/app/config/config.h index 0af96a0c2..c6c03329b 100644 --- a/src/app/config/config.h +++ b/src/app/config/config.h @@ -303,6 +303,8 @@ STATIC int open_and_add_file_log(const log_severity_list_t *severity, STATIC int options_init_logs(const or_options_t *old_options, const or_options_t *options, int validate_only); +STATIC int options_create_directories(char **msg_out); + #ifdef TOR_UNIT_TESTS int options_validate(const or_options_t *old_options, or_options_t *options, diff --git a/src/test/test_options_act.c b/src/test/test_options_act.c index aaef1d911..abc1c6548 100644 --- a/src/test/test_options_act.c +++ b/src/test/test_options_act.c @@ -109,11 +109,7 @@ test_options_act_create_dirs(void *arg) opts->KeyDirectory = tor_strdup(fn); opts->DataDirectoryGroupReadable = 1; opts->CacheDirectoryGroupReadable = -1; /* default. */ -#if 1 - /* Bug 27992: this setting shouldn't be needed, but for now it is, in the - * unusual case that DataDirectory == KeyDirectory */ - opts->KeyDirectoryGroupReadable = 1; -#endif + opts->KeyDirectoryGroupReadable = -1; /* default */ r = options_create_directories(&msg); tt_int_op(r, OP_EQ, 0); tt_ptr_op(msg, OP_EQ, NULL);