s4:descriptor LDB module - make the "nTSecurityDescriptor" attribute fully behave as in AD

- fix crash when provided "nTSecurityDescriptor" attribute is empty
- print out the correct error codes if it's provided multi-valued
- simplify the "recalculate_sd" control handling

2 files changed, 117 insertions, 52 deletions

diff --git a/source4/dsdb/samdb/ldb_modules/descriptor.c b/source4/dsdb/samdb/ldb_modules/descriptor.c
index 731b7bcc2a..0515dfe592 100644
--- a/source4/dsdb/samdb/ldb_modules/descriptor.c
+++ b/source4/dsdb/samdb/ldb_modules/descriptor.c
@@ -55,6 +55,7 @@ struct descriptor_context {
 	struct ldb_reply *search_res;
 	struct ldb_reply *search_oc_res;
 	struct ldb_val *parentsd_val;
+	struct ldb_message_element *sd_element;
 	struct ldb_val *sd_val;
 	int (*step_fn)(struct descriptor_context *);
 };
@@ -553,14 +554,13 @@ static int descriptor_do_mod(struct descriptor_context *ac)
 	struct ldb_context *ldb;
 	const struct dsdb_schema *schema;
 	struct ldb_request *mod_req;
-	struct ldb_message_element *objectclass_element, *tmp_element, *oldsd_el;
+	struct ldb_message_element *objectclass_element, *oldsd_el;
 	struct ldb_val *oldsd_val = NULL;
 	int ret;
 	DATA_BLOB *sd;
 	const struct dsdb_class *objectclass;
 	struct ldb_control *sd_control;
 	struct ldb_control *sd_control2;
-	int flags = 0;
 	uint32_t sd_flags = 0;
 
 	ldb = ldb_module_get_ctx(ac->module);
@@ -578,7 +578,8 @@ static int descriptor_do_mod(struct descriptor_context *ac)
 	}
 
 	sd_control = ldb_request_get_control(ac->req, LDB_CONTROL_SD_FLAGS_OID);
-	sd_control2 = ldb_request_get_control(ac->req, LDB_CONTROL_RECALCULATE_SD_OID);
+	sd_control2 = ldb_request_get_control(ac->req,
+					      LDB_CONTROL_RECALCULATE_SD_OID);
 	if (sd_control) {
 		struct ldb_sd_flags_control *sdctr = (struct ldb_sd_flags_control *)sd_control->data;
 		sd_flags = sdctr->secinfo_flags;
@@ -586,34 +587,44 @@ static int descriptor_do_mod(struct descriptor_context *ac)
 		sd_flags = sd_flags & 0x0000000F;
 	}
 	if (sd_flags != 0) {
-		oldsd_el = ldb_msg_find_element(ac->search_oc_res->message, "nTSecurityDescriptor");
+		oldsd_el = ldb_msg_find_element(ac->search_oc_res->message,
+						"nTSecurityDescriptor");
 		if (oldsd_el) {
 			oldsd_val = oldsd_el->values;
 		}
 	}
-	sd = get_new_descriptor(ac->module, ac->msg->dn, ac, objectclass,
-				ac->parentsd_val, ac->sd_val, oldsd_val, sd_flags);
-	if (ac->sd_val) {
-		tmp_element = ldb_msg_find_element(ac->msg,
-						   "nTSecurityDescriptor");
-		flags = tmp_element->flags;
-		ldb_msg_remove_attr(ac->msg, "nTSecurityDescriptor");
-	}
 
