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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter binaries that are installed to node_modules/.bin to avoid collisions with system-wide binaries #3610

Open
1 task done
webstrand opened this issue Jul 21, 2021 · 7 comments

Comments

@webstrand
Copy link
Contributor

webstrand commented Jul 21, 2021

Describe the user story

Allowing dependencies to add arbitrarily named binaries to node_modules/.bin leaves users vulnerable to conflicts between the binaries installed to node_modules/.bin and system-wide binaries.

For example a malicious package installing sudo could be used to harvest passwords when accidentally executed by a benign package script like "serve": "sudo nodemon ./app.js". Some users even add node_modules/.bin to their shell session's PATH for the convenience of avoiding pnpx everywhere.

I am specifically motivated by discovering that some system-provided binaries that I rely on could collide with binaries provided by dependencies, for example: foreground (execline) might collide with the binary foreground (foreground). On my system, I counted 345 potential collisions between system-wide binary names and those provided by various NPM packages.

Describe the solution you'd like

I want to be able to restrict which binaries get installed into node_modules/.bin. I propose adding a "installBins" property to package.json that specifies which binaries get installed per package.

{
    "installBins": {
        // Limit the installed binaries to ones that are members of the array
        "package-name-a": [ "foo", "bar" ],
        
        // Limit the installed binaries to ones that match the RegExp
        "package-name-b": "^(foo|bar)$",
        
        // Wildcard rule that matches all packages not otherwise specified
        "*": [ "baz" ]
    }
}

A binary defined by a package may only be installed if:

  • The package is listed by name in installBins and the rule matches the binary.
  • There is a wildcard rule "*" and that rule matches the binary.
  • There is not a wildcard rule; all binaries from packages not listed are installed in the absence of a wildcard rule.

A rule matches a given binary name when:

  • The rule is an array, the binary name matches one of the array members exactly.
  • The rule is a string, the binary name matches RegExp(rule).

The rules defined in the root package only affect binaries installed in $root/node_modules/.bin and do not affect binaries that get installed in $dependencyDir/node_modules/.bin. However, every dependency, direct or transitive, would filter its own $dependencyDir/node_modules/.bin by its own installBins rules when installed.

Describe the drawbacks of your solution

  • This mostly prevents innocuous name collisions. Malicious packages can use other avenues of attack to capture passwords and attack systems.
  • Care must be taken that a package that uses installBins doesn't break when installed as a dependency by another package manager. Although I hope to convince other package managers to adopt this convention.
  • It's inconvenient to exclude a single binary without specifying every other binary. A RegExp like /^(?!bar)(.*)$/ will match every binary except for bar. The array syntax could be extended such that ["*", "!bar"] behaves the same as the former RegExp, but I am unconvinced.
  • Unclear how it should interact with a flattened node_modules, since there's one merged .bin for all of the dependencies. Merging the installBins rules for all of the dependencies could be a costly operation.

Describe alternatives you've considered

  • Inverting the rules such that only binaries that match the rule get filtered out. This would make it more convenient to filter out specific binaries, but isn't as convenient for my use-case: filtering out unexpected binaries.
  • Filtering rules could be put inside of another file, instead of package.json. But packages shouldn't behave differently depending on whether they're top-level or installed as a dependency. So these rules ought to be part of the package definition.
  • Use --no-bin-links and manually install the necessary binaries via a post-install hook. I'd have to switch away from pnpm, and this doesn't help when the package is installed as a dependency.
  • Add a flag --prefer-system-path that causes pnpm to append node_modules/.bin to the PATH rather than prepending it when running scripts.
  • Add a flag --warn-on-binary-collision that causes pnpm to warn or prompt for user interaction when it detects that a binary would collide with a system-provided binary.
@webstrand
Copy link
Contributor Author

Related issue: #1488 Although the proposed solution of adding a property to dependenciesMeta would affect the entire workspace rather than just the individual package

@zkochan
Copy link
Member

zkochan commented Jul 25, 2021

Wouldn't just a simple allow list be sufficient?

{
  "linkBins": ["foo", "bar"]
}

@webstrand
Copy link
Contributor Author

webstrand commented Jul 25, 2021

If you think being able to configure individual packages is overly complex, I could live with that simplified version. It doesn't solve the issue of picking which package gets to install a specific bin, in the case of conflicts. In which case this'd no longer be a solution for #1488.

We could specify that { "linkBins": ["foo", "bar"] } is an equivalent shorthand for

{
    "linkBins": {
        "*": ["foo", "bar"]
    }
}

if you're just worried that the syntax is overly complex for common use cases?

We'd also need regex syntax:

{
    "linkBins": "^(?!bar$).*"
}

if we wanted to retain the ability to exclude specific bins, but accept everything else.

In my opinion, I'd like to retain the ability to specify which packages install which binaries.

@zkochan
Copy link
Member

zkochan commented Jul 25, 2021

I have created an RFC in the Yarn repo yarnpkg/rfcs#118. The consensus was that it is not the package manager's problem if there are conflicting bins.

Regarding regex support, if it is only needed for excluding, I would probably suggest just a dedicated field for that.

{
    "ignoreBins": ["bar"]
}

@zkochan
Copy link
Member

zkochan commented Jul 25, 2021

cc @pnpm/collaborators

@javier-garcia-meteologica
Copy link
Contributor

Specifying which binaries get installed per package would be a great security improvement. For package aliases, linkBins would use the alias instead of the package name, right?

pnpm add foo@npm:bar@2
{
    "linkBins": {
        "foo": ["bar"]
    }
}

Using a separate field for linked and ignored bins reminds me of typescript include and exclude configuration options. Specifying an includelist and then a excludelist on top provides flexibility and reliability (what is excluded is guaranteed to be excluded).

I'm interested in how a name conflict would be resolved with this proposal. Let's say there is a conflict with the bin utilities named foo and bar between 2 packages.

# bins of pkg-a
org-x org-y org-z foo bar

# bins of pkg-b
foo bar

I want to pick org-x, org-y and org-z from pkg-a; and foo and bar from pkg-b. Which configurations would be valid?

{
    "linkBins": {
        "pkg-a": ["org-x", "org-y", "org-z"],
        "pkg-b": ["foo", "bar"]
    }
}
{
    "linkBins": {
        "pkg-a": ["org-.*"],
        "pkg-b": ["*"]
    }
}
{
    "linkBins": {
        "pkg-a": ["^(?!foo|bar$)"],
        "pkg-b": ["*"]
    }
}
{
    "ignoreBins": {
        "pkg-a": ["foo", "bar"]
    }
}
{
    "ignoreBins": "^pkg-a\\.(?:foo|bar$)"
}

@zkochan
Copy link
Member

zkochan commented Jul 26, 2021

This should be discussed with npm and Yarn teams as this will influence how depednencies are installed (if they are published with these new fields). So an RFC should be created in this repository: https://github.com/npm/rfcs

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

No branches or pull requests

3 participants