diff --git a/cmd/osv-scanner/__snapshots__/fix_test.snap b/cmd/osv-scanner/__snapshots__/fix_test.snap index 96f2fb1a73..11c486e4b9 100755 --- a/cmd/osv-scanner/__snapshots__/fix_test.snap +++ b/cmd/osv-scanner/__snapshots__/fix_test.snap @@ -1742,6 +1742,72 @@ Warning: `fix` exists as both a subcommand of OSV-Scanner and as a file on the f --- +[TestRun_Fix/fix_non-interactive_override_pom.xml - 1] +Resolving /pom.xml... +Found 11 vulnerabilities matching the filter +Can fix 11/11 matching vulnerabilities by overriding 4 dependencies +OVERRIDE-PACKAGE: org.apache.httpcomponents:httpclient,4.5.13 +OVERRIDE-PACKAGE: org.codehaus.plexus:plexus-utils,3.0.24 +OVERRIDE-PACKAGE: org.jsoup:jsoup,1.15.3 +OVERRIDE-PACKAGE: commons-io:commons-io,2.7 +REMAINING-VULNS: 0 +UNFIXABLE-VULNS: 0 +Rewriting /pom.xml... + +--- + +[TestRun_Fix/fix_non-interactive_override_pom.xml - 2] +Warning: `fix` exists as both a subcommand of OSV-Scanner and as a file on the filesystem. `fix` is assumed to be a subcommand here. If you intended for `fix` to be an argument to `fix`, you must specify `fix fix` in your command line. + +--- + +[TestRun_Fix/fix_non-interactive_override_pom.xml - 3] + + 4.0.0 + + dev.osv + osv-fix + 1 + + + 4.5.13 + + + + + + commons-io + commons-io + 2.7 + + + org.jsoup + jsoup + 1.15.3 + + + org.apache.httpcomponents + httpclient + ${httpclient.version} + + + + + + + org.apache.maven.wagon + wagon-http + 3.0.0 + + + org.codehaus.plexus + plexus-utils + 3.0.24 + + + +--- + [TestRun_Fix/fix_non-interactive_relock_package.json - 1] Resolving /package.json... Found 5 vulnerabilities matching the filter diff --git a/cmd/osv-scanner/fix/fixtures/override-maven/pom.xml b/cmd/osv-scanner/fix/fixtures/override-maven/pom.xml new file mode 100644 index 0000000000..716883282e --- /dev/null +++ b/cmd/osv-scanner/fix/fixtures/override-maven/pom.xml @@ -0,0 +1,39 @@ + + 4.0.0 + + dev.osv + osv-fix + 1 + + + 4.0 + + + + + + org.jsoup + jsoup + 1.14.1 + + + org.apache.httpcomponents + httpclient + ${httpclient.version} + + + + + + + org.apache.maven.wagon + wagon-http + 3.0.0 + + + org.codehaus.plexus + plexus-utils + 3.0 + + + \ No newline at end of file diff --git a/cmd/osv-scanner/fix/interactive.go b/cmd/osv-scanner/fix/interactive.go index 5409c46d6d..4fe208bdbb 100644 --- a/cmd/osv-scanner/fix/interactive.go +++ b/cmd/osv-scanner/fix/interactive.go @@ -13,6 +13,10 @@ import ( func interactiveMode(ctx context.Context, opts osvFixOptions) error { if !remediation.SupportsRelax(opts.ManifestRW) && !remediation.SupportsInPlace(opts.LockfileRW) { + if remediation.SupportsOverride(opts.ManifestRW) { + return errors.New("override strategy is not supported in interactive mode, please use --non-interactive") + } + return errors.New("no supported remediation strategies found") } diff --git a/cmd/osv-scanner/fix/main.go b/cmd/osv-scanner/fix/main.go index d6788c0842..8813d31b41 100644 --- a/cmd/osv-scanner/fix/main.go +++ b/cmd/osv-scanner/fix/main.go @@ -76,8 +76,7 @@ func Command(stdout, stderr io.Writer, r *reporter.Reporter) *cli.Command { &cli.StringFlag{ Category: autoModeCategory, Name: "strategy", - Usage: "remediation approach to use; value can be: in-place, relock", - Value: "relock", + Usage: "remediation approach to use; value can be: in-place, relock, override", Action: func(ctx *cli.Context, s string) error { if !ctx.Bool("non-interactive") { // This flag isn't used in interactive mode @@ -92,6 +91,10 @@ func Command(stdout, stderr io.Writer, r *reporter.Reporter) *cli.Command { if !ctx.IsSet("manifest") { return errors.New("relock strategy requires manifest file") } + case "override": + if !ctx.IsSet("manifest") { + return errors.New("override strategy requires manifest file") + } default: return fmt.Errorf("unsupported strategy \"%s\" - must be one of: in-place, relock", s) } @@ -157,11 +160,6 @@ func Command(stdout, stderr io.Writer, r *reporter.Reporter) *cli.Command { } func action(ctx *cli.Context, stdout, stderr io.Writer) (reporter.Reporter, error) { - // The Action on strategy isn't run when using the default values. Check if the manifest is set. - if ctx.Bool("non-interactive") && ctx.String("strategy") == "relock" && !ctx.IsSet("manifest") { - return nil, errors.New("relock strategy requires manifest file") - } - if !ctx.IsSet("manifest") && !ctx.IsSet("lockfile") { return nil, errors.New("manifest or lockfile is required") } @@ -232,11 +230,29 @@ func action(ctx *cli.Context, stdout, stderr io.Writer) (reporter.Reporter, erro r := reporter.NewTableReporter(stdout, stderr, reporter.InfoLevel, false, 0) maxUpgrades := ctx.Int("apply-top") - switch ctx.String("strategy") { + strategy := ctx.String("strategy") + + if !ctx.IsSet("strategy") { + // Choose a default strategy based on the manifest/lockfile provided. + switch { + case remediation.SupportsRelax(opts.ManifestRW): + strategy = "relock" + case remediation.SupportsOverride(opts.ManifestRW): + strategy = "override" + case remediation.SupportsInPlace(opts.LockfileRW): + strategy = "in-place" + default: + return nil, errors.New("no supported remediation strategies for manifest/lockfile") + } + } + + switch strategy { case "relock": return r, autoRelock(ctx.Context, r, opts, maxUpgrades) case "in-place": return r, autoInPlace(ctx.Context, r, opts, maxUpgrades) + case "override": + return r, autoOverride(ctx.Context, r, opts, maxUpgrades) default: // The strategy flag should already be validated by this point. panic(fmt.Sprintf("non-interactive mode attempted to run with unhandled strategy: \"%s\"", ctx.String("strategy"))) diff --git a/cmd/osv-scanner/fix/noninteractive.go b/cmd/osv-scanner/fix/noninteractive.go index 73f24d7e43..6b7e9b1818 100644 --- a/cmd/osv-scanner/fix/noninteractive.go +++ b/cmd/osv-scanner/fix/noninteractive.go @@ -247,6 +247,117 @@ func relockUnfixableVulns(diffs []resolution.ResolutionDiff) []*resolution.Resol return unfixable } +func autoOverride(ctx context.Context, r reporter.Reporter, opts osvFixOptions, maxUpgrades int) error { + if !remediation.SupportsOverride(opts.ManifestRW) { + return errors.New("override strategy is not supported for manifest") + } + + r.Infof("Resolving %s...\n", opts.Manifest) + f, err := lockfile.OpenLocalDepFile(opts.Manifest) + if err != nil { + return err + } + + manif, err := opts.ManifestRW.Read(f) + f.Close() + if err != nil { + return err + } + + opts.Client.PreFetch(ctx, manif.Requirements, manif.FilePath) + res, err := resolution.Resolve(ctx, opts.Client, manif) + if err != nil { + return err + } + + if errs := res.Errors(); len(errs) > 0 { + r.Warnf("WARNING: encountered %d errors during dependency resolution:\n", len(errs)) + r.Warnf(resolutionErrorString(res, errs)) + } + + res.FilterVulns(opts.MatchVuln) + // TODO: count vulnerabilities per unique version as scan action does + totalVulns := len(res.Vulns) + r.Infof("Found %d vulnerabilities matching the filter\n", totalVulns) + + allPatches, err := remediation.ComputeOverridePatches(ctx, opts.Client, res, opts.RemediationOptions) + if err != nil { + return err + } + + if err := opts.Client.WriteCache(manif.FilePath); err != nil { + r.Warnf("WARNING: failed to write resolution cache: %v\n", err) + } + + if len(allPatches) == 0 { + r.Infof("No dependency patches are possible\n") + r.Infof("REMAINING-VULNS: %d\n", totalVulns) + r.Infof("UNFIXABLE-VULNS: %d\n", totalVulns) + + return nil + } + + depPatches, nFixed := autoChooseOverridePatches(allPatches, maxUpgrades) + nUnfixable := len(relockUnfixableVulns(allPatches)) + r.Infof("Can fix %d/%d matching vulnerabilities by overriding %d dependencies\n", nFixed, totalVulns, len(depPatches)) + for _, p := range depPatches { + r.Infof("OVERRIDE-PACKAGE: %s,%s\n", p.Pkg.Name, p.NewRequire) + } + r.Infof("REMAINING-VULNS: %d\n", totalVulns-nFixed) + r.Infof("UNFIXABLE-VULNS: %d\n", nUnfixable) + // TODO: Print the FIXED-VULN-IDS, REMAINING-VULN-IDS, UNFIXABLE-VULN-IDS + // TODO: Consider potentially introduced vulnerabilities + + r.Infof("Rewriting %s...\n", opts.Manifest) + if err := manifest.Overwrite(opts.ManifestRW, opts.Manifest, manifest.ManifestPatch{Manifest: &manif, Deps: depPatches}); err != nil { + return err + } + + return nil +} + +func autoChooseOverridePatches(diffs []resolution.ResolutionDiff, maxUpgrades int) ([]manifest.DependencyPatch, int) { + if maxUpgrades == 0 { + return nil, 0 + } + + var patches []manifest.DependencyPatch + pkgChanged := make(map[resolve.PackageKey]bool) // dependencies we've already applied a patch to + fixedVulns := make(map[string]bool) // vulns that have already been fixed by a patch + for _, diff := range diffs { + // If this patch is incompatible with existing patches, skip adding it to the patch list. + + // A patch is incompatible if any of its changed packages have already been changed by an existing patch. + if slices.ContainsFunc(diff.Deps, func(dp manifest.DependencyPatch) bool { return pkgChanged[dp.Pkg] }) { + continue + } + // A patch is also incompatible if any fixed vulnerability has already been fixed by another patch. + // This would happen if updating the version of one package has a side effect of also updating or removing one of its vulnerable dependencies. + // e.g. We have {foo@1 -> bar@1}, and two possible patches [foo@3, bar@2]. + // Patching foo@3 makes {foo@3 -> bar@3}, which also fixes the vulnerability in bar. + // Applying both patches would force {foo@3 -> bar@2}, which is less desirable. + if slices.ContainsFunc(diff.RemovedVulns, func(rv resolution.ResolutionVuln) bool { return fixedVulns[rv.Vulnerability.ID] }) { + continue + } + + // Add all individual package patches to the final patch list, and track the vulns this is anticipated to fix. + for _, dp := range diff.Deps { + patches = append(patches, dp) + pkgChanged[dp.Pkg] = true + } + for _, rv := range diff.RemovedVulns { + fixedVulns[rv.Vulnerability.ID] = true + } + + maxUpgrades-- + if maxUpgrades == 0 { + break + } + } + + return patches, len(fixedVulns) +} + func resolutionErrorString(res *resolution.ResolutionResult, errs []resolution.ResolutionError) string { // we pass in the []ResolutionErrors because calling res.Errors() is costly s := strings.Builder{} diff --git a/cmd/osv-scanner/fix_test.go b/cmd/osv-scanner/fix_test.go index eef78a9191..e8b4289f1c 100644 --- a/cmd/osv-scanner/fix_test.go +++ b/cmd/osv-scanner/fix_test.go @@ -54,6 +54,12 @@ func TestRun_Fix(t *testing.T) { exit: 0, manifest: "./fix/fixtures/relock-npm/package.json", }, + { + name: "fix non-interactive override pom.xml", + args: []string{"", "fix", "--non-interactive", "--strategy=override"}, + exit: 0, + manifest: "./fix/fixtures/override-maven/pom.xml", + }, // TODO: add tests with the cli flags } for _, tt := range tests { diff --git a/internal/remediation/override.go b/internal/remediation/override.go index 9cedc63974..fd41d40439 100644 --- a/internal/remediation/override.go +++ b/internal/remediation/override.go @@ -68,8 +68,10 @@ func ComputeOverridePatches(ctx context.Context, cl client.ResolutionClient, res } if res.err != nil { - // TODO: stop goroutines - return nil, res.err + // Resolution errors seem to happen when a package/version cannot be found, which isn't uncommon. + // Just silently skip for now, treating it the same as unfixable. + // TODO: Log the error somehow. + continue } diff := result.CalculateDiff(res.result) diff --git a/internal/resolution/manifest/maven.go b/internal/resolution/manifest/maven.go index 6e18ec0b52..017bc78291 100644 --- a/internal/resolution/manifest/maven.go +++ b/internal/resolution/manifest/maven.go @@ -312,6 +312,11 @@ func (m MavenDependencyPatches) addPatch(changedDep DependencyPatch, fromBase bo return fmt.Errorf("MavenDepTypeToDependency: %w", err) } + // If this dependency did not already exist in the base project, we want to add it to the dependencyManagement section + if !fromBase { + o = manifest.OriginManagement + } + substrings := strings.Split(changedDep.Pkg.Name, ":") if len(substrings) != 2 { return fmt.Errorf("invalid Maven name: %s", changedDep.Pkg.Name)