From 62a84194aa00d0e2560a27c55be35a6ef052d2e4 Mon Sep 17 00:00:00 2001 From: jknudsen Date: Thu, 25 Jan 2024 16:09:17 +0100 Subject: [PATCH 1/6] feat: filter out ESC3 false positives --- packages/go/analysis/ad/adcs.go | 43 +++++++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/packages/go/analysis/ad/adcs.go b/packages/go/analysis/ad/adcs.go index 68115acbfe..a2a6d3a653 100644 --- a/packages/go/analysis/ad/adcs.go +++ b/packages/go/analysis/ad/adcs.go @@ -141,7 +141,14 @@ func PostADCSESC3(ctx context.Context, tx graph.Transaction, outC chan<- analysi cache.EnterpriseCAEnrollers[eca1.ID], cache.EnterpriseCAEnrollers[eca2.ID], delegatedAgents.Slice()) - results.Or(tempResults) + + + // Add principals to result set unless it's a user and DNS is required + if filteredResult, err := filterTempResultsForESC3(tx, tempResults, certTemplateOne); err != nil { + return err + } else { + results.Or(filteredResult) + } } } } else { @@ -151,7 +158,13 @@ func PostADCSESC3(ctx context.Context, tx graph.Transaction, outC chan<- analysi cache.CertTemplateControllers[certTemplateTwo.ID], cache.EnterpriseCAEnrollers[eca1.ID], cache.EnterpriseCAEnrollers[eca2.ID]) - results.Or(tempResults) + + // Add principals to result set unless it's a user and DNS is required + if filteredResult, err := filterTempResultsForESC3(tx, tempResults, certTemplateOne); err != nil { + return err + } else { + results.Or(filteredResult) + } } } } @@ -240,6 +253,32 @@ func checkDNSValidity(node *graph.Node, validCertTemplates []*graph.Node, groupE return false } +func filterTempResultsForESC3(tx graph.Transaction, tempResults cardinality.Duplex[uint32], certTemplate *graph.Node) (cardinality.Duplex[uint32], error) { + principalsEnabledForESC3 := cardinality.NewBitmap32() + + tempResults.Each(func(value uint32) (bool, error) { + sourceID := graph.ID(value) + + if resultNode, err := tx.Nodes().Filter(query.Equals(query.NodeID(), sourceID)).First(); err != nil { + return true, nil + } else { + if resultNode.Kinds.ContainsOneOf(ad.User) { + if subjectAltRequireDNS, err := certTemplate.Properties.Get(ad.SubjectAltRequireDNS.String()).Bool(); err != nil { + log.Errorf("%s property access error %d: %v", ad.SubjectAltRequireDNS.String(), certTemplate.ID, err) + } else if subjectAltRequireDomainDNS, err := certTemplate.Properties.Get(ad.SubjectAltRequireDomainDNS.String()).Bool(); err != nil { + log.Errorf("%s property access error %d: %v", ad.SubjectAltRequireDomainDNS.String(), certTemplate.ID, err) + } else if !subjectAltRequireDNS && !subjectAltRequireDomainDNS { + principalsEnabledForESC3.Add(value) + } + } else { + principalsEnabledForESC3.Add(value) + } + } + return true, nil + }) + return principalsEnabledForESC3, nil +} + func filterTempResultsForESC6a(tx graph.Transaction, tempResults cardinality.Duplex[uint32], groupExpansions impact.PathAggregator, validCertTemplates []*graph.Node, cache ADCSCache) cardinality.Duplex[uint32] { principalsEnabledForESC6a := cardinality.NewBitmap32() From e5de2e9410ad885050a7abe6fd81f06b9bb1abf5 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Mon, 29 Jan 2024 16:38:24 -0500 Subject: [PATCH 2/6] fix: handle esc3 filtering without retraversal --- packages/go/analysis/ad/adcs.go | 54 ++++++++++++++------------------- 1 file changed, 22 insertions(+), 32 deletions(-) diff --git a/packages/go/analysis/ad/adcs.go b/packages/go/analysis/ad/adcs.go index 911c97fed6..acb2f3787a 100644 --- a/packages/go/analysis/ad/adcs.go +++ b/packages/go/analysis/ad/adcs.go @@ -142,7 +142,6 @@ func PostADCSESC3(ctx context.Context, tx graph.Transaction, outC chan<- analysi cache.EnterpriseCAEnrollers[eca2.ID], delegatedAgents.Slice()) - // Add principals to result set unless it's a user and DNS is required if filteredResult, err := filterTempResultsForESC3(tx, tempResults, certTemplateOne); err != nil { return err @@ -159,12 +158,29 @@ func PostADCSESC3(ctx context.Context, tx graph.Transaction, outC chan<- analysi cache.EnterpriseCAEnrollers[eca1.ID], cache.EnterpriseCAEnrollers[eca2.ID]) - // Add principals to result set unless it's a user and DNS is required - if filteredResult, err := filterTempResultsForESC3(tx, tempResults, certTemplateOne); err != nil { - return err - } else { - results.Or(filteredResult) + if userNodes, err := ops.FetchNodeSet(tx.Nodes().Filterf(func() graph.Criteria { + return query.And( + query.KindIn(query.Node(), ad.User), + query.InIDs(query.NodeID(), cardinality.DuplexToGraphIDs(tempResults)...), + ) + })); err != nil { + if !graph.IsErrNotFound(err) { + return err + } + } else if len(userNodes) > 0 { + if subjRequireDns, err := certTemplateOne.Properties.Get(ad.SubjectAltRequireDNS.String()).Bool(); err != nil { + log.Debugf("Failed to retrieve subjectAltRequireDNS for template %d: %v", certTemplateOne.ID, err) + tempResults.Xor(cardinality.NodeSetToDuplex(userNodes)) + } else if subjRequireDomainDns, err := certTemplateOne.Properties.Get(ad.SubjectAltRequireDomainDNS.String()).Bool(); err != nil { + log.Debugf("Failed to retrieve subjectAltRequireDomainDNS for template %d: %v", certTemplateOne.ID, err) + tempResults.Xor(cardinality.NodeSetToDuplex(userNodes)) + } else if subjRequireDns || subjRequireDomainDns { + //If either of these properties is true, we need to remove all these users from our victims list + tempResults.Xor(cardinality.NodeSetToDuplex(userNodes)) + } } + + results.Or(tempResults) } } } @@ -253,32 +269,6 @@ func checkDNSValidity(node *graph.Node, validCertTemplates []*graph.Node, groupE return false } -func filterTempResultsForESC3(tx graph.Transaction, tempResults cardinality.Duplex[uint32], certTemplate *graph.Node) (cardinality.Duplex[uint32], error) { - principalsEnabledForESC3 := cardinality.NewBitmap32() - - tempResults.Each(func(value uint32) (bool, error) { - sourceID := graph.ID(value) - - if resultNode, err := tx.Nodes().Filter(query.Equals(query.NodeID(), sourceID)).First(); err != nil { - return true, nil - } else { - if resultNode.Kinds.ContainsOneOf(ad.User) { - if subjectAltRequireDNS, err := certTemplate.Properties.Get(ad.SubjectAltRequireDNS.String()).Bool(); err != nil { - log.Errorf("%s property access error %d: %v", ad.SubjectAltRequireDNS.String(), certTemplate.ID, err) - } else if subjectAltRequireDomainDNS, err := certTemplate.Properties.Get(ad.SubjectAltRequireDomainDNS.String()).Bool(); err != nil { - log.Errorf("%s property access error %d: %v", ad.SubjectAltRequireDomainDNS.String(), certTemplate.ID, err) - } else if !subjectAltRequireDNS && !subjectAltRequireDomainDNS { - principalsEnabledForESC3.Add(value) - } - } else { - principalsEnabledForESC3.Add(value) - } - } - return true, nil - }) - return principalsEnabledForESC3, nil -} - func filterTempResultsForESC6a(tx graph.Transaction, tempResults cardinality.Duplex[uint32], groupExpansions impact.PathAggregator, validCertTemplates []*graph.Node, cache ADCSCache) cardinality.Duplex[uint32] { principalsEnabledForESC6a := cardinality.NewBitmap32() From f3cf2619d4eca1567aa2bcde6fb072ee13613680 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Mon, 29 Jan 2024 16:39:01 -0500 Subject: [PATCH 3/6] fix: handle esc3 filtering without retraversal --- packages/go/analysis/ad/adcs.go | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/packages/go/analysis/ad/adcs.go b/packages/go/analysis/ad/adcs.go index acb2f3787a..138d6f6f5b 100644 --- a/packages/go/analysis/ad/adcs.go +++ b/packages/go/analysis/ad/adcs.go @@ -143,10 +143,26 @@ func PostADCSESC3(ctx context.Context, tx graph.Transaction, outC chan<- analysi delegatedAgents.Slice()) // Add principals to result set unless it's a user and DNS is required - if filteredResult, err := filterTempResultsForESC3(tx, tempResults, certTemplateOne); err != nil { - return err - } else { - results.Or(filteredResult) + if userNodes, err := ops.FetchNodeSet(tx.Nodes().Filterf(func() graph.Criteria { + return query.And( + query.KindIn(query.Node(), ad.User), + query.InIDs(query.NodeID(), cardinality.DuplexToGraphIDs(tempResults)...), + ) + })); err != nil { + if !graph.IsErrNotFound(err) { + return err + } + } else if len(userNodes) > 0 { + if subjRequireDns, err := certTemplateOne.Properties.Get(ad.SubjectAltRequireDNS.String()).Bool(); err != nil { + log.Debugf("Failed to retrieve subjectAltRequireDNS for template %d: %v", certTemplateOne.ID, err) + tempResults.Xor(cardinality.NodeSetToDuplex(userNodes)) + } else if subjRequireDomainDns, err := certTemplateOne.Properties.Get(ad.SubjectAltRequireDomainDNS.String()).Bool(); err != nil { + log.Debugf("Failed to retrieve subjectAltRequireDomainDNS for template %d: %v", certTemplateOne.ID, err) + tempResults.Xor(cardinality.NodeSetToDuplex(userNodes)) + } else if subjRequireDns || subjRequireDomainDns { + //If either of these properties is true, we need to remove all these users from our victims list + tempResults.Xor(cardinality.NodeSetToDuplex(userNodes)) + } } } } From 7bb24c71ae880586d783a65e35660e9f1f8d39d4 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Mon, 29 Jan 2024 16:42:16 -0500 Subject: [PATCH 4/6] fix: handle esc3 filtering without retraversal --- packages/go/analysis/ad/adcs.go | 68 ++++++++++++++++----------------- 1 file changed, 32 insertions(+), 36 deletions(-) diff --git a/packages/go/analysis/ad/adcs.go b/packages/go/analysis/ad/adcs.go index 138d6f6f5b..18bcb4c802 100644 --- a/packages/go/analysis/ad/adcs.go +++ b/packages/go/analysis/ad/adcs.go @@ -143,26 +143,12 @@ func PostADCSESC3(ctx context.Context, tx graph.Transaction, outC chan<- analysi delegatedAgents.Slice()) // Add principals to result set unless it's a user and DNS is required - if userNodes, err := ops.FetchNodeSet(tx.Nodes().Filterf(func() graph.Criteria { - return query.And( - query.KindIn(query.Node(), ad.User), - query.InIDs(query.NodeID(), cardinality.DuplexToGraphIDs(tempResults)...), - ) - })); err != nil { + if filteredResults, err := filterESC3UserResults(tx, tempResults, certTemplateOne); err != nil { if !graph.IsErrNotFound(err) { return err } - } else if len(userNodes) > 0 { - if subjRequireDns, err := certTemplateOne.Properties.Get(ad.SubjectAltRequireDNS.String()).Bool(); err != nil { - log.Debugf("Failed to retrieve subjectAltRequireDNS for template %d: %v", certTemplateOne.ID, err) - tempResults.Xor(cardinality.NodeSetToDuplex(userNodes)) - } else if subjRequireDomainDns, err := certTemplateOne.Properties.Get(ad.SubjectAltRequireDomainDNS.String()).Bool(); err != nil { - log.Debugf("Failed to retrieve subjectAltRequireDomainDNS for template %d: %v", certTemplateOne.ID, err) - tempResults.Xor(cardinality.NodeSetToDuplex(userNodes)) - } else if subjRequireDns || subjRequireDomainDns { - //If either of these properties is true, we need to remove all these users from our victims list - tempResults.Xor(cardinality.NodeSetToDuplex(userNodes)) - } + } else { + results.Or(filteredResults) } } } @@ -174,29 +160,13 @@ func PostADCSESC3(ctx context.Context, tx graph.Transaction, outC chan<- analysi cache.EnterpriseCAEnrollers[eca1.ID], cache.EnterpriseCAEnrollers[eca2.ID]) - if userNodes, err := ops.FetchNodeSet(tx.Nodes().Filterf(func() graph.Criteria { - return query.And( - query.KindIn(query.Node(), ad.User), - query.InIDs(query.NodeID(), cardinality.DuplexToGraphIDs(tempResults)...), - ) - })); err != nil { + if filteredResults, err := filterESC3UserResults(tx, tempResults, certTemplateOne); err != nil { if !graph.IsErrNotFound(err) { return err } - } else if len(userNodes) > 0 { - if subjRequireDns, err := certTemplateOne.Properties.Get(ad.SubjectAltRequireDNS.String()).Bool(); err != nil { - log.Debugf("Failed to retrieve subjectAltRequireDNS for template %d: %v", certTemplateOne.ID, err) - tempResults.Xor(cardinality.NodeSetToDuplex(userNodes)) - } else if subjRequireDomainDns, err := certTemplateOne.Properties.Get(ad.SubjectAltRequireDomainDNS.String()).Bool(); err != nil { - log.Debugf("Failed to retrieve subjectAltRequireDomainDNS for template %d: %v", certTemplateOne.ID, err) - tempResults.Xor(cardinality.NodeSetToDuplex(userNodes)) - } else if subjRequireDns || subjRequireDomainDns { - //If either of these properties is true, we need to remove all these users from our victims list - tempResults.Xor(cardinality.NodeSetToDuplex(userNodes)) - } + } else { + results.Or(filteredResults) } - - results.Or(tempResults) } } } @@ -219,6 +189,32 @@ func PostADCSESC3(ctx context.Context, tx graph.Transaction, outC chan<- analysi return nil } +func filterESC3UserResults(tx graph.Transaction, tempResults cardinality.Duplex[uint32], certTemplate *graph.Node) (cardinality.Duplex[uint32], error) { + if userNodes, err := ops.FetchNodeSet(tx.Nodes().Filterf(func() graph.Criteria { + return query.And( + query.KindIn(query.Node(), ad.User), + query.InIDs(query.NodeID(), cardinality.DuplexToGraphIDs(tempResults)...), + ) + })); err != nil { + if !graph.IsErrNotFound(err) { + return nil, err + } + } else if len(userNodes) > 0 { + if subjRequireDns, err := certTemplate.Properties.Get(ad.SubjectAltRequireDNS.String()).Bool(); err != nil { + log.Debugf("Failed to retrieve subjectAltRequireDNS for template %d: %v", certTemplate.ID, err) + tempResults.Xor(cardinality.NodeSetToDuplex(userNodes)) + } else if subjRequireDomainDns, err := certTemplate.Properties.Get(ad.SubjectAltRequireDomainDNS.String()).Bool(); err != nil { + log.Debugf("Failed to retrieve subjectAltRequireDomainDNS for template %d: %v", certTemplate.ID, err) + tempResults.Xor(cardinality.NodeSetToDuplex(userNodes)) + } else if subjRequireDns || subjRequireDomainDns { + //If either of these properties is true, we need to remove all these users from our victims list + tempResults.Xor(cardinality.NodeSetToDuplex(userNodes)) + } + } + + return tempResults, nil +} + func principalControlsCertTemplate(principal, certTemplate *graph.Node, groupExpansions impact.PathAggregator, cache ADCSCache) bool { var ( expandedTemplateControllers = cache.ExpandedCertTemplateControllers[certTemplate.ID] From d393806e411d6f9c7c89c5ee7309ecb47b91df42 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Mon, 29 Jan 2024 16:43:12 -0500 Subject: [PATCH 5/6] chore: rename function for re-use --- packages/go/analysis/ad/adcs.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/go/analysis/ad/adcs.go b/packages/go/analysis/ad/adcs.go index 18bcb4c802..cadff27206 100644 --- a/packages/go/analysis/ad/adcs.go +++ b/packages/go/analysis/ad/adcs.go @@ -143,7 +143,7 @@ func PostADCSESC3(ctx context.Context, tx graph.Transaction, outC chan<- analysi delegatedAgents.Slice()) // Add principals to result set unless it's a user and DNS is required - if filteredResults, err := filterESC3UserResults(tx, tempResults, certTemplateOne); err != nil { + if filteredResults, err := filterUserDNSResults(tx, tempResults, certTemplateOne); err != nil { if !graph.IsErrNotFound(err) { return err } @@ -160,7 +160,7 @@ func PostADCSESC3(ctx context.Context, tx graph.Transaction, outC chan<- analysi cache.EnterpriseCAEnrollers[eca1.ID], cache.EnterpriseCAEnrollers[eca2.ID]) - if filteredResults, err := filterESC3UserResults(tx, tempResults, certTemplateOne); err != nil { + if filteredResults, err := filterUserDNSResults(tx, tempResults, certTemplateOne); err != nil { if !graph.IsErrNotFound(err) { return err } @@ -189,7 +189,7 @@ func PostADCSESC3(ctx context.Context, tx graph.Transaction, outC chan<- analysi return nil } -func filterESC3UserResults(tx graph.Transaction, tempResults cardinality.Duplex[uint32], certTemplate *graph.Node) (cardinality.Duplex[uint32], error) { +func filterUserDNSResults(tx graph.Transaction, tempResults cardinality.Duplex[uint32], certTemplate *graph.Node) (cardinality.Duplex[uint32], error) { if userNodes, err := ops.FetchNodeSet(tx.Nodes().Filterf(func() graph.Criteria { return query.And( query.KindIn(query.Node(), ad.User), From 9285680b14d711095bd114b554c53c0fed3a8430 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Tue, 30 Jan 2024 10:23:59 -0500 Subject: [PATCH 6/6] chore: log and continue --- packages/go/analysis/ad/adcs.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/packages/go/analysis/ad/adcs.go b/packages/go/analysis/ad/adcs.go index cadff27206..83b1614cb1 100644 --- a/packages/go/analysis/ad/adcs.go +++ b/packages/go/analysis/ad/adcs.go @@ -144,9 +144,7 @@ func PostADCSESC3(ctx context.Context, tx graph.Transaction, outC chan<- analysi // Add principals to result set unless it's a user and DNS is required if filteredResults, err := filterUserDNSResults(tx, tempResults, certTemplateOne); err != nil { - if !graph.IsErrNotFound(err) { - return err - } + log.Errorf("Error filtering user dns results: %v", err) } else { results.Or(filteredResults) } @@ -161,9 +159,7 @@ func PostADCSESC3(ctx context.Context, tx graph.Transaction, outC chan<- analysi cache.EnterpriseCAEnrollers[eca2.ID]) if filteredResults, err := filterUserDNSResults(tx, tempResults, certTemplateOne); err != nil { - if !graph.IsErrNotFound(err) { - return err - } + log.Errorf("Error filtering user dns results: %v", err) } else { results.Or(filteredResults) }