From patchwork Sat Apr 14 22:31:09 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Zack Weinberg X-Patchwork-Id: 26748 Received: (qmail 86908 invoked by alias); 14 Apr 2018 22:31:24 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 86729 invoked by uid 89); 14 Apr 2018 22:31:13 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_PASS autolearn=ham version=3.3.2 spammy=strikes, Something, hiding X-HELO: mailbackend.panix.com From: Zack Weinberg To: libc-alpha@sourceware.org Cc: schwab@suse.de Subject: [PATCH] Overhaul __libc_check_standard_fds and correct open modes. Date: Sat, 14 Apr 2018 18:31:09 -0400 Message-Id: <20180414223109.4170-1-zackw@panix.com> MIME-Version: 1.0 __libc_check_standard_fds is called very early in startup of set-id processes; it ensures that file descriptors 0, 1, and 2 are open, by opening /dev/null or /dev/full when they're not. The code hasn't been changed significantly since 2005, and makes a number of dubious design choices. This patch is mostly structural cleanup, but one of the changes is semantically visible. The current behavior of this function, when it encounters closed fds, is to open /dev/full for *writing* as stdin, and /dev/null for *reading* as stdout and stderr. The stated rationale for this (hiding in an old ChangeLog -- it looks like a typo in the file) is "Reverse modes so that common operations on the descriptors fail." This strikes me as unwise. The whole point of this function is to backstop insufficiently defensively programmed suid executables. If they're not themselves taking precautions against fds 0, 1, and 2 being closed, why do we believe they are checking for the unusual errors that will occur as a result of fd 0 being open O_WRONLY, and so on? Safer, I think, to stick to common expectations: fd 0 being readable (but empty), fds 1 and 2 being writable. I kept the use of /dev/full, so all writes to 1/2 will still fail, but with an error code (ENOSPC) that programs _should_ be checking for. The structural cleanups are as follows: * Shift the decision of _how_ to open each file descriptor, when necessary, into the subroutine check_one_fd, which now takes only one argument. * Reorganize check_one_fd to return early in the common case that the fd is already open (which unfortunately changes the indentation of the bulk of the function). * Use __glibc_likely / __glibc_unlikely instead of __builtin_expect. * Use __open64_nocancel and __fcntl_nocancel instead of __open_nocancel and __libc_fcntl, respectively. * If ABORT_INSTRUCTION is not defined, define it as _exit (127). * Revise all of the comments. * csu/check_fds.c (check_standard_fds): Now takes only one argument, the fd to check. For stdin, open /dev/null for reading; for stdout and stderr, open /dev/full for writing. Use __glibc_likely/unlikely. Use __open64_nocancel and __fcntl_nocancel. Streamline control flow and update commentary. (__libc_check_standard_fds): Update to match. (ABORT_INSTRUCTION): If not defined, define as _exit (127). --- csu/check_fds.c | 98 +++++++++++++++++++++++++------------------------ 1 file changed, 51 insertions(+), 47 deletions(-) diff --git a/csu/check_fds.c b/csu/check_fds.c index 2b181d44ab..0e7acdf3ad 100644 --- a/csu/check_fds.c +++ b/csu/check_fds.c @@ -27,7 +27,7 @@ #include #ifndef ABORT_INSTRUCTION /* No such instruction is available. */ -# define ABORT_INSTRUCTION +# define ABORT_INSTRUCTION _exit (127) #endif #include @@ -37,61 +37,65 @@ /* Should other OSes (e.g., Hurd) have different versions which can be written in a better way? */ static void -check_one_fd (int fd, int mode) +check_one_fd (int fd) { - /* Note that fcntl() with this parameter is not a cancellation point. */ - if (__builtin_expect (__libc_fcntl (fd, F_GETFD), 0) == -1 - && errno == EBADF) - { - const char *name; - dev_t dev; + if (__glibc_likely (__fcntl_nocancel (fd, F_GETFD) >= 0) || errno != EBADF) + return; - /* For writable descriptors we use /dev/full. */ - if ((mode & O_ACCMODE) == O_WRONLY) - { - name = _PATH_DEV "full"; - dev = __gnu_dev_makedev (DEV_FULL_MAJOR, DEV_FULL_MINOR); - } - else - { - name = _PATH_DEVNULL; - dev = __gnu_dev_makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR); - } + /* One of the standard file descriptors is not open. Open a + harmless file, so that the SUID program we are about to start + does not accidentally use this descriptor for something else. */ + const char *name; + dev_t dev; + int mode; - /* Something is wrong with this descriptor, it's probably not - opened. Open /dev/null so that the SUID program we are - about to start does not accidentally use this descriptor. */ - int nullfd = __open_nocancel (name, mode, 0); + /* For stdin, simply open /dev/null for reading; this is most likely + to behave as programs expect (no input available). For stdout + and stderr, open /dev/full for writing, which will cause all + writes to fail with ENOSPC. */ + if (fd == STDIN_FILENO) + { + mode = O_RDONLY; + name = _PATH_DEVNULL; + dev = __gnu_dev_makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR); + } + else + { + mode = O_WRONLY; + name = _PATH_DEV "full"; + dev = __gnu_dev_makedev (DEV_FULL_MAJOR, DEV_FULL_MINOR); + } - /* We are very paranoid here. With all means we try to ensure - that we are actually opening the /dev/null device and nothing - else. + /* Because STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO are the three + lowest file descriptor numbers, in that order, and they are being + checked in that order (see below), this should return the + descriptor that we just found was not open. For extra paranoia, + use O_NOFOLLOW so that we will not open /dev/null or /dev/full if + it has been replaced with a symlink to somewhere else. */ + int nfd = __open64_nocancel (name, mode | O_NOFOLLOW, 0); - Note that the following code assumes that STDIN_FILENO, - STDOUT_FILENO, STDERR_FILENO are the three lowest file - decsriptor numbers, in this order. */ - struct stat64 st; - if (__builtin_expect (nullfd != fd, 0) - || __builtin_expect (__fxstat64 (_STAT_VER, fd, &st), 0) != 0 - || __builtin_expect (S_ISCHR (st.st_mode), 1) == 0 - || st.st_rdev != dev) - /* We cannot even give an error message here since it would - run into the same problems. */ - while (1) - /* Try for ever and ever. */ - ABORT_INSTRUCTION; - } + /* Verify that we got the correct descriptor number, and that what + we just opened is really the device we wanted to open. */ + struct stat64 st; + if (__glibc_likely (nfd == fd + && __fxstat64 (_STAT_VER, fd, &st) == 0 + && S_ISCHR (st.st_mode) + && st.st_rdev == dev)) + return; + + /* There is no safe way to report an error when stderr might be + closed. Crash the program. */ + while (1) + /* Try for ever and ever. */ + ABORT_INSTRUCTION; } void __libc_check_standard_fds (void) { - /* Check all three standard file descriptors. The O_NOFOLLOW flag - is really paranoid but some people actually are. If /dev/null - should happen to be a symlink to somewhere else and not the - device commonly known as "/dev/null" we bail out. */ - check_one_fd (STDIN_FILENO, O_WRONLY | O_NOFOLLOW); - check_one_fd (STDOUT_FILENO, O_RDONLY | O_NOFOLLOW); - check_one_fd (STDERR_FILENO, O_RDONLY | O_NOFOLLOW); + /* Check all three standard file descriptors. */ + check_one_fd (STDIN_FILENO); + check_one_fd (STDOUT_FILENO); + check_one_fd (STDERR_FILENO); }