Skip to content

Commit 6c251ef

Browse files
authored
fix ACL false neg/pos (#189)
1 parent 26001a3 commit 6c251ef

1 file changed

Lines changed: 31 additions & 29 deletions

File tree

src/CommonLib/Processors/ACLProcessor.cs

Lines changed: 31 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -366,10 +366,10 @@ public async IAsyncEnumerable<ACE> ProcessACL(byte[] ntSecurityDescriptor, strin
366366
_log.LogTrace("Processing ACE with rights {Rights} and guid {GUID} on object {Name}", aceRights,
367367
aceType, objectName);
368368

369-
//GenericAll applies to every object
370-
if (aceRights.HasFlag(ActiveDirectoryRights.GenericAll))
371-
{
372-
if (aceType is ACEGuids.AllGuid or "")
369+
//GenericAll, WriteDacl, and WriteOwner apply to every object
370+
//All three require ObjectType (aceType) is "AllGuid" or not set (see: https://github.com/SpecterOps/BloodHound/issues/613)
371+
if (aceType is ACEGuids.AllGuid or "") {
372+
if (aceRights.HasFlag(ActiveDirectoryRights.GenericAll)) {
373373
aces.Add(new ACE
374374
{
375375
PrincipalType = resolvedPrincipal.ObjectType,
@@ -378,31 +378,31 @@ public async IAsyncEnumerable<ACE> ProcessACL(byte[] ntSecurityDescriptor, strin
378378
RightName = EdgeNames.GenericAll,
379379
InheritanceHash = aceInheritanceHash
380380
});
381-
//This is a special case. If we don't continue here, every other ACE will match because GenericAll includes all other permissions
382-
continue;
381+
//This is a special case. If we don't continue here, every other ACE will match because GenericAll includes all other permissions
382+
continue;
383+
}
384+
if (aceRights.HasFlag(ActiveDirectoryRights.WriteDacl)) {
385+
aces.Add(new ACE
386+
{
387+
PrincipalType = resolvedPrincipal.ObjectType,
388+
PrincipalSID = resolvedPrincipal.ObjectIdentifier,
389+
IsInherited = inherited,
390+
RightName = EdgeNames.WriteDacl,
391+
InheritanceHash = aceInheritanceHash
392+
});
393+
}
394+
if (aceRights.HasFlag(ActiveDirectoryRights.WriteOwner)) {
395+
aces.Add(new ACE
396+
{
397+
PrincipalType = resolvedPrincipal.ObjectType,
398+
PrincipalSID = resolvedPrincipal.ObjectIdentifier,
399+
IsInherited = inherited,
400+
RightName = EdgeNames.WriteOwner,
401+
InheritanceHash = aceInheritanceHash
402+
});
403+
}
383404
}
384405

385-
//WriteDACL and WriteOwner are always useful no matter what the object type is as well because they enable all other attacks
386-
if (aceRights.HasFlag(ActiveDirectoryRights.WriteDacl))
387-
aces.Add(new ACE
388-
{
389-
PrincipalType = resolvedPrincipal.ObjectType,
390-
PrincipalSID = resolvedPrincipal.ObjectIdentifier,
391-
IsInherited = inherited,
392-
RightName = EdgeNames.WriteDacl,
393-
InheritanceHash = aceInheritanceHash
394-
});
395-
396-
if (aceRights.HasFlag(ActiveDirectoryRights.WriteOwner))
397-
aces.Add(new ACE
398-
{
399-
PrincipalType = resolvedPrincipal.ObjectType,
400-
PrincipalSID = resolvedPrincipal.ObjectIdentifier,
401-
IsInherited = inherited,
402-
RightName = EdgeNames.WriteOwner,
403-
InheritanceHash = aceInheritanceHash
404-
});
405-
406406
//Cool ACE courtesy of @rookuu. Allows a principal to add itself to a group and no one else
407407
if (aceRights.HasFlag(ActiveDirectoryRights.Self) &&
408408
!aceRights.HasFlag(ActiveDirectoryRights.WriteProperty) &&
@@ -418,7 +418,8 @@ public async IAsyncEnumerable<ACE> ProcessACL(byte[] ntSecurityDescriptor, strin
418418
});
419419

420420
//Process object type specific ACEs. Extended rights apply to users, domains, computers, and cert templates
421-
if (aceRights.HasFlag(ActiveDirectoryRights.ExtendedRight))
421+
if (aceRights.HasFlag(ActiveDirectoryRights.ExtendedRight) ||
422+
aceRights.HasFlag(ActiveDirectoryRights.GenericAll)) //GenericAll also works (see: https://github.com/SpecterOps/BloodHound/issues/613#issuecomment-2728437374)
422423
{
423424
if (objectType == Label.Domain)
424425
{
@@ -538,7 +539,8 @@ public async IAsyncEnumerable<ACE> ProcessACL(byte[] ntSecurityDescriptor, strin
538539

539540
//GenericWrite encapsulates WriteProperty, so process them in tandem to avoid duplicate edges
540541
if (aceRights.HasFlag(ActiveDirectoryRights.GenericWrite) ||
541-
aceRights.HasFlag(ActiveDirectoryRights.WriteProperty))
542+
aceRights.HasFlag(ActiveDirectoryRights.WriteProperty) ||
543+
aceRights.HasFlag(ActiveDirectoryRights.GenericAll)) //GenericAll also works (see: https://github.com/SpecterOps/BloodHound/issues/613#issuecomment-2728437374)
542544
{
543545
if (objectType is Label.User
544546
or Label.Group

0 commit comments

Comments
 (0)