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

Fix git submodule usage in the testsuite #11541

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

Conversation

cboylan
Copy link
Contributor

@cboylan cboylan commented Oct 25, 2022

Git patched a security issue (CVE-2022-39253) which prevents the use of local file pathed submodules by default. The reason for this is that untrusted submodules could be abused to expose file system contents.

The pip test suite tests handling of git submodules and to do so creates a git repo which it then uses as a submodule. This was failing due to git's stricter rules on acceptable submodules. Thankfully, pip creates and controls the content of this git repo which means pip can trust it. To express that to git we pass -c protocol.file.allow=always to the git submodule add command which overrides the default behavior.

This fixes #11540

Git patched a security issue (CVE-2022-39253) which prevents the use of
local file pathed submodules by default. The reason for this is that
untrusted submodules could be abused to expose file system contents.

The pip test suite tests handling of git submodules and to do so creates
a git repo which it then uses as a submodule. This was failing due to
git's stricter rules on acceptable submodules. Thankfully, pip creates
and controls the content of this git repo which means pip can trust it.
To express that to git we pass `-c protocol.file.allow=always` to the
git submodule add command which overrides the default behavior.

This fixes pypa#11540
@@ -0,0 +1,2 @@
Fix pip's submodule usage in the test suite to accomodate Git updates
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a trivial news entry.

@cboylan
Copy link
Contributor Author

cboylan commented Oct 25, 2022

My local git hasn't updated yet and while this PR does fix the first level of issue previously exposed it doesn't fix the new problem this exposes. The problem here is https://github.com/pypa/pip/blob/main/src/pip/_internal/vcs/git.py#L490-L493 is running submodule update during packages installs and that could potentially run against untrusted submodules. I'm not sure what the correct way to address this is within pip.

@uranusjr
Copy link
Member

uranusjr commented Oct 26, 2022

When building from VCS, the user is implicitly trusting the source tree (since the build script is free to do anything when the tree is built anyway), so IMO it’s acceptable to simply trust the file protocol there.

@cboylan
Copy link
Contributor Author

cboylan commented Oct 26, 2022

When building from VCS, the user is implicitly trusting the source tree (since the build script is free to do anything when the tree is built anyway), so IMO it’s acceptable to simply trust the file protocol there.

I'm unwilling to make that assertion myself simply because I am not familiar enough with the use case here. For example, I'm not sure it is true that the submodule has any code executed in it during the pip install process. The top level repo may have its setup.py script run, and in that case you definitely have to trust the top level repo.

But is that true of the submodule? Eventually, if you run the software that has been installed then you are theoretically trusting the submodule if it is part of the final package contents. What about the case where pip install fails to successfully install a package? If you are exposed to a git vulnerability then that would be sufficient whereas before it would've required you to execute the installed code in a subsequent step?

Whatever decision is made should probably be made by someone other than myself. Feel free to update this PR if you like or we can close this one in favor of a replacement if that is easier.

@pradyunsg
Copy link
Member

As someone who hasn't even looked at the vulnerability and is not a git submodule user, is there any reason to not just set the git config globally in our CI, where we know its OK for us to do so safely?

@pradyunsg
Copy link
Member

#11547 does what I suggested above.

@uranusjr
Copy link
Member

uranusjr commented Oct 26, 2022

I just tried to globally set protocol.file.allow to always but that didn’t seem to work. Not sure if I’m missing something (additional restrictions regarding git submodule?) or there’s something specific to GitHub Actions. Further experiements will happen in #11551.

@pfmoore
Copy link
Member

pfmoore commented Oct 27, 2022

I've no idea what's going on with the git CVE, but I'm a bit concerned that we're simply patching over the testsuite issue here. Is there a risk that this change in git might be a problem for pip users? Do we have any idea how people use the submodule functionality in pip? As I said somewhere else, I've no understanding of why pip has specialised code for submodules, but do we know what the use case is for that code? Is it more or less likely to be used with local submodules than the average git usage of submodules?

My concern here is that we end up no longer supporting file URLs in submodules, so we've in effect desupported a previously-working usage, with no warning and no mitigation (short of "downgrade git"). That's not our fault, except in the sense that if there is a way to say "this usage is OK" to git, we can't find it and hence can't advise our users.

I don't have any good answers here, other than to suggest that when we fix the testsuite, we don't mark the fix as "skip news", but rather include a news entry that explains what git did, and what we had to do as a result. That way, our users at least have a point of reference they can work from.

@uranusjr
Copy link
Member

Instead of trying to make file: work in CI, I decided to take an alternative route, switching the test to use another protocol instead. It turns out serving git: is relatively simple.

The PR is available in #11554. This feels like a better fix to me. As a bonus, the test should now run on Windows since git: uses a URL and the path difference is no longer an issue.

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.

Pip test suite submodule handling broken after Git update
4 participants