From 735605a6b0ad3ed98d46d1df920e64879a8c69cc Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Wed, 10 Dec 2014 14:15:54 +1300 Subject: [PATCH] dsdb: Improve userAccountControl handling We now always check the ACL and invarient rules using the same function The change to libds is because UF_PARTIAL_SECRETS_ACCOUNT is a flag, not an account type This list should only be of the account exclusive account type bits. Bug: https://bugzilla.samba.org/show_bug.cgi?id=10993 Pair-programmed-with: Garming Sam Signed-off-by: Garming Sam Signed-off-by: Andrew Bartlett Reviewed-by: Stefan Metzmacher --- source4/dsdb/samdb/ldb_modules/samldb.c | 166 +++++++++++++++--- .../dsdb/tests/python/user_account_control.py | 24 ++- 2 files changed, 158 insertions(+), 32 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/samldb.c b/source4/dsdb/samdb/ldb_modules/samldb.c index c9dc010449f..36be6ad0b52 100644 --- a/source4/dsdb/samdb/ldb_modules/samldb.c +++ b/source4/dsdb/samdb/ldb_modules/samldb.c @@ -945,10 +945,10 @@ static int samldb_schema_info_update(struct samldb_ctx *ac) } static int samldb_prim_group_tester(struct samldb_ctx *ac, uint32_t rid); -static int samldb_check_user_account_control_acl(struct samldb_ctx *ac, - struct dom_sid *sid, - uint32_t user_account_control, - uint32_t user_account_control_old); +static int samldb_check_user_account_control_rules(struct samldb_ctx *ac, + struct dom_sid *sid, + uint32_t user_account_control, + uint32_t user_account_control_old); /* * "Objectclass" trigger (MS-SAMR 3.1.1.8.1) @@ -1066,9 +1066,10 @@ static int samldb_objectclass_trigger(struct samldb_ctx *ac) uac_generated = true; } - /* Temporary duplicate accounts aren't allowed */ - if ((user_account_control & UF_TEMP_DUPLICATE_ACCOUNT) != 0) { - return LDB_ERR_OTHER; + ret = samldb_check_user_account_control_rules(ac, NULL, + user_account_control, 0); + if (ret != LDB_SUCCESS) { + return ret; } /* Workstation and (read-only) DC objects do need objectclass "computer" */ @@ -1160,11 +1161,6 @@ static int samldb_objectclass_trigger(struct samldb_ctx *ac) } } - ret = samldb_check_user_account_control_acl(ac, NULL, - user_account_control, 0); - if (ret != LDB_SUCCESS) { - return ret; - } } break; } @@ -1452,6 +1448,110 @@ static int samldb_prim_group_trigger(struct samldb_ctx *ac) return ret; } +static int samldb_check_user_account_control_invariants(struct samldb_ctx *ac, + uint32_t user_account_control) +{ + int i, ret = 0; + bool need_check = false; + const struct uac_to_guid { + uint32_t uac; + bool never; + uint32_t needs; + uint32_t not_with; + const char *error_string; + } map[] = { + { + .uac = UF_TEMP_DUPLICATE_ACCOUNT, + .never = true, + .error_string = "Updating the UF_TEMP_DUPLICATE_ACCOUNT flag is never allowed" + }, + { + .uac = UF_PARTIAL_SECRETS_ACCOUNT, + .needs = UF_WORKSTATION_TRUST_ACCOUNT, + .error_string = "Setting UF_PARTIAL_SECRETS_ACCOUNT only permitted with UF_WORKSTATION_TRUST_ACCOUNT" + }, + { + .uac = UF_TRUSTED_FOR_DELEGATION, + .not_with = UF_PARTIAL_SECRETS_ACCOUNT, + .error_string = "Setting UF_TRUSTED_FOR_DELEGATION not allowed with UF_PARTIAL_SECRETS_ACCOUNT" + }, + { + .uac = UF_NORMAL_ACCOUNT, + .not_with = UF_ACCOUNT_TYPE_MASK & ~UF_NORMAL_ACCOUNT, + .error_string = "Setting more than one account type not permitted" + }, + { + .uac = UF_WORKSTATION_TRUST_ACCOUNT, + .not_with = UF_ACCOUNT_TYPE_MASK & ~UF_WORKSTATION_TRUST_ACCOUNT, + .error_string = "Setting more than one account type not permitted" + }, + { + .uac = UF_INTERDOMAIN_TRUST_ACCOUNT, + .not_with = UF_ACCOUNT_TYPE_MASK & ~UF_INTERDOMAIN_TRUST_ACCOUNT, + .error_string = "Setting more than one account type not permitted" + }, + { + .uac = UF_SERVER_TRUST_ACCOUNT, + .not_with = UF_ACCOUNT_TYPE_MASK & ~UF_SERVER_TRUST_ACCOUNT, + .error_string = "Setting more than one account type not permitted" + }, + { + .uac = UF_TRUSTED_FOR_DELEGATION, + .not_with = UF_PARTIAL_SECRETS_ACCOUNT, + .error_string = "Setting UF_TRUSTED_FOR_DELEGATION not allowed with UF_PARTIAL_SECRETS_ACCOUNT" + } + }; + + for (i = 0; i < ARRAY_SIZE(map); i++) { + if (user_account_control & map[i].uac) { + need_check = true; + break; + } + } + if (need_check == false) { + return LDB_SUCCESS; + } + + for (i = 0; i < ARRAY_SIZE(map); i++) { + uint32_t this_uac = user_account_control & map[i].uac; + if (this_uac != 0) { + if (map[i].never) { + ret = LDB_ERR_OTHER; + break; + } else if (map[i].needs != 0) { + if ((map[i].needs & user_account_control) == 0) { + ret = LDB_ERR_OTHER; + break; + } + } else if (map[i].not_with != 0) { + if ((map[i].not_with & user_account_control) != 0) { + ret = LDB_ERR_OTHER; + break; + } + } + } + } + if (ret != LDB_SUCCESS) { + switch (ac->req->operation) { + case LDB_ADD: + ldb_asprintf_errstring(ldb_module_get_ctx(ac->module), + "Failed to add %s: %s", + ldb_dn_get_linearized(ac->msg->dn), + map[i].error_string); + break; + case LDB_MODIFY: + ldb_asprintf_errstring(ldb_module_get_ctx(ac->module), + "Failed to modify %s: %s", + ldb_dn_get_linearized(ac->msg->dn), + map[i].error_string); + break; + default: + return ldb_module_operr(ac->module); + } + } + return ret; +} + /** * Validate that the restriction in point 5 of MS-SAMR 3.1.1.8.10 userAccountControl is honoured * @@ -1619,6 +1719,24 @@ static int samldb_check_user_account_control_acl(struct samldb_ctx *ac, return ret; } +static int samldb_check_user_account_control_rules(struct samldb_ctx *ac, + struct dom_sid *sid, + uint32_t user_account_control, + uint32_t user_account_control_old) +{ + int ret; + ret = samldb_check_user_account_control_invariants(ac, user_account_control); + if (ret != LDB_SUCCESS) { + return ret; + } + ret = samldb_check_user_account_control_acl(ac, sid, user_account_control, user_account_control_old); + if (ret != LDB_SUCCESS) { + return ret; + } + return ret; +} + + /** * This function is called on LDB modify operations. It performs some additions/ * replaces on the current LDB message when "userAccountControl" changes. @@ -1725,11 +1843,21 @@ static int samldb_user_account_control_change(struct samldb_ctx *ac) if (new_ufa == 0) { /* * When there is no account type embedded in "userAccountControl" - * fall back to the old. + * fall back to UF_NORMAL_ACCOUNT. */ - new_ufa = old_ufa; + new_ufa = UF_NORMAL_ACCOUNT; new_uac |= new_ufa; } + sid = samdb_result_dom_sid(res, res->msgs[0], "objectSid"); + if (sid == NULL) { + return ldb_module_operr(ac->module); + } + + ret = samldb_check_user_account_control_rules(ac, sid, new_uac, old_uac); + if (ret != LDB_SUCCESS) { + return ret; + } + new_atype = ds_uf2atype(new_ufa); new_pgrid = ds_uf2prim_group_rid(new_uac); @@ -1848,16 +1976,6 @@ static int samldb_user_account_control_change(struct samldb_ctx *ac) ldb_msg_remove_attr(ac->msg, "userAccountControl"); } - sid = samdb_result_dom_sid(res, res->msgs[0], "objectSid"); - if (sid == NULL) { - return ldb_module_operr(ac->module); - } - - ret = samldb_check_user_account_control_acl(ac, sid, new_uac, old_uac); - if (ret != LDB_SUCCESS) { - return ret; - } - return LDB_SUCCESS; } diff --git a/source4/dsdb/tests/python/user_account_control.py b/source4/dsdb/tests/python/user_account_control.py index 00501bbc337..69108835096 100644 --- a/source4/dsdb/tests/python/user_account_control.py +++ b/source4/dsdb/tests/python/user_account_control.py @@ -317,6 +317,16 @@ class UserAccountControlTests(samba.tests.TestCase): self.assertEqual(int(res[0]["userAccountControl"][0]), UF_NORMAL_ACCOUNT|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD) + m = ldb.Message() + m.dn = res[0].dn + m["userAccountControl"] = ldb.MessageElement(str(UF_WORKSTATION_TRUST_ACCOUNT|UF_PARTIAL_SECRETS_ACCOUNT|UF_TRUSTED_FOR_DELEGATION), + ldb.FLAG_MOD_REPLACE, "userAccountControl") + try: + self.admin_samdb.modify(m) + self.fail("Unexpectedly able to set userAccountControl to UF_WORKSTATION_TRUST_ACCOUNT|UF_PARTIAL_SECRETS_ACCOUNT|UF_TRUSTED_FOR_DELEGATION on %s" % m.dn) + except LdbError, (enum, estr): + self.assertEqual(ldb.ERR_OTHER, enum) + m = ldb.Message() m.dn = res[0].dn m["userAccountControl"] = ldb.MessageElement(str(UF_WORKSTATION_TRUST_ACCOUNT|UF_PARTIAL_SECRETS_ACCOUNT), @@ -340,7 +350,7 @@ class UserAccountControlTests(samba.tests.TestCase): scope=SCOPE_SUBTREE, attrs=["userAccountControl"]) - self.assertEqual(int(res[0]["userAccountControl"][0]), UF_WORKSTATION_TRUST_ACCOUNT| UF_ACCOUNTDISABLE) + self.assertEqual(int(res[0]["userAccountControl"][0]), UF_NORMAL_ACCOUNT| UF_ACCOUNTDISABLE) def test_uac_bits_set(self): @@ -372,10 +382,9 @@ class UserAccountControlTests(samba.tests.TestCase): # These bits really are privileged priv_bits = set([UF_INTERDOMAIN_TRUST_ACCOUNT, UF_SERVER_TRUST_ACCOUNT, - UF_TRUSTED_FOR_DELEGATION, UF_TRUSTED_TO_AUTHENTICATE_FOR_DELEGATION, - UF_PARTIAL_SECRETS_ACCOUNT]) + UF_TRUSTED_FOR_DELEGATION, UF_TRUSTED_TO_AUTHENTICATE_FOR_DELEGATION]) - invalid_bits = set([UF_TEMP_DUPLICATE_ACCOUNT]) + invalid_bits = set([UF_TEMP_DUPLICATE_ACCOUNT, UF_PARTIAL_SECRETS_ACCOUNT]) for bit in bits: m = ldb.Message() @@ -420,7 +429,7 @@ class UserAccountControlTests(samba.tests.TestCase): "description") self.samdb.modify(m) - invalid_bits = set([UF_TEMP_DUPLICATE_ACCOUNT]) + invalid_bits = set([UF_TEMP_DUPLICATE_ACCOUNT, UF_PARTIAL_SECRETS_ACCOUNT]) super_priv_bits = set([UF_INTERDOMAIN_TRUST_ACCOUNT]) @@ -483,7 +492,7 @@ class UserAccountControlTests(samba.tests.TestCase): self.sd_utils.dacl_add_ace("OU=test_computer_ou1," + self.base_dn, mod) - invalid_bits = set([UF_TEMP_DUPLICATE_ACCOUNT]) + invalid_bits = set([UF_TEMP_DUPLICATE_ACCOUNT, UF_PARTIAL_SECRETS_ACCOUNT]) # These bits are privileged, but authenticated users have that CAR by default, so this is a pain to test priv_to_auth_users_bits = set([UF_PASSWD_NOTREQD, UF_ENCRYPTED_TEXT_PASSWORD_ALLOWED, @@ -491,8 +500,7 @@ class UserAccountControlTests(samba.tests.TestCase): # These bits really are privileged priv_bits = set([UF_INTERDOMAIN_TRUST_ACCOUNT, UF_SERVER_TRUST_ACCOUNT, - UF_TRUSTED_FOR_DELEGATION, UF_TRUSTED_TO_AUTHENTICATE_FOR_DELEGATION, - UF_PARTIAL_SECRETS_ACCOUNT]) + UF_TRUSTED_FOR_DELEGATION, UF_TRUSTED_TO_AUTHENTICATE_FOR_DELEGATION]) for bit in bits: try: