CVE-2018-10919 security: Move object-specific access checks into separate function

Object-specific access checks refer to a specific section of the
MS-ADTS, and the code closely matches the spec. We need to extend this
logic to properly handle the Control-Access Right (CR), so it makes
sense to split the logic out into its own function.

This patch just moves the code, and should not alter the logic (apart
from ading in the boolean grant_access return variable.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13434

Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
Reviewed-by: Gary Lockyer <gary@catalyst.net.nz>

libcli/security/access_check.c

@@ -374,6 +374,57 @@ static const struct GUID *get_ace_object_type(struct security_ace *ace)
 	return NULL;
 }
 
+/**
+ * Evaluates access rights specified in a object-specific ACE for an AD object.
+ * This logic corresponds to MS-ADTS 5.1.3.3.3 Checking Object-Specific Access.
+ * @param[in] ace - the ACE being processed
+ * @param[in/out] tree - remaining_access gets updated for the tree
+ * @param[out] grant_access - set to true if the ACE grants sufficient access
+ *                            rights to the object/attribute
+ * @returns NT_STATUS_OK, unless access was denied
+ */
+static NTSTATUS check_object_specific_access(struct security_ace *ace,
+					     struct object_tree *tree,
+					     bool *grant_access)
+{
+	struct object_tree *node = NULL;
+	const struct GUID *type = NULL;
+
+	*grant_access = false;
+
+	/*
+	 * check only in case we have provided a tree,
+	 * the ACE has an object type and that type
+	 * is in the tree
+	 */
+	type = get_ace_object_type(ace);
+
+	if (!tree) {
+		return NT_STATUS_OK;
+	}
+
+	if (!type) {
+		node = tree;
+	} else {
+		if (!(node = get_object_tree_by_GUID(tree, type))) {
+			return NT_STATUS_OK;
+		}
+	}
+
+	if (ace->type == SEC_ACE_TYPE_ACCESS_ALLOWED_OBJECT) {
+		object_tree_modify_access(node, ace->access_mask);
+		if (node->remaining_access == 0) {
+			*grant_access = true;
+			return NT_STATUS_OK;
+		}
+	} else {
+		if (node->remaining_access & ace->access_mask){
+			return NT_STATUS_ACCESS_DENIED;
+		}
+	}
+	return NT_STATUS_OK;
+}
+
 /**
  * @brief Perform directoryservice (DS) related access checks for a given user
  *
@@ -405,8 +456,6 @@ NTSTATUS sec_access_check_ds(const struct security_descriptor *sd,
 {
 	uint32_t i;
 	uint32_t bits_remaining;
-	struct object_tree *node;
-	const struct GUID *type;
 	struct dom_sid self_sid;
 
 	dom_sid_parse(SID_NT_SELF, &self_sid);
@@ -456,6 +505,8 @@ NTSTATUS sec_access_check_ds(const struct security_descriptor *sd,
 	for (i=0; bits_remaining && i < sd->dacl->num_aces; i++) {
 		struct dom_sid *trustee;
 		struct security_ace *ace = &sd->dacl->aces[i];
+		NTSTATUS status;
+		bool grant_access = false;
 
 		if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY) {
 			continue;
@@ -486,34 +537,15 @@ NTSTATUS sec_access_check_ds(const struct security_descriptor *sd,
 			break;
 		case SEC_ACE_TYPE_ACCESS_DENIED_OBJECT:
 		case SEC_ACE_TYPE_ACCESS_ALLOWED_OBJECT:
-			/*
+			status = check_object_specific_access(ace, tree,
-			 * check only in case we have provided a tree,
+							      &grant_access);
-			 * the ACE has an object type and that type
-			 * is in the tree
-			 */
-			type = get_ace_object_type(ace);
 
-			if (!tree) {
+			if (!NT_STATUS_IS_OK(status)) {
-				continue;
+				return status;
 			}
 
-			if (!type) {
+			if (grant_access) {
-				node = tree;
+				return NT_STATUS_OK;
-			} else {
-				if (!(node = get_object_tree_by_GUID(tree, type))) {
-					continue;
-				}
-			}
-
-			if (ace->type == SEC_ACE_TYPE_ACCESS_ALLOWED_OBJECT) {
-				object_tree_modify_access(node, ace->access_mask);
-				if (node->remaining_access == 0) {
-					return NT_STATUS_OK;
-				}
-			} else {
-				if (node->remaining_access & ace->access_mask){
-					return NT_STATUS_ACCESS_DENIED;
-				}
 			}
 			break;
 		default:	/* Other ACE types not handled/supported */