Re: [PATCH 2/2] net: Implement SO_PASSCGROUP to enable passing cgroup path

[Simo Sorce <ssorce@redhat.com>]

On Thu, 2014-04-17 at 11:04 -0700, Andy Lutomirski wrote:
> On Thu, Apr 17, 2014 at 10:52 AM, Simo Sorce <ssorce@redhat.com> wrote:
> > On Thu, 2014-04-17 at 10:26 -0700, Andy Lutomirski wrote:
> >> On Thu, Apr 17, 2014 at 10:12 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > On Thu, Apr 17, 2014 at 09:55:08AM -0700, Andy Lutomirski wrote:
> >> >> On Thu, Apr 17, 2014 at 9:48 AM, Simo Sorce <ssorce@redhat.com> wrote:
> >> >> > On Thu, 2014-04-17 at 09:37 -0700, Andy Lutomirski wrote:
> >> >> >> On Thu, Apr 17, 2014 at 9:24 AM, Simo Sorce <ssorce@redhat.com> wrote:
> >> >> >> > On Thu, 2014-04-17 at 09:11 -0700, Andy Lutomirski wrote:
> >> >> >> >>
> >> >> >> >> No.  The logging daemon thinks it wants to know who the writer is, but
> >> >> >> >> the logging daemon is wrong.  It actually wants to know who composed a
> >> >> >> >> log message destined to it.  The caller of write(2) may or may not be
> >> >> >> >> the same entity.
> >> >> >> >
> >> >> >> > This works both ways, and doesn't really matter, you are *no* better off
> >> >> >> > w/o this interface.
> >> >> >> >
> >> >> >> >> If this form of SO_PASSCGROUP somehow makes it into a pull request for
> >> >> >> >> Linus, I will ask him not to pull it and/or to revert it.  I think
> >> >> >> >> he'll agree that write(2) MUST NOT care who called it.
> >> >> >> >
> >> >> >> > And write() does not, there is no access control check being performed
> >> >> >> > here. This call is the same as getting the pid of the process and
> >> >> >> > crawling /proc with that information, just more efficient and race-free.
> >> >> >>
> >> >> >> Doing it using the pid of writer is wrong.  So is doing it with the
> >> >> >> cgroup of the writer.  The fact that it's even possible to use the pid
> >> >> >> of the caller of write(2) is a mistake, but that particular mistake
> >> >> >> is, unfortunately, well-enshrined in history.
> >> >> >>
> >> >> >> >
> >> >> >> > I repeat, it is *not* access control.
> >> >> >> >
> >> >> >>
> >> >> >> Sure it is.
> >> >> >>
> >> >> >> Either correct attribution of logs doesn't matter, in which case it
> >> >> >> makes little difference how you do it, or it does matter, in which
> >> >> >> case it should be done right.
> >> >> >
> >> >> > Well journald can *also* get  SO_PEERCGROUP and log anomalies if the 2
> >> >> > differ. That is if the log happens on a connected socket.
> >> >> >
> >> >> > If the log happens on a unix datagram* then SO_PEERCGROUP is not
> >> >> > available because there is no connect(), however write() cannot be used
> >> >> > either, only sendmsg() AFAIK, so the "setuid" binary attack does not
> >> >> > apply.
> >> >> >
> >> >>
> >> >> Or you could only send SCM_CGROUP when the writer asks sendmsg to send
> >> >> it, in which case this whole problem goes away.
> >> >
> >> > Sending SCM_CGROUP explicitly is also sending cgroup info at write(2) time
> >> > and if receiver uses that info for access control, it can be problematic.
> >> >
> >>
> >> Not really.  write(2) can't send SCM_CGROUP.  Callers of sendmsg(2)
> >> who supply SCM_CGROUP are explicitly indicating that they want their
> >> cgroup associated with that message.  Callers of write(2) and send(2)
> >> are simply indicating that they have some bytes that they want to
> >> shove into whatever's at the other end of the fd.
> >
> > So you are telling me that you want to change all code that writes to
> > stderr to be changed to use sendmsg() instead of write() in order to get
> > that information ?
> 
> No.  I'm telling you that I want whoever writes the logging code to
> change *the logging code* to use sendmsg.
> 

> > If you are using datagram sockets then the sender explicitly has to use
> > sendmsg() already and if a setuid binary can be convinced to send
> > arbitrary data to an arbitrary datagram sokcet you have bigger problems
> > in that binary, and said binary will send you whatever cgroup it is in.
> 
> Really?

I want to retract my comment in light of the fact you can use connect()
on a datagram socket and then kernel will "helpfully" then allow you to
use write() on it, I forgot about this detail.

For the logging q. above I wouldn't see any issue unless the journald
people were planning on using connect() themselves to pie things like
stderr over a datagram socket.
If that were the case, well then ...

OTOH, I still need to understand how you attack a setuid binary to fake
a cgroup, it's not like uig/gid information that is changed by the
simple fact of invoking the setuid binary, and this is a brand new
contract, so it is not unreasonable to put in the security
considerations that a setuid binary should take extracare to where they
emit stdout/stderr message should they decide to change their cgroup ...

Simo.