Created attachment 453296 [details] ndoutils-2.1.1.ebuild The attached ebuild is for a project that recently dropped DESTDIR from its build system. The ebuild sets --bindir=/usr/bin, and as a result, that path gets passed straight to the installation routine. This eventually causes sandbox violations (when it tries to write to /usr/bin), but not before... /usr/lib/portage/python3.4/ebuild-helpers/xattr/install \ -c -m 775 -o nagios -g nagios -d /var/lib /usr/lib/portage/python3.4/ebuild-helpers/xattr/install \ -c -m 775 -o nagios -g nagios -d /usr/bin and both of those succeed. On a hardened system, that leaves things totally hosed, and on a normal system you're still screwed -- just more quietly. Is it possible to detect ownership changes like that outside of the sandbox during src_install?
i don't know what you mean by "outside of the sandbox". by default, src_install runs entirely inside the sandbox. what version of sandbox are you running exactly ?
sandbox already captures chown(2) and fchownat(2). # mkdir /test # sandbox chown nobody:nobody /test * ACCESS DENIED: fchownat: /test chown: changing ownership of '/test': Permission denied * --------------------------- ACCESS VIOLATION SUMMARY --------------------------- * LOG FILE: "/var/log/sandbox/sandbox-32277.log" * VERSION 1.0 FORMAT: F - Function called FORMAT: S - Access Status FORMAT: P - Path as passed to function FORMAT: A - Absolute Path (not canonical) FORMAT: R - Canonical Path FORMAT: C - Command Line F: fchownat S: deny P: /test A: /test R: /test C: chown nobody:nobody /test * -------------------------------------------------------------------------------- It looks like install-xattr is using fchown(2). strace -f -o /tmp/strace.log install-xattr -o nobody -g nobody -d /test 32490 mkdir("/test", 0700) = -1 EEXIST (File exists) 32490 open("/test", O_RDONLY|O_NOCTTY|O_NONBLOCK|O_DIRECTORY) = 3 32490 fstat(3, {st_mode=S_IFDIR|0755, st_size=0, ...}) = 0 32490 fchown(3, 65534, 65534) = 0 32490 close(3) = 0
I think floppym answered the real question, but sandbox-2.10-r1 if it still matters.
not catching fchown is by design -- it operates on a fd, not on a path, and it should require write access. in order to get that write access, you need to open() with write access, and sandbox would have caught & rejected it then. the reason sandbox catches fchownat is because you can pass in a path name which needs checking. the fact that you can fchown()/fchmod() on a fd opened with O_RDONLY is weird and not what sandbox was accounting for which is why it doesn't catch it. a simple test case from that strace: int main(int argc, char *argv[]) { char path[] = "foo"; int fd = open(path, O_RDONLY); fchown(fd, 100, 100); fchmod(fd, 0000); close(fd); }
Created attachment 516938 [details, diff] 0001-tests-add-test-case-for-fchown-fchmod-with-O_RDONLY-.patch
Created attachment 516940 [details, diff] 0002-libsandbox-add-awful-hacky-fchown-fchmod-fix-on-linu.patch
Created attachment 516944 [details, diff] 0003-tests-add-more-tests-to-make-sure-the-new-fchown-fch.patch It took me all night to figure out WTF the sandbox is doing, but here is the stupidest proof-of-concept that works (if you're on linux).
Created attachment 618702 [details, diff] 0001-libsandbox-add-support-for-fchown-fchmod-on-linux.patch
Created attachment 618704 [details, diff] 0002-tests-add-test-case-for-fchown-fchmod-with-O_RDONLY.patch
Created attachment 618706 [details, diff] 0003-tests-add-more-tests-to-make-sure-fchown-fchmod-are-.patch Here's a cleaned-up patch series. I probably should not have called this approach hacky, since it turns out the sandbox is already using it =) Now, I check the value of SANDBOX_PROC_SELF_FD, which is defined by an existing autoconf macro test, to enable the additional support. The test cases also utilize that constant, so the test suite will pass even on a non-linux system. This would be really nice to have since basically everyone is on linux now, and an errant fchown/fchmod can really mess up your system. Please give it a review!
So, does anyone have any opinion on this? I don't like using /proc but I guess it's better than nothing. If nobody opposes, I'd like to merge this soonish.
Beep beep?
Created attachment 864316 [details, diff] sandbox-fchown-fchmod-master.patch
Created attachment 864317 [details, diff] sandbox-fchown-fchmod-stable.patch I rebased this series on the current master branch, and then backported it to the stable-2.x branch where it might actually get released now.
The proposed patch seems ok to me. I will plan on merging the fix for 908765 and releasing that first since it fixes a regression, and then cut another release for this change.
git outputs some whitespace warnings when I apply the series locally. I think it wants tabs-before spaces instead of spaces-before tabs. Could you fix that? Applying: libsandbox: add support for fchown/fchmod on linux .git/rebase-apply/patch:20: space before tab in indent. sb_nr == SB_NR_FCHMOD || .git/rebase-apply/patch:22: space before tab in indent. sb_nr == SB_NR_FCHOWN || warning: 2 lines add whitespace errors. Also, would you mind opening a PR for this instead of attaching patches? We have a basic CI workflow set up there. https://github.com/gentoo/sandbox
(In reply to Mike Gilbert from comment #16) > git outputs some whitespace warnings when I apply the series locally. I > think it wants tabs-before spaces instead of spaces-before tabs. Could you > fix that? Fudged while rebasing the patches, thanks. They're fixed in the PRs. > Also, would you mind opening a PR for this instead of attaching patches? We > have a basic CI workflow set up there. > > https://github.com/gentoo/sandbox https://github.com/gentoo/sandbox/pull/8 https://github.com/gentoo/sandbox/pull/9
The bug has been closed via the following commit(s): https://gitweb.gentoo.org/proj/sandbox.git/commit/?id=652097eb4228ac9ba9973811b2832fc77f2048a2 commit 652097eb4228ac9ba9973811b2832fc77f2048a2 Author: Michael Orlitzky <mjo@gentoo.org> AuthorDate: 2018-01-28 03:38:26 +0000 Commit: Mike Gilbert <floppym@gentoo.org> CommitDate: 2023-06-22 13:54:38 +0000 tests: add more tests to make sure fchown/fchmod are handled correctly. Closes: https://bugs.gentoo.org/599706 Signed-off-by: Michael Orlitzky <mjo@gentoo.org> Signed-off-by: Mike Gilbert <floppym@gentoo.org> tests/fchmod-2.sh | 11 +++++++++++ tests/fchmod.at | 1 + tests/fchown-2.sh | 11 +++++++++++ tests/fchown.at | 1 + 4 files changed, 24 insertions(+) Additionally, it has been referenced in the following commit(s): https://gitweb.gentoo.org/proj/sandbox.git/commit/?id=05e32f542c145253eb01ae4005ca13c63a1c79d8 commit 05e32f542c145253eb01ae4005ca13c63a1c79d8 Author: Michael Orlitzky <mjo@gentoo.org> AuthorDate: 2018-01-28 01:05:02 +0000 Commit: Mike Gilbert <floppym@gentoo.org> CommitDate: 2023-06-22 13:54:38 +0000 tests: add test case for fchown/fchmod with O_RDONLY. Bug: https://bugs.gentoo.org/599706 Signed-off-by: Michael Orlitzky <mjo@gentoo.org> Signed-off-by: Mike Gilbert <floppym@gentoo.org> tests/fchmod-0.c | 35 +++++++++++++++++++++++++++++++++++ tests/fchmod-1.sh | 14 ++++++++++++++ tests/fchmod.at | 1 + tests/fchown-0.c | 34 ++++++++++++++++++++++++++++++++++ tests/fchown-1.sh | 14 ++++++++++++++ tests/fchown.at | 1 + tests/local.mk | 2 ++ 7 files changed, 101 insertions(+) https://gitweb.gentoo.org/proj/sandbox.git/commit/?id=6c1adf9bb858f64bb00ffddc861dc8a248217242 commit 6c1adf9bb858f64bb00ffddc861dc8a248217242 Author: Michael Orlitzky <michael@orlitzky.com> AuthorDate: 2023-06-20 21:58:57 +0000 Commit: Mike Gilbert <floppym@gentoo.org> CommitDate: 2023-06-22 13:54:38 +0000 libsandbox: add support for fchown/fchmod on linux The fchown/fchmod functions use a file descriptor obtained from open(), and the sandbox relies on its open() wrapper for safety. But it turns out that fchown/fchmod can operate on a descriptor opened O_RDONLY, which the open() wrapper is happy to give you. Oops. This is bug 599706. There's no POSIX way to map the descriptor to a path once you've got it, but on linux you can use the magic path "/proc/self/fd/%i" which should be a symlink pointing to the path passed to open(). Once we have that path, we can use the existing "is this path safe" machinery in the sandbox. There is precedent for this approach in sandbox, and the SANDBOX_PROC_SELF_FD macro already exists to indicate that the feature is available. Bug: https://bugs.gentoo.org/599706 Signed-off-by: Michael Orlitzky <mjo@gentoo.org> Signed-off-by: Mike Gilbert <floppym@gentoo.org> libsandbox/libsandbox.c | 17 +++++++++++++++++ libsandbox/libsandbox.h | 7 +++++++ libsandbox/symbols.h.in | 2 ++ libsandbox/trace.c | 13 +++++++++++++ libsandbox/wrapper-funcs/fchmod.c | 11 +++++++++++ libsandbox/wrapper-funcs/fchown.c | 11 +++++++++++ 6 files changed, 61 insertions(+)
