From f72856736ac39c7e926c02c11f854f43400366d4 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Sat, 18 Oct 2014 22:03:01 +0200 Subject: [PATCH 86/92] KRB5: Move checking for illegal RE to krb5_utils.c MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Otherwise we would have to link krb5_child with pcre and transfer the regex, which would be cumbersome. Check for illegal patterns when expanding the template instead. Related: https://fedorahosted.org/sssd/ticket/2370 Reviewed-by: Sumit Bose Reviewed-by: Lukáš Slebodník --- src/providers/krb5/krb5_auth.c | 5 +-- src/providers/krb5/krb5_ccache.c | 38 ++------------------ src/providers/krb5/krb5_ccache.h | 7 +--- src/providers/krb5/krb5_utils.c | 36 +++++++++++++++++-- src/providers/krb5/krb5_utils.h | 4 +-- src/tests/krb5_child-test.c | 2 +- src/tests/krb5_utils-tests.c | 78 ++++++++++++++++------------------------ 7 files changed, 73 insertions(+), 97 deletions(-) diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c index bd8b51f47462f1eaef8da61b42caedda3475a4e7..5ed561601ac80e53ee795b458c5bf0ca410951bc 100644 --- a/src/providers/krb5/krb5_auth.c +++ b/src/providers/krb5/krb5_auth.c @@ -302,7 +302,9 @@ static errno_t krb5_auth_prepare_ccache_name(struct krb5child_req *kr, DEBUG(SSSDBG_TRACE_ALL, "Recreating ccache file.\n"); ccname_template = dp_opt_get_cstring(kr->krb5_ctx->opts, KRB5_CCNAME_TMPL); - kr->ccname = expand_ccname_template(kr, kr, ccname_template, true, + kr->ccname = expand_ccname_template(kr, kr, ccname_template, + kr->krb5_ctx->illegal_path_re, + true, be_ctx->domain->case_sensitive); if (kr->ccname == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "expand_ccname_template failed.\n"); @@ -310,7 +312,6 @@ static errno_t krb5_auth_prepare_ccache_name(struct krb5child_req *kr, } ret = sss_krb5_precreate_ccache(kr->ccname, - kr->krb5_ctx->illegal_path_re, kr->uid, kr->gid); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "ccache creation failed.\n"); diff --git a/src/providers/krb5/krb5_ccache.c b/src/providers/krb5/krb5_ccache.c index 5586963338616519f36e5d75e796a597d3ac2f22..c0f5b7b8ced3fd2d6d8cbbf4e3339caba60888ff 100644 --- a/src/providers/krb5/krb5_ccache.c +++ b/src/providers/krb5/krb5_ccache.c @@ -33,28 +33,6 @@ #include "util/sss_krb5.h" #include "util/util.h" -static errno_t -check_ccache_re(const char *filename, pcre *illegal_re) -{ - errno_t ret; - - ret = pcre_exec(illegal_re, NULL, filename, strlen(filename), - 0, 0, NULL, 0); - if (ret == 0) { - DEBUG(SSSDBG_OP_FAILURE, - "Illegal pattern in ccache directory name [%s].\n", filename); - return EINVAL; - } else if (ret == PCRE_ERROR_NOMATCH) { - DEBUG(SSSDBG_TRACE_LIBS, - "Ccache directory name [%s] does not contain " - "illegal patterns.\n", filename); - return EOK; - } - - DEBUG(SSSDBG_CRIT_FAILURE, "pcre_exec failed [%d].\n", ret); - return EFAULT; -} - struct string_list { struct string_list *next; struct string_list *prev; @@ -162,9 +140,7 @@ static errno_t check_parent_stat(struct stat *parent_stat, uid_t uid) return EOK; } -errno_t create_ccache_dir(const char *ccdirname, - pcre *illegal_re, - uid_t uid, gid_t gid) +static errno_t create_ccache_dir(const char *ccdirname, uid_t uid, gid_t gid) { int ret = EFAULT; struct stat parent_stat; @@ -188,13 +164,6 @@ errno_t create_ccache_dir(const char *ccdirname, goto done; } - if (illegal_re != NULL) { - ret = check_ccache_re(ccdirname, illegal_re); - if (ret != EOK) { - goto done; - } - } - ret = find_ccdir_parent_data(tmp_ctx, ccdirname, &parent_stat, &missing_parents); if (ret != EOK) { @@ -242,8 +211,7 @@ done: return ret; } -errno_t sss_krb5_precreate_ccache(const char *ccname, pcre *illegal_re, - uid_t uid, gid_t gid) +errno_t sss_krb5_precreate_ccache(const char *ccname, uid_t uid, gid_t gid) { TALLOC_CTX *tmp_ctx = NULL; const char *filename; @@ -287,7 +255,7 @@ errno_t sss_krb5_precreate_ccache(const char *ccname, pcre *illegal_re, *end = '\0'; } while (*(end+1) == '\0'); - ret = create_ccache_dir(ccdirname, illegal_re, uid, gid); + ret = create_ccache_dir(ccdirname, uid, gid); done: talloc_free(tmp_ctx); return ret; diff --git a/src/providers/krb5/krb5_ccache.h b/src/providers/krb5/krb5_ccache.h index 9f0b3ac84b7af118c315ca00a7c52f200534d97e..e39f96cad6f46c4003103dce4eadf007bc0f8920 100644 --- a/src/providers/krb5/krb5_ccache.h +++ b/src/providers/krb5/krb5_ccache.h @@ -35,12 +35,7 @@ struct tgt_times { time_t renew_till; }; -errno_t create_ccache_dir(const char *ccdirname, - pcre *illegal_re, - uid_t uid, gid_t gid); - -errno_t sss_krb5_precreate_ccache(const char *ccname, pcre *illegal_re, - uid_t uid, gid_t gid); +errno_t sss_krb5_precreate_ccache(const char *ccname, uid_t uid, gid_t gid); errno_t sss_krb5_cc_destroy(const char *ccname, uid_t uid, gid_t gid); diff --git a/src/providers/krb5/krb5_utils.c b/src/providers/krb5/krb5_utils.c index ae72b04be236cfce9b6f794c602887491ba487a9..de2d94503744b80b0a3365efb227cd05434579ff 100644 --- a/src/providers/krb5/krb5_utils.c +++ b/src/providers/krb5/krb5_utils.c @@ -202,9 +202,31 @@ done: #define S_EXP_USERNAME "{username}" #define L_EXP_USERNAME (sizeof(S_EXP_USERNAME) - 1) +static errno_t +check_ccache_re(const char *filename, pcre *illegal_re) +{ + errno_t ret; + + ret = pcre_exec(illegal_re, NULL, filename, strlen(filename), + 0, 0, NULL, 0); + if (ret == 0) { + DEBUG(SSSDBG_OP_FAILURE, + "Illegal pattern in ccache directory name [%s].\n", filename); + return EINVAL; + } else if (ret == PCRE_ERROR_NOMATCH) { + DEBUG(SSSDBG_TRACE_LIBS, + "Ccache directory name [%s] does not contain " + "illegal patterns.\n", filename); + return EOK; + } + + DEBUG(SSSDBG_CRIT_FAILURE, "pcre_exec failed [%d].\n", ret); + return EFAULT; +} + char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr, - const char *template, bool file_mode, - bool case_sensitive) + const char *template, pcre *illegal_re, + bool file_mode, bool case_sensitive) { char *copy; char *p; @@ -217,6 +239,7 @@ char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr, TALLOC_CTX *tmp_ctx = NULL; char action; bool rerun; + int ret; if (template == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "Missing template.\n"); @@ -320,7 +343,7 @@ char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr, } dummy = expand_ccname_template(tmp_ctx, kr, cache_dir_tmpl, - false, case_sensitive); + illegal_re, false, case_sensitive); if (dummy == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "Expanding credential cache directory " @@ -411,6 +434,13 @@ char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr, goto done; } + if (illegal_re != NULL) { + ret = check_ccache_re(result, illegal_re); + if (ret != EOK) { + goto done; + } + } + res = talloc_move(mem_ctx, &result); done: talloc_zfree(tmp_ctx); diff --git a/src/providers/krb5/krb5_utils.h b/src/providers/krb5/krb5_utils.h index ce5ce1ebcf6db14579191840600e684d41a2fdbe..0155905b5bc7469d09aecbd51cae0e8cc61b3952 100644 --- a/src/providers/krb5/krb5_utils.h +++ b/src/providers/krb5/krb5_utils.h @@ -43,8 +43,8 @@ errno_t check_if_cached_upn_needs_update(struct sysdb_ctx *sysdb, const char *upn); char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr, - const char *template, bool file_mode, - bool case_sensitive); + const char *template, pcre *illegal_re, + bool file_mode, bool case_sensitive); errno_t get_domain_or_subdomain(struct be_ctx *be_ctx, char *domain_name, diff --git a/src/tests/krb5_child-test.c b/src/tests/krb5_child-test.c index 09f23d5386e3c70efc5ce54fa199c1a6e8656eec..8826a28ed5ea064317c62682003dc0e9a6df01b6 100644 --- a/src/tests/krb5_child-test.c +++ b/src/tests/krb5_child-test.c @@ -239,6 +239,7 @@ create_dummy_req(TALLOC_CTX *mem_ctx, const char *user, kr->ccname = expand_ccname_template(kr, kr, dp_opt_get_cstring(kr->krb5_ctx->opts, KRB5_CCNAME_TMPL), + kr->krb5_ctx->illegal_path_re, true, true); if (!kr->ccname) goto fail; @@ -254,7 +255,6 @@ create_dummy_req(TALLOC_CTX *mem_ctx, const char *user, kr->ccname, kr->uid, kr->gid); ret = sss_krb5_precreate_ccache(kr->ccname, - kr->krb5_ctx->illegal_path_re, kr->uid, kr->gid); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "create_ccache_dir failed.\n"); diff --git a/src/tests/krb5_utils-tests.c b/src/tests/krb5_utils-tests.c index 52d8a18576b23c627c7ef3358bd34f4b2dbae6f7..409c0f01d2cce9c24a648306007b9fa7f5bc8372 100644 --- a/src/tests/krb5_utils-tests.c +++ b/src/tests/krb5_utils-tests.c @@ -131,13 +131,13 @@ START_TEST(test_private_ccache_dir_in_user_dir) ret = chmod(user_dir, 0600); fail_unless(ret == EOK, "chmod failed."); - ret = sss_krb5_precreate_ccache(filename, NULL, uid, gid); + ret = sss_krb5_precreate_ccache(filename, uid, gid); fail_unless(ret == EINVAL, "sss_krb5_precreate_ccache does not return EINVAL " "while x-bit is missing."); ret = chmod(user_dir, 0700); fail_unless(ret == EOK, "chmod failed."); - ret = sss_krb5_precreate_ccache(filename, NULL, uid, gid); + ret = sss_krb5_precreate_ccache(filename, uid, gid); fail_unless(ret == EOK, "sss_krb5_precreate_ccache failed."); check_dir(dn3, uid, gid, 0700); @@ -175,7 +175,7 @@ START_TEST(test_private_ccache_dir_in_wrong_user_dir) filename = talloc_asprintf(tmp_ctx, "%s/ccfile", subdirname); fail_unless(filename != NULL, "talloc_asprintf failed."); - ret = sss_krb5_precreate_ccache(filename, NULL, 12345, 12345); + ret = sss_krb5_precreate_ccache(filename, 12345, 12345); fail_unless(ret == EINVAL, "Creating private ccache dir in wrong user " "dir does not failed with EINVAL."); @@ -185,16 +185,14 @@ END_TEST START_TEST(test_illegal_patterns) { - int ret; char *cwd; char *dirname; char *filename; - uid_t uid = getuid(); - gid_t gid = getgid(); pcre *illegal_re; const char *errstr; int errval; int errpos; + char *result = NULL; illegal_re = pcre_compile2(ILLEGAL_PATH_PATTERN, 0, &errval, &errstr, &errpos, NULL); @@ -209,33 +207,28 @@ START_TEST(test_illegal_patterns) free(cwd); fail_unless(dirname != NULL, "talloc_asprintf failed."); - - filename = talloc_asprintf(tmp_ctx, "abc/./ccfile"); - fail_unless(filename != NULL, "talloc_asprintf failed."); - ret = create_ccache_dir(filename, illegal_re, uid, gid); - fail_unless(ret == EINVAL, "create_ccache_dir allowed relative path [%s].", - filename); + result = expand_ccname_template(tmp_ctx, kr, "abc/./ccfile", illegal_re, true, true); + fail_unless(result == NULL, "expand_ccname_template allowed relative path\n"); filename = talloc_asprintf(tmp_ctx, "%s/abc/./ccfile", dirname); fail_unless(filename != NULL, "talloc_asprintf failed."); - ret = create_ccache_dir(filename, illegal_re, uid, gid); - fail_unless(ret == EINVAL, "create_ccache_dir allowed " - "illegal pattern '/./' in filename [%s].", - filename); + result = expand_ccname_template(tmp_ctx, kr, filename, illegal_re, true, true); + fail_unless(result == NULL, "expand_ccname_template allowed " + "illegal pattern '/./'\n"); filename = talloc_asprintf(tmp_ctx, "%s/abc/../ccfile", dirname); fail_unless(filename != NULL, "talloc_asprintf failed."); - ret = create_ccache_dir(filename, illegal_re, uid, gid); - fail_unless(ret == EINVAL, "create_ccache_dir allowed " - "illegal pattern '/../' in filename [%s].", - filename); + result = expand_ccname_template(tmp_ctx, kr, filename, illegal_re, true, true); + fail_unless(result == NULL, "expand_ccname_template allowed " + "illegal pattern '/../' in filename [%s].", + filename); filename = talloc_asprintf(tmp_ctx, "%s/abc//ccfile", dirname); fail_unless(filename != NULL, "talloc_asprintf failed."); - ret = create_ccache_dir(filename, illegal_re, uid, gid); - fail_unless(ret == EINVAL, "create_ccache_dir allowed " - "illegal pattern '//' in filename [%s].", - filename); + result = expand_ccname_template(tmp_ctx, kr, filename, illegal_re, true, true); + fail_unless(result == NULL, "expand_ccname_template allowed " + "illegal pattern '//' in filename [%s].", + filename); pcre_free(illegal_re); } @@ -248,17 +241,7 @@ START_TEST(test_cc_dir_create) char *cwd; uid_t uid = getuid(); gid_t gid = getgid(); - pcre *illegal_re; errno_t ret; - const char *errstr; - int errval; - int errpos; - - illegal_re = pcre_compile2(ILLEGAL_PATH_PATTERN, 0, - &errval, &errstr, &errpos, NULL); - fail_unless(illegal_re != NULL, "Invalid Regular Expression pattern at " - " position %d. (Error: %d [%s])\n", - errpos, errval, errstr); cwd = getcwd(NULL, 0); fail_unless(cwd != NULL, "getcwd failed."); @@ -269,7 +252,7 @@ START_TEST(test_cc_dir_create) residual = talloc_asprintf(tmp_ctx, "DIR:%s/%s", dirname, "ccdir"); fail_unless(residual != NULL, "talloc_asprintf failed."); - ret = sss_krb5_precreate_ccache(residual, illegal_re, uid, gid); + ret = sss_krb5_precreate_ccache(residual, uid, gid); fail_unless(ret == EOK, "sss_krb5_precreate_ccache failed\n"); ret = rmdir(dirname); if (ret < 0) ret = errno; @@ -282,14 +265,13 @@ START_TEST(test_cc_dir_create) residual = talloc_asprintf(tmp_ctx, "DIR:%s/%s", dirname, "ccdir/"); fail_unless(residual != NULL, "talloc_asprintf failed."); - ret = sss_krb5_precreate_ccache(residual, illegal_re, uid, gid); + ret = sss_krb5_precreate_ccache(residual, uid, gid); fail_unless(ret == EOK, "sss_krb5_precreate_ccache failed\n"); ret = rmdir(dirname); if (ret < 0) ret = errno; fail_unless(ret == 0, "Cannot remove %s: %s\n", dirname, strerror(ret)); talloc_free(residual); free(cwd); - pcre_free(illegal_re); } END_TEST @@ -356,7 +338,7 @@ static void do_test(const char *file_template, const char *dir_template, ret = dp_opt_set_string(kr->krb5_ctx->opts, KRB5_CCACHEDIR, dir_template); fail_unless(ret == EOK, "Failed to set Ccache dir"); - result = expand_ccname_template(tmp_ctx, kr, file_template, true, true); + result = expand_ccname_template(tmp_ctx, kr, file_template, NULL, true, true); fail_unless(result != NULL, "Cannot expand template [%s].", file_template); fail_unless(strcmp(result, expected) == 0, @@ -391,14 +373,14 @@ START_TEST(test_case_sensitive) ret = dp_opt_set_string(kr->krb5_ctx->opts, KRB5_CCACHEDIR, CCACHE_DIR); fail_unless(ret == EOK, "Failed to set Ccache dir"); - result = expand_ccname_template(tmp_ctx, kr, file_template, true, true); + result = expand_ccname_template(tmp_ctx, kr, file_template, NULL, true, true); fail_unless(result != NULL, "Cannot expand template [%s].", file_template); fail_unless(strcmp(result, expected_cs) == 0, "Expansion failed, result [%s], expected [%s].", result, expected_cs); - result = expand_ccname_template(tmp_ctx, kr, file_template, true, false); + result = expand_ccname_template(tmp_ctx, kr, file_template, NULL, true, false); fail_unless(result != NULL, "Cannot expand template [%s].", file_template); fail_unless(strcmp(result, expected_ci) == 0, @@ -445,7 +427,7 @@ START_TEST(test_ccache_dir) ret = dp_opt_set_string(kr->krb5_ctx->opts, KRB5_CCACHEDIR, BASE"_%d"); fail_unless(ret == EOK, "Failed to set Ccache dir"); - result = expand_ccname_template(tmp_ctx, kr, "%d/"FILENAME, true, true); + result = expand_ccname_template(tmp_ctx, kr, "%d/"FILENAME, NULL, true, true); fail_unless(result == NULL, "Using %%d in ccache dir should fail."); } @@ -461,7 +443,7 @@ START_TEST(test_pid) ret = dp_opt_set_string(kr->krb5_ctx->opts, KRB5_CCACHEDIR, BASE"_%P"); fail_unless(ret == EOK, "Failed to set Ccache dir"); - result = expand_ccname_template(tmp_ctx, kr, "%d/"FILENAME, true, true); + result = expand_ccname_template(tmp_ctx, kr, "%d/"FILENAME, NULL, true, true); fail_unless(result == NULL, "Using %%P in ccache dir should fail."); } @@ -480,7 +462,7 @@ START_TEST(test_unknown_template) char *result; int ret; - result = expand_ccname_template(tmp_ctx, kr, test_template, true, true); + result = expand_ccname_template(tmp_ctx, kr, test_template, NULL, true, true); fail_unless(result == NULL, "Unknown template [%s] should fail.", test_template); @@ -488,7 +470,7 @@ START_TEST(test_unknown_template) ret = dp_opt_set_string(kr->krb5_ctx->opts, KRB5_CCACHEDIR, BASE"_%X"); fail_unless(ret == EOK, "Failed to set Ccache dir"); test_template = "%d/"FILENAME; - result = expand_ccname_template(tmp_ctx, kr, test_template, true, true); + result = expand_ccname_template(tmp_ctx, kr, test_template, NULL, true, true); fail_unless(result == NULL, "Unknown template [%s] should fail.", test_template); @@ -500,7 +482,7 @@ START_TEST(test_NULL) char *test_template = NULL; char *result; - result = expand_ccname_template(tmp_ctx, kr, test_template, true, true); + result = expand_ccname_template(tmp_ctx, kr, test_template, NULL, true, true); fail_unless(result == NULL, "Expected NULL as a result for an empty input.", test_template); @@ -512,7 +494,7 @@ START_TEST(test_no_substitution) const char *test_template = BASE; char *result; - result = expand_ccname_template(tmp_ctx, kr, test_template, true, true); + result = expand_ccname_template(tmp_ctx, kr, test_template, NULL, true, true); fail_unless(result != NULL, "Cannot expand template [%s].", test_template); fail_unless(strcmp(result, test_template) == 0, @@ -529,7 +511,7 @@ START_TEST(test_krb5_style_expansion) file_template = BASE"/%{uid}/%{USERID}/%{euid}/%{username}"; expected = BASE"/"UID"/"UID"/"UID"/"USERNAME; - result = expand_ccname_template(tmp_ctx, kr, file_template, true, true); + result = expand_ccname_template(tmp_ctx, kr, file_template, NULL, true, true); fail_unless(result != NULL, "Cannot expand template [%s].", file_template); fail_unless(strcmp(result, expected) == 0, @@ -538,7 +520,7 @@ START_TEST(test_krb5_style_expansion) file_template = BASE"/%{unknown}"; expected = BASE"/%{unknown}"; - result = expand_ccname_template(tmp_ctx, kr, file_template, true, false); + result = expand_ccname_template(tmp_ctx, kr, file_template, NULL, true, true); fail_unless(result != NULL, "Cannot expand template [%s].", file_template); fail_unless(strcmp(result, expected) == 0, -- 1.9.3