[27274] in CVS-changelog-for-Kerberos-V5

home help back first fref pref prev next nref lref last post

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

home help back first fref pref prev next nref lref last post