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

Secure NPM #94

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Secure NPM #94

wants to merge 3 commits into from

Conversation

wilk
Copy link

@wilk wilk commented Jul 16, 2018

No description provided.

@wilk
Copy link
Author

wilk commented Jul 16, 2018

Refs to: yarnpkg/yarn#1169

@arcanis
Copy link
Member

arcanis commented Jul 16, 2018

Hey! Thanks, I have a few comments

  • Is it a theoretical issue, or are you aware of attacks that could have been avoided by your measures?

  • Do you suggest that the registry should run a build (ie untrusted code) at each push? Aren't you worried that it requires a heavy infrastructure and proper security measures? Wouldn't that make the registry itself more vulnerable to getting hacked?

  • What prevents me from simply removing the commit from Github after its associated release has been published?

  • What prevents me from running the build steps myself before adding the generated (and minified, and possibly tampered) file to the repository? How would your idea help with that case?

  • If Github goes dark, what's the fallback? Similarly, have you considered alternative strategies when the users don't want to use Github? How would that work with people using workspaces?

  • How would it work with closed-source packages?

  • Before anything like this can be implemented into Yarn, the registry has to implement it first.

Overall I personally don't think this solves enough problem to be worth the added complexity.

@wilk
Copy link
Author

wilk commented Jul 16, 2018

@arcanis

Is it a theoretical issue, or are you aware of attacks that could have been avoided by your measures?

This PoC is an improvement for facing issues like this and this.
It does not guarantee that no malicious code will be published any more but it guarantees a match between the published build and the public source code: thus it allows users to verify the source code online.

Do you suggest that the registry should run a build (ie untrusted code) at each push? Aren't you worried that it requires a heavy infrastructure and proper security measures? Wouldn't that make the registry itself more vulnerable to getting hacked?

If the build is sand-boxed (for instance on docker on a separate CI server), then there shouldn't be no threat for the registry.

What prevents me from simply removing the commit from Github after its associated release has been published?

Nothing but then you'll become an untrusted author.
It may work once but then the community will be aware of your actions.
Then, you could be blacklisted by the community and the registry itself.

What prevents me from running the build steps myself before adding the generated (and minified, and possibly tampered) file to the repository? How would your idea help with that case?

The public repository holds only the source code.
The build is what the author wants to publish on the registry.
However, the author should not publish the build directly any more. Instead, the registry has to do it for the author.
This procedure is well explained inside the linked article.

If Github goes dark, what's the fallback? Similarly, have you considered alternative strategies when the users don't want to use Github? How would that work with people using workspaces?

Github is a placeholder for a generic public repository.
There should be a list of trusted public repos, like Github.
I didn't understand the question about workspaces: does it replace the repository?

How would it work with closed-source packages?

It wouldn't.
Closed-source packages should be marked as "closed" or "proprietary" and the consumer should be informed about it during the installation.
Then, the responsibility is upon the consumer.

Before anything like this can be implemented into Yarn, the registry has to implement it first.

So, yarn do not really publish anything on any registry?

Overall I personally don't think this solves enough problem to be worth the added complexity.

Fair enough.
However, the current situation force consumers to trust untrusted authors and unless something big happens, everything will work fine.
This is a short-term situation: personally, I think it's about "when" not "if".

@arcanis
Copy link
Member

arcanis commented Jul 16, 2018

This PoC is an improvement for facing issues like this and this.

Unless I misunderstood something, the worm was only detected because it was bogus, not because someone actually checked the source code. Your proposal would help a bit, but only because the maintainers could see more easily that something has been pushed on their repository than on their npm releases. But that's something that should be fixed on the registry side imo.

If the build is sand-boxed (for instance on docker on a separate CI server), then there shouldn't be no threat for the registry.

That's a big if. A sandbox must be maintained to be kept secure, which requires resources. Just imagine if a zero-day appeared in Docker, just imagine the impact such an issue could have (granted, it would cause issues at big scale, not only on the npm registry). All published packages could be tampered and include a worm. And since the users would trust the github source code (at first), it would delay the time needed for the bug to be noticed.

However, the author should not publish the build directly any more.

What if they do? You're fighting against an ecosystem, so you need to be prepared to have an answer. I can see multiple reasons why someone would want not to bother with a delegated build system.

So, yarn do not really publish anything on any registry?

We do publish, but if the registry doesn't implement your "please build the package from my repository" spec, I don't see what point there would be implementing something on our side.

This is a short-term situation: personally, I think it's about "when" not "if".

I agree, but I don't think this proposal will prevent this situation.

@Daniel15
Copy link
Member

Daniel15 commented Jul 16, 2018

Overall this sounds somewhat similar to how Packagist (PHP) works. With Packagist, the only way to push a package is by tagging your repo with a version number, and then their server pulls from your repo and produces a tarball. However, it works in PHP because there's no compilation step, so the code in the repo is exactly the code that's executed. That's not the case with a lot of modern JavaScript, which unfortunately requires a compilation step before execution.

I'm not quite sure I understand the benefit of pushing the compiled code to a Git repo. Now instead of creating a tarball of arbitrary code, you're just pushing that arbitrary code into a Git repo before packaging? I don't see how that's more secure - it's still arbitrary code being packaged. It won't prevent someone from hiding malicious code somewhere.

So, yarn do not really publish anything on any registry?

Yarn uses the npm registry; we don't have a separate registry.

@ljharb
Copy link

ljharb commented Jul 16, 2018

Additionally, there's no requirement that any npm package have any JavaScript in it at all.

@wilk
Copy link
Author

wilk commented Jul 17, 2018

@arcanis

Unless I misunderstood something, the worm was only detected because it was bogus, not because someone actually checked the source code. Your proposal would help a bit, but only because the maintainers could see more easily that something has been pushed on their repository than on their npm releases.

This is how Open Source Software works.

But that's something that should be fixed on the registry side imo.

How?
It'd amazing!

That's a big if. A sandbox must be maintained to be kept secure, which requires resources. Just imagine if a zero-day appeared in Docker, just imagine the impact such an issue could have (granted, it would cause issues at big scale, not only on the npm registry). All published packages could be tampered and include a worm.

All systems can be hacked. Even hardware.
But we cannot not doing things because they can be hacked.

And since the users would trust the github source code (at first), it would delay the time needed for the bug to be noticed.

This is the current situation.

What if they do? You're fighting against an ecosystem, so you need to be prepared to have an answer.

I explained myself badly.
Let me rephrase it: authors may put their build onto Github but it doesn't matter. Authors must provide source code along, otherwise it's a closed source package.

I can see multiple reasons why someone would want not to bother with a delegated build system.

The point is: if an author is publishing a package, the registry cannot trust him just because it's easier to do.
This is not a solution.

We do publish, but if the registry doesn't implement your "please build the package from my repository" spec, I don't see what point there would be implementing something on our side.

That's right, then this PR is non-sense here on Yarn 👍

I agree, but I don't think this proposal will prevent this situation.

As I said, this is not the solution: it's just an improvement to stay closer to the Open Source specs and thus an improvement to the security (community-watched).

@Daniel15

May I suggest you to read (or to read again) the article I've linked above?
The procedure is clear and there's no compiled code pushed on any public repos.

@ljharb

Additionally, there's no requirement that any npm package have any JavaScript in it at all.

And this is a reason more to implement something like SNPM and to separate closed source packages from open source ones.

Frankly, I think NPM's consumers (yes, me included) trust NPM just for convenience and the status quo will be kept until a sever major security issue happens.

@wilk wilk closed this Jul 18, 2018
@wilk wilk reopened this Jul 18, 2018
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.

4 participants