feat: Add binary version parsing for nodejs, php and python #6524

kovacs-levent · 2024-04-19T10:28:58Z

Description

Implement binary version parsing for at least key binaries. The feature should be extended upon, but first round, I'm going to be focusing stuff which matters for my use-case (PHP, NodeJS, Python)... I'm keeping Java binary parsing since it's already been implemented by laurentdelosieresmano in the related PR👍 Since go-dep-parser was moved to this Repo, I'm reopening the PR and adding Python dep parsing.

Related issues

Trivy is not scanning standalone java / nodejs / PHP binaries #1064 - It was prematurely closed due to inactivity and without any support from maintainers to implement the feature request

Related PRs

Parse java php nodejs binary version go-dep-parser#35 - Old PR for the same feature request from 3 years ago, stuck in review

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

CLAassistant · 2024-04-19T10:29:03Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Levente Kovacs seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

kovacs-levent · 2024-04-19T12:47:52Z

I signed the cla... Not sure why it's not triggering as signed. On my forked branch, every test passed, so if you trigger the tests it should be good.

kovacs-levent · 2024-04-19T13:32:30Z

Hmm, while all tests are passing, I do not see the new binary results when I build the trivy from source and run the same experiment as in #6457, I'm not sure why that is the case.

EDIT: Guess I realized that I not only need to implement the parsing, but the analyzer logic as part of fanal. I'll try that.

kovacs-levent · 2024-04-20T01:32:37Z

I have spent some time with this. Now python version is detected correctly by trivy and added into the sbom in a way that xeol detects it properly according to my use case.

I looked into implementing vulnerability scanning as well for standalone binaries, but I think at first only having it part of sboms is a step forward.

There are a few problems regarding implementing this with standalone binaries:

It's not an OS package, hence the whole PR. But that also means I can't implement the vuln scanning as part of the ospkg detector library. When I tried that, trivy started to correctly detect the vulns in the binaries, but it also discarded everything else due to conflicting OS versions (if I implement it as ospkg detector driver, then I just lost all dpkg vulns for example).
It's also not really a langpkg. but this could be solved with some extra code as part of the driver logic and without messing more with the core. However, none of the ecosystems in trivy-db are really fit for querying vulnerabilities in these standalone binaries. The best bet would be NVD I thought.
BUT NVD does not have an interface to query it in trivy-db. This means that I can't query NVD's information about the standalone binary, neither OS package sources.
A Get function could be implemented for NVD, but I suspect that the best way to query it is through CPE2.3 vectors. For standalone-binaries, this is easy to generate correctly I think, but afaik, trivy does not support CPE for complexity reasons (it's also not in the SBOMs).

The only way I see to implement this nicely is by adding CPE support and also putting together a Get function for the NVD vulnsrc linked in 3rd point. Other way: just hack together an NVD get interface since it'd be used only for the scanner as a fallback in case of generic binary is detected, not for regular vulnerability scanning.

Now that I know how to implement these things, I think I can put together the remaining parts (Java, PHP, NodeJS binary as part of package detection and SBOMs) in the upcoming days and the PR would be ready.

kovacs-levent · 2024-04-20T10:54:55Z

I have added support for PHP and Nodejs standalone binaries and it works like a charm with the sboms. I'm dropping support for Java because I found that it's harder to correctly determine the package type (JRE vs. JDK), and I won't go down the rabbithole since it's not part of my usecase.

Only issue I discovered yesterday is that this will break lots of integration tests which I'll have to adjust those accordingly. After I'll adjust integration tests, I think this PR can be merged.

DmitriyLewen

Hello @kovacs-levent
Thanks for this PR.

I left some comments. Some these comments same for nodejs, php and python.

Also parsers are very similar.
I think we can merge them to 1 package. Something like this:
parser:

├── executable
│   ├── exe.go
│   ├── nodejs
│       ├── parse.go
│       ├── parse_test.go
│       └── testdata
│   ├── python
│       ├── parse.go
│       ├── parse_test.go
│       └── testdata
│   └── php
│       ├── parse.go
│       ├── parse_test.go
│       └── testdata

about analyzers:
looks like we can add new logic here - https://github.com/aquasecurity/trivy/blob/main/pkg/fanal/analyzer/executable/executable.go

If a php/python/npm binary is found, use its parser and add the library.

wdyt?

DmitriyLewen · 2024-04-22T04:30:38Z

go.mod

@@ -124,6 +124,7 @@ require (
 	github.com/alecthomas/chroma v0.10.0
 	github.com/antchfx/htmlquery v1.3.0
 	github.com/apparentlymart/go-cidr v1.1.0
+	github.com/aquasecurity/go-dep-parser v0.0.0-20240213093706-423cd04548a5


go-dep-parser in Trivy now - https://github.com/aquasecurity/trivy/tree/main/pkg/dependency/parser

I removed the go-dep-parser as a dependency, it only uses the internal package now

DmitriyLewen · 2024-04-22T04:57:07Z

pkg/dependency/parser/python/binary/exe.go

+		return nil, err
+	}
+
+	if bytes.HasPrefix(data, []byte("\x7FELF")) {


What about windows and macos formats?

Haven't thought about that yet. Will have to check on those.

I tried testing with Windows PEs, the interface can be implemented for exe and OpenExe, but the regex based detection methods don't work on the PE files I tried (I did manual testing to confirm). I honestly don't have any experience with macos binaries 🫤 So not sure if I can implement the interface for those. I also tried to obtain files to test with, but failed, since now I don't have access to a macos system. Any idea if regex based detection would work on those?

If not, I'd rather not bother with implementing this for those binaries, even though you could implement the interface and executableAnalyzer would still run on those, the binary version detection won't give package in the AnalysisResult.

DmitriyLewen · 2024-04-22T04:58:27Z

pkg/dependency/parser/python/binary/parse.go

+	}
+
+	// Python's version pattern is [NUL]3.11.2[NUL]
+	re := regexp.MustCompile(`^\d{1,4}\.\d{1,4}\.\d{1,4}[-._a-zA-Z0-9]*$`)


Can you add a comment with a link where you get this regex?

It's from syft... Should I link that? All the regexes I used, I mostly copied them from syft. For example python regex:
https://github.com/anchore/syft/blob/b04bc0fbfe5742a970aa3f06594b1e82c87eb4a5/syft/pkg/cataloger/binary/classifiers.go#L495

DmitriyLewen · 2024-04-22T04:59:31Z

pkg/dependency/parser/python/binary/parse.go

+
+	var libs []types.Library
+	libs = append(libs, types.Library{
+		ID: packageID(name, vers),


You use dependent.ID once. So you can just use it here.

Suggested change

ID: packageID(name, vers),

ID: dependency.ID(ftypes.PythonGeneric, name, version),

Removed the packageID function from all parsers and replaced with dependency.ID call only

DmitriyLewen · 2024-04-22T05:08:31Z

pkg/dependency/parser/python/binary/parse.go

+		}
+	}
+
+	return "python", vers


Is there a way to determine package name?
Looks like it might be confusing if all detected binaries are named "python"

I don't think so unfortunately, if there was additional package information, the standalone binary detection would not be needed. But I will look into it if it's obtainable from some metadata.

There's a way to try detect it with OS package managers, but they don't necessarily yield results unfortunately. I think if these were in package manager, then OS pkg analyzer would be scanning these, so probably also not the right way to go about it.

I wasn't able to find any way to figure out more concrete package name from the standalone binaries.

DmitriyLewen · 2024-04-22T05:14:46Z

pkg/fanal/analyzer/language/python/binary/binary_test.go

+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			a := pythonBinaryAnalyzer{}
+			fileInfo, err := os.Lstat(tt.filePath)


Looks like we can use 1 file and change names, permissions, etc. in test.
Then we can reduce the number of copies of test files.

Yeah, I'll rework the tests for fanal in an upcoming commit.

Fanal test cases were reworked. It uses the common logic of executable tests. Added test cases for Python/Node/PHP binaries.

DmitriyLewen · 2024-04-22T05:18:06Z

pkg/scanner/langpkg/scan.go

@@ -65,7 +65,6 @@ func (s *scanner) Scan(target types.ScanTarget, _ types.ScanOptions) (types.Resu
 		}

 		logger := log.WithPrefix(string(app.Type))
-


Looks like we don't need this change.

Hmm, not sure why I did it, but I'll remove it in next commit, it's just some "automatic" line deletion I did I guess :)

DmitriyLewen · 2024-04-22T05:44:33Z

pkg/dependency/parser/python/binary/exe.go

@@ -0,0 +1,82 @@
+// Ported from https://github.com/golang/go/blob/e9c96835971044aa4ace37c7787de231bbde05d9/src/cmd/go/internal/version/exe.go


It looks like Go 1.22 doesn't have this file. Can you please update the link and double check - we may need to add some changes.

I unfortunately didn't find this in Go 1.22 either. I'll take a look into this in next commit.

I have updated the link and also the implementation was changed.

It's not a 1on1 port, since they repurposed it for capturing Go main module version, but we need to read whole executable file, but I used the same ideas. It also contains interfaces for other binaries (didn't put those in, only elf).

DmitriyLewen · 2024-04-22T06:10:58Z

