Improved on FindOrAddAdjacentSID, and use that while parsing groups t…

…hat are pointing to missing DNs
lkarlslund · Nov 15, 2023 · 65c6fcf · 65c6fcf
1 parent f671596
commit 65c6fcf
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 25 deletions.
diff --git a/modules/engine/objects.go b/modules/engine/objects.go
@@ -360,10 +360,15 @@ func (os *Objects) Merge(attrtomerge []Attribute, source *Object) bool {
 						}
 
 						// Test that all mergeapprovers confirm this to be a valid merge
+						if source.SID().String() == "S-1-5-21-1912508229-386351500-4206070068-4522" {
+							ui.Trace().Msgf("Gotcha")
+						}
+
 						for _, mfi := range mergeapprovers {
 							res, err := mfi.mergefunc(source, target)
 							switch err {
 							case ErrDontMerge:
+								ui.Trace().Msgf("Merge approver %v rejected merging %v with %v on attribute %v", mfi.name, source.Label(), target.Label(), mergeattr.String())
 								return false // break
 							case ErrMergeOnThis, nil:
 								// Let the code below do the merge
@@ -671,39 +676,54 @@ func (os *Objects) FindOrAddSID(s windowssecurity.SID) *Object {
 	return o.First()
 }
 
-func (os *Objects) FindOrAddAdjacentSID(s windowssecurity.SID, r *Object) *Object {
-	switch s.Component(2) {
-	case 21: // Full "domain" SID
-		result, _ := os.FindMultiOrAdd(ObjectSid, AttributeValueSID(s), func() *Object {
+func (os *Objects) FindOrAddAdjacentSID(s windowssecurity.SID, r *Object, flexinit ...any) *Object {
+	sidobject, _ := os.FindOrAddAdjacentSIDFound(s, r, flexinit...)
+	return sidobject
+}
+
+func (os *Objects) FindOrAddAdjacentSIDFound(s windowssecurity.SID, r *Object, flexinit ...any) (*Object, bool) {
+	// If it's relative to a computer, then let's see if we can find it (there could be SID collisions across local machines)
+	if r.Type() == ObjectTypeMachine && r.HasAttr(DataSource) {
+		// See if we can find it relative to the computer
+		if o, found := os.FindTwoMulti(ObjectSid, AttributeValueSID(s), DataSource, r.OneAttr(DataSource)); found {
+			return o.First(), true
+		}
+	}
+
+	// Let's assume it's not relative to a computer, and therefore truly unique
+	if s.Component(2) == 21 && s.Component(3) != 0 {
+		result, found := os.FindMultiOrAdd(ObjectSid, AttributeValueSID(s), func() *Object {
 			no := NewObject(
 				ObjectSid, AttributeValueSID(s),
 			)
+			no.SetFlex(flexinit...)
 			return no
 		})
-		return result.First()
+		return result.First(), found
 	}
 
+	// This is relative to an object that is part of a domain, so lets use that as a lookup reference
 	if r.HasAttr(DomainContext) {
-		// From outside, we need to find the domain part
 		if o, found := os.FindTwoMulti(ObjectSid, AttributeValueSID(s), DomainContext, r.OneAttr(DomainContext)); found {
-			return o.First()
+			return o.First(), true
 		}
 	}
-	// From inside same source, that is easy
+
+	// Use the objects datasource as the relative reference
 	if r.HasAttr(DataSource) {
 		if o, found := os.FindTwoMulti(ObjectSid, AttributeValueSID(s), DataSource, r.OneAttr(DataSource)); found {
-			return o.First()
+			return o.First(), true
 		}
 	}
 
-	// Not found, we have write lock so create it
-	no, _ := os.FindOrAdd(ObjectSid, AttributeValueSID(s),
+	// Not found, so fall back to just looking up the SID
+	no, found := os.FindOrAdd(ObjectSid, AttributeValueSID(s),
 		IgnoreBlanks,
 		DomainContext, r.Attr(DomainContext),
 		DataSource, r.Attr(DataSource),
 	)
 
-	return no
+	return no, found
 }
 
 func findMostLocal(os []*Object) *Object {

diff --git a/modules/engine/objecttype.go b/modules/engine/objecttype.go
@@ -30,6 +30,7 @@ var (
 	ObjectTypeBuiltinDomain              = NewObjectType("BuiltinDomain", "Builtin-Domain")
 	ObjectTypeContainer                  = NewObjectType("Container", "Container").SetDefault(Last, false)
 	ObjectTypeComputer                   = NewObjectType("Computer", "Computer")
+	ObjectTypeMachine                    = NewObjectType("Machine", "Machine")
 	ObjectTypeGroupPolicyContainer       = NewObjectType("GroupPolicyContainer", "Group-Policy-Container")
 	ObjectTypeTrust                      = NewObjectType("Trust", "Trusted-Domain")
 	ObjectTypeAttributeSchema            = NewObjectType("AttributeSchema", "Attribute-Schema")

diff --git a/modules/integrations/activedirectory/analyze/analyze-ad.go b/modules/integrations/activedirectory/analyze/analyze-ad.go
@@ -1579,23 +1579,20 @@ func init() {
 			object.Attr(activedirectory.Member).Iterate(func(member engine.AttributeValue) bool {
 				memberobject, found := ao.Find(engine.DistinguishedName, member)
 				if !found {
-					var sid engine.AttributeValueSID
 					if stringsid, _, found := strings.Cut(member.String(), ",CN=ForeignSecurityPrincipals,"); found {
 						// We can figure out what the SID is
-						if c, err := windowssecurity.ParseStringSID(stringsid); err == nil {
-							sid = engine.AttributeValueSID(c)
-							member = nil
+						if sid, err := windowssecurity.ParseStringSID(stringsid[3:]); err == nil {
+							memberobject = ao.FindOrAddAdjacentSID(sid, object)
+						} else {
+							ui.Warn().Msgf("Could not extract SID from Foreign-Security-Principal %v: %v", object.DN(), err)
 						}
-					} else {
+					}
+					if memberobject == nil {
 						ui.Warn().Msgf("Possible hardening? %v is a member of %v, which is not found - adding synthetic group. Your analysis will be degraded, try dumping with Domain Admin rights.", object.DN(), member)
+						memberobject, _ = ao.FindOrAdd(engine.DistinguishedName, member,
+							engine.DataLoader, "Autogenerated",
+						)
 					}
-					memberobject = engine.NewObject(
-						engine.IgnoreBlanks,
-						engine.DistinguishedName, member,
-						engine.ObjectSid, sid,
-						engine.DataLoader, "Autogenerated",
-					)
-					ao.Add(memberobject)
 				}
 				memberobject.EdgeTo(object, activedirectory.EdgeMemberOfGroup)
 				return true
@@ -1740,7 +1737,7 @@ func init() {
 			ui.Warn().Msgf("%v groups could not be resolved, this could affect analysis results", warnlines)
 		}
 
-	}, "Resolve expanding group names to real names from GPOs",
+	}, "Resolve expanding environment variables in group names to real names from GPOs",
 		engine.AfterMerge,
 	)
 }