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

Don't split git references on unicode separators #9827

Merged
merged 2 commits into from
Apr 24, 2021

Conversation

pradyunsg
Copy link
Member

Previously, maliciously formatted tags could be used to hijack a
commit-based pin. Using the fact that the split here allowed for
all of unicode's whitespace characters as separators -- which git allows
as a part of a tag name -- it is possible to force a different revision
to be installed; if an attacker gains access to the repository.

This change stops splitting the string on unicode characters, by forcing
the splits to happen on newlines and ASCII spaces.

@pradyunsg pradyunsg added type: security Has potential security implications C: vcs pip's interaction with version control systems like git, svn and bzr labels Apr 24, 2021
@pradyunsg pradyunsg added this to the 21.1 milestone Apr 24, 2021
Previously, maliciously formatted tags could be used to hijack a
commit-based pin. Using the fact that the split here allowed for
all of unicode's whitespace characters as separators -- which git allows
as a part of a tag name -- it is possible to force a different revision
to be installed; if an attacker gains access to the repository.

This change stops splitting the string on unicode characters, by forcing
the splits to happen on newlines and ASCII spaces.
@pradyunsg pradyunsg force-pushed the fix-git-improper-tag-handling branch from d85a28c to 0e4938d Compare April 24, 2021 09:19
@pradyunsg
Copy link
Member Author

I've intentionally not added a test demonstrating the attack -- I'd like to do that after we have a release with the fix.

Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

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

LGTM

@sbidoul sbidoul merged commit e46bdda into pypa:main Apr 24, 2021
@pradyunsg pradyunsg deleted the fix-git-improper-tag-handling branch April 24, 2021 09:45
FasterSpeeding added a commit to hikari-py/hikari that referenced this pull request May 1, 2021
This avoids it erroring due to pypa/pip#9827
FasterSpeeding added a commit to hikari-py/hikari that referenced this pull request May 1, 2021
trbs added a commit to django-extensions/django-extensions that referenced this pull request May 2, 2021
@frenzymadness
Copy link
Contributor

Does this security vulnerability deserve a CVE number? Is there a plan to request one?

@m403
Copy link

m403 commented Jun 3, 2021

FYI - this issue was assigned CVE-2021-3572 by Red Hat, Inc.

@frenzymadness
Copy link
Contributor

I've intentionally not added a test demonstrating the attack -- I'd like to do that after we have a release with the fix.

Would it be possible to add a test now?

@frenzymadness
Copy link
Contributor

I've tried to describe my effort in the linked issue.

aalexanderr added a commit to aalexanderr/fetchcode that referenced this pull request Aug 23, 2021
WARNING: fetchcode's vcs will not work on this commit.
This is result of separating pip's code from changes made in scope of
this repository.

This should make it easier to track potentially replicated issues from
pip when taking their vcs pkg.

It also made cleaning up easier, due to some maintenance activities done
in pip:
- dropping Python 2 & 3.5 support
  pypa/pip#9189
- modernized code after above - partially done, tracked in:
  pypa/pip#8802
- added py3.9 support
- updated vendored libraries (e.g. fixing CVE-2021-28363)
  multiple PRs

pip._internal.vcs (and related code) changes:
- Fetch resources that are missing locally:
  pypa/pip#8817
- Improve SVN version parser (Windows)
  pypa/pip#8665
- Always close stderr after subprocess completion:
  pypa/pip#9156
- Remove vcs export feature:
  pypa/pip#9713
- Remove support for git+ssh@ scheme in favour of git+ssh://
  pypa/pip#9436
- Security fix in git tags parsing (CVE-2021-3572):
  pypa/pip#9827
- Reimplement Git version parsing:
  pypa/pip#10117

In next commits, most of pip's internals will be removed from fetchcode,
leaving only vcs module with supporting code (like utils functions,
tests (which will be added & submitted with this change))

This will allow for changes such as ability to add return codes
(probably via futures) from long running downloads and other features.

Switching to having own vcs module might also be a good call due to
pip._internal.vcs close integration with pip's cli in vcs module (some
pip code has been commented out in commit mentioned below)

While generally copy-pasting code without history is bad idea, this
commit follows precedence set in this repo by:
8046215
with exception that all changes to pip's code will be submitted as separate
commits.

It has been agreed with @pombredanne & @TG1999 that history from pip
will be rebased on fetchcode by @pombredanne (thanks!). It will be done
only for the files that are of concern for fetchcode to limit noise in
git history.

I'm leaving this commit without SoB intentionally, as this is not my
work, but that of the many pip's authors:
https://github.com/pypa/pip/blob/21.2.4/AUTHORS.txt
License of pip: MIT (https://pypi.org/project/pip/)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C: vcs pip's interaction with version control systems like git, svn and bzr type: security Has potential security implications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants