some fixes to musl

some fixes to musl

[Vasiliy Kulikov]

Hi,

This is a patch against v0.7.12.  It is only compile tested.

fdopendir():
- POSIX defines EBADF if fd is invalid.  I suppose it is OK
  to assume fd is invalid if fstat() failed.
- fcntl(fd, F_SETFD, FD_CLOEXEC) may fail (at least in theory).  If it
  fails, the consequences are not expected.

rewinddir(), seekdir():
- lseek(fd, 0, SEEK_SET) may fail too, I suppose it can be true for
  network file systems or FUSE fses.  Continuing the work with pos=-1 is
  somewhat not expected.

cuserid():
- POSIX says an argument may be NULL, in this case the buffer should be
  statically allocated.

forkpty():
- It should be guaranteed that master fd is closed, tty is setup, slave
  fd is dup'ed to 1,2,3.  The latter can be broken by setting small
  rlimit.  setsid() is checked for company :)  I think the only way to
  handle the failure is _exit().  While it may be not the best choise,
  however, continuing the work with half dropped privileges is more
  dangerous.

openpty():
- close() shouldn't change errno updated by failed ioctl()/open().
- I suppose the last calls to tcsetattr() and ioctl() may fail too.

realpath() (no patch):
- IMO the logic is wrong.  While opening the file and asking the kernel
  the full path is interesting, it won't work with not readable files.
  Also if a file is a device or tty the opening might have unexpected
  side effects.  I don't dare to reimplement it myself, though ;-)

_vsyslog():
- sendto() may fail in case of signal delivery.


I wonder what is the Right Way to handle close() failures in generic
case.  If it is fork'ish function, _exit() can be used.  If it is some
privilege dropping function, failure can be returned in errno, but the
task state (hanged/unclosed fd) is not expected by the caller.

More dangerous case is function that is not designed to return error at
all.  E.g. close() inside of closelog() may fail, but the caller cannot
be notified about it by design.  If the caller want to do chroot(),
which prevents re-opening log fd, the failure would break task's
expectation of dropped privileges (closed log fd).

diff --git a/src/dirent/fdopendir.c b/src/dirent/fdopendir.c
index c4b8e61..0aaa735 100644
--- a/src/dirent/fdopendir.c
+++ b/src/dirent/fdopendir.c
@@ -12,7 +12,11 @@ DIR *fdopendir(int fd)
 	DIR *dir;
 	struct stat st;
 
-	if (fstat(fd, &st) < 0 || !S_ISDIR(st.st_mode)) {
+	if (fstat(fd, &st) < 0) {
+		errno = EBADF;
+		return 0;
+	}
+	if (!S_ISDIR(st.st_mode)) {
 		errno = ENOTDIR;
 		return 0;
 	}
@@ -20,7 +24,10 @@ DIR *fdopendir(int fd)
 		return 0;
 	}
 
-	fcntl(fd, F_SETFD, FD_CLOEXEC);
+	if (fcntl(fd, F_SETFD, FD_CLOEXEC)) {
+		free(dir);
+		return 0;
+	}
 	dir->fd = fd;
 	return dir;
 }
diff --git a/src/dirent/rewinddir.c b/src/dirent/rewinddir.c
index c6138f7..afa80ac 100644
--- a/src/dirent/rewinddir.c
+++ b/src/dirent/rewinddir.c
@@ -6,8 +6,9 @@
 void rewinddir(DIR *dir)
 {
 	LOCK(&dir->lock);
-	lseek(dir->fd, 0, SEEK_SET);
-	dir->buf_pos = dir->buf_end = 0;
-	dir->tell = 0;
+	if (lseek(dir->fd, 0, SEEK_SET) != (off_t)-1) {
+		dir->buf_pos = dir->buf_end = 0;
+		dir->tell = 0;
+	}
 	UNLOCK(&dir->lock);
 }
diff --git a/src/dirent/seekdir.c b/src/dirent/seekdir.c
index 81a0e33..df4f403 100644
--- a/src/dirent/seekdir.c
+++ b/src/dirent/seekdir.c
@@ -5,8 +5,13 @@
 
 void seekdir(DIR *dir, long off)
 {
+	off_t pos;
+
 	LOCK(&dir->lock);
-	dir->tell = lseek(dir->fd, off, SEEK_SET);
-	dir->buf_pos = dir->buf_end = 0;
+	pos = lseek(dir->fd, off, SEEK_SET);
+	if (pos != (off_t)-1) {
+		dir->tell = pos;
+		dir->buf_pos = dir->buf_end = 0;
+	}
 	UNLOCK(&dir->lock);
 }
diff --git a/src/misc/cuserid.c b/src/misc/cuserid.c
index 4e78798..4bf9450 100644
--- a/src/misc/cuserid.c
+++ b/src/misc/cuserid.c
@@ -7,6 +7,10 @@ char *cuserid(char *buf)
 {
 	struct passwd pw, *ppw;
 	long pwb[256];
+	static char cbuff[L_cuserid];
+
+	if (buf == NULL)
+		buf = cbuff;
 	if (getpwuid_r(geteuid(), &pw, (void *)pwb, sizeof pwb, &ppw))
 		return 0;
 	snprintf(buf, L_cuserid, "%s", pw.pw_name);
diff --git a/src/misc/forkpty.c b/src/misc/forkpty.c
index 0bbf2de..2fc0dec 100644
--- a/src/misc/forkpty.c
+++ b/src/misc/forkpty.c
@@ -10,12 +10,13 @@ int forkpty(int *m, char *name, const struct termios *tio, const struct winsize
 	if (openpty(m, &s, name, tio, ws) < 0) return -1;
 	pid = fork();
 	if (!pid) {
-		close(*m);
-		setsid();
-		ioctl(s, TIOCSCTTY, (char *)0);
-		dup2(s, 0);
-		dup2(s, 1);
-		dup2(s, 2);
+		if (close(*m) ||
+		    (setsid() == (pid_t)-1) ||
+		    (ioctl(s, TIOCSCTTY, (char *)0) == -1) ||
+		    (dup2(s, 0) == -1) ||
+		    (dup2(s, 1) == -1) ||
+		    (dup2(s, 2) == -1))
+			_exit(1);
 		if (s>2) close(s);
 		return 0;
 	}
diff --git a/src/misc/openpty.c b/src/misc/openpty.c
index 0b4eb22..14e4329 100644
--- a/src/misc/openpty.c
+++ b/src/misc/openpty.c
@@ -3,6 +3,7 @@
 #include <unistd.h>
 #include <pty.h>
 #include <stdio.h>
+#include <errno.h>
 
 /* Nonstandard, but vastly superior to the standard functions */
 
@@ -10,24 +11,41 @@ int openpty(int *m, int *s, char *name, const struct termios *tio, const struct
 {
 	int n=0;
 	char buf[20];
+	int old_errno;
 
 	*m = open("/dev/ptmx", O_RDWR|O_NOCTTY);
 	if (!*m) return -1;
 
 	if (ioctl(*m, TIOCSPTLCK, &n) || ioctl (*m, TIOCGPTN, &n)) {
+		old_errno = errno;
 		close(*m);
+		errno = old_errno;
 		return -1;
 	}
 
 	if (!name) name = buf;
 	snprintf(name, sizeof buf, "/dev/pts/%d", n);
 	if ((*s = open(name, O_RDWR|O_NOCTTY)) < 0) {
+		old_errno = errno;
 		close(*m);
+		errno = old_errno;
 		return -1;
 	}
 
-	if (tio) tcsetattr(*s, TCSANOW, tio);
-	if (ws) ioctl(*s, TIOCSWINSZ, ws);
+	if (tio && tcsetattr(*s, TCSANOW, tio)) {
+		old_errno = errno;
+		close(*s);
+		close(*m);
+		errno = old_errno;
+		return -1;
+	}
+	if (ws && ioctl(*s, TIOCSWINSZ, ws)) {
+		old_errno = errno;
+		close(*s);
+		close(*m);
+		errno = old_errno;
+		return -1;
+	}
 
 	return 0;
 }
diff --git a/src/misc/syslog.c b/src/misc/syslog.c
index cbe6520..5cd5a09 100644
--- a/src/misc/syslog.c
+++ b/src/misc/syslog.c
@@ -8,6 +8,7 @@
 #include <signal.h>
 #include <string.h>
 #include <pthread.h>
+#include <errno.h>
 #include "libc.h"
 
 static int lock;
@@ -73,6 +74,8 @@ static void _vsyslog(int priority, const char *message, va_list ap)
 	char buf[256];
 	int pid;
 	int l, l2;
+	ssize_t sent;
+	int pos;
 
 	if (log_fd < 0) {
 		__openlog(log_ident, log_opt | LOG_NDELAY, log_facility);
@@ -95,7 +98,17 @@ static void _vsyslog(int priority, const char *message, va_list ap)
 	if (l2 >= 0) {
 		l += l2;
 		if (buf[l-1] != '\n') buf[l++] = '\n';
-		sendto(log_fd, buf, l, 0, (void *)&log_addr, 11);
+		pos = 0;
+		while (l) {
+			sent = sendto(log_fd, buf+pos, l, 0, (void *)&log_addr, 11);
+			if (sent == -1) {
+				if (errno == EINTR || errno == EAGAIN)
+					continue;
+				break;
+			}
+			l -= sent;
+			pos += sent;
+		}
 	}
 
 	UNLOCK(&lock);
--

Re: some fixes to musl

[Rich Felker]

On Thu, Jul 21, 2011 at 09:02:55PM +0400, Vasiliy Kulikov wrote:
> Hi,
> 
> This is a patch against v0.7.12.  It is only compile tested.

Thanks. Can you look over my below review of the issues and see if you
agree on what's actually an issue and what's not. I like to avoid
tinfoil-hat programming -- that is, testing for error conditions from
interfaces that have no standard reason to fail when used in the way
they're being used, and for which it would be a very poor
quality-of-implementation issue for a kernel implementation to add new
implementation-specific failure conditions.

I'm aware of course that some interfaces *can* fail for nonstandard
reasons under Linux (dup2 and set*uid come to mind), and I've tried to
work around these and shield applications from the brokenness...

> fdopendir():
> - POSIX defines EBADF if fd is invalid.  I suppose it is OK
>   to assume fd is invalid if fstat() failed.

fstat will already set errno to EBADF if appropriate. In that case we
just need to avoid overwriting it. I'll fix that by separating failure
into two conditionals.

> - fcntl(fd, F_SETFD, FD_CLOEXEC) may fail (at least in theory).  If it
>   fails, the consequences are not expected.

The only way it can fail is if fd is invalid, which was already tested
by fstat. If another thread or signal handler closes fd here and
causes a race condition, behavior is going to be unpredictable
(undefined) anyway.

> rewinddir(), seekdir():
> - lseek(fd, 0, SEEK_SET) may fail too, I suppose it can be true for
>   network file systems or FUSE fses.  Continuing the work with pos=-1 is
>   somewhat not expected.

I'm not sure what could be meaningfully done in that case... I would
actually consider it a broken fs driver if lseek to 0 ever fails on a
directory but it's worth investigating if it's a real issue.

> cuserid():
> - POSIX says an argument may be NULL, in this case the buffer should be
>   statically allocated.

I'll check it out.

> forkpty():
> - It should be guaranteed that master fd is closed, tty is setup, slave
>   fd is dup'ed to 1,2,3.  The latter can be broken by setting small
>   rlimit.

The only way it could fail is if one of these fds was not open to
begin with, which would be pretty unusual. It's unclear to me whether
it's better to fail early or fail later.. unfortunately there's no
good way to report failure to the caller after fork succeeds. Perhaps
I could attempt fcntl F_DUPFD on the slave pty in the parent process
before forking to reserve fd slots 0-2 if they're not already taken...

>  setsid() is checked for company :)  I think the only way to

I'm pretty sure setsid cannot fail. There's no justifiable reason for
failure. POSIX says:

  [EPERM] The calling process is already a process group leader, or
  the process group ID of a process other than the calling process
  matches the process ID of the calling process.

So supposedly it can fail if the process id of an old process what was
previously a process group leader got reused while some processes in
the old process group had not yet terminated. I would contend that a
quality implementation would not allow a process id to be reassigned
if any processes still have that id as their process group id. Do you
know if Linux satisfies this condition?

>   handle the failure is _exit().  While it may be not the best choise,
>   however, continuing the work with half dropped privileges is more
>   dangerous.

Can you clarify what you mean about "dropped privileges"?

> openpty():
> - close() shouldn't change errno updated by failed ioctl()/open().

This call to close cannot fail short of another thread or signal
handler poking at file desciptor numbers that don't belong to it.

> - I suppose the last calls to tcsetattr() and ioctl() may fail too.

Indeed, and this is testable/reportable. I suppose it would be best to
mimic what existing implementations do here.

> realpath() (no patch):

This function is just a hack to stand in until I write a real version
of it. See the commit message. I agree totally that it's wrong.

> _vsyslog():
> - sendto() may fail in case of signal delivery.

This should probably be tested and retried since syslog cannot report
errors to the caller.

> I wonder what is the Right Way to handle close() failures in generic
> case.

I think 99% of them are complete non-issues. The whole idea that close
can fail (for reasons other than EBADF) is a misdesign, but it can
generally only happen for odd devices or NFS (which is broken in many
more fundamental ways anyway). It certainly can't fail for terminal
devices, pipes, sockets, directories, etc. which are the only places
libc needs it.

>  If it is fork'ish function, _exit() can be used.  If it is some
> privilege dropping function, failure can be returned in errno, but the
> task state (hanged/unclosed fd) is not expected by the caller.
> 
> More dangerous case is function that is not designed to return error at
> all.  E.g. close() inside of closelog() may fail, but the caller cannot
> be notified about it by design.  If the caller want to do chroot(),
> which prevents re-opening log fd, the failure would break task's
> expectation of dropped privileges (closed log fd).

Non-issue. close cannot fail on a local unix socket.

Rich

Re: some fixes to musl

[Solar Designer]

Rich,

On Thu, Jul 21, 2011 at 02:21:01PM -0400, Rich Felker wrote:
> [...] I like to avoid
> tinfoil-hat programming -- that is, testing for error conditions from
> interfaces that have no standard reason to fail when used in the way
> they're being used, and for which it would be a very poor
> quality-of-implementation issue for a kernel implementation to add new
> implementation-specific failure conditions.

Yet such things will and do happen, and the kernel developers find good
reasons to justify those additional failure conditions.  In many cases,
it is difficult to even figure out whether a certain syscall can fail
and with what specific error codes - those come from far deeper layers,
sometimes even from third-party modules.  Maybe this is wrong or maybe
not, but it's the reality.

So I respectfully disagree with your approach to this.

> I'm aware of course that some interfaces *can* fail for nonstandard
> reasons under Linux (dup2 and set*uid come to mind), and I've tried to
> work around these and shield applications from the brokenness...

In a discussion currently on LKML, everyone appears to agree that it's
the applications not checking the return value that are broken and that
it's unfortunate that the kernel has to include workarounds for such
broken applications - such as deferring setuid() failure to execve().

My current opinion is that _both_ applications (and libraries) not
checking return value from "can't fail" syscalls are broken and the
kernel is broken for introducing new failure conditions to some
syscalls that historically "couldn't fail" and where such ignored
failures would have security consequences.  When there are no security
consequences, I am OK with the kernel introducing new failure
conditions - the applications (and libraries) need to be improved over
time to check the return value, which they should have always been doing
in the first place.

I understand that you want to keep the code small and clean rather than
cluttered with tinfoil-hat return value checks, but I am afraid this
approach may only be acceptable when you make calls to your own
interfaces, where you control the possible failure conditions.  And even
then things may break when you make changes to those interfaces later,
but forget to add the return value checks elsewhere in your code...

Well, maybe you can make exceptions for "can't happen" failures where
the impact of the failure if it does happen is negligible... but then
it is non-obvious where to draw the line.  Is the impact of a failed
close() negligible?  Not necessarily; it could even be a security hole
if the fd gets inherited by an untrusted child process.

Personally, I've been using different approaches to this in different
ones of my programs.  For musl, I think the "always check" approach may
be the better one.  Yes, the code size increase from those error
handling paths is unfortunate...  Some use of goto can make them smaller
and keep them out of the same cache lines with actually running code.

Just an opinion.

Alexander

Re: some fixes to musl

[Vasiliy Kulikov]

On Thu, Jul 21, 2011 at 14:21 -0400, Rich Felker wrote:
> > fdopendir():
> > - POSIX defines EBADF if fd is invalid.  I suppose it is OK
> >   to assume fd is invalid if fstat() failed.
> 
> fstat will already set errno to EBADF if appropriate.

I think it's wrong.  *stat(2) calls LSM hook, which may fail and return
arbitrary error.  I wish there were *explicit* rules of LSMs what
restrictions hooks have got.  But I don't know any similar documentation
:(  So, I assume the theoretically worst case - any LSM hook may return
arbitrary error.  Yes, it might break many implicit assumptions, etc.
etc.  But AFAIU, POSIX and other standards define a set of error that
might be used by implementations, leaving implementations free to
define their own errors.  So, strictly speaking, fstat() returning smth
like ENOMEM is not POSIX violation.  IMO LSM policy and error
handling code should have as little interconnection as possible, so the
code should assume almost every syscall can fail for "another reason"
without any thinking like "wtf, how this syscall could fail?!?".

> In that case we
> just need to avoid overwriting it. I'll fix that by separating failure
> into two conditionals.


> > - fcntl(fd, F_SETFD, FD_CLOEXEC) may fail (at least in theory).  If it
> >   fails, the consequences are not expected.
> 
> The only way it can fail is if fd is invalid, which was already tested
> by fstat. If another thread or signal handler closes fd here and
> causes a race condition, behavior is going to be unpredictable
> (undefined) anyway.

The same here, fcntl() has LSM hook.

> > rewinddir(), seekdir():
> > - lseek(fd, 0, SEEK_SET) may fail too, I suppose it can be true for
> >   network file systems or FUSE fses.  Continuing the work with pos=-1 is
> >   somewhat not expected.
> 
> I'm not sure what could be meaningfully done in that case... I would
> actually consider it a broken fs driver if lseek to 0 ever fails on a
> directory but it's worth investigating if it's a real issue.

I don't think you can do anything meaningful here.  I was thinking about
negative ->tell would lead to some buffer overflow.  Looking into the
code I see it is not used internally it musl, so checking ->tell doesn't
help much.  => This part of the patch is indeed meaningless.

> > forkpty():
> > - It should be guaranteed that master fd is closed, tty is setup, slave
> >   fd is dup'ed to 1,2,3.  The latter can be broken by setting small
> >   rlimit.
> 
> The only way it could fail is if one of these fds was not open to
> begin with, which would be pretty unusual.

But possible.  Also POSIX defines EINTR error in dup2(), but you handle
EBUSY only.

> It's unclear to me whether
> it's better to fail early or fail later.. unfortunately there's no
> good way to report failure to the caller after fork succeeds. Perhaps
> I could attempt fcntl F_DUPFD on the slave pty in the parent process
> before forking to reserve fd slots 0-2 if they're not already taken...

This is a good idea.

> >  setsid() is checked for company :)  I think the only way to
> 
> I'm pretty sure setsid cannot fail. There's no justifiable reason for
> failure. POSIX says:
> 
>   [EPERM] The calling process is already a process group leader, or
>   the process group ID of a process other than the calling process
>   matches the process ID of the calling process.

Looks like Linux follows POSIX here.

> >   handle the failure is _exit().  While it may be not the best choise,
> >   however, continuing the work with half dropped privileges is more
> >   dangerous.
> 
> Can you clarify what you mean about "dropped privileges"?

Here privilege dropping was close() with master fd and closing 0,1,2 fds
if they were opened.

> > - I suppose the last calls to tcsetattr() and ioctl() may fail too.
> 
> Indeed, and this is testable/reportable. I suppose it would be best to
> mimic what existing implementations do here.

My 2 cents here - glibc is not the best example to mimic.  It doesn't
check some syscalls that I would check (where it is really simple to
introduce a check).


> > I wonder what is the Right Way to handle close() failures in generic
> > case.
> 
> I think 99% of them are complete non-issues. The whole idea that close
> can fail (for reasons other than EBADF) is a misdesign, but it can
> generally only happen for odd devices or NFS (which is broken in many
> more fundamental ways anyway). It certainly can't fail for terminal
> devices, pipes, sockets, directories, etc. which are the only places
> libc needs it.

Maybe.  I'm worried for fd leaking only, when a process drops some
privileges leaving some fd in half working state opened.  Indeed, it
might be not a musl case.


Thanks,

-- 
Vasiliy

Re: some fixes to musl

[Rich Felker]

On Thu, Jul 21, 2011 at 09:02:55PM +0400, Vasiliy Kulikov wrote:
> Hi,
> 
> This is a patch against v0.7.12.  It is only compile tested.
> 
> fdopendir():
> - POSIX defines EBADF if fd is invalid.  I suppose it is OK
>   to assume fd is invalid if fstat() failed.

Fixed.

> - fcntl(fd, F_SETFD, FD_CLOEXEC) may fail (at least in theory).  If it
>   fails, the consequences are not expected.

Also, per POSIX:

  It is unspecified whether the FD_CLOEXEC flag will be set on the
  file descriptor by a successful call to fdopendir().

http://pubs.opengroup.org/onlinepubs/9699919799/functions/fdopendir.html

> rewinddir(), seekdir():
> - lseek(fd, 0, SEEK_SET) may fail too, I suppose it can be true for
>   network file systems or FUSE fses.  Continuing the work with pos=-1 is
>   somewhat not expected.

I've checked all code that uses the position and it doesn't matter.
The DIR buffer will be empty so the next readdir call will invoke
getdents and get an error again.

> cuserid():
> - POSIX says an argument may be NULL, in this case the buffer should be
>   statically allocated.

This function was removed in SUSv3 and was legacy even in v2. Also
historical versions of the function differed in behavior...

> forkpty():
> - It should be guaranteed that master fd is closed, tty is setup, slave
>   fd is dup'ed to 1,2,3.  The latter can be broken by setting small
>   rlimit.  setsid() is checked for company :)  I think the only way to
>   handle the failure is _exit().  While it may be not the best choise,
>   however, continuing the work with half dropped privileges is more
>   dangerous.
> 
> openpty():
> - close() shouldn't change errno updated by failed ioctl()/open().
> - I suppose the last calls to tcsetattr() and ioctl() may fail too.

Going to try to find a good solution for these...

> _vsyslog():
> - sendto() may fail in case of signal delivery.

I'm opting not to change this one. EINTR can only happen if the
application intentionally installed an interrupting signal handler,
and in this case, the author probably intended to be able to interrupt
any blocking operation. For instance consider the typical use of
alarm(). With your proposed change, syslog would block forever in the
socket buffers were full (syslog daemon hung) and an application
aiming for hardcore robustness/realtime use could not recover.

As-is, the worst that could happen is log messages being lost, but
this will only happen in applications where the programmer has
explicitly asked for interrupting signals.

Also note that the loop-and-retry code was wrong because the socket is
a datagram socket. Retrying mid-buffer would send a new malformed
datagram to the syslog daemon...

> I wonder what is the Right Way to handle close() failures in generic
> case.  If it is fork'ish function, _exit() can be used.  If it is some
> privilege dropping function, failure can be returned in errno, but the
> task state (hanged/unclosed fd) is not expected by the caller.

And thus probably unrecoverable, since the caller does not know the
file descriptor number you failed to close...

> More dangerous case is function that is not designed to return error at
> all.  E.g. close() inside of closelog() may fail, but the caller cannot
> be notified about it by design.  If the caller want to do chroot(),
> which prevents re-opening log fd, the failure would break task's
> expectation of dropped privileges (closed log fd).

My view is that a system aiming at production quality must ensure that
close cannot fail for valid file descriptors. The ability of close to
fail will lead to unrecoverable errors in any program that uses
stdio/fclose, because:

1. The fclose() function shall perform the equivalent of a close() on
the file descriptor that is associated with the stream pointed to by
stream.

2. After the call to fclose(), any use of stream results in undefined
behavior.

If close can fail, then fclose can fail for the same reasons, making
the FILE that refers to the open file unusable, but leaving the file
descriptor in limbo. This is a lot like the issue you mentioned with
closelog, except it affects 99% of applications rather than 1-5% of
applications.

Rich

Re: some fixes to musl

[Rich Felker]

On Thu, Jul 21, 2011 at 11:17:04PM +0400, Vasiliy Kulikov wrote:
> On Thu, Jul 21, 2011 at 14:21 -0400, Rich Felker wrote:
> > > fdopendir():
> > > - POSIX defines EBADF if fd is invalid.  I suppose it is OK
> > >   to assume fd is invalid if fstat() failed.
> > 
> > fstat will already set errno to EBADF if appropriate.
> 
> I think it's wrong.  *stat(2) calls LSM hook, which may fail and return
> arbitrary error.  I wish there were *explicit* rules of LSMs what
> restrictions hooks have got.  But I don't know any similar documentation
> :(  So, I assume the theoretically worst case - any LSM hook may return
> arbitrary error.  Yes, it might break many implicit assumptions, etc.

It should be obvious that a bogus LSM will create gaping security
holes if it's allowed such power.

> etc.  But AFAIU, POSIX and other standards define a set of error that
> might be used by implementations, leaving implementations free to
> define their own errors.  So, strictly speaking, fstat() returning smth
> like ENOMEM is not POSIX violation.  IMO LSM policy and error

But it would be a POSIX violation for fstat to yield EBADF when the
file descriptor is actually valid. POSIX allows implementations
defining new error conditions using errno values not already
documented for the interface, or which are reasonably similar to a
POSIX-documented condition and using the same error code, but it
doesn't allow nonsensical use of them.

> handling code should have as little interconnection as possible, so the
> code should assume almost every syscall can fail for "another reason"
> without any thinking like "wtf, how this syscall could fail?!?".

It's impossible to program this way in full generality. If you're
already in the middle of "backing out" due to one error condition,
most possible errors in the cleanup code will be impossible to handle
because they preclude restoration of state and maintaining any
reasonable invariants. It may not be required by any standard, but
it's impossible to write robust programs on a system where pure
resource-deallocation functions can fail arbitrarily.

> > > - fcntl(fd, F_SETFD, FD_CLOEXEC) may fail (at least in theory).  If it
> > >   fails, the consequences are not expected.
> > 
> > The only way it can fail is if fd is invalid, which was already tested
> > by fstat. If another thread or signal handler closes fd here and
> > causes a race condition, behavior is going to be unpredictable
> > (undefined) anyway.
> 
> The same here, fcntl() has LSM hook.

All a hook on F_SETFD could do is break things. It has no valid usage.
If somebody wants to break their system intentionally, they can do it
in plenty of other ways...

> > > forkpty():
> > > - It should be guaranteed that master fd is closed, tty is setup, slave
> > >   fd is dup'ed to 1,2,3.  The latter can be broken by setting small
> > >   rlimit.
> > 
> > The only way it could fail is if one of these fds was not open to
> > begin with, which would be pretty unusual.
> 
> But possible.  Also POSIX defines EINTR error in dup2(), but you handle
> EBUSY only.

As far as I know there's no way Linux dup2 can sleep/block and thus no
way it can give EINTR... But at least EINTR cannot happen in
applications that don't explicitly intend to use/handle it.

> > It's unclear to me whether
> > it's better to fail early or fail later.. unfortunately there's no
> > good way to report failure to the caller after fork succeeds. Perhaps
> > I could attempt fcntl F_DUPFD on the slave pty in the parent process
> > before forking to reserve fd slots 0-2 if they're not already taken...
> 
> This is a good idea.

Going to try to implement it...

> > Can you clarify what you mean about "dropped privileges"?
> 
> Here privilege dropping was close() with master fd and closing 0,1,2 fds
> if they were opened.

Well dup2 can't fail due to resource limits, only due to the implicit
close on the old fds 0,1,2...

> Maybe.  I'm worried for fd leaking only, when a process drops some
> privileges leaving some fd in half working state opened.  Indeed, it
> might be not a musl case.

I'll certainly aim to fix any of those where (1) a leak is actually
possible, and (2) the interface makes it possible to avoid the leak.
Some, like closelog as you noted, make it impossible...

Rich

Re: some fixes to musl

[Rich Felker]

On Thu, Jul 21, 2011 at 09:57:39PM -0400, Rich Felker wrote:
> > forkpty():
> > - It should be guaranteed that master fd is closed, tty is setup, slave
> >   fd is dup'ed to 1,2,3.  The latter can be broken by setting small
> >   rlimit.  setsid() is checked for company :)  I think the only way to
> >   handle the failure is _exit().  While it may be not the best choise,
> >   however, continuing the work with half dropped privileges is more
> >   dangerous.
> > 
> > openpty():
> > - close() shouldn't change errno updated by failed ioctl()/open().
> > - I suppose the last calls to tcsetattr() and ioctl() may fail too.
> 
> Going to try to find a good solution for these...

I believe I've fixed forkpty's issue with fd exhaustion. Please tell
me if anything seems wrong.

Rich

Re: some fixes to musl

[Vasiliy Kulikov]

On Thu, Jul 21, 2011 at 23:00 +0400, Solar Designer wrote:
> Personally, I've been using different approaches to this in different
> ones of my programs.  For musl, I think the "always check" approach may
> be the better one.  Yes, the code size increase from those error
> handling paths is unfortunate...  Some use of goto can make them smaller
> and keep them out of the same cache lines with actually running code.

The problem here is that there might be no good way to handle errors
of error handling code.  If we allocate resource A, then B, and B
allocation fails, we should release A and return error code.  What to do
if releasing A fails?  Return error code and leave A allocated
(==leaked)?  Try to release it in a cycle (potential infinite loop)?
Terminate the process (not expected by the caller)?

I don't have a good solution...

-- 
Vasiliy

Re: some fixes to musl

[Vasiliy Kulikov]

On Fri, Jul 22, 2011 at 00:30 -0400, Rich Felker wrote:
> On Thu, Jul 21, 2011 at 09:57:39PM -0400, Rich Felker wrote:
> > > forkpty():
> > > - It should be guaranteed that master fd is closed, tty is setup, slave
> > >   fd is dup'ed to 1,2,3.  The latter can be broken by setting small
> > >   rlimit.  setsid() is checked for company :)  I think the only way to
> > >   handle the failure is _exit().  While it may be not the best choise,
> > >   however, continuing the work with half dropped privileges is more
> > >   dangerous.
> > > 
> > > openpty():
> > > - close() shouldn't change errno updated by failed ioctl()/open().
> > > - I suppose the last calls to tcsetattr() and ioctl() may fail too.
> > 
> > Going to try to find a good solution for these...
> 
> I believe I've fixed forkpty's issue with fd exhaustion. Please tell
> me if anything seems wrong.

Looks like it fixes the problem with *expected* dup2() failures.
However, I'm still worried about theoretical (*m) leaking...

-- 
Vasiliy

Re: some fixes to musl

[Rich Felker]

On Fri, Jul 22, 2011 at 12:19:10PM +0400, Vasiliy Kulikov wrote:
> On Thu, Jul 21, 2011 at 23:00 +0400, Solar Designer wrote:
> > Personally, I've been using different approaches to this in different
> > ones of my programs.  For musl, I think the "always check" approach may
> > be the better one.  Yes, the code size increase from those error
> > handling paths is unfortunate...  Some use of goto can make them smaller
> > and keep them out of the same cache lines with actually running code.
> 
> The problem here is that there might be no good way to handle errors
> of error handling code.  If we allocate resource A, then B, and B
> allocation fails, we should release A and return error code.  What to do
> if releasing A fails?  Return error code and leave A allocated
> (==leaked)?  Try to release it in a cycle (potential infinite loop)?
> Terminate the process (not expected by the caller)?
> 
> I don't have a good solution...

The good solution is to demand that kernel developers not create such
bugs and calling them out on it when they do...

Rich

close(2) failure cases (was: some fixes to musl)

[Vasiliy Kulikov]

Rich,

On Thu, Jul 21, 2011 at 14:21 -0400, Rich Felker wrote:
> > I wonder what is the Right Way to handle close() failures in generic
> > case.
> 
> I think 99% of them are complete non-issues. The whole idea that close
> can fail (for reasons other than EBADF) is a misdesign, but it can
> generally only happen for odd devices or NFS (which is broken in many
> more fundamental ways anyway). It certainly can't fail for terminal
> devices, pipes, sockets, directories, etc. which are the only places
> libc needs it.

I've looked at some Linux code related to close().  It may fail iff
file_ops->flush() is not NULL, and close() fails iff ->flush() fails.

So, as the following files don't implement ->flush, they cannot fail on
close():

* tty
* pipe
* sockets (all families)
* some classic devices: full, null, zero, mem, kmem

I cannot say something for sure for directories.  However, NFS'
implementation doesn't implement it => close() cannot fail.  (The
comment in the code says all directories ops are synchronous.)

