[27274] in CVS-changelog-for-Kerberos-V5
krb5 commit: Fix quoting issues in LDAP KDB module
daemon@ATHENA.MIT.EDU (Greg Hudson)
Sat Nov 17 15:44:17 2012
Date: Sat, 17 Nov 2012 15:44:15 -0500
From: Greg Hudson <ghudson@mit.edu>
Message-Id: <201211172044.qAHKiFZF015498@drugstore.mit.edu>
To: cvs-krb5@mit.edu
Reply-To: krbdev@mit.edu
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: cvs-krb5-bounces@mit.edu
https://github.com/krb5/krb5/commit/85898e8f1c9e4f5bff70e1ff810519363b262eb4
commit 85898e8f1c9e4f5bff70e1ff810519363b262eb4
Author: Greg Hudson <ghudson@mit.edu>
Date: Sat Nov 17 15:30:32 2012 -0500
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 <hanmao_shi@apple.com>.
ticket: 7296
src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c | 18 ++++--
src/plugins/kdb/ldap/libkdb_ldap/ldap_pwd_policy.c | 16 +----
src/plugins/kdb/ldap/libkdb_ldap/ldap_realm.c | 77 ++++----------------
src/tests/kdbtest.c | 34 +++++----
src/tests/t_kdb.py | 2 -
5 files changed, 48 insertions(+), 99 deletions(-)
diff --git a/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c b/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c
index a7101a7..c386a9e 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 09cfb8c..e955f8e 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 45649da..7e0d456 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 11c433d..b569b56 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 "<test*>".
*/
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[] = "<test*>";
+
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 dd79b42..2f0d6fd 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.
_______________________________________________
cvs-krb5 mailing list
cvs-krb5@mit.edu
https://mailman.mit.edu/mailman/listinfo/cvs-krb5
