diff mbox series

[v2] Ensure standard file descriptors are open on start

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

Commit Message

Arsen Arsenović Aug. 19, 2020, 12:41 p.m. UTC
ISO C requires that standard input, output and error are always open on
program startup.
---
I've removed the changes to the access mode used when opening the three standard
file descriptors, to address Paul's concerns.

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

Comments

Joseph Myers Aug. 19, 2020, 4:28 p.m. UTC | #1
On Wed, 19 Aug 2020, Arsen Arsenović via Libc-alpha wrote:

> ISO C requires that standard input, output and error are always open on
> program startup.

ISO C doesn't talk about file descriptors at all.  The objects stdin, 
stdout and stderr need to be initialized, but it's fine for all I/O on 
them to fail.

> +  /* Ensure the standard streams are opened, as required by POSIX and C. For
> +     dynamic programs this is already handled in the dynamic loader.  */

Please give specific references, not just "as required by POSIX and C".  
What exactly do you think requires these descriptors to be open?  The 
POSIX specification for execve explicitly says "If file descriptor 0, 1, 
or 2 would otherwise be closed after a successful call to one of the exec 
family of functions, implementations may open an unspecified file for the 
file descriptor in the new process image. If a standard utility or a 
conforming application is executed with file descriptor 0 not open for 
reading or with file descriptor 1 or 2 not open for writing, the 
environment in which the utility or application is executed shall be 
deemed non-conforming, and consequently the utility or application might 
not behave as described in this standard.".  So at least that description 
of the process of creating a new process image only permits opening those 
file descriptors but does not require it and indeed explicitly does not 
impose requirements on how code behaves if started with those file 
descriptors not open.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/execve.html
Zack Weinberg Aug. 19, 2020, 5:46 p.m. UTC | #2
On Wed, Aug 19, 2020 at 12:28 PM Joseph Myers <joseph@codesourcery.com> wrote:
> On Wed, 19 Aug 2020, Arsen Arsenović via Libc-alpha wrote:
>
> > ISO C requires that standard input, output and error are always open on
> > program startup.
>
> ISO C doesn't talk about file descriptors at all.  The objects stdin,
> stdout and stderr need to be initialized, but it's fine for all I/O on
> them to fail.
>
> > +  /* Ensure the standard streams are opened, as required by POSIX and C. For
> > +     dynamic programs this is already handled in the dynamic loader.  */
>
> Please give specific references, not just "as required by POSIX and C".
> What exactly do you think requires these descriptors to be open?

Are you raising a hard objection to this change, Joseph?  I think it
makes sense just on QoI grounds.  Specifically, the reason we already
do this for set-ID programs (it could be very bad if the program
accidentally writes to a file that it didn't expect to be assigned fd
1 or 2) seems to apply nearly as well to ordinary programs (it's not a
_security_ issue but it could still cause data loss).

zw
Joseph Myers Aug. 19, 2020, 5:50 p.m. UTC | #3
On Wed, 19 Aug 2020, Zack Weinberg wrote:

> > Please give specific references, not just "as required by POSIX and C".
> > What exactly do you think requires these descriptors to be open?
> 
> Are you raising a hard objection to this change, Joseph?  I think it

No, I'm not objecting to the change, provided the comment is revised with 
more specific standard references (for example, to the execve text 
permitting but not requiring opening file descriptors like this).
Adhemerval Zanella Netto Aug. 19, 2020, 6:20 p.m. UTC | #4
On 19/08/2020 14:46, Zack Weinberg wrote:
> On Wed, Aug 19, 2020 at 12:28 PM Joseph Myers <joseph@codesourcery.com> wrote:
>> On Wed, 19 Aug 2020, Arsen Arsenović via Libc-alpha wrote:
>>
>>> ISO C requires that standard input, output and error are always open on
>>> program startup.
>>
>> ISO C doesn't talk about file descriptors at all.  The objects stdin,
>> stdout and stderr need to be initialized, but it's fine for all I/O on
>> them to fail.
>>
>>> +  /* Ensure the standard streams are opened, as required by POSIX and C. For
>>> +     dynamic programs this is already handled in the dynamic loader.  */
>>
>> Please give specific references, not just "as required by POSIX and C".
>> What exactly do you think requires these descriptors to be open?
> 
> Are you raising a hard objection to this change, Joseph?  I think it
> makes sense just on QoI grounds.  Specifically, the reason we already
> do this for set-ID programs (it could be very bad if the program
> accidentally writes to a file that it didn't expect to be assigned fd
> 1 or 2) seems to apply nearly as well to ordinary programs (it's not a
> _security_ issue but it could still cause data loss).


But is it really a useful hardening, even for SUID binaries?  The 
check_one_fd only check if the file descriptor is opened and redirects
it to /dev/full otherwise.  It does really 'protect' if a constructor
or a LD_PRELOAD redirects the STD*_FILENO to something else.

I think the 'Bad file descriptor' is enough information to indicate
that the process was launched in a non-expected manner. So what exactly
__libc_check_standard_fds is trying to mitigate/improve here on SUID?
Florian Weimer Aug. 19, 2020, 7:13 p.m. UTC | #5
* Adhemerval Zanella via Libc-alpha:

> But is it really a useful hardening, even for SUID binaries?  The 
> check_one_fd only check if the file descriptor is opened and redirects
> it to /dev/full otherwise.  It does really 'protect' if a constructor
> or a LD_PRELOAD redirects the STD*_FILENO to something else.

The protection is against messages intended for standard input and
standard error showing up in explicitly open files (which would
otherwise receive descriptors 3 and higher).  This is not too
far-fetched, given that such messages could well have parts that are
under control of a different user.
Adhemerval Zanella Netto Aug. 19, 2020, 7:25 p.m. UTC | #6
On 19/08/2020 16:13, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> But is it really a useful hardening, even for SUID binaries?  The 
>> check_one_fd only check if the file descriptor is opened and redirects
>> it to /dev/full otherwise.  It does really 'protect' if a constructor
>> or a LD_PRELOAD redirects the STD*_FILENO to something else.
> 
> The protection is against messages intended for standard input and
> standard error showing up in explicitly open files (which would
> otherwise receive descriptors 3 and higher).  This is not too
> far-fetched, given that such messages could well have parts that are
> under control of a different user.
> 

Yes, but in which scenario this is really a vector of attack? One could
still open/close/dup to redirect the messages.
Florian Weimer Aug. 19, 2020, 7:27 p.m. UTC | #7
* Adhemerval Zanella:

> On 19/08/2020 16:13, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>> 
>>> But is it really a useful hardening, even for SUID binaries?  The 
>>> check_one_fd only check if the file descriptor is opened and redirects
>>> it to /dev/full otherwise.  It does really 'protect' if a constructor
>>> or a LD_PRELOAD redirects the STD*_FILENO to something else.
>> 
>> The protection is against messages intended for standard input and
>> standard error showing up in explicitly open files (which would
>> otherwise receive descriptors 3 and higher).  This is not too
>> far-fetched, given that such messages could well have parts that are
>> under control of a different user.
>
> Yes, but in which scenario this is really a vector of attack? One could
> still open/close/dup to redirect the messages.

In this cenario, the attacking user has only control over the
descriptors when the process is launched.
Adhemerval Zanella Netto Aug. 19, 2020, 7:35 p.m. UTC | #8
On 19/08/2020 16:27, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 19/08/2020 16:13, Florian Weimer wrote:
>>> * Adhemerval Zanella via Libc-alpha:
>>>
>>>> But is it really a useful hardening, even for SUID binaries?  The 
>>>> check_one_fd only check if the file descriptor is opened and redirects
>>>> it to /dev/full otherwise.  It does really 'protect' if a constructor
>>>> or a LD_PRELOAD redirects the STD*_FILENO to something else.
>>>
>>> The protection is against messages intended for standard input and
>>> standard error showing up in explicitly open files (which would
>>> otherwise receive descriptors 3 and higher).  This is not too
>>> far-fetched, given that such messages could well have parts that are
>>> under control of a different user.
>>
>> Yes, but in which scenario this is really a vector of attack? One could
>> still open/close/dup to redirect the messages.
> 
> In this cenario, the attacking user has only control over the
> descriptors when the process is launched.
> 