pkg/fanal/analyzer/language/python/binary/binary.go

+	pythonLibNameRegex := regexp.MustCompile("^libpython[0-9]+(?:[.0-9]+)+[a-z]?[.]so.*$")
+	pythonBinaryNameRegex := regexp.MustCompile("(?:.*/|^)python(?P<version>[0-9]+(?:[.0-9]+)+)?$")


will be great if you add some comment about these regex (e.g. link to docs)

Same as for the other regexes, it's from Syft, should I link that?
https://github.com/anchore/syft/blob/b04bc0fbfe5742a970aa3f06594b1e82c87eb4a5/syft/pkg/cataloger/binary/classifiers.go#L495

DmitriyLewen · 2024-04-22T06:12:36Z

pkg/fanal/analyzer/const.go

+	TypePip       	  Type = "pip"
+	TypePipenv    	  Type = "pipenv"
+	TypePoetry    	  Type = "poetry"
+	TypePythonGeneric Type = "python"


looks like we can use executable for new binaries

Change all the types to become "TypeXExecutable".

I understand what you meant here now, that all the new binaries could be TypeExecutable.

I use the language specific types in fanal executable.go, to figure out which binary type was detected in case of "detectable" binary. It can be localized to executable.go instead and use the common type for the "outside" return types.

I added them as a separate, new group of variables instead in this same file.

kovacs-levent · 2024-04-27T10:27:52Z

Thanks for the feedback, I will make the changes when I have the time, this week’s been busy, but I’ll try to make progress when I have the time.

kovacs-levent · 2024-06-03T09:54:20Z

As you can see, I finally got to finish up this PR. I'm going to work some more on this in the upcoming days, so that all the comments are reflected in the changes.

I did refactor the code to have the proposed structure.

…y into parse-binary-versions

kovacs-levent · 2024-06-03T13:08:14Z

I had some troubles with rebasing, probably shouldn't have waited so long with that 😓 I had to force push, because other people's commits started appearing here, so I did a hard reset. Sorry for the ugly commit history 🤷

kovacs-levent · 2024-06-04T12:26:39Z

I have encountered a weird issue now... I found that if Rekor sbom source is not specificed, then ExecutableAnalyzer is getting disabled. https://github.com/aquasecurity/trivy/blob/main/pkg/commands/artifact/run.go#L514 Can someone shed some light on why is that? The commit message contains a PR... Seems like completely unrelated issue: #6163

For some reason, the whole executable analyzer was disabled, instead of only disabling Rekor queries based on the executables. Can you please provide some insight on whether it's fine to remove this, and how could I go about it to disable Rekor queries instead? @DmitriyLewen

kovacs-levent · 2024-06-04T12:29:20Z

Problem is that, with Rekor enabled, it works fine for me, but it takes significantly longer to scan with Rekor, even though I'm not interested in that information.

kovacs-levent · 2024-06-04T14:29:26Z

I have encountered a weird issue now... I found that if Rekor sbom source is not specificed, then ExecutableAnalyzer is getting disabled. https://github.com/aquasecurity/trivy/blob/main/pkg/commands/artifact/run.go#L514 Can someone shed some light on why is that? The commit message contains a PR... Seems like completely unrelated issue: #6163

For some reason, the whole executable analyzer was disabled, instead of only disabling Rekor queries based on the executables. Can you please provide some insight on whether it's fine to remove this, and how could I go about it to disable Rekor queries instead? @DmitriyLewen

I fixed this issue in the following commit. Now if the Rekor flag isn't enabled, it only skips the "unpackaged" hook, so Rekor queries are only made if the Rekor is actually specified, but executable analysis still happens regardless.

DmitriyLewen · 2024-06-05T04:05:33Z

Hello @kovacs-levent
Thanks for your work!

I'm working on other priority tasks.
I will consider this PR when I have time for it

Regards, Dmitriy

Add binary version parsing for java, nodejs, php and python

105f39e

kovacs-levent requested review from knqyf263 and DmitriyLewen as code owners April 19, 2024 10:28

Levente Kovacs added 5 commits April 19, 2024 12:34

Add parse test for Python binary

b6027e8

go mod tidy

5b3eec8

lint via gci + fix lint errors

3ead37c

Remove := because of linter

82ae5f5

Fix parser to grab correct match index

407ebd8

Levente Kovacs added 3 commits April 19, 2024 15:32

Normalize to binary go package

8e8443c

Implement standalone python binary scanning in fanal

526cd87

Python version parsing

1a1425f

kovacs-levent marked this pull request as draft April 20, 2024 08:37

Remove java and add nodejs and php support for standalone binaries

35a0a66

