Overhaul __libc_check_standard_fds and correct open modes.
Commit Message
__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(-)
Comments
On 04/14/2018 03:31 PM, Zack Weinberg wrote:
> 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?
We don't believe that. All we believe is that the programs that are
checking for errors will get error diagnostics that will help developers
diagnose the problem, while the programs that are not checking for
errors will get the similar behavior now that they would get under your
proposed change.
The proposed change would make sense if it were more helpful for
programs that check for errors to get EOF on input and ENOSPC on output,
than for them to get EBADF on both input and output. I am skeptical that
this would help, though. I think the current behavior is more helpful,
since EBADF is more likely to point developers in the right direction
that their programs are misusing file descriptors. EOF on input and
ENOSPC on output are relatively routine situations that can occur with
well-written programs. In contrast, EBADF typically means your program
has a bug, and this more-accurately describes the programs in question.
On 04/15/2018 12:31 AM, Zack Weinberg wrote:
> 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?
These defensive descriptors could be inherited by child processes, which
perform such checking.
Thanks,
Florian
@@ -27,7 +27,7 @@
#include <abort-instr.h>
#ifndef ABORT_INSTRUCTION
/* No such instruction is available. */
-# define ABORT_INSTRUCTION
+# define ABORT_INSTRUCTION _exit (127)
#endif
#include <device-nrs.h>
@@ -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);
}