Bug 13983 - __libc_message() shouldn't blindly write to STDERR_FILENO
Summary: __libc_message() shouldn't blindly write to STDERR_FILENO
Status: WAITING
Alias: None
Product: glibc
Classification: Unclassified
Component: libc (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Marek Polacek
URL: http://www.austingroupbugs.net/view.p...
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-15 17:01 UTC by William Pitcock
Modified: 2014-06-25 11:16 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
check-stderr.patch (656 bytes, patch)
2012-04-15 23:24 UTC, Marek Polacek
Details | Diff
check-stderr-v2.patch (547 bytes, patch)
2012-04-16 08:52 UTC, William Pitcock
Details | Diff
check-stderr-v3.patch (549 bytes, patch)
2012-04-16 09:09 UTC, William Pitcock
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description William Pitcock 2012-04-15 17:01:40 UTC
hi,

blindly writing to STDERR_FILENO is not posix-compliant, it is possible that STDERR_FILENO may have been reassigned to a different descriptor.

see austin group bug #555 for more information.

i believe the solution is to check if STDERR_FILENO is a TTY (using isatty()) before using it, if it is not a TTY and there is no /dev/tty available to us, then we should just go directly to vsyslog() branch.
Comment 1 Marek Polacek 2012-04-15 23:18:13 UTC
E.g. here:

$ cat x.c
#include <stdio.h>
#include <string.h>

int
main (void)
{
  fclose (stderr);
  char a[5] = { };
  strcpy (a, "foobarbaz");
}
$ gcc -O2 -D_FORTIFY_SOURCE=2 x.c
$ LIBC_FATAL_STDERR_=1 ./testrun.sh ./a.out
Aborted

we use stderr stream after it's been closed, which is undefined; it looks like we really should check STDERR_FILENO before writing to it.
Comment 2 Marek Polacek 2012-04-15 23:24:11 UTC
Created attachment 6342 [details]
check-stderr.patch

Untested fix.
Comment 3 William Pitcock 2012-04-16 01:07:32 UTC
hi,

actually, isatty() is not sufficient here as it will break stderr redirection.

however, the intention of this function is that it is meant to print to a TTY, so perhaps breaking stderr redirection is not a big deal?

i am using a similar patch locally.
Comment 4 Rich Felker 2012-04-16 04:55:59 UTC
Personally, I would flag this INVALID. The only times __libc_message message are called are when the calling program has already invoked undefined behavior, so there is no question of whether this violating requirements of POSIX; once you invoke UB, POSIX has nothing more to say.

With that said, there is some security argument to be made for avoiding potentially-dangerous writes. However, I think it's mostly nullified by the stupidity of closing fd 2. The correct way to suppress stderr is to open /dev/null and dup2 it onto fd 2. If you close any of the standard file descriptors, it's your responsibility to make sure that no further file descriptors are created, or if they are, that no functions that would perform io on the new file descriptor expecting it to be one of the standard streams can be called.

Note that isatty is not sufficient to avoid clobbering unrelated data; there are plenty of non-interactive (even non-tty-like) uses for ptys, and one could envision uses where erroneously writing text directed for stderr into them could be very destructive.
Comment 5 Paul Pluzhnikov 2012-04-16 05:59:03 UTC
(In reply to comment #4)
> Personally, I would flag this INVALID.

In addition, dumping messages to /dev/tty is just plain wrong.

I run some nightly builds from cron, and redirect their stdout/stderr.

Recently I discovered that if I run the same build by hand, I get several error messages in my terminal. I have *no clue* which commands they are coming from. They are *not* useful.

(Now that I looked at __libc_message, I am going to set LIBC_FATAL_STDERR_ in the environment to direct them back to stderr.)
Comment 6 William Pitcock 2012-04-16 07:02:41 UTC
(In reply to comment #4)
> Personally, I would flag this INVALID. The only times __libc_message message
> are called are when the calling program has already invoked undefined behavior,
> so there is no question of whether this violating requirements of POSIX; once
> you invoke UB, POSIX has nothing more to say.

No. POSIX has something to say: don't write to stderr (including STDERR_FILENO) if someone closed it.  See Austin Group bug #555.

Invoking undefined behaviour by causing a memory error or fortify error does not give the libc license to invoke further undefined behaviour.  Sorry, but it doesn't.

> 
> With that said, there is some security argument to be made for avoiding
> potentially-dangerous writes. However, I think it's mostly nullified by the
> stupidity of closing fd 2. The correct way to suppress stderr is to open
> /dev/null and dup2 it onto fd 2.

This is not presently mandated by POSIX.  See Austin Group bug #555.  I have suggested to them that they should mandate this in their specification for fclose().  That said, we still should not blindly write to STDERR_FILENO.

> If you close any of the standard file descriptors, it's your responsibility
> to make sure that no further file descriptors are created, or if they are, 
> that no functions that would perform io on the new file descriptor expecting
> it to be one of the standard streams can be called.

Which is impossible because libc blindly performs I/O on the new file descriptor because it calls writev() on STDERR_FILENO, without checking if it is still stderr or not.  This is at least presently incorrect per POSIX, unless such a crash debugger is explicitly requested by the end user.  See Austin Group bug #555.

> 
> Note that isatty is not sufficient to avoid clobbering unrelated data; there
> are plenty of non-interactive (even non-tty-like) uses for ptys, and one could
> envision uses where erroneously writing text directed for stderr into them
> could be very destructive.

As I said in comment 3, isatty is not a sufficient diagnostic anyway.
Comment 7 William Pitcock 2012-04-16 07:10:38 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > Personally, I would flag this INVALID.
> 
> In addition, dumping messages to /dev/tty is just plain wrong.
> 
> I run some nightly builds from cron, and redirect their stdout/stderr.
> 
> Recently I discovered that if I run the same build by hand, I get several error
> messages in my terminal. I have *no clue* which commands they are coming from.
> They are *not* useful.

The idea here is that libc doesn't leak private information to log files which might be accessible by people on the box that you wouldn't want to know just succeeded in a specific kind of memory corruption or buffer overflow or whatever.

From a security standpoint this is probably a good thing.

> (Now that I looked at __libc_message, I am going to set LIBC_FATAL_STDERR_ in
> the environment to direct them back to stderr.)

This is fine too, but glibc should make sure stderr really is stderr.
Comment 8 Marek Polacek 2012-04-16 08:19:06 UTC
I'm not sure anymore whether this is worth fixing.

(In reply to comment #7)
> This is fine too, but glibc should make sure stderr really is stderr.

Ok, but how should we do that?
Comment 9 William Pitcock 2012-04-16 08:33:46 UTC
(In reply to comment #8)
> I'm not sure anymore whether this is worth fixing.
> 
> (In reply to comment #7)
> > This is fine too, but glibc should make sure stderr really is stderr.
> 
> Ok, but how should we do that?

At libio/iofclose.c:85, there is:

  if (fp != _IO_stdin && fp != _IO_stdout && fp != _IO_stderr)
    {
      fp->_IO_file_flags = 0;
      free(fp);
    }

This means we can always use fwrite on _IO_stdin, _IO_stdout, _IO_stderr, as they will always stay around.  So, we can just use fwrite() with static buffers, as they will have fp->_fileno = -1, which will silently fail.  Or we can just use _IO_stderr->_fileno instead of STDERR_FILENO define.
Comment 10 William Pitcock 2012-04-16 08:48:11 UTC
Test-building a patch which fixes this by looking at _IO_stderr.
Comment 11 William Pitcock 2012-04-16 08:52:48 UTC
Created attachment 6343 [details]
check-stderr-v2.patch

This patch works with the testcase and results in the error being sent to syslog as expected.
Comment 12 Marek Polacek 2012-04-16 08:58:47 UTC
Thanks, can you please post it to libc-alpha for review?
Comment 13 William Pitcock 2012-04-16 09:09:38 UTC
Created attachment 6344 [details]
check-stderr-v3.patch

A stronger check would be to compare against STDERR_FILENO instead of -1.  This allows us to check that fd=2 is explictly stderr.
Comment 14 William Pitcock 2012-04-16 09:10:25 UTC
Hi,

Yes.  I will post a GIT patch momentarily to libc-alpha.
Comment 15 Rich Felker 2012-04-16 10:22:04 UTC
William, you're wrong. Undefined behavior is undefined behavior. A great example would be if you overflow a buffer on the stack (without fortify) and overwrite the return value. Now, when your program is run with input from an attacker, the attacker's shellcode (which is not part of your program) is able to write to fd #2 all it likes. This does not violate the requirements of POSIX because undefined behavior is undefined behavior. If you invoke undefined behavior, ANYTHING can happen. All bets are off. Note that, per the C standard, undefined behavior is even prescient. It's not just code after the UB is invoked that can do ANYTHING. Even code that runs conceptually before the UB is invoked can do anything, provided it can be determined that UB is invoked at some point in the program.

If you don't like this, you should be programming in Java rather than C.
Comment 16 William Pitcock 2012-04-16 16:44:37 UTC
(In reply to comment #15)
> William, you're wrong. Undefined behavior is undefined behavior. A great
> example would be if you overflow a buffer on the stack (without fortify) and
> overwrite the return value. Now, when your program is run with input from an
> attacker, the attacker's shellcode (which is not part of your program) is able
> to write to fd #2 all it likes. This does not violate the requirements of POSIX
> because undefined behavior is undefined behavior. If you invoke undefined
> behavior, ANYTHING can happen. All bets are off. Note that, per the C standard,
> undefined behavior is even prescient. It's not just code after the UB is
> invoked that can do ANYTHING. Even code that runs conceptually before the UB is
> invoked can do anything, provided it can be determined that UB is invoked at
> some point in the program.
> 
> If you don't like this, you should be programming in Java rather than C.

If you believe that libc should blindly write to fd=2, then by all means implement such functionality in Musl.

The fact of the matter is that there is a patch to ensure that we do not have undefined behaviour.

If you want developers to dup2, then push for an update to POSIX.1 to have fclose() documentation explicitly instruct people to use dup2.  There is presently no such documentation, and the general consensus (that we should not use STDERR_FILENO if stderr is closed) appears to be that my interpretation is correct given the current specification.  Thusly, this *is* a bug and if it should not be, then the specification should be corrected to allow this behaviour.

Explicitly allowing usage of STD..._FILENO when the streams associated with those _FILENOs have been closed would require updating the specification as well.
Comment 17 Rich Felker 2012-04-16 17:21:32 UTC
Hey, I didn't mean to stir up such reactions... I agree it's bad for glibc to make these writes. My point was that it's a quality of implementation issue rather than a conformance issue. My preference (and action in musl, since you brought it up) is never to write debugging messages at all, and simply crash as soon as possible when memory corruption has been detected; this is also the most secure route since it avoids making any further function calls (which, if the GOT/PLT was corrupted, could be unsafe). I'll admit it's also the least informative, as it requires core files or running under a debugger to discover what happened.

With that said, glibc has chosen the approach of being verbose and attempting to report some forms of UB to the user through messages. Originally (based on the non-working isatty approach) I thought it prohibitively difficult to avoid the situation, but since there's now a clean and working patch for the issue, I withdraw my claim that the bug report is invalid. I think the fallback syslog path could use some auditing though. Unless it's extremely simple, it could cause programs in a corrupt state to hang (deadlock issues) or worse; just losing the debug output in this case may be the lesser of two evils.

I still maintain that an application which calls fclose(stderr) is broken, and if it's doing anything nontrivial, probably nonportable. For example, if it calls a library which happens to try to write something to stderr, and stderr has been closed, the result is undefined behavior. It may work on many implementations, but it's a bad idea. It may be nice for glibc's documentation to recommend alternatives to fclose for the standard streams (for instance, freopen or dup2 with /dev/null, though freopen has some of its own pitalls).
Comment 18 OndrejBilka 2013-06-11 17:36:04 UTC
A search where this was posted revealed this thread.

http://comments.gmane.org/gmane.comp.lib.glibc.alpha/19820

William, could you post v2 with comments from that thread.