Incorrect BOM when private repos are added

[erichs]

Related to ShiftLeftSecurity/sast-scan#291,

I have attempted to document my finding here: https://github.com/erichs/testdependency-resolution

[prabhu]

@erichs This is good insight. What is happening is the ordering of searches performed by cdxgen and the presence of node_modules directory. If you remove node_modules directory and run cdxgen it will only use the package-lock.json or yarn.lock. If there is a node_modules folder, then cdxgen gives importance to node_modules folder. Why? With node.js, the presence of a vulnerable library inside node_modules is often enough to exploit the application especially with Remote Code Execution or Template Injection vulnerability. Unless, the application uses a packing tool such as webpack or esbuild with the correct tree shaking features it is a good idea to report all versions of a package found inside node_modules.

Hope this helps.

[erichs]

If there were indeed vulnerable modules versions present in the node_modules folder, I would absolutely agree with you: install-time hooks could be abused to perform RCE, maliciously modify code/templates, etc. I tried to show above that the vulnerable package versions found in the BOM are NOT present in the node_modules folder at all:

❯ find . -name lodash
./node_modules/@types/lodash
./node_modules/lodash

❯ jq .version node_modules/lodash/package.json
"4.17.21"

In the example repo above, npm/yarn are satisfying a transitive dep declaration like lodash: ^4.17.11 (vulnerable) with the "hoisted/deduped" package version of 4.17.21 (not vulnerable). Despite the fact that the lockfile deep inside node_modules actually says version: 4.17.11 for lodash, the vulnerable package was never installed, and doesn't exist in the node_modules folder.

This should be orthogonal to tree-shaking/packing.

I think it is actually incorrect to rely on the lockfiles inside of node_modules folders for BOM representation because of this hoisting/deduping mechanism both package managers employ: only the top-level lockfile is actually the source-of-truth for what got locally resolved/installed.

Happy to hop on a screenshare to demo this behavior, if that helps!

[erichs]

Upon further thought, I'm not fully confident in my claim that:

only the top-level lockfile is actually the source-of-truth for what got locally resolved/installed

I think it might be necessary to actually visit the modules on disk (when present) to collect an accurate BOM.

(over)-simplifying: find . -name package.json -exec jq '"\(.name)@\(.version)"' {} \;

[prabhu]

Sure, let's setup a zoom session to go through this. Can you email me at prabhu @ shiftleft. io

[erichs]

Perfect, thanks!

[prabhu]

@erichs I've made several changes to the nodejs portion. Could you retest with the latest version and let me know how it looks?

[erichs]

@prabhu sorry for the delayed response, here. I retested with the isolated problematic yarn.lock file here, and got clean results:

~/repos/jupiterone/testdependency-resolution main*
❯ cdxgen --version                           
3.0.6

testdependency-resolution git/main*  

~/repos/jupiterone/testdependency-resolution main*
❯ cdxgen -r -o bom-yarn.json                 
BOM file written to bom-yarn.json

testdependency-resolution git/main*  9s

~/repos/jupiterone/testdependency-resolution main* 9s
❯ grep 'pkg:npm/lodash@' bom-yarn.json
      "purl": "pkg:npm/lodash@4.17.21",
      "bom-ref": "pkg:npm/lodash@4.17.21"

Version 3.0.6 appears to have fixed this issue. 🥇 Thank you!

[prabhu]

Awesome! Thank you for re-testing.