Nixpkgs: Switch p7zip to actively maintained fork
Issue description
The original P7zip was last updated over 3 years ago and has accrued security vulns in the meantime. It has been (correctly) marked as insecure in the package definition, but it would be of more use if it were replaced by a fork that fixed these problems. I suggest this one, as it seems to be the most actively maintained.
petersjt014
All 6 comments
Here's a diff between the latest release of p7zip and this fork, excluding the new bundled libraries (C/{fast-lzma2,lz4,zstd,zstdmt}), if anyone would like to take a look. I haven't pored over it in detail, but some notes:
This fork is linked from the 7-Zip site, so it's probably a reasonable canonical choice.
The bulk of the changes seem to be feature and documentation additions rather than refactors or security fixes.
It seems that the repository has commits identified as fixing CVE-2016-9296 and CVE-2018-5996, but nothing clearly marked for other vulnerabilities we have patches for. We should make sure it incorporates fixes for these before switching to it as a presumed-secure upstream; we can't stay on top of fixing and patching every single vulnerability that comes up ourselves, so having the known CVE patches incorporated seems like a reasonable baseline expectation.
The README doesn't mention the non-free licensing of the RAR code. Not really a big deal since this is a mess upstream too, just noting it.
I feel like the really big task in keeping p7zip updated will be keeping it up to date with 7-Zip upstream, which I assume it shares a lot of code with. It's not clear if there's been much thought put into that here; there have been 10 upstream 7-Zip releases since the last p7zip release so there's probably a significant amount of divergence already (3 years of work, probably including a lot of security fixes, compared to the ~month this fork has existed). Discussion in the thread about p7zip's fate seems to imply that the divergence is significant enough that it might be better to base a new p7zip on 7-Zip proper rather than the existing port.
Sorry for the negativity; these aren't intended to block this, just note down the concerns I'd have before committing to a fork. It would be great if we had a secure, updated p7zip package, since a lot of people are used to the interface, it has wide format support, and some software uses it internally. But from having skimmed over the code a bit and seen the existing vulnerabilities I know it'll be a big task to keep it updated and free of holes, and I think we should apply due diligence here given the liability it's been. I'd feel more comfortable re-enabling it once it's removed from builds (see e.g. #87837) and runtime dependencies where possible to reduce the general exposure.
emilazy
on 19 May 2020
Sorry for the negativity
No problem, this was going to be messy no matter what.
I'd agree that upstreaming the nixpkgs fixes should be first priority, and that a big 7zip --> p7zip refactor should be farther down the road. In the meantime, do you think we could simply switch to the new fork while keeping the patches in the build process? It look like that we'd have at least a few less vulns, which would be useful even if it's still marked as insecure since some people/processes are still using it anyway.
Do you think that can that be done now, or does it need to wait for #87837?
also: cc @7c6f434c (Michael Raskin), the maintainer.
petersjt014
on 20 May 2020
We have 4 CVE patches. There is a changelog that claims to apply some patches for 3 CVEs, and the last patch is defacto applied. Are there any more CVE patches we know of?
7c6f434c
on 20 May 2020
Here are CVE lists for 7zip and pzip.
The nixfile's four patches are:
- CVE-2016-9296
- CVE-2017-17969
- CVE-2018-5996
- CVE-2018-10115
Szcnick's fork seems to have all 4, although 2018-10115 is only noted here (also CVE-2015-1038 is listed under 15.14 in the changelog).
Assuming Szcnick's fixes are good (and that 2015-1038 is a non-issue, which I think it is), switching now would provide equal security plus zstd support and the other new features.
Going off the 7zip list, I _think_ this is what we got so far:
CVE | has patch | last vuln | note
--|--|--|--
CVE-2008-6536 | | 4.5.7
CVE-2015-1038 | y
CVE-2016-2334 | | 16.00
CVE-2016-2335 | | 15.05
CVE-2016-7804 | | | windows-only
CVE-2016-9296 | y
CVE-2017-17969 | y
CVE-2018-5996 | y
CVE-2018-10115 | y
CVE-2018-10172 | | | windows-only
Edit: I just found this list, which contains several others. I haven't yet found any patches for them, although I also don't know if they're relevant.
petersjt014
on 22 May 2020
This issue has been mentioned on NixOS Discourse. There might be relevant details there:
https://discourse.nixos.org/t/p7zip-and-possible-rces/6951/16
nixos-discourse
on 8 Jun 2020
Let's hope everything goes well…
7c6f434c
on 11 Jun 2020
Related issues
langston-barrett
·
3Comments
sid-kap
·
3Comments
copumpkin
·
3Comments
chris-martin
·
3Comments
ghost
·
3Comments
Most helpful comment
Here's a diff between the latest release of p7zip and this fork, excluding the new bundled libraries (
C/{fast-lzma2,lz4,zstd,zstdmt}), if anyone would like to take a look. I haven't pored over it in detail, but some notes:This fork is linked from the 7-Zip site, so it's probably a reasonable canonical choice.
The bulk of the changes seem to be feature and documentation additions rather than refactors or security fixes.
It seems that the repository has commits identified as fixing CVE-2016-9296 and CVE-2018-5996, but nothing clearly marked for other vulnerabilities we have patches for. We should make sure it incorporates fixes for these before switching to it as a presumed-secure upstream; we can't stay on top of fixing and patching every single vulnerability that comes up ourselves, so having the known CVE patches incorporated seems like a reasonable baseline expectation.
The README doesn't mention the non-free licensing of the RAR code. Not really a big deal since this is a mess upstream too, just noting it.
I feel like the really big task in keeping p7zip updated will be keeping it up to date with 7-Zip upstream, which I assume it shares a lot of code with. It's not clear if there's been much thought put into that here; there have been 10 upstream 7-Zip releases since the last p7zip release so there's probably a significant amount of divergence already (3 years of work, probably including a lot of security fixes, compared to the ~month this fork has existed). Discussion in the thread about p7zip's fate seems to imply that the divergence is significant enough that it might be better to base a new p7zip on 7-Zip proper rather than the existing port.
Sorry for the negativity; these aren't intended to block this, just note down the concerns I'd have before committing to a fork. It would be great if we had a secure, updated p7zip package, since a lot of people are used to the interface, it has wide format support, and some software uses it internally. But from having skimmed over the code a bit and seen the existing vulnerabilities I know it'll be a big task to keep it updated and free of holes, and I think we should apply due diligence here given the liability it's been. I'd feel more comfortable re-enabling it once it's removed from builds (see e.g. #87837) and runtime dependencies where possible to reduce the general exposure.