>From 9bf2ce3e471a375101941daf31aacc3a798d5316 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Sat, 18 Oct 2014 22:03:01 +0200 Subject: [PATCH 4/6] KRB5: Move checking for illegal RE to krb5_utils.c Otherwise we would have to link krb5_child with pcre and transfer the regex, which wold be cumbersome. Check for illegal patterns when expanding the template instead. --- src/providers/krb5/krb5_ccache.c | 35 ++---------------- src/providers/krb5/krb5_ccache.h | 4 +-- src/providers/krb5/krb5_utils.c | 36 +++++++++++++++++-- src/providers/krb5/krb5_utils.h | 4 +-- src/tests/krb5_utils-tests.c | 78 ++++++++++++++++------------------------ 5 files changed, 68 insertions(+), 89 deletions(-) diff --git a/src/providers/krb5/krb5_ccache.c b/src/providers/krb5/krb5_ccache.c index 5586963338616519f36e5d75e796a597d3ac2f22..912d51aba7a1ac9c44654fc49e2ff53eb82a6580 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; @@ -163,7 +141,6 @@ static errno_t check_parent_stat(struct stat *parent_stat, uid_t uid) } errno_t create_ccache_dir(const char *ccdirname, - pcre *illegal_re, uid_t uid, gid_t gid) { int ret = EFAULT; @@ -188,13 +165,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 +212,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 +256,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..5ff98864e94a897e39fca80286c5f67feb2da261 100644 --- a/src/providers/krb5/krb5_ccache.h +++ b/src/providers/krb5/krb5_ccache.h @@ -36,11 +36,9 @@ struct tgt_times { }; 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 5f4078f2c2f30eddc004851e38b5fdafe93532e2..1ca16100c434c9fd6ea6aff125fb4902ebe29e83 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_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