Based on strace and brief source reading, I have come to conclusion that distfile integrity check and unpack is not atomic operation. This may allow attack, when distfile is changed after checksums were verified, but before distfile is unpacked. This implies, that to ensure security, DISTDIR must be trusted location. But, based on my experience of long time gentoo user, this does not seem to be sufficiently documented. Because checksums are verified during every package merge, some users may fall into false sense of security and put DISTDIR on network share. Another issue is, that even if DISTDIR is local, emerge does not check permissions to ensure that distfile is not writable by other users than portage.
(Manually expanded the portage team members to workaround the BZ limitation wrt aliases.) zac: would you mind taking a look? IIRC you handled a bunch of TOCTOU stuff in the past but it was before my time
(In reply to klamp from comment #0) > This implies, that to ensure security, DISTDIR must be trusted location. As discussed in bug 149062, this applies to essentially any directory where portage stores data, including PKGDIR, PORTAGE_TMPDIR/portage, CCACHE_DIR, and repository locations referenced by repos.conf. > Another issue is, that even if DISTDIR is local, emerge does not check > permissions to ensure that distfile is not writable by other users than > portage. Permission checks for all of these directories would definitely improve security. As it is, portage will emit a warning about the portage group in cases when the current operation requires privileges to one of these directories and the current user doesn't have sufficient privileges: https://gitweb.gentoo.org/proj/portage.git/commit/?id=fd76b435d5ce953e91a0efa8f6706aa3bd505f62
> Because checksums are verified during every package merge, some users may fall into false sense of security and put DISTDIR on network share. Having DISTDIR on a network share doesn't necessarily make it "untrusted". It really depends on who has write access to the network share.
(In reply to Zac Medico from comment #2) > (In reply to klamp from comment #0) > > This implies, that to ensure security, DISTDIR must be trusted location. > > As discussed in bug 149062, this applies to essentially any directory where > portage stores data, including PKGDIR, PORTAGE_TMPDIR/portage, CCACHE_DIR, > and repository locations referenced by repos.conf. Problem is that with those other directories it falls into "common sense", maybe except PKGDIR if packages can be and are crypto signed. But DISTDIR is specifically used to store files which are cryptographically verified before use, which may lead users to believe that it is safe to use not trusted location. Recommend way to setup distfile cache is via apt-cacher [1], but users attempted network share too [2]. I suspect that average user is not aware that there exist chain of trust between gentoo system and DISTDIR server. [1] https://wiki.gentoo.org/wiki/Local_distfiles_cache [2] https://forums.gentoo.org/viewtopic-t-412722.html
(In reply to klamp from comment #4) Pages on the Gentoo wiki written by users are not official documentation and should bot be treated as such.
We could add some text to make.conf(5) to document the various locations that are security-sensitive. If we want to treat DISTDIR as untrusted, we could potentially copy the the distfiles to a location under PORTAGE_TMPDIR before verification, instead of using symlinks.
Not sure if all of the unpackers would react well to the archive being an un-seekable pipe, but anyway we could tee the archive to the unpacker and a digest checker simultaneously, so that the digest check and unpack are atomic with respect to each other. In this case, the unpacker would refer to the archive as /dev/stdin or /dev/fd/${fd}, which would be a pipe that reads from tee.
(In reply to Zac Medico from comment #7) That sounds overly complex and error-prone. To make that work we would need to delay Manifest verification to the unpack phase, or validate the Manifest multiple times.
Created attachment 872439 [details, diff] man/make.conf.5: note locations with trust issues
(In reply to Zac Medico from comment #7) I don't understand how this would be implemented without creating a copy of the distfiles, either in memory or on disk. Basically, we have to read the entire file to compute the Manifest hashes. And we can't pass the data to the unpacker until after the Manifest has been validated. Am I missing something?
(In reply to Mike Gilbert from comment #10) > (In reply to Zac Medico from comment #7) > > I don't understand how this would be implemented without creating a copy of > the distfiles, either in memory or on disk. Most of the unpackers can probably handle an unseekable stream as input, so it's enough to use a pipe from tee which only buffers a portion of the file. For example, compressed tar streams do not require seek, and I've just checked unzip -l /dev/stdin and 7z l /dev/stdin which both appear to work on zip files. However, we'd have to test all of unpackers to see if they work, and there's really no guarantee that behavior will not change over time for some of them. However, it might be argued that all unpackers *should* support this type of use case, given that using stdin for input files is a fairly common unix thing since it avoids the need for temporary files. > Basically, we have to read the entire file to compute the Manifest hashes. > And we can't pass the data to the unpacker until after the Manifest has been > validated. > > Am I missing something? When you consider the possibility of vulnerabilities in the unpackers, yes it's definitely safer to check the hashes in advance. Checking them simultaneous to unpack would leave room for some kind of exploit to occur before the manifest check can prevent it. Such an exploit might be unlikely, but it's certainly not unheard of.
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/proj/portage.git/commit/?id=b2f543f9b3815ac8e7d7f53ab387ce51f4b8311e commit b2f543f9b3815ac8e7d7f53ab387ce51f4b8311e Author: Mike Gilbert <floppym@gentoo.org> AuthorDate: 2023-10-09 20:46:31 +0000 Commit: Mike Gilbert <floppym@gentoo.org> CommitDate: 2023-10-09 20:46:31 +0000 man/make.conf.5: note locations with trust issues Bug: https://bugs.gentoo.org/915330 Signed-off-by: Mike Gilbert <floppym@gentoo.org> man/make.conf.5 | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
(In reply to Zac Medico from comment #11) > checked unzip -l /dev/stdin and 7z l /dev/stdin which both appear to work on > zip files. Actually this is not true. They work when /dev/stdin is a file redirect, but not if it's a pipe from cat.
The bug has been closed via the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=3500483f75789c36e379c1b6546a09df7c11e2b1 commit 3500483f75789c36e379c1b6546a09df7c11e2b1 Author: Sam James <sam@gentoo.org> AuthorDate: 2023-10-20 00:49:33 +0000 Commit: Sam James <sam@gentoo.org> CommitDate: 2023-10-20 00:51:00 +0000 sys-apps/portage: add 3.0.53 Closes: https://bugs.gentoo.org/915120 Closes: https://bugs.gentoo.org/821529 Closes: https://bugs.gentoo.org/914441 Closes: https://bugs.gentoo.org/914722 Closes: https://bugs.gentoo.org/914873 Closes: https://bugs.gentoo.org/915099 Closes: https://bugs.gentoo.org/915123 Closes: https://bugs.gentoo.org/915128 Closes: https://bugs.gentoo.org/915136 Closes: https://bugs.gentoo.org/915330 Closes: https://bugs.gentoo.org/915494 Closes: https://bugs.gentoo.org/915834 Closes: https://bugs.gentoo.org/915903 Closes: https://bugs.gentoo.org/900224 Signed-off-by: Sam James <sam@gentoo.org> sys-apps/portage/Manifest | 1 + sys-apps/portage/portage-3.0.53.ebuild | 238 +++++++++++++++++++++++++++++++++ 2 files changed, 239 insertions(+)
We could keep this open as an enhancement request. Not sure I feel that strongly about it.
