Skip to content

Commit

Permalink
Adhere to conditional statement standard, refactor doesAny[Inherited]…
Browse files Browse the repository at this point in the history 
…AceGrantOwnerRights, rename confusing struct
  • Loading branch information
@Mayyhem @zinic
Mayyhem authored and zinic committed Jan 21, 2025
1 parent daad1f8 commit a4d38bb
Show file tree
Hide file tree
Showing 3 changed files with 133 additions and 147 deletions.
252 changes: 124 additions & 128 deletions packages/go/analysis/ad/owns.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,68 +37,77 @@ func PostOwnsAndWriteOwner(ctx context.Context, db graph.Database, groupExpansio
operation := analysis.NewPostRelationshipOperation(ctx, db, "PostOwnsAndWriteOwner")

// Get the dSHeuristics values for all domains
dsHeuristicsCache, anyEnforced, err := GetDsHeuristicsCache(ctx, db)
if err != nil {
if dsHeuristicsCache, anyEnforced, err := GetDsHeuristicsCache(ctx, db); err != nil {
log.Errorf("failed fetching dsheuristics values for postownsandwriteowner: %w", err)
return nil, err
}

adminGroupIds, err := FetchAdminGroupIds(ctx, db, groupExpansions)
if err != nil {
} else if adminGroupIds, err := FetchAdminGroupIds(ctx, db, groupExpansions); err != nil {
// Get the admin group IDs
log.Errorf("failed fetching admin group ids values for postownsandwriteowner: %w", err)
}
} else {

// Get all source nodes of Owns ACEs (i.e., owning principals) where the target node has no ACEs granting abusable explicit permissions to OWNER RIGHTS
if err := operation.Operation.SubmitReader(func(ctx context.Context, tx graph.Transaction, outC chan<- analysis.CreatePostRelationshipJob) error {
relationships, err := ops.FetchRelationships(tx.Relationships().Filterf(func() graph.Criteria {
return query.And(
query.Kind(query.Relationship(), ad.OwnsRaw),
query.Kind(query.Start(), ad.Entity),
)
}))
if err != nil {
log.Errorf("failed to fetch OwnsRaw relationships: %w", err)
}
// Get all source nodes of Owns ACEs (i.e., owning principals) where the target node has no ACEs granting abusable explicit permissions to OWNER RIGHTS
if err := operation.Operation.SubmitReader(func(ctx context.Context, tx graph.Transaction, outC chan<- analysis.CreatePostRelationshipJob) error {
if relationships, err := ops.FetchRelationships(tx.Relationships().Filterf(func() graph.Criteria {
return query.And(
query.Kind(query.Relationship(), ad.OwnsRaw),
query.Kind(query.Start(), ad.Entity),
)
})); err != nil {
log.Errorf("failed to fetch OwnsRaw relationships for postownsandwriteowner: %w", err)
} else {
for _, rel := range relationships {

for _, rel := range relationships {
// Check if ANY domain enforces BlockOwnerImplicitRights (dSHeuristics[28] == 1)
if anyEnforced {

// Check if ANY domain enforces BlockOwnerImplicitRights (dSHeuristics[28] == 1)
if anyEnforced {
// Get the target node of the OwnsRaw relationship
if targetNode, err := ops.FetchNode(tx, rel.EndID); err != nil {
log.Errorf("failed fetching OwnsRaw target node for postownsandwriteowner: %w", err)
continue

// Get the target node of the OwnsRaw relationship
if targetNode, err := ops.FetchNode(tx, rel.EndID); err != nil {
log.Errorf("failed fetching OwnsRaw target node postownsandwriteowner: %w", err)
continue
} else if domainSid, err := targetNode.Properties.GetOrDefault(ad.DomainSID.String(), "").String(); err != nil {
// Get the domain SID of the target node
log.Errorf("failed fetching domain SID for postownsandwriteowner: %w", err)
continue

} else if domainSid, err := targetNode.Properties.GetOrDefault(ad.DomainSID.String(), "").String(); err != nil {
// Get the domain SID of the target node
continue
} else {
enforced, ok := dsHeuristicsCache[domainSid]
if !ok {
enforced = false
}
} else {
enforced, ok := dsHeuristicsCache[domainSid]
if !ok {
enforced = false
}
// If THIS domain does NOT enforce BlockOwnerImplicitRights, add the Owns edge
if !enforced {
isInherited, err := rel.Properties.GetOrDefault(common.IsInherited.String(), false).Bool()
if err != nil {
isInherited = false
}

// If THIS domain does NOT enforce BlockOwnerImplicitRights, add the Owns edge
if !enforced {
isInherited, err := rel.Properties.GetOrDefault(common.IsInherited.String(), false).Bool()
if err != nil {
isInherited = false
channels.Submit(ctx, outC, analysis.CreatePostRelationshipJob{
FromID: rel.StartID,
ToID: rel.EndID,
Kind: ad.Owns,
RelProperties: map[string]any{ad.IsACL.String(): true, common.IsInherited.String(): isInherited},
})
} else if isComputerDerived, err := isTargetNodeComputerDerived(targetNode); err != nil {
// If no abusable permissions are granted to OWNER RIGHTS, check if the target node is a computer or derived object (MSA or GMSA)
continue
} else if (isComputerDerived && adminGroupIds != nil && adminGroupIds.Contains(rel.StartID.Uint64())) || !isComputerDerived {
// If the target node is a computer or derived object, add the Owns edge if the owning principal is a member of DA/EA (or is either group's SID)
// If the target node is NOT a computer or derived object, add the Owns edge
isInherited, err := rel.Properties.GetOrDefault(common.IsInherited.String(), false).Bool()
if err != nil {
isInherited = false
}
channels.Submit(ctx, outC, analysis.CreatePostRelationshipJob{
FromID: rel.StartID,
ToID: rel.EndID,
Kind: ad.Owns,
RelProperties: map[string]any{ad.IsACL.String(): true, common.IsInherited.String(): isInherited},
})
}
}

channels.Submit(ctx, outC, analysis.CreatePostRelationshipJob{
FromID: rel.StartID,
ToID: rel.EndID,
Kind: ad.Owns,
RelProperties: map[string]any{ad.IsACL.String(): true, common.IsInherited.String(): isInherited},
})
} else if isComputerDerived, err := isTargetNodeComputerDerived(targetNode); err != nil {
// If no abusable permissions are granted to OWNER RIGHTS, check if the target node is a computer or derived object (MSA or GMSA)
continue
} else if (isComputerDerived && adminGroupIds != nil && adminGroupIds.Contains(rel.StartID.Uint64())) || !isComputerDerived {
// If the target node is a computer or derived object, add the Owns edge if the owning principal is a member of DA/EA (or is either group's SID)
// If the target node is NOT a computer or derived object, add the Owns edge
} else {
// If no domain enforces BlockOwnerImplicitRights (dSHeuristics[28] == 1) or we can't fetch the attribute, we can skip this analysis and just add the Owns relationship
isInherited, err := rel.Properties.GetOrDefault(common.IsInherited.String(), false).Bool()
if err != nil {
isInherited = false
Expand All @@ -111,59 +120,76 @@ func PostOwnsAndWriteOwner(ctx context.Context, db graph.Database, groupExpansio
})
}
}
} else {
// If no domain enforces BlockOwnerImplicitRights (dSHeuristics[28] == 1) or we can't fetch the attribute, we can skip this analysis and just add the Owns relationship
isInherited, err := rel.Properties.GetOrDefault(common.IsInherited.String(), false).Bool()
if err != nil {
isInherited = false
}
channels.Submit(ctx, outC, analysis.CreatePostRelationshipJob{
FromID: rel.StartID,
ToID: rel.EndID,
Kind: ad.Owns,
RelProperties: map[string]any{ad.IsACL.String(): true, common.IsInherited.String(): isInherited},
})
}
return nil
}); err != nil {
log.Errorf("failed to process Owns relationships for postownsandwriteowner: %w", err)
}
return nil
}); err != nil {
log.Errorf("failed to process Owns relationships: %w", err)
}

// Get all source nodes of WriteOwner ACEs where the target node has no ACEs granting explicit abusable permissions to OWNER RIGHTS
if err := operation.Operation.SubmitReader(func(ctx context.Context, tx graph.Transaction, outC chan<- analysis.CreatePostRelationshipJob) error {
// Get all source nodes of WriteOwner ACEs where the target node has no ACEs granting explicit abusable permissions to OWNER RIGHTS
if err := operation.Operation.SubmitReader(func(ctx context.Context, tx graph.Transaction, outC chan<- analysis.CreatePostRelationshipJob) error {

relationships, err := ops.FetchRelationships(tx.Relationships().Filterf(func() graph.Criteria {
return query.And(
query.Kind(query.Relationship(), ad.WriteOwnerRaw),
query.Kind(query.Start(), ad.Entity),
)
}))
if err != nil {
log.Errorf("failed to fetch WriteOwnerRaw relationships: %w", err)
}
if relationships, err := ops.FetchRelationships(tx.Relationships().Filterf(func() graph.Criteria {
return query.And(
query.Kind(query.Relationship(), ad.WriteOwnerRaw),
query.Kind(query.Start(), ad.Entity),
)
})); err != nil {
log.Errorf("failed to fetch WriteOwnerRaw relationships for postownsandwriteowner: %w", err)
} else {
for _, rel := range relationships {

for _, rel := range relationships {
// Check if ANY domain enforces BlockOwnerImplicitRights (dSHeuristics[28] == 1)
if anyEnforced {

// Check if ANY domain enforces BlockOwnerImplicitRights (dSHeuristics[28] == 1)
if anyEnforced {
// Get the target node of the WriteOwnerRaw relationship
if targetNode, err := ops.FetchNode(tx, rel.EndID); err != nil {
log.Errorf("failed fetching WriteOwnerRaw target node for postownsandwriteowner: %w", err)
continue

// Get the target node of the WriteOwnerRaw relationship
if targetNode, err := ops.FetchNode(tx, rel.EndID); err != nil {
log.Errorf("failed fetching WriteOwnerRaw target node postownsandwriteowner: %w", err)
continue
} else if domainSid, err := targetNode.Properties.GetOrDefault(ad.DomainSID.String(), "").String(); err != nil {
// Get the domain SID of the target node
log.Errorf("failed fetching domain SID for postownsandwriteowner: %w", err)
continue

} else if domainSid, err := targetNode.Properties.GetOrDefault(ad.DomainSID.String(), "").String(); err != nil {
// Get the domain SID of the target node
continue
} else {
enforced, ok := dsHeuristicsCache[domainSid]
if !ok {
enforced = false
}
} else {
enforced, ok := dsHeuristicsCache[domainSid]
if !ok {
enforced = false
}

// If THIS domain does NOT enforce BlockOwnerImplicitRights, add the WriteOwner edge
if !enforced {
// If THIS domain does NOT enforce BlockOwnerImplicitRights, add the WriteOwner edge
if !enforced {
isInherited, err := rel.Properties.GetOrDefault(common.IsInherited.String(), false).Bool()
if err != nil {
isInherited = false
}
channels.Submit(ctx, outC, analysis.CreatePostRelationshipJob{
FromID: rel.StartID,
ToID: rel.EndID,
Kind: ad.WriteOwner,
RelProperties: map[string]any{ad.IsACL.String(): true, common.IsInherited.String(): isInherited},
})

} else if isComputerDerived, err := isTargetNodeComputerDerived(targetNode); err == nil {
// If no abusable permissions are granted to OWNER RIGHTS, check if the target node is a computer or derived object (MSA or GMSA)
if !isComputerDerived {
isInherited, err := rel.Properties.GetOrDefault(common.IsInherited.String(), false).Bool()
if err != nil {
isInherited = false
}
// If the target node is NOT a computer or derived object, add the WriteOwner edge
channels.Submit(ctx, outC, analysis.CreatePostRelationshipJob{
FromID: rel.StartID,
ToID: rel.EndID,
Kind: ad.WriteOwner,
RelProperties: map[string]any{ad.IsACL.String(): true, common.IsInherited.String(): isInherited},
})
}
}
}
} else {
// If no domain enforces BlockOwnerImplicitRights (dSHeuristics[28] == 1) or we can't fetch the attribute, we can skip this analysis and just add the WriteOwner relationship
isInherited, err := rel.Properties.GetOrDefault(common.IsInherited.String(), false).Bool()
if err != nil {
isInherited = false
Expand All @@ -174,44 +200,14 @@ func PostOwnsAndWriteOwner(ctx context.Context, db graph.Database, groupExpansio
Kind: ad.WriteOwner,
RelProperties: map[string]any{ad.IsACL.String(): true, common.IsInherited.String(): isInherited},
})

} else if isComputerDerived, err := isTargetNodeComputerDerived(targetNode); err == nil {
// If no abusable permissions are granted to OWNER RIGHTS, check if the target node is a computer or derived object (MSA or GMSA)
if !isComputerDerived {
isInherited, err := rel.Properties.GetOrDefault(common.IsInherited.String(), false).Bool()
if err != nil {
isInherited = false
}
// If the target node is NOT a computer or derived object, add the WriteOwner edge
channels.Submit(ctx, outC, analysis.CreatePostRelationshipJob{
FromID: rel.StartID,
ToID: rel.EndID,
Kind: ad.WriteOwner,
RelProperties: map[string]any{ad.IsACL.String(): true, common.IsInherited.String(): isInherited},
})
}
}
}
} else {
// If no domain enforces BlockOwnerImplicitRights (dSHeuristics[28] == 1) or we can't fetch the attribute, we can skip this analysis and just add the WriteOwner relationship
isInherited, err := rel.Properties.GetOrDefault(common.IsInherited.String(), false).Bool()
if err != nil {
isInherited = false
}
channels.Submit(ctx, outC, analysis.CreatePostRelationshipJob{
FromID: rel.StartID,
ToID: rel.EndID,
Kind: ad.WriteOwner,
RelProperties: map[string]any{ad.IsACL.String(): true, common.IsInherited.String(): isInherited},
})
}

return nil
}); err != nil {
log.Errorf("failed to process WriteOwner relationships for postownsandwriteowner: %w", err)
}
return nil
}); err != nil {
log.Errorf("failed to process WriteOwner relationships: %w", err)
}

return &operation.Stats, operation.Done()
}

Expand Down
24 changes: 7 additions & 17 deletions packages/go/ein/ad.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,7 @@ func ParseGroupMembershipData(group Group) ParsedGroupMembershipData {
return result
}

// FIXME: Consider removing `Cache` from the name as this ended up confusing reviwers
type WriteOwnerLimitedCache struct {
type WriteOwnerLimitedPrincipal struct {
SourceData IngestibleSource
IsInherited bool
}
Expand Down Expand Up @@ -233,7 +232,7 @@ func ParseACEData(targetNode IngestibleNode, aces []ACE, targetID string, target
ownerPrincipalInfo IngestibleSource
ownerLimitedPrivs = make([]string, 0)
writeOwnerLimitedPrivs = make([]string, 0)
potentialWriteOwnerLimitedPrincipals = make([]WriteOwnerLimitedCache, 0)
potentialWriteOwnerLimitedPrincipals = make([]WriteOwnerLimitedPrincipal, 0)
converted = make([]IngestibleRelationship, 0)
)

Expand Down Expand Up @@ -333,19 +332,12 @@ func ParseACEData(targetNode IngestibleNode, aces []ACE, targetID string, target
// We can tell if any non-abusable ACE is present and inherited by checking the DoesAnyInheritedAceGrantOwnerRights property
// If there are no inherited abusable permissions but there are inherited non-abusable permissions,
// the non-abusable permissions will NOT be deleted on ownership change, so WriteOwner will NOT be abusable

// FIXME: This should be rewritten as-per the pattern presented later in this function - see the following fixme
doesAnyInheritedAceGrantOwnerRights, exists := targetNode.PropertyMap[ad.DoesAnyInheritedAceGrantOwnerRights.String()]
if exists {
doesAnyInheritedAceGrantOwnerRights, ok := doesAnyInheritedAceGrantOwnerRights.(bool)
if ok {
if doesAnyInheritedAceGrantOwnerRights {
return converted
}
// If the non-abusable rights were NOT inherited, they are deleted on ownership change and WriteOwner may be abusable
// Post will determine if WriteOwner is abusable based on dSHeuristics:BlockOwnerImplicitRights enforcement and object type
}
if doesAnyInheritedAceGrantOwnerRights, hasValidProperty := getFromPropertyMap[bool](targetNode.PropertyMap, ad.DoesAnyInheritedAceGrantOwnerRights.String()); hasValidProperty && doesAnyInheritedAceGrantOwnerRights {
return converted
}
// If the non-abusable rights were NOT inherited, they are deleted on ownership change and WriteOwner may be abusable
// Post will determine if WriteOwner is abusable based on dSHeuristics:BlockOwnerImplicitRights enforcement and object type

// If there are abusable permissions granted to the OWNER RIGHTS SID, but they are not inherited,
// they will be deleted on ownership change and WriteOwner may be abusable, so create a WriteOwnerRaw
// edge for post-processing so we can check BlockOwnerImplicitRights enforcement and object type
Expand All @@ -367,8 +359,6 @@ func ParseACEData(targetNode IngestibleNode, aces []ACE, targetID string, target
} else {
// If no abusable permissions in the ACL are granted to the OWNER RIGHTS SID check whether any permissions were
// granted to the OWNER RIGHTS SID that are not abusable

// FIXME: Example of the requested above refactor
if doesAnyAceGrantOwnerRights, hasValidProperty := getFromPropertyMap[bool](targetNode.PropertyMap, ad.DoesAnyAceGrantOwnerRights.String()); hasValidProperty && doesAnyAceGrantOwnerRights {
// If the non-abusable rights were inherited, they are not deleted on ownership change and WriteOwner is not abusable
// Do NOT create the OwnsRaw or WriteOwnerRaw edges if the OWNER RIGHTS SID has inherited, non-abusable permissions
Expand Down
4 changes: 2 additions & 2 deletions packages/go/ein/incoming_models.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ func (s ACE) Kind() graph.Kind {
return parseADKind(s.PrincipalType)
}

func (s ACE) GetCachedValue() WriteOwnerLimitedCache {
return WriteOwnerLimitedCache{
func (s ACE) GetCachedValue() WriteOwnerLimitedPrincipal {
return WriteOwnerLimitedPrincipal{
SourceData: IngestibleSource{
Source: s.PrincipalSID,
SourceType: s.Kind(),
Expand Down

0 comments on commit a4d38bb

Please sign in to comment.