Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

npm: detect UPM packages (Unity) #3597

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

schischi
Copy link
Contributor

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/com.unity.scriptablebuildpipeline@1.21/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

  • Reviewed contribution guidelines
  • PR is descriptively titled 馃搼 and links the original issue above 馃敆
  • Tests pass -- look for a green checkbox 鉁旓笍 a few minutes after opening your PR
    Run tests locally to check for errors.
  • Commits are in uniquely-named feature branch and has no merge conflicts 馃搧
  • Looked for possible updates in documentation and added updates if applicable
  • Updated CHANGELOG.rst

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/com.unity.scriptablebuildpipeline@1.21/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 <adrs@fb.com>
@pombredanne
Copy link
Member

Thanks!

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

This makes sense.

Copy link
Member

@pombredanne pombredanne left a 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',
Copy link
Member

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

Copy link
Member

@AyanSinhaMahapatra AyanSinhaMahapatra left a 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",
Copy link
Member

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鈥檚 format is similar to [npm](https://www.npmjs.com/)鈥檚 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?

Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants