From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lists.gentoo.org (pigeon.gentoo.org [208.92.234.80]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by finch.gentoo.org (Postfix) with ESMTPS id 81D88158086 for ; Thu, 28 Oct 2021 03:41:47 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id C87CAE0882; Thu, 28 Oct 2021 03:41:46 +0000 (UTC) Received: from smtp.gentoo.org (mail.gentoo.org [IPv6:2001:470:ea4a:1:5054:ff:fec7:86e4]) (using TLSv1.2 with cipher ECDHE-RSA-CHACHA20-POLY1305 (256/256 bits)) (No client certificate requested) by pigeon.gentoo.org (Postfix) with ESMTPS id 35098E0882 for ; Thu, 28 Oct 2021 03:41:46 +0000 (UTC) Received: from oystercatcher.gentoo.org (oystercatcher.gentoo.org [148.251.78.52]) (using TLSv1.2 with cipher ECDHE-RSA-CHACHA20-POLY1305 (256/256 bits)) (No client certificate requested) by smtp.gentoo.org (Postfix) with ESMTPS id BBBA8342B73 for ; Thu, 28 Oct 2021 03:41:44 +0000 (UTC) Received: from localhost.localdomain (localhost [IPv6:::1]) by oystercatcher.gentoo.org (Postfix) with ESMTP id 127D915A for ; Thu, 28 Oct 2021 03:41:43 +0000 (UTC) From: "Mike Frysinger" To: gentoo-commits@lists.gentoo.org Content-Transfer-Encoding: 8bit Content-type: text/plain; charset=UTF-8 Reply-To: gentoo-dev@lists.gentoo.org, "Mike Frysinger" Message-ID: <1635391020.49e6eb50d1c77f06d8b4c728cd222d3d404c8d08.vapier@gentoo> Subject: [gentoo-commits] proj/sandbox:master commit in: libsandbox/ X-VCS-Repository: proj/sandbox X-VCS-Files: libsandbox/libsandbox.c X-VCS-Directories: libsandbox/ X-VCS-Committer: vapier X-VCS-Committer-Name: Mike Frysinger X-VCS-Revision: 49e6eb50d1c77f06d8b4c728cd222d3d404c8d08 X-VCS-Branch: master Date: Thu, 28 Oct 2021 03:41:43 +0000 (UTC) Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-Id: Gentoo Linux mail X-BeenThere: gentoo-commits@lists.gentoo.org X-Auto-Response-Suppress: DR, RN, NRN, OOF, AutoReply X-Archives-Salt: 16e71c6a-b28e-424a-aa6d-ae67657475a1 X-Archives-Hash: d4c1e96233b0b4613179779e86d4ac5a commit: 49e6eb50d1c77f06d8b4c728cd222d3d404c8d08 Author: Mike Frysinger chromium org> AuthorDate: Thu Oct 28 03:17:00 2021 +0000 Commit: Mike Frysinger gentoo org> CommitDate: Thu Oct 28 03:17:00 2021 +0000 URL: https://gitweb.gentoo.org/proj/sandbox.git/commit/?id=49e6eb50 libsandbox: drop lstat check for symlink funcs When checking paths for violations, we need to know whether the path is a symlink, and whether the current function dereferences them. If it dereferences, we have to check the symlink and its target. If it doesn't, we can skip the target check. The helper to see if the function operates on symlinks ends with an lstat on the path itself -- if it exists and is a symlink, we will skip the target check. If it doesn't exist, or isn't a symlink, we check the target. This logic doesn't make sense since (1) if it doesn't exist, or isn't a symlink, there is no "target" and (2) the symlink nature of the function is unchanged. In practice, this largely doesn't matter. If the path wasn't a symlink, and it (as the source) already passed checks, then it's also going to pass checks (as the target) since they're the same path. However, we get into a fun TOCTOU race: if there are multiple things trying to create a symlink at the same path, then we can get into a state where: - process 1 calls a symlink func on a path doesn't exist - lstat fails, so symlink_func() returns false - the kernel contexts switches away from process 1 - process 2 calls a symlink func on the same path - lstat fails, so symlink_func() returns false - the target path is "resolved" and passes validation - process 2 creates the symlink to a place like /usr/bin/foo - process 1 resumes - the target path is resolved since it now actually exists - the target is a bad path (/usr/bin/foo) - sandbox denies the access even though it's a func that only operates on symlinks and never dereferences This scenario too rarely happens (causes it's so weird), but it is possible. A quick way to reproduce is with: while [[ ! -e $SANDBOX_LOG ]] ; do ln -s /bin/bash ./f & ln -s /bin/bash ./f & ln -s /bin/bash ./f & ln -s /bin/bash ./f & ln -s /bin/bash ./f & rm -f f wait done Eventually this will manage to trigger the TOCTOU race. So just delete the lstat check in the symlink_func() helper. If the path doesn't exist, we can safely let it fail. If the path shows up in parallel, either as a symlink or not, we already validated it as being safe, so letting the func be called is safe. Bug: https://issuetracker.google.com/issues/204375293 Signed-off-by: Mike Frysinger gentoo.org> libsandbox/libsandbox.c | 51 ++++++++++++++++++++++--------------------------- 1 file changed, 23 insertions(+), 28 deletions(-) diff --git a/libsandbox/libsandbox.c b/libsandbox/libsandbox.c index 4e92cbe..b4db9ba 100644 --- a/libsandbox/libsandbox.c +++ b/libsandbox/libsandbox.c @@ -671,36 +671,31 @@ static int check_prefixes(char **prefixes, int num_prefixes, const char *path) } /* Is this a func that works on symlinks, and is the file a symlink ? */ -static bool symlink_func(int sb_nr, int flags, const char *abs_path) +static bool symlink_func(int sb_nr, int flags) { - struct stat st; - /* These funcs always operate on symlinks */ - if (!(sb_nr == SB_NR_UNLINK || - sb_nr == SB_NR_UNLINKAT || - sb_nr == SB_NR_LCHOWN || - sb_nr == SB_NR_LREMOVEXATTR || - sb_nr == SB_NR_LSETXATTR || - sb_nr == SB_NR_REMOVE || - sb_nr == SB_NR_RENAME || - sb_nr == SB_NR_RENAMEAT || - sb_nr == SB_NR_RENAMEAT2 || - sb_nr == SB_NR_RMDIR || - sb_nr == SB_NR_SYMLINK || - sb_nr == SB_NR_SYMLINKAT)) - { - /* These funcs sometimes operate on symlinks */ - if (!((sb_nr == SB_NR_FCHOWNAT || - sb_nr == SB_NR_FCHMODAT || - sb_nr == SB_NR_UTIMENSAT) && - (flags & AT_SYMLINK_NOFOLLOW))) - return false; - } + if (sb_nr == SB_NR_UNLINK || + sb_nr == SB_NR_UNLINKAT || + sb_nr == SB_NR_LCHOWN || + sb_nr == SB_NR_LREMOVEXATTR || + sb_nr == SB_NR_LSETXATTR || + sb_nr == SB_NR_REMOVE || + sb_nr == SB_NR_RENAME || + sb_nr == SB_NR_RENAMEAT || + sb_nr == SB_NR_RENAMEAT2 || + sb_nr == SB_NR_RMDIR || + sb_nr == SB_NR_SYMLINK || + sb_nr == SB_NR_SYMLINKAT) + return true; - if (-1 != lstat(abs_path, &st) && S_ISLNK(st.st_mode)) + /* These funcs sometimes operate on symlinks */ + if ((sb_nr == SB_NR_FCHOWNAT || + sb_nr == SB_NR_FCHMODAT || + sb_nr == SB_NR_UTIMENSAT) && + (flags & AT_SYMLINK_NOFOLLOW)) return true; - else - return false; + + return false; } static int check_access(sbcontext_t *sbcontext, int sb_nr, const char *func, @@ -709,7 +704,7 @@ static int check_access(sbcontext_t *sbcontext, int sb_nr, const char *func, int old_errno = errno; int result = 0; int retval; - bool sym_func = symlink_func(sb_nr, flags, abs_path); + bool sym_func = symlink_func(sb_nr, flags); retval = check_prefixes(sbcontext->deny_prefixes, sbcontext->num_deny_prefixes, abs_path); @@ -904,7 +899,7 @@ static int check_syscall(sbcontext_t *sbcontext, int sb_nr, const char *func, * itself does not dereference. This speeds things up and avoids updating * the atime implicitly. #415475 */ - if (symlink_func(sb_nr, flags, absolute_path)) + if (symlink_func(sb_nr, flags)) resolved_path = absolute_path; else resolved_path = resolve_path(file, 1);