What I am failing to see is how feasible is the scenario that
__libc_check_standard_fds tries to mitigate, specially in SUID binaries;
and if it really worth to extend it to all modes by adding the extra
syscall on all process execution.
Arsen Arsenović Aug. 19, 2020, 7:40 p.m. UTC | #9
> Please give specific references, not just "as required by POSIX and C".  
> What exactly do you think requires these descriptors to be open?
The sections that lead me to believe this were:
http://www.iso-9899.info/n1570.html#7.21.3p7
https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_05
What would be the best way to reference these in source code? I can wait for
some more potential feedback to aggregate, and for a consensus to be reached,
before updating the patch with that, to reduce patch spam.

POSIX also, in other pages, occasionally mentions the danger of a file being
unexpectedly open as one of three special file descriptors, which I presume is
the reason for the hardening glibc was already doing for SUID binaries.

> "If a standard utility or a conforming application is executed with file
> descriptor 0 not open for reading or with file descriptor 1 or 2 not open for
> writing, the environment in which the utility or application is executed shall
> be deemed non-conforming, and consequently the utility or application might
> not behave as described in this standard."
This specific part of the quote would seem to imply that standard input, output
and error must be opened for reading and writing respectively?
Or do you think this only applies if the implementation decides to handle
opening the file descriptors?
Michael Morrell Aug. 19, 2020, 9:04 p.m. UTC | #10
The ISO C quote says they don't need to be opened (an application can just start using them), but it doesn't say they have to be open.  It is perfectly OK for them to be closed at startup.  Also, it talks about the FILE streams stdin, stdout, and stderr, not file descriptors 0, 1, and 2.

-----Original Message-----
From: Libc-alpha <libc-alpha-bounces@sourceware.org> On Behalf Of Arsen Arsenovic via Libc-alpha
Sent: Wednesday, August 19, 2020 12:40 PM
To: Joseph Myers <joseph@codesourcery.com>
Cc: libc-alpha@sourceware.org
Subject: Re: [PATCH v2] Ensure standard file descriptors are open on start

> Please give specific references, not just "as required by POSIX and C".  
> What exactly do you think requires these descriptors to be open?
The sections that lead me to believe this were:
http://www.iso-9899.info/n1570.html#7.21.3p7
https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_05
What would be the best way to reference these in source code? I can wait for some more potential feedback to aggregate, and for a consensus to be reached, before updating the patch with that, to reduce patch spam.

POSIX also, in other pages, occasionally mentions the danger of a file being unexpectedly open as one of three special file descriptors, which I presume is the reason for the hardening glibc was already doing for SUID binaries.

> "If a standard utility or a conforming application is executed with 
> file descriptor 0 not open for reading or with file descriptor 1 or 2 
> not open for writing, the environment in which the utility or 
> application is executed shall be deemed non-conforming, and 
> consequently the utility or application might not behave as described in this standard."
This specific part of the quote would seem to imply that standard input, output and error must be opened for reading and writing respectively?
Or do you think this only applies if the implementation decides to handle opening the file descriptors?

--
Arsen Arsenović
Joseph Myers Aug. 19, 2020, 10:32 p.m. UTC | #11
On Wed, 19 Aug 2020, Arsen Arsenović via Libc-alpha wrote:

> > Please give specific references, not just "as required by POSIX and C".  
> > What exactly do you think requires these descriptors to be open?
> The sections that lead me to believe this were:
> http://www.iso-9899.info/n1570.html#7.21.3p7
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_05

