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

Dependency confusion #147

Open
timholy opened this issue Feb 17, 2021 · 5 comments
Open

Dependency confusion #147

timholy opened this issue Feb 17, 2021 · 5 comments
Labels
URGENT Extremely high priority

Comments

@timholy
Copy link
Member

timholy commented Feb 17, 2021

@GunnarFarneback added new functionality to protect registries against a malicious attack via "dependency confusion". See details in JuliaRegistries/RegistryCI.jl#348. This registry has now been protected thanks to JuliaRegistries/General#30175. I have not yet taken the time to dig into this issue in detail, but we should check whether we need to reciprocate: do we need to make sure that General is protected against dependency confusion from this registry?

Since it's a potential security hole, this is a high-priority issue and should be tackled ASAP. @kdw503, can you take this on?

@timholy timholy added the URGENT Extremely high priority label Feb 17, 2021
@kdw503
Copy link
Contributor

kdw503 commented Feb 17, 2021

Ok, I will.

@kdw503
Copy link
Contributor

kdw503 commented Feb 17, 2021

As far as I understand until now, GunnarFarneback created PR in the RegistryCI.jl that can handle dependency confusion including public registries(for example HolyLabRegistry) when try to register new package to a certain registry(for example General) who is using automerge.yml. And, if the PR are merged, The registry who is using automerge.yml can protect the public registry by just add the public registry name to the public_registries field when calling RegistryCI.AutoMerge.run() in the automerge.yml file. And, ericphanson gracefully created PR adding our registry to the public_registries field in the General registry. And now, both PRs are merged and our registry is being protected from the dependency confusion by General registry. I think we don't need any further action about this issue except keeping our registry always be reachable. Is my understanding right? I wonder why they make the guideline check fail when a certain public registry is not reachable. That looks a little scary. If our registry is not reachable, no one can register new package to the General registry until our issue is fixed. Is my understanding right? I think it would be better just ignoring unreachable public registry. This means that the public registry is not being protected until the issue is resolved. Does it look possible?

@GunnarFarneback
Copy link

GunnarFarneback commented Feb 17, 2021

Yes, the issue goes both ways, with the difference that everybody uses General and anybody can register packages there.

Naturally, only people who have added HolyLabRegistry could possibly be affected by a dependency confusion there against General. The next requirement for a dependency confusion is that there is a UUID collision between packages in the registries. UUIDs don't collide by chance (sub-microscopic probabilities ignored), it either requires incompetence (copying UUID from somewhere else or not creating it randomly) or malice.

So let's assume a package is registered in HolyLabRegistry with a UUID that matches a package in General.

  • If they have different names, Pkg will notice and neither package can be installed. This is not a security problem, rather a denial of service than can be painful enough.
  • If they have the same name, Pkg will consider them the same and merge the revisions. This is a feature when it's actually the same package that has been dual-registered. However, if they are not the same, a user who updates the package may suddenly get a version from the wrong source, i.e. a dependency confusion.

To guard against this would either involve running RegistryCI before merging new packages into the registry or in some other way perform the check from JuliaRegistries/RegistryCI.jl#348 against General.

Finally a heads up. I'm going to send out some test balloons to verify that the dependency confusion detection really works, so don't worry if you see an attempt to register one of your packages in General, at least not if it's coming from me. Update: Test has been performed and was successful.

@GunnarFarneback
Copy link

If our registry is not reachable, no one can register new package to the General registry until our issue is fixed. Is my understanding right?

Correct. Not until it's reachable again or it has been removed from the AutoMerge configuration.

We don't know whether this is going to be a practical problem or not but for now we prefer to be alerted when it happens rather than not noticing that something's amiss with the detection.

@timholy
Copy link
Member Author

timholy commented Feb 18, 2021

You're right that since we control who can register packages here, General is in a sense protected against us...unless we fail to notice a malicious registration attempt. Just to be certain that doesn't happen, if that's not terribly difficult I think it would be nice to set up an automatic test on any new package registration here. But the problem will be solved as long as we are personally vigilant and check manually. The rate of new package registrations here is very low so this is not a major burden, but forgetting to check could be awful so the stakes are high.

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

No branches or pull requests

3 participants