From 01932af9dba429fb59064500c43173250f014172 Mon Sep 17 00:00:00 2001 From: aceg1k <86647736+aceg1k@users.noreply.github.com> Date: Tue, 29 Jun 2021 08:43:32 +0200 Subject: [PATCH 1/2] Data assets stored, sent or received are processed Data assets of a technical asset that are stored by that technical asset or are sent to or received by that technical asset via a communication link are also always implicitly processed by that technical asset. - Always add stored, sent and received data assets to processed assets. - Remove code no longer necessay due to the above change. - Ensure uniqueness of `DataAssetsProcessed`, `DataAssetsStored`, `DataAssetsSent` and `DataAssetsReceived`. - Do not require presence of `data_assets_processed`, `data_assets_stored`, `data_assets_sent` or `data_assets_received`. - Fix wording in rules because stored data assets are already processed. --- main.go | 50 +++++++----- model/types.go | 80 ++----------------- raa/raa/raa.go | 2 + .../accidental-secret-leak-rule.go | 2 +- .../container-platform-escape-rule.go | 2 +- .../cross-site-scripting-rule.go | 2 +- .../ldap-injection/ldap-injection-rule.go | 4 +- ...ssing-authentication-second-factor-rule.go | 4 +- .../missing-authentication-rule.go | 4 +- .../missing-cloud-hardening-rule.go | 2 +- .../missing-file-validation-rule.go | 2 +- .../missing-hardening-rule.go | 2 +- .../missing-identity-store-rule.go | 2 +- .../missing-vault/missing-vault-rule.go | 2 +- .../built-in/missing-waf/missing-waf-rule.go | 2 +- .../path-traversal/path-traversal-rule.go | 4 +- .../search-query-injection-rule.go | 2 +- .../service-registry-poisoning-rule.go | 2 +- .../sql-nosql-injection-rule.go | 4 +- .../unencrypted-asset-rule.go | 1 + .../unnecessary-data-asset-rule.go | 4 +- .../unnecessary-data-transfer-rule.go | 8 +- .../unnecessary-technical-asset-rule.go | 6 +- .../untrusted-deserialization-rule.go | 2 +- .../xml-external-entity-rule.go | 2 +- support/schema.json | 8 +- 26 files changed, 75 insertions(+), 130 deletions(-) diff --git a/main.go b/main.go index a193d2e1..e874b27e 100644 --- a/main.go +++ b/main.go @@ -4088,23 +4088,25 @@ func parseModel(inputFilename string) { panic(errors.New("unknown 'usage' value of technical asset '" + title + "': " + fmt.Sprintf("%v", asset.Usage))) } - var dataAssetsProcessed = make([]string, 0) - if asset.Data_assets_processed != nil { - dataAssetsProcessed = make([]string, len(asset.Data_assets_processed)) - for i, parsedProcessedAsset := range asset.Data_assets_processed { - referencedAsset := fmt.Sprintf("%v", parsedProcessedAsset) - checkDataAssetTargetExists(referencedAsset, "technical asset '"+title+"'") - dataAssetsProcessed[i] = referencedAsset - } - } - var dataAssetsStored = make([]string, 0) if asset.Data_assets_stored != nil { - dataAssetsStored = make([]string, len(asset.Data_assets_stored)) - for i, parsedStoredAssets := range asset.Data_assets_stored { + for _, parsedStoredAssets := range asset.Data_assets_stored { referencedAsset := fmt.Sprintf("%v", parsedStoredAssets) - checkDataAssetTargetExists(referencedAsset, "technical asset '"+title+"'") - dataAssetsStored[i] = referencedAsset + if !model.Contains(dataAssetsStored, referencedAsset) { + checkDataAssetTargetExists(referencedAsset, "technical asset '"+title+"'") + dataAssetsStored = append(dataAssetsStored, referencedAsset) + } + } + } + + var dataAssetsProcessed = dataAssetsStored + if asset.Data_assets_processed != nil { + for _, parsedProcessedAsset := range asset.Data_assets_processed { + referencedAsset := fmt.Sprintf("%v", parsedProcessedAsset) + if !model.Contains(dataAssetsProcessed, referencedAsset) { + checkDataAssetTargetExists(referencedAsset, "technical asset '"+title+"'") + dataAssetsProcessed = append(dataAssetsProcessed, referencedAsset) + } } } @@ -4501,16 +4503,26 @@ func parseModel(inputFilename string) { if commLink.Data_assets_sent != nil { for _, dataAssetSent := range commLink.Data_assets_sent { referencedAsset := fmt.Sprintf("%v", dataAssetSent) - checkDataAssetTargetExists(referencedAsset, "communication link '"+commLinkTitle+"' of technical asset '"+title+"'") - dataAssetsSent = append(dataAssetsSent, referencedAsset) + if !model.Contains(dataAssetsSent, referencedAsset) { + checkDataAssetTargetExists(referencedAsset, "communication link '"+commLinkTitle+"' of technical asset '"+title+"'") + dataAssetsSent = append(dataAssetsSent, referencedAsset) + if !model.Contains(dataAssetsProcessed, referencedAsset) { + dataAssetsProcessed = append(dataAssetsProcessed, referencedAsset) + } + } } } if commLink.Data_assets_received != nil { for _, dataAssetReceived := range commLink.Data_assets_received { referencedAsset := fmt.Sprintf("%v", dataAssetReceived) - checkDataAssetTargetExists(referencedAsset, "communication link '"+commLinkTitle+"' of technical asset '"+title+"'") - dataAssetsReceived = append(dataAssetsReceived, referencedAsset) + if !model.Contains(dataAssetsSent, referencedAsset) { + checkDataAssetTargetExists(referencedAsset, "communication link '"+commLinkTitle+"' of technical asset '"+title+"'") + dataAssetsReceived = append(dataAssetsReceived, referencedAsset) + if !model.Contains(dataAssetsProcessed, referencedAsset) { + dataAssetsProcessed = append(dataAssetsProcessed, referencedAsset) + } + } } } @@ -5137,7 +5149,7 @@ func writeDataAssetDiagramGraphvizDOT(diagramFilenameDOT string, dpi int) *os.Fi } sort.Sort(model.ByOrderAndIdSort(techAssets)) for _, technicalAsset := range techAssets { - if len(technicalAsset.DataAssetsStored) > 0 || len(technicalAsset.DataAssetsProcessed) > 0 { + if len(technicalAsset.DataAssetsProcessed) > 0 { dotContent.WriteString(makeTechAssetNode(technicalAsset, true)) dotContent.WriteString("\n") } diff --git a/model/types.go b/model/types.go index ca249481..0636bf14 100644 --- a/model/types.go +++ b/model/types.go @@ -1146,9 +1146,6 @@ func (what DataAsset) IsDataBreachPotentialStillAtRisk() bool { if Contains(ParsedModelRoot.TechnicalAssets[techAsset].DataAssetsProcessed, what.Id) { return true } - if Contains(ParsedModelRoot.TechnicalAssets[techAsset].DataAssetsStored, what.Id) { - return true - } } } return false @@ -1164,12 +1161,6 @@ func (what DataAsset) IdentifiedDataBreachProbability() DataBreachProbability { break } } - if Contains(ParsedModelRoot.TechnicalAssets[techAsset].DataAssetsStored, what.Id) { - if risk.DataBreachProbability > highestProbability { - highestProbability = risk.DataBreachProbability - break - } - } } } return highestProbability @@ -1185,12 +1176,6 @@ func (what DataAsset) IdentifiedDataBreachProbabilityStillAtRisk() DataBreachPro break } } - if Contains(ParsedModelRoot.TechnicalAssets[techAsset].DataAssetsStored, what.Id) { - if risk.DataBreachProbability > highestProbability { - highestProbability = risk.DataBreachProbability - break - } - } } } return highestProbability @@ -1204,10 +1189,6 @@ func (what DataAsset) IdentifiedDataBreachProbabilityRisksStillAtRisk() []Risk { result = append(result, risk) break } - if Contains(ParsedModelRoot.TechnicalAssets[techAsset].DataAssetsStored, what.Id) { - result = append(result, risk) - break - } } } return result @@ -1221,10 +1202,6 @@ func (what DataAsset) IdentifiedDataBreachProbabilityRisks() []Risk { result = append(result, risk) break } - if Contains(ParsedModelRoot.TechnicalAssets[techAsset].DataAssetsStored, what.Id) { - result = append(result, risk) - break - } } } return result @@ -1387,12 +1364,6 @@ func (what TechnicalAsset) HighestConfidentiality() Confidentiality { highest = dataAsset.Confidentiality } } - for _, dataId := range what.DataAssetsStored { - dataAsset := ParsedModelRoot.DataAssets[dataId] - if dataAsset.Confidentiality > highest { - highest = dataAsset.Confidentiality - } - } return highest } @@ -1440,12 +1411,6 @@ func (what TechnicalAsset) HighestIntegrity() Criticality { highest = dataAsset.Integrity } } - for _, dataId := range what.DataAssetsStored { - dataAsset := ParsedModelRoot.DataAssets[dataId] - if dataAsset.Integrity > highest { - highest = dataAsset.Integrity - } - } return highest } @@ -1457,12 +1422,6 @@ func (what TechnicalAsset) HighestAvailability() Criticality { highest = dataAsset.Availability } } - for _, dataId := range what.DataAssetsStored { - dataAsset := ParsedModelRoot.DataAssets[dataId] - if dataAsset.Availability > highest { - highest = dataAsset.Availability - } - } return highest } @@ -2234,9 +2193,9 @@ func (what CommunicationLink) DetermineArrowLineStyle() string { return "solid" } -// dotted when model forgery attempt (i.e. nothing being processed or stored) +// dotted when model forgery attempt (i.e. nothing being processed) func (what TechnicalAsset) DetermineShapeBorderLineStyle() string { - if len(what.DataAssetsProcessed) == 0 && len(what.DataAssetsStored) == 0 || what.OutOfScope { + if len(what.DataAssetsProcessed) == 0 || what.OutOfScope { return "dotted" // dotted, because it's strange when too many technical communication links transfer no data... some ok, but many in a diagram ist a sign of model forgery... } return "solid" @@ -2358,27 +2317,17 @@ func (what TechnicalAsset) IsZero() bool { } func (what TechnicalAsset) ProcessesOrStoresDataAsset(dataAssetId string) bool { - if Contains(what.DataAssetsProcessed, dataAssetId) { - return true - } - if Contains(what.DataAssetsStored, dataAssetId) { - return true - } - return false + return Contains(what.DataAssetsProcessed, dataAssetId) } -// red when >= confidential data stored in unencrypted technical asset +// red when >= confidential data processed in unencrypted technical asset +// NOTE: maybe this should only check stored data, not processed data? func (what TechnicalAsset) DetermineLabelColor() string { // TODO: Just move into main.go and let the generated risk determine the color, don't duplicate the logic here // Check for red if what.Integrity == MissionCritical { return colors.Red } - for _, storedDataAsset := range what.DataAssetsStored { - if ParsedModelRoot.DataAssets[storedDataAsset].Integrity == MissionCritical { - return colors.Red - } - } for _, processedDataAsset := range what.DataAssetsProcessed { if ParsedModelRoot.DataAssets[processedDataAsset].Integrity == MissionCritical { return colors.Red @@ -2388,11 +2337,6 @@ func (what TechnicalAsset) DetermineLabelColor() string { if what.Integrity == Critical { return colors.Amber } - for _, storedDataAsset := range what.DataAssetsStored { - if ParsedModelRoot.DataAssets[storedDataAsset].Integrity == Critical { - return colors.Amber - } - } for _, processedDataAsset := range what.DataAssetsProcessed { if ParsedModelRoot.DataAssets[processedDataAsset].Integrity == Critical { return colors.Amber @@ -2426,18 +2370,13 @@ func (what TechnicalAsset) DetermineLabelColor() string { // red when mission-critical integrity, but still unauthenticated (non-readonly) channels access it // amber when critical integrity, but still unauthenticated (non-readonly) channels access it -// pink when model forgery attempt (i.e. nothing being processed or stored) +// pink when model forgery attempt (i.e. nothing being processed) func (what TechnicalAsset) DetermineShapeBorderColor() string { // TODO: Just move into main.go and let the generated risk determine the color, don't duplicate the logic here // Check for red if what.Confidentiality == StrictlyConfidential { return colors.Red } - for _, storedDataAsset := range what.DataAssetsStored { - if ParsedModelRoot.DataAssets[storedDataAsset].Confidentiality == StrictlyConfidential { - return colors.Red - } - } for _, processedDataAsset := range what.DataAssetsProcessed { if ParsedModelRoot.DataAssets[processedDataAsset].Confidentiality == StrictlyConfidential { return colors.Red @@ -2447,11 +2386,6 @@ func (what TechnicalAsset) DetermineShapeBorderColor() string { if what.Confidentiality == Confidential { return colors.Amber } - for _, storedDataAsset := range what.DataAssetsStored { - if ParsedModelRoot.DataAssets[storedDataAsset].Confidentiality == Confidential { - return colors.Amber - } - } for _, processedDataAsset := range what.DataAssetsProcessed { if ParsedModelRoot.DataAssets[processedDataAsset].Confidentiality == Confidential { return colors.Amber @@ -2587,7 +2521,7 @@ func (what CommunicationLink) DetermineArrowColor() string { func (what TechnicalAsset) DetermineShapeFillColor() string { fillColor := colors.VeryLightGray - if len(what.DataAssetsProcessed) == 0 && len(what.DataAssetsStored) == 0 || + if len(what.DataAssetsProcessed) == 0 || what.Technology == UnknownTechnology { fillColor = colors.LightPink // lightPink, because it's strange when too many technical assets process no data... some ok, but many in a diagram ist a sign of model forgery... } else if len(what.CommunicationLinks) == 0 && len(IncomingTechnicalCommunicationLinksMappedByTargetId[what.Id]) == 0 { diff --git a/raa/raa/raa.go b/raa/raa/raa.go index a0b98aa3..40317d4c 100644 --- a/raa/raa/raa.go +++ b/raa/raa/raa.go @@ -98,12 +98,14 @@ func calculateAttackerAttractiveness(techAsset model.TechnicalAsset) float64 { score += dataAsset.Integrity.AttackerAttractivenessForProcessedOrStoredData() * dataAsset.Quantity.QuantityFactor() score += dataAsset.Availability.AttackerAttractivenessForProcessedOrStoredData() } + // NOTE: Assuming all stored data is also processed, this effectively scores stored data twice for _, dataAssetStored := range techAsset.DataAssetsStored { dataAsset := model.ParsedModelRoot.DataAssets[dataAssetStored] score += dataAsset.Confidentiality.AttackerAttractivenessForProcessedOrStoredData() * dataAsset.Quantity.QuantityFactor() score += dataAsset.Integrity.AttackerAttractivenessForProcessedOrStoredData() * dataAsset.Quantity.QuantityFactor() score += dataAsset.Availability.AttackerAttractivenessForProcessedOrStoredData() } + // NOTE: To send or receive data effectively is processing that data and it's questionable if the attractiveness increases further for _, dataFlow := range techAsset.CommunicationLinks { for _, dataAssetSent := range dataFlow.DataAssetsSent { dataAsset := model.ParsedModelRoot.DataAssets[dataAssetSent] diff --git a/risks/built-in/accidental-secret-leak/accidental-secret-leak-rule.go b/risks/built-in/accidental-secret-leak/accidental-secret-leak-rule.go index 3f4f9b4c..43051393 100644 --- a/risks/built-in/accidental-secret-leak/accidental-secret-leak-rule.go +++ b/risks/built-in/accidental-secret-leak/accidental-secret-leak-rule.go @@ -23,7 +23,7 @@ func Category() model.RiskCategory { Function: model.Operations, STRIDE: model.InformationDisclosure, DetectionLogic: "In-scope sourcecode repositories and artifact registries.", - RiskAssessment: "The risk rating depends on the sensitivity of the technical asset itself and of the data assets processed and stored.", + RiskAssessment: "The risk rating depends on the sensitivity of the technical asset itself and of the data assets processed.", FalsePositives: "Usually no false positives.", ModelFailurePossibleReason: false, CWE: 200, diff --git a/risks/built-in/container-platform-escape/container-platform-escape-rule.go b/risks/built-in/container-platform-escape/container-platform-escape-rule.go index 520b3d1d..dfb23c18 100644 --- a/risks/built-in/container-platform-escape/container-platform-escape-rule.go +++ b/risks/built-in/container-platform-escape/container-platform-escape-rule.go @@ -28,7 +28,7 @@ func Category() model.RiskCategory { Function: model.Operations, STRIDE: model.ElevationOfPrivilege, DetectionLogic: "In-scope container platforms.", - RiskAssessment: "The risk rating depends on the sensitivity of the technical asset itself and of the data assets processed and stored.", + RiskAssessment: "The risk rating depends on the sensitivity of the technical asset itself and of the data assets processed.", FalsePositives: "Container platforms not running parts of the target architecture can be considered " + "as false positives after individual review.", ModelFailurePossibleReason: false, diff --git a/risks/built-in/cross-site-scripting/cross-site-scripting-rule.go b/risks/built-in/cross-site-scripting/cross-site-scripting-rule.go index a6da6781..471597c5 100644 --- a/risks/built-in/cross-site-scripting/cross-site-scripting-rule.go +++ b/risks/built-in/cross-site-scripting/cross-site-scripting-rule.go @@ -21,7 +21,7 @@ func Category() model.RiskCategory { Function: model.Development, STRIDE: model.Tampering, DetectionLogic: "In-scope web applications.", - RiskAssessment: "The risk rating depends on the sensitivity of the data processed or stored in the web application.", + RiskAssessment: "The risk rating depends on the sensitivity of the data processed in the web application.", FalsePositives: "When the technical asset " + "is not accessed via a browser-like component (i.e not by a human user initiating the request that " + "gets passed through all components until it reaches the web application) this can be considered a false positive.", diff --git a/risks/built-in/ldap-injection/ldap-injection-rule.go b/risks/built-in/ldap-injection/ldap-injection-rule.go index e76d2706..4e69b582 100644 --- a/risks/built-in/ldap-injection/ldap-injection-rule.go +++ b/risks/built-in/ldap-injection/ldap-injection-rule.go @@ -9,7 +9,7 @@ func Category() model.RiskCategory { Id: "ldap-injection", Title: "LDAP-Injection", Description: "When an LDAP server is accessed LDAP-Injection risks might arise. " + - "The risk rating depends on the sensitivity of the LDAP server itself and of the data assets processed or stored.", + "The risk rating depends on the sensitivity of the LDAP server itself and of the data assets processed.", Impact: "If this risk remains unmitigated, attackers might be able to modify LDAP queries and access more data from the LDAP server than allowed.", ASVS: "V5 - Validation, Sanitization and Encoding Verification Requirements", CheatSheet: "https://cheatsheetseries.owasp.org/cheatsheets/LDAP_Injection_Prevention_Cheat_Sheet.html", @@ -21,7 +21,7 @@ func Category() model.RiskCategory { Function: model.Development, STRIDE: model.Tampering, DetectionLogic: "In-scope clients accessing LDAP servers via typical LDAP access protocols.", - RiskAssessment: "The risk rating depends on the sensitivity of the LDAP server itself and of the data assets processed or stored.", + RiskAssessment: "The risk rating depends on the sensitivity of the LDAP server itself and of the data assets processed.", FalsePositives: "LDAP server queries by search values not consisting of parts controllable by the caller can be considered " + "as false positives after individual review.", ModelFailurePossibleReason: false, diff --git a/risks/built-in/missing-authentication-second-factor/missing-authentication-second-factor-rule.go b/risks/built-in/missing-authentication-second-factor/missing-authentication-second-factor-rule.go index e491655d..d1466400 100644 --- a/risks/built-in/missing-authentication-second-factor/missing-authentication-second-factor-rule.go +++ b/risks/built-in/missing-authentication-second-factor/missing-authentication-second-factor-rule.go @@ -10,7 +10,7 @@ func Category() model.RiskCategory { Id: "missing-authentication-second-factor", Title: "Missing Two-Factor Authentication (2FA)", Description: "Technical assets (especially multi-tenant systems) should authenticate incoming requests with " + - "two-factor (2FA) authentication when the asset processes or stores highly sensitive data (in terms of confidentiality, integrity, and availability) and is accessed by humans.", + "two-factor (2FA) authentication when the asset processes highly sensitive data (in terms of confidentiality, integrity, and availability) and is accessed by humans.", Impact: "If this risk is unmitigated, attackers might be able to access or modify highly sensitive data without strong authentication.", ASVS: "V2 - Authentication Verification Requirements", CheatSheet: "https://cheatsheetseries.owasp.org/cheatsheets/Multifactor_Authentication_Cheat_Sheet.html", @@ -21,7 +21,7 @@ func Category() model.RiskCategory { Function: model.BusinessSide, STRIDE: model.ElevationOfPrivilege, DetectionLogic: "In-scope technical assets (except " + model.LoadBalancer.String() + ", " + model.ReverseProxy.String() + ", " + model.WAF.String() + ", " + model.IDS.String() + ", and " + model.IPS.String() + ") should authenticate incoming requests via two-factor authentication (2FA) " + - "when the asset processes or stores highly sensitive data (in terms of confidentiality, integrity, and availability) and is accessed by a client used by a human user.", + "when the asset processes highly sensitive data (in terms of confidentiality, integrity, and availability) and is accessed by a client used by a human user.", RiskAssessment: model.MediumSeverity.String(), FalsePositives: "Technical assets which do not process requests regarding functionality or data linked to end-users (customers) " + "can be considered as false positives after individual review.", diff --git a/risks/built-in/missing-authentication/missing-authentication-rule.go b/risks/built-in/missing-authentication/missing-authentication-rule.go index 9d002242..4fb9d553 100644 --- a/risks/built-in/missing-authentication/missing-authentication-rule.go +++ b/risks/built-in/missing-authentication/missing-authentication-rule.go @@ -8,7 +8,7 @@ func Category() model.RiskCategory { return model.RiskCategory{ Id: "missing-authentication", Title: "Missing Authentication", - Description: "Technical assets (especially multi-tenant systems) should authenticate incoming requests when the asset processes or stores sensitive data. ", + Description: "Technical assets (especially multi-tenant systems) should authenticate incoming requests when the asset processes sensitive data. ", Impact: "If this risk is unmitigated, attackers might be able to access or modify sensitive data in an unauthenticated way.", ASVS: "V2 - Authentication Verification Requirements", CheatSheet: "https://cheatsheetseries.owasp.org/cheatsheets/Authentication_Cheat_Sheet.html", @@ -18,7 +18,7 @@ func Category() model.RiskCategory { Check: "Are recommendations from the linked cheat sheet and referenced ASVS chapter applied?", Function: model.Architecture, STRIDE: model.ElevationOfPrivilege, - DetectionLogic: "In-scope technical assets (except " + model.LoadBalancer.String() + ", " + model.ReverseProxy.String() + ", " + model.ServiceRegistry.String() + ", " + model.WAF.String() + ", " + model.IDS.String() + ", and " + model.IPS.String() + " and in-process calls) should authenticate incoming requests when the asset processes or stores " + + DetectionLogic: "In-scope technical assets (except " + model.LoadBalancer.String() + ", " + model.ReverseProxy.String() + ", " + model.ServiceRegistry.String() + ", " + model.WAF.String() + ", " + model.IDS.String() + ", and " + model.IPS.String() + " and in-process calls) should authenticate incoming requests when the asset processes " + "sensitive data. This is especially the case for all multi-tenant assets (there even non-sensitive ones).", RiskAssessment: "The risk rating (medium or high) " + "depends on the sensitivity of the data sent across the communication link. Monitoring callers are exempted from this risk.", diff --git a/risks/built-in/missing-cloud-hardening/missing-cloud-hardening-rule.go b/risks/built-in/missing-cloud-hardening/missing-cloud-hardening-rule.go index e7dddb3a..c16f4a6d 100644 --- a/risks/built-in/missing-cloud-hardening/missing-cloud-hardening-rule.go +++ b/risks/built-in/missing-cloud-hardening/missing-cloud-hardening-rule.go @@ -28,7 +28,7 @@ func Category() model.RiskCategory { Function: model.Operations, STRIDE: model.Tampering, DetectionLogic: "In-scope cloud components (either residing in cloud trust boundaries or more specifically tagged with cloud provider types).", - RiskAssessment: "The risk rating depends on the sensitivity of the technical asset itself and of the data assets processed and stored.", + RiskAssessment: "The risk rating depends on the sensitivity of the technical asset itself and of the data assets processed.", FalsePositives: "Cloud components not running parts of the target architecture can be considered " + "as false positives after individual review.", ModelFailurePossibleReason: false, diff --git a/risks/built-in/missing-file-validation/missing-file-validation-rule.go b/risks/built-in/missing-file-validation/missing-file-validation-rule.go index c8633038..67879d52 100644 --- a/risks/built-in/missing-file-validation/missing-file-validation-rule.go +++ b/risks/built-in/missing-file-validation/missing-file-validation-rule.go @@ -22,7 +22,7 @@ func Category() model.RiskCategory { Function: model.Development, STRIDE: model.Spoofing, DetectionLogic: "In-scope technical assets with custom-developed code accepting file data formats.", - RiskAssessment: "The risk rating depends on the sensitivity of the technical asset itself and of the data assets processed and stored.", + RiskAssessment: "The risk rating depends on the sensitivity of the technical asset itself and of the data assets processed.", FalsePositives: "Fully trusted (i.e. cryptographically signed or similar) files can be considered " + "as false positives after individual review.", ModelFailurePossibleReason: false, diff --git a/risks/built-in/missing-hardening/missing-hardening-rule.go b/risks/built-in/missing-hardening/missing-hardening-rule.go index b4795740..b920b2be 100644 --- a/risks/built-in/missing-hardening/missing-hardening-rule.go +++ b/risks/built-in/missing-hardening/missing-hardening-rule.go @@ -25,7 +25,7 @@ func Category() model.RiskCategory { STRIDE: model.Tampering, DetectionLogic: "In-scope technical assets with RAA values of " + strconv.Itoa(raaLimit) + " % or higher. " + "Generally for high-value targets like datastores, application servers, identity providers and ERP systems this limit is reduced to " + strconv.Itoa(raaLimitReduced) + " %", - RiskAssessment: "The risk rating depends on the sensitivity of the data processed or stored in the technical asset.", + RiskAssessment: "The risk rating depends on the sensitivity of the data processed in the technical asset.", FalsePositives: "Usually no false positives.", ModelFailurePossibleReason: false, CWE: 16, diff --git a/risks/built-in/missing-identity-store/missing-identity-store-rule.go b/risks/built-in/missing-identity-store/missing-identity-store-rule.go index 9096e320..d4f8c8a9 100644 --- a/risks/built-in/missing-identity-store/missing-identity-store-rule.go +++ b/risks/built-in/missing-identity-store/missing-identity-store-rule.go @@ -21,7 +21,7 @@ func Category() model.RiskCategory { STRIDE: model.Spoofing, DetectionLogic: "Models with authenticated data-flows authorized via enduser-identity missing an in-scope identity store.", RiskAssessment: "The risk rating depends on the sensitivity of the enduser-identity authorized technical assets and " + - "their data assets processed and stored.", + "their data assets processed.", FalsePositives: "Models only offering data/services without any real authentication need " + "can be considered as false positives after individual review.", ModelFailurePossibleReason: true, diff --git a/risks/built-in/missing-vault/missing-vault-rule.go b/risks/built-in/missing-vault/missing-vault-rule.go index a046131b..c5d6bf53 100644 --- a/risks/built-in/missing-vault/missing-vault-rule.go +++ b/risks/built-in/missing-vault/missing-vault-rule.go @@ -22,7 +22,7 @@ func Category() model.RiskCategory { Function: model.Architecture, STRIDE: model.InformationDisclosure, DetectionLogic: "Models without a Vault (Secret Storage).", - RiskAssessment: "The risk rating depends on the sensitivity of the technical asset itself and of the data assets processed and stored.", + RiskAssessment: "The risk rating depends on the sensitivity of the technical asset itself and of the data assets processed.", FalsePositives: "Models where no technical assets have any kind of sensitive config data to protect " + "can be considered as false positives after individual review.", ModelFailurePossibleReason: true, diff --git a/risks/built-in/missing-waf/missing-waf-rule.go b/risks/built-in/missing-waf/missing-waf-rule.go index 684cc9af..152722ab 100644 --- a/risks/built-in/missing-waf/missing-waf-rule.go +++ b/risks/built-in/missing-waf/missing-waf-rule.go @@ -21,7 +21,7 @@ func Category() model.RiskCategory { Function: model.Operations, STRIDE: model.Tampering, DetectionLogic: "In-scope web-services and/or web-applications accessed across a network trust boundary not having a Web Application Firewall (WAF) in front of them.", - RiskAssessment: "The risk rating depends on the sensitivity of the technical asset itself and of the data assets processed and stored.", + RiskAssessment: "The risk rating depends on the sensitivity of the technical asset itself and of the data assets processed.", FalsePositives: "Targets only accessible via WAFs or reverse proxies containing a WAF component (like ModSecurity) can be considered " + "as false positives after individual review.", ModelFailurePossibleReason: false, diff --git a/risks/built-in/path-traversal/path-traversal-rule.go b/risks/built-in/path-traversal/path-traversal-rule.go index 1258c039..bf354d1a 100644 --- a/risks/built-in/path-traversal/path-traversal-rule.go +++ b/risks/built-in/path-traversal/path-traversal-rule.go @@ -9,7 +9,7 @@ func Category() model.RiskCategory { Id: "path-traversal", Title: "Path-Traversal", Description: "When a filesystem is accessed Path-Traversal or Local-File-Inclusion (LFI) risks might arise. " + - "The risk rating depends on the sensitivity of the technical asset itself and of the data assets processed or stored.", + "The risk rating depends on the sensitivity of the technical asset itself and of the data assets processed.", Impact: "If this risk is unmitigated, attackers might be able to read sensitive files (configuration data, key/credential files, deployment files, " + "business data files, etc.) from the filesystem of affected components.", ASVS: "V12 - File and Resources Verification Requirements", @@ -23,7 +23,7 @@ func Category() model.RiskCategory { Function: model.Development, STRIDE: model.InformationDisclosure, DetectionLogic: "Filesystems accessed by in-scope callers.", - RiskAssessment: "The risk rating depends on the sensitivity of the data stored inside the technical asset.", + RiskAssessment: "The risk rating depends on the sensitivity of the data processed inside the technical asset.", FalsePositives: "File accesses by filenames not consisting of parts controllable by the caller can be considered " + "as false positives after individual review.", ModelFailurePossibleReason: false, diff --git a/risks/built-in/search-query-injection/search-query-injection-rule.go b/risks/built-in/search-query-injection/search-query-injection-rule.go index 936ab6e6..e3ada1cf 100644 --- a/risks/built-in/search-query-injection/search-query-injection-rule.go +++ b/risks/built-in/search-query-injection/search-query-injection-rule.go @@ -24,7 +24,7 @@ func Category() model.RiskCategory { Function: model.Development, STRIDE: model.Tampering, DetectionLogic: "In-scope clients accessing search engine servers via typical search access protocols.", - RiskAssessment: "The risk rating depends on the sensitivity of the search engine server itself and of the data assets processed or stored.", + RiskAssessment: "The risk rating depends on the sensitivity of the search engine server itself and of the data assets processed.", FalsePositives: "Server engine queries by search values not consisting of parts controllable by the caller can be considered " + "as false positives after individual review.", ModelFailurePossibleReason: false, diff --git a/risks/built-in/service-registry-poisoning/service-registry-poisoning-rule.go b/risks/built-in/service-registry-poisoning/service-registry-poisoning-rule.go index 5cee3a3e..a6d1313c 100644 --- a/risks/built-in/service-registry-poisoning/service-registry-poisoning-rule.go +++ b/risks/built-in/service-registry-poisoning/service-registry-poisoning-rule.go @@ -20,7 +20,7 @@ func Category() model.RiskCategory { STRIDE: model.Spoofing, DetectionLogic: "In-scope service registries.", RiskAssessment: "The risk rating depends on the sensitivity of the technical assets accessing the service registry " + - "as well as the data assets processed or stored.", + "as well as the data assets processed.", FalsePositives: "Service registries not used for service discovery " + "can be considered as false positives after individual review.", ModelFailurePossibleReason: false, diff --git a/risks/built-in/sql-nosql-injection/sql-nosql-injection-rule.go b/risks/built-in/sql-nosql-injection/sql-nosql-injection-rule.go index 331f54e8..c62ea6ff 100644 --- a/risks/built-in/sql-nosql-injection/sql-nosql-injection-rule.go +++ b/risks/built-in/sql-nosql-injection/sql-nosql-injection-rule.go @@ -9,7 +9,7 @@ func Category() model.RiskCategory { Id: "sql-nosql-injection", Title: "SQL/NoSQL-Injection", Description: "When a database is accessed via database access protocols SQL/NoSQL-Injection risks might arise. " + - "The risk rating depends on the sensitivity technical asset itself and of the data assets processed or stored.", + "The risk rating depends on the sensitivity technical asset itself and of the data assets processed.", Impact: "If this risk is unmitigated, attackers might be able to modify SQL/NoSQL queries to steal and modify data and eventually further escalate towards a deeper system penetration via code executions.", ASVS: "V5 - Validation, Sanitization and Encoding Verification Requirements", CheatSheet: "https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html", @@ -20,7 +20,7 @@ func Category() model.RiskCategory { Function: model.Development, STRIDE: model.Tampering, DetectionLogic: "Database accessed via typical database access protocols by in-scope clients.", - RiskAssessment: "The risk rating depends on the sensitivity of the data stored inside the database.", + RiskAssessment: "The risk rating depends on the sensitivity of the data processed inside the database.", FalsePositives: "Database accesses by queries not consisting of parts controllable by the caller can be considered " + "as false positives after individual review.", ModelFailurePossibleReason: false, diff --git a/risks/built-in/unencrypted-asset/unencrypted-asset-rule.go b/risks/built-in/unencrypted-asset/unencrypted-asset-rule.go index cdf23c6b..564c08e1 100644 --- a/risks/built-in/unencrypted-asset/unencrypted-asset-rule.go +++ b/risks/built-in/unencrypted-asset/unencrypted-asset-rule.go @@ -24,6 +24,7 @@ func Category() model.RiskCategory { "storing data assets rated at least as " + model.Confidential.String() + " or " + model.Critical.String() + ". " + "For technical assets storing data assets rated as " + model.StrictlyConfidential.String() + " or " + model.MissionCritical.String() + " the " + "encryption must be of type " + model.DataWithEnduserIndividualKey.String() + ".", + // NOTE: the risk assesment does not only consider the CIs of the *stored* data-assets RiskAssessment: "Depending on the confidentiality rating of the stored data-assets either medium or high risk.", FalsePositives: "When all sensitive data stored within the asset is already fully encrypted on document or data level.", ModelFailurePossibleReason: false, diff --git a/risks/built-in/unnecessary-data-asset/unnecessary-data-asset-rule.go b/risks/built-in/unnecessary-data-asset/unnecessary-data-asset-rule.go index 2af7c618..b713e48f 100644 --- a/risks/built-in/unnecessary-data-asset/unnecessary-data-asset-rule.go +++ b/risks/built-in/unnecessary-data-asset/unnecessary-data-asset-rule.go @@ -9,7 +9,7 @@ func Category() model.RiskCategory { return model.RiskCategory{ Id: "unnecessary-data-asset", Title: "Unnecessary Data Asset", - Description: "When a data asset is not processed or stored by any data assets and also not transferred by any " + + Description: "When a data asset is not processed by any data assets and also not transferred by any " + "communication links, this is an indicator for an unnecessary data asset (or for an incomplete model).", Impact: "If this risk is unmitigated, attackers might be able to access unnecessary data assets using " + "other vulnerabilities.", @@ -20,7 +20,7 @@ func Category() model.RiskCategory { Check: "Are recommendations from the linked cheat sheet and referenced ASVS chapter applied?", Function: model.Architecture, STRIDE: model.ElevationOfPrivilege, - DetectionLogic: "Modelled data assets not processed or stored by any data assets and also not transferred by any " + + DetectionLogic: "Modelled data assets not processed by any data assets and also not transferred by any " + "communication links.", RiskAssessment: model.LowSeverity.String(), FalsePositives: "Usually no false positives as this looks like an incomplete model.", diff --git a/risks/built-in/unnecessary-data-transfer/unnecessary-data-transfer-rule.go b/risks/built-in/unnecessary-data-transfer/unnecessary-data-transfer-rule.go index f33c58a0..74f74680 100644 --- a/risks/built-in/unnecessary-data-transfer/unnecessary-data-transfer-rule.go +++ b/risks/built-in/unnecessary-data-transfer/unnecessary-data-transfer-rule.go @@ -9,15 +9,15 @@ func Category() model.RiskCategory { return model.RiskCategory{ Id: "unnecessary-data-transfer", Title: "Unnecessary Data Transfer", - Description: "When a technical asset sends or receives data assets, which it neither processes or stores this is " + + Description: "When a technical asset sends or receives data assets, which it does not processes this is " + "an indicator for unnecessarily transferred data (or for an incomplete model). When the unnecessarily " + "transferred data assets are sensitive, this poses an unnecessary risk of an increased attack surface.", Impact: "If this risk is unmitigated, attackers might be able to target unnecessarily transferred data.", ASVS: "V1 - Architecture, Design and Threat Modeling Requirements", CheatSheet: "https://cheatsheetseries.owasp.org/cheatsheets/Attack_Surface_Analysis_Cheat_Sheet.html", Action: "Attack Surface Reduction", - Mitigation: "Try to avoid sending or receiving sensitive data assets which are not required (i.e. neither " + - "processed or stored) by the involved technical asset.", + Mitigation: "Try to avoid sending or receiving sensitive data assets which are not required (i.e. not " + + "processed) by the involved technical asset.", Check: "Are recommendations from the linked cheat sheet and referenced ASVS chapter applied?", Function: model.Architecture, STRIDE: model.ElevationOfPrivilege, @@ -28,7 +28,7 @@ func Category() model.RiskCategory { "either " + model.LowSeverity.String() + " or " + model.MediumSeverity.String() + ".", FalsePositives: "Technical assets missing the model entries of either processing or storing the mentioned data assets " + "can be considered as false positives (incomplete models) after individual review. These should then be addressed by " + - "completing the model so that all necessary data assets are processed and/or stored by the technical asset involved.", + "completing the model so that all necessary data assets are processed by the technical asset involved.", ModelFailurePossibleReason: true, CWE: 1008, } diff --git a/risks/built-in/unnecessary-technical-asset/unnecessary-technical-asset-rule.go b/risks/built-in/unnecessary-technical-asset/unnecessary-technical-asset-rule.go index 012117e8..a7d110e3 100644 --- a/risks/built-in/unnecessary-technical-asset/unnecessary-technical-asset-rule.go +++ b/risks/built-in/unnecessary-technical-asset/unnecessary-technical-asset-rule.go @@ -8,14 +8,14 @@ func Category() model.RiskCategory { return model.RiskCategory{ Id: "unnecessary-technical-asset", Title: "Unnecessary Technical Asset", - Description: "When a technical asset does not process or store any data assets, this is " + + Description: "When a technical asset does not process any data assets, this is " + "an indicator for an unnecessary technical asset (or for an incomplete model). " + "This is also the case if the asset has no communication links (either outgoing or incoming).", Impact: "If this risk is unmitigated, attackers might be able to target unnecessary technical assets.", ASVS: "V1 - Architecture, Design and Threat Modeling Requirements", CheatSheet: "https://cheatsheetseries.owasp.org/cheatsheets/Attack_Surface_Analysis_Cheat_Sheet.html", Action: "Attack Surface Reduction", - Mitigation: "Try to avoid using technical assets that do not process or store anything.", + Mitigation: "Try to avoid using technical assets that do not process anything.", Check: "Are recommendations from the linked cheat sheet and referenced ASVS chapter applied?", Function: model.Architecture, STRIDE: model.ElevationOfPrivilege, @@ -35,7 +35,7 @@ func GenerateRisks() []model.Risk { risks := make([]model.Risk, 0) for _, id := range model.SortedTechnicalAssetIDs() { technicalAsset := model.ParsedModelRoot.TechnicalAssets[id] - if len(technicalAsset.DataAssetsProcessed) == 0 && len(technicalAsset.DataAssetsStored) == 0 || + if len(technicalAsset.DataAssetsProcessed) == 0 || (len(technicalAsset.CommunicationLinks) == 0 && len(model.IncomingTechnicalCommunicationLinksMappedByTargetId[technicalAsset.Id]) == 0) { risks = append(risks, createRisk(technicalAsset)) } diff --git a/risks/built-in/untrusted-deserialization/untrusted-deserialization-rule.go b/risks/built-in/untrusted-deserialization/untrusted-deserialization-rule.go index 5da7f2db..c251ec95 100644 --- a/risks/built-in/untrusted-deserialization/untrusted-deserialization-rule.go +++ b/risks/built-in/untrusted-deserialization/untrusted-deserialization-rule.go @@ -24,7 +24,7 @@ func Category() model.RiskCategory { Function: model.Architecture, STRIDE: model.Tampering, DetectionLogic: "In-scope technical assets accepting serialization data formats (including EJB and RMI protocols).", - RiskAssessment: "The risk rating depends on the sensitivity of the technical asset itself and of the data assets processed and stored.", + RiskAssessment: "The risk rating depends on the sensitivity of the technical asset itself and of the data assets processed.", FalsePositives: "Fully trusted (i.e. cryptographically signed or similar) data deserialized can be considered " + "as false positives after individual review.", ModelFailurePossibleReason: false, diff --git a/risks/built-in/xml-external-entity/xml-external-entity-rule.go b/risks/built-in/xml-external-entity/xml-external-entity-rule.go index e6e4778a..3ca09ef8 100644 --- a/risks/built-in/xml-external-entity/xml-external-entity-rule.go +++ b/risks/built-in/xml-external-entity/xml-external-entity-rule.go @@ -21,7 +21,7 @@ func Category() model.RiskCategory { Function: model.Development, STRIDE: model.InformationDisclosure, DetectionLogic: "In-scope technical assets accepting XML data formats.", - RiskAssessment: "The risk rating depends on the sensitivity of the technical asset itself and of the data assets processed and stored. " + + RiskAssessment: "The risk rating depends on the sensitivity of the technical asset itself and of the data assets processed. " + "Also for cloud-based environments the exploitation impact is at least medium, as cloud backend services can be attacked via SSRF (and XXE vulnerabilities are often also SSRF vulnerabilities).", FalsePositives: "Fully trusted (i.e. cryptographically signed or similar) XML data can be considered " + "as false positives after individual review.", diff --git a/support/schema.json b/support/schema.json index 067b5372..57e29742 100644 --- a/support/schema.json +++ b/support/schema.json @@ -470,7 +470,7 @@ "type": "boolean" }, "data_assets_processed": { - "description": "Data assets processed", + "description": "Data assets processed; all data assets stored or send or received via a communication link are implicitly also processed", "type": [ "array", "null" @@ -680,9 +680,7 @@ "vpn", "ip_filtered", "readonly", - "usage", - "data_assets_sent", - "data_assets_received" + "usage" ] } }, @@ -705,8 +703,6 @@ "multi_tenant", "redundant", "custom_developed_parts", - "data_assets_processed", - "data_assets_stored", "data_formats_accepted", "communication_links" ] From 29abff21d446d10b9d2f119a5f90467a1b00f6e7 Mon Sep 17 00:00:00 2001 From: aceg1k <86647736+aceg1k@users.noreply.github.com> Date: Tue, 29 Jun 2021 13:18:01 +0200 Subject: [PATCH 2/2] Data assets are processed by communication targets Data assets sent to or received by a technical asset as the target of a communication link are also always implicitly processed by that target. - Data assets sent to or received by a target through a communication link are always added to that targets' processed data assets. --- main.go | 23 +++++++++++++++++++++++ support/schema.json | 2 +- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/main.go b/main.go index e874b27e..35e6737f 100644 --- a/main.go +++ b/main.go @@ -4598,6 +4598,29 @@ func parseModel(inputFilename string) { } } + // A target of a communication link implicitly processes all data assets that are sent to or received by that target + for id, techAsset := range model.ParsedModelRoot.TechnicalAssets { + for _, commLink := range techAsset.CommunicationLinks { + if commLink.TargetId == id { + continue + } + targetTechAsset := model.ParsedModelRoot.TechnicalAssets[commLink.TargetId] + dataAssetsProcessedByTarget := targetTechAsset.DataAssetsProcessed + for _, dataAssetSent := range commLink.DataAssetsSent { + if !model.Contains(dataAssetsProcessedByTarget, dataAssetSent) { + dataAssetsProcessedByTarget = append(dataAssetsProcessedByTarget, dataAssetSent) + } + } + for _, dataAssetReceived := range commLink.DataAssetsReceived { + if !model.Contains(dataAssetsProcessedByTarget, dataAssetReceived) { + dataAssetsProcessedByTarget = append(dataAssetsProcessedByTarget, dataAssetReceived) + } + } + targetTechAsset.DataAssetsProcessed = dataAssetsProcessedByTarget + model.ParsedModelRoot.TechnicalAssets[commLink.TargetId] = targetTechAsset + } + } + // Trust Boundaries =============================================================================== checklistToAvoidAssetBeingModeledInMultipleTrustBoundaries := make(map[string]bool) model.ParsedModelRoot.TrustBoundaries = make(map[string]model.TrustBoundary) diff --git a/support/schema.json b/support/schema.json index 57e29742..8d07954a 100644 --- a/support/schema.json +++ b/support/schema.json @@ -470,7 +470,7 @@ "type": "boolean" }, "data_assets_processed": { - "description": "Data assets processed; all data assets stored or send or received via a communication link are implicitly also processed", + "description": "Data assets processed; all data assets stored or sent or received via a communication link (be it as a source or a target) are implicitly also processed and do not need to be listed here.", "type": [ "array", "null"