-	if (sd) {
-		ret = ldb_msg_add_steal_value(ac->msg,
-					      "nTSecurityDescriptor", sd);
-		if (ret != LDB_SUCCESS) {
-			return ret;
-		}
-		tmp_element = ldb_msg_find_element(ac->msg,
-						   "nTSecurityDescriptor");
-		if (sd_control2) {
-			tmp_element->flags = LDB_FLAG_MOD_REPLACE;
-		} else {
-			tmp_element->flags = flags;
+	sd = get_new_descriptor(ac->module, ac->msg->dn, ac,
+				objectclass, ac->parentsd_val,
+				ac->sd_val, oldsd_val, sd_flags);
+	if (sd != NULL) {
+		if (ac->sd_val != NULL) {
+			ac->sd_element->values[0] = *sd;
+		} else if (sd_control2 != NULL) {
+			/* In this branche we really do force the recalculation
+			 * of the SD */
+			ldb_msg_remove_attr(ac->msg, "nTSecurityDescriptor");
+
+			ret = ldb_msg_add_steal_value(ac->msg,
+						      "nTSecurityDescriptor",
+						      sd);
+			if (ret != LDB_SUCCESS) {
+				return ret;
+			}
+			ac->sd_element = ldb_msg_find_element(ac->msg,
+							      "nTSecurityDescriptor");
+			ac->sd_element->flags = LDB_FLAG_MOD_REPLACE;
 		}
 	}
+
+	/* mark the controls as non-critical since we've handled them */
+	if (sd_control != NULL) {
+		sd_control->critical = 0;
+	}
+	if (sd_control2 != NULL) {
+		sd_control2->critical = 0;
+	}
+
 	ret = ldb_build_mod_req(&mod_req, ldb, ac,
 				ac->msg,
 				ac->req->controls,
@@ -623,11 +634,6 @@ static int descriptor_do_mod(struct descriptor_context *ac)
 	if (ret != LDB_SUCCESS) {
 		return ret;
 	}
-	/* mark it non-critical, so we don't get an error from the
-	   backend, but mark that we've handled it */
-	if (sd_control) {
-		sd_control->critical = 0;
-	}
 
 	return ldb_next_request(ac->module, mod_req);
 }
@@ -637,7 +643,7 @@ static int descriptor_do_add(struct descriptor_context *ac)
 	struct ldb_context *ldb;
 	const struct dsdb_schema *schema;
 	struct ldb_request *add_req;
-	struct ldb_message_element *objectclass_element, *sd_element = NULL;
+	struct ldb_message_element *objectclass_element;
 	int ret;
 	DATA_BLOB *sd;
 	const struct dsdb_class *objectclass;
@@ -676,12 +682,16 @@ static int descriptor_do_add(struct descriptor_context *ac)
 		return ldb_operr(ldb);
 	}
 
-
-	/* get the security descriptor values*/
-	sd_element = ldb_msg_find_element(ac->msg, "nTSecurityDescriptor");
-	if (sd_element) {
-		ac->sd_val = talloc_memdup(ac, &sd_element->values[0], sizeof(struct ldb_val));
+	/* Check if there is a valid security descriptor provided */
+	ac->sd_element = dsdb_get_single_valued_attr(ac->msg,
+						     "nTSecurityDescriptor",
+						     ac->req->operation);
+	if ((ac->sd_element != NULL) && (ac->sd_element->num_values == 1)) {
+		ac->sd_val = talloc_memdup(ac,
+					   &ac->sd_element->values[0],
+					   sizeof(struct ldb_val));
 	}
+
 	/* NC's have no parent */
 	/* FIXME: this has to be made dynamic at some point */
 	if ((ldb_dn_compare(ac->msg->dn, (ldb_get_schema_basedn(ldb))) == 0) ||
@@ -702,15 +712,16 @@ static int descriptor_do_add(struct descriptor_context *ac)
 		sd = get_new_descriptor(ac->module, ac->msg->dn, ac,
 					objectclass, ac->parentsd_val,
 					ac->sd_val, NULL, 0);
-		if (ac->sd_val) {
-			ldb_msg_remove_attr(ac->msg, "nTSecurityDescriptor");
-		}
-
-		if (sd) {
-			ret = ldb_msg_add_steal_value(ac->msg,
-						      "nTSecurityDescriptor", sd);
-			if (ret != LDB_SUCCESS) {
-				return ret;
+		if (sd != NULL) {
+			if (ac->sd_val != NULL) {
+				ac->sd_element->values[0] = *sd;
+			} else if (ac->sd_element == NULL) {
+				ret = ldb_msg_add_steal_value(ac->msg,
+							      "nTSecurityDescriptor",
+							      sd);
+				if (ret != LDB_SUCCESS) {
+					return ret;
+				}
 			}
 		}
 
@@ -759,9 +770,12 @@ static int descriptor_change(struct ldb_module *module, struct ldb_request *req)
 		break;
 	case LDB_MODIFY:
 		dn = req->op.mod.message->dn;
-		sd_element = ldb_msg_find_element(req->op.mod.message, "nTSecurityDescriptor");
-		/* This control allow forcing the recalculation of the SD */
-		sd_control = ldb_request_get_control(req, LDB_CONTROL_RECALCULATE_SD_OID);
+		sd_element = ldb_msg_find_element(req->op.mod.message,
+						  "nTSecurityDescriptor");
+		/* This control forces the recalculation of the SD also when
+		 * no modification is performed. */
+		sd_control = ldb_request_get_control(req,
+						     LDB_CONTROL_RECALCULATE_SD_OID);
 		if (!sd_element && !sd_control) {
 			return ldb_next_request(module, req);
 		}
diff --git a/source4/dsdb/tests/python/ldap.py b/source4/dsdb/tests/python/ldap.py
index b4fe8cd2ee..18af214fd7 100755
--- a/source4/dsdb/tests/python/ldap.py
+++ b/source4/dsdb/tests/python/ldap.py
@@ -1437,7 +1437,7 @@ objectClass: container
 
         res = self.ldb.search(base=("<WKGUID=ab1d30f3768811d1aded00c04fd8d5cd,%s>" % self.base_dn), scope=SCOPE_BASE, attrs=[])
         self.assertEquals(len(res), 1)
-        
+
         res2 = self.ldb.search(scope=SCOPE_BASE, attrs=["wellKnownObjects"], expression=("wellKnownObjects=B:32:ab1d30f3768811d1aded00c04fd8d5cd:%s" % res[0].dn))
         self.assertEquals(len(res2), 1)
 
@@ -2343,10 +2343,23 @@ objectClass: posixAccount"""% (self.base_dn))
         user_name = "testdescriptoruser1"
         user_dn = "CN=%s,CN=Users,%s" % (user_name, self.base_dn)
         #
-        # Test add_ldif() with SDDL security descriptor input
+        # Test an empty security descriptor (naturally this shouldn't work)
         #
         self.delete_force(self.ldb, user_dn)
         try:
+            self.ldb.add({ "dn": user_dn,
+                           "objectClass": "user",
+                           "sAMAccountName": user_name,
+                           "nTSecurityDescriptor": [] })
+            self.fail()
+        except LdbError, (num, _):
+            self.assertEquals(num, ERR_CONSTRAINT_VIOLATION)
+        finally:
+            self.delete_force(self.ldb, user_dn)
+        #
+        # Test add_ldif() with SDDL security descriptor input
+        #
+        try:
             sddl = "O:DUG:DUD:PAI(A;;RPWP;;;AU)S:PAI"
             self.ldb.add_ldif("""
 dn: """ + user_dn + """
@@ -2407,11 +2420,49 @@ nTSecurityDescriptor:: """ + desc_base64)
         user_name = "testdescriptoruser2"
         user_dn = "CN=%s,CN=Users,%s" % (user_name, self.base_dn)
         #
-        # Delete user object and test modify_ldif() with SDDL security descriptor input
+        # Test an empty security descriptor (naturally this shouldn't work)
+        #
+        self.delete_force(self.ldb, user_dn)
+        self.ldb.add({ "dn": user_dn,
+                       "objectClass": "user",
+                       "sAMAccountName": user_name })
+
+        m = Message()
+        m.dn = Dn(ldb, user_dn)
+        m["nTSecurityDescriptor"] = MessageElement([], FLAG_MOD_ADD,
+                                                   "nTSecurityDescriptor")
+        try:
+            self.ldb.modify(m)
+            self.fail()
+        except LdbError, (num, _):
+            self.assertEquals(num, ERR_CONSTRAINT_VIOLATION)
+
+        m = Message()
+        m.dn = Dn(ldb, user_dn)
+        m["nTSecurityDescriptor"] = MessageElement([], FLAG_MOD_REPLACE,
+                                                   "nTSecurityDescriptor")
+        try:
+            self.ldb.modify(m)
+            self.fail()
+        except LdbError, (num, _):
+            self.assertEquals(num, ERR_UNWILLING_TO_PERFORM)
+
+        m = Message()
+        m.dn = Dn(ldb, user_dn)
+        m["nTSecurityDescriptor"] = MessageElement([], FLAG_MOD_DELETE,
+                                                   "nTSecurityDescriptor")
+        try:
+            self.ldb.modify(m)
+            self.fail()
+        except LdbError, (num, _):
+            self.assertEquals(num, ERR_UNWILLING_TO_PERFORM)
+
+        self.delete_force(self.ldb, user_dn)
+        #
+        # Test modify_ldif() with SDDL security descriptor input
         # Add ACE to the original descriptor test
         #
         try:
-            self.delete_force(self.ldb, user_dn)
             self.ldb.add_ldif("""
 dn: """ + user_dn + """
 objectclass: user
@@ -2585,7 +2636,7 @@ class BaseDnTests(unittest.TestCase):
         res = self.ldb.search("", scope=SCOPE_BASE,
                 attrs=["namingContexts", "defaultNamingContext", "schemaNamingContext", "configurationNamingContext"])
         self.assertEquals(len(res), 1)
-        
+
         ncs = set([])
         for nc in res[0]["namingContexts"]:
             self.assertTrue(nc not in ncs)