diff mbox series

Ensure standard file descriptors are open on start

Message ID 20200818175018.27213-1-arsen@aarsen.me
State New
Headers show
Series Ensure standard file descriptors are open on start | expand

Commit Message

Arsen Arsenović Aug. 18, 2020, 5:50 p.m. UTC
ISO C requires that standard input, output and error are always open on
program startup.
---
Prior to these changes, a program could be launched that has no standard input,
output or error stream. This is in conflict with the C and POSIX standards, and
causes some programs (such as autoconf configure scripts) to fail. This behavior
can be easily replicated using a "wrapper" program such as:

    #include <unistd.h>

    int main(int argc, char **argv) {
        close(0);
        execvp(argv[1], argv + 1);
    }

Launching cat via ./close_stdin cat will, without these changes, result in an
EBADF, but will silently exit (as it is reading /dev/null) with these changes.
The latter behavior imitates what the C standard requires.

 csu/check_fds.c  | 10 +++++-----
 csu/libc-start.c |  9 +++------
 elf/dl-sysdep.c  |  7 ++-----
 3 files changed, 10 insertions(+), 16 deletions(-)

Comments

Paul Eggert Aug. 18, 2020, 8:11 p.m. UTC | #1
On 8/18/20 10:50 AM, Arsen Arsenović via Libc-alpha wrote:
> -  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_one_fd (STDIN_FILENO, O_RDONLY | O_NOFOLLOW);
> +  check_one_fd (STDOUT_FILENO, O_WRONLY | O_NOFOLLOW);
> +  check_one_fd (STDERR_FILENO, O_WRONLY | O_NOFOLLOW);

Why is this change needed? Even if ISO C requires that standard streams be open, 
it doesn't require that one can do I/O successfully with the streams.
Arsen Arsenović Aug. 18, 2020, 10:31 p.m. UTC | #2
> > -  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_one_fd (STDIN_FILENO, O_RDONLY | O_NOFOLLOW);
> > +  check_one_fd (STDOUT_FILENO, O_WRONLY | O_NOFOLLOW);
> > +  check_one_fd (STDERR_FILENO, O_WRONLY | O_NOFOLLOW);
> 
> Why is this change needed? Even if ISO C requires that standard streams be
> open, it doesn't require that one can do I/O successfully with the streams.
It seems to me that the sanest option would be making all three file descriptors
read/wrote only (as appropriate), by opening /dev/null. This makes all reads
EOFs and all writes successful, which would be what, at least, I expect when
closing one of the standard streams.
Regardless, with and without this change, the configure script I mentioned still
runs fine (since the standard streams it rightfully assumes exist indeed do
exist), so I can remove it, if that's a better solution.
Paul Eggert Aug. 18, 2020, 10:37 p.m. UTC | #3
On 8/18/20 3:31 PM, Arsen Arsenović wrote:
 > This makes all reads EOFs and all writes successful, which would be what,
 > at least, I expect when closing one of the standard streams.

That's not what I expect. There is a benefit for a program like 'cat' to
report an error (instead of silently succeeding) if its invoker closed stdin
or stdout. That's why glibc behaves the way it does, and it seems a behavior
worth keeping.
Arsen Arsenović Aug. 18, 2020, 11:28 p.m. UTC | #4
> That's not what I expect. There is a benefit for a program like 'cat' to
> report an error (instead of silently succeeding) if its invoker closed stdin
> or stdout. That's why glibc behaves the way it does, and it seems a behavior
> worth keeping.
Fair enough. It is, however, still not correct to have any of those missing on
startup. I can resubmit the patch with this fixed.
diff mbox series

Patch

diff --git a/csu/check_fds.c b/csu/check_fds.c
index 30634b81..f0f88268 100644
--- a/csu/check_fds.c
+++ b/csu/check_fds.c
@@ -58,8 +58,8 @@  check_one_fd (int fd, 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.  */
+	 opened.  Open /dev/null so that the program we are about to
+	 start does not accidentally use this descriptor.  */
       int nullfd = __open_nocancel (name, mode, 0);
 
       /* We are very paranoid here.  With all means we try to ensure
@@ -90,7 +90,7 @@  __libc_check_standard_fds (void)
      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_one_fd (STDIN_FILENO, O_RDONLY | O_NOFOLLOW);
+  check_one_fd (STDOUT_FILENO, O_WRONLY | O_NOFOLLOW);
+  check_one_fd (STDERR_FILENO, O_WRONLY | O_NOFOLLOW);
 }
diff --git a/csu/libc-start.c b/csu/libc-start.c
index 4005caf8..f99efda0 100644
--- a/csu/libc-start.c
+++ b/csu/libc-start.c
@@ -253,12 +253,9 @@  LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
   if (fini)
     __cxa_atexit ((void (*) (void *)) fini, NULL, NULL);
 
-  /* Some security at this point.  Prevent starting a SUID binary where
-     the standard file descriptors are not opened.  We have to do this
-     only for statically linked applications since otherwise the dynamic
-     loader did the work already.  */
-  if (__builtin_expect (__libc_enable_secure, 0))
-    __libc_check_standard_fds ();
+  /* Ensure the standard streams are opened, as required by POSIX and C. For
+     dynamic programs this is already handled in the dynamic loader.  */
+  __libc_check_standard_fds ();
 #endif
 
   /* Call the initializer of the program, if any.  */
diff --git a/elf/dl-sysdep.c b/elf/dl-sysdep.c
index 85457082..83070413 100644
--- a/elf/dl-sysdep.c
+++ b/elf/dl-sysdep.c
@@ -243,11 +243,8 @@  _dl_sysdep_start (void **start_argptr,
     __sbrk (GLRO(dl_pagesize)
 	    - ((_end - (char *) 0) & (GLRO(dl_pagesize) - 1)));
 
-  /* If this is a SUID program we make sure that FDs 0, 1, and 2 are
-     allocated.  If necessary we are doing it ourself.  If it is not
-     possible we stop the program.  */
-  if (__builtin_expect (__libc_enable_secure, 0))
-    __libc_check_standard_fds ();
+  /* Ensure all the standard streams are open (C and POSIX require this) */
+  __libc_check_standard_fds ();
 
   (*dl_main) (phdr, phnum, &user_entry, GLRO(dl_auxv));
   return user_entry;