Feat: NuGet Scanner

[Johannestegner]

Work in progress for NuGet scanning (#681).
Currently, it uses the GitHub SA vulnsrc only.

Analyzer: aquasecurity/fanal#139
Parser: aquasecurity/go-dep-parser#14 (merged)

Further, due to nugets legacy package versions (x.x.x.x), an update to the package version-check logic is required, which I started to work on in #705 something that also might be possible to help with the following issue: #702

[codecov]

Codecov Report

Merging #686 (1fce536) into main (7b86f81) will decrease coverage by 0.15%.
The diff coverage is 41.66%.

@@            Coverage Diff             @@
##             main     #686      +/-   ##
==========================================
- Coverage   68.66%   68.50%   -0.16%     
==========================================
  Files          57       57              
  Lines        2202     2210       +8     
==========================================
+ Hits         1512     1514       +2     
- Misses        558      564       +6     
  Partials      132      132              

Impacted Files	Coverage Δ
pkg/detector/library/driver.go	46.66% <0.00%> (-4.56%)	⬇️
pkg/scanner/local/scan.go	84.94% <ø> (ø)
pkg/vulnerability/vulnerability.go	76.23% <50.00%> (-1.09%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b86f81...1fce536. Read the comment docs.

[masahiro331]

@Johannestegner
Thank you for great PR.

I've read the NuGet documentation.
Is it possible that a value like "1.0.01.1" could be put in semver.Version?

https://docs.microsoft.com/en-us/nuget/concepts/package-versioning#normalized-version-numbers

[Johannestegner]

Hi @masahiro331!
You are correct.
And as you probably already guessed, this was something that I did not think about when implementing the initial code for the scanner! So good catch! hehe.

I'm thinking that there are multiple ways that this case could be resolved, but I'm not really sure what the best way would be.
I do feel that it would be kind of a risk to update the MatchVersion code to handle it, as there is quite a lot depending on that code to work correctly, although it might actually be the best way...

What do you think would be the best way to handle the case?

[masahiro331]

@Johannestegner
I hope this helps!

First

I've created a vulnerability detection feature in my environment.
There is no need to add pkg/detector/library/nuget/advisory.go

You can implement it by adding the following code.
https://github.com/aquasecurity/trivy/blob/master/pkg/scanner/local/scan.go#L22-L23

       _ "github.com/aquasecurity/fanal/analyzer/library/nuget"

https://github.com/aquasecurity/trivy/blob/master/pkg/detector/library/driver.go#L32
filename ref: https://github.com/aquasecurity/fanal/pull/139/files#diff-0f67d9d34e8f171dd718f71c0d09a3d7732446dc688860a84e2db7f698d3228dR17

       case "packages.lock.json":
               driver = newCSharpDriver()

https://github.com/aquasecurity/trivy/blob/master/pkg/detector/library/driver.go#L81

func newCSharpDriver() Driver {
       return NewDriver(vulnerability.CSharp, ghsa.NewAdvisory(ecosystem.Nuget))
}

Add const.
https://github.com/aquasecurity/trivy-db/blob/master/pkg/vulnsrc/vulnerability/const.go

CSharp = "c-sharp"

Second

Version comparison problem.
This is a very difficult, I can't judge it myself.

[Johannestegner]

Been trying to decipher the nuget docs for version specification...

When looking at the up-to-date docs, it seems like NuGet packages are intended to use a SEMVER-2.0 convention, which is good, cause that is what the scanner is currently built to support. BUT, I know that there are packages out there with a x.x.x.x versioning convention (for example, AWSSDK.Core latest version is 3.5.1.26), which from my research is a legacy versioning (ProGet 4 vs new ProGet 5) convention.

The old versioning standard uses:
<major>.<minor>.<build>.<revision> where, from my gathering, the revision number is used in cases such as nightly builds. But I'm not sure what the intended usage is, seeing anyone could basically use it as they wish...

I'm feeling a bit clueless with the versioning issue, I mean... easiest way would just be to ignore the revision number, but that would potentially create issues if someone uses the revision number as something else than just to indicate "same version, we just rebuilt the binaries". Neither could it be seen as a pre-release version as it's the other way around basically.

The ways I see it, either the utils code would have to be rewritten in some way (or a new version parsing extension added or something), or the revision number is obliterated and we all pretend it never existed... :P

But... I'm only a contributor, so this should be a decision of the maintainers! :)

[simar7]

@Johannestegner
Thank you for great PR.

I've read the NuGet documentation.
Is it possible that a value like "1.0.01.1" could be put in semver.Version?

https://docs.microsoft.com/en-us/nuget/concepts/package-versioning#normalized-version-numbers

Thanks for sharing this @masahiro331 but the way I read this is that going 3.4+ it will conform to semver versioning. Is it not the case?

If so is the case we can make a disclaimer that we will only support NuGet 3.4+ or above.

[chgl]

The ways I see it, either the utils code would have to be rewritten in some way (or a new version parsing extension added or something), or the revision number is obliterated and we all pretend it never existed... :P

If the revision generally refers to build metadata, one could convert <major>.<minor>.<build>.<revision> to semver like so: <major>.<minor>.<build>+<revision> (https://semver.org/#spec-item-10). This way at least this information isn't lost. but it might lead to confusion when displaying the parsed version or have any other unforeseen consequences...

[masahiro331]

This import probably not necessary.

[Johannestegner]

Actually, I think that the advisory code might be required again if we are to manipulate the versions of the legacy dependencies, right? Or am I overthinking this? :)

[Johannestegner]

If the revision generally refers to build metadata, one could convert <major>.<minor>.<build>.<revision> to semver like so: <major>.<minor>.<build>+<revision> (https://semver.org/#spec-item-10). This way at least this information isn't lost. but it might lead to confusion when displaying the parsed version or have any other unforeseen consequences...

Yes, that is a way it could be handled! Will take a look at that :)

[Johannestegner]

Alright, after throughout code viewing, I have came to the conclusion that if we want to handle this in a neat way (which it should be imho) some extra rewrite is required.
As the code works as of now, if it's a x.x.x.x version, it is in util.go FormatPatchVersion changed to a x.x.x-x type of version.

The - is counted as a pre-release in the semver code, which would then make Validate on the constraint return false.

I'm not fully sure how the semver package handles + more than it is counted as "metadata", but will do some testing in the utils_test.go file after posting this...

If the constraint is accepted with the metadata version, that should be enough, else an extra check on the metadata version will have to be done.
So in short:

trivy/pkg/scanner/utils/utils.go

Line 66 in 0b5d936

func FormatPatchVersion(version string) string {

Should return a x.x.x+x instead of x.x.x-x version in case there are 4 parts. (guessing version = strings.Join(part[:3], ".") + "-" + strings.Join(part[3:], ".") should use + "+" + instead of -).

[Johannestegner]

Alright, after a lot of messy work, I did succeed in this. I have added the code to this PR, but I'm about to branch it out and add it as a separate PR instead due to the fact that it is a change that is not nuget specific, but to all "revision" packages (such as ruby and other similar packages).

---

Edit:

Removed the code from this pull request in favor of #705

[knqyf263]

version package in aquasecurity/go-version supports leading zeros.
https://github.com/aquasecurity/go-version/blob/main/pkg/version/constraint_test.go#L223-L226

Also, it supports more than 3 numbers such as v3.2.6.1.
https://github.com/aquasecurity/go-version/blob/main/pkg/version/constraint_test.go#L218-L221

I hope it works with NuGet as well. I'm going to make it possible to use different version libraries for each language. Could you merge/rebase the following branch into this branch? I may update a little bit more according to a review, but I don't believe it will change so much.
#740

[Johannestegner]

Rebased and will take a closer look later today! :)

[knqyf263]

Not urgent at all. Take your time!

[knqyf263]

@Johannestegner Thank you for the great contribution!

[Johannestegner]

Awesome that it got in!

Tyvm for the reviews and help :)