diff --git a/OperationAddAccountIfMissing.cpp b/OperationAddAccountIfMissing.cpp index 267daad..9672284 100644 --- a/OperationAddAccountIfMissing.cpp +++ b/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; diff --git a/OperationCanonicalizeAcls.cpp b/OperationCanonicalizeAcls.cpp index 4b08332..1c53bf6 100644 --- a/OperationCanonicalizeAcls.cpp +++ b/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; } diff --git a/OperationCheckCanonical.cpp b/OperationCheckCanonical.cpp index 0b66a4f..f03d6b7 100644 --- a/OperationCheckCanonical.cpp +++ b/OperationCheckCanonical.cpp @@ -17,9 +17,24 @@ 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); @@ -27,14 +42,12 @@ bool OperationCheckCanonical::ProcessAclAction(WCHAR * const sSdPart, ObjectEntr // 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) diff --git a/OperationCheckCanonical.h b/OperationCheckCanonical.h index dc4d830..218a166 100644 --- a/OperationCheckCanonical.h +++ b/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; diff --git a/OperationCopyDomain.cpp b/OperationCopyDomain.cpp index 5bc8929..61b47e1 100644 --- a/OperationCopyDomain.cpp +++ b/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 & 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;