From b6ed4781703a49d2d9d4188b34847eb5115bbdd2 Mon Sep 17 00:00:00 2001 From: Yevhen Zavhorodnii Date: Fri, 31 May 2024 17:14:40 +0100 Subject: [PATCH] Refactoring to reduce nesting and cover more uncovered use cases --- .../server_side_request_forgery_rule.go | 18 +-- .../server_side_request_forgery_rule_test.go | 135 ++++++++++++++++++ 2 files changed, 145 insertions(+), 8 deletions(-) diff --git a/pkg/security/risks/builtin/server_side_request_forgery_rule.go b/pkg/security/risks/builtin/server_side_request_forgery_rule.go index 108d0889..b5f17733 100644 --- a/pkg/security/risks/builtin/server_side_request_forgery_rule.go +++ b/pkg/security/risks/builtin/server_side_request_forgery_rule.go @@ -70,14 +70,16 @@ func (r *ServerSideRequestForgeryRule) createRisk(input *types.Model, technicalA uniqueDataBreachTechnicalAssetIDs := make(map[string]interface{}) uniqueDataBreachTechnicalAssetIDs[technicalAsset.Id] = true for _, potentialTargetAsset := range input.TechnicalAssets { - if technicalAsset.IsSameTrustBoundaryNetworkOnly(input, potentialTargetAsset.Id) { - for _, commLinkIncoming := range input.IncomingTechnicalCommunicationLinksMappedByTargetId[potentialTargetAsset.Id] { - if commLinkIncoming.Protocol.IsPotentialWebAccessProtocol() { - uniqueDataBreachTechnicalAssetIDs[potentialTargetAsset.Id] = true - if potentialTargetAsset.HighestProcessedConfidentiality(input) == types.StrictlyConfidential { - impact = types.MediumImpact - } - } + if !technicalAsset.IsSameTrustBoundaryNetworkOnly(input, potentialTargetAsset.Id) { + continue + } + for _, commLinkIncoming := range input.IncomingTechnicalCommunicationLinksMappedByTargetId[potentialTargetAsset.Id] { + if !commLinkIncoming.Protocol.IsPotentialWebAccessProtocol() { + continue + } + uniqueDataBreachTechnicalAssetIDs[potentialTargetAsset.Id] = true + if potentialTargetAsset.HighestProcessedConfidentiality(input) == types.StrictlyConfidential { + impact = types.MediumImpact } } } diff --git a/pkg/security/risks/builtin/server_side_request_forgery_rule_test.go b/pkg/security/risks/builtin/server_side_request_forgery_rule_test.go index 82bbb7fb..003186f5 100644 --- a/pkg/security/risks/builtin/server_side_request_forgery_rule_test.go +++ b/pkg/security/risks/builtin/server_side_request_forgery_rule_test.go @@ -312,6 +312,67 @@ func TestServerSideRequestForgeryRuleGenerateRisksStrictlyConfidentialTrustBound assert.Equal(t, "Server-Side Request Forgery (SSRF) risk at Test Technical Asset server-side web-requesting the target Test Target Asset via Test Communication Link", risks[0].Title) } +func TestServerSideRequestForgeryRuleGenerateRisksStrictlyConfidentialDifferentTrustBoundaryNotCloudLowImpactRisksCreated(t *testing.T) { + rule := NewServerSideRequestForgeryRule() + tb1 := &types.TrustBoundary{ + Id: "tb1", + Type: types.NetworkVirtualLAN, + TechnicalAssetsInside: []string{"ta1"}, + } + tb2 := &types.TrustBoundary{ + Id: "tb2", + Type: types.NetworkVirtualLAN, + TechnicalAssetsInside: []string{"ta2"}, + } + comm := &types.CommunicationLink{ + Protocol: types.HTTP, + TargetId: "ta2", + Title: "Test Communication Link", + } + risks, err := rule.GenerateRisks(&types.Model{ + TechnicalAssets: map[string]*types.TechnicalAsset{ + "ta1": { + Id: "ta1", + Title: "Test Technical Asset", + OutOfScope: false, + Technologies: types.TechnologyList{ + { + Name: "service-registry", + Attributes: map[string]bool{ + types.IsClient: false, + types.LoadBalancer: false, + }, + }, + }, + CommunicationLinks: []*types.CommunicationLink{comm}, + }, + "ta2": { + Id: "ta2", + Title: "Test Target Asset", + OutOfScope: false, + Confidentiality: types.Confidential, + }, + }, + IncomingTechnicalCommunicationLinksMappedByTargetId: map[string][]*types.CommunicationLink{ + "ta2": {comm}, + }, + TrustBoundaries: map[string]*types.TrustBoundary{ + "tb1": tb1, + "tb2": tb2, + }, + DirectContainingTrustBoundaryMappedByTechnicalAssetId: map[string]*types.TrustBoundary{ + "ta1": tb1, + "ta2": tb2, + }, + }) + + assert.Nil(t, err) + assert.Len(t, risks, 1) + assert.Equal(t, types.LowImpact, risks[0].ExploitationImpact) + assert.Equal(t, types.Likely, risks[0].ExploitationLikelihood) + assert.Equal(t, "Server-Side Request Forgery (SSRF) risk at Test Technical Asset server-side web-requesting the target Test Target Asset via Test Communication Link", risks[0].Title) +} + func TestServerSideRequestForgeryRuleGenerateRisksWithinDevopsUnlikelyLikelihoodRisksCreated(t *testing.T) { rule := NewServerSideRequestForgeryRule() risks, err := rule.GenerateRisks(&types.Model{ @@ -352,3 +413,77 @@ func TestServerSideRequestForgeryRuleGenerateRisksWithinDevopsUnlikelyLikelihood assert.Equal(t, types.Unlikely, risks[0].ExploitationLikelihood) assert.Equal(t, "Server-Side Request Forgery (SSRF) risk at Test Technical Asset server-side web-requesting the target Test Target Asset via Test Communication Link", risks[0].Title) } + +func TestServerSideRequestForgeryRuleGenerateRisksStrictlyConfidentialNotHttpAccessDifferentTrustBoundaryNotCloudLowImpactRisksCreated(t *testing.T) { + rule := NewServerSideRequestForgeryRule() + tb1 := &types.TrustBoundary{ + Id: "tb1", + Type: types.NetworkVirtualLAN, + TechnicalAssetsInside: []string{"ta1"}, + } + tb2 := &types.TrustBoundary{ + Id: "tb2", + Type: types.NetworkVirtualLAN, + TechnicalAssetsInside: []string{"ta2", "ta3"}, + } + comm := &types.CommunicationLink{ + Protocol: types.HTTP, + TargetId: "ta2", + Title: "Test Communication Link", + } + comm2 := &types.CommunicationLink{ + Protocol: types.SqlAccessProtocol, + TargetId: "ta1", + Title: "Test Communication Link 2", + } + risks, err := rule.GenerateRisks(&types.Model{ + TechnicalAssets: map[string]*types.TechnicalAsset{ + "ta1": { + Id: "ta1", + Title: "Test Technical Asset", + OutOfScope: false, + Technologies: types.TechnologyList{ + { + Name: "service-registry", + Attributes: map[string]bool{ + types.IsClient: false, + types.LoadBalancer: false, + }, + }, + }, + CommunicationLinks: []*types.CommunicationLink{comm, comm2}, + }, + "ta2": { + Id: "ta2", + Title: "Test Target Asset", + OutOfScope: false, + Confidentiality: types.Confidential, + }, + "ta3": { + Id: "ta3", + Title: "Test Source Asset", + OutOfScope: false, + Confidentiality: types.StrictlyConfidential, + }, + }, + IncomingTechnicalCommunicationLinksMappedByTargetId: map[string][]*types.CommunicationLink{ + "ta2": {comm}, + "ta1": {comm2}, + }, + TrustBoundaries: map[string]*types.TrustBoundary{ + "tb1": tb1, + "tb2": tb2, + }, + DirectContainingTrustBoundaryMappedByTechnicalAssetId: map[string]*types.TrustBoundary{ + "ta1": tb1, + "ta2": tb2, + "ta3": tb2, + }, + }) + + assert.Nil(t, err) + assert.Len(t, risks, 1) + assert.Equal(t, types.LowImpact, risks[0].ExploitationImpact) + assert.Equal(t, types.Likely, risks[0].ExploitationLikelihood) + assert.Equal(t, "Server-Side Request Forgery (SSRF) risk at Test Technical Asset server-side web-requesting the target Test Target Asset via Test Communication Link", risks[0].Title) +}