fix(dotnet): don't include non-runtime libraries into report for `*.deps.json` files #7039

DmitriyLewen · 2024-06-27T11:48:27Z

Description

Dependencies with empty runtime, runtimeTarget and native fields in target section are not needed by the runtime, and the dotnet build command doesn't create *.dll files for them.
(see #4282 (comment)).
Don't include these libraries into report.

Related Issues

Close feat(.NET): mark some deps from .deps.json files as Dev #7079

Related Discussions

Close NET6 CVE-2019-0980 CVE-2019-0981 - possible bug in .deps.json trans deps resolution #4282

Checklist

I've read the guidelines for contributing to this repository.
I've followed the conventions in the PR title.
I've added tests that prove my fix is effective or that my feature works.
I've updated the documentation with the relevant information (if needed).
I've added usage information (if the PR introduces new options)
I've included a "before" and "after" example to the description (if the PR is a user interface change).

ericstj · 2024-06-27T15:43:59Z

pkg/dependency/parser/dotnet/core_deps/parse.go

+			// 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.


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.

in general I think the same way.
thank you for detailing this 👍

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 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

The package might be referenced with ExcludeAssets="Runtime"

Do we need to check this case?
Can you write more info about this?

docs don't have info about ExcludeAssets field.

in general I think the same way.

thank you for detailing this 👍

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 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

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.

@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?

@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.

For instance - a build target that inserts code into the binary. That package might appear without any runtime assets, however such a package is not the norm. .

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?

deps.json isn'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 in project.assets.json. The old format packages.config had 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.

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?

They may or may not be included, it depends on the type of package and how its referenced. deps.json is 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.

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.json

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.

Done 👍
@knqyf263 @ericstj can you review this PR?

knqyf263 · 2024-07-02T08:08:30Z

@DmitriyLewen Can you create an issue so we can add it to the v0.54.0 milestone?

DmitriyLewen · 2024-07-02T08:58:42Z

@knqyf263 Created #7079

ericstj

The behavior looks correct, just some suggestions to change the comments to match and add a test.

ericstj · 2024-07-16T15:38:48Z

pkg/dependency/parser/dotnet/core_deps/parse.go

+		})
+		return true
+	}
+	// Check that `runtime`, `runtimeTarget` and `native` sections are empty


Looks to me like it checks if they are not empty.

Suggested change

// Check that `runtime`, `runtimeTarget` and `native` sections are empty

// Check that `runtime`, `runtimeTarget` and `native` sections are not empty

fixed in afe12b4

ericstj · 2024-07-16T15:41:03Z

pkg/dependency/parser/dotnet/core_deps/parse.go

-	Type      string `json:"type"`
-	StartLine int
-	EndLine   int
+// isRuntimeLibrary returns true if library doesn't contain `runtime`, `runtimeTarget` and `native` sections.


Suggested change

// isRuntimeLibrary returns true if library doesn't contain `runtime`, `runtimeTarget` and `native` sections.

// isRuntimeLibrary returns true if library contains `runtime`, `runtimeTarget` or `native` sections, or if the library is missing from `targetLibs`.

I think this function returns true if the library contains one of these sections, or if the library is completely missing from the section. The behavior looks correct, just the comment seems inverted.

I updated this function, but forgot to update comment.
Thanks!
Fixed in afe12b4

ericstj · 2024-07-16T17:39:10Z

pkg/dependency/parser/dotnet/core_deps/testdata/without-runtime.deps.json

