-
-
Notifications
You must be signed in to change notification settings - Fork 551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
npm: detect UPM packages (Unity) #3597
base: develop
Are you sure you want to change the base?
Conversation
Unity uses npm packages to distribute various types of features and assets. See: https://docs.unity3d.com/Manual/upm-manifestPkg.html The manifest files are using the same name and syntax, but the content, naming convention and of course the upstream repository are not the same. This is quite annoying because a Unity `package.json` file is gonna be detected as a NPM package from the npmjs registry, but it's actually coming from Unity's repo, which makes a big difference. Indeed, since both Unity and NodeJS are using NPM and the same package.json files, malicious packages have been pushed to the npmjs registry with the name of Unity packages. For example the package: `com.unity.scriptablebuildpipeline` is perfectly fine in Unity (https://docs.unity3d.com/Packages/[email protected]/manual/index.html), but is a malware in npmjs (https://www.npmjs.com/package/com.unity.scriptablebuildpipeline). This means that if ScanCode is being used in a Unity codebase and malicious packages checks are being executed, then we end up with a LOT of scary false positives. See this advisory: https://github.com/ossf/malicious-packages/blob/cca311974602d1940fab6a98adba611b505cf27d/malicious/npm/com.unity.scriptablebuildpipeline/MAL-2022-2102.json#L13 When matching an inventory against a list of malicious packages, one must take into account the repository from where the packages were fetched, we can't just rely on a canonical purl. This PR changes the NPM code that parses `package.json` to detect when a file is from UPM and change the urls accordingly so that scanners can take better decisions. This is probably out of scope for this PR, but maybe we should consider creating a separate purl type for UPM since they don't really share anything with NPM, except the package manager (different languages, different packages, different repositories, different tools etc). Signed-off-by: Adrien Schildknecht <[email protected]>
Thanks!
This makes sense. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall. Just a few nits... Thanks
def get_urls(namespace, name, version, is_upm=False, **kwargs): | ||
if is_upm: | ||
return dict( | ||
repository_homepage_url='https://docs.unity3d.com/Manual/Packages.html', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some sorts home page for each package or package version? if not, this is best left empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schischi thanks for the PR!
See comments on whether this should be a new purl type. This should be discussed more.
@@ -0,0 +1,85 @@ | |||
[ | |||
{ | |||
"type": "npm", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schischi I'm not sure if we can use type as npm here, as these packages are not really present in the main package respository for npm, ie. at npmjs, see https://www.npmjs.com/search?q=com.unity.ml-agents which does not point to any package, so this does not really align with https://github.com/package-url/purl-spec/blob/master/PURL-TYPES.rst#npm possibly? Also from https://docs.unity3d.com/Manual/upm-manifestPkg.html:
The file’s format is similar to [npm](https://www.npmjs.com/)’s package.json format, but uses different semantics for some of its properties
So this is not really same as npm, just similar? Do you think this should be a new type in the purl-spec instead (like upm
instead of npm
)?
what do you think @pombredanne?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be a upm
or unity
thing. These are not npms for sure.
Unity uses npm packages to distribute various types of features and assets. See: https://docs.unity3d.com/Manual/upm-manifestPkg.html
The manifest files are using the same name and syntax, but the content, naming convention and of course the upstream repository are not the same.
This is quite annoying because a Unity
package.json
file is gonna be detected as a NPM package from the npmjs registry, but it's actually coming from Unity's repo, which makes a big difference.Indeed, since both Unity and NodeJS are using NPM and the same package.json files, malicious packages have been pushed to the npmjs registry with the name of Unity packages.
For example the package:
com.unity.scriptablebuildpipeline
is perfectly fine in Unity (https://docs.unity3d.com/Packages/[email protected]/manual/index.html), but is a malware in npmjs (https://www.npmjs.com/package/com.unity.scriptablebuildpipeline).This means that if ScanCode is being used in a Unity codebase and malicious packages checks are being executed, then we end up with a LOT of scary false positives. See this advisory: https://github.com/ossf/malicious-packages/blob/cca311974602d1940fab6a98adba611b505cf27d/malicious/npm/com.unity.scriptablebuildpipeline/MAL-2022-2102.json#L13
When matching an inventory against a list of malicious packages, one must take into account the repository from where the packages were fetched, we can't just rely on a canonical purl.
This PR changes the NPM code that parses
package.json
to detect when a file is from UPM and change the urls accordingly so that scanners can take better decisions.This is probably out of scope for this PR, but maybe we should consider creating a separate purl type for UPM since they don't really share anything with NPM, except the package manager (different languages, different packages, different repositories, different tools etc).
Fixes #0000
Tasks
Run tests locally to check for errors.