CVE-2020-25722 s4/dsdb/samldb: reject SPN with too few/many components

BUG: https://bugzilla.samba.org/show_bug.cgi?id=14564

Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>

diff --git a/selftest/knownfail.d/ldap_spn b/selftest/knownfail.d/ldap_spn
index b7eb6f30e7af4af1c3c72442be1f0221bca5306c..63f9fe02ef7df4b0b9880b2ea23325df7e7954f2 100644 (file)
--- a/selftest/knownfail.d/ldap_spn
+++ b/selftest/knownfail.d/ldap_spn
@@ -1,3 +1 @@
 samba.tests.ldap_spn.+LdapSpnTest.test_spn_dodgy_spns
-samba.tests.ldap_spn.+LdapSpnTest.test_spn_one_part_spns_no_slashes_
-samba.tests.ldap_spn.+LdapSpnTest.test_spn_too_many_spn_parts

diff --git a/source4/dsdb/samdb/ldb_modules/samldb.c b/source4/dsdb/samdb/ldb_modules/samldb.c
index f5141936a604fc5a5cf59ce9a177eb7dace5dbfe..006658d2bcef66caa0a30e98ab1f21b9441752a4 100644 (file)
--- a/source4/dsdb/samdb/ldb_modules/samldb.c
+++ b/source4/dsdb/samdb/ldb_modules/samldb.c
@@ -3868,6 +3868,37 @@ static int check_spn_direct_collision(struct ldb_context *ldb,
 }
 
 
+static int count_spn_components(struct ldb_val val)
+{
+       /*
+        * a 3 part servicePrincipalName has two slashes, like
+        * ldap/example.com/DomainDNSZones.example.com.
+        *
+        * In krb5_parse_name_flags() we don't count "\/" as a slash (i.e.
+        * escaped by a backslash), but this is not the behaviour of Windows
+        * on setting a servicePrincipalName -- slashes are counted regardless
+        * of backslashes.
+        *
+        * Accordingly, here we ignore backslashes. This will reject
+        * multi-slash SPNs that krb5_parse_name_flags() would accept, and
+        * allow ones in the form "a\/b" that it won't parse.
+        */
+       size_t i;
+       int slashes = 0;
+       for (i = 0; i < val.length; i++) {
+               char c = val.data[i];
+               if (c == '/') {
+                       slashes++;
+                       if (slashes == 3) {
+                               /* at this point we don't care */
+                               return 4;
+                       }
+               }
+       }
+       return slashes + 1;
+}
+
+
 /* Check that "servicePrincipalName" changes do not introduce a collision
  * globally. */
 static int samldb_spn_uniqueness_check(struct samldb_ctx *ac,
@@ -3883,8 +3914,18 @@ static int samldb_spn_uniqueness_check(struct samldb_ctx *ac,
        }
 
        for (i = 0; i < spn_el->num_values; i++) {
+               int n_components;
                spn = (char *)spn_el->values[i].data;
 
+               n_components = count_spn_components(spn_el->values[i]);
+               if (n_components > 3 || n_components < 2) {
+                       ldb_asprintf_errstring(ldb,
+                                              "samldb: spn[%s] invalid with %u components",
+                                              spn, n_components);
+                       talloc_free(tmp_ctx);
+                       return LDB_ERR_CONSTRAINT_VIOLATION;
+               }
+
                ret = check_spn_direct_collision(ldb,
                                                 tmp_ctx,
                                                 spn,