218 lines
8.2 KiB
Diff
218 lines
8.2 KiB
Diff
From e1c52ac05a9ff505d2e5eac2f1ece4e95844ee71 Mon Sep 17 00:00:00 2001
|
|
From: Joseph Sutton <josephsutton@catalyst.net.nz>
|
|
Date: Tue, 7 Jun 2022 17:38:55 +1200
|
|
Subject: [PATCH 13/15] CVE-2022-32743 dsdb/modules/acl: Allow simultaneous
|
|
sAMAccountName, dNSHostName, and servicePrincipalName change
|
|
|
|
If the message changes the sAMAccountName, we'll check dNSHostName and
|
|
servicePrincipalName values against the new value of sAMAccountName,
|
|
rather than the account's current value. Similarly, if the message
|
|
changes the dNSHostName, we'll check servicePrincipalName values against
|
|
the new dNSHostName. This allows setting more than one of these
|
|
attributes simultaneously with validated write rights.
|
|
|
|
We now pass 'struct ldb_val' to acl_validate_spn_value() instead of
|
|
simple strings. Previously, we were relying on the data inside 'struct
|
|
ldb_val' having a terminating zero byte, even though this is not
|
|
guaranteed.
|
|
|
|
BUG: https://bugzilla.samba.org/show_bug.cgi?id=14833
|
|
|
|
Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
|
|
Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
|
|
---
|
|
selftest/knownfail.d/netlogon-dns-host-name | 2 -
|
|
selftest/knownfail.d/validated-dns-host-name | 3 -
|
|
source4/dsdb/samdb/ldb_modules/acl.c | 85 +++++++++++++++++++++-------
|
|
3 files changed, 65 insertions(+), 25 deletions(-)
|
|
delete mode 100644 selftest/knownfail.d/netlogon-dns-host-name
|
|
delete mode 100644 selftest/knownfail.d/validated-dns-host-name
|
|
|
|
diff --git a/selftest/knownfail.d/netlogon-dns-host-name b/selftest/knownfail.d/netlogon-dns-host-name
|
|
deleted file mode 100644
|
|
index 3eca0cd..0000000
|
|
--- a/selftest/knownfail.d/netlogon-dns-host-name
|
|
+++ /dev/null
|
|
@@ -1,2 +0,0 @@
|
|
-^samba.tests.py_credentials.samba.tests.py_credentials.PyCredentialsTests.test_set_dns_hostname_valid\(
|
|
-^samba.tests.py_credentials.samba.tests.py_credentials.PyCredentialsTests.test_set_dns_hostname_valid_denied\(
|
|
diff --git a/selftest/knownfail.d/validated-dns-host-name b/selftest/knownfail.d/validated-dns-host-name
|
|
deleted file mode 100644
|
|
index 4b61658..0000000
|
|
--- a/selftest/knownfail.d/validated-dns-host-name
|
|
+++ /dev/null
|
|
@@ -1,3 +0,0 @@
|
|
-^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_spn\(
|
|
-^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_spn_matching_account_name_new\(
|
|
-^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_spn_matching_dns_host_name_invalid\(
|
|
diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c
|
|
index 82f6ec3..4098ae2 100644
|
|
--- a/source4/dsdb/samdb/ldb_modules/acl.c
|
|
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
|
|
@@ -529,10 +529,10 @@ static int acl_sDRightsEffective(struct ldb_module *module,
|
|
|
|
static int acl_validate_spn_value(TALLOC_CTX *mem_ctx,
|
|
struct ldb_context *ldb,
|
|
- const char *spn_value,
|
|
+ const struct ldb_val *spn_value,
|
|
uint32_t userAccountControl,
|
|
- const char *samAccountName,
|
|
- const char *dnsHostName,
|
|
+ const struct ldb_val *samAccountName,
|
|
+ const struct ldb_val *dnsHostName,
|
|
const char *netbios_name,
|
|
const char *ntds_guid)
|
|
{
|
|
@@ -543,6 +543,7 @@ static int acl_validate_spn_value(TALLOC_CTX *mem_ctx,
|
|
char *instanceName;
|
|
char *serviceType;
|
|
char *serviceName;
|
|
+ const char *spn_value_str = NULL;
|
|
size_t account_name_len;
|
|
const char *forest_name = samdb_forest_name(ldb, mem_ctx);
|
|
const char *base_domain = samdb_default_domain_name(ldb, mem_ctx);
|
|
@@ -551,7 +552,18 @@ static int acl_validate_spn_value(TALLOC_CTX *mem_ctx,
|
|
bool is_dc = (userAccountControl & UF_SERVER_TRUST_ACCOUNT) ||
|
|
(userAccountControl & UF_PARTIAL_SECRETS_ACCOUNT);
|
|
|
|
- if (strcasecmp_m(spn_value, samAccountName) == 0) {
|
|
+ spn_value_str = talloc_strndup(mem_ctx,
|
|
+ (const char *)spn_value->data,
|
|
+ spn_value->length);
|
|
+ if (spn_value_str == NULL) {
|
|
+ return ldb_oom(ldb);
|
|
+ }
|
|
+
|
|
+ if (spn_value->length == samAccountName->length &&
|
|
+ strncasecmp((const char *)spn_value->data,
|
|
+ (const char *)samAccountName->data,
|
|
+ spn_value->length) == 0)
|
|
+ {
|
|
/* MacOS X sets this value, and setting an SPN of your
|
|
* own samAccountName is both pointless and safe */
|
|
return LDB_SUCCESS;
|
|
@@ -565,7 +577,7 @@ static int acl_validate_spn_value(TALLOC_CTX *mem_ctx,
|
|
"Could not initialize kerberos context.");
|
|
}
|
|
|
|
- ret = krb5_parse_name(krb_ctx, spn_value, &principal);
|
|
+ ret = krb5_parse_name(krb_ctx, spn_value_str, &principal);
|
|
if (ret) {
|
|
krb5_free_context(krb_ctx);
|
|
return LDB_ERR_CONSTRAINT_VIOLATION;
|
|
@@ -618,8 +630,10 @@ static int acl_validate_spn_value(TALLOC_CTX *mem_ctx,
|
|
}
|
|
}
|
|
|
|
- account_name_len = strlen(samAccountName);
|
|
- if (account_name_len && samAccountName[account_name_len - 1] == '$') {
|
|
+ account_name_len = samAccountName->length;
|
|
+ if (account_name_len &&
|
|
+ samAccountName->data[account_name_len - 1] == '$')
|
|
+ {
|
|
/* Account for the '$' character. */
|
|
--account_name_len;
|
|
}
|
|
@@ -627,12 +641,18 @@ static int acl_validate_spn_value(TALLOC_CTX *mem_ctx,
|
|
/* instanceName can be samAccountName without $ or dnsHostName
|
|
* or "ntds_guid._msdcs.forest_domain for DC objects */
|
|
if (strlen(instanceName) == account_name_len
|
|
- && strncasecmp(instanceName, samAccountName,
|
|
- account_name_len) == 0) {
|
|
+ && strncasecmp(instanceName,
|
|
+ (const char *)samAccountName->data,
|
|
+ account_name_len) == 0)
|
|
+ {
|
|
goto success;
|
|
}
|
|
if ((dnsHostName != NULL) &&
|
|
- (strcasecmp(instanceName, dnsHostName) == 0)) {
|
|
+ strlen(instanceName) == dnsHostName->length &&
|
|
+ (strncasecmp(instanceName,
|
|
+ (const char *)dnsHostName->data,
|
|
+ dnsHostName->length) == 0))
|
|
+ {
|
|
goto success;
|
|
}
|
|
if (is_dc) {
|
|
@@ -650,10 +670,13 @@ fail:
|
|
krb5_free_context(krb_ctx);
|
|
ldb_debug_set(ldb, LDB_DEBUG_WARNING,
|
|
"acl: spn validation failed for "
|
|
- "spn[%s] uac[0x%x] account[%s] hostname[%s] "
|
|
+ "spn[%.*s] uac[0x%x] account[%.*s] hostname[%.*s] "
|
|
"nbname[%s] ntds[%s] forest[%s] domain[%s]\n",
|
|
- spn_value, (unsigned)userAccountControl,
|
|
- samAccountName, dnsHostName,
|
|
+ (int)spn_value->length, spn_value->data,
|
|
+ (unsigned)userAccountControl,
|
|
+ (int)samAccountName->length, samAccountName->data,
|
|
+ dnsHostName != NULL ? (int)dnsHostName->length : 0,
|
|
+ dnsHostName != NULL ? (const char *)dnsHostName->data : "",
|
|
netbios_name, ntds_guid,
|
|
forest_name, base_domain);
|
|
return LDB_ERR_CONSTRAINT_VIOLATION;
|
|
@@ -686,9 +709,9 @@ static int acl_check_spn(TALLOC_CTX *mem_ctx,
|
|
struct ldb_result *netbios_res;
|
|
struct ldb_dn *partitions_dn = samdb_partitions_dn(ldb, tmp_ctx);
|
|
uint32_t userAccountControl;
|
|
- const char *samAccountName;
|
|
- const char *dnsHostName;
|
|
const char *netbios_name;
|
|
+ const struct ldb_val *dns_host_name_val = NULL;
|
|
+ const struct ldb_val *sam_account_name_val = NULL;
|
|
struct GUID ntds;
|
|
char *ntds_guid = NULL;
|
|
|
|
@@ -773,9 +796,31 @@ static int acl_check_spn(TALLOC_CTX *mem_ctx,
|
|
return ret;
|
|
}
|
|
|
|
+ dns_host_name_val = ldb_msg_find_ldb_val(acl_res->msgs[0], "dNSHostName");
|
|
+
|
|
+ ret = dsdb_msg_get_single_value(req->op.mod.message,
|
|
+ "dNSHostName",
|
|
+ dns_host_name_val,
|
|
+ &dns_host_name_val,
|
|
+ req->operation);
|
|
+ if (ret != LDB_SUCCESS) {
|
|
+ talloc_free(tmp_ctx);
|
|
+ return ret;
|
|
+ }
|
|
+
|
|
userAccountControl = ldb_msg_find_attr_as_uint(acl_res->msgs[0], "userAccountControl", 0);
|
|
- dnsHostName = ldb_msg_find_attr_as_string(acl_res->msgs[0], "dnsHostName", NULL);
|
|
- samAccountName = ldb_msg_find_attr_as_string(acl_res->msgs[0], "samAccountName", NULL);
|
|
+
|
|
+ sam_account_name_val = ldb_msg_find_ldb_val(acl_res->msgs[0], "sAMAccountName");
|
|
+
|
|
+ ret = dsdb_msg_get_single_value(req->op.mod.message,
|
|
+ "sAMAccountName",
|
|
+ sam_account_name_val,
|
|
+ &sam_account_name_val,
|
|
+ req->operation);
|
|
+ if (ret != LDB_SUCCESS) {
|
|
+ talloc_free(tmp_ctx);
|
|
+ return ret;
|
|
+ }
|
|
|
|
ret = dsdb_module_search(module, tmp_ctx,
|
|
&netbios_res, partitions_dn,
|
|
@@ -806,10 +851,10 @@ static int acl_check_spn(TALLOC_CTX *mem_ctx,
|
|
for (i=0; i < el->num_values; i++) {
|
|
ret = acl_validate_spn_value(tmp_ctx,
|
|
ldb,
|
|
- (char *)el->values[i].data,
|
|
+ &el->values[i],
|
|
userAccountControl,
|
|
- samAccountName,
|
|
- dnsHostName,
|
|
+ sam_account_name_val,
|
|
+ dns_host_name_val,
|
|
netbios_name,
|
|
ntds_guid);
|
|
if (ret != LDB_SUCCESS) {
|
|
--
|
|
1.8.3.1
|
|
|