fix: parsing crash on malformed pnpm lockfile #1327

ivmeta · 2024-10-17T21:46:14Z

Scalibr is crashing with a SIGSEGV while trying to parse this directory:

https://github.com/semgrep/semgrep/tree/develop/cli/tests/default/e2e/targets/dependency_aware

This is due to the lack of an array length check after the dependencyPath split.

Added failing lockfile as a test.

Steps to reproduce it

$ git clone https://github.com/semgrep/semgrep
$ osv-scanner semgrep/cli/tests/default/e2e/targets/dependency_aware/pnpm-error-key/pnpm-lock.yaml

panic: runtime error: index out of range [0] with length 0

goroutine 1 [running]:
github.com/google/osv-scanner/pkg/lockfile.extractPnpmPackageNameAndVersion({0xc000328fd0?, 0xb?}, 0x401599999999999a?)
	/tmp/osv-scanner/pkg/lockfile/parse-pnpm-lock.go:95 +0x34f
github.com/google/osv-scanner/pkg/lockfile.parsePnpmLock({0xc000814230?, 0xc0004a6fc0?})
	/tmp/osv-scanner/pkg/lockfile/parse-pnpm-lock.go:141 +0x145
github.com/google/osv-scanner/pkg/lockfile.PnpmLockExtractor.Extract({}, {0x7f545125f268, 0xc00015de40})
	/tmp/osv-scanner/pkg/lockfile/parse-pnpm-lock.go:208 +0x25b
github.com/google/osv-scanner/pkg/lockfile.ExtractDeps({0x7f545125f268, 0xc00015de40}, {0x0, 0x0})
	/tmp/osv-scanner/pkg/lockfile/extract.go:61 +0x24a
github.com/google/osv-scanner/pkg/osvscanner.scanLockfile({0x19374a8, 0xc000359e00}, {0xc00037e540, 0x69}, {0x0, 0x0}, 0x0)
	/tmp/osv-scanner/pkg/osvscanner/osvscanner.go:371 +0x8aa
github.com/google/osv-scanner/pkg/osvscanner.scanDir.func1({0x7ffd822dfd35?, 0x100?}, {0x1932558, 0xc0004a4b90}, {0x0?, 0x0?})
	/tmp/osv-scanner/pkg/osvscanner/osvscanner.go:168 +0x745
path/filepath.walkDir({0x7ffd822dfd35, 0x54}, {0x1932558, 0xc0004a4b90}, 0xc000814970)
	/usr/lib/golang/src/path/filepath/path.go:443 +0x50
path/filepath.WalkDir({0x7ffd822dfd35, 0x54}, 0xc000814970)
	/usr/lib/golang/src/path/filepath/path.go:533 +0x7b
github.com/google/osv-scanner/pkg/osvscanner.scanDir({0x19374a8?, 0xc000359e00?}, {0x7ffd822dfd35?, 0x54?}, 0x0?, 0x0?, 0x1?, 0x0?)
	/tmp/osv-scanner/pkg/osvscanner/osvscanner.go:126 +0x212
github.com/google/osv-scanner/pkg/osvscanner.DoScan({{0x0, 0x0, 0x0}, {0x0, 0x0, 0x0}, {0xc0004a4a80, 0x1, 0x1}, {0x0, ...}, ...}, ...)
	/tmp/osv-scanner/pkg/osvscanner/osvscanner.go:906 +0xf73
github.com/google/osv-scanner/cmd/osv-scanner/scan.action(0xc000359500, {0x19255e0, 0xc0000f6028}, {0x19255e0, 0xc0000f6030})
	/tmp/osv-scanner/cmd/osv-scanner/scan/main.go:208 +0xca8
github.com/google/osv-scanner/cmd/osv-scanner/scan.Command.func2(0xc000815ab8?)
	/tmp/osv-scanner/cmd/osv-scanner/scan/main.go:145 +0x2c
github.com/urfave/cli/v2.(*Command).Run(0xc0005d2160, 0xc000359500, {0xc000688080, 0x2, 0x2})
	GODIR/pkg/mod/github.com/urfave/cli/v2@v2.27.5/command.go:276 +0x97d
github.com/urfave/cli/v2.(*Command).Run(0xc0005d2840, 0xc0003593c0, {0xc0004a6060, 0x3, 0x3})
	GODIR/pkg/mod/github.com/urfave/cli/v2@v2.27.5/command.go:269 +0xbb7
github.com/urfave/cli/v2.(*App).RunContext(0xc00024c600, {0x1932328, 0x2397500}, {0xc0004a6060, 0x3, 0x3})
	GODIR/pkg/mod/github.com/urfave/cli/v2@v2.27.5/app.go:333 +0x5a5
github.com/urfave/cli/v2.(*App).Run(...)
	GODIR/pkg/mod/github.com/urfave/cli/v2@v2.27.5/app.go:307
main.run({0xc0000400a0, 0x2, 0x2}, {0x19255e0, 0xc0000f6028}, {0x19255e0, 0xc0000f6030})
	/tmp/osv-scanner/cmd/osv-scanner/main.go:52 +0x678
main.main()
	/tmp/osv-scanner/cmd/osv-scanner/main.go:126 +0x45

google-cla · 2024-10-17T21:46:18Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

G-Rath · 2024-10-17T22:13:47Z

pkg/lockfile/parse-pnpm-lock_test.go

@@ -586,3 +586,13 @@ func TestParsePnpmLock_Files(t *testing.T) {
 		},
 	})
 }
