mirror of
https://git.savannah.gnu.org/git/guix.git
synced 2026-04-06 21:20:33 +02:00
daemon: Actually remove unreadable directories.
Fixes a regression introduced in 7173c2c0ca. Additional discussion at
https://codeberg.org/guix/guix/pulls/5977.
* nix/libutil/util.cc (_deletePathAt): chmod directory and retry open when it
fails with EACCES. Do this using an O_PATH file descriptor referenced via
/proc/self/fd whenever possible to avoid it being replaced by a
non-directory immediately before being chmod'ed.
* nix/libutil/util.hh (deletePath): document TOCTTOU race on non-linux systems
where hardlinks aren't protected.
* tests/derivations.scm ("unreadable directories in build tree can be
removed"): new test.
Fixes: guix/guix#5891
Reported-by: Liliana Marie Prikler <liliana.prikler@gmail.com>
Change-Id: I749127fe5254ebabc8387a2f0ef47e3c116bfcc5
Signed-off-by: Ludovic Courtès <ludo@gnu.org>
Merges: #6460
This commit is contained in:
committed by
Ludovic Courtès
parent
a1611ced6d
commit
865cb0188c
@@ -380,8 +380,55 @@ static void _deletePathAt(int fd, const Path & path, const Path & fullPath, unsi
|
|||||||
O_DIRECTORY |
|
O_DIRECTORY |
|
||||||
O_NOFOLLOW |
|
O_NOFOLLOW |
|
||||||
O_CLOEXEC);
|
O_CLOEXEC);
|
||||||
if(!dirfd.isOpen())
|
if(!dirfd.isOpen()) {
|
||||||
throw SysError(std::format("opening `{}'", fullPath));
|
if (errno != EACCES)
|
||||||
|
throw SysError(std::format("opening `{}'", fullPath));
|
||||||
|
/* Target directory must not have the right permissions for us to
|
||||||
|
* access it. Try changing them. We only do this after the
|
||||||
|
* initial attempt fails because some of the ways we might try
|
||||||
|
* changing the permissions have race conditions, and we'd rather
|
||||||
|
* avoid them if we can (e.g. because we're root). */
|
||||||
|
#ifdef O_PATH
|
||||||
|
{
|
||||||
|
AutoCloseFD pathfd = openat(fd, path.c_str(),
|
||||||
|
O_PATH |
|
||||||
|
O_DIRECTORY |
|
||||||
|
O_NOFOLLOW |
|
||||||
|
O_CLOEXEC);
|
||||||
|
if (!pathfd.isOpen())
|
||||||
|
throw SysError(std::format("opening `{}'", fullPath));
|
||||||
|
|
||||||
|
/* fchmod doesn't work with O_PATH file descriptors. fchmodat
|
||||||
|
* does, but only on very recent kernels (linux 6.6). Despite
|
||||||
|
* this, regular chmod will work with a /proc/self/fd/N
|
||||||
|
* filename that names an O_PATH file descriptor. */
|
||||||
|
string procPath = "/proc/self/fd/" + std::to_string(pathfd);
|
||||||
|
if (chmod(procPath.c_str(), S_IRUSR | S_IWUSR | S_IXUSR) != 0) {
|
||||||
|
if (errno != ENOENT)
|
||||||
|
throw SysError(std::format("chmod of `{}", procPath));
|
||||||
|
/* Fall through */
|
||||||
|
} else {
|
||||||
|
goto retry_open;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
#endif
|
||||||
|
/* !!! If a malicious process can have replaced the directory at
|
||||||
|
PATH with a hardlink to an important file, this may change
|
||||||
|
its permissions to become overly-strict! This should only
|
||||||
|
be a concern where /proc/sys/fs/protected_hardlinks is 0, or
|
||||||
|
on systems without protected_hardlinks. */
|
||||||
|
if (fchmodat(fd, path.c_str(), S_IRUSR | S_IWUSR | S_IXUSR, AT_SYMLINK_NOFOLLOW) != 0)
|
||||||
|
throw SysError(std::format("fchmodat of `{}'", fullPath));
|
||||||
|
|
||||||
|
retry_open:
|
||||||
|
dirfd = openat(fd, path.c_str(),
|
||||||
|
O_RDONLY |
|
||||||
|
O_DIRECTORY |
|
||||||
|
O_NOFOLLOW |
|
||||||
|
O_CLOEXEC);
|
||||||
|
if (!dirfd.isOpen())
|
||||||
|
throw SysError(std::format("opening `{}'", fullPath));
|
||||||
|
}
|
||||||
|
|
||||||
/* st.st_mode may currently be from a different file than what we
|
/* st.st_mode may currently be from a different file than what we
|
||||||
actually opened, get it straight from the file instead */
|
actually opened, get it straight from the file instead */
|
||||||
|
|||||||
@@ -98,7 +98,14 @@ void writeLine(int fd, string s);
|
|||||||
/* Delete a path; i.e., in the case of a directory, it is deleted
|
/* Delete a path; i.e., in the case of a directory, it is deleted
|
||||||
recursively. Don't use this at home, kids. The second variant
|
recursively. Don't use this at home, kids. The second variant
|
||||||
returns the number of bytes and blocks freed, and 'linkThreshold' denotes
|
returns the number of bytes and blocks freed, and 'linkThreshold' denotes
|
||||||
the number of links under which a file is accounted for in 'bytesFreed'. */
|
the number of links under which a file is accounted for in 'bytesFreed'.
|
||||||
|
|
||||||
|
Note that if a directory is unreadable, chmod will be invoked on it to make
|
||||||
|
it u+rwx. On non-linux systems with no equivalent to
|
||||||
|
/proc/sys/fs/protected_hardlinks, a TOCTTOU race may allow the directory to
|
||||||
|
be replaced with a hardlink to an important file, making that file's
|
||||||
|
permissions overly-strict.
|
||||||
|
*/
|
||||||
void deletePath(const Path & path);
|
void deletePath(const Path & path);
|
||||||
|
|
||||||
void deletePath(const Path & path, unsigned long long & bytesFreed,
|
void deletePath(const Path & path, unsigned long long & bytesFreed,
|
||||||
|
|||||||
@@ -1,5 +1,5 @@
|
|||||||
;;; GNU Guix --- Functional package management for GNU
|
;;; GNU Guix --- Functional package management for GNU
|
||||||
;;; Copyright © 2012-2025 Ludovic Courtès <ludo@gnu.org>
|
;;; Copyright © 2012-2026 Ludovic Courtès <ludo@gnu.org>
|
||||||
;;;
|
;;;
|
||||||
;;; This file is part of GNU Guix.
|
;;; This file is part of GNU Guix.
|
||||||
;;;
|
;;;
|
||||||
@@ -967,6 +967,20 @@
|
|||||||
(build-derivations %store (list drv))
|
(build-derivations %store (list drv))
|
||||||
#f)))
|
#f)))
|
||||||
|
|
||||||
|
(test-assert "unreadable directories in build tree can be removed"
|
||||||
|
;; Unreadable directories created in the build tree should be removed
|
||||||
|
;; without problem. See <https://codeberg.org/guix/guix/issues/5891>.
|
||||||
|
(let* ((drv (build-expression->derivation
|
||||||
|
%store
|
||||||
|
(string-append (number->string (random (expt 2 64) (%seed))
|
||||||
|
16)
|
||||||
|
"-unreadable-test")
|
||||||
|
'(begin
|
||||||
|
(mkdir "unreadable" #o000)
|
||||||
|
(mkdir %output)))))
|
||||||
|
(build-derivations %store (list drv))
|
||||||
|
(file-is-directory? (derivation->output-path drv))))
|
||||||
|
|
||||||
|
|
||||||
(define %coreutils
|
(define %coreutils
|
||||||
(false-if-exception
|
(false-if-exception
|
||||||
|
|||||||
Reference in New Issue
Block a user