diff --git a/changes/bug20484 b/changes/bug20484 new file mode 100644 index 000000000..9a0b95cb3 --- /dev/null +++ b/changes/bug20484 @@ -0,0 +1,5 @@ + o Minor bugfixes (single onion services): + - Start correctly when creating a single onion service in a + directory that did not previously exist. Fixes bug 20484; bugfix on + 0.2.9.3-alpha. + diff --git a/changes/bug20529 b/changes/bug20529 new file mode 100644 index 000000000..276be5b2b --- /dev/null +++ b/changes/bug20529 @@ -0,0 +1,4 @@ + o Minor bugfixes (hidden services): + - When configuring hidden services, check every hidden service directory's + permissions. Previously, we only checked the last hidden service. + Fixes bug 20529; bugfix on 13942 commit 85bfad1 in 0.2.6.2-alpha. diff --git a/doc/tor.1.txt b/doc/tor.1.txt index b502e447d..e279460f8 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -2898,6 +2898,10 @@ __HiddenServiceDirectory__**/client_keys**:: Authorization data for a hidden service that is only accessible by authorized clients. +__HiddenServiceDirectory__**/onion_service_non_anonymous**:: + This file is present if a hidden service key was created in + **HiddenServiceNonAnonymousMode**. + SEE ALSO -------- **torsocks**(1), **torify**(1) + diff --git a/src/common/util.c b/src/common/util.c index 916296790..a7bce2ea6 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -2191,11 +2191,13 @@ file_status(const char *fname) } } -/** 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 - * 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. If CPD_GROUP_OK is set, then it's okay if the directory +/** Check whether dirname exists and is private. If yes return 0. + * If dirname does not exist: + * - if check&CPD_CREATE, try to create it and return 0 on success. + * - if check&CPD_CHECK, and we think we can create it, return 0. + * - if check&CPD_CHECK is false, and the directory exists, return 0. + * - otherwise, 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_GROUP_READ is set, existing directory behaves as CPD_GROUP_OK and * if the directory is created it will use mode 0750 with group read diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 3afe88055..56dbacdaf 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -72,6 +72,10 @@ static ssize_t rend_service_parse_intro_for_v3( size_t plaintext_len, char **err_msg_out); +static int rend_service_check_private_dir(const or_options_t *options, + const rend_service_t *s, + int create); + /** Represents the mapping from a virtual port of a rendezvous service to * a real port on some IP. */ @@ -462,6 +466,11 @@ rend_config_services(const or_options_t *options, int validate_only) for (line = options->RendConfigLines; line; line = line->next) { if (!strcasecmp(line->key, "HiddenServiceDir")) { if (service) { /* register the one we just finished parsing */ + if (rend_service_check_private_dir(options, service, 0) < 0) { + rend_service_free(service); + return -1; + } + if (validate_only) rend_service_free(service); else @@ -672,12 +681,7 @@ rend_config_services(const or_options_t *options, int validate_only) } } if (service) { - cpd_check_t check_opts = CPD_CHECK_MODE_ONLY|CPD_CHECK; - if (service->dir_group_readable) { - check_opts |= CPD_GROUP_READ; - } - - if (check_private_dir(service->directory, check_opts, options->User) < 0) { + if (rend_service_check_private_dir(options, service, 0) < 0) { rend_service_free(service); return -1; } @@ -1001,7 +1005,9 @@ service_is_single_onion_poisoned(const rend_service_t *service) fstatus = file_status(poison_fname); tor_free(poison_fname); - /* If this fname is occupied, the hidden service has been poisoned. */ + /* If this fname is occupied, the hidden service has been poisoned. + * fstatus can be FN_ERROR if the service directory does not exist, in that + * case, there is obviously no private key. */ if (fstatus == FN_FILE || fstatus == FN_EMPTY) { return 1; } @@ -1017,7 +1023,9 @@ rend_service_private_key_exists(const rend_service_t *service) char *private_key_path = rend_service_path(service, private_key_fname); const file_status_t private_key_status = file_status(private_key_path); tor_free(private_key_path); - /* Only non-empty regular private key files could have been used before. */ + /* Only non-empty regular private key files could have been used before. + * fstatus can be FN_ERROR if the service directory does not exist, in that + * case, there is obviously no private key. */ return private_key_status == FN_FILE; } @@ -1090,6 +1098,10 @@ poison_new_single_onion_hidden_service_dir(const rend_service_t *service) return -1; } + /* Make sure the directory exists */ + if (rend_service_check_private_dir(get_options(), service, 1) < 0) + return -1; + poison_fname = rend_service_sos_poison_path(service); switch (file_status(poison_fname)) { @@ -1245,6 +1257,37 @@ rend_service_derive_key_digests(struct rend_service_t *s) return 0; } +/** Make sure that the directory for s is private, using the config in + * options. + * If create is true: + * - if the directory exists, change permissions if needed, + * - if the directory does not exist, create it with the correct permissions. + * If create is false: + * - if the directory exists, check permissions, + * - if the directory does not exist, check if we think we can create it. + * Return 0 on success, -1 on failure. */ +static int +rend_service_check_private_dir(const or_options_t *options, + const rend_service_t *s, + int create) +{ + cpd_check_t check_opts = CPD_NONE; + if (create) { + check_opts |= CPD_CREATE; + } else { + check_opts |= CPD_CHECK_MODE_ONLY; + check_opts |= CPD_CHECK; + } + if (s->dir_group_readable) { + check_opts |= CPD_GROUP_READ; + } + /* Check/create directory */ + if (check_private_dir(s->directory, check_opts, options->User) < 0) { + return -1; + } + return 0; +} + /** Load and/or generate private keys for the hidden service s, * possibly including keys for client authorization. Return 0 on success, -1 * on failure. */ @@ -1253,23 +1296,9 @@ rend_service_load_keys(rend_service_t *s) { char *fname = NULL; char buf[128]; - cpd_check_t check_opts = CPD_CREATE; - if (s->dir_group_readable) { - check_opts |= CPD_GROUP_READ; - } - /* Check/create directory */ - if (check_private_dir(s->directory, check_opts, get_options()->User) < 0) { + if (rend_service_check_private_dir(get_options(), s, 1) < 0) goto err; - } -#ifndef _WIN32 - if (s->dir_group_readable) { - /* Only new dirs created get new opts, also enforce group read. */ - if (chmod(s->directory, 0750)) { - log_warn(LD_FS,"Unable to make %s group-readable.", s->directory); - } - } -#endif /* Load key */ fname = rend_service_path(s, private_key_fname); diff --git a/src/test/test.h b/src/test/test.h index 770f403ce..25336ac83 100644 --- a/src/test/test.h +++ b/src/test/test.h @@ -73,6 +73,7 @@ {print_ = (I64_PRINTF_TYPE) value_;}, {}, TT_EXIT_TEST_FUNCTION) const char *get_fname(const char *name); +const char *get_fname_rnd(const char *name); struct crypto_pk_t *pk_generate(int idx); #define US2_CONCAT_2__(a, b) a ## __ ## b diff --git a/src/test/test_hs.c b/src/test/test_hs.c index fd5ab1564..b42bc590c 100644 --- a/src/test/test_hs.c +++ b/src/test/test_hs.c @@ -518,6 +518,11 @@ mock_get_options(void) return mock_options; } +/* arg can't be 0 (the test fails) or 2 (the test is skipped) */ +#define CREATE_HS_DIR_NONE ((intptr_t)0x04) +#define CREATE_HS_DIR1 ((intptr_t)0x08) +#define CREATE_HS_DIR2 ((intptr_t)0x10) + /* Test that single onion poisoning works. */ static void test_single_onion_poisoning(void *arg) @@ -528,15 +533,15 @@ test_single_onion_poisoning(void *arg) MOCK(get_options, mock_get_options); int ret = -1; - mock_options->DataDirectory = tor_strdup(get_fname("test_data_dir")); + intptr_t create_dir_mask = (intptr_t)arg; + /* Get directories with a random suffix so we can repeat the tests */ + mock_options->DataDirectory = tor_strdup(get_fname_rnd("test_data_dir")); rend_service_t *service_1 = tor_malloc_zero(sizeof(rend_service_t)); - char *dir1 = tor_strdup(get_fname("test_hs_dir1")); + char *dir1 = tor_strdup(get_fname_rnd("test_hs_dir1")); rend_service_t *service_2 = tor_malloc_zero(sizeof(rend_service_t)); - char *dir2 = tor_strdup(get_fname("test_hs_dir2")); + char *dir2 = tor_strdup(get_fname_rnd("test_hs_dir2")); smartlist_t *services = smartlist_new(); - (void) arg; - /* No services, no problem! */ mock_options->HiddenServiceSingleHopMode = 0; mock_options->HiddenServiceNonAnonymousMode = 0; @@ -549,22 +554,20 @@ test_single_onion_poisoning(void *arg) ret = rend_service_list_verify_single_onion_poison(services, mock_options); tt_assert(ret == 0); - /* Create directories for both services */ - -#ifdef _WIN32 - ret = mkdir(mock_options->DataDirectory); - tt_assert(ret == 0); - ret = mkdir(dir1); - tt_assert(ret == 0); - ret = mkdir(dir2); -#else - ret = mkdir(mock_options->DataDirectory, 0700); - tt_assert(ret == 0); - ret = mkdir(dir1, 0700); - tt_assert(ret == 0); - ret = mkdir(dir2, 0700); -#endif + /* Create the data directory, and, if the correct bit in arg is set, + * create a directory for that service. + * The data directory is required for the lockfile, which is used when + * loading keys. */ + ret = check_private_dir(mock_options->DataDirectory, CPD_CREATE, NULL); tt_assert(ret == 0); + if (create_dir_mask & CREATE_HS_DIR1) { + ret = check_private_dir(dir1, CPD_CREATE, NULL); + tt_assert(ret == 0); + } + if (create_dir_mask & CREATE_HS_DIR2) { + ret = check_private_dir(dir2, CPD_CREATE, NULL); + tt_assert(ret == 0); + } service_1->directory = dir1; service_2->directory = dir2; @@ -694,7 +697,7 @@ test_single_onion_poisoning(void *arg) tt_assert(ret < 0); done: - /* TODO: should we delete the directories here? */ + /* The test harness deletes the directories at exit */ rend_service_free(service_1); rend_service_free(service_2); smartlist_free(services); @@ -716,8 +719,14 @@ struct testcase_t hs_tests[] = { NULL, NULL }, { "hs_auth_cookies", test_hs_auth_cookies, TT_FORK, NULL, NULL }, - { "single_onion_poisoning", test_single_onion_poisoning, TT_FORK, - NULL, NULL }, + { "single_onion_poisoning_create_dir_none", test_single_onion_poisoning, + TT_FORK, &passthrough_setup, (void*)(CREATE_HS_DIR_NONE) }, + { "single_onion_poisoning_create_dir1", test_single_onion_poisoning, + TT_FORK, &passthrough_setup, (void*)(CREATE_HS_DIR1) }, + { "single_onion_poisoning_create_dir2", test_single_onion_poisoning, + TT_FORK, &passthrough_setup, (void*)(CREATE_HS_DIR2) }, + { "single_onion_poisoning_create_dir_both", test_single_onion_poisoning, + TT_FORK, &passthrough_setup, (void*)(CREATE_HS_DIR1 | CREATE_HS_DIR2) }, END_OF_TESTCASES }; diff --git a/src/test/testing_common.c b/src/test/testing_common.c index fd7c4e707..9c6580f78 100644 --- a/src/test/testing_common.c +++ b/src/test/testing_common.c @@ -102,18 +102,41 @@ setup_directory(void) temp_dir_setup_in_pid = getpid(); } -/** Return a filename relative to our testing temporary directory */ -const char * -get_fname(const char *name) +/** Return a filename relative to our testing temporary directory, based on + * name and suffix. If name is NULL, return the name of the testing temporary + * directory. */ +static const char * +get_fname_suffix(const char *name, const char *suffix) { static char buf[1024]; setup_directory(); if (!name) return temp_dir; - tor_snprintf(buf,sizeof(buf),"%s/%s",temp_dir,name); + tor_snprintf(buf,sizeof(buf),"%s/%s%s%s",temp_dir,name,suffix ? "_" : "", + suffix ? suffix : ""); return buf; } +/** Return a filename relative to our testing temporary directory. If name is + * NULL, return the name of the testing temporary directory. */ +const char * +get_fname(const char *name) +{ + return get_fname_suffix(name, NULL); +} + +/** Return a filename with a random suffix, relative to our testing temporary + * directory. If name is NULL, return the name of the testing temporary + * directory, without any suffix. */ +const char * +get_fname_rnd(const char *name) +{ + char rnd[256], rnd32[256]; + crypto_rand(rnd, RAND_PATH_BYTES); + base32_encode(rnd32, sizeof(rnd32), rnd, RAND_PATH_BYTES); + return get_fname_suffix(name, rnd32); +} + /* Remove a directory and all of its subdirectories */ static void rm_rf(const char *dir) @@ -217,6 +240,9 @@ free_pregenerated_keys(void) static void * passthrough_test_setup(const struct testcase_t *testcase) { + /* Make sure the passthrough doesn't unintentionally fail or skip tests */ + tor_assert(testcase->setup_data); + tor_assert(testcase->setup_data != (void*)TT_SKIP); return testcase->setup_data; } static int