CVE-2015-8467: samdb: Match MS15-096 behaviour for userAccountControl

Swapping between account types is now restricted

Bug: https://bugzilla.samba.org/show_bug.cgi?id=11552

Signed-off-by: Andrew Bartlett <abartlet@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
Autobuild-User(master): Stefan Metzmacher <metze@samba.org>
Autobuild-Date(master): Wed Dec 16 16:03:18 CET 2015 on sn-devel-104

diff --git a/source4/dsdb/samdb/ldb_modules/samldb.c b/source4/dsdb/samdb/ldb_modules/samldb.c
index fbb392de42b5757466ecfa8b90d79679b2ba2826..153c85ea571a824fa6cd3f83704424c7aec53595 100644 (file)
--- a/source4/dsdb/samdb/ldb_modules/samldb.c
+++ b/source4/dsdb/samdb/ldb_modules/samldb.c
@@ -1558,12 +1558,15 @@ static int samldb_check_user_account_control_acl(struct samldb_ctx *ac,
        struct security_token *user_token;
        struct security_descriptor *domain_sd;
        struct ldb_dn *domain_dn = ldb_get_default_basedn(ldb_module_get_ctx(ac->module));
+       struct ldb_context *ldb = ldb_module_get_ctx(ac->module);
        const struct uac_to_guid {
                uint32_t uac;
+               uint32_t priv_to_change_from;
                const char *oid;
                const char *guid;
                enum sec_privilege privilege;
                bool delete_is_privileged;
+               bool admin_required;
                const char *error_string;
        } map[] = {
                {
@@ -1591,6 +1594,16 @@ static int samldb_check_user_account_control_acl(struct samldb_ctx *ac,
                        .guid = GUID_DRS_DS_INSTALL_REPLICA,
                        .error_string = "Adding the UF_PARTIAL_SECRETS_ACCOUNT bit in userAccountControl requires the DS-Install-Replica right that was not given on the Domain object"
                },
+               {
+                       .uac = UF_WORKSTATION_TRUST_ACCOUNT,
+                       .priv_to_change_from = UF_NORMAL_ACCOUNT,
+                       .error_string = "Swapping UF_NORMAL_ACCOUNT to UF_WORKSTATION_TRUST_ACCOUNT requires the user to be a member of the domain admins group"
+               },
+               {
+                       .uac = UF_NORMAL_ACCOUNT,
+                       .priv_to_change_from = UF_WORKSTATION_TRUST_ACCOUNT,
+                       .error_string = "Swapping UF_WORKSTATION_TRUST_ACCOUNT to UF_NORMAL_ACCOUNT requires the user to be a member of the domain admins group"
+               },
                {
                        .uac = UF_INTERDOMAIN_TRUST_ACCOUNT,
                        .oid = DSDB_CONTROL_PERMIT_INTERDOMAIN_TRUST_UAC_OID,
@@ -1643,7 +1656,7 @@ static int samldb_check_user_account_control_acl(struct samldb_ctx *ac,
                return ldb_module_operr(ac->module);
        }
 
-       ret = dsdb_get_sd_from_ldb_message(ldb_module_get_ctx(ac->module),
+       ret = dsdb_get_sd_from_ldb_message(ldb,
                                           ac, res->msgs[0], &domain_sd);
 
        if (ret != LDB_SUCCESS) {
@@ -1670,12 +1683,19 @@ static int samldb_check_user_account_control_acl(struct samldb_ctx *ac,
                                if (have_priv == false) {
                                        ret = LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS;
                                }
-                       } else {
+                       } else if (map[i].priv_to_change_from & user_account_control_old) {
+                               bool is_admin = security_token_has_builtin_administrators(user_token);
+                               if (is_admin == false) {
+                                       ret = LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS;
+                               }
+                       } else if (map[i].guid) {
                                ret = acl_check_extended_right(ac, domain_sd,
                                                               user_token,
                                                               map[i].guid,
                                                               SEC_ADS_CONTROL_ACCESS,
                                                               sid);
+                       } else {
+                               ret = LDB_SUCCESS;
                        }
                        if (ret != LDB_SUCCESS) {
                                break;

diff --git a/source4/dsdb/tests/python/user_account_control.py b/source4/dsdb/tests/python/user_account_control.py
index 16c7f81d477fe10cb547255ccb5cc7910cde261d..a53c4f93c5d19a4355ef0ddb9b02113b6e855a69 100644 (file)
--- a/source4/dsdb/tests/python/user_account_control.py
+++ b/source4/dsdb/tests/python/user_account_control.py
@@ -240,6 +240,16 @@ class UserAccountControlTests(samba.tests.TestCase):
         m.dn = res[0].dn
         m["userAccountControl"] = ldb.MessageElement(str(samba.dsdb.UF_WORKSTATION_TRUST_ACCOUNT),
                                                      ldb.FLAG_MOD_REPLACE, "userAccountControl")
+        try:
+            self.samdb.modify(m)
+            self.fail("Unexpectedly able to set userAccountControl to be an Workstation on %s" % m.dn)
+        except LdbError, (enum, estr):
+            self.assertEqual(ldb.ERR_INSUFFICIENT_ACCESS_RIGHTS, enum)
+
+        m = ldb.Message()
+        m.dn = res[0].dn
+        m["userAccountControl"] = ldb.MessageElement(str(samba.dsdb.UF_NORMAL_ACCOUNT),
+                                                     ldb.FLAG_MOD_REPLACE, "userAccountControl")
         self.samdb.modify(m)
 
         m = ldb.Message()
@@ -306,7 +316,12 @@ class UserAccountControlTests(samba.tests.TestCase):
         m.dn = res[0].dn
         m["userAccountControl"] = ldb.MessageElement(str(samba.dsdb.UF_WORKSTATION_TRUST_ACCOUNT),
                                                      ldb.FLAG_MOD_REPLACE, "userAccountControl")
-        self.samdb.modify(m)
+        try:
+            self.samdb.modify(m)
+            self.fail("Unexpectedly able to set userAccountControl to be an Workstation on %s" % m.dn)
+        except LdbError, (enum, estr):
+            self.assertEqual(ldb.ERR_INSUFFICIENT_ACCESS_RIGHTS, enum)
+
 
     def test_admin_mod_uac(self):
         computername=self.computernames[0]
@@ -382,9 +397,10 @@ class UserAccountControlTests(samba.tests.TestCase):
         priv_to_auth_users_bits = set([UF_PASSWD_NOTREQD, UF_ENCRYPTED_TEXT_PASSWORD_ALLOWED,
                                        UF_DONT_EXPIRE_PASSWD])
 
-        # These bits really are privileged
+        # These bits really are privileged, or can't be changed from UF_NORMAL as a non-admin
         priv_bits = set([UF_INTERDOMAIN_TRUST_ACCOUNT, UF_SERVER_TRUST_ACCOUNT,
-                         UF_TRUSTED_FOR_DELEGATION, UF_TRUSTED_TO_AUTHENTICATE_FOR_DELEGATION])
+                         UF_TRUSTED_FOR_DELEGATION, UF_TRUSTED_TO_AUTHENTICATE_FOR_DELEGATION,
+                         UF_WORKSTATION_TRUST_ACCOUNT])
 
         invalid_bits = set([UF_TEMP_DUPLICATE_ACCOUNT, UF_PARTIAL_SECRETS_ACCOUNT])
 
@@ -446,7 +462,7 @@ class UserAccountControlTests(samba.tests.TestCase):
                             int("0x10000000", 16), int("0x20000000", 16), int("0x40000000", 16), int("0x80000000", 16)])
         super_priv_bits = set([UF_INTERDOMAIN_TRUST_ACCOUNT])
 
-        priv_to_remove_bits = set([UF_TRUSTED_FOR_DELEGATION, UF_TRUSTED_TO_AUTHENTICATE_FOR_DELEGATION])
+        priv_to_remove_bits = set([UF_TRUSTED_FOR_DELEGATION, UF_TRUSTED_TO_AUTHENTICATE_FOR_DELEGATION, UF_WORKSTATION_TRUST_ACCOUNT])
 
         for bit in bits:
             # Reset this to the initial position, just to be sure
@@ -507,6 +523,31 @@ class UserAccountControlTests(samba.tests.TestCase):
             except LdbError, (enum, estr):
                 self.fail("Unable to set userAccountControl bit 0x%08X on %s: %s" % (bit, m.dn, estr))
 
+            res = self.admin_samdb.search("%s" % self.base_dn,
+                                          expression="(&(objectClass=computer)(samAccountName=%s$))" % computername,
+                                          scope=SCOPE_SUBTREE,
+                                          attrs=["userAccountControl"])
+
+            if bit in account_types:
+                self.assertEqual(int(res[0]["userAccountControl"][0]),
+                                 bit|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD,
+                                 "bit 0X%08x should have been added (0X%08x vs 0X%08x)"
+                                 % (bit, int(res[0]["userAccountControl"][0]),
+                                    bit|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD))
+            elif bit in ignored_bits:
+                self.assertEqual(int(res[0]["userAccountControl"][0]),
+                                 UF_NORMAL_ACCOUNT|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD,
+                                 "bit 0X%08x should have been added (0X%08x vs 0X%08x)"
+                                 % (bit, int(res[0]["userAccountControl"][0]),
+                                    UF_NORMAL_ACCOUNT|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD))
+
+            else:
+                self.assertEqual(int(res[0]["userAccountControl"][0]),
+                                 bit|UF_NORMAL_ACCOUNT|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD,
+                                 "bit 0X%08x should have been added (0X%08x vs 0X%08x)"
+                                 % (bit, int(res[0]["userAccountControl"][0]),
+                                    bit|UF_NORMAL_ACCOUNT|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD))
+
             try:
                 m = ldb.Message()
                 m.dn = res[0].dn
@@ -520,7 +561,7 @@ class UserAccountControlTests(samba.tests.TestCase):
                 if bit in priv_to_remove_bits:
                     self.assertEqual(enum, ldb.ERR_INSUFFICIENT_ACCESS_RIGHTS)
                 else:
-                    self.fail("Unexpectedly able to remove userAccountControl bit 0x%08X on %s: %s" % (bit, m.dn, estr))
+                    self.fail("Unexpectedly unable to remove userAccountControl bit 0x%08X on %s: %s" % (bit, m.dn, estr))
 
             res = self.admin_samdb.search("%s" % self.base_dn,
                                           expression="(&(objectClass=computer)(samAccountName=%s$))" % computername,
@@ -528,9 +569,14 @@ class UserAccountControlTests(samba.tests.TestCase):
                                           attrs=["userAccountControl"])
 
             if bit in priv_to_remove_bits:
-                self.assertEqual(int(res[0]["userAccountControl"][0]),
-                                 bit|UF_NORMAL_ACCOUNT|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD,
-                                 "bit 0X%08x should not have been removed" % bit)
+                if bit in account_types:
+                    self.assertEqual(int(res[0]["userAccountControl"][0]),
+                                     bit|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD,
+                                     "bit 0X%08x should not have been removed" % bit)
+                else:
+                    self.assertEqual(int(res[0]["userAccountControl"][0]),
+                                     bit|UF_NORMAL_ACCOUNT|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD,
+                                     "bit 0X%08x should not have been removed" % bit)
             else:
                 self.assertEqual(int(res[0]["userAccountControl"][0]),
                                  UF_NORMAL_ACCOUNT|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD,
@@ -553,7 +599,6 @@ 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, 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,
                                        UF_DONT_EXPIRE_PASSWD])