+
+func TestParsePnpmLock_Invalid(t *testing.T) {


can you assert the packages that are expected to be extracted as part of the test?

G-Rath · 2024-10-17T22:13:49Z

pkg/lockfile/fixtures/pnpm/invalid.yaml

could you give this a more descriptive name? it can still be terse, but currently "invalid" could mean a lot of things (the first that times to mind is that it's invalid YAML) - something like "with-invalid-path" would be enough

Arguably the file is valid (I'm not an expert in pnpm lockfiles myself), but osv-scanner is failing to parse it correctly (and it crashes) on the axios@1.2.5 line.

This test file just validates that osv-scanner doesn't crash while reading it.

Happy to update the test case function / fixture file name to whatever you think captures this subtlety.

My understanding is that this is an invalid key - while I'm also not an expect on pnpm lockfiles, I've not seen keys like this previously (if I had, then I'd have already covered this case in one of the 2-3 times I've effectively rewritten a part of this parser for a new lockfile version 😅) and it's also in a semgrep fixture called "pnpm-error-key".

I think it is valid to call it with-invalid-path (or maybe with-invalid-package-path would be even better 🤔)

another-rex · 2024-10-18T00:10:48Z

FYI, if you are using osv-scalibr, the latest version (v0.1.3) has fixed this issue already.

We should still merge this fix in for osv-scanner

G-Rath · 2024-10-18T03:59:04Z

@another-rex actually shouldn't this be treated like #1139? Afaik pnpm will never generate this itself, even if it doesn't error (which I've not tested) - so if we consider this something we should fix, I'd say we should land #1139 too

another-rex · 2024-10-18T04:46:21Z

I think for both the expected behavior is to return an error instead of panicing when encountering an invalid lockfile (partially because if we are encountering an unexpected line, the other information we extract might not be correct either).

Looking at the implementation in this PR it seems like you are just ignoring the invalid package line when finding it, can you change it to return an error instead, thanks!

That should match pre-#1139 if I understand correcty.

ivmeta · 2024-10-18T16:48:42Z

Got it! Didn't know if you wanted to have partial results in response to a single invalid package path.

Added tests and changed the fixture / test methods names.

ps: Agree the file is invalid in the first place, but running Scalibr/osv-scanner against a filesystem with a semgrep checkout threw this panic and that was a bit of a bummer :)

pkg/lockfile/parse-pnpm-lock_test.go

Co-authored-by: Gareth Jones <Jones258@Gmail.com>

another-rex · 2024-10-22T23:20:35Z

Just need the CLA completed, and then this should be good to merge!

codecov-commenter · 2024-10-22T23:23:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.45%. Comparing base (e054385) to head (cfbc0f1).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1327      +/-   ##
==========================================
+ Coverage   68.43%   68.45%   +0.01%     
==========================================
  Files         183      183              
  Lines       17606    17615       +9     
==========================================
+ Hits        12049    12058       +9     
  Misses       4895     4895              
  Partials      662      662

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ivmeta · 2024-10-23T00:20:24Z

Just need the CLA completed, and then this should be good to merge!

Running some internal approval processes. As soon as that done on our side, I'll sign the CLA.

Apologies for the delay!

ivmeta added 2 commits October 17, 2024 11:16

Update parse-pnpm-lock.go

22be1d6

Added failing lockfile

dc1c5ca

G-Rath suggested changes Oct 17, 2024

View reviewed changes

Return error on invalid package path

79ed909

Fix go.mod golang version

ba76fb6

G-Rath approved these changes Oct 22, 2024

View reviewed changes

pkg/lockfile/parse-pnpm-lock_test.go Outdated Show resolved Hide resolved

Update pkg/lockfile/parse-pnpm-lock_test.go

23c7047

Co-authored-by: Gareth Jones <Jones258@Gmail.com>

Merge branch 'main' into remotes/origin/pnpm-parse-fix

cfbc0f1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: parsing crash on malformed pnpm lockfile #1327

fix: parsing crash on malformed pnpm lockfile #1327

ivmeta commented Oct 17, 2024

google-cla bot commented Oct 17, 2024

G-Rath Oct 17, 2024

G-Rath Oct 17, 2024

ivmeta Oct 18, 2024

G-Rath Oct 18, 2024

another-rex commented Oct 18, 2024

G-Rath commented Oct 18, 2024

another-rex commented Oct 18, 2024 •

edited

Loading

ivmeta commented Oct 18, 2024

another-rex commented Oct 22, 2024

codecov-commenter commented Oct 22, 2024 •

edited

Loading

ivmeta commented Oct 23, 2024

fix: parsing crash on malformed pnpm lockfile #1327

Are you sure you want to change the base?

fix: parsing crash on malformed pnpm lockfile #1327

Conversation

ivmeta commented Oct 17, 2024

Steps to reproduce it

google-cla bot commented Oct 17, 2024

G-Rath Oct 17, 2024

Choose a reason for hiding this comment

G-Rath Oct 17, 2024

Choose a reason for hiding this comment

ivmeta Oct 18, 2024

Choose a reason for hiding this comment

G-Rath Oct 18, 2024

Choose a reason for hiding this comment

another-rex commented Oct 18, 2024

G-Rath commented Oct 18, 2024

another-rex commented Oct 18, 2024 • edited Loading

ivmeta commented Oct 18, 2024

another-rex commented Oct 22, 2024

codecov-commenter commented Oct 22, 2024 • edited Loading

Codecov Report

ivmeta commented Oct 23, 2024

another-rex commented Oct 18, 2024 •

edited

Loading

codecov-commenter commented Oct 22, 2024 •

edited

Loading