kovacs-levent changed the title ~~feat: Add binary version parsing for java, nodejs, php and python~~ feat: Add binary version parsing for nodejs, php and python Apr 20, 2024

DmitriyLewen reviewed Apr 22, 2024

View reviewed changes

Refactor to remove duplicated code, reflect some review comments

699531d

Levente Kovacs added 7 commits June 3, 2024 13:11

Add binary version parsing for java, nodejs, php and python

a708e38

Add parse test for Python binary

68624ea

go mod tidy

d704cda

lint via gci + fix lint errors

a57944f

Remove := because of linter

d7c795b

Fix parser to grab correct match index

41f0857

Normalize to binary go package

66a008b

Levente Kovacs added 5 commits June 3, 2024 13:13

Implement standalone python binary scanning in fanal

a9f14c6

Python version parsing

5c326db

Remove java and add nodejs and php support for standalone binaries

da71578

Refactor to remove duplicated code, reflect some review comments

39c24d0

Merge branch 'parse-binary-versions' of github.com:kovacs-levent/triv…

1a78560

…y into parse-binary-versions

kovacs-levent force-pushed the parse-binary-versions branch from 1a78560 to 39c24d0 Compare June 3, 2024 12:38

Levente Kovacs added 2 commits June 3, 2024 14:42

Refactor to remove duplicated code, reflect some review comments

ed91d36

Fix dependency errors

e6e3165

kovacs-levent force-pushed the parse-binary-versions branch from 39c24d0 to e6e3165 Compare June 3, 2024 12:57

Merge branch 'parse-binary-versions' of github.com:kovacs-levent/triv…

9e39387

…y into parse-binary-versions

Levente Kovacs added 8 commits June 3, 2024 15:15

Resolve previously unfixed conflict

5f93328

Rework fanal tests

6ba4de5

Remove unused import

3e71c17

Fix go.mod

5ac66dc

Fix lint errors

739880a

Add seperate types const for detectable binaries

169e9ae

Remove unused types

f35d924

fix lint issues

5834ab9

Fix Rekor disabling executable analysis, and only disable Rekor queries

3cf74b4

Update Exe implementation inspired by updated buildinfo.go

e0a45ee

	ID: packageID(name, vers),
	ID: dependency.ID(ftypes.PythonGeneric, name, version),

		@@ -65,7 +65,6 @@ func (s *scanner) Scan(target types.ScanTarget, _ types.ScanOptions) (types.Resu
		}

		logger := log.WithPrefix(string(app.Type))

		@@ -0,0 +1,82 @@
		// Ported from https://github.com/golang/go/blob/e9c96835971044aa4ace37c7787de231bbde05d9/src/cmd/go/internal/version/exe.go

		pythonLibNameRegex := regexp.MustCompile("^libpython[0-9]+(?:[.0-9]+)+[a-z]?[.]so.*$")
		pythonBinaryNameRegex := regexp.MustCompile("(?:.*/\|^)python(?P<version>[0-9]+(?:[.0-9]+)+)?$")

feat: Add binary version parsing for nodejs, php and python #6524

Are you sure you want to change the base?

feat: Add binary version parsing for nodejs, php and python #6524

Conversation

kovacs-levent commented Apr 19, 2024 • edited

Description

Related issues

Related PRs

Checklist

CLAassistant commented Apr 19, 2024 • edited

kovacs-levent commented Apr 19, 2024

kovacs-levent commented Apr 19, 2024 • edited

kovacs-levent commented Apr 20, 2024 • edited

kovacs-levent commented Apr 20, 2024

DmitriyLewen 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

kovacs-levent Jun 3, 2024 • edited

Choose a reason for hiding this comment

kovacs-levent Jun 4, 2024 • edited

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

kovacs-levent Jun 5, 2024 • edited

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

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

kovacs-levent Jun 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kovacs-levent commented Apr 27, 2024

kovacs-levent commented Jun 3, 2024 • edited

kovacs-levent commented Jun 3, 2024

kovacs-levent commented Jun 4, 2024 • edited

kovacs-levent commented Jun 4, 2024

kovacs-levent commented Jun 4, 2024

DmitriyLewen commented Jun 5, 2024

kovacs-levent commented Apr 19, 2024 •

edited

CLAassistant commented Apr 19, 2024 •

edited

kovacs-levent commented Apr 19, 2024 •

edited

kovacs-levent commented Apr 20, 2024 •

edited

kovacs-levent Jun 3, 2024 •

edited

kovacs-levent Jun 4, 2024 •

edited

kovacs-levent Jun 5, 2024 •

edited

kovacs-levent Jun 3, 2024 •

edited

kovacs-levent commented Jun 3, 2024 •

edited

kovacs-levent commented Jun 4, 2024 •

edited