Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20110722020820.GD132@brightrain.aerifal.cx>
Date: Thu, 21 Jul 2011 22:08:20 -0400
From: Rich Felker <dalias@...ifal.cx>
To: musl@...ts.openwall.com
Subject: Re: some fixes to musl

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

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.