From f72856736ac39c7e926c02c11f854f43400366d4 Mon Sep 17 00:00:00 2001

From: Jakub Hrozek <jhrozek@redhat.com>

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 <sbose@redhat.com>

Reviewed-by: Lukáš Slebodník <lslebodn@redhat.com>

---

 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