From 5edb0d25663d54df4f38d11ec1ac9762f6372171 Mon Sep 17 00:00:00 2001 From: xgoffin <86716549+xgoffin@users.noreply.github.com> Date: Thu, 9 Jun 2022 10:32:08 +0200 Subject: [PATCH] feat(fortifyExecuteScan): implement a system to limit the number of API calls upon request failures (#3818) * feat(fortifyExecuteScan): add a max number of retries for API calls in SARIF conversion * feat(checkmarxExecuteScan): implement max number of retries on API call for descriptions in SARIF processing * feat(checkmarx/fortify): extra logging line when failing an API request in SARIF conversion * fix(fortifyExecuteScan): panic if undefined projectversion in sarif * fix(fortifyExecuteScan): logging improvement * fix(fortifyExecuteScan): wrong if condition caused crash * fix(fortifyExecuteScan): do not log if retries hit -1, adjust logging * fix(SARIF): commenting API calls for Checkmarx until a solution can be found for the API issues * feat(SARIF): add omitempty to extensions --- pkg/checkmarx/cxxml_to_sarif.go | 60 +++++++++++++--------------- pkg/checkmarx/cxxml_to_sarif_test.go | 2 +- pkg/format/sarif.go | 2 +- pkg/fortify/fpr_to_sarif.go | 40 ++++++++++++------- pkg/fortify/fpr_to_sarif_test.go | 33 +++++++++++---- 5 files changed, 80 insertions(+), 57 deletions(-) diff --git a/pkg/checkmarx/cxxml_to_sarif.go b/pkg/checkmarx/cxxml_to_sarif.go index 1c9da38b5..63ab00231 100644 --- a/pkg/checkmarx/cxxml_to_sarif.go +++ b/pkg/checkmarx/cxxml_to_sarif.go @@ -153,32 +153,40 @@ func Parse(sys System, data []byte, scanID int) (format.SARIF, error) { baseURL := "https://" + strings.Split(cxxml.DeepLink, "/")[2] + "/CxWebClient/ScanQueryDescription.aspx?" cweIdsForTaxonomies := make(map[string]int) //use a map to avoid duplicates cweCounter := 0 - var apiDescription string + //maxretries := 5 //CxXML files contain a CxXMLResults > Query object, which represents a broken rule or type of vuln //This Query object contains a list of Result objects, each representing an occurence //Each Result object contains a ResultPath, which represents the exact location of the occurence (the "Snippet") log.Entry().Debug("[SARIF] Now handling results.") for i := 0; i < len(cxxml.Query); i++ { - descriptionFetched := false //add cweid to array cweIdsForTaxonomies[cxxml.Query[i].CweID] = cweCounter cweCounter = cweCounter + 1 for j := 0; j < len(cxxml.Query[i].Result); j++ { + var apiDescription string result := *new(format.Results) + // COMMENTED UNTIL CHECKMARX API WORK AS INTENDED // For rules later, fetch description - if !descriptionFetched { + /*if maxretries == 0 { // Don't spam logfile: only enter the loop if maxretries is positive, and only display the error if it hits 0 + log.Entry().Error("request failed: maximum number of retries reached, descriptions will no longer be fetched") + maxretries = maxretries - 1 + } else if maxretries > 0 { if sys != nil { apiShortDescription, err := sys.GetShortDescription(scanID, cxxml.Query[i].Result[j].Path.PathID) if err != nil { + maxretries = maxretries - 1 + log.Entry().Debug("request failed: remaining retries ", maxretries) log.Entry().Error(err) } else { - descriptionFetched = true apiDescription = apiShortDescription.Text } + } else { + maxretries = maxretries - 1 + log.Entry().Debug("request failed: no system instance, remaining retries ", maxretries) } - } + }*/ //General result.RuleID = "checkmarx-" + cxxml.Query[i].ID @@ -186,13 +194,13 @@ func Parse(sys System, data []byte, scanID int) (format.SARIF, error) { result.Level = "none" msg := new(format.Message) //msg.Text = cxxml.Query[i].Name + ": " + cxxml.Query[i].Categories - msg.Text = cxxml.Query[i].Name + if apiDescription != "" { + msg.Text = apiDescription + } else { + msg.Text = cxxml.Query[i].Name + } result.Message = msg - //analysisTarget := new(format.ArtifactLocation) - //analysisTarget.URI = cxxml.Query[i].Result[j].FileName - //analysisTarget.Index = index - //result.AnalysisTarget = analysisTarget if cxxml.Query[i].Name != "" { msg := new(format.Message) msg.Text = cxxml.Query[i].Name @@ -200,22 +208,6 @@ func Parse(sys System, data []byte, scanID int) (format.SARIF, error) { //Locations for k := 0; k < len(cxxml.Query[i].Result[j].Path.PathNode); k++ { loc := *new(format.Location) - /*index := 0 - //Check if that artifact has been added - added := false - for file := 0; file < len(sarif.Runs[0].Artifacts); file++ { - if sarif.Runs[0].Artifacts[file].Location.Uri == cxxml.Query[i].Result[j].FileName { - added = true - index = file - break - } - } - if !added { - artifact := format.Artifact{Location: format.SarifLocation{Uri: cxxml.Query[i].Result[j].FileName}} - sarif.Runs[0].Artifacts = append(sarif.Runs[0].Artifacts, artifact) - index = len(sarif.Runs[0].Artifacts) - 1 - } - loc.PhysicalLocation.ArtifactLocation.Index = index */ loc.PhysicalLocation.ArtifactLocation.URI = cxxml.Query[i].Result[j].FileName loc.PhysicalLocation.Region.StartLine = cxxml.Query[i].Result[j].Path.PathNode[k].Line loc.PhysicalLocation.Region.EndLine = cxxml.Query[i].Result[j].Path.PathNode[k].Line @@ -223,9 +215,6 @@ func Parse(sys System, data []byte, scanID int) (format.SARIF, error) { snip := new(format.SnippetSarif) snip.Text = cxxml.Query[i].Result[j].Path.PathNode[k].Snippet.Line.Code loc.PhysicalLocation.Region.Snippet = snip - //loc.PhysicalLocation.ContextRegion.StartLine = cxxml.Query[i].Result[j].Path.PathNode[k].Line - //loc.PhysicalLocation.ContextRegion.EndLine = cxxml.Query[i].Result[j].Path.PathNode[k].Line - //loc.PhysicalLocation.ContextRegion.Snippet = snip result.Locations = append(result.Locations, loc) //Related Locations @@ -309,14 +298,21 @@ func Parse(sys System, data []byte, scanID int) (format.SARIF, error) { rule.Help.Text = rule.HelpURI rule.ShortDescription = new(format.Message) rule.ShortDescription.Text = cxxml.Query[i].Name - if apiDescription != "" { + rule.Properties = new(format.SarifRuleProperties) + /*if apiDescription != "" { rule.FullDescription = new(format.Message) rule.FullDescription.Text = apiDescription - } else if cxxml.Query[i].Categories != "" { + } else */if cxxml.Query[i].Categories != "" { rule.FullDescription = new(format.Message) rule.FullDescription.Text = cxxml.Query[i].Categories + + //split categories on ; + cats := strings.Split(cxxml.Query[i].Categories, ";") + for cat := 0; cat < len(cats); cat++ { + rule.Properties.Tags = append(rule.Properties.Tags, cats[cat]) + } } - rule.Properties = new(format.SarifRuleProperties) + if cxxml.Query[i].CweID != "" { rule.Properties.Tags = append(rule.Properties.Tags, "external/cwe/cwe-"+cxxml.Query[i].CweID) } diff --git a/pkg/checkmarx/cxxml_to_sarif_test.go b/pkg/checkmarx/cxxml_to_sarif_test.go index 7684c4691..8d46510f8 100644 --- a/pkg/checkmarx/cxxml_to_sarif_test.go +++ b/pkg/checkmarx/cxxml_to_sarif_test.go @@ -121,7 +121,7 @@ func TestParse(t *testing.T) { assert.Equal(t, len(sarif.Runs[0].Tool.Driver.Rules), 2) assert.Equal(t, sarif.Runs[0].Results[2].Properties.ToolState, "Confirmed") assert.Equal(t, sarif.Runs[0].Results[2].Properties.ToolAuditMessage, "Changed status to Confirmed \n Dummy comment") - assert.Equal(t, "This is a dummy short description.", sarif.Runs[0].Tool.Driver.Rules[0].FullDescription.Text) + //assert.Equal(t, "This is a dummy short description.", sarif.Runs[0].Tool.Driver.Rules[0].FullDescription.Text) }) t.Run("Missing sys", func(t *testing.T) { diff --git a/pkg/format/sarif.go b/pkg/format/sarif.go index 9bcc11b3a..f47f0d00a 100644 --- a/pkg/format/sarif.go +++ b/pkg/format/sarif.go @@ -103,7 +103,7 @@ type SarifProperties struct { // Tool these structs are relevant to the Tool object type Tool struct { Driver Driver `json:"driver"` - Extensions []Driver `json:"extensions"` + Extensions []Driver `json:"extensions,omitempty"` } // Driver meta information for the scan and tool context diff --git a/pkg/fortify/fpr_to_sarif.go b/pkg/fortify/fpr_to_sarif.go index 4f708b166..8993ebd36 100644 --- a/pkg/fortify/fpr_to_sarif.go +++ b/pkg/fortify/fpr_to_sarif.go @@ -542,17 +542,21 @@ func Parse(sys System, project *models.Project, projectVersion *models.ProjectVe log.Entry().Debug("Querying Fortify SSC for batch audit data") oneRequestPerIssueMode := false var auditData []*models.ProjectVersionIssue - if sys != nil { + maxretries := 5 // Maximum number of requests allowed to fail before stopping them + if sys != nil && projectVersion != nil { auditData, err = sys.GetAllIssueDetails(projectVersion.ID) - if err != nil { + if err != nil || len(auditData) == 0 { // It's reasonable to admit that with a length of 0, something went wrong log.Entry().WithError(err).Error("failed to get all audit data, defaulting to one-request-per-issue basis") oneRequestPerIssueMode = true + // We do not lower maxretries here in case a "real" bug happened } else { log.Entry().Debug("Request successful, data frame size: ", len(auditData), " audits") } } else { - log.Entry().Error("no system instance found, lookup impossible") + log.Entry().Error("no system instance or project version found, lookup impossible") oneRequestPerIssueMode = true + maxretries = 1 // Set to 1 if the sys instance isn't defined: chances are it couldn't be created, we'll live a chance if there was an unknown bug + log.Entry().Debug("request failed: remaining retries ", maxretries) } //Now, we handle the sarif @@ -785,17 +789,12 @@ func Parse(sys System, project *models.Project, projectVersion *models.ProjectVe prop.Confidence = fvdl.Vulnerabilities.Vulnerability[i].InstanceInfo.Confidence prop.InstanceID = fvdl.Vulnerabilities.Vulnerability[i].InstanceInfo.InstanceID //Get the audit data - if sys != nil { - if err := integrateAuditData(prop, fvdl.Vulnerabilities.Vulnerability[i].InstanceInfo.InstanceID, sys, project, projectVersion, auditData, filterSet, oneRequestPerIssueMode); err != nil { - log.Entry().Debug(err) - prop.Audited = false - prop.ToolState = "Unknown" - prop.ToolAuditMessage = "Error fetching audit state" + if err := integrateAuditData(prop, fvdl.Vulnerabilities.Vulnerability[i].InstanceInfo.InstanceID, sys, project, projectVersion, auditData, filterSet, oneRequestPerIssueMode, maxretries); err != nil { + log.Entry().Debug(err) + maxretries = maxretries - 1 + if maxretries >= 0 { + log.Entry().Debug("request failed: remaining retries ", maxretries) } - } else { - prop.Audited = false - prop.ToolState = "Unknown" - prop.ToolAuditMessage = "Cannot fetch audit state" } result.Properties = prop @@ -1157,16 +1156,25 @@ func Parse(sys System, project *models.Project, projectVersion *models.ProjectVe return sarif, nil } -func integrateAuditData(ruleProp *format.SarifProperties, issueInstanceID string, sys System, project *models.Project, projectVersion *models.ProjectVersion, auditData []*models.ProjectVersionIssue, filterSet *models.FilterSet, oneRequestPerIssue bool) error { +func integrateAuditData(ruleProp *format.SarifProperties, issueInstanceID string, sys System, project *models.Project, projectVersion *models.ProjectVersion, auditData []*models.ProjectVersionIssue, filterSet *models.FilterSet, oneRequestPerIssue bool, maxretries int) error { // Set default values ruleProp.Audited = false ruleProp.FortifyCategory = "Unknown" ruleProp.ToolSeverity = "Unknown" - ruleProp.ToolState = "Unreviewed" + ruleProp.ToolState = "Unknown" + ruleProp.ToolAuditMessage = "Error fetching audit state" // We set this as default for the error phase, then reset it to nothing ruleProp.ToolSeverityIndex = 0 ruleProp.ToolStateIndex = 0 // These default values allow for the property bag to be filled even if an error happens later. They all should be overwritten by a normal course of the progrma. + if maxretries == 0 { + // Max retries reached, we stop there to avoid a longer execution time + err := errors.New("request failed: maximum number of retries reached, placeholder values will be set from now on for audit data") + return err + } else if maxretries < 0 { + return nil // Avoid spamming logfile + } if sys == nil { + ruleProp.ToolAuditMessage = "Cannot fetch audit state: no sys instance" err := errors.New("no system instance, lookup impossible for " + issueInstanceID) return err } @@ -1174,6 +1182,8 @@ func integrateAuditData(ruleProp *format.SarifProperties, issueInstanceID string err := errors.New("project or projectVersion is undefined: lookup aborted for " + issueInstanceID) return err } + // Reset the audit message + ruleProp.ToolAuditMessage = "" var data []*models.ProjectVersionIssue var err error if oneRequestPerIssue { diff --git a/pkg/fortify/fpr_to_sarif_test.go b/pkg/fortify/fpr_to_sarif_test.go index 779f3224c..871e2716f 100644 --- a/pkg/fortify/fpr_to_sarif_test.go +++ b/pkg/fortify/fpr_to_sarif_test.go @@ -385,7 +385,7 @@ If you are concerned about leaking system data via NFC on an Android device, you assert.Equal(t, len(sarif.Runs[0].Results), 2) assert.Equal(t, len(sarif.Runs[0].Tool.Driver.Rules), 1) assert.Equal(t, sarif.Runs[0].Results[0].Properties.ToolState, "Unknown") - assert.Equal(t, sarif.Runs[0].Results[0].Properties.ToolAuditMessage, "Cannot fetch audit state") + assert.Equal(t, sarif.Runs[0].Results[0].Properties.ToolAuditMessage, "Cannot fetch audit state: no sys instance") }) } @@ -441,7 +441,7 @@ func TestIntegrateAuditData(t *testing.T) { project := models.Project{} projectVersion := models.ProjectVersion{ID: 11037} auditData, _ := sys.GetAllIssueDetails(projectVersion.ID) - err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, &projectVersion, auditData, filterSet, false) + err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, &projectVersion, auditData, filterSet, false, 5) assert.NoError(t, err, "error") assert.Equal(t, ruleProp.Audited, true) assert.Equal(t, ruleProp.ToolState, "Exploitable") @@ -456,7 +456,7 @@ func TestIntegrateAuditData(t *testing.T) { ruleProp := *new(format.SarifProperties) projectVersion := models.ProjectVersion{ID: 11037} auditData, _ := sys.GetAllIssueDetails(projectVersion.ID) - err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, nil, &projectVersion, auditData, filterSet, false) + err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, nil, &projectVersion, auditData, filterSet, false, 5) assert.Error(t, err, "project or projectVersion is undefined: lookup aborted for 11037") }) @@ -464,7 +464,7 @@ func TestIntegrateAuditData(t *testing.T) { ruleProp := *new(format.SarifProperties) project := models.Project{} auditData, _ := sys.GetAllIssueDetails(11037) - err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, nil, auditData, filterSet, false) + err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, nil, auditData, filterSet, false, 5) assert.Error(t, err, "project or projectVersion is undefined: lookup aborted for 11037") }) @@ -473,7 +473,7 @@ func TestIntegrateAuditData(t *testing.T) { project := models.Project{} projectVersion := models.ProjectVersion{ID: 11037} auditData, _ := sys.GetAllIssueDetails(projectVersion.ID) - err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", nil, &project, &projectVersion, auditData, filterSet, false) + err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", nil, &project, &projectVersion, auditData, filterSet, false, 5) assert.Error(t, err, "no system instance, lookup impossible for DUMMYDUMMYDUMMY") }) @@ -482,7 +482,7 @@ func TestIntegrateAuditData(t *testing.T) { project := models.Project{} projectVersion := models.ProjectVersion{ID: 11037} auditData, _ := sys.GetAllIssueDetails(projectVersion.ID) - err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, &projectVersion, auditData, nil, false) + err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, &projectVersion, auditData, nil, false, 5) assert.Error(t, err, "no filter set defined, category will be missing from 11037") }) @@ -490,7 +490,7 @@ func TestIntegrateAuditData(t *testing.T) { ruleProp := *new(format.SarifProperties) project := models.Project{} projectVersion := models.ProjectVersion{ID: 11037} - err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, &projectVersion, nil, filterSet, false) + err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, &projectVersion, nil, filterSet, false, 5) assert.Error(t, err, "not exactly 1 issue found for instance ID 11037, found 0") }) @@ -498,7 +498,7 @@ func TestIntegrateAuditData(t *testing.T) { ruleProp := *new(format.SarifProperties) project := models.Project{} projectVersion := models.ProjectVersion{ID: 11037} - err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, &projectVersion, nil, filterSet, true) + err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, &projectVersion, nil, filterSet, true, 5) assert.NoError(t, err, "error") assert.Equal(t, ruleProp.Audited, true) assert.Equal(t, ruleProp.ToolState, "Exploitable") @@ -509,4 +509,21 @@ func TestIntegrateAuditData(t *testing.T) { assert.Equal(t, ruleProp.FortifyCategory, "Audit All") }) + t.Run("Max retries set to 0: error raised", func(t *testing.T) { + ruleProp := *new(format.SarifProperties) + project := models.Project{} + projectVersion := models.ProjectVersion{ID: 11037} + auditData, _ := sys.GetAllIssueDetails(11037) + err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, &projectVersion, auditData, filterSet, false, 0) + assert.Error(t, err, "request failed: maximum number of retries reached, placeholder values will be set from now on for audit data") + }) + + t.Run("Max retries set to -1: fail silently", func(t *testing.T) { + ruleProp := *new(format.SarifProperties) + project := models.Project{} + projectVersion := models.ProjectVersion{ID: 11037} + auditData, _ := sys.GetAllIssueDetails(11037) + err := integrateAuditData(&ruleProp, "DUMMYDUMMYDUMMY", sys, &project, &projectVersion, auditData, filterSet, false, -1) + assert.NoError(t, err) + }) }