Protect Non-Canonical ACL From SetEntriesInAcl()

- In cases where SetEntriesInAcl() is used, only allow processing if the ACL is canonical since SetEntriesInAcl() may not properly merge ACEs if the ACL is not canonical.

OperationAddAccountIfMissing.cpp

@@ -1,4 +1,5 @@
 #include "OperationAddAccountIfMissing.h"
+#include "OperationCheckCanonical.h"
 #include "InputOutput.h"
 #include "Functions.h"
 
@@ -58,6 +59,15 @@ bool OperationAddAccountIfMissing::ProcessAclAction(WCHAR * const sSdPart, Objec
 	// only attempt to add permissions if our flags do not match
 	if (iPermissionMask != FILE_ALL_ACCESS || iInheritMask != iDesiredInheritMask)
 	{
+		// since SetEntriesInAcl reacts poorly / unexpectedly in cases where the
+		// acl is not canonical, just error out and continue on
+		if (!OperationCheckCanonical::IsAclCanonical(tCurrentAcl))
+		{
+			InputOutput::AddError(L"Could not add '" + sAddSid + 
+				L"' to access control list since ACL was not canonical.", sSdPart);
+			return false;
+		}
+
 		EXPLICIT_ACCESS tEa;
 		tEa.grfAccessPermissions = FILE_ALL_ACCESS;
 		tEa.grfAccessMode = GRANT_ACCESS;

OperationCanonicalizeAcls.cpp

@@ -18,27 +18,8 @@ bool OperationCanonicalizeAcls::ProcessAclAction(WCHAR * const sSdPart, ObjectEn
 	// sanity check (null acl is considered valid)
 	if (tCurrentAcl == NULL) return false;
 
-	// use the simpler algorithm to determine if correct
-	bool bHasProblems = false;
-	OperationCheckCanonical::AceOrder oOrderOverall = OperationCheckCanonical::Unspecified;
-	ACCESS_ACE * tCheckAce = FirstAce(tCurrentAcl);
-	for (ULONG iEntry = 0; iEntry < tCurrentAcl->AceCount; tCheckAce = NextAce(tCheckAce), iEntry++)
-	{
-		// determine the overall over type of this ace
-		OperationCheckCanonical::AceOrder oThisAceOrder = OperationCheckCanonical::DetermineAceOrder(tCheckAce);
-
-		// make sure this order is not less then the current order
-		if (oThisAceOrder < oOrderOverall)
-		{
-			bHasProblems = true;
-			break;
-		}
-
-		oOrderOverall = oThisAceOrder;
-	}
-
-	// if no problem, then no need to perform a sort
-	if (!bHasProblems)
+	// if no problem, then no need to perform a reorder
+	if (OperationCheckCanonical::IsAclCanonical(tCurrentAcl))
 	{
 		return false;
 	}	

OperationCheckCanonical.cpp

@@ -17,24 +17,37 @@ bool OperationCheckCanonical::ProcessAclAction(WCHAR * const sSdPart, ObjectEntr
 	// sanity check (null acl is considered valid)
 	if (tCurrentAcl == NULL) return false;
 
+	// do the check and report
+	if (!IsAclCanonical(tCurrentAcl))
+	{
+		InputOutput::AddInfo(L"Access control list is not canonical", sSdPart);
+	}
+
+	// report the
+	return false;
+}
+
+bool OperationCheckCanonical::IsAclCanonical(PACL & tAcl)
+{
+	// sanity check (null acl is considered valid)
+	if (tAcl == NULL) return true;
+
 	AceOrder oOrderOverall = Unspecified;
-	ACCESS_ACE * tAce = FirstAce(tCurrentAcl);
-	for (ULONG iEntry = 0; iEntry < tCurrentAcl->AceCount; tAce = NextAce(tAce), iEntry++)
+	ACCESS_ACE * tAce = FirstAce(tAcl);
+	for (ULONG iEntry = 0; iEntry < tAcl->AceCount; tAce = NextAce(tAce), iEntry++)
 	{
 		// check inheritance bits
 		AceOrder oThisAceOrder = DetermineAceOrder(tAce);
 
 		// make sure this order is not less then the current order
 		if (oThisAceOrder < oOrderOverall)
 		{
-			InputOutput::AddInfo(L"Access control list is not canonical", sSdPart);
 			return false;
 		}
 		oOrderOverall = oThisAceOrder;
 	}
 
-	// report the
-	return false;
+	return true;
 }
 
 OperationCheckCanonical::AceOrder OperationCheckCanonical::DetermineAceOrder(ACCESS_ACE * tAce)

OperationCheckCanonical.h

@@ -25,6 +25,7 @@ class OperationCheckCanonical : public Operation
 
 	// public functions
 	static AceOrder DetermineAceOrder(ACCESS_ACE * tAce);
+	static bool IsAclCanonical(PACL & tCurrentAcl);
 
 	// overrides
 	bool ProcessAclAction(WCHAR * const sSdPart, ObjectEntry & tObjectEntry, PACL & tCurrentAcl, bool & bAclReplacement) override;

OperationCopyDomain.cpp

@@ -1,4 +1,5 @@
 #include "OperationCopyDomain.h"
+#include "OperationCheckCanonical.h"
 #include "InputOutput.h"
 #include "Functions.h"
 
@@ -44,6 +45,9 @@ OperationCopyDomain::OperationCopyDomain(std::queue<std::wstring> & oArgList) :
 
 bool OperationCopyDomain::ProcessAclAction(WCHAR * const sSdPart, ObjectEntry & tObjectEntry, PACL & tCurrentAcl, bool & bAclReplacement)
 {
+	// check on canonicalization status so if can error if the acl needs to be updated
+	const bool bAclIsCanonical = OperationCheckCanonical::IsAclCanonical(tCurrentAcl);
+
 	// check explicit effective rights from sid (no groups)
 	bool bAclIsDirty = false;
 	if (tCurrentAcl != NULL)
@@ -148,6 +152,16 @@ bool OperationCopyDomain::ProcessAclAction(WCHAR * const sSdPart, ObjectEntry &
 				continue;
 			}
 
+			// since SetEntriesInAcl reacts poorly / unexpectedly in cases where the
+			// acl is not canonical, just error out and continue on
+			if (!bAclIsCanonical)
+			{
+				std::wstring sTargetAccountName = GetNameFromSid(tTargetAccountSid);
+				InputOutput::AddError(L"Could not add '" + sTargetAccountName + L"' for domain '"
+					+ sTargetDomain + L"' to access control list since ACL was not canonical.", sSdPart);
+				continue;
+			}
+
 			// create a structure to add the missing permissions
 			EXPLICIT_ACCESS tEa;
 			tEa.grfAccessPermissions = tAceDacl->Mask;