From effed7fb1ca30fa741cad8a8a2178a0008ed285d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 7 Nov 2019 12:28:29 -0500 Subject: [PATCH 01/13] Move some ControlSocket checks to options_validate_cb() There is no reason for them be in options_act_reversible(). --- src/app/config/config.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index 06607e3807..ff9bf833f9 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -1455,20 +1455,6 @@ options_act_reversible,(const or_options_t *old_options, char **msg)) sd_notifyf(0, "MAINPID=%ld\n", (long int)getpid()); #endif -#ifndef HAVE_SYS_UN_H - if (options->ControlSocket || options->ControlSocketsGroupWritable) { - *msg = tor_strdup("Unix domain sockets (ControlSocket) not supported " - "on this OS/with this build."); - goto rollback; - } -#else /* defined(HAVE_SYS_UN_H) */ - if (options->ControlSocketsGroupWritable && !options->ControlSocket) { - *msg = tor_strdup("Setting ControlSocketGroupWritable without setting" - "a ControlSocket makes no sense."); - goto rollback; - } -#endif /* !defined(HAVE_SYS_UN_H) */ - if (running_tor) { int n_ports=0; /* We need to set the connection limit before we can open the listeners. */ @@ -3180,6 +3166,20 @@ options_validate_cb(const void *old_options_, void *options_, char **msg) &world_writable_control_socket) < 0) return -1; +#ifndef HAVE_SYS_UN_H + if (options->ControlSocket || options->ControlSocketsGroupWritable) { + *msg = tor_strdup("Unix domain sockets (ControlSocket) not supported " + "on this OS/with this build."); + return -1; + } +#else /* defined(HAVE_SYS_UN_H) */ + if (options->ControlSocketsGroupWritable && !options->ControlSocket) { + *msg = tor_strdup("Setting ControlSocketGroupWritable without setting" + "a ControlSocket makes no sense."); + return -1; + } +#endif /* !defined(HAVE_SYS_UN_H) */ + /* Set UseEntryGuards from the configured value, before we check it below. * We change UseEntryGuards when it's incompatible with other options, * but leave UseEntryGuards_option with the original value. From 006ce47ffa0dbc60dc0e3fc5d58e24943e291ef9 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 7 Nov 2019 12:47:20 -0500 Subject: [PATCH 02/13] Extract a function for one-time-only pre-reversible options. These changes _only_ happen at startup, and happen before _any_ reversible option change is set. --- src/app/config/config.c | 109 +++++++++++++++++++++++----------------- 1 file changed, 64 insertions(+), 45 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index ff9bf833f9..57835f95dd 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -904,8 +904,8 @@ static smartlist_t *configured_ports = NULL; /** True iff we're currently validating options, and any calls to * get_options() are likely to be bugs. */ static int in_option_validation = 0; -/* True iff we've initialized libevent */ -static int libevent_initialized = 0; +/** True iff we have run options_act_once_on_startup() */ +static bool have_set_startup_options = false; /* A global configuration manager to handle all configuration objects. */ static config_mgr_t *options_mgr = NULL; @@ -1085,7 +1085,7 @@ config_free_all(void) cleanup_protocol_warning_severity_level(); - libevent_initialized = 0; + have_set_startup_options = false; config_mgr_free(options_mgr); } @@ -1422,6 +1422,65 @@ create_keys_directory(const or_options_t *options) /* Helps determine flags to pass to switch_id. */ static int have_low_ports = -1; +/** Take case of initial startup tasks that must occur before any of the + * transactional option-related changes are allowed. */ +static int +options_act_once_on_startup(char **msg_out) +{ + if (have_set_startup_options) + return 0; + + const or_options_t *options = get_options(); + int running_tor = options->command == CMD_RUN_TOR; + + if (!running_tor) + return 0; + + /* Daemonize _first_, since we only want to open most of this stuff in + * the subprocess. Libevent bases can't be reliably inherited across + * processes. */ + if (options->RunAsDaemon) { + if (! start_daemon_has_been_called()) + subsystems_prefork(); + /* No need to roll back, since you can't change the value. */ + if (start_daemon()) + subsystems_postfork(); + } + +#ifdef HAVE_SYSTEMD + /* Our PID may have changed, inform supervisor */ + sd_notifyf(0, "MAINPID=%ld\n", (long int)getpid()); +#endif + + /* Set up libevent. (We need to do this before we can register the + * listeners as listeners.) */ + init_libevent(options); + + /* This has to come up after libevent is initialized. */ + control_initialize_event_queue(); + + /* + * Initialize the scheduler - this has to come after + * options_init_from_torrc() sets up libevent - why yes, that seems + * completely sensible to hide the libevent setup in the option parsing + * code! It also needs to happen before init_keys(), so it needs to + * happen here too. How yucky. */ + scheduler_init(); + + /* Attempt to lock all current and future memory with mlockall() only once. + * This must happen before setuid. */ + if (options->DisableAllSwap) { + if (tor_mlockall() == -1) { + *msg_out = tor_strdup("DisableAllSwap failure. Do you have proper " + "permissions?"); + return -1; + } + } + + have_set_startup_options = true; + return 0; +} + /** Fetch the active option list, and take actions based on it. All of the * things we do should survive being done repeatedly. If present, * old_options contains the previous value of the options. @@ -1439,21 +1498,8 @@ options_act_reversible,(const or_options_t *old_options, char **msg)) int logs_marked = 0, logs_initialized = 0; int old_min_log_level = get_min_log_level(); - /* Daemonize _first_, since we only want to open most of this stuff in - * the subprocess. Libevent bases can't be reliably inherited across - * processes. */ - if (running_tor && options->RunAsDaemon) { - if (! start_daemon_has_been_called()) - subsystems_prefork(); - /* No need to roll back, since you can't change the value. */ - if (start_daemon()) - subsystems_postfork(); - } - -#ifdef HAVE_SYSTEMD - /* Our PID may have changed, inform supervisor */ - sd_notifyf(0, "MAINPID=%ld\n", (long int)getpid()); -#endif + if (options_act_once_on_startup(msg) < 0) + goto rollback; if (running_tor) { int n_ports=0; @@ -1471,24 +1517,6 @@ options_act_reversible,(const or_options_t *old_options, char **msg)) options->ConnLimit_ = old_options->ConnLimit_; } - /* Set up libevent. (We need to do this before we can register the - * listeners as listeners.) */ - if (running_tor && !libevent_initialized) { - init_libevent(options); - libevent_initialized = 1; - - /* This has to come up after libevent is initialized. */ - control_initialize_event_queue(); - - /* - * Initialize the scheduler - this has to come after - * options_init_from_torrc() sets up libevent - why yes, that seems - * completely sensible to hide the libevent setup in the option parsing - * code! It also needs to happen before init_keys(), so it needs to - * happen here too. How yucky. */ - scheduler_init(); - } - /* Adjust the port configuration so we can launch listeners. */ /* 31851: some ports are relay-only */ if (parse_ports(options, 0, msg, &n_ports, NULL)) { @@ -1533,15 +1561,6 @@ options_act_reversible,(const or_options_t *old_options, char **msg)) } #endif /* defined(HAVE_NET_IF_H) && defined(HAVE_NET_PFVAR_H) */ - /* Attempt to lock all current and future memory with mlockall() only once */ - if (options->DisableAllSwap) { - if (tor_mlockall() == -1) { - *msg = tor_strdup("DisableAllSwap failure. Do you have proper " - "permissions?"); - goto done; - } - } - /* Setuid/setgid as appropriate */ if (options->User) { tor_assert(have_low_ports != -1); From 20c24e72d92c2d15ef78d683d71b6e627ac66799 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 7 Nov 2019 12:59:50 -0500 Subject: [PATCH 03/13] options_act_reversible(): Extract more startup-only pieces. These have to happen after opening listeners and before opening logs :/ --- src/app/config/config.c | 158 +++++++++++++++++++++++++--------------- 1 file changed, 100 insertions(+), 58 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index 57835f95dd..1ef23824d1 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -1431,7 +1431,7 @@ options_act_once_on_startup(char **msg_out) return 0; const or_options_t *options = get_options(); - int running_tor = options->command == CMD_RUN_TOR; + const bool running_tor = options->command == CMD_RUN_TOR; if (!running_tor) return 0; @@ -1481,15 +1481,109 @@ options_act_once_on_startup(char **msg_out) return 0; } +/** + * Change our user ID if we're configured to do so. + **/ +static int +options_switch_id(char **msg_out) +{ + const or_options_t *options = get_options(); + + /* Setuid/setgid as appropriate */ + if (options->User) { + tor_assert(have_low_ports != -1); + unsigned switch_id_flags = 0; + if (options->KeepBindCapabilities == 1) { + switch_id_flags |= SWITCH_ID_KEEP_BINDLOW; + switch_id_flags |= SWITCH_ID_WARN_IF_NO_CAPS; + } + if (options->KeepBindCapabilities == -1 && have_low_ports) { + switch_id_flags |= SWITCH_ID_KEEP_BINDLOW; + } + if (switch_id(options->User, switch_id_flags) != 0) { + /* No need to roll back, since you can't change the value. */ + *msg_out = tor_strdup("Problem with User value. See logs for details."); + return -1; + } + } + + return 0; +} + +/** + * Create our DataDirectory, CacheDirectory, and KeyDirectory, and + * set their permissions correctly. + */ +static int +options_create_directories(char **msg_out) +{ + const or_options_t *options = get_options(); + const bool running_tor = options->command == CMD_RUN_TOR; + + /* Ensure data directory is private; create if possible. */ + /* It's okay to do this in "options_act_reversible()" even though it isn't + * actually reversible, since you can't change the DataDirectory while + * Tor is running. */ + if (check_and_create_data_directory(running_tor /* create */, + options->DataDirectory, + options->DataDirectoryGroupReadable, + options->User, + msg_out) < 0) { + return -1; + } + if (check_and_create_data_directory(running_tor /* create */, + options->KeyDirectory, + options->KeyDirectoryGroupReadable, + 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; + } + if (check_and_create_data_directory(running_tor /* create */, + options->CacheDirectory, + cache_dir_group_readable, + options->User, + msg_out) < 0) { + return -1; + } + + return 0; +} + /** Fetch the active option list, and take actions based on it. All of the * things we do should survive being done repeatedly. If present, * old_options contains the previous value of the options. * + * This function is only truly "reversible" _after_ the first time it + * is run. The first time that it runs, it performs some irreversible + * tasks in the correct sequence between the reversible option changes. + * + * Option changes should only be marked as "reversible" if they cannot + * be validated before switching them, but they can be switched back if + * some other validateion fails. + * * Return 0 if all goes well, return -1 if things went badly. */ MOCK_IMPL(STATIC int, options_act_reversible,(const or_options_t *old_options, char **msg)) { + const bool first_time = ! have_set_startup_options; smartlist_t *new_listeners = smartlist_new(); or_options_t *options = get_options_mutable(); int running_tor = options->command == CMD_RUN_TOR; @@ -1561,66 +1655,14 @@ options_act_reversible,(const or_options_t *old_options, char **msg)) } #endif /* defined(HAVE_NET_IF_H) && defined(HAVE_NET_PFVAR_H) */ - /* Setuid/setgid as appropriate */ - if (options->User) { - tor_assert(have_low_ports != -1); - unsigned switch_id_flags = 0; - if (options->KeepBindCapabilities == 1) { - switch_id_flags |= SWITCH_ID_KEEP_BINDLOW; - switch_id_flags |= SWITCH_ID_WARN_IF_NO_CAPS; - } - if (options->KeepBindCapabilities == -1 && have_low_ports) { - switch_id_flags |= SWITCH_ID_KEEP_BINDLOW; - } - if (switch_id(options->User, switch_id_flags) != 0) { - /* No need to roll back, since you can't change the value. */ - *msg = tor_strdup("Problem with User value. See logs for details."); + if (first_time) { + if (options_switch_id(msg) < 0) + goto done; + + if (options_create_directories(msg) < 0) goto done; - } } - /* Ensure data directory is private; create if possible. */ - /* It's okay to do this in "options_act_reversible()" even though it isn't - * actually reversible, since you can't change the DataDirectory while - * Tor is running. */ - if (check_and_create_data_directory(running_tor /* create */, - options->DataDirectory, - options->DataDirectoryGroupReadable, - options->User, - msg) < 0) { - goto done; - } - if (check_and_create_data_directory(running_tor /* create */, - options->KeyDirectory, - options->KeyDirectoryGroupReadable, - options->User, - msg) < 0) { - goto done; - } - - /* 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; - } - if (check_and_create_data_directory(running_tor /* create */, - options->CacheDirectory, - cache_dir_group_readable, - options->User, - msg) < 0) { - goto done; - } /* Bail out at this point if we're not going to be a client or server: * we don't run Tor itself. */ From 5060007f4b959c5b8cd483817969252c4e4138aa Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 7 Nov 2019 17:42:47 -0500 Subject: [PATCH 04/13] Split log configuration out of options_act_reversible(). --- scripts/maint/practracker/exceptions.txt | 2 +- src/app/config/config.c | 199 ++++++++++++++++------- src/app/config/config.h | 2 +- 3 files changed, 144 insertions(+), 59 deletions(-) diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt index a70119e8a2..1c3bf9cbeb 100644 --- a/scripts/maint/practracker/exceptions.txt +++ b/scripts/maint/practracker/exceptions.txt @@ -33,7 +33,7 @@ # # Remember: It is better to fix the problem than to add a new exception! -problem file-size /src/app/config/config.c 7212 +problem file-size /src/app/config/config.c 7400 problem include-count /src/app/config/config.c 80 problem function-size /src/app/config/config.c:options_act_reversible() 298 problem function-size /src/app/config/config.c:options_act() 381 diff --git a/src/app/config/config.c b/src/app/config/config.c index 1ef23824d1..082dff3132 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -862,6 +862,8 @@ static void options_clear_cb(const config_mgr_t *mgr, void *opts); static setopt_err_t options_validate_and_set(const or_options_t *old_options, or_options_t *new_options, char **msg_out); +struct log_transaction_t; +static void options_rollback_log_transaction(struct log_transaction_t *xn); /** Magic value for or_options_t. */ #define OR_OPTIONS_MAGIC 9090909 @@ -1566,6 +1568,139 @@ options_create_directories(char **msg_out) return 0; } +/** Structure to represent an incompleted configuration of a set of logs. + * + * This structure is generated by options_start_log_transaction(), and is + * either committed by options_commit_log_transaction() or rolled back by + * options_rollback_log_transaction(). */ +typedef struct log_transaction_t { + /** Previous lowest severity of any configured log. */ + int old_min_log_level; + /** True if we have marked the previous logs to be closed */ + bool logs_marked; + /** True if we initialized the new set of logs */ + bool logs_initialized; + /** True if our safelogging configuration is different from what it was + * previously (or if we are starting for the first time). */ + bool safelogging_changed; +} log_transaction_t; + +/** + * Start configuring our logs based on the current value of get_options(). + * + * The value old_options holds either the previous options object, + * or NULL if we're starting for the first time. + * + * On success, return a log_transaction_t that we can either roll back or + * commit. + * + * On failure return NULL and write a message into a newly allocated string in + * *msg_out. + **/ +static log_transaction_t * +options_start_log_transaction(const or_options_t *old_options, + char **msg_out) +{ + const or_options_t *options = get_options(); + const bool running_tor = options->command == CMD_RUN_TOR; + + log_transaction_t *xn = tor_malloc_zero(sizeof(log_transaction_t)); + xn->old_min_log_level = get_min_log_level(); + + if (! running_tor) + goto done; + + mark_logs_temp(); /* Close current logs once new logs are open. */ + xn->logs_marked = true; + /* Configure the tor_log(s) */ + if (options_init_logs(old_options, options, 0)<0) { + *msg_out = tor_strdup("Failed to init Log options. See logs for details."); + options_rollback_log_transaction(xn); + xn = NULL; + goto done; + } + + xn->safelogging_changed = !old_options || + old_options->SafeLogging_ != options->SafeLogging_; + + xn->logs_initialized = true; + + done: + return xn; +} + +/** + * Finish configuring the logs that started to get configured with xn. + * Frees xn. + **/ +static void +options_commit_log_transaction(log_transaction_t *xn) +{ + const or_options_t *options = get_options(); + tor_assert(xn); + + if (xn->logs_marked) { + log_severity_list_t *severity = + tor_malloc_zero(sizeof(log_severity_list_t)); + close_temp_logs(); + add_callback_log(severity, control_event_logmsg); + logs_set_pending_callback_callback(control_event_logmsg_pending); + control_adjust_event_log_severity(); + tor_free(severity); + tor_log_update_sigsafe_err_fds(); + } + + if (xn->logs_initialized) { + flush_log_messages_from_startup(); + } + + { + const char *badness = NULL; + int bad_safelog = 0, bad_severity = 0, new_badness = 0; + if (options->SafeLogging_ != SAFELOG_SCRUB_ALL) { + bad_safelog = 1; + if (xn->safelogging_changed) + new_badness = 1; + } + if (get_min_log_level() >= LOG_INFO) { + bad_severity = 1; + if (get_min_log_level() != xn->old_min_log_level) + new_badness = 1; + } + if (bad_safelog && bad_severity) + badness = "you disabled SafeLogging, and " + "you're logging more than \"notice\""; + else if (bad_safelog) + badness = "you disabled SafeLogging"; + else + badness = "you're logging more than \"notice\""; + if (new_badness) + log_warn(LD_GENERAL, "Your log may contain sensitive information - %s. " + "Don't log unless it serves an important reason. " + "Overwrite the log afterwards.", badness); + } + + tor_free(xn); +} + +/** + * Revert the log configuration changes that that started to get configured + * with xn. Frees xn. + **/ +static void +options_rollback_log_transaction(log_transaction_t *xn) +{ + if (!xn) + return; + + if (xn->logs_marked) { + rollback_log_changes(); + control_adjust_event_log_severity(); + } + + tor_free(xn); +} + /** Fetch the active option list, and take actions based on it. All of the * things we do should survive being done repeatedly. If present, * old_options contains the previous value of the options. @@ -1587,10 +1722,9 @@ options_act_reversible,(const or_options_t *old_options, char **msg)) smartlist_t *new_listeners = smartlist_new(); or_options_t *options = get_options_mutable(); int running_tor = options->command == CMD_RUN_TOR; + log_transaction_t *log_transaction = NULL; int set_conn_limit = 0; int r = -1; - int logs_marked = 0, logs_initialized = 0; - int old_min_log_level = get_min_log_level(); if (options_act_once_on_startup(msg) < 0) goto rollback; @@ -1663,62 +1797,16 @@ options_act_reversible,(const or_options_t *old_options, char **msg)) goto done; } - /* Bail out at this point if we're not going to be a client or server: * we don't run Tor itself. */ - if (!running_tor) - goto commit; - - mark_logs_temp(); /* Close current logs once new logs are open. */ - logs_marked = 1; - /* Configure the tor_log(s) */ - if (options_init_logs(old_options, options, 0)<0) { - *msg = tor_strdup("Failed to init Log options. See logs for details."); + log_transaction = options_start_log_transaction(old_options, msg); + if (log_transaction == NULL) goto rollback; - } - logs_initialized = 1; - commit: + // Commit! r = 0; - if (logs_marked) { - log_severity_list_t *severity = - tor_malloc_zero(sizeof(log_severity_list_t)); - close_temp_logs(); - add_callback_log(severity, control_event_logmsg); - logs_set_pending_callback_callback(control_event_logmsg_pending); - control_adjust_event_log_severity(); - tor_free(severity); - tor_log_update_sigsafe_err_fds(); - } - if (logs_initialized) { - flush_log_messages_from_startup(); - } - { - const char *badness = NULL; - int bad_safelog = 0, bad_severity = 0, new_badness = 0; - if (options->SafeLogging_ != SAFELOG_SCRUB_ALL) { - bad_safelog = 1; - if (!old_options || old_options->SafeLogging_ != options->SafeLogging_) - new_badness = 1; - } - if (get_min_log_level() >= LOG_INFO) { - bad_severity = 1; - if (get_min_log_level() != old_min_log_level) - new_badness = 1; - } - if (bad_safelog && bad_severity) - badness = "you disabled SafeLogging, and " - "you're logging more than \"notice\""; - else if (bad_safelog) - badness = "you disabled SafeLogging"; - else - badness = "you're logging more than \"notice\""; - if (new_badness) - log_warn(LD_GENERAL, "Your log may contain sensitive information - %s. " - "Don't log unless it serves an important reason. " - "Overwrite the log afterwards.", badness); - } + options_commit_log_transaction(log_transaction); if (set_conn_limit) { /* @@ -1754,10 +1842,7 @@ options_act_reversible,(const or_options_t *old_options, char **msg)) r = -1; tor_assert(*msg); - if (logs_marked) { - rollback_log_changes(); - control_adjust_event_log_severity(); - } + options_rollback_log_transaction(log_transaction); if (set_conn_limit && old_options) set_max_file_descriptors((unsigned)old_options->ConnLimit, @@ -4857,7 +4942,7 @@ options_init_log_granularity(const or_options_t *options, * Initialize the logs based on the configuration file. */ STATIC int -options_init_logs(const or_options_t *old_options, or_options_t *options, +options_init_logs(const or_options_t *old_options, const or_options_t *options, int validate_only) { config_line_t *opt; diff --git a/src/app/config/config.h b/src/app/config/config.h index eeba9e64d0..0af96a0c26 100644 --- a/src/app/config/config.h +++ b/src/app/config/config.h @@ -301,7 +301,7 @@ STATIC int open_and_add_file_log(const log_severity_list_t *severity, const char *fname, int truncate_log); STATIC int options_init_logs(const or_options_t *old_options, - or_options_t *options, int validate_only); + const or_options_t *options, int validate_only); #ifdef TOR_UNIT_TESTS int options_validate(const or_options_t *old_options, From 929b46f44a64f1e6d0150d6102e9478a444af730 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 7 Nov 2019 22:23:28 -0500 Subject: [PATCH 05/13] Split listener configuration out of options_act_reversible() --- src/app/config/config.c | 286 +++++++++++++++++++++++++--------------- 1 file changed, 183 insertions(+), 103 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index 082dff3132..ab40946589 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -864,6 +864,9 @@ static setopt_err_t options_validate_and_set(const or_options_t *old_options, char **msg_out); struct log_transaction_t; static void options_rollback_log_transaction(struct log_transaction_t *xn); +struct listener_transaction_t; +static void options_rollback_listener_transaction( + struct listener_transaction_t *xn); /** Magic value for or_options_t. */ #define OR_OPTIONS_MAGIC 9090909 @@ -1568,6 +1571,180 @@ options_create_directories(char **msg_out) return 0; } +/** Structure to represent an incomplete configuration of a set of + * listeners. + * + * This structure is generated by options_start_listener_transaction(), and is + * either committed by options_commit_listener_transaction() or rolled back by + * options_rollback_listener_transaction(). */ +typedef struct listener_transaction_t { + bool set_conn_limit; /**< True if we've set the connection limit */ + unsigned old_conn_limit; /**< If nonzero, previous connlimit value. */ + smartlist_t *new_listeners; /**< List of new listeners that we opened. */ +} listener_transaction_t; + +/** + * Start configuring our listeners based on the current value of + * get_options(). + * + * The value old_options holds either the previous options object, + * or NULL if we're starting for the first time. + * + * On success, return a listener_transaction_t that we can either roll back or + * commit. + * + * On failure return NULL and write a message into a newly allocated string in + * *msg_out. + **/ +static listener_transaction_t * +options_start_listener_transaction(const or_options_t *old_options, + char **msg_out) +{ + listener_transaction_t *xn = tor_malloc_zero(sizeof(listener_transaction_t)); + xn->new_listeners = smartlist_new(); + or_options_t *options = get_options_mutable(); + const bool running_tor = options->command == CMD_RUN_TOR; + + if (! running_tor) { + return xn; + } + + int n_ports=0; + /* We need to set the connection limit before we can open the listeners. */ + if (! sandbox_is_active()) { + if (set_max_file_descriptors((unsigned)options->ConnLimit, + &options->ConnLimit_) < 0) { + *msg_out = tor_strdup("Problem with ConnLimit value. " + "See logs for details."); + goto rollback; + } + xn->set_conn_limit = true; + if (old_options) + xn->old_conn_limit = (unsigned)old_options->ConnLimit; + } else { + tor_assert(old_options); + options->ConnLimit_ = old_options->ConnLimit_; + } + + /* Adjust the port configuration so we can launch listeners. */ + /* 31851: some ports are relay-only */ + if (parse_ports(options, 0, msg_out, &n_ports, NULL)) { + if (!*msg_out) + *msg_out = tor_strdup("Unexpected problem parsing port config"); + goto rollback; + } + + /* Set the hibernation state appropriately.*/ + consider_hibernation(time(NULL)); + + /* Launch the listeners. (We do this before we setuid, so we can bind to + * ports under 1024.) We don't want to rebind if we're hibernating or + * shutting down. If networking is disabled, this will close all but the + * control listeners, but disable those. */ + /* 31851: some listeners are relay-only */ + if (!we_are_hibernating()) { + if (retry_all_listeners(xn->new_listeners, + options->DisableNetwork) < 0) { + *msg_out = tor_strdup("Failed to bind one of the listener ports."); + goto rollback; + } + } + if (options->DisableNetwork) { + /* Aggressively close non-controller stuff, NOW */ + log_notice(LD_NET, "DisableNetwork is set. Tor will not make or accept " + "non-control network connections. Shutting down all existing " + "connections."); + connection_mark_all_noncontrol_connections(); + /* We can't complete circuits until the network is re-enabled. */ + note_that_we_maybe_cant_complete_circuits(); + } + +#if defined(HAVE_NET_IF_H) && defined(HAVE_NET_PFVAR_H) + /* Open /dev/pf before (possibly) dropping privileges. */ + if (options->TransPort_set && + options->TransProxyType_parsed == TPT_DEFAULT) { + if (get_pf_socket() < 0) { + *msg_out = tor_strdup("Unable to open /dev/pf for transparent proxy."); + goto rollback; + } + } +#endif /* defined(HAVE_NET_IF_H) && defined(HAVE_NET_PFVAR_H) */ + + return xn; + + rollback: + options_rollback_listener_transaction(xn); + return NULL; +} + +/** + * Finish configuring the listeners that started to get configured with + * xn. Frees xn. + **/ +static void +options_commit_listener_transaction(listener_transaction_t *xn) +{ + tor_assert(xn); + if (xn->set_conn_limit) { + or_options_t *options = get_options_mutable(); + /* + * If we adjusted the conn limit, recompute the OOS threshold too + * + * How many possible sockets to keep in reserve? If we have lots of + * possible sockets, keep this below a limit and set ConnLimit_high_thresh + * very close to ConnLimit_, but if ConnLimit_ is low, shrink it in + * proportion. + * + * Somewhat arbitrarily, set socks_in_reserve to 5% of ConnLimit_, but + * cap it at 64. + */ + int socks_in_reserve = options->ConnLimit_ / 20; + if (socks_in_reserve > 64) socks_in_reserve = 64; + + options->ConnLimit_high_thresh = options->ConnLimit_ - socks_in_reserve; + options->ConnLimit_low_thresh = (options->ConnLimit_ / 4) * 3; + log_info(LD_GENERAL, + "Recomputed OOS thresholds: ConnLimit %d, ConnLimit_ %d, " + "ConnLimit_high_thresh %d, ConnLimit_low_thresh %d", + options->ConnLimit, options->ConnLimit_, + options->ConnLimit_high_thresh, + options->ConnLimit_low_thresh); + + /* Give the OOS handler a chance with the new thresholds */ + connection_check_oos(get_n_open_sockets(), 0); + } + + smartlist_free(xn->new_listeners); + tor_free(xn); +} + +/** + * Revert the listener configuration changes that that started to get + * configured with xn. Frees xn. + **/ +static void +options_rollback_listener_transaction(listener_transaction_t *xn) +{ + if (! xn) + return; + + or_options_t *options = get_options_mutable(); + + if (xn->set_conn_limit && xn->old_conn_limit) + set_max_file_descriptors(xn->old_conn_limit, &options->ConnLimit_); + + SMARTLIST_FOREACH(xn->new_listeners, connection_t *, conn, + { + log_notice(LD_NET, "Closing partially-constructed %s on %s:%d", + conn_type_to_string(conn->type), conn->address, conn->port); + connection_close_immediate(conn); + connection_mark_for_close(conn); + }); + + smartlist_free(xn->new_listeners); + tor_free(xn); +} + /** Structure to represent an incompleted configuration of a set of logs. * * This structure is generated by options_start_log_transaction(), and is @@ -1719,75 +1896,16 @@ MOCK_IMPL(STATIC int, options_act_reversible,(const or_options_t *old_options, char **msg)) { const bool first_time = ! have_set_startup_options; - smartlist_t *new_listeners = smartlist_new(); - or_options_t *options = get_options_mutable(); - int running_tor = options->command == CMD_RUN_TOR; log_transaction_t *log_transaction = NULL; - int set_conn_limit = 0; + listener_transaction_t *listener_transaction = NULL; int r = -1; if (options_act_once_on_startup(msg) < 0) goto rollback; - if (running_tor) { - int n_ports=0; - /* We need to set the connection limit before we can open the listeners. */ - if (! sandbox_is_active()) { - if (set_max_file_descriptors((unsigned)options->ConnLimit, - &options->ConnLimit_) < 0) { - *msg = tor_strdup("Problem with ConnLimit value. " - "See logs for details."); - goto rollback; - } - set_conn_limit = 1; - } else { - tor_assert(old_options); - options->ConnLimit_ = old_options->ConnLimit_; - } - - /* Adjust the port configuration so we can launch listeners. */ - /* 31851: some ports are relay-only */ - if (parse_ports(options, 0, msg, &n_ports, NULL)) { - if (!*msg) - *msg = tor_strdup("Unexpected problem parsing port config"); - goto rollback; - } - - /* Set the hibernation state appropriately.*/ - consider_hibernation(time(NULL)); - - /* Launch the listeners. (We do this before we setuid, so we can bind to - * ports under 1024.) We don't want to rebind if we're hibernating or - * shutting down. If networking is disabled, this will close all but the - * control listeners, but disable those. */ - /* 31851: some listeners are relay-only */ - if (!we_are_hibernating()) { - if (retry_all_listeners(new_listeners, options->DisableNetwork) < 0) { - *msg = tor_strdup("Failed to bind one of the listener ports."); - goto rollback; - } - } - if (options->DisableNetwork) { - /* Aggressively close non-controller stuff, NOW */ - log_notice(LD_NET, "DisableNetwork is set. Tor will not make or accept " - "non-control network connections. Shutting down all existing " - "connections."); - connection_mark_all_noncontrol_connections(); - /* We can't complete circuits until the network is re-enabled. */ - note_that_we_maybe_cant_complete_circuits(); - } - } - -#if defined(HAVE_NET_IF_H) && defined(HAVE_NET_PFVAR_H) - /* Open /dev/pf before dropping privileges. */ - if (options->TransPort_set && - options->TransProxyType_parsed == TPT_DEFAULT) { - if (get_pf_socket() < 0) { - *msg = tor_strdup("Unable to open /dev/pf for transparent proxy."); - goto rollback; - } - } -#endif /* defined(HAVE_NET_IF_H) && defined(HAVE_NET_PFVAR_H) */ + listener_transaction = options_start_listener_transaction(old_options, msg); + if (listener_transaction == NULL) + goto rollback; if (first_time) { if (options_switch_id(msg) < 0) @@ -1808,33 +1926,7 @@ options_act_reversible,(const or_options_t *old_options, char **msg)) options_commit_log_transaction(log_transaction); - if (set_conn_limit) { - /* - * If we adjusted the conn limit, recompute the OOS threshold too - * - * How many possible sockets to keep in reserve? If we have lots of - * possible sockets, keep this below a limit and set ConnLimit_high_thresh - * very close to ConnLimit_, but if ConnLimit_ is low, shrink it in - * proportion. - * - * Somewhat arbitrarily, set socks_in_reserve to 5% of ConnLimit_, but - * cap it at 64. - */ - int socks_in_reserve = options->ConnLimit_ / 20; - if (socks_in_reserve > 64) socks_in_reserve = 64; - - options->ConnLimit_high_thresh = options->ConnLimit_ - socks_in_reserve; - options->ConnLimit_low_thresh = (options->ConnLimit_ / 4) * 3; - log_info(LD_GENERAL, - "Recomputed OOS thresholds: ConnLimit %d, ConnLimit_ %d, " - "ConnLimit_high_thresh %d, ConnLimit_low_thresh %d", - options->ConnLimit, options->ConnLimit_, - options->ConnLimit_high_thresh, - options->ConnLimit_low_thresh); - - /* Give the OOS handler a chance with the new thresholds */ - connection_check_oos(get_n_open_sockets(), 0); - } + options_commit_listener_transaction(listener_transaction); goto done; @@ -1843,21 +1935,9 @@ options_act_reversible,(const or_options_t *old_options, char **msg)) tor_assert(*msg); options_rollback_log_transaction(log_transaction); - - if (set_conn_limit && old_options) - set_max_file_descriptors((unsigned)old_options->ConnLimit, - &options->ConnLimit_); - - SMARTLIST_FOREACH(new_listeners, connection_t *, conn, - { - log_notice(LD_NET, "Closing partially-constructed %s on %s:%d", - conn_type_to_string(conn->type), conn->address, conn->port); - connection_close_immediate(conn); - connection_mark_for_close(conn); - }); + options_rollback_listener_transaction(listener_transaction); done: - smartlist_free(new_listeners); return r; } From cd8c96ce02735db83c241c26e06b9bab50a0dae4 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 11 Nov 2019 16:31:50 -0500 Subject: [PATCH 06/13] Typo fix in warning message about ControlSocket --- src/app/config/config.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index ab40946589..edab684d7c 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -3400,7 +3400,7 @@ options_validate_cb(const void *old_options_, void *options_, char **msg) } #else /* defined(HAVE_SYS_UN_H) */ if (options->ControlSocketsGroupWritable && !options->ControlSocket) { - *msg = tor_strdup("Setting ControlSocketGroupWritable without setting" + *msg = tor_strdup("Setting ControlSocketGroupWritable without setting " "a ControlSocket makes no sense."); return -1; } From 9951afe177107c9bae5b1731bcb01c59c68e5ebc Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 11 Nov 2019 16:32:25 -0500 Subject: [PATCH 07/13] parseconf test: ControlSocketsGroupWriteable without ControlSocket. --- src/test/conf_examples/controlsock/error | 1 + src/test/conf_examples/controlsock/torrc | 1 + 2 files changed, 2 insertions(+) create mode 100644 src/test/conf_examples/controlsock/error create mode 100644 src/test/conf_examples/controlsock/torrc diff --git a/src/test/conf_examples/controlsock/error b/src/test/conf_examples/controlsock/error new file mode 100644 index 0000000000..8fbea37894 --- /dev/null +++ b/src/test/conf_examples/controlsock/error @@ -0,0 +1 @@ +not supported on this OS\|without setting a ControlSocket diff --git a/src/test/conf_examples/controlsock/torrc b/src/test/conf_examples/controlsock/torrc new file mode 100644 index 0000000000..dd3cb7ede5 --- /dev/null +++ b/src/test/conf_examples/controlsock/torrc @@ -0,0 +1 @@ +ControlSocketsGroupWritable 1 From 3094651fa3c71429e8efb0e23087a78addd6728f Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 19 Nov 2019 11:52:10 -0500 Subject: [PATCH 08/13] New unit tests for options_create_directories(). --- src/test/include.am | 1 + src/test/test.c | 1 + src/test/test.h | 1 + src/test/test_options_act.c | 181 ++++++++++++++++++++++++++++++++++++ 4 files changed, 184 insertions(+) create mode 100644 src/test/test_options_act.c diff --git a/src/test/include.am b/src/test/include.am index bd7ab71a20..6697dbb171 100644 --- a/src/test/include.am +++ b/src/test/include.am @@ -177,6 +177,7 @@ src_test_test_SOURCES += \ src/test/test_oom.c \ src/test/test_oos.c \ src/test/test_options.c \ + src/test/test_options_act.c \ src/test/test_pem.c \ src/test/test_periodic_event.c \ src/test/test_policy.c \ diff --git a/src/test/test.c b/src/test/test.c index 90c0058be8..292082d4fb 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -732,6 +732,7 @@ struct testgroup_t testgroups[] = { { "oom/", oom_tests }, { "oos/", oos_tests }, { "options/", options_tests }, + { "options/act/", options_act_tests }, { "parsecommon/", parsecommon_tests }, { "periodic-event/" , periodic_event_tests }, { "policy/" , policy_tests }, diff --git a/src/test/test.h b/src/test/test.h index 967562890f..45c22d70f7 100644 --- a/src/test/test.h +++ b/src/test/test.h @@ -252,6 +252,7 @@ extern struct testcase_t nodelist_tests[]; extern struct testcase_t oom_tests[]; extern struct testcase_t oos_tests[]; extern struct testcase_t options_tests[]; +extern struct testcase_t options_act_tests[]; extern struct testcase_t parsecommon_tests[]; extern struct testcase_t pem_tests[]; extern struct testcase_t periodic_event_tests[]; diff --git a/src/test/test_options_act.c b/src/test/test_options_act.c new file mode 100644 index 0000000000..aaef1d9110 --- /dev/null +++ b/src/test/test_options_act.c @@ -0,0 +1,181 @@ +/* Copyright (c) 2001-2004, Roger Dingledine. + * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. + * Copyright (c) 2007-2019, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +#define CONFIG_PRIVATE +#include "core/or/or.h" +#include "app/config/config.h" + +#include "test/test.h" +#include "test/log_test_helpers.h" +#include "test/test_helpers.h" + +#ifndef _WIN32 +#include + +/** + * Check whether fname is readable. On success set + * *is_group_readable_out to as appropriate and return 0. On failure + * return -1. + */ +static int +get_file_mode(const char *fname, unsigned *permissions_out) +{ + struct stat st; + int r = stat(fname, &st); + if (r < 0) + return -1; + *permissions_out = (unsigned) st.st_mode; + return 0; +} +#define assert_mode(fn,mask,expected) STMT_BEGIN \ + unsigned mode_; \ + int tmp_ = get_file_mode((fn), &mode_); \ + if (tmp_ < 0) { \ + TT_DIE(("Couldn't stat %s: %s", (fn), strerror(errno))); \ + } \ + if ((mode_ & (mask)) != (expected)) { \ + TT_DIE(("Bad mode %o on %s", mode_, (fn))); \ + } \ + STMT_END +#else +/* "group-readable" isn't meaningful on windows */ +#define assert_mode(fn,mask,expected) STMT_NIL +#endif + +static or_options_t *mock_opts; +static const or_options_t * +mock_get_options(void) +{ + return mock_opts; +} + +static void +test_options_act_create_dirs(void *arg) +{ + (void)arg; + MOCK(get_options, mock_get_options); + char *msg = NULL; + or_options_t *opts = mock_opts = options_new(); + + /* We're testing options_create_directories(), which assumes that + validate_data_directories() has already been called, and all of + KeyDirectory, DataDirectory, and CacheDirectory are set. */ + + /* Success case 1: all directories are the default */ + char *fn; + fn = tor_strdup(get_fname_rnd("ddir")); + opts->DataDirectory = tor_strdup(fn); + opts->CacheDirectory = tor_strdup(fn); + tor_asprintf(&opts->KeyDirectory, "%s/keys", fn); + opts->DataDirectoryGroupReadable = 1; + opts->CacheDirectoryGroupReadable = -1; /* default. */ + int r = options_create_directories(&msg); + tt_int_op(r, OP_EQ, 0); + tt_ptr_op(msg, OP_EQ, NULL); + tt_int_op(FN_DIR, OP_EQ, file_status(opts->DataDirectory)); + tt_int_op(FN_DIR, OP_EQ, file_status(opts->CacheDirectory)); + tt_int_op(FN_DIR, OP_EQ, file_status(opts->KeyDirectory)); + assert_mode(opts->DataDirectory, 0777, 0750); + assert_mode(opts->KeyDirectory, 0777, 0700); + tor_free(fn); + tor_free(opts->KeyDirectory); + or_options_free(opts); + + /* Success case 2: all directories are different. */ + opts = mock_opts = options_new(); + opts->DataDirectory = tor_strdup(get_fname_rnd("ddir")); + opts->CacheDirectory = tor_strdup(get_fname_rnd("cdir")); + opts->KeyDirectory = tor_strdup(get_fname_rnd("kdir")); + opts->CacheDirectoryGroupReadable = 1; // cache directory group readable + r = options_create_directories(&msg); + tt_int_op(r, OP_EQ, 0); + tt_ptr_op(msg, OP_EQ, NULL); + tt_int_op(FN_DIR, OP_EQ, file_status(opts->DataDirectory)); + tt_int_op(FN_DIR, OP_EQ, file_status(opts->CacheDirectory)); + tt_int_op(FN_DIR, OP_EQ, file_status(opts->KeyDirectory)); + assert_mode(opts->DataDirectory, 0777, 0700); + assert_mode(opts->KeyDirectory, 0777, 0700); + assert_mode(opts->CacheDirectory, 0777, 0750); + tor_free(fn); + or_options_free(opts); + + /* Success case 3: all directories are the same. */ + opts = mock_opts = options_new(); + fn = tor_strdup(get_fname_rnd("ddir")); + opts->DataDirectory = tor_strdup(fn); + opts->CacheDirectory = tor_strdup(fn); + 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 + r = options_create_directories(&msg); + tt_int_op(r, OP_EQ, 0); + tt_ptr_op(msg, OP_EQ, NULL); + tt_int_op(FN_DIR, OP_EQ, file_status(opts->DataDirectory)); + tt_int_op(FN_DIR, OP_EQ, file_status(opts->CacheDirectory)); + tt_int_op(FN_DIR, OP_EQ, file_status(opts->KeyDirectory)); + assert_mode(opts->DataDirectory, 0777, 0750); + assert_mode(opts->KeyDirectory, 0777, 0750); + assert_mode(opts->CacheDirectory, 0777, 0750); + tor_free(fn); + or_options_free(opts); + + /* Failure case 1: Can't make datadir. */ + opts = mock_opts = options_new(); + opts->DataDirectory = tor_strdup(get_fname_rnd("ddir")); + opts->CacheDirectory = tor_strdup(get_fname_rnd("cdir")); + opts->KeyDirectory = tor_strdup(get_fname_rnd("kdir")); + write_str_to_file(opts->DataDirectory, "foo", 0); + r = options_create_directories(&msg); + tt_int_op(r, OP_LT, 0); + tt_assert(!strcmpstart(msg, "Couldn't create private data directory")); + or_options_free(opts); + tor_free(msg); + + /* Failure case 2: Can't make keydir. */ + opts = mock_opts = options_new(); + opts->DataDirectory = tor_strdup(get_fname_rnd("ddir")); + opts->CacheDirectory = tor_strdup(get_fname_rnd("cdir")); + opts->KeyDirectory = tor_strdup(get_fname_rnd("kdir")); + write_str_to_file(opts->KeyDirectory, "foo", 0); + r = options_create_directories(&msg); + tt_int_op(r, OP_LT, 0); + tt_assert(!strcmpstart(msg, "Couldn't create private data directory")); + or_options_free(opts); + tor_free(msg); + + /* Failure case 3: Can't make cachedir. */ + opts = mock_opts = options_new(); + opts->DataDirectory = tor_strdup(get_fname_rnd("ddir")); + opts->CacheDirectory = tor_strdup(get_fname_rnd("cdir")); + opts->KeyDirectory = tor_strdup(get_fname_rnd("kdir")); + write_str_to_file(opts->CacheDirectory, "foo", 0); + r = options_create_directories(&msg); + tt_int_op(r, OP_LT, 0); + tt_assert(!strcmpstart(msg, "Couldn't create private data directory")); + tor_free(fn); + or_options_free(opts); + tor_free(msg); + + done: + UNMOCK(get_options); + or_options_free(opts); + mock_opts = NULL; + tor_free(fn); + tor_free(msg); +} + +#ifndef COCCI +#define T(name) { #name, test_options_act_##name, TT_FORK, NULL, NULL } +#endif + +struct testcase_t options_act_tests[] = { + T(create_dirs), + END_OF_TESTCASES +}; From a30d143228b4211fd24093c244117e07e9409de5 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 19 Nov 2019 11:59:21 -0500 Subject: [PATCH 09/13] 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(-) create mode 100644 changes/ticket27992 diff --git a/changes/ticket27992 b/changes/ticket27992 new file mode 100644 index 0000000000..9329a78915 --- /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 ed9efb6fca..4cbfa01a06 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 edab684d7c..e0a334c797 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"), @@ -1515,11 +1515,39 @@ options_switch_id(char **msg_out) return 0; } +/** + * Helper. Given a data directory (datadir) and another directory + * (subdir) with respective group-writable permissions + * datadir_gr and subdir_gr, 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 0af96a0c26..c6c03329bb 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 aaef1d9110..abc1c65481 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); From 89c355b38601e0eda4e999219946bf2431d5de51 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 19 Nov 2019 15:45:12 -0500 Subject: [PATCH 10/13] Some tests for log changes, commit, and rollback --- src/app/config/config.c | 8 ++-- src/app/config/config.h | 6 +++ src/test/test_options_act.c | 95 +++++++++++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+), 5 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index e0a334c797..8c5a53209c 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -862,8 +862,6 @@ static void options_clear_cb(const config_mgr_t *mgr, void *opts); static setopt_err_t options_validate_and_set(const or_options_t *old_options, or_options_t *new_options, char **msg_out); -struct log_transaction_t; -static void options_rollback_log_transaction(struct log_transaction_t *xn); struct listener_transaction_t; static void options_rollback_listener_transaction( struct listener_transaction_t *xn); @@ -1801,7 +1799,7 @@ typedef struct log_transaction_t { * On failure return NULL and write a message into a newly allocated string in * *msg_out. **/ -static log_transaction_t * +STATIC log_transaction_t * options_start_log_transaction(const or_options_t *old_options, char **msg_out) { @@ -1837,7 +1835,7 @@ options_start_log_transaction(const or_options_t *old_options, * Finish configuring the logs that started to get configured with xn. * Frees xn. **/ -static void +STATIC void options_commit_log_transaction(log_transaction_t *xn) { const or_options_t *options = get_options(); @@ -1891,7 +1889,7 @@ options_commit_log_transaction(log_transaction_t *xn) * Revert the log configuration changes that that started to get configured * with xn. Frees xn. **/ -static void +STATIC void options_rollback_log_transaction(log_transaction_t *xn) { if (!xn) diff --git a/src/app/config/config.h b/src/app/config/config.h index c6c03329bb..15c9352467 100644 --- a/src/app/config/config.h +++ b/src/app/config/config.h @@ -304,6 +304,12 @@ 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); +struct log_transaction_t; +STATIC struct log_transaction_t *options_start_log_transaction( + const or_options_t *old_options, + char **msg_out); +STATIC void options_commit_log_transaction(struct log_transaction_t *xn); +STATIC void options_rollback_log_transaction(struct log_transaction_t *xn); #ifdef TOR_UNIT_TESTS int options_validate(const or_options_t *old_options, diff --git a/src/test/test_options_act.c b/src/test/test_options_act.c index abc1c65481..0a9be28c54 100644 --- a/src/test/test_options_act.c +++ b/src/test/test_options_act.c @@ -6,6 +6,7 @@ #define CONFIG_PRIVATE #include "core/or/or.h" #include "app/config/config.h" +#include "lib/encoding/confline.h" #include "test/test.h" #include "test/log_test_helpers.h" @@ -167,11 +168,105 @@ test_options_act_create_dirs(void *arg) tor_free(msg); } +static void +test_options_act_log_transition(void *arg) +{ + (void)arg; + or_options_t *opts = mock_opts = options_new(); + or_options_t *old_opts = NULL; + opts->LogTimeGranularity = 1000; + opts->SafeLogging_ = SAFELOG_SCRUB_ALL; + struct log_transaction_t *lt = NULL; + char *msg = NULL; + MOCK(get_options, mock_get_options); + + tt_ptr_op(opts->Logs, OP_EQ, NULL); + config_line_append(&opts->Logs, "Log", "notice stdout"); + lt = options_start_log_transaction(NULL, &msg); + tt_assert(lt); + tt_assert(!msg); + + // commit, see that there is a change. + options_commit_log_transaction(lt); + lt=NULL; + tt_int_op(get_min_log_level(), OP_EQ, LOG_NOTICE); + + // Now drop to debug. + old_opts = opts; + opts = mock_opts = options_new(); + opts->LogTimeGranularity = 1000; + opts->SafeLogging_ = SAFELOG_SCRUB_ALL; + config_line_append(&opts->Logs, "Log", "debug stdout"); + lt = options_start_log_transaction(old_opts, &msg); + tt_assert(lt); + tt_assert(!msg); + + setup_full_capture_of_logs(LOG_NOTICE); + options_commit_log_transaction(lt); + lt=NULL; + expect_single_log_msg_containing("may contain sensitive information"); + tt_int_op(get_min_log_level(), OP_EQ, LOG_DEBUG); + + // Turn off SafeLogging + or_options_free(old_opts); + mock_clean_saved_logs(); + old_opts = opts; + opts = mock_opts = options_new(); + opts->SafeLogging_ = SAFELOG_SCRUB_NONE; + opts->LogTimeGranularity = 1000; + config_line_append(&opts->Logs, "Log", "debug stdout"); + lt = options_start_log_transaction(old_opts, &msg); + tt_assert(lt); + tt_assert(!msg); + options_commit_log_transaction(lt); + lt=NULL; + expect_single_log_msg_containing("may contain sensitive information"); + tt_int_op(get_min_log_level(), OP_EQ, LOG_DEBUG); + + // Try rolling back. + or_options_free(old_opts); + mock_clean_saved_logs(); + old_opts = opts; + opts = mock_opts = options_new(); + opts->SafeLogging_ = SAFELOG_SCRUB_NONE; + opts->LogTimeGranularity = 1000; + config_line_append(&opts->Logs, "Log", "notice stdout"); + lt = options_start_log_transaction(old_opts, &msg); + tt_assert(lt); + tt_assert(!msg); + options_rollback_log_transaction(lt); + expect_no_log_entry(); + lt = NULL; + tt_int_op(get_min_log_level(), OP_EQ, LOG_DEBUG); + + // Now try some bad options. + or_options_free(opts); + mock_clean_saved_logs(); + opts = mock_opts = options_new(); + opts->LogTimeGranularity = 1000; + config_line_append(&opts->Logs, "Log", "warn blaznert"); + lt = options_start_log_transaction(old_opts, &msg); + tt_assert(!lt); + tt_str_op(msg, OP_EQ, "Failed to init Log options. See logs for details."); + expect_single_log_msg_containing("Couldn't parse"); + tt_int_op(get_min_log_level(), OP_EQ, LOG_DEBUG); + + done: + UNMOCK(get_options); + or_options_free(opts); + or_options_free(old_opts); + tor_free(msg); + if (lt) + options_rollback_log_transaction(lt); + teardown_capture_of_logs(); +} + #ifndef COCCI #define T(name) { #name, test_options_act_##name, TT_FORK, NULL, NULL } #endif struct testcase_t options_act_tests[] = { T(create_dirs), + T(log_transition), END_OF_TESTCASES }; From acb97cfa689577b9d3a22eab7673bda6e47918f0 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 20 Nov 2019 08:26:37 -0500 Subject: [PATCH 11/13] log config: Set safelogging_changed even if we aren't running Tor. --- src/app/config/config.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index 8c5a53209c..06e45361cc 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -1808,6 +1808,8 @@ options_start_log_transaction(const or_options_t *old_options, log_transaction_t *xn = tor_malloc_zero(sizeof(log_transaction_t)); xn->old_min_log_level = get_min_log_level(); + xn->safelogging_changed = !old_options || + old_options->SafeLogging_ != options->SafeLogging_; if (! running_tor) goto done; @@ -1822,9 +1824,6 @@ options_start_log_transaction(const or_options_t *old_options, goto done; } - xn->safelogging_changed = !old_options || - old_options->SafeLogging_ != options->SafeLogging_; - xn->logs_initialized = true; done: From a3d06179ce8b60ee598fa806354cee497d1840b9 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 20 Nov 2019 09:27:33 -0500 Subject: [PATCH 12/13] Typo/grammar fixes. --- src/app/config/config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index 06e45361cc..1bd70889c0 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -1770,7 +1770,7 @@ options_rollback_listener_transaction(listener_transaction_t *xn) tor_free(xn); } -/** Structure to represent an incompleted configuration of a set of logs. +/** Structure to represent an incomplete configuration of a set of logs. * * This structure is generated by options_start_log_transaction(), and is * either committed by options_commit_log_transaction() or rolled back by @@ -1912,7 +1912,7 @@ options_rollback_log_transaction(log_transaction_t *xn) * * Option changes should only be marked as "reversible" if they cannot * be validated before switching them, but they can be switched back if - * some other validateion fails. + * some other validation fails. * * Return 0 if all goes well, return -1 if things went badly. */ From b33f3c960db2195906eaafa02798ebefa30c2116 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 20 Nov 2019 09:49:25 -0500 Subject: [PATCH 13/13] options_act_reversible: add more comments to explain ordering --- src/app/config/config.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index 1bd70889c0..5b7b798549 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -1902,8 +1902,10 @@ options_rollback_log_transaction(log_transaction_t *xn) tor_free(xn); } -/** Fetch the active option list, and take actions based on it. All of the - * things we do should survive being done repeatedly. If present, +/** + * Fetch the active option list, and take actions based on it. All of + * the things we do in this function should survive being done + * repeatedly, OR be done only once when starting Tor. If present, * old_options contains the previous value of the options. * * This function is only truly "reversible" _after_ the first time it @@ -1924,9 +1926,19 @@ options_act_reversible,(const or_options_t *old_options, char **msg)) listener_transaction_t *listener_transaction = NULL; int r = -1; + /* The ordering of actions in this function is not free, sadly. + * + * First of all, we _must_ daemonize before we take all kinds of + * initialization actions, since they need to happen in the + * subprocess. + */ if (options_act_once_on_startup(msg) < 0) goto rollback; + /* Once we've handled most of once-off initialization, we need to + * open our listeners before we switch IDs. (If we open listeners first, + * we might not be able to bind to low ports.) + */ listener_transaction = options_start_listener_transaction(old_options, msg); if (listener_transaction == NULL) goto rollback; @@ -1934,7 +1946,13 @@ options_act_reversible,(const or_options_t *old_options, char **msg)) if (first_time) { if (options_switch_id(msg) < 0) goto done; + } + /* On the other hand, we need to touch the file system _after_ we + * switch IDs: otherwise, we'll be making directories and opening files + * with the wrong permissions. + */ + if (first_time) { if (options_create_directories(msg) < 0) goto done; }