Message ID | 20200819124124.17481-1-arsen@aarsen.me |
---|---|
State | Changes Requested, archived |
Headers |
Return-Path: <libc-alpha-bounces@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 030CC386F44F; Wed, 19 Aug 2020 12:41:54 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 030CC386F44F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1597840914; bh=InShBhIKoa6P6sw/nzu/4j87uuE//8II/V/VMu4gSH8=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=pQktMj8UmyHoT138v9D4+XpeAu2NT2O8Hs8OqI8tl2QTY0x/8Th4kAAo2mYjmfw2P 3sIVQWCdkU/69Ky/DkeciXllAdkCN3A5j373mP25lc6mp0b3AEQ+g8yYXh6DOuP8kd DKdQc/hcPAKU6jgewUdtGofjYw4/fHF97wjsGr6g= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mout-p-102.mailbox.org (mout-p-102.mailbox.org [IPv6:2001:67c:2050::465:102]) by sourceware.org (Postfix) with ESMTPS id 830FF3857C54 for <libc-alpha@sourceware.org>; Wed, 19 Aug 2020 12:41:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 830FF3857C54 Received: from smtp2.mailbox.org (smtp2.mailbox.org [IPv6:2001:67c:2050:105:465:1:2:0]) (using TLSv1.2 with cipher ECDHE-RSA-CHACHA20-POLY1305 (256/256 bits)) (No client certificate requested) by mout-p-102.mailbox.org (Postfix) with ESMTPS id 4BWnTn53yjzKmgv for <libc-alpha@sourceware.org>; Wed, 19 Aug 2020 14:41:49 +0200 (CEST) X-Virus-Scanned: amavisd-new at heinlein-support.de Received: from smtp2.mailbox.org ([80.241.60.241]) by spamfilter01.heinlein-hosting.de (spamfilter01.heinlein-hosting.de [80.241.56.115]) (amavisd-new, port 10030) with ESMTP id s5LKuSXAsOYA for <libc-alpha@sourceware.org>; Wed, 19 Aug 2020 14:41:46 +0200 (CEST) To: libc-alpha@sourceware.org Subject: [PATCH v2] Ensure standard file descriptors are open on start Date: Wed, 19 Aug 2020 14:41:25 +0200 Message-Id: <20200819124124.17481-1-arsen@aarsen.me> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-MBO-SPAM-Probability: * X-Rspamd-Score: 0.80 / 15.00 / 15.00 X-Rspamd-Queue-Id: BA8F21798 X-Rspamd-UID: 2439b6 X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_INFOUSMEBIZ, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list <libc-alpha.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=subscribe> From: =?utf-8?q?Arsen_Arsenovi=C4=87_via_Libc-alpha?= <libc-alpha@sourceware.org> Reply-To: =?utf-8?q?Arsen_Arsenovi=C4=87?= <arsen@aarsen.me> Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" <libc-alpha-bounces@sourceware.org> |
Series |
[v2] Ensure standard file descriptors are open on start
|
|
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
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
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
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).
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?
* 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.
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.
* 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.
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.
> 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?
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ć
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.
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
> 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.
>>> 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.
> >>> 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).
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
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.
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.
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
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 --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;