Those appear to be specifically about the stdio streams, not the file 
descriptors.  I think a stream that returns an EBADF error for all 
operations would be perfectly valid for the purposes of those 
requirements.

> This specific part of the quote would seem to imply that standard input, output
> and error must be opened for reading and writing respectively?

Yes - that is, what the implementation does if standard input is not open 
for reading, or standard output and error are not open for writing, is not 
specified.
Rich Felker Aug. 19, 2020, 11:16 p.m. UTC | #12
On Wed, Aug 19, 2020 at 09:40:25PM +0200, Arsen Arsenović via Libc-alpha wrote:
> > Please give specific references, not just "as required by POSIX and C".  
> > What exactly do you think requires these descriptors to be open?
> The sections that lead me to believe this were:
> http://www.iso-9899.info/n1570.html#7.21.3p7
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_05
> What would be the best way to reference these in source code? I can wait for
> some more potential feedback to aggregate, and for a consensus to be reached,
> before updating the patch with that, to reduce patch spam.
> 
> POSIX also, in other pages, occasionally mentions the danger of a file being
> unexpectedly open as one of three special file descriptors, which I presume is
> the reason for the hardening glibc was already doing for SUID binaries.
> 
> > "If a standard utility or a conforming application is executed with file
> > descriptor 0 not open for reading or with file descriptor 1 or 2 not open for
> > writing, the environment in which the utility or application is executed shall
> > be deemed non-conforming, and consequently the utility or application might
> > not behave as described in this standard."
> This specific part of the quote would seem to imply that standard input, output
> and error must be opened for reading and writing respectively?
> Or do you think this only applies if the implementation decides to handle
> opening the file descriptors?

In regards to this topic, there's a related issue open against the
standard you should probably look at:

https://austingroupbugs.net/view.php?id=1347

along with the question that inspired it:

https://stackoverflow.com/questions/62052909

It's not exactly the same thing, but it's about the distinction
between FILE and fd modes and which text actually imposes requirements
on the implementation vs on what the application can assume.

Rich
Arsen Arsenović Aug. 19, 2020, 11:45 p.m. UTC | #13
> Those appear to be specifically about the stdio streams, not the file 
> descriptors.  I think a stream that returns an EBADF error for all 
> operations would be perfectly valid for the purposes of those 
> requirements.
Right, ISO C has no concept of file descriptors at all, and the paragraph I
referenced in POSIX is (almost) a copy of the one in C, so yes, it's most
likely talking about the std{in,out,err} objects.

> > This specific part of the quote would seem to imply that standard input, output
> > and error must be opened for reading and writing respectively?
> 
> Yes - that is, what the implementation does if standard input is not open 
> for reading, or standard output and error are not open for writing, is not 
> specified.
The "may" in the sentence indeed does leave it unspecified, but it seems hinted
that these file descriptors should be open for startup. It also says that this
should only happen if a special file descriptors stays closed after exec, so
there is no need to worry about the case where the parent makes fd 0 write only.
And considering the hint, and that the edge case of stdin being closed seems to
have been missed even by the autotools developers (I noticed this when running
autoconf-1.64s ./configure from an automation tool that closed fd 0), which I'd
expect to have noticed this problem in *some* system, I'd say it's probably a
change that can't hurt, but could help, since it seems to have been assumed by
everyone forever.

Also, the sentence talked about here seems to be pretty explicit: either the
implementation opens fd 0 for reading and fds 1 and 2 for writing, or it is
non-conforming.
This also means that the changes Paul was concerned about (due to which
initially I revisioned the patch), actually, should be in the patch also.
Of course, that is under the assumption that this doesn't only apply when the
implementation decides to do something in this case.
Paul Eggert Aug. 20, 2020, 10 p.m. UTC | #14
>>> This specific part of the quote would seem to imply that standard
 >>> input, output and error must be opened for reading and writing
 >>> respectively?
 >>
 >> Yes - that is, what the implementation does if standard input is not
 >> open for reading, or standard output and error are not open for writing,
 >> is not specified.
 > The "may" in the sentence indeed does leave it unspecified, but it seems
 > hinted that these file descriptors should be open for startup.

I'm not following this; what "may" is being referred to here?

Here's the quote again, from
<https://pubs.opengroup.org/onlinepubs/9699919799/functions/execve.html>:

"If a standard utility or a conforming application is executed with file
descriptor 0 not open for reading or with file descriptor 1 or 2 not open for
writing, the environment in which the utility or application is executed shall
be deemed non-conforming, and consequently the utility or application might
not behave as described in this standard."

What this is saying, is that if program A executes program B with messed-up
file descriptors 0, 1, or 2, then B's behavior is not specified. That is,
POSIX does not place any requirement on the Glibc startup in B, and Glibc can
do whatever it wants in this situation.

 > It also says that this should only happen if a special file descriptors
 > stays closed after exec, so there is no need to worry about the case where
 > the parent makes fd 0 write only.

I don't understand this remark.

 > And considering the hint, and that the edge case of stdin being closed
 > seems to have been missed even by the autotools developers (I noticed this
 > when running autoconf-1.64s ./configure from an automation tool that closed
 > fd 0)

If Autoconf doesn't operate well when file descriptor 0 is closed then it
might be worth changing Autoconf, if it's not too much trouble. But because of
the above quotation from POSIX, no change to Autoconf can be solve this
problem on all possible POSIX platforms, because POSIX does not specify how
the 'autoconf' command (which is a shell script) will work if you invoke it
with file descriptor 0 closed.

If you want a portable fix to this problem, you can fix the build procedure
that calls Autoconf so that it does not close file descriptor 0. This is a
good idea anyway, as closing fd 0 can break any program on a POSIX platform.

 > Also, the sentence talked about here seems to be pretty explicit: either
 > the implementation opens fd 0 for reading and fds 1 and 2 for writing, or
 > it is non-conforming.

No, the quotation doesn't say that. It says that if program A invokes program
B with fd 0 closed, then B's behavior is not specified. This is a constraint
on program A, not on the libc implementation.

There's a good reason glibc, if it's to do anything, should open fd 0 with
(say) O_PATH instead of O_RDONLY when fd 0 is closed: this would help catch
bugs in programs. Programs should not rely on a closed fd 0 behaving as if it
were reading from /dev/null.
Arsen Arsenović Aug. 20, 2020, 11:25 p.m. UTC | #15
> >>> This specific part of the quote would seem to imply that standard
> >>> input, output and error must be opened for reading and writing
> >>> respectively?
> >>
> >> Yes - that is, what the implementation does if standard input is not
> >> open for reading, or standard output and error are not open for writing,
> >> is not specified.
> > The "may" in the sentence indeed does leave it unspecified, but it seems
> > hinted that these file descriptors should be open for startup.
> 
> I'm not following this; what "may" is being referred to here?
In this sentence: "If file descriptor 0, 1, or 2 would otherwise be closed after
a successful call to one of the exec family of functions, implementations *MAY*
open an unspecified file for the file descriptor in the new process image."

> "If a standard utility or a conforming application is executed with file
> descriptor 0 not open for reading or with file descriptor 1 or 2 not open for
> writing, the environment in which the utility or application is executed shall
> be deemed non-conforming, and consequently the utility or application might
> not behave as described in this standard."
> 
> What this is saying, is that if program A executes program B with messed-up
> file descriptors 0, 1, or 2, then B's behavior is not specified. That is,
> POSIX does not place any requirement on the Glibc startup in B, and Glibc can
> do whatever it wants in this situation.
I see. So the question becomes is it desirable for glibc to do anything, not
whether POSIX requires it to be so.

> > It also says that this should only happen if a special file descriptors
> > stays closed after exec, so there is no need to worry about the case where
> > the parent makes fd 0 write only.
>
> I don't understand this remark.
This is referring to the quote above, the implementation may decide to open a
file in their place, if it would be otherwise closed in the new process image,
which would mean in the case that the file was open but in the wrong mode (e.g.
stdin as write only) glibc should do nothing, eliminating that case, and the
analog cases for stdout/err.

> > And considering the hint, and that the edge case of stdin being closed
> > seems to have been missed even by the autotools developers (I noticed this
> > when running autoconf-1.64s ./configure from an automation tool that closed
> > fd 0)
> 
> If Autoconf doesn't operate well when file descriptor 0 is closed then it
> might be worth changing Autoconf, if it's not too much trouble. But because of
> the above quotation from POSIX, no change to Autoconf can be solve this
> problem on all possible POSIX platforms, because POSIX does not specify how
> the 'autoconf' command (which is a shell script) will work if you invoke it
> with file descriptor 0 closed.
> 
> If you want a portable fix to this problem, you can fix the build procedure
> that calls Autoconf so that it does not close file descriptor 0. This is a
> good idea anyway, as closing fd 0 can break any program on a POSIX platform.
I've already implemented a fix in the build procedure, but as I understood it
at the time, the problem was external at it's root.

> > Also, the sentence talked about here seems to be pretty explicit: either
> > the implementation opens fd 0 for reading and fds 1 and 2 for writing, or
> > it is non-conforming.
> 
> No, the quotation doesn't say that. It says that if program A invokes program
> B with fd 0 closed, then B's behavior is not specified. This is a constraint
> on program A, not on the libc implementation.
> 
> There's a good reason glibc, if it's to do anything, should open fd 0 with
> (say) O_PATH instead of O_RDONLY when fd 0 is closed: this would help catch
> bugs in programs. Programs should not rely on a closed fd 0 behaving as if it
> were reading from /dev/null.
I agree, that makes most sense if the constraint I was talking about is not on
glibc, which it appears to be the case. It would also explain why this was
previously done for SUID binaries, bugs in those can be very dangerous, but bugs
outside of those can be very dangerous, too, for the same reasons (say, an
application writing a log message to fd 3 directly, which, due to the caller
closing fd 3 actually turned out to be a database connection, it is better to
fail than to send possibly dangerous content to an undefined place).
Zack Weinberg Aug. 27, 2020, 3:56 p.m. UTC | #16
On Thu, Aug 20, 2020 at 6:00 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
> Arsen Arsenović wrote:
>  > And considering the hint, and that the edge case of stdin being closed
>  > seems to have been missed even by the autotools developers (I noticed this
>  > when running autoconf-1.64s ./configure from an automation tool that closed
>  > fd 0)
>
> If Autoconf doesn't operate well when file descriptor 0 is closed then it
> might be worth changing Autoconf, if it's not too much trouble.

I looked into making that change.  However, it is very difficult to
detect and handle closed fds 0,1,2 reliably in a shell script.  In C
it's easy (just call fcntl([012], F_GETFL, 0)) but shell programs
cannot make that system call directly, and all of the alternatives
have significant limitations (e.g. not being able to tell whether a fd
is open for *writing*), run foul of bugs (e.g. `exec 3>&0` will
succeed with Solaris /bin/sh, regardless of whether fd 0 was open), or
are not portable enough to use as the sole method (e.g. Linux's
/proc/<pid>/fdinfo/<fd> virtual files).  If you're curious what I did,
see https://lists.gnu.org/archive/html/autoconf-patches/2020-08/msg00025.html
and msg00026.html, but right now I am inclined *not* to put those
changes into the upcoming Autoconf 2.70.

After the experience of writing those patches, I'm definitely in favor
of GNU libc making a guarantee that fds 0,1,2 are open when
application code starts executing, for all processes, regardless of
what POSIX does or does not say.

zw
Paul Eggert Aug. 27, 2020, 6:21 p.m. UTC | #17
On 8/27/20 8:56 AM, Zack Weinberg wrote:
> 
> https://lists.gnu.org/archive/html/autoconf-patches/2020-08/msg00025.html
> ... but right now I am inclined *not*  to put those
> changes into the upcoming Autoconf 2.70.

I responded in 
<https://lists.gnu.org/archive/html/autoconf-patches/2020-08/msg00026.html> that 
there's a simple (3-line - see below) and portable-enough solution to the 
problem in Autoconf, since all we need to do is ensure that FDs 0 through 2 are 
open, not that they're open the right way.

> After the experience of writing those patches, I'm definitely in favor
> of GNU libc making a guarantee that fds 0,1,2 are open when
> application code starts executing, for all processes, regardless of
> what POSIX does or does not say.

I agree they should be open. But it's better for FD 0 to be write-only and FDs 
1+2 to be read-only, as that's more likely to prevent buggy programs from 
misbehaving further. For Autoconf, I suggest:

      (exec 3<&0) 2>/dev/null || exec 0>/dev/null
      (exec 3>&1) 2>/dev/null || exec 1</dev/null
      (exec 3>&2) || exec 2</dev/null

to do something similar at the shell level.
Arsen Arsenović Aug. 28, 2020, 12:12 a.m. UTC | #18
Excuse me, I sent this message, by mistake, to Paul directly, without CCing the
mailing list. To further discussion, I am reposting this. I'm sorry for the
inconvenience:

> I agree they should be open. But it's better for FD 0 to be write-only and
> FDs 1+2 to be read-only, as that's more likely to prevent buggy programs
> from misbehaving further.
Since I agree with this too, we have an agreement on what should happen(?):
1) update comments in the patch to be more specific
2) ensure "wrong direction" opens are used to detect errors (like they were
   originally)

I can do this tomorrow in spare time, if that is what the team sees as the
right thing to do. I'm waiting for further comments in case I missed something.
Arsen Arsenović Sept. 4, 2020, 5:49 p.m. UTC | #19
On 20/08/28 02:12, Arsen Arsenović via Libc-alpha wrote:
> Excuse me, I sent this message, by mistake, to Paul directly, without CCing the
> mailing list. To further discussion, I am reposting this. I'm sorry for the
> inconvenience:
> 
> > I agree they should be open. But it's better for FD 0 to be write-only and
> > FDs 1+2 to be read-only, as that's more likely to prevent buggy programs
> > from misbehaving further.
> Since I agree with this too, we have an agreement on what should happen(?):
> 1) update comments in the patch to be more specific
> 2) ensure "wrong direction" opens are used to detect errors (like they were
>    originally)
> 
> I can do this tomorrow in spare time, if that is what the team sees as the
> right thing to do. I'm waiting for further comments in case I missed something.
> 
> -- 
> Arsen Arsenović

ping
Arsen Arsenović Sept. 11, 2020, 7:47 a.m. UTC | #20
On 20/08/28 02:12, Arsen Arsenović via Libc-alpha wrote:
> Excuse me, I sent this message, by mistake, to Paul directly, without CCing the
> mailing list. To further discussion, I am reposting this. I'm sorry for the
> inconvenience:
> 
> > I agree they should be open. But it's better for FD 0 to be write-only and
> > FDs 1+2 to be read-only, as that's more likely to prevent buggy programs
> > from misbehaving further.
> Since I agree with this too, we have an agreement on what should happen(?):
> 1) update comments in the patch to be more specific
> 2) ensure "wrong direction" opens are used to detect errors (like they were
>    originally)
> 
> I can do this tomorrow in spare time, if that is what the team sees as the
> right thing to do. I'm waiting for further comments in case I missed something.
> 
> -- 
> Arsen Arsenović

ping^2
diff mbox series

Patch

diff --git a/csu/check_fds.c b/csu/check_fds.c
index 30634b81..d2bca0a3 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
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;