From a2b9a84460429181f2a4fa7e2bb5ab49fd561274 Mon Sep 17 00:00:00 2001 From: Sumit Bose Date: Mon, 9 Dec 2019 11:31:14 +0100 Subject: [PATCH] certmap: sanitize LDAP search filter The sss_certmap_get_search_filter() will now sanitize the values read from the certificates before adding them to a search filter. To be able to get the plain values as well sss_certmap_expand_mapping_rule() is added. Resolves: https://github.com/SSSD/sssd/issues/5135 Reviewed-by: Alexey Tikhonov --- Makefile.am | 2 +- src/lib/certmap/sss_certmap.c | 42 ++++++++++-- src/lib/certmap/sss_certmap.exports | 5 ++ src/lib/certmap/sss_certmap.h | 35 ++++++++-- src/responder/pam/pamsrv_p11.c | 5 +- src/tests/cmocka/test_certmap.c | 98 +++++++++++++++++++++++++++- src/util/util.c | 94 --------------------------- src/util/util_ext.c | 99 +++++++++++++++++++++++++++++ 8 files changed, 272 insertions(+), 108 deletions(-) diff --git a/Makefile.am b/Makefile.am index 059e1eaf6..4bacabdda 100644 --- a/Makefile.am +++ b/Makefile.am @@ -2163,7 +2163,7 @@ libsss_certmap_la_LIBADD = \ $(NULL) libsss_certmap_la_LDFLAGS = \ -Wl,--version-script,$(srcdir)/src/lib/certmap/sss_certmap.exports \ - -version-info 1:0:1 + -version-info 2:0:2 if HAVE_NSS libsss_certmap_la_SOURCES += \ diff --git a/src/lib/certmap/sss_certmap.c b/src/lib/certmap/sss_certmap.c index 703782b53..f19e57732 100644 --- a/src/lib/certmap/sss_certmap.c +++ b/src/lib/certmap/sss_certmap.c @@ -441,10 +441,12 @@ static int expand_san(struct sss_certmap_ctx *ctx, static int expand_template(struct sss_certmap_ctx *ctx, struct parsed_template *parsed_template, struct sss_cert_content *cert_content, + bool sanitize, char **expanded) { int ret; char *exp = NULL; + char *exp_sanitized = NULL; if (strcmp("issuer_dn", parsed_template->name) == 0) { ret = rdn_list_2_dn_str(ctx, parsed_template->conversion, @@ -455,6 +457,8 @@ static int expand_template(struct sss_certmap_ctx *ctx, } else if (strncmp("subject_", parsed_template->name, 8) == 0) { ret = expand_san(ctx, parsed_template, cert_content->san_list, &exp); } else if (strcmp("cert", parsed_template->name) == 0) { + /* cert blob is already sanitized */ + sanitize = false; ret = expand_cert(ctx, parsed_template, cert_content, &exp); } else { CM_DEBUG(ctx, "Unsupported template name."); @@ -471,6 +475,16 @@ static int expand_template(struct sss_certmap_ctx *ctx, goto done; } + if (sanitize) { + ret = sss_filter_sanitize(ctx, exp, &exp_sanitized); + if (ret != EOK) { + CM_DEBUG(ctx, "Failed to sanitize expanded template."); + goto done; + } + talloc_free(exp); + exp = exp_sanitized; + } + ret = 0; done: @@ -485,7 +499,7 @@ done: static int get_filter(struct sss_certmap_ctx *ctx, struct ldap_mapping_rule *parsed_mapping_rule, - struct sss_cert_content *cert_content, + struct sss_cert_content *cert_content, bool sanitize, char **filter) { struct ldap_mapping_rule_comp *comp; @@ -503,7 +517,7 @@ static int get_filter(struct sss_certmap_ctx *ctx, result = talloc_strdup_append(result, comp->val); } else if (comp->type == comp_template) { ret = expand_template(ctx, comp->parsed_template, cert_content, - &expanded); + sanitize, &expanded); if (ret != 0) { CM_DEBUG(ctx, "Failed to expanded template."); goto done; @@ -791,8 +805,9 @@ done: return ret; } -int sss_certmap_get_search_filter(struct sss_certmap_ctx *ctx, +static int expand_mapping_rule_ex(struct sss_certmap_ctx *ctx, const uint8_t *der_cert, size_t der_size, + bool sanitize, char **_filter, char ***_domains) { int ret; @@ -819,7 +834,8 @@ int sss_certmap_get_search_filter(struct sss_certmap_ctx *ctx, return EINVAL; } - ret = get_filter(ctx, ctx->default_mapping_rule, cert_content, &filter); + ret = get_filter(ctx, ctx->default_mapping_rule, cert_content, sanitize, + &filter); goto done; } @@ -829,7 +845,7 @@ int sss_certmap_get_search_filter(struct sss_certmap_ctx *ctx, if (ret == 0) { /* match */ ret = get_filter(ctx, r->parsed_mapping_rule, cert_content, - &filter); + sanitize, &filter); if (ret != 0) { CM_DEBUG(ctx, "Failed to get filter"); goto done; @@ -873,6 +889,22 @@ done: return ret; } +int sss_certmap_get_search_filter(struct sss_certmap_ctx *ctx, + const uint8_t *der_cert, size_t der_size, + char **_filter, char ***_domains) +{ + return expand_mapping_rule_ex(ctx, der_cert, der_size, true, + _filter, _domains); +} + +int sss_certmap_expand_mapping_rule(struct sss_certmap_ctx *ctx, + const uint8_t *der_cert, size_t der_size, + char **_expanded, char ***_domains) +{ + return expand_mapping_rule_ex(ctx, der_cert, der_size, false, + _expanded, _domains); +} + int sss_certmap_init(TALLOC_CTX *mem_ctx, sss_certmap_ext_debug *debug, void *debug_priv, struct sss_certmap_ctx **ctx) diff --git a/src/lib/certmap/sss_certmap.exports b/src/lib/certmap/sss_certmap.exports index a9e48d6d0..7d7667738 100644 --- a/src/lib/certmap/sss_certmap.exports +++ b/src/lib/certmap/sss_certmap.exports @@ -16,3 +16,8 @@ SSS_CERTMAP_0.1 { global: sss_certmap_display_cert_content; } SSS_CERTMAP_0.0; + +SSS_CERTMAP_0.2 { + global: + sss_certmap_expand_mapping_rule; +} SSS_CERTMAP_0.1; diff --git a/src/lib/certmap/sss_certmap.h b/src/lib/certmap/sss_certmap.h index 7da2d1c58..058d4f9e4 100644 --- a/src/lib/certmap/sss_certmap.h +++ b/src/lib/certmap/sss_certmap.h @@ -103,7 +103,7 @@ int sss_certmap_add_rule(struct sss_certmap_ctx *ctx, * * @param[in] ctx certmap context previously initialized with * @ref sss_certmap_init - * @param[in] der_cert binary blog with the DER encoded certificate + * @param[in] der_cert binary blob with the DER encoded certificate * @param[in] der_size size of the certificate blob * * @return @@ -119,10 +119,11 @@ int sss_certmap_match_cert(struct sss_certmap_ctx *ctx, * * @param[in] ctx certmap context previously initialized with * @ref sss_certmap_init - * @param[in] der_cert binary blog with the DER encoded certificate + * @param[in] der_cert binary blob with the DER encoded certificate * @param[in] der_size size of the certificate blob - * @param[out] filter LDAP filter string, caller should free the data by - * calling sss_certmap_free_filter_and_domains + * @param[out] filter LDAP filter string, expanded templates are sanitized, + * caller should free the data by calling + * sss_certmap_free_filter_and_domains * @param[out] domains NULL-terminated array of strings with the domains the * rule applies, caller should free the data by calling * sss_certmap_free_filter_and_domains @@ -136,8 +137,32 @@ int sss_certmap_get_search_filter(struct sss_certmap_ctx *ctx, const uint8_t *der_cert, size_t der_size, char **filter, char ***domains); +/** + * @brief Expand the mapping rule by replacing the templates + * + * @param[in] ctx certmap context previously initialized with + * @ref sss_certmap_init + * @param[in] der_cert binary blob with the DER encoded certificate + * @param[in] der_size size of the certificate blob + * @param[out] expanded expanded mapping rule, templates are filled in + * verbatim in contrast to sss_certmap_get_search_filter, + * caller should free the data by + * calling sss_certmap_free_filter_and_domains + * @param[out] domains NULL-terminated array of strings with the domains the + * rule applies, caller should free the data by calling + * sss_certmap_free_filter_and_domains + * + * @return + * - 0: certificate matches a rule + * - ENOENT: certificate does not match + * - EINVAL: internal error + */ +int sss_certmap_expand_mapping_rule(struct sss_certmap_ctx *ctx, + const uint8_t *der_cert, size_t der_size, + char **_expanded, char ***_domains); /** * @brief Free data returned by @ref sss_certmap_get_search_filter + * and @ref sss_certmap_expand_mapping_rule * * @param[in] filter LDAP filter strings returned by * sss_certmap_get_search_filter @@ -150,7 +175,7 @@ void sss_certmap_free_filter_and_domains(char *filter, char **domains); * @brief Get a string with the content of the certificate used by the library * * @param[in] mem_ctx Talloc memory context, may be NULL - * @param[in] der_cert binary blog with the DER encoded certificate + * @param[in] der_cert binary blob with the DER encoded certificate * @param[in] der_size size of the certificate blob * @param[out] desc Multiline string showing the certificate content * which is used by libsss_certmap diff --git a/src/responder/pam/pamsrv_p11.c b/src/responder/pam/pamsrv_p11.c index 3f0afaeff..cdf239e07 100644 --- a/src/responder/pam/pamsrv_p11.c +++ b/src/responder/pam/pamsrv_p11.c @@ -1049,9 +1049,10 @@ static char *get_cert_prompt(TALLOC_CTX *mem_ctx, goto done; } - ret = sss_certmap_get_search_filter(ctx, der, der_size, &filter, &domains); + ret = sss_certmap_expand_mapping_rule(ctx, der, der_size, + &filter, &domains); if (ret != 0) { - DEBUG(SSSDBG_OP_FAILURE, "sss_certmap_get_search_filter failed.\n"); + DEBUG(SSSDBG_OP_FAILURE, "sss_certmap_expand_mapping_rule failed.\n"); goto done; } diff --git a/src/tests/cmocka/test_certmap.c b/src/tests/cmocka/test_certmap.c index c882202a0..232ff7878 100644 --- a/src/tests/cmocka/test_certmap.c +++ b/src/tests/cmocka/test_certmap.c @@ -1431,6 +1431,15 @@ static void test_sss_certmap_get_search_filter(void **state) &filter, &domains); assert_int_equal(ret, 0); assert_non_null(filter); + assert_string_equal(filter, "rule100=CN=Certificate\\20Authority,O=IPA.DEVEL" + "CN=ipa-devel.ipa.devel,O=IPA.DEVEL"); + assert_null(domains); + + ret = sss_certmap_expand_mapping_rule(ctx, discard_const(test_cert_der), + sizeof(test_cert_der), + &filter, &domains); + assert_int_equal(ret, 0); + assert_non_null(filter); assert_string_equal(filter, "rule100=CN=Certificate Authority,O=IPA.DEVEL" "CN=ipa-devel.ipa.devel,O=IPA.DEVEL"); assert_null(domains); @@ -1445,6 +1454,17 @@ static void test_sss_certmap_get_search_filter(void **state) &filter, &domains); assert_int_equal(ret, 0); assert_non_null(filter); + assert_string_equal(filter, "rule99=CN=Certificate\\20Authority,O=IPA.DEVEL" + "CN=ipa-devel.ipa.devel,O=IPA.DEVEL"); + assert_non_null(domains); + assert_string_equal(domains[0], "test.dom"); + assert_null(domains[1]); + + ret = sss_certmap_expand_mapping_rule(ctx, discard_const(test_cert_der), + sizeof(test_cert_der), + &filter, &domains); + assert_int_equal(ret, 0); + assert_non_null(filter); assert_string_equal(filter, "rule99=CN=Certificate Authority,O=IPA.DEVEL" "CN=ipa-devel.ipa.devel,O=IPA.DEVEL"); assert_non_null(domains); @@ -1466,6 +1486,16 @@ static void test_sss_certmap_get_search_filter(void **state) assert_string_equal(domains[0], "test.dom"); assert_null(domains[1]); + ret = sss_certmap_expand_mapping_rule(ctx, discard_const(test_cert_der), + sizeof(test_cert_der), + &filter, &domains); + assert_int_equal(ret, 0); + assert_non_null(filter); + assert_string_equal(filter, "rule98=userCertificate;binary=" TEST_CERT_BIN); + assert_non_null(domains); + assert_string_equal(domains[0], "test.dom"); + assert_null(domains[1]); + ret = sss_certmap_add_rule(ctx, 97, "KRB5:CN=Certificate Authority,O=IPA.DEVEL", "LDAP:rule97={issuer_dn!nss_x500}{subject_dn}", @@ -1476,6 +1506,17 @@ static void test_sss_certmap_get_search_filter(void **state) &filter, &domains); assert_int_equal(ret, 0); assert_non_null(filter); + assert_string_equal(filter, "rule97=O=IPA.DEVEL,CN=Certificate\\20Authority" + "CN=ipa-devel.ipa.devel,O=IPA.DEVEL"); + assert_non_null(domains); + assert_string_equal(domains[0], "test.dom"); + assert_null(domains[1]); + + ret = sss_certmap_expand_mapping_rule(ctx, discard_const(test_cert_der), + sizeof(test_cert_der), + &filter, &domains); + assert_int_equal(ret, 0); + assert_non_null(filter); assert_string_equal(filter, "rule97=O=IPA.DEVEL,CN=Certificate Authority" "CN=ipa-devel.ipa.devel,O=IPA.DEVEL"); assert_non_null(domains); @@ -1492,6 +1533,17 @@ static void test_sss_certmap_get_search_filter(void **state) &filter, &domains); assert_int_equal(ret, 0); assert_non_null(filter); + assert_string_equal(filter, "rule96=O=IPA.DEVEL,CN=Certificate\\20Authority" + "O=IPA.DEVEL,CN=ipa-devel.ipa.devel"); + assert_non_null(domains); + assert_string_equal(domains[0], "test.dom"); + assert_null(domains[1]); + + ret = sss_certmap_expand_mapping_rule(ctx, discard_const(test_cert_der), + sizeof(test_cert_der), + &filter, &domains); + assert_int_equal(ret, 0); + assert_non_null(filter); assert_string_equal(filter, "rule96=O=IPA.DEVEL,CN=Certificate Authority" "O=IPA.DEVEL,CN=ipa-devel.ipa.devel"); assert_non_null(domains); @@ -1510,6 +1562,14 @@ static void test_sss_certmap_get_search_filter(void **state) assert_string_equal(filter, "(userCertificate;binary=" TEST_CERT_BIN ")"); assert_null(domains); + ret = sss_certmap_expand_mapping_rule(ctx, discard_const(test_cert_der), + sizeof(test_cert_der), + &filter, &domains); + assert_int_equal(ret, 0); + assert_non_null(filter); + assert_string_equal(filter, "(userCertificate;binary=" TEST_CERT_BIN ")"); + assert_null(domains); + ret = sss_certmap_add_rule(ctx, 94, "KRB5:CN=Certificate Authority,O=IPA.DEVEL", "LDAP:rule94={issuer_dn!ad_x500}{subject_dn!ad_x500}", @@ -1520,12 +1580,22 @@ static void test_sss_certmap_get_search_filter(void **state) &filter, &domains); assert_int_equal(ret, 0); assert_non_null(filter); - assert_string_equal(filter, "rule94=O=IPA.DEVEL,CN=Certificate Authority" + assert_string_equal(filter, "rule94=O=IPA.DEVEL,CN=Certificate\\20Authority" "O=IPA.DEVEL,CN=ipa-devel.ipa.devel"); assert_non_null(domains); assert_string_equal(domains[0], "test.dom"); assert_null(domains[1]); + ret = sss_certmap_expand_mapping_rule(ctx, discard_const(test_cert_der), + sizeof(test_cert_der), + &filter, &domains); + assert_int_equal(ret, 0); + assert_non_null(filter); + assert_string_equal(filter, "rule94=O=IPA.DEVEL,CN=Certificate Authority" + "O=IPA.DEVEL,CN=ipa-devel.ipa.devel"); + assert_non_null(domains); + assert_string_equal(domains[0], "test.dom"); + assert_null(domains[1]); ret = sss_certmap_add_rule(ctx, 89, NULL, "(rule89={subject_nt_principal})", @@ -1539,6 +1609,14 @@ static void test_sss_certmap_get_search_filter(void **state) assert_string_equal(filter, "(rule89=tu1@ad.devel)"); assert_null(domains); + ret = sss_certmap_expand_mapping_rule(ctx, discard_const(test_cert2_der), + sizeof(test_cert2_der), + &filter, &domains); + assert_int_equal(ret, 0); + assert_non_null(filter); + assert_string_equal(filter, "(rule89=tu1@ad.devel)"); + assert_null(domains); + ret = sss_certmap_add_rule(ctx, 88, NULL, "(rule88={subject_nt_principal.short_name})", NULL); @@ -1560,6 +1638,15 @@ static void test_sss_certmap_get_search_filter(void **state) &filter, &domains); assert_int_equal(ret, 0); assert_non_null(filter); + assert_string_equal(filter, "rule87=DC=devel,DC=ad,CN=ad-AD-SERVER-CA" + "DC=devel,DC=ad,CN=Users,CN=t\\20u,E=test.user@email.domain"); + assert_null(domains); + + ret = sss_certmap_expand_mapping_rule(ctx, discard_const(test_cert2_der), + sizeof(test_cert2_der), + &filter, &domains); + assert_int_equal(ret, 0); + assert_non_null(filter); assert_string_equal(filter, "rule87=DC=devel,DC=ad,CN=ad-AD-SERVER-CA" "DC=devel,DC=ad,CN=Users,CN=t u,E=test.user@email.domain"); assert_null(domains); @@ -1573,6 +1660,15 @@ static void test_sss_certmap_get_search_filter(void **state) &filter, &domains); assert_int_equal(ret, 0); assert_non_null(filter); + assert_string_equal(filter, "rule86=DC=devel,DC=ad,CN=ad-AD-SERVER-CA" + "DC=devel,DC=ad,CN=Users,CN=t\\20u,E=test.user@email.domain"); + assert_null(domains); + + ret = sss_certmap_expand_mapping_rule(ctx, discard_const(test_cert2_der), + sizeof(test_cert2_der), + &filter, &domains); + assert_int_equal(ret, 0); + assert_non_null(filter); assert_string_equal(filter, "rule86=DC=devel,DC=ad,CN=ad-AD-SERVER-CA" "DC=devel,DC=ad,CN=Users,CN=t u,E=test.user@email.domain"); assert_null(domains); diff --git a/src/util/util.c b/src/util/util.c index d9bd3cb59..19d447328 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -436,100 +436,6 @@ errno_t sss_hash_create(TALLOC_CTX *mem_ctx, unsigned long count, return sss_hash_create_ex(mem_ctx, count, tbl, 0, 0, 0, 0, NULL, NULL); } -errno_t sss_filter_sanitize_ex(TALLOC_CTX *mem_ctx, - const char *input, - char **sanitized, - const char *ignore) -{ - char *output; - size_t i = 0; - size_t j = 0; - char *allowed; - - /* Assume the worst-case. We'll resize it later, once */ - output = talloc_array(mem_ctx, char, strlen(input) * 3 + 1); - if (!output) { - return ENOMEM; - } - - while (input[i]) { - /* Even though this character might have a special meaning, if it's - * explicitly allowed, just copy it and move on - */ - if (ignore == NULL) { - allowed = NULL; - } else { - allowed = strchr(ignore, input[i]); - } - if (allowed) { - output[j++] = input[i++]; - continue; - } - - switch(input[i]) { - case '\t': - output[j++] = '\\'; - output[j++] = '0'; - output[j++] = '9'; - break; - case ' ': - output[j++] = '\\'; - output[j++] = '2'; - output[j++] = '0'; - break; - case '*': - output[j++] = '\\'; - output[j++] = '2'; - output[j++] = 'a'; - break; - case '(': - output[j++] = '\\'; - output[j++] = '2'; - output[j++] = '8'; - break; - case ')': - output[j++] = '\\'; - output[j++] = '2'; - output[j++] = '9'; - break; - case '\\': - output[j++] = '\\'; - output[j++] = '5'; - output[j++] = 'c'; - break; - case '\r': - output[j++] = '\\'; - output[j++] = '0'; - output[j++] = 'd'; - break; - case '\n': - output[j++] = '\\'; - output[j++] = '0'; - output[j++] = 'a'; - break; - default: - output[j++] = input[i]; - } - - i++; - } - output[j] = '\0'; - *sanitized = talloc_realloc(mem_ctx, output, char, j+1); - if (!*sanitized) { - talloc_free(output); - return ENOMEM; - } - - return EOK; -} - -errno_t sss_filter_sanitize(TALLOC_CTX *mem_ctx, - const char *input, - char **sanitized) -{ - return sss_filter_sanitize_ex(mem_ctx, input, sanitized, NULL); -} - char * sss_escape_ip_address(TALLOC_CTX *mem_ctx, int family, const char *addr) { diff --git a/src/util/util_ext.c b/src/util/util_ext.c index 04dc02a8a..a89b60f76 100644 --- a/src/util/util_ext.c +++ b/src/util/util_ext.c @@ -29,6 +29,11 @@ #define EOK 0 +#ifndef HAVE_ERRNO_T +#define HAVE_ERRNO_T +typedef int errno_t; +#endif + int split_on_separator(TALLOC_CTX *mem_ctx, const char *str, const char sep, bool trim, bool skip_empty, char ***_list, int *size) @@ -141,3 +146,97 @@ bool string_in_list(const char *string, char **list, bool case_sensitive) return false; } + +errno_t sss_filter_sanitize_ex(TALLOC_CTX *mem_ctx, + const char *input, + char **sanitized, + const char *ignore) +{ + char *output; + size_t i = 0; + size_t j = 0; + char *allowed; + + /* Assume the worst-case. We'll resize it later, once */ + output = talloc_array(mem_ctx, char, strlen(input) * 3 + 1); + if (!output) { + return ENOMEM; + } + + while (input[i]) { + /* Even though this character might have a special meaning, if it's + * explicitly allowed, just copy it and move on + */ + if (ignore == NULL) { + allowed = NULL; + } else { + allowed = strchr(ignore, input[i]); + } + if (allowed) { + output[j++] = input[i++]; + continue; + } + + switch(input[i]) { + case '\t': + output[j++] = '\\'; + output[j++] = '0'; + output[j++] = '9'; + break; + case ' ': + output[j++] = '\\'; + output[j++] = '2'; + output[j++] = '0'; + break; + case '*': + output[j++] = '\\'; + output[j++] = '2'; + output[j++] = 'a'; + break; + case '(': + output[j++] = '\\'; + output[j++] = '2'; + output[j++] = '8'; + break; + case ')': + output[j++] = '\\'; + output[j++] = '2'; + output[j++] = '9'; + break; + case '\\': + output[j++] = '\\'; + output[j++] = '5'; + output[j++] = 'c'; + break; + case '\r': + output[j++] = '\\'; + output[j++] = '0'; + output[j++] = 'd'; + break; + case '\n': + output[j++] = '\\'; + output[j++] = '0'; + output[j++] = 'a'; + break; + default: + output[j++] = input[i]; + } + + i++; + } + output[j] = '\0'; + *sanitized = talloc_realloc(mem_ctx, output, char, j+1); + if (!*sanitized) { + talloc_free(output); + return ENOMEM; + } + + return EOK; +} + +errno_t sss_filter_sanitize(TALLOC_CTX *mem_ctx, + const char *input, + char **sanitized) +{ + return sss_filter_sanitize_ex(mem_ctx, input, sanitized, NULL); +} -- 2.21.3