From: Greg Hudson Date: Sat, 17 Nov 2012 20:30:32 +0000 (-0500) Subject: Fix quoting issues in LDAP KDB module X-Git-Tag: krb5-1.12-alpha1~472 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=85898e8f1c9e4f5bff70e1ff810519363b262eb4;p=thirdparty%2Fkrb5.git Fix quoting issues in LDAP KDB module Modify ldap_filter_correct() to quote special characters for DN strings as well as filters, since it is already used to quote a DN string in krb5_ldap_name_to_policydn() and there's no harm in over-quoting. In krb5_ldap_put_principal(), quote the unparsed principal name for use in DNs we choose. In krb5_ldap_create_password_policy(), use the policy name for the CN of the policy entry instead of the (possibly quoted) first element of the DN. Adapted from a patch by Jim Shi . ticket: 7296 --- diff --git a/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c b/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c index a7101a738d..c386a9ea9f 100644 --- a/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c +++ b/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c @@ -496,6 +496,7 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry, LDAPMessage *result=NULL, *ent=NULL; char *user=NULL, *subtree=NULL, *principal_dn=NULL; char **values=NULL, *strval[10]={NULL}, errbuf[1024]; + char *filtuser=NULL; struct berval **bersecretkey=NULL; LDAPMod **mods=NULL; krb5_boolean create_standalone_prinicipal=FALSE; @@ -534,6 +535,11 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry, if (((st=krb5_unparse_name(context, entry->princ, &user)) != 0) || ((st=krb5_ldap_unparse_principal_name(user)) != 0)) goto cleanup; + filtuser = ldap_filter_correct(user); + if (filtuser == NULL) { + st = ENOMEM; + goto cleanup; + } } /* Identity the type of operation, it can be @@ -554,7 +560,7 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry, goto cleanup; if (entry->mask & KADM5_LOAD) { - int tree = 0, ntrees = 0, princlen = 0, numlentries = 0; + int tree = 0, ntrees = 0, numlentries = 0; char **subtreelist = NULL, *filter = NULL; /* A load operation is special, will do a mix-in (add krbprinc @@ -572,12 +578,11 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry, "name not found")); goto cleanup; } - princlen = strlen(FILTER) + strlen(user) + 2 + 1; /* 2 for closing brackets */ - if ((filter = malloc(princlen)) == NULL) { + if (asprintf(&filter, FILTER"%s))", filtuser) < 0) { + filter = NULL; st = ENOMEM; goto cleanup; } - snprintf(filter, princlen, FILTER"%s))", user); /* get the current subtree list */ if ((st = krb5_get_subtree_info(ldap_context, &subtreelist, &ntrees)) != 0) @@ -684,7 +689,7 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry, CHECK_NULL(subtree); if (asprintf(&standalone_principal_dn, "krbprincipalname=%s,%s", - user, subtree) < 0) + filtuser, subtree) < 0) standalone_principal_dn = NULL; CHECK_NULL(standalone_principal_dn); /* @@ -1262,6 +1267,9 @@ cleanup: if (user) free(user); + if (filtuser) + free(filtuser); + free_xargs(xargs); if (standalone_principal_dn) diff --git a/src/plugins/kdb/ldap/libkdb_ldap/ldap_pwd_policy.c b/src/plugins/kdb/ldap/libkdb_ldap/ldap_pwd_policy.c index 09cfb8ca03..e955f8e404 100644 --- a/src/plugins/kdb/ldap/libkdb_ldap/ldap_pwd_policy.c +++ b/src/plugins/kdb/ldap/libkdb_ldap/ldap_pwd_policy.c @@ -140,7 +140,7 @@ krb5_ldap_create_password_policy(krb5_context context, osa_policy_ent_t policy) kdb5_dal_handle *dal_handle=NULL; krb5_ldap_context *ldap_context=NULL; krb5_ldap_server_handle *ldap_server_handle=NULL; - char **rdns=NULL, *strval[2]={NULL}, *policy_dn; + char *strval[2]={NULL}, *policy_dn; /* Clear the global error string */ krb5_clear_error_message(context); @@ -156,16 +156,7 @@ krb5_ldap_create_password_policy(krb5_context context, osa_policy_ent_t policy) if (st != 0) goto cleanup; - /* get the first component of the dn to set the cn attribute */ - rdns = ldap_explode_dn(policy_dn, 1); - if (rdns == NULL) { - st = EINVAL; - krb5_set_error_message(context, st, - _("Invalid password policy DN syntax")); - goto cleanup; - } - - strval[0] = rdns[0]; + strval[0] = policy->name; if ((st=krb5_add_str_mem_ldap_mod(&mods, "cn", LDAP_MOD_ADD, strval)) != 0) goto cleanup; @@ -184,9 +175,6 @@ krb5_ldap_create_password_policy(krb5_context context, osa_policy_ent_t policy) } cleanup: - if (rdns) - ldap_value_free(rdns); - if (policy_dn != NULL) free (policy_dn); ldap_mods_free(mods, 1); diff --git a/src/plugins/kdb/ldap/libkdb_ldap/ldap_realm.c b/src/plugins/kdb/ldap/libkdb_ldap/ldap_realm.c index 45649da02c..7e0d45689d 100644 --- a/src/plugins/kdb/ldap/libkdb_ldap/ldap_realm.c +++ b/src/plugins/kdb/ldap/libkdb_ldap/ldap_realm.c @@ -70,72 +70,25 @@ char *krbContainerRefclass[] = { "krbContainerRefAux", NULL}; * list realms from eDirectory */ -/* - * Function to remove all special characters from a string (rfc2254). - * Use whenever exact matching is to be done ... - */ +/* Return a copy of in, quoting all characters which are special in an LDAP + * filter (RFC 4515) or DN string (RFC 4514). Return NULL on failure. */ char * ldap_filter_correct (char *in) { - size_t i, count; - char *out, *ptr; - size_t len = strlen(in); - - for (i = 0, count = 0; i < len; i++) - switch (in[i]) { - case '*': - case '(': - case ')': - case '\\': - case '\0': - count ++; - } - - out = (char *)malloc((len + (count * 2) + 1) * sizeof (char)); - assert (out != NULL); - memset(out, 0, len + (count * 2) + 1); - - for (i = 0, ptr = out; i < len; i++) - switch (in[i]) { - case '*': - ptr[0] = '\\'; - ptr[1] = '2'; - ptr[2] = 'a'; - ptr += 3; - break; - case '(': - ptr[0] = '\\'; - ptr[1] = '2'; - ptr[2] = '8'; - ptr += 3; - break; - case ')': - ptr[0] = '\\'; - ptr[1] = '2'; - ptr[2] = '9'; - ptr += 3; - break; - case '\\': - ptr[0] = '\\'; - ptr[1] = '5'; - ptr[2] = 'c'; - ptr += 3; + size_t count; + const char special[] = "*()\\ #\"+,;<>"; + struct k5buf buf; + + krb5int_buf_init_dynamic(&buf); + while (TRUE) { + count = strcspn(in, special); + krb5int_buf_add_len(&buf, in, count); + in += count; + if (*in == '\0') break; - case '\0': - ptr[0] = '\\'; - ptr[1] = '0'; - ptr[2] = '0'; - ptr += 3; - break; - default: - ptr[0] = in[i]; - ptr += 1; - break; - } - - /* ptr[count - 1] = '\0'; */ - - return out; + krb5int_buf_add_fmt(&buf, "\\%2x", (unsigned char)*in++); + } + return krb5int_buf_data(&buf); } static int diff --git a/src/tests/kdbtest.c b/src/tests/kdbtest.c index 11c433de03..b569b56235 100644 --- a/src/tests/kdbtest.c +++ b/src/tests/kdbtest.c @@ -67,8 +67,8 @@ check_cond(int value, int lineno) } static krb5_data princ_data[2] = { - { KV5M_DATA, 3, "xyz" }, - { KV5M_DATA, 3, "abc" } + { KV5M_DATA, 6, "xy*(z)" }, + { KV5M_DATA, 12, "+<> *()\\#\",;" } }; static krb5_principal_data sample_princ = { @@ -88,14 +88,14 @@ static krb5_principal_data xrealm_princ = { /* * tl1 through tl4 are normalized to attributes in the LDAP back end. tl5 is * stored as untranslated tl-data. tl3 contains an encoded osa_princ_ent with - * a policy reference to "testpol". + * a policy reference to "". */ static krb5_tl_data tl5 = { NULL, KRB5_TL_MKVNO, 2, U("\0\1") }; static krb5_tl_data tl4 = { &tl5, KRB5_TL_LAST_ADMIN_UNLOCK, 4, U("\6\0\0\0") }; static krb5_tl_data tl3 = { &tl4, KRB5_TL_KADM_DATA, 32, U("\x12\x34\x5C\x01\x00\x00\x00\x08" - "\x74\x65\x73\x74\x70\x6F\x6C\x00" + "\x3C\x74\x65\x73\x74\x2A\x3E\x00" "\x00\x00\x08\x00\x00\x00\x00\x00" "\x00\x00\x00\x02\x00\x00\x00\x00") }; static krb5_tl_data tl2 = { &tl3, KRB5_TL_MOD_PRINC, 8, U("\5\6\7\0x@Y\0") }; @@ -131,6 +131,8 @@ static krb5_key_data keys[] = { }; #undef U +static char polname[] = ""; + static krb5_db_entry sample_entry = { 0, KRB5_KDB_V1_BASE_LENGTH, @@ -159,7 +161,7 @@ static krb5_db_entry sample_entry = { static osa_policy_ent_rec sample_policy = { 0, /* version */ - "testpol", /* name */ + polname, /* name */ 1357, /* pw_min_life */ 100, /* pw_max_life */ 6, /* pw_min_length */ @@ -294,24 +296,24 @@ main() CHECK(krb5_db_open(ctx, NULL, KRB5_KDB_OPEN_RW | KRB5_KDB_SRV_TYPE_ADMIN)); CHECK(krb5_db_inited(ctx)); - /* Manipulate a policy, leaving testpol in place at the end. */ + /* Manipulate a policy, leaving it in place at the end. */ CHECK_COND(krb5_db_put_policy(ctx, &sample_policy) != 0); - CHECK_COND(krb5_db_delete_policy(ctx, "testpol") != 0); - CHECK_COND(krb5_db_get_policy(ctx, "testpol", &pol) == KRB5_KDB_NOENTRY); + CHECK_COND(krb5_db_delete_policy(ctx, polname) != 0); + CHECK_COND(krb5_db_get_policy(ctx, polname, &pol) == KRB5_KDB_NOENTRY); CHECK(krb5_db_create_policy(ctx, &sample_policy)); CHECK_COND(krb5_db_create_policy(ctx, &sample_policy) != 0); - CHECK(krb5_db_get_policy(ctx, "testpol", &pol)); + CHECK(krb5_db_get_policy(ctx, polname, &pol)); check_policy(pol); pol->pw_min_length--; CHECK(krb5_db_put_policy(ctx, pol)); krb5_db_free_policy(ctx, pol); - CHECK(krb5_db_get_policy(ctx, "testpol", &pol)); + CHECK(krb5_db_get_policy(ctx, polname, &pol)); CHECK_COND(pol->pw_min_length == sample_policy.pw_min_length - 1); krb5_db_free_policy(ctx, pol); - CHECK(krb5_db_delete_policy(ctx, "testpol")); + CHECK(krb5_db_delete_policy(ctx, polname)); CHECK_COND(krb5_db_put_policy(ctx, &sample_policy) != 0); - CHECK_COND(krb5_db_delete_policy(ctx, "testpol") != 0); - CHECK_COND(krb5_db_get_policy(ctx, "testpol", &pol) == KRB5_KDB_NOENTRY); + CHECK_COND(krb5_db_delete_policy(ctx, polname) != 0); + CHECK_COND(krb5_db_get_policy(ctx, polname, &pol) == KRB5_KDB_NOENTRY); CHECK(krb5_db_create_policy(ctx, &sample_policy)); count = 0; CHECK(krb5_db_iter_policy(ctx, NULL, iter_pol_handler, &count)); @@ -375,8 +377,8 @@ main() CHECK(krb5_dbe_update_tl_data(ctx, ent, &tl_no_policy)); ent->mask = KADM5_POLICY_CLR | KADM5_KEY_DATA; CHECK(krb5_db_put_principal(ctx, ent)); - /* Deleting testpol should work now that the reference is gone. */ - CHECK(krb5_db_delete_policy(ctx, "testpol")); + /* Deleting polname should work now that the reference is gone. */ + CHECK(krb5_db_delete_policy(ctx, polname)); /* Put the modified entry again (with KDB_TL_USER_INFO tl-data for LDAP) as * from a load operation. */ @@ -391,7 +393,7 @@ main() /* Exercise principal iteration code. */ count = 0; - CHECK(krb5_db_iterate(ctx, "xyz*", iter_princ_handler, &count)); + CHECK(krb5_db_iterate(ctx, "xy*", iter_princ_handler, &count)); CHECK_COND(count == 1); CHECK(krb5_db_fini(ctx)); diff --git a/src/tests/t_kdb.py b/src/tests/t_kdb.py index dd79b4283d..2f0d6fd90e 100644 --- a/src/tests/t_kdb.py +++ b/src/tests/t_kdb.py @@ -264,8 +264,6 @@ if out: # We could still use tests to exercise: # * DB arg handling in krb5_ldap_create # * krbAllowedToDelegateTo attribute processing -# * Special character handling in ldap_filter_correct (some bugs to -# fix first, see #7296 and September 2012 krbdev discussion) # * A load operation overwriting a standalone principal entry which # already exists but doesn't have a krbPrincipalName attribute # matching the principal name.