Skip to content

Commit

Permalink
Do not reply that a package has an unfixed vulnerability when in fact…
Browse files Browse the repository at this point in the history
… 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
  • Loading branch information
jhrozek committed Sep 18, 2024
1 parent b7f908e commit de7ca83
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 1 deletion.
15 changes: 15 additions & 0 deletions internal/engine/eval/vulncheck/report.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,21 @@ const (
`
)

const (
maliciousVulnFoundTemplate = `Malicious vulnerability found for dependency <tt>{{.Name}}</tt>:
| 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 = `<h3>Summary of vulnerabilities found</h3>
Expand Down
42 changes: 41 additions & 1 deletion internal/engine/eval/vulncheck/review.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand All @@ -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)
Expand Down

0 comments on commit de7ca83

Please sign in to comment.