Skip to content

Commit

Permalink
internal/scan, vulncheck: use packages.load for mod info
Browse files Browse the repository at this point in the history
Govulncheck previously used go list to get mod info, which does not work
in modules with a vendor directory. Therefore, module information needs
to be extracted from package information instead.

There is one change to the behavior of govulncheck ran in module mode in
a certain edge case: if one runs govulncheck with the ./... package
pattern in a subdirectory of a module, govulncheck will only show the
vulnerabilities affecting that subdirectory as opposed to the entire
module. This does not affect govulncheck default behavior nor the
behavior of govulncheck when ran from the root of a module at any scan
level.

Fixes golang/go#65124

Change-Id: Ie3b0cb0b9486fb94efeb05ee0c76d19c9f595877
Reviewed-on: https://go-review.googlesource.com/c/vuln/+/557495
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Zvonimir Pavlinovic <zpavlinovic@google.com>
  • Loading branch information
Maceo Thompson committed Jan 22, 2024
1 parent 0047a16 commit 3072335
Show file tree
Hide file tree
Showing 10 changed files with 63 additions and 133 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
# Test of source mode on a module with a replace directive.

$ govulncheck -C ${moddir}/replace ./... --> FAIL 3
Scanning your code and P packages across M dependent module for known vulnerabilities...
Scanning your code and P packages across M dependent modules for known vulnerabilities...

Vulnerability #1: GO-2021-0113
Due to improper index calculation, an incorrectly formatted language tag can
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ Vulnerability #1: GO-2021-0113

=== Informational ===

There are 4 vulnerabilities in modules that you require that are
There are 2 vulnerabilities in modules that you require that are
neither imported nor called. You may not need to take any action.
See https://pkg.go.dev/golang.org/x/vuln/cmd/govulncheck for details.

Expand All @@ -30,24 +30,7 @@ Vulnerability #1: GO-2022-0969
Found in: net/http@go1.18
Fixed in: net/http@go1.18.6

Vulnerability #2: GO-2021-0265
A maliciously crafted path can cause Get and other query functions to
consume excessive amounts of CPU and time.
More info: https://pkg.go.dev/vuln/GO-2021-0265
Module: github.com/tidwall/gjson
Found in: github.com/tidwall/gjson@v1.6.5
Fixed in: github.com/tidwall/gjson@v1.9.3

Vulnerability #3: GO-2021-0054
Due to improper bounds checking, maliciously crafted JSON objects can cause
an out-of-bounds panic. If parsing user input, this may be used as a denial
of service vector.
More info: https://pkg.go.dev/vuln/GO-2021-0054
Module: github.com/tidwall/gjson
Found in: github.com/tidwall/gjson@v1.6.5
Fixed in: github.com/tidwall/gjson@v1.6.6

Vulnerability #4: GO-2020-0015
Vulnerability #2: GO-2020-0015
An attacker could provide a single byte to a UTF16 decoder instantiated with
UseBOM or ExpectBOM to trigger an infinite loop if the String function on
the Decoder is called, or the Decoder is passed to transform.String. If used
Expand Down Expand Up @@ -83,7 +66,7 @@ Vulnerability #1: GO-2021-0113

=== Informational ===

There are 4 vulnerabilities in modules that you require that are
There are 2 vulnerabilities in modules that you require that are
neither imported nor called. You may not need to take any action.
See https://pkg.go.dev/golang.org/x/vuln/cmd/govulncheck for details.

Expand All @@ -96,24 +79,7 @@ Vulnerability #1: GO-2022-0969
Found in: net/http@go1.18
Fixed in: net/http@go1.18.6

Vulnerability #2: GO-2021-0265
A maliciously crafted path can cause Get and other query functions to
consume excessive amounts of CPU and time.
More info: https://pkg.go.dev/vuln/GO-2021-0265
Module: github.com/tidwall/gjson
Found in: github.com/tidwall/gjson@v1.6.5
Fixed in: github.com/tidwall/gjson@v1.9.3

Vulnerability #3: GO-2021-0054
Due to improper bounds checking, maliciously crafted JSON objects can cause
an out-of-bounds panic. If parsing user input, this may be used as a denial
of service vector.
More info: https://pkg.go.dev/vuln/GO-2021-0054
Module: github.com/tidwall/gjson
Found in: github.com/tidwall/gjson@v1.6.5
Fixed in: github.com/tidwall/gjson@v1.6.6

Vulnerability #4: GO-2020-0015
Vulnerability #2: GO-2020-0015
An attacker could provide a single byte to a UTF16 decoder instantiated with
UseBOM or ExpectBOM to trigger an infinite loop if the String function on
the Decoder is called, or the Decoder is passed to transform.String. If used
Expand Down
17 changes: 5 additions & 12 deletions internal/scan/source.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,36 +30,29 @@ func runSource(ctx context.Context, handler govulncheck.Handler, cfg *config, cl
return errNoGoMod
}
var pkgs []*packages.Package
var mods []*packages.Module
graph := vulncheck.NewPackageGraph(cfg.GoVersion)
pkgConfig := &packages.Config{
Dir: dir,
Tests: cfg.test,
Env: cfg.env,
}
mods, err := graph.LoadModules(pkgConfig)
pkgs, mods, err = graph.LoadPackagesAndMods(pkgConfig, cfg.tags, cfg.patterns)
if err != nil {
if isGoVersionMismatchError(err) {
return fmt.Errorf("%v\n\n%v", errGoVersionMismatch, err)
}
return fmt.Errorf("loading modules: %w", err)
}
if cfg.ScanLevel.WantPackages() {
pkgs, err = graph.LoadPackages(pkgConfig, cfg.tags, cfg.patterns)
if err != nil {
if isGoVersionMismatchError(err) {
return fmt.Errorf("%v\n\n%v", errGoVersionMismatch, err)
}
return fmt.Errorf("loading packages: %w", err)
}
return fmt.Errorf("loading packages: %w", err)
}

if err := handler.Progress(sourceProgressMessage(pkgs, len(mods)-1, cfg.ScanLevel)); err != nil {
return err
}

if cfg.ScanLevel.WantPackages() && len(pkgs) == 0 {
return nil // early exit
}
return vulncheck.Source(ctx, handler, pkgs, &cfg.Config, client, graph)
return vulncheck.Source(ctx, handler, pkgs, mods, &cfg.Config, client, graph)
}

// sourceProgressMessage returns a string of the form
Expand Down
81 changes: 36 additions & 45 deletions internal/vulncheck/packages.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,7 @@
package vulncheck

import (
"bytes"
"encoding/json"
"errors"
"fmt"
"os/exec"
"path/filepath"
"strings"

"golang.org/x/tools/go/packages"
Expand All @@ -37,43 +32,6 @@ func NewPackageGraph(goVersion string) *PackageGraph {
return graph
}

func (g *PackageGraph) LoadModules(cfg *packages.Config) (mods []*packages.Module, err error) {
cmd := exec.Command("go", "list", "-m", "-json")
// Quick fix for go.dev/issue/65155
// TODO: Fix go.dev/issue/65124
// This check makes it so that govulncheck doesn't crash if running on a
// vendored module from the root of a module. Essentially only here so that
// the vendor test doesn't fail until #65124 is fixed.
if fileExists(filepath.Join(cfg.Dir, "vendor")) {
cmd.Args = append(cmd.Args, "-mod=readonly")
}

cmd.Args = append(cmd.Args, "all")
cmd.Env = append(cmd.Env, cfg.Env...)
cmd.Dir = cfg.Dir
out, err := cmd.Output()
if err != nil {
if ee := (*exec.ExitError)(nil); errors.As(err, &ee) && len(ee.Stderr) > 0 {
return nil, fmt.Errorf("%v: %v\n%s", cmd, err, ee.Stderr)
}
return nil, fmt.Errorf("%v: %v", cmd, err)
}

dec := json.NewDecoder(bytes.NewReader(out))
for dec.More() {
var m *packages.Module
err = dec.Decode(&m)
if err != nil {
if err != nil {
return nil, fmt.Errorf("decoding output of %v: %v", cmd, err)
}
}
mods = append(mods, m)
}
g.AddModules(mods...)
return mods, nil
}

// AddModules adds the modules and any replace modules provided.
// It will ignore modules that have duplicate paths to ones the graph already holds.
func (g *PackageGraph) AddModules(mods ...*packages.Module) {
Expand Down Expand Up @@ -158,7 +116,7 @@ func (g *PackageGraph) GetPackage(path string) *packages.Package {

// LoadPackages loads the packages specified by the patterns into the graph.
// See golang.org/x/tools/go/packages.Load for details of how it works.
func (g *PackageGraph) LoadPackages(cfg *packages.Config, tags []string, patterns []string) ([]*packages.Package, error) {
func (g *PackageGraph) LoadPackagesAndMods(cfg *packages.Config, tags []string, patterns []string) ([]*packages.Package, []*packages.Module, error) {
if len(tags) > 0 {
cfg.BuildFlags = []string{fmt.Sprintf("-tags=%s", strings.Join(tags, ","))}
}
Expand All @@ -173,7 +131,7 @@ func (g *PackageGraph) LoadPackages(cfg *packages.Config, tags []string, pattern

pkgs, err := packages.Load(cfg, patterns...)
if err != nil {
return nil, err
return nil, nil, err
}
var perrs []packages.Error
packages.Visit(pkgs, nil, func(p *packages.Package) {
Expand All @@ -183,7 +141,40 @@ func (g *PackageGraph) LoadPackages(cfg *packages.Config, tags []string, pattern
err = &packageError{perrs}
}
g.AddPackages(pkgs...)
return pkgs, err
return pkgs, extractModules(pkgs), err
}

// extractModules collects modules in `pkgs` up to uniqueness of
// module path and version.
func extractModules(pkgs []*packages.Package) []*packages.Module {
modMap := map[string]*packages.Module{}
seen := map[*packages.Package]bool{}
var extract func(*packages.Package, map[string]*packages.Module)
extract = func(pkg *packages.Package, modMap map[string]*packages.Module) {
if pkg == nil || seen[pkg] {
return
}
if pkg.Module != nil {
if pkg.Module.Replace != nil {
modMap[pkg.Module.Replace.Path] = pkg.Module
} else {
modMap[pkg.Module.Path] = pkg.Module
}
}
seen[pkg] = true
for _, imp := range pkg.Imports {
extract(imp, modMap)
}
}
for _, pkg := range pkgs {
extract(pkg, modMap)
}

modules := []*packages.Module{}
for _, mod := range modMap {
modules = append(modules, mod)
}
return modules
}

// packageError contains errors from loading a set of packages.
Expand Down
2 changes: 1 addition & 1 deletion internal/vulncheck/slicing_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func Do(i I, input string) {
})

graph := NewPackageGraph("go1.18")
pkgs, err := graph.LoadPackages(e.Config, nil, []string{path.Join(e.Temp(), "/module/slice")})
pkgs, _, err := graph.LoadPackagesAndMods(e.Config, nil, []string{path.Join(e.Temp(), "/module/slice")})
if err != nil {
t.Fatal(err)
}
Expand Down
10 changes: 3 additions & 7 deletions internal/vulncheck/source.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ import (
)

// Source detects vulnerabilities in pkgs and emits the findings to handler.
func Source(ctx context.Context, handler govulncheck.Handler, pkgs []*packages.Package, cfg *govulncheck.Config, client *client.Client, graph *PackageGraph) error {
vr, err := source(ctx, handler, pkgs, cfg, client, graph)
func Source(ctx context.Context, handler govulncheck.Handler, pkgs []*packages.Package, mods []*packages.Module, cfg *govulncheck.Config, client *client.Client, graph *PackageGraph) error {
vr, err := source(ctx, handler, pkgs, mods, cfg, client, graph)
if err != nil {
return err
}
Expand All @@ -33,7 +33,7 @@ func Source(ctx context.Context, handler govulncheck.Handler, pkgs []*packages.P
// and produces a Result that contains info on detected vulnerabilities.
//
// Assumes that pkgs are non-empty and belong to the same program.
func source(ctx context.Context, handler govulncheck.Handler, pkgs []*packages.Package, cfg *govulncheck.Config, client *client.Client, graph *PackageGraph) (*Result, error) {
func source(ctx context.Context, handler govulncheck.Handler, pkgs []*packages.Package, mods []*packages.Module, cfg *govulncheck.Config, client *client.Client, graph *PackageGraph) (*Result, error) {
ctx, cancel := context.WithCancel(ctx)
defer cancel()

Expand All @@ -57,10 +57,6 @@ func source(ctx context.Context, handler govulncheck.Handler, pkgs []*packages.P
}()
}

var mods []*packages.Module
for _, m := range graph.modules {
mods = append(mods, m)
}
mv, err := FetchVulnerabilities(ctx, client, mods)
if err != nil {
return nil, err
Expand Down
20 changes: 10 additions & 10 deletions internal/vulncheck/source_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ func TestCalls(t *testing.T) {

// Load x and y as entry packages.
graph := NewPackageGraph("go1.18")
pkgs, err := graph.LoadPackages(e.Config, nil, []string{path.Join(e.Temp(), "entry/x"), path.Join(e.Temp(), "entry/y")})
pkgs, mods, err := graph.LoadPackagesAndMods(e.Config, nil, []string{path.Join(e.Temp(), "entry/x"), path.Join(e.Temp(), "entry/y")})
if err != nil {
t.Fatal(err)
}
Expand All @@ -205,7 +205,7 @@ func TestCalls(t *testing.T) {
}

cfg := &govulncheck.Config{ScanLevel: "symbol"}
result, err := source(context.Background(), test.NewMockHandler(), pkgs, cfg, c, graph)
result, err := source(context.Background(), test.NewMockHandler(), pkgs, mods, cfg, c, graph)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -298,7 +298,7 @@ func TestAllSymbolsVulnerable(t *testing.T) {

// Load x as entry package.
graph := NewPackageGraph("go1.18")
pkgs, err := graph.LoadPackages(e.Config, nil, []string{path.Join(e.Temp(), "entry/x")})
pkgs, mods, err := graph.LoadPackagesAndMods(e.Config, nil, []string{path.Join(e.Temp(), "entry/x")})
if err != nil {
t.Fatal(err)
}
Expand All @@ -307,7 +307,7 @@ func TestAllSymbolsVulnerable(t *testing.T) {
}

cfg := &govulncheck.Config{ScanLevel: "symbol"}
result, err := source(context.Background(), test.NewMockHandler(), pkgs, cfg, client, graph)
result, err := source(context.Background(), test.NewMockHandler(), pkgs, mods, cfg, client, graph)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -363,7 +363,7 @@ func TestNoSyntheticNodes(t *testing.T) {

// Load x as entry package.
graph := NewPackageGraph("go1.18")
pkgs, err := graph.LoadPackages(e.Config, nil, []string{path.Join(e.Temp(), "entry/x")})
pkgs, mods, err := graph.LoadPackagesAndMods(e.Config, nil, []string{path.Join(e.Temp(), "entry/x")})
if err != nil {
t.Fatal(err)
}
Expand All @@ -377,7 +377,7 @@ func TestNoSyntheticNodes(t *testing.T) {
}

cfg := &govulncheck.Config{ScanLevel: "symbol"}
result, err := source(context.Background(), test.NewMockHandler(), pkgs, cfg, c, graph)
result, err := source(context.Background(), test.NewMockHandler(), pkgs, mods, cfg, c, graph)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -443,7 +443,7 @@ func TestRecursion(t *testing.T) {

// Load x as entry package.
graph := NewPackageGraph("go1.18")
pkgs, err := graph.LoadPackages(e.Config, nil, []string{path.Join(e.Temp(), "entry/x")})
pkgs, mods, err := graph.LoadPackagesAndMods(e.Config, nil, []string{path.Join(e.Temp(), "entry/x")})
if err != nil {
t.Fatal(err)
}
Expand All @@ -457,7 +457,7 @@ func TestRecursion(t *testing.T) {
}

cfg := &govulncheck.Config{ScanLevel: "symbol"}
result, err := source(context.Background(), test.NewMockHandler(), pkgs, cfg, c, graph)
result, err := source(context.Background(), test.NewMockHandler(), pkgs, mods, cfg, c, graph)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -508,7 +508,7 @@ func TestIssue57174(t *testing.T) {

// Load x as entry package.
graph := NewPackageGraph("go1.18")
pkgs, err := graph.LoadPackages(e.Config, nil, []string{path.Join(e.Temp(), "entry/x")})
pkgs, mods, err := graph.LoadPackagesAndMods(e.Config, nil, []string{path.Join(e.Temp(), "entry/x")})
if err != nil {
t.Fatal(err)
}
Expand All @@ -522,7 +522,7 @@ func TestIssue57174(t *testing.T) {
}

cfg := &govulncheck.Config{ScanLevel: "symbol"}
_, err = source(context.Background(), test.NewMockHandler(), pkgs, cfg, c, graph)
_, err = source(context.Background(), test.NewMockHandler(), pkgs, mods, cfg, c, graph)
if err != nil {
t.Fatal(err)
}
Expand Down
16 changes: 0 additions & 16 deletions internal/vulncheck/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,8 @@ package vulncheck
import (
"bytes"
"context"
"errors"
"go/token"
"go/types"
"os"
"sort"
"strings"

Expand Down Expand Up @@ -359,17 +357,3 @@ func modVersion(mod *packages.Module) string {
}
return mod.Version
}

// fileExists checks if file path exists. Returns true
// if the file exists or it cannot prove that it does
// not exist. Otherwise, returns false.
func fileExists(path string) bool {
if _, err := os.Stat(path); err == nil {
return true
} else if errors.Is(err, os.ErrNotExist) {
return false
}
// Conservatively return true if os.Stat fails
// for some other reason.
return true
}
Loading

0 comments on commit 3072335

Please sign in to comment.