From de7ca83b7a023b17c608addb22c52ab1c10a2727 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Wed, 18 Sep 2024 14:01:25 +0200 Subject: [PATCH] Do not reply that a package has an unfixed vulnerability when in fact it is malicious Malicious packages that have a vulnerability entry `MAL-` are in fact malicious. Our OSV evaluator handled the `MAL-` vulnerabilities the same as all the others which meant that it would just reply with "A vulnerability was found, but no fixed version exists yet". A malicious package is unlikely to not be malicious again, so let's put a sterner warning including a link to the vulnerability into the reply. Fixes: #4528 --- internal/engine/eval/vulncheck/report.go | 15 +++++++++ internal/engine/eval/vulncheck/review.go | 42 +++++++++++++++++++++++- 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/internal/engine/eval/vulncheck/report.go b/internal/engine/eval/vulncheck/report.go index 442a97f88c..c8f3e13784 100644 --- a/internal/engine/eval/vulncheck/report.go +++ b/internal/engine/eval/vulncheck/report.go @@ -80,6 +80,21 @@ const ( ` ) +const ( + maliciousVulnFoundTemplate = `Malicious vulnerability found for dependency {{.Name}}: + +| ID | Summary | Details | +|----|---------|---------| +{{- range .Vulns}} +| [{{.ID}}](https://osv.dev/vulnerability/{{.ID}}) | {{.Summary}} | {{.Details}} | +{{- end}} + +Please review and remove this dependency immediately.` + + maliciousVulnFoundFallbackFmt = `Malicious vulnerability found for dependency %s. +Please review and remove this dependency immediately.", dep.Dep.Name)` +) + const ( tableVulnerabilitiesHeaderName = "vulnerabilitiesTableHeader" tableVulnerabilitiesHeader = `

Summary of vulnerabilities found

diff --git a/internal/engine/eval/vulncheck/review.go b/internal/engine/eval/vulncheck/review.go index d101a9806d..e73f53bd45 100644 --- a/internal/engine/eval/vulncheck/review.go +++ b/internal/engine/eval/vulncheck/review.go @@ -16,11 +16,13 @@ package vulncheck import ( + "bytes" "context" "errors" "fmt" "io" "strings" + "text/template" "github.com/google/go-github/v63/github" "github.com/rs/zerolog" @@ -186,6 +188,41 @@ func newReviewPrHandler( return handler, nil } +func getMaliciousVulns(vulns []Vulnerability) []Vulnerability { + var malicious []Vulnerability + for _, vuln := range vulns { + if strings.HasPrefix(vuln.ID, "MAL-") { + malicious = append(malicious, vuln) + } + } + return malicious +} + +func handleMaliciousVulns(dep *pbinternal.PrDependencies_ContextualDependency, vulns []Vulnerability) string { + maliciousVulns := getMaliciousVulns(vulns) + if len(maliciousVulns) == 0 { + return "" + } + + tmpl, err := template.New("maliciousVuln").Parse(maliciousVulnFoundTemplate) + if err != nil { + return fmt.Sprintf(maliciousVulnFoundFallbackFmt, dep.Dep.Name) + } + var buf bytes.Buffer + err = tmpl.Execute(&buf, struct { + Name string + Vulns []Vulnerability + }{ + Name: dep.Dep.Name, + Vulns: maliciousVulns, + }) + if err != nil { + return fmt.Sprintf(maliciousVulnFoundFallbackFmt, dep.Dep.Name) + } + + return buf.String() +} + func (ra *reviewPrHandler) trackVulnerableDep( ctx context.Context, dep *pbinternal.PrDependencies_ContextualDependency, @@ -204,7 +241,10 @@ func (ra *reviewPrHandler) trackVulnerableDep( case errors.Is(patch.GetFormatterMeta().pkgRegistryLookupError, ErrPkgNotFound): body = pkgRepoInfoNotFound case patch.GetFormatterMeta().pkgRegistryLookupError == nil: - if !patch.HasPatchedVersion() { + maliciousBody := handleMaliciousVulns(dep, vulnResp.Vulns) + if maliciousBody != "" { + body = maliciousBody + } else if !patch.HasPatchedVersion() { body = fmt.Sprintf(vulnFoundWithNoPatchFmt, dep.Dep.Name) } else { comment := patch.IndentedString(location.leadingWhitespace, location.line, dep.Dep)