close() may fail on NFS regural files.  As there is API to work with
/etc/* files, and / can be NFS mount point, it's good to check for
close() (and all other syscalls) failures on such files (whether write()
is actually succeeded).

Even if close() fails, the fd is freed.  So fd leakage is impossible.


As musl is Linux specific libc, you're free to rely on Linux specific
restrictions/features.  However, I don't know whether close()
implementation is guaranteed to be errorless for these types of files.
If no, it may be changed in the future by introducing some new feature
(and it will not break POSIX as POSIX explicitly defines close()
failure possibility).  It's very unlikely though, as breaking tons of
programs relying on implicit Linux behaviour is not encouraged by Linus.


Thanks,

-- 
Vasiliy

Re: some fixes to musl

[Vasiliy Kulikov]

On Thu, Jul 21, 2011 at 22:08 -0400, Rich Felker wrote:
> On Thu, Jul 21, 2011 at 11:17:04PM +0400, Vasiliy Kulikov wrote:
> > On Thu, Jul 21, 2011 at 14:21 -0400, Rich Felker wrote:
> > > > fdopendir():
> > > > - POSIX defines EBADF if fd is invalid.  I suppose it is OK
> > > >   to assume fd is invalid if fstat() failed.
> > > 
> > > fstat will already set errno to EBADF if appropriate.
> > 
> > I think it's wrong.  *stat(2) calls LSM hook, which may fail and return
> > arbitrary error.  I wish there were *explicit* rules of LSMs what
> > restrictions hooks have got.  But I don't know any similar documentation
> > :(  So, I assume the theoretically worst case - any LSM hook may return
> > arbitrary error.  Yes, it might break many implicit assumptions, etc.
> 
> It should be obvious that a bogus LSM will create gaping security
> holes if it's allowed such power.

I don't say such handling is perfect, just want to show it can be
somehow justified:

Most of LSM hooks maintain some security context associated with
file/task/socket/etc.  Some actions may require (re)allocation of this
context.  If the allocation fails (e.g. OOM), it's wrong to allow the
task to continue the work with the object, which has an outdated context
(not updated against some recent activity).  So, unexpected ENOMEM is
returned.

Such allocation may be a result of e.g. asynchronous logging or
statistics update.


I totally agree with you that injecting such faults where a program doesn't
expect it is wrong, but sometimes it can be not obvious for kernel
developers that it breaks something.  Even if they don't want to
introduce such cases, it can be unintendedly introduced by a bug
(e.g. sendmail setuid vulnerability).  Such bugs may be tricky to catch
and may appear in very rare situations.  Trying to catch such cases would
make the program much more robust against kernel bugs/changes (unless the
error handling code complicates the whole logic and/or is not well
tested).

Thanks,

-- 
Vasiliy

Re: close(2) failure cases (was: some fixes to musl)

[Rich Felker]

On Sun, Jul 24, 2011 at 01:19:18PM +0400, Vasiliy Kulikov wrote:
> I've looked at some Linux code related to close().  It may fail iff
> file_ops->flush() is not NULL, and close() fails iff ->flush() fails.

Thank you for making the effort to investigate!

> Even if close() fails, the fd is freed.  So fd leakage is impossible.

Is this true even in the case of EINTR?

> As musl is Linux specific libc, you're free to rely on Linux specific
> restrictions/features.  However, I don't know whether close()
> implementation is guaranteed to be errorless for these types of files.
> If no, it may be changed in the future by introducing some new feature
> (and it will not break POSIX as POSIX explicitly defines close()
> failure possibility).  It's very unlikely though, as breaking tons of
> programs relying on implicit Linux behaviour is not encouraged by Linus.

Not only does it break lots of programs; it also makes fixing those
programs impossible. There is simply no way to recovery from failed
resource deallocation. (Think of C++ and RAII...)

Rich

Re: some fixes to musl

[Rich Felker]

On Sun, Jul 24, 2011 at 01:39:11PM +0400, Vasiliy Kulikov wrote:
> > It should be obvious that a bogus LSM will create gaping security
> > holes if it's allowed such power.
> 
> I don't say such handling is perfect, just want to show it can be
> somehow justified:
> 
> Most of LSM hooks maintain some security context associated with
> file/task/socket/etc.  Some actions may require (re)allocation of this
> context.  If the allocation fails (e.g. OOM), it's wrong to allow the
> task to continue the work with the object, which has an outdated context
> (not updated against some recent activity).  So, unexpected ENOMEM is
> returned.

Are you saying that all operations on the associated object
(file/task/socket/etc.) are automatically first subjected to an LSM
hook? Or just that the LSM module author might set up a hook?

In the latter case, I would say it's a bug and potential gaping
vulnerability for a module to install a hook on any
resource-deallocation function.

If it's the former, I would say it's a bug and gaping security hole in
the whole LSM architecture. Failure to account for dependence of a
critical operation on resource allocation is a classic programming
error. To fix it, I would imagine the allocation either needs to be
moved to the action that caused (re)allocation to be needed, or LSM
hooks need to be skipped for resource deallocation functions, or LSM
hooks need to fallback to success rather than failure when resource
(re)allocation fails during a resource deallocation operation. But
since I'm not familiar with the LSM architecture, this is only
speculation.

In short, I think you've raised really good points, and LSM probably
needs an audit...

> I totally agree with you that injecting such faults where a program doesn't
> expect it is wrong, but sometimes it can be not obvious for kernel
> developers that it breaks something.

Not being aware that you're breaking something isn't a very good
excuse. In any case, following a principle of "deallocation cannot
fail" should not be too hard, and it's required for C++ RAII apps to
work anyway (what can you do if an operation fails during a
destructor?!)

> Even if they don't want to
> introduce such cases, it can be unintendedly introduced by a bug
> (e.g. sendmail setuid vulnerability).  Such bugs may be tricky to catch
> and may appear in very rare situations.  Trying to catch such cases would
> make the program much more robust against kernel bugs/changes (unless the
> error handling code complicates the whole logic and/or is not well
> tested).

I agree that it's better to check for failure and handle it where you
can. What bothers me is the creation of error cases that are usually
impossible to handle.

Rich

Re: close(2) failure cases (was: some fixes to musl)

[Vasiliy Kulikov]

Rich,

On Sun, Jul 24, 2011 at 08:24 -0400, Rich Felker wrote:
> > Even if close() fails, the fd is freed.  So fd leakage is impossible.
> 
> Is this true even in the case of EINTR?

For all types of fd the fd is deleted from fd table, then
FS-specific function is called.  Any error would be returned to the
program, but fd would be already deregistered.


I agree with POSIX in part that close() should somehow signal about
failed IO (e.g. no free disk space) and error return code is good
enough.  However, I feel it was wrong to leave undefined behaviour of fd in
case of error.  If the file is so important that the error must be
handled by the program, it really should do *sync() and react on its
error.  IMO close() should unconditionally close fd.  (The same for
fclose(3), etc.)

Thanks,

-- 
Vasiliy

Re: some fixes to musl

[Vasiliy Kulikov]

Rich,

On Sun, Jul 24, 2011 at 08:56 -0400, Rich Felker wrote:
> On Sun, Jul 24, 2011 at 01:39:11PM +0400, Vasiliy Kulikov wrote:
> > > It should be obvious that a bogus LSM will create gaping security
> > > holes if it's allowed such power.
> > 
> > I don't say such handling is perfect, just want to show it can be
> > somehow justified:
> > 
> > Most of LSM hooks maintain some security context associated with
> > file/task/socket/etc.  Some actions may require (re)allocation of this
> > context.  If the allocation fails (e.g. OOM), it's wrong to allow the
> > task to continue the work with the object, which has an outdated context
> > (not updated against some recent activity).  So, unexpected ENOMEM is
> > returned.
> 
> Are you saying that all operations on the associated object
> (file/task/socket/etc.) are automatically first subjected to an LSM
> hook? Or just that the LSM module author might set up a hook?

The LSM may set up a hook.  The hook may deny any access to an object.
But not all hooks are registered by all LSM implementations.  Some hook
implementations are simply log the event and return success
unconditionally.  IIRC, only SELinux implements all hooks (among all
in-tree LSM implementations).  If a hook is not registered, it is a
noop.


> In the latter case, I would say it's a bug and potential gaping
> vulnerability for a module to install a hook on any
> resource-deallocation function.

It should probably needs better review, but at first sight only
shutdown() among all deallocations may fail because of LSM.  And only
SELinux implements it.  I suppose hooking shutdown() makes sense as a
task may get an fd e.g. for read only access, and shutdown would break
RO restriction.

close(2) is not hooked.


> In short, I think you've raised really good points, and LSM probably
> needs an audit...

Yes, and possible breakage of implicit expectations is not the only
reason for the review :-)

Thanks,

-- 
Vasiliy

Re: close(2) failure cases (was: some fixes to musl)

[Rich Felker]

On Sun, Jul 24, 2011 at 09:49:03PM +0400, Vasiliy Kulikov wrote:
> Rich,
> 
> On Sun, Jul 24, 2011 at 08:24 -0400, Rich Felker wrote:
> > > Even if close() fails, the fd is freed.  So fd leakage is impossible.
> > 
> > Is this true even in the case of EINTR?
> 
> For all types of fd the fd is deleted from fd table, then
> FS-specific function is called.  Any error would be returned to the
> program, but fd would be already deregistered.

Good to know. I may need to investigate and verify that this behaves
as expected in conjunction with thread cancellation while blocked at
close(), and if not, work on a work-around...

Do you know any reliable way to setup the kernel to block/sleep for a
measurable length of time on close() so that I could test this?

> I agree with POSIX in part that close() should somehow signal about
> failed IO (e.g. no free disk space) and error return code is good
> enough.  However, I feel it was wrong to leave undefined behaviour of fd in
> case of error.  If the file is so important that the error must be
> handled by the program, it really should do *sync() and react on its
> error.  IMO close() should unconditionally close fd.  (The same for
> fclose(3), etc.)

Note that the way POSIX leaves the state of the fd indeterminate if
close fails makes it impossible to write robust portable
multi-threaded programs that use files in any non-trivial way. You
can't retry closing a file descriptor you already passed to close,
because it might get assigned to a new file opened in another thread,
in which case you would close the other thread's newly-opened file. I
consider this a major flaw in the standard (one of the many oversights
of not considering the interaction of certain behaviors with threads)
and hope to raise the issue as a defect report and push for the next
version of the standard to define the behavior that the fd always be
freed.

Rich

Re: close(2) failure cases (was: some fixes to musl)

[Vasiliy Kulikov]

Rich,

On Sun, Jul 24, 2011 at 18:29 -0400, Rich Felker wrote:
> Do you know any reliable way to setup the kernel to block/sleep for a
> measurable length of time on close() so that I could test this?

The easiest way is patching sys_close() ;-)

But as you likely need a test reproducable on all kernel versions, you
should use some FS that blocks/loops in either fops->flush() or
fops->release().  The easiest way should be FUSE and a custom FUSE
module where you just call sleep() inside of ->release().


Thanks,

-- 
Vasiliy