From 53d0e5d31e0f50d632771d835a5f97ce266eb4ba Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Wed, 22 Sep 2021 11:29:02 +1200 Subject: [PATCH] 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 Reviewed-by: Douglas Bagnall --- selftest/knownfail.d/uac_dollar_lock | 1 - selftest/knownfail.d/uac_mod_lock | 12 --- source4/dsdb/samdb/ldb_modules/samldb.c | 171 ++++++++++++++++++++++++++++---- 3 files changed, 154 insertions(+), 30 deletions(-) delete mode 100644 selftest/knownfail.d/uac_dollar_lock diff --git a/selftest/knownfail.d/uac_dollar_lock b/selftest/knownfail.d/uac_dollar_lock deleted file mode 100644 index 8c70c859fa4..00000000000 --- a/selftest/knownfail.d/uac_dollar_lock +++ /dev/null @@ -1 +0,0 @@ -^samba4.user_account_control.python\(.*\).__main__.UserAccountControlTests.test_objectclass_uac_dollar_lock_UF_WORKSTATION_TRUST_ACCOUNT_computer_cc_plain \ No newline at end of file diff --git a/selftest/knownfail.d/uac_mod_lock b/selftest/knownfail.d/uac_mod_lock index a84a236c355..068a47c6e61 100644 --- a/selftest/knownfail.d/uac_mod_lock +++ b/selftest/knownfail.d/uac_mod_lock @@ -20,21 +20,9 @@ ^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_priv_user_UF_NORMAL_ACCOUNT_to_computer_UF_NORMAL_ACCOUNT_remove_dollar ^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_priv_user_UF_NORMAL_ACCOUNT_to_computer_UF_WORKSTATION_TRUST_ACCOUNT_keep_dollar ^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_priv_user_UF_NORMAL_ACCOUNT_to_computer_UF_WORKSTATION_TRUST_ACCOUNT_remove_dollar -^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_wp_computer_UF_NORMAL_ACCOUNT_to_None_UF_NORMAL_ACCOUNT_remove_dollar -^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_wp_computer_UF_NORMAL_ACCOUNT_to_computer_UF_NORMAL_ACCOUNT_remove_dollar ^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_wp_computer_UF_NORMAL_ACCOUNT_to_user_UF_NORMAL_ACCOUNT_keep_dollar -^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_wp_computer_UF_NORMAL_ACCOUNT_to_user_UF_NORMAL_ACCOUNT_remove_dollar -^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_wp_computer_UF_SERVER_TRUST_ACCOUNT_to_None_UF_SERVER_TRUST_ACCOUNT_remove_dollar -^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_wp_computer_UF_SERVER_TRUST_ACCOUNT_to_None_UF_WORKSTATION_TRUST_ACCOUNT_remove_dollar -^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_wp_computer_UF_SERVER_TRUST_ACCOUNT_to_computer_UF_SERVER_TRUST_ACCOUNT_remove_dollar -^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_wp_computer_UF_SERVER_TRUST_ACCOUNT_to_computer_UF_WORKSTATION_TRUST_ACCOUNT_remove_dollar ^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_wp_computer_UF_SERVER_TRUST_ACCOUNT_to_user_UF_SERVER_TRUST_ACCOUNT_keep_dollar -^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_wp_computer_UF_SERVER_TRUST_ACCOUNT_to_user_UF_SERVER_TRUST_ACCOUNT_remove_dollar ^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_wp_computer_UF_SERVER_TRUST_ACCOUNT_to_user_UF_WORKSTATION_TRUST_ACCOUNT_keep_dollar -^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_wp_computer_UF_SERVER_TRUST_ACCOUNT_to_user_UF_WORKSTATION_TRUST_ACCOUNT_remove_dollar -^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_wp_computer_UF_WORKSTATION_TRUST_ACCOUNT_to_None_UF_WORKSTATION_TRUST_ACCOUNT_remove_dollar -^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_wp_computer_UF_WORKSTATION_TRUST_ACCOUNT_to_computer_UF_WORKSTATION_TRUST_ACCOUNT_remove_dollar ^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_wp_computer_UF_WORKSTATION_TRUST_ACCOUNT_to_user_UF_WORKSTATION_TRUST_ACCOUNT_keep_dollar -^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_wp_computer_UF_WORKSTATION_TRUST_ACCOUNT_to_user_UF_WORKSTATION_TRUST_ACCOUNT_remove_dollar ^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_wp_user_UF_NORMAL_ACCOUNT_to_computer_UF_NORMAL_ACCOUNT_keep_dollar ^samba4.user_account_control.python\(ad_dc_default\).__main__.UserAccountControlTests.test_mod_lock_wp_user_UF_NORMAL_ACCOUNT_to_computer_UF_NORMAL_ACCOUNT_remove_dollar diff --git a/source4/dsdb/samdb/ldb_modules/samldb.c b/source4/dsdb/samdb/ldb_modules/samldb.c index 3eeab82126f..e947f788810 100644 --- a/source4/dsdb/samdb/ldb_modules/samldb.c +++ b/source4/dsdb/samdb/ldb_modules/samldb.c @@ -71,6 +71,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; @@ -235,12 +242,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; } @@ -557,17 +638,31 @@ 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); } @@ -576,11 +671,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; } @@ -1495,6 +1589,20 @@ static int samldb_objectclass_trigger(struct samldb_ctx *ac) 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); if (ret != LDB_SUCCESS) { @@ -4010,12 +4118,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.11.4.GIT