@@ -0,0 +1,96 @@
+{


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 System.Net.Http etc. Here's a sample deps file:
testRuntimeDeps.deps.json

This was created with

dotnet new console -f net6.0 dotnet add package NETStandard.Library --version 1.6.0 dotnet build

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.

Your test file is too large.
So I only inserted System.Net.Http and NETStandard.Library libraries into test file. - ca87c12

DmitriyLewen · 2024-07-19T05:36:51Z

I updated PR according to notes
@knqyf263 @ericstj if you don't have any suggestions - i think we can merge this PR.

…eps.json` files (aquasecurity#7039)

ericstj · 2024-08-27T16:39:27Z

@DmitriyLewen It looks like this might not be working correctly. We're still seeing reports of this when using the latest trivvy 0.54.1. Is there a way to get some diagnostic information out of trivvy to see why it's still flagging vulnerabilities here?

DmitriyLewen · 2024-08-28T01:47:31Z

Hello @ericstj
Trivy has debug flag, but I'm not sure if it will help you.
It seems like one way is to use the IDE debugger to step through it.
Or you can send me a test file and I'll check it.

ericstj · 2024-08-28T18:30:56Z

@DmitriyLewen
The command I was trying was this:
docker run -it aquasec/trivy:0.54.1 image mcr.microsoft.com/dotnet/sdk:8.0 --scanners vuln --vuln-type library --debug

It reports a vulnerability in System.Formats.Asn1, however all files present are updated.

I believe it's still seeing mention to the older package versions in deps files, however I believe all those deps files should have empty targets sections. If I search the container for any other copies of this file, I do see one in powershell, but that's also of sufficient version to not trigger.

DmitriyLewen · 2024-08-29T06:25:55Z

Hello @ericstj
I found file with System.Formats.Asn1dependency
Trivy found this file from spdx file:

➜  7039 docker run -it --rm mcr.microsoft.com/dotnet/sdk:8.0 cat /usr/share/powershell/.store/powershell.linux.arm64/7.4.4/powershell.linux.arm64/7.4.4/tools/net8.0/any/Modules/PSReadLine/_manifest/spdx_2.2/manifest.spdx.json | grep '"name": "System.Formats.Asn1"' -A 16
      "name": "System.Formats.Asn1",
      "SPDXID": "SPDXRef-Package-D60BE9A079A339572CC368D77C4FE3CD4860E5B2846B9831B99485A3ABD77F4B",
      "downloadLocation": "NOASSERTION",
      "filesAnalyzed": false,
      "licenseConcluded": "NOASSERTION",
      "licenseDeclared": "NOASSERTION",
      "copyrightText": "NOASSERTION",
      "versionInfo": "6.0.0",
      "externalRefs": [
        {
          "referenceCategory": "PACKAGE-MANAGER",
          "referenceType": "purl",
          "referenceLocator": "pkg:nuget/[email protected]"
        }
      ],
      "supplier": "NOASSERTION"
    },

ericstj · 2024-08-29T15:20:38Z

Oh, that's interesting. It's an SBOM from powershell. It's another false positive since they aren't actually shipping that binary. I'll follow up with them to see if it's a problem they can fix.

Thank you for your help. Is there any trick to getting the map of components out of the scanner or more logging out of the scanner?

DmitriyLewen · 2024-08-30T04:17:50Z

Is there any trick to getting the map of components out of the scanner

json format + '--list-all-pkgs flag shows all detected packages.

ericstj · 2024-08-30T18:58:32Z

Perfect, thank you!

DmitriyLewen added 5 commits June 27, 2024 17:24

feat(parser): check Targets to mark libs as dev

298f192

test(parser): add testcase with dev field

c8df60a

test(fanal): add id field

7066c9a

test(integration): update golden file

e583108

docs: add info about --include-dev-deps flag into .NET page

d8dcd12

DmitriyLewen self-assigned this Jun 27, 2024

DmitriyLewen marked this pull request as ready for review June 27, 2024 12:05

DmitriyLewen requested a review from knqyf263 as a code owner June 27, 2024 12:05

ericstj reviewed Jun 27, 2024

View reviewed changes

DmitriyLewen mentioned this pull request Jul 2, 2024

feat(.NET): mark some deps from .deps.json files as Dev #7079

Closed

DmitriyLewen added 2 commits July 15, 2024 08:19

refactor: remove non-runtime libs

2725e2c

docs: update coverage page

21c6cd3

DmitriyLewen force-pushed the fix-dotnet-core/add-dev-flag-for-libs-built-into-netcore branch from 910d497 to 21c6cd3 Compare July 15, 2024 02:35

docs: remove unused links

5799449

DmitriyLewen changed the title ~~fix(dotnet): mark libraries built into .NETCore as Dev~~ fix(dotnet): don't include non-runtime libraries for *.deps.json files Jul 15, 2024

DmitriyLewen changed the title ~~fix(dotnet): don't include non-runtime libraries for *.deps.json files~~ fix(dotnet): don't include non-runtime libraries into report for *.deps.json files Jul 15, 2024

knqyf263 approved these changes Jul 15, 2024

View reviewed changes

ericstj approved these changes Jul 16, 2024

View reviewed changes

DmitriyLewen added 2 commits July 17, 2024 13:33

fix: update incorrect comments

afe12b4

test: add System.Net.Http and NETStandard.Library to test file

ca87c12

knqyf263 added this pull request to the merge queue Jul 19, 2024

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jul 19, 2024

knqyf263 added this pull request to the merge queue Jul 19, 2024

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jul 19, 2024

knqyf263 added this pull request to the merge queue Jul 22, 2024

Merged via the queue into aquasecurity:main with commit 5bc662b Jul 22, 2024
17 checks passed

aqua-bot mentioned this pull request Jul 22, 2024

release: v0.54.0 [main] #7075

Merged

skahn007gl pushed a commit to skahn007gl/trivy that referenced this pull request Jul 23, 2024

fix(dotnet): don't include non-runtime libraries into report for `*.d…

1165017

…eps.json` files (aquasecurity#7039)

DmitriyLewen deleted the fix-dotnet-core/add-dev-flag-for-libs-built-into-netcore branch August 28, 2024 01:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dotnet): don't include non-runtime libraries into report for `*.deps.json` files #7039

fix(dotnet): don't include non-runtime libraries into report for `*.deps.json` files #7039

DmitriyLewen commented Jun 27, 2024 •

edited

Loading

ericstj Jun 27, 2024 •

edited

Loading

DmitriyLewen Jun 28, 2024

DmitriyLewen Jun 28, 2024

knqyf263 Jun 28, 2024

knqyf263 Jul 1, 2024

knqyf263 Jul 9, 2024 •

edited

Loading

ericstj Jul 11, 2024 •

edited

Loading

DmitriyLewen Jul 12, 2024

knqyf263 Jul 12, 2024

DmitriyLewen Jul 15, 2024

knqyf263 commented Jul 2, 2024

DmitriyLewen commented Jul 2, 2024

ericstj left a comment

ericstj Jul 16, 2024

DmitriyLewen Jul 17, 2024

ericstj Jul 16, 2024

DmitriyLewen Jul 17, 2024

ericstj Jul 16, 2024

DmitriyLewen Jul 17, 2024

DmitriyLewen commented Jul 19, 2024

ericstj commented Aug 27, 2024

DmitriyLewen commented Aug 28, 2024

ericstj commented Aug 28, 2024

DmitriyLewen commented Aug 29, 2024

ericstj commented Aug 29, 2024

DmitriyLewen commented Aug 30, 2024

ericstj commented Aug 30, 2024

		// 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.

	// Check that `runtime`, `runtimeTarget` and `native` sections are empty
	// Check that `runtime`, `runtimeTarget` and `native` sections are not empty

	// isRuntimeLibrary returns true if library doesn't contain `runtime`, `runtimeTarget` and `native` sections.
	// isRuntimeLibrary returns true if library contains `runtime`, `runtimeTarget` or `native` sections, or if the library is missing from `targetLibs`.

fix(dotnet): don't include non-runtime libraries into report for *.deps.json files #7039

fix(dotnet): don't include non-runtime libraries into report for *.deps.json files #7039

Conversation

DmitriyLewen commented Jun 27, 2024 • edited Loading

Description

Related Issues

Related Discussions

Checklist

ericstj Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knqyf263 Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

ericstj Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knqyf263 commented Jul 2, 2024

DmitriyLewen commented Jul 2, 2024

ericstj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitriyLewen commented Jul 19, 2024

ericstj commented Aug 27, 2024

DmitriyLewen commented Aug 28, 2024

ericstj commented Aug 28, 2024

DmitriyLewen commented Aug 29, 2024

ericstj commented Aug 29, 2024

DmitriyLewen commented Aug 30, 2024

ericstj commented Aug 30, 2024

fix(dotnet): don't include non-runtime libraries into report for `*.deps.json` files #7039

fix(dotnet): don't include non-runtime libraries into report for `*.deps.json` files #7039

DmitriyLewen commented Jun 27, 2024 •

edited

Loading

ericstj Jun 27, 2024 •

edited

Loading

knqyf263 Jul 9, 2024 •

edited

Loading

ericstj Jul 11, 2024 •

edited

Loading