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.

Sign up for GitHub

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

Jump to bottom

python27: fix CVE-2021-23336 #118403

Merged
merged 1 commit into from
Apr 12, 2021
Merged

python27: fix CVE-2021-23336 #118403

merged 1 commit into from
Apr 12, 2021

Conversation

dotlambda
Copy link
Member

Motivation for this change

fixes #116917

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@dotlambda
python27: fix CVE-2021-23336
fa410ea 
From the archive `python-gentoo-patches-2.7.18_p8.tar.xz` found at
https://dev.gentoo.org/~mgorny/dist/python/, I copied
`0024-3.6-bpo-42967-only-use-as-a-query-string-separator-G.patch`.
@dotlambda dotlambda added 1.severity: security Issues which raise a security issue, or PRs that fix one 9.needs: port to stable A PR needs a backport to the stable release. labels Apr 3, 2021
@dotlambda dotlambda requested review from mweinelt and FRidh April 3, 2021 13:20
@mweinelt
Copy link
Member

mweinelt commented Apr 3, 2021 •
edited
Loading

What is the source of the patchset? Is it python/cpython#24297? Looks like https://gitweb.gentoo.org/fork/cpython.git/commit/?h=gentoo-2.7-vanilla&id=255d1d464a7f45a20986aa014c78e85ef47e6591 in fact.

@mweinelt
Copy link
Member

mweinelt commented Apr 3, 2021

Anyway, not sure if this needs to go to staging, as we don't recurse into python27Packages for a while now.

@dotlambda
Copy link
Member Author

What is the source of the patchset?

See the commit message.

@dotlambda
Copy link
Member Author

dotlambda commented Apr 3, 2021 •
edited
Loading

Anyway, not sure if this needs to go to staging, as we don't recurse into python27Packages for a while now.

Nixpkgs-review listed more than 2000 packages. I guess most of these are only dependencies though.

@FRidh
Copy link
Member

FRidh commented Apr 3, 2021

This is an API change. As is argued in https://bugs.python.org/issue42967, this should not have been merged into CPython (and I agree). By now, "the damage is done". It may cause some breakage with some other downstream packages. Be prepared to patch up failing packages that are still in use.

I won't merge this, but won't block it either.

SuperSandro2000
SuperSandro2000 reviewed Apr 3, 2021
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

Be prepared to patch up failing packages that are still in use.

Python2 package are in a broken state anyway and regularly break üackages that still rely on them like nixops. Also from the issue it is not clear what will be broken.

@dotlambda
Copy link
Member Author

I can confirm that nixops still builds.

@dotlambda
Copy link
Member Author

Even though I agree with @FRidh's assessment that upstream shouldn't have merged the patch, I believe that we should go with (I believe) the majority of Linux distributions and apply this patch. We don't want our users to come across an unexpected difference to other distros.
If anyone agrees, please go ahead and merge.

@AndersonTorres
Copy link
Member

IF the upstream merged this, can we get the upstream version then, instead of a specific distro patch?

@dotlambda
Copy link
Member Author

dotlambda commented Apr 11, 2021 •
edited
Loading

Upstream only applied this patch to versions of Python that are still supported. Python 2.7 is not.

@AndersonTorres
Copy link
Member

The other distros are using it.
The patch does not hurt anyone, and even better, it enhances security.
So the best course of action is to use it.
On the other hand, if py27 is abandonwared, we should do the same.

@AndersonTorres AndersonTorres merged commit fea3171 into NixOS:staging Apr 12, 2021
@mweinelt
Copy link
Member

On the other hand, if py27 is abandonwared, we should do the same.

People keep python2.7 alive for example for NixOps. 😞

@7c6f434c
Copy link
Member

7c6f434c commented Apr 12, 2021 via email

@dotlambda dotlambda deleted the CVE-2021-23336 branch April 13, 2021 07:41
@AndersonTorres
Copy link
Member

Can NixOps be rewritten in Py3?

@dotlambda
Copy link
Member Author

Can NixOps be rewritten in Py3?

See NixOS/nixops#1242.

@AndersonTorres
Copy link
Member

AndersonTorres commented Apr 19, 2021 •
edited
Loading

On the other hand, if py27 is abandonwared, we should do the same.

People keep python2.7 alive for example for NixOps.

Just to make it clear: I am not saying that we will drop the Py27 files now. We will just not care for them, at least not with the same motivation as before. After all, we erase nothing from a Git repo.

@TredwellGit TredwellGit removed the 9.needs: port to stable A PR needs a backport to the stable release. label Aug 20, 2021
@FliegendeWurst FliegendeWurst mentioned this pull request Jan 20, 2022
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@mweinelt mweinelt mweinelt approved these changes

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
1.severity: security Issues which raise a security issue, or PRs that fix one 6.topic: python 10.rebuild-darwin: 501-1000 10.rebuild-darwin: 501+ 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dotlambda @mweinelt @FRidh @AndersonTorres @7c6f434c @SuperSandro2000 @TredwellGit