250 lines
7.5 KiB
Diff
250 lines
7.5 KiB
Diff
From a76d5d6202379c1f38f7f91df1cce006754463b2 Mon Sep 17 00:00:00 2001
|
|
From: Andrew Bartlett <abartlet@samba.org>
|
|
Date: Wed, 22 Sep 2021 11:29:02 +1200
|
|
Subject: [PATCH 070/266] CVE-2020-25722 dsdb: Add restrictions on computer
|
|
accounts without a trailing $
|
|
|
|
BUG: https://bugzilla.samba.org/show_bug.cgi?id=14753
|
|
|
|
Signed-off-by: Andrew Bartlett <abartlet@samba.org>
|
|
Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
|
|
|
|
Conflict:remove test
|
|
Reference:https://gitlab.com/samba-team/samba/-/commit/a76d5d6202379c1f38f7f91df1cce006754463b2
|
|
---
|
|
source4/dsdb/samdb/ldb_modules/samldb.c | 172 +++++++++++++++++++++---
|
|
1 file changed, 155 insertions(+), 17 deletions(-)
|
|
|
|
diff --git a/source4/dsdb/samdb/ldb_modules/samldb.c b/source4/dsdb/samdb/ldb_modules/samldb.c
|
|
index 283a2de..7881dd0 100644
|
|
--- a/source4/dsdb/samdb/ldb_modules/samldb.c
|
|
+++ b/source4/dsdb/samdb/ldb_modules/samldb.c
|
|
@@ -68,6 +68,13 @@ struct samldb_ctx {
|
|
/* used for add operations */
|
|
enum samldb_add_type type;
|
|
|
|
+ /*
|
|
+ * should we apply the need_trailing_dollar restriction to
|
|
+ * samAccountName
|
|
+ */
|
|
+
|
|
+ bool need_trailing_dollar;
|
|
+
|
|
/* the resulting message */
|
|
struct ldb_message *msg;
|
|
|
|
@@ -232,12 +239,86 @@ static int samldb_unique_attr_check(struct samldb_ctx *ac, const char *attr,
|
|
|
|
static int samldb_sam_accountname_valid_check(struct samldb_ctx *ac)
|
|
{
|
|
- int ret = samldb_unique_attr_check(ac, "samAccountName", NULL,
|
|
- ldb_get_default_basedn(
|
|
- ldb_module_get_ctx(ac->module)));
|
|
- if (ret == LDB_ERR_OBJECT_CLASS_VIOLATION) {
|
|
+ int ret = 0;
|
|
+ bool is_admin;
|
|
+ struct security_token *user_token = NULL;
|
|
+ struct ldb_context *ldb = ldb_module_get_ctx(ac->module);
|
|
+ struct ldb_message_element *el = dsdb_get_single_valued_attr(ac->msg, "samAccountName",
|
|
+ ac->req->operation);
|
|
+ if (el == NULL || el->num_values == 0) {
|
|
+ ldb_asprintf_errstring(ldb,
|
|
+ "%08X: samldb: 'samAccountName' can't be deleted/empty!",
|
|
+ W_ERROR_V(WERR_DS_ILLEGAL_MOD_OPERATION));
|
|
+ if (ac->req->operation == LDB_ADD) {
|
|
+ return LDB_ERR_CONSTRAINT_VIOLATION;
|
|
+ } else {
|
|
+ return LDB_ERR_UNWILLING_TO_PERFORM;
|
|
+ }
|
|
+ }
|
|
+
|
|
+ ret = samldb_unique_attr_check(ac, "samAccountName", NULL,
|
|
+ ldb_get_default_basedn(
|
|
+ ldb_module_get_ctx(ac->module)));
|
|
+
|
|
+ /*
|
|
+ * Error code munging to try and match what must be some quite
|
|
+ * strange code-paths in Windows
|
|
+ */
|
|
+ if (ret == LDB_ERR_CONSTRAINT_VIOLATION
|
|
+ && ac->req->operation == LDB_MODIFY) {
|
|
+ ret = LDB_ERR_ATTRIBUTE_OR_VALUE_EXISTS;
|
|
+ } else if (ret == LDB_ERR_OBJECT_CLASS_VIOLATION) {
|
|
ret = LDB_ERR_CONSTRAINT_VIOLATION;
|
|
}
|
|
+
|
|
+ if (ret != LDB_SUCCESS) {
|
|
+ return ret;
|
|
+ }
|
|
+
|
|
+ if (!ac->need_trailing_dollar) {
|
|
+ return LDB_SUCCESS;
|
|
+ }
|
|
+
|
|
+ /* This does not permit a single $ */
|
|
+ if (el->values[0].length < 2) {
|
|
+ ldb_asprintf_errstring(ldb,
|
|
+ "%08X: samldb: 'samAccountName' "
|
|
+ "can't just be one character!",
|
|
+ W_ERROR_V(WERR_DS_ILLEGAL_MOD_OPERATION));
|
|
+ return LDB_ERR_UNWILLING_TO_PERFORM;
|
|
+ }
|
|
+
|
|
+ user_token = acl_user_token(ac->module);
|
|
+ if (user_token == NULL) {
|
|
+ return LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS;
|
|
+ }
|
|
+
|
|
+ is_admin
|
|
+ = security_token_has_builtin_administrators(user_token);
|
|
+
|
|
+ if (is_admin) {
|
|
+ /*
|
|
+ * Administrators are allowed to select strange names.
|
|
+ * This is poor practice but not prevented.
|
|
+ */
|
|
+ return false;
|
|
+ }
|
|
+
|
|
+ if (el->values[0].data[el->values[0].length - 1] != '$') {
|
|
+ ldb_asprintf_errstring(ldb,
|
|
+ "%08X: samldb: 'samAccountName' "
|
|
+ "must have a trailing $!",
|
|
+ W_ERROR_V(WERR_DS_ILLEGAL_MOD_OPERATION));
|
|
+ return LDB_ERR_UNWILLING_TO_PERFORM;
|
|
+ }
|
|
+ if (el->values[0].data[el->values[0].length - 2] == '$') {
|
|
+ ldb_asprintf_errstring(ldb,
|
|
+ "%08X: samldb: 'samAccountName' "
|
|
+ "must not have a double trailing $!",
|
|
+ W_ERROR_V(WERR_DS_ILLEGAL_MOD_OPERATION));
|
|
+ return LDB_ERR_UNWILLING_TO_PERFORM;
|
|
+ }
|
|
+
|
|
return ret;
|
|
}
|
|
|
|
@@ -554,17 +635,32 @@ static int samldb_schema_add_handle_mapiid(struct samldb_ctx *ac)
|
|
}
|
|
|
|
/* sAMAccountName handling */
|
|
-static int samldb_generate_sAMAccountName(struct ldb_context *ldb,
|
|
+static int samldb_generate_sAMAccountName(struct samldb_ctx *ac,
|
|
struct ldb_message *msg)
|
|
{
|
|
+ struct ldb_context *ldb = ldb_module_get_ctx(ac->module);
|
|
char *name;
|
|
|
|
- /* Format: $000000-000000000000 */
|
|
+ /*
|
|
+ * This is currently a Samba-only behaviour, to add a trailing
|
|
+ * $ even for the generated accounts.
|
|
+ */
|
|
+
|
|
+ if (ac->need_trailing_dollar) {
|
|
+ /* Format: $000000-00000000000$ */
|
|
+ name = talloc_asprintf(msg, "$%.6X-%.6X%.5X$",
|
|
+ (unsigned int)generate_random(),
|
|
+ (unsigned int)generate_random(),
|
|
+ (unsigned int)generate_random());
|
|
+ } else {
|
|
+ /* Format: $000000-000000000000 */
|
|
+
|
|
+ name = talloc_asprintf(msg, "$%.6X-%.6X%.6X",
|
|
+ (unsigned int)generate_random(),
|
|
+ (unsigned int)generate_random(),
|
|
+ (unsigned int)generate_random());
|
|
+ }
|
|
|
|
- name = talloc_asprintf(msg, "$%.6X-%.6X%.6X",
|
|
- (unsigned int)generate_random(),
|
|
- (unsigned int)generate_random(),
|
|
- (unsigned int)generate_random());
|
|
if (name == NULL) {
|
|
return ldb_oom(ldb);
|
|
}
|
|
@@ -573,11 +669,10 @@ static int samldb_generate_sAMAccountName(struct ldb_context *ldb,
|
|
|
|
static int samldb_check_sAMAccountName(struct samldb_ctx *ac)
|
|
{
|
|
- struct ldb_context *ldb = ldb_module_get_ctx(ac->module);
|
|
int ret;
|
|
|
|
if (ldb_msg_find_element(ac->msg, "sAMAccountName") == NULL) {
|
|
- ret = samldb_generate_sAMAccountName(ldb, ac->msg);
|
|
+ ret = samldb_generate_sAMAccountName(ac, ac->msg);
|
|
if (ret != LDB_SUCCESS) {
|
|
return ret;
|
|
}
|
|
@@ -1473,6 +1568,20 @@ static int samldb_objectclass_trigger(struct samldb_ctx *ac)
|
|
if (ret != LDB_SUCCESS) {
|
|
return ret;
|
|
}
|
|
+
|
|
+ /*
|
|
+ * Require, for non-admin modifications, a trailing $
|
|
+ * for either objectclass=computer or a trust account
|
|
+ * type in userAccountControl
|
|
+ */
|
|
+ if ((user_account_control
|
|
+ & UF_TRUST_ACCOUNT_MASK) != 0) {
|
|
+ ac->need_trailing_dollar = true;
|
|
+ }
|
|
+
|
|
+ if (is_computer_objectclass) {
|
|
+ ac->need_trailing_dollar = true;
|
|
+ }
|
|
|
|
/* add "sAMAccountType" attribute */
|
|
ret = dsdb_user_obj_set_account_type(ldb, ac->msg, user_account_control, NULL);
|
|
@@ -3989,12 +4098,41 @@ static int samldb_modify(struct ldb_module *module, struct ldb_request *req)
|
|
|
|
el = ldb_msg_find_element(ac->msg, "sAMAccountName");
|
|
if (el != NULL) {
|
|
+ uint32_t user_account_control;
|
|
+ struct ldb_result *res = NULL;
|
|
+ const char * const attrs[] = { "userAccountControl",
|
|
+ "objectclass",
|
|
+ NULL };
|
|
+ ret = dsdb_module_search_dn(ac->module,
|
|
+ ac,
|
|
+ &res,
|
|
+ ac->msg->dn,
|
|
+ attrs,
|
|
+ DSDB_FLAG_NEXT_MODULE | DSDB_SEARCH_SHOW_DELETED,
|
|
+ ac->req);
|
|
+ if (ret != LDB_SUCCESS) {
|
|
+ return ret;
|
|
+ }
|
|
+
|
|
+ user_account_control
|
|
+ = ldb_msg_find_attr_as_uint(res->msgs[0],
|
|
+ "userAccountControl",
|
|
+ 0);
|
|
+
|
|
+ if ((user_account_control
|
|
+ & UF_TRUST_ACCOUNT_MASK) != 0) {
|
|
+ ac->need_trailing_dollar = true;
|
|
+
|
|
+ } else if (samdb_find_attribute(ldb,
|
|
+ res->msgs[0],
|
|
+ "objectclass",
|
|
+ "computer")
|
|
+ != NULL) {
|
|
+ ac->need_trailing_dollar = true;
|
|
+ }
|
|
+
|
|
ret = samldb_sam_accountname_valid_check(ac);
|
|
- /*
|
|
- * Other errors are checked for elsewhere, we just
|
|
- * want to prevent duplicates
|
|
- */
|
|
- if (ret == LDB_ERR_ENTRY_ALREADY_EXISTS) {
|
|
+ if (ret != LDB_SUCCESS) {
|
|
return ret;
|
|
}
|
|
}
|
|
--
|
|
2.27.0
|
|
|