fix(dotnet): don't include non-runtime libraries into report for *.deps.json files
#7039
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,10 +22,11 @@ | |
| "Type": "dotnet-core", | ||
| "Packages": [ | ||
| { | ||
| "ID": "Newtonsoft.Json/9.0.1", | ||
| "Name": "Newtonsoft.Json", | ||
| "Identifier": { | ||
| "PURL": "pkg:nuget/[email protected]", | ||
| "UID": "e678401f5d07418a" | ||
| }, | ||
| "Version": "9.0.1", | ||
| "Layer": {}, | ||
|
|
@@ -40,10 +41,11 @@ | |
| "Vulnerabilities": [ | ||
| { | ||
| "VulnerabilityID": "GHSA-5crp-9r3c-p9vr", | ||
| "PkgID": "Newtonsoft.Json/9.0.1", | ||
| "PkgName": "Newtonsoft.Json", | ||
| "PkgIdentifier": { | ||
| "PURL": "pkg:nuget/[email protected]", | ||
| "UID": "e678401f5d07418a" | ||
| }, | ||
| "InstalledVersion": "9.0.1", | ||
| "FixedVersion": "13.0.1", | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,23 +2,51 @@ package core_deps | |||||
|
|
||||||
| import ( | ||||||
| "io" | ||||||
| "sort" | ||||||
| "strings" | ||||||
| "sync" | ||||||
|
|
||||||
| "github.com/liamg/jfather" | ||||||
| "github.com/samber/lo" | ||||||
| "golang.org/x/xerrors" | ||||||
|
|
||||||
| "github.com/aquasecurity/trivy/pkg/dependency" | ||||||
| ftypes "github.com/aquasecurity/trivy/pkg/fanal/types" | ||||||
| "github.com/aquasecurity/trivy/pkg/log" | ||||||
| xio "github.com/aquasecurity/trivy/pkg/x/io" | ||||||
| ) | ||||||
|
|
||||||
| type dotNetDependencies struct { | ||||||
| Libraries map[string]dotNetLibrary `json:"libraries"` | ||||||
| RuntimeTarget RuntimeTarget `json:"runtimeTarget"` | ||||||
| Targets map[string]map[string]Target `json:"targets"` | ||||||
| } | ||||||
|
|
||||||
| type dotNetLibrary struct { | ||||||
| Type string `json:"type"` | ||||||
| StartLine int | ||||||
| EndLine int | ||||||
| } | ||||||
|
|
||||||
| type RuntimeTarget struct { | ||||||
| Name string `json:"name"` | ||||||
| } | ||||||
|
|
||||||
| type Target struct { | ||||||
| Runtime any `json:"runtime"` | ||||||
| RuntimeTargets any `json:"runtimeTargets"` | ||||||
| Native any `json:"native"` | ||||||
| } | ||||||
|
|
||||||
| type Parser struct { | ||||||
| logger *log.Logger | ||||||
| once sync.Once | ||||||
| } | ||||||
|
|
||||||
| func NewParser() *Parser { | ||||||
| return &Parser{ | ||||||
| logger: log.WithPrefix("dotnet"), | ||||||
| once: sync.Once{}, | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -29,11 +57,14 @@ func (p *Parser) Parse(r xio.ReadSeekerAt) ([]ftypes.Package, []ftypes.Dependenc | |||||
| if err != nil { | ||||||
| return nil, nil, xerrors.Errorf("read error: %w", err) | ||||||
| } | ||||||
| if err = jfather.Unmarshal(input, &depsFile); err != nil { | ||||||
| return nil, nil, xerrors.Errorf("failed to decode .deps.json file: %w", err) | ||||||
| } | ||||||
|
|
||||||
| // Select target for RuntimeTarget | ||||||
| target := depsFile.Targets[depsFile.RuntimeTarget.Name] | ||||||
|
|
||||||
| var pkgs ftypes.Packages | ||||||
| for nameVer, lib := range depsFile.Libraries { | ||||||
| if !strings.EqualFold(lib.Type, "package") { | ||||||
| continue | ||||||
|
|
@@ -47,6 +78,7 @@ func (p *Parser) Parse(r xio.ReadSeekerAt) ([]ftypes.Package, []ftypes.Dependenc | |||||
| } | ||||||
|
|
||||||
| pkgs = append(pkgs, ftypes.Package{ | ||||||
| ID: dependency.ID(ftypes.DotNetCore, split[0], split[1]), | ||||||
| Name: split[0], | ||||||
| Version: split[1], | ||||||
| Locations: []ftypes.Location{ | ||||||
|
|
@@ -55,20 +87,36 @@ func (p *Parser) Parse(r xio.ReadSeekerAt) ([]ftypes.Package, []ftypes.Dependenc | |||||
| EndLine: lib.EndLine, | ||||||
| }, | ||||||
| }, | ||||||
| // We're still not sure that we need to skip libraries built into .NETCore (or that we detect them correctly). | ||||||
| // So we mark these libraries as Dev to skip the scan by default, but keep the options for displaying these libraries. | ||||||
| Dev: p.isLibraryBuiltIntoNetCore(target, depsFile.RuntimeTarget.Name, nameVer), | ||||||
| }) | ||||||
| } | ||||||
|
|
||||||
| sort.Sort(pkgs) | ||||||
| return pkgs, nil, nil | ||||||
| } | ||||||
|
|
||||||
| // isLibraryBuiltIntoNetCore returns true if library doesn't contain `runtime`, `runtimeTarget` and `native` sections. | ||||||
| // See https://github.com/aquasecurity/trivy/discussions/4282#discussioncomment-8830365 for more details. | ||||||
| func (p *Parser) isLibraryBuiltIntoNetCore(target map[string]Target, runtimeTargetName, library string) bool { | ||||||
| // `Target` for `RuntimeTarget.Name` not found | ||||||
| if target == nil { | ||||||
| p.once.Do(func() { | ||||||
| p.logger.Debug("Unable to find `Target` for Runtime Target Name. All dependencies from `libraries` section will be included in the report", log.String("RuntimeTarget", runtimeTargetName)) | ||||||
| }) | ||||||
| return false | ||||||
| } | ||||||
| lib, ok := target[library] | ||||||
| // Selected target doesn't contain library | ||||||
| if !ok { | ||||||
| p.once.Do(func() { | ||||||
| p.logger.Debug("Unable to determine that the library is built into .NET Core. Library not found in `Target` section.", log.String("RuntimeTarget", runtimeTargetName), log.String("Library", library)) | ||||||
| }) | ||||||
| return false | ||||||
| } | ||||||
| // Check that `runtime`, `runtimeTarget` and `native` sections are empty | ||||||
|
There was a problem hiding this comment. Looks to me like it checks if they are not empty.
Suggested change
There was a problem hiding this comment. fixed in afe12b4 |
||||||
| return lo.IsEmpty(lib) | ||||||
| } | ||||||
|
|
||||||
| // UnmarshalJSONWithMetadata needed to detect start and end lines of deps | ||||||
|
|
||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
File renamed without changes.
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| { | ||
|
There was a problem hiding this comment. Would it be possible to add another testcase that references some of the commonly reported false positives and proves that those aren't flagged? For example - a deps file listing a vulnerable version of This was created with This shows that all the old packages (some vulnerable) making up the NETStandard surface area are all excluded when targeting the latest frameworks which provide inbox support for all those package. There was a problem hiding this comment. Your test file is too large. |
||
| "runtimeTarget": { | ||
| "name": ".NETCoreApp,Version=v6.0", | ||
| "signature": "" | ||
| }, | ||
| "compilationOptions": {}, | ||
| "targets": { | ||
| ".NETCoreApp,Version=v6.0": { | ||
| "hello2/1.0.0": { | ||
| "dependencies": { | ||
| "JsonDiffPatch": "2.0.61" | ||
| }, | ||
| "runtime": { | ||
| "hello2.dll": {} | ||
| } | ||
| }, | ||
| "JsonDiffPatch/2.0.61": { | ||
| "dependencies": { | ||
| "Microsoft.NETCore.App": "1.1.2" | ||
| }, | ||
| "runtime": { | ||
| "lib/netcoreapp1.1/JsonDiffPatch.dll": { | ||
| "assemblyVersion": "1.0.0.0", | ||
| "fileVersion": "1.0.0.0" | ||
| } | ||
| } | ||
| }, | ||
| "Libuv/1.9.1": { | ||
| "dependencies": { | ||
| "Microsoft.NETCore.Platforms": "1.1.0" | ||
| }, | ||
| "runtimeTargets": { | ||
| "runtimes/debian-x64/native/libuv.so": { | ||
| "rid": "debian-x64", | ||
| "assetType": "native", | ||
| "fileVersion": "0.0.0.0" | ||
| }, | ||
| "runtimes/fedora-x64/native/libuv.so": { | ||
| "rid": "fedora-x64", | ||
| "assetType": "native", | ||
| "fileVersion": "0.0.0.0" | ||
| } | ||
| } | ||
| }, | ||
| "Microsoft.NETCore.App/1.1.2": { | ||
| "dependencies": { | ||
| "Libuv": "1.9.1", | ||
| "System.Collections.Immutable": "1.3.0" | ||
| } | ||
| }, | ||
| "Microsoft.NETCore.Platforms/1.1.0": {} | ||
| } | ||
| }, | ||
| "libraries": { | ||
| "hello2/1.0.0": { | ||
| "type": "project", | ||
| "serviceable": false, | ||
| "sha512": "" | ||
| }, | ||
| "JsonDiffPatch/2.0.61": { | ||
| "type": "package", | ||
| "serviceable": true, | ||
| "sha512": "sha512-nZ4QtcU3jR+CBT69qcJBvCcWi5uKgPRrrvSMm4V8Z76ljJ/MFo1P55qXk/nQY0q0WC4v94m5qH4SDhovFfci+Q==", | ||
| "path": "jsondiffpatch/2.0.61", | ||
| "hashPath": "jsondiffpatch.2.0.61.nupkg.sha512" | ||
| }, | ||
| "Libuv/1.9.1": { | ||
| "type": "package", | ||
| "serviceable": true, | ||
| "sha512": "sha512-uqX2Frwf9PW8MaY7PRNY6HM5BpW1D8oj1EdqzrmbEFD5nH63Yat3aEjN/tws6Tw6Fk7LwmLBvtUh32tTeTaHiA==", | ||
| "path": "libuv/1.9.1", | ||
| "hashPath": "libuv.1.9.1.nupkg.sha512" | ||
| }, | ||
| "Microsoft.NETCore.App/1.1.2": { | ||
| "type": "package", | ||
| "serviceable": true, | ||
| "sha512": "sha512-fcN0Ob6rjY7Zu0770cA5l9wRJvj7+ltJPPdryUidejkkhao+y2AYrtezBTlP9nCSFXLmYR9BtaknORT17x8reA==", | ||
| "path": "microsoft.netcore.app/1.1.2", | ||
| "hashPath": "microsoft.netcore.app.1.1.2.nupkg.sha512" | ||
| }, | ||
| "Microsoft.NETCore.Platforms/1.1.0": { | ||
| "type": "package", | ||
| "serviceable": true, | ||
| "sha512": "sha512-kz0PEW2lhqygehI/d6XsPCQzD7ff7gUJaVGPVETX611eadGsA3A877GdSlU0LRVMCTH/+P3o2iDTak+S08V2+A==", | ||
| "path": "microsoft.netcore.platforms/1.1.0", | ||
| "hashPath": "microsoft.netcore.platforms.1.1.0.nupkg.sha512" | ||
| }, | ||
| "System.Collections.Immutable/1.3.0": { | ||
| "type": "package", | ||
| "serviceable": true, | ||
| "sha512": "sha512-zukBRPUuNxwy9m4TGWLxKAnoiMc9+B+8VXeXVyPiBPvOd7yLgAlZ1DlsRWJjMx4VsvhhF2+6q6kO2GRbPja6hA==", | ||
| "path": "system.collections.immutable/1.3.0", | ||
| "hashPath": "system.collections.immutable.1.3.0.nupkg.sha512" | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Packages which don't contribute runtime assets to the application cannot contribute to a runtime vulnerability since nothing from the package persists at runtime. Runtime assets can be identified by examining the targets section since that's what the host uses to probe for those.
There are lots of ways packages might be referenced by an app and excluded from runtime.
The package might be referenced with
ExcludeAssets="Runtime", the package might have been superseded by the .NET runtime itself and excluded but the build, or the package might have overlapping assets with another package that is preferred. In all cases - if it's bits don't make it to the final app it cannot be a source of a vulnerability in that app.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in general I think the same way.
thank you for detailing this 👍
There are times when users want to see dependencies that are not used at runtime (we encountered this in nodejs).
That's why we've added options to allow users to see all dependencies (we hide them by default). for example, this may be needed not for scanning vulnerabilities, but for generating a SBOM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check this case?
Can you write more info about this?
docs don't have info about
ExcludeAssetsfield.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add more contexts. As interest in supply chain security grows, there is a demand to understand vulnerabilities in packages that are used for CI/CD and other purposes, even if they are not included in production applications and are used only for development.
If the library is not used at all, even for development purposes, there is no need to include it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericstj Since we're unfamiliar with .NET Core, we need your input. Given the above context, do you think we don't have to save these libraries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericstj In other ecosystems, it is common to include test assertion libraries as development libraries. And while these libraries are not used at runtime, their vulnerability is important.
This example you gave is certainly not legitimate, but are libraries used for development purposes in general use such as the one I gave not included in deps.json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deps.jsonisn't the file to see all dependencies that might be used at development time. It's used for loading binaries at runtime.At development time projects use
PackageReference(direct references) in the project, which gets computed to the full package graph inproject.assets.json. The old formatpackages.confighad a flattened graph in the single file and was really just a log for the package modifications that were made to the project file separately. These two files are better for identifying development dependencies - and are used by component governance for that purpose. They are present on the build machine / source control. There's also the new SBOM infrastructure which is better suited as a catch all.They may or may not be included, it depends on the type of package and how its referenced.
deps.jsonis only used for telling the runtime what to load. When a library is present you can be certain it's used - so if that library/package is vulnerable it should be flagged. If the package is listed in the deps file, you can be certain it was at least referenced - but it may have been eliminated by the build and not loadable at runtime - so it should not be flagged. You can't assume that a package being absent means it wasn't used at dev time since the deps.json doesn't list all development time dependencies.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm.. It looks like that we can remove non-runtime dependencies.
Also we need to write in docs that Trivy only detects runtime deps from
*.deps.jsonThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your explanation. It might be similar to Go binary scanning. It doesn't have any development modules, while go.mod includes it.
@DmitriyLewen We can just delete non-runtime dependencies and improve it later if we see any feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
@knqyf263 @ericstj can you review this PR?