>From ca7ac1cebf9f13be9b7ffc56a618dbcbaf0c6b60 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Fri, 24 Jul 2015 13:12:43 +0200 Subject: [PATCH 1/2] UTIL: New utility function sss_unique_filename() to create a random filename Instead of copying the same code using mkstemp over and over, provide a common version. --- src/providers/ad/ad_gpo_child.c | 8 +-- src/providers/krb5/krb5_child.c | 37 ++++++-------- src/providers/krb5/krb5_common.c | 8 +-- src/providers/ldap/ldap_child.c | 15 ++---- src/responder/ssh/sshsrv_cmd.c | 6 +-- src/tests/cmocka/test_utils.c | 103 +++++++++++++++++++++++++++++++++++++++ src/util/util.c | 52 ++++++++++++++++++++ src/util/util.h | 13 +++++ 8 files changed, 190 insertions(+), 52 deletions(-) diff --git a/src/providers/ad/ad_gpo_child.c b/src/providers/ad/ad_gpo_child.c index 03951af041463000d51695fe1a86609ed6c7c1c2..e0e0c068ff496bf28ec9fa4ab7b2105238719217 100644 --- a/src/providers/ad/ad_gpo_child.c +++ b/src/providers/ad/ad_gpo_child.c @@ -276,7 +276,6 @@ static errno_t gpo_cache_store_file(const char *smb_path, int fd = -1; char *tmp_name = NULL; ssize_t written; - mode_t old_umask; char *filename = NULL; char *smb_path_with_suffix = NULL; TALLOC_CTX *tmp_ctx = NULL; @@ -318,13 +317,10 @@ static errno_t gpo_cache_store_file(const char *smb_path, goto done; } - old_umask = umask(077); - fd = mkstemp(tmp_name); - umask(old_umask); + fd = sss_unique_file(tmp_name, &ret); if (fd == -1) { - ret = errno; DEBUG(SSSDBG_CRIT_FAILURE, - "mkstemp failed [%d][%s].\n", ret, strerror(ret)); + "sss_unique_file failed [%d][%s].\n", ret, strerror(ret)); goto done; } diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index 2c5e446a0e0c3f55261a39d8d3f3bc09aded3cb9..a37f912b64ec09f7c7f232ad6efd64926bfa8e74 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -668,11 +668,8 @@ done: static errno_t handle_randomized(char *in) { - size_t ccname_len; char *ccname = NULL; int ret; - int fd; - mode_t old_umask; /* We only treat the FILE type case in a special way due to the history * of storing FILE type ccache in /tmp and associated security issues */ @@ -684,26 +681,20 @@ static errno_t handle_randomized(char *in) return EOK; } - ccname_len = strlen(ccname); - if (ccname_len >= 6 && strcmp(ccname + (ccname_len - 6), "XXXXXX") == 0) { - /* NOTE: this call is only used to create a unique name, as later - * krb5_cc_initialize() will unlink and recreate the file. - * This is ok because this part of the code is called with - * privileges already dropped when handling user ccache, or the ccache - * is stored in a private directory. So we do not have huge issues if - * something races, we mostly care only about not accidentally use - * an existing name and thus failing in the process of saving the - * cache. Malicious races can only be avoided by libkrb5 itself. */ - old_umask = umask(077); - fd = mkstemp(ccname); - umask(old_umask); - if (fd == -1) { - ret = errno; - DEBUG(SSSDBG_CRIT_FAILURE, - "mkstemp(\"%s\") failed [%d]: %s!\n", - ccname, ret, strerror(ret)); - return ret; - } + /* NOTE: this call is only used to create a unique name, as later + * krb5_cc_initialize() will unlink and recreate the file. + * This is ok because this part of the code is called with + * privileges already dropped when handling user ccache, or the ccache + * is stored in a private directory. So we do not have huge issues if + * something races, we mostly care only about not accidentally use + * an existing name and thus failing in the process of saving the + * cache. Malicious races can only be avoided by libkrb5 itself. */ + ret = sss_unique_filename(ccname); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, + "mkstemp(\"%s\") failed [%d]: %s!\n", + ccname, ret, strerror(ret)); + return ret; } return EOK; diff --git a/src/providers/krb5/krb5_common.c b/src/providers/krb5/krb5_common.c index 81d4048b63dba98706bbef1936df7f10f79e1ae5..9d3f1a20193285b49f7a55fef5deb806780cc662 100644 --- a/src/providers/krb5/krb5_common.c +++ b/src/providers/krb5/krb5_common.c @@ -419,7 +419,6 @@ errno_t write_krb5info_file(const char *realm, const char *server, const char *name_tmpl = NULL; size_t server_len; ssize_t written; - mode_t old_umask; if (realm == NULL || *realm == '\0' || server == NULL || *server == '\0' || service == NULL || *service == '\0') { @@ -459,13 +458,10 @@ errno_t write_krb5info_file(const char *realm, const char *server, goto done; } - old_umask = umask(077); - fd = mkstemp(tmp_name); - umask(old_umask); + fd = sss_unique_file(tmp_name, &ret); if (fd == -1) { - ret = errno; DEBUG(SSSDBG_CRIT_FAILURE, - "mkstemp failed [%d][%s].\n", ret, strerror(ret)); + "sss_unique_file failed [%d][%s].\n", ret, strerror(ret)); goto done; } diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c index 82481d6e75c86f7be49625a669691b235589d9a7..338345d89fa0484eaa8fcf99f1e0f10eb61a1d91 100644 --- a/src/providers/ldap/ldap_child.c +++ b/src/providers/ldap/ldap_child.c @@ -254,7 +254,6 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx, const char **ccname_out, time_t *expire_time_out) { - int fd; char *ccname; char *ccname_dummy; char *realm_name = NULL; @@ -274,7 +273,6 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx, TALLOC_CTX *tmp_ctx; char *ccname_file_dummy = NULL; char *ccname_file; - mode_t old_umask; tmp_ctx = talloc_new(memctx); if (tmp_ctx == NULL) { @@ -408,21 +406,14 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx, goto done; } - old_umask = umask(077); - fd = mkstemp(ccname_file_dummy); - umask(old_umask); - if (fd == -1) { - ret = errno; + ret = sss_unique_filename(ccname_file_dummy); + if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, - "mkstemp failed: %s:[%d].\n", + "sss_unique_filename failed: %s:[%d].\n", strerror(ret), ret); krberr = KRB5KRB_ERR_GENERIC; goto done; } - /* We only care about creating a unique file name here, we don't - * need the fd - */ - close(fd); krberr = krb5_get_init_creds_keytab(context, &my_creds, kprinc, keytab, 0, NULL, &options); diff --git a/src/responder/ssh/sshsrv_cmd.c b/src/responder/ssh/sshsrv_cmd.c index 4833587910cade32ecb0b5f65b417d58a498b01e..8c650d04030ed6984f064448320df2cddc66f6e8 100644 --- a/src/responder/ssh/sshsrv_cmd.c +++ b/src/responder/ssh/sshsrv_cmd.c @@ -555,7 +555,6 @@ ssh_host_pubkeys_update_known_hosts(struct ssh_cmd_ctx *cmd_ctx) char *filename = NULL; char *entstr; ssize_t wret; - mode_t old_mask; tmp_ctx = talloc_new(NULL); if (!tmp_ctx) { @@ -578,12 +577,9 @@ ssh_host_pubkeys_update_known_hosts(struct ssh_cmd_ctx *cmd_ctx) goto done; } - old_mask = umask(0133); - fd = mkstemp(filename); - umask(old_mask); + fd = sss_unique_file_ex(filename, 0133, &ret); if (fd == -1) { filename = NULL; - ret = errno; goto done; } diff --git a/src/tests/cmocka/test_utils.c b/src/tests/cmocka/test_utils.c index d3d00feda0bdd4048519f90ba48ae9d1042a95a1..94aa7675f3024d573fe75cdfedca37f6800b0513 100644 --- a/src/tests/cmocka/test_utils.c +++ b/src/tests/cmocka/test_utils.c @@ -1212,6 +1212,102 @@ void test_fix_domain_in_name_list(void **state) talloc_free(dom); } +struct unique_file_test_ctx { + char *filename; +}; + +static int unique_file_test_setup(void **state) +{ + struct unique_file_test_ctx *test_ctx; + + assert_true(leak_check_setup()); + check_leaks_push(global_talloc_context); + + test_ctx = talloc_zero(global_talloc_context, struct unique_file_test_ctx); + assert_non_null(test_ctx); + + test_ctx->filename = talloc_strdup(test_ctx, "test_unique_file_XXXXXX"); + assert_non_null(test_ctx); + + *state = test_ctx; + return 0; +} + +static int unique_file_test_teardown(void **state) +{ + struct unique_file_test_ctx *test_ctx; + errno_t ret; + + test_ctx = talloc_get_type(*state, struct unique_file_test_ctx); + + errno = 0; + ret = unlink(test_ctx->filename); + if (ret != 0 && errno != ENOENT) { + fail(); + } + + talloc_free(test_ctx); + assert_true(check_leaks_pop(global_talloc_context) == true); + assert_true(leak_check_teardown()); + return 0; +} + +static void test_sss_unique_file(void **state) +{ + struct unique_file_test_ctx *test_ctx; + int fd; + errno_t ret; + struct stat sb; + + test_ctx = talloc_get_type(*state, struct unique_file_test_ctx); + + fd = sss_unique_file(test_ctx->filename, &ret); + assert_int_not_equal(fd, -1); + assert_int_equal(ret, EOK); + + ret = check_fd(fd, geteuid(), getegid(), + (S_IRUSR | S_IWUSR | S_IFREG), 0, &sb); + close(fd); + assert_int_equal(ret, EOK); +} + +static void test_sss_unique_file_neg(void **state) +{ + int fd; + errno_t ret; + + fd = sss_unique_file(discard_const("badpattern"), &ret); + assert_int_equal(fd, -1); + assert_int_equal(ret, EINVAL); +} + +static void test_sss_unique_filename(void **state) +{ + struct unique_file_test_ctx *test_ctx; + int fd; + errno_t ret; + char *tmp_filename; + + test_ctx = talloc_get_type(*state, struct unique_file_test_ctx); + + tmp_filename = talloc_strdup(test_ctx, test_ctx->filename); + assert_non_null(tmp_filename); + + ret = sss_unique_filename(test_ctx->filename); + assert_int_equal(ret, EOK); + + assert_int_equal(strncmp(test_ctx->filename, + tmp_filename, + strlen(tmp_filename) - sizeof("XXXXXX")), + 0); + talloc_free(tmp_filename); + + ret = check_and_open_readonly(test_ctx->filename, &fd, + geteuid(), getegid(), + (S_IRUSR | S_IWUSR | S_IFREG), 0); + close(fd); +} + int main(int argc, const char *argv[]) { poptContext pc; @@ -1275,6 +1371,13 @@ int main(int argc, const char *argv[]) cmocka_unit_test_setup_teardown(test_fix_domain_in_name_list, confdb_test_setup, confdb_test_teardown), + cmocka_unit_test_setup_teardown(test_sss_unique_file, + unique_file_test_setup, + unique_file_test_teardown), + cmocka_unit_test(test_sss_unique_file_neg), + cmocka_unit_test_setup_teardown(test_sss_unique_filename, + unique_file_test_setup, + unique_file_test_teardown), }; /* Set debug level to invalid value so we can deside if -d 0 was used. */ diff --git a/src/util/util.c b/src/util/util.c index 782cd026b7928e607a8980fb5f333c794feb5b1a..4dafbab2d41d7eb27eea62c766866816be48074c 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -979,3 +979,55 @@ errno_t sss_utc_to_time_t(const char *str, const char *format, time_t *_unix_tim *_unix_time = ut; return EOK; } + +int sss_unique_file_ex(char *path_tmpl, mode_t file_umask, errno_t *_err) +{ + size_t tmpl_len; + errno_t ret; + int fd = -1; + mode_t old_umask; + + tmpl_len = strlen(path_tmpl); + if (tmpl_len < 6 || strcmp(path_tmpl + (tmpl_len - 6), "XXXXXX") != 0) { + DEBUG(SSSDBG_OP_FAILURE, + "Template too short or doesn't end with XXXXXX!\n"); + ret = EINVAL; + goto done; + } + + old_umask = umask(file_umask); + fd = mkstemp(path_tmpl); + umask(old_umask); + if (fd == -1) { + ret = errno; + DEBUG(SSSDBG_OP_FAILURE, + "mkstemp(\"%s\") failed [%d]: %s!\n", + path_tmpl, ret, strerror(ret)); + goto done; + } + + ret = EOK; +done: + if (_err) { + *_err = ret; + } + return fd; +} + +int sss_unique_file(char *path_tmpl, errno_t *_err) +{ + return sss_unique_file_ex(path_tmpl, 077, _err); +} + +errno_t sss_unique_filename(char *path_tmpl) +{ + int fd; + errno_t ret; + + fd = sss_unique_file(path_tmpl, &ret); + /* We only care about a unique file name */ + if (fd >= 0) { + close(fd); + } + return ret; +} diff --git a/src/util/util.h b/src/util/util.h index 94a3ddea839f0998cb7796f1d2fe13f743de3aaf..ab72eed85fb9457f50acce9ba009425c1b04e59d 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -658,4 +658,17 @@ int get_seuser(TALLOC_CTX *mem_ctx, const char *login_name, /* convert time from generalized form to unix time */ errno_t sss_utc_to_time_t(const char *str, const char *format, time_t *unix_time); +/* Creates a unique file using mkstemp with provided umask. The template + * must end with XXXXXX. Returns the fd, sets _err to an errno value on error. + * + * Prefer using sss_unique_file() as it uses a secure umask internally. + */ +int sss_unique_file_ex(char *path_tmpl, mode_t file_umask, errno_t *_err); +int sss_unique_file(char *path_tmpl, errno_t *_err); + +/* Creates a unique filename using mkstemp with secure umask. The template + * must end with XXXXXX + */ +int sss_unique_filename(char *path_tmpl); + #endif /* __SSSD_UTIL_H__ */ -- 2.4.3