Re: RFC: [PATCH] SCM_CREDS support

[Samuel Thibault]

Svante Signell, le Thu 24 Oct 2013 13:40:02 +0200, a écrit :
> We are now checking authorization on the receive side.

Could you explain *how* your patch is working?  That is again the piece
of information which is missing in your patch submission.  Us having
to guess from the source code is not the best way to get your patches
accepted.

> Additionally, maybe sending two ports to
> the receiver with the corresponding check on the receiver side is not
> enough security, therefore this RFC. 

Well, the question is quite simple: what happens when the sender
provides faked ports, e.g. pointing to other proc/auth servers?  That's
where having to explain how the patch is working would possibly even
work out the security issues.

> +  void *control = message->msg_control;
> +  socklen_t controllen = message->msg_controllen;

Why storing control and controllen here?  I guess it's a leftover, you
should clean it.

> @@ -106,6 +110,33 @@
> +  /* SCM_CREDS support: Create and send the ancillary data  */
> +  cmsg = CMSG_FIRSTHDR (message);
> +  if (cmsg != NULL && cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == 
> SCM_CREDS)
> +    {
> +      for (; cmsg; cmsg = CMSG_NXTHDR (message, cmsg))
> +     {
> +       ucredp = (struct cmsgcred *) CMSG_DATA(cmsg);
> +       process_t proc = getproc ();
> +       auth_t auth = getauth ();
> +       nports = 2;
> +       ports = __alloca (nports * sizeof (mach_port_t));
> +       ports[0] = proc;
> +       ports[1] = auth;

So what if here we providing the ports to other proc/auth servers?

> +      goto label;

Why skipping SCM_RIGHTS support?  The message may contain *both*
SCM_RIGHT and SCM_CREDS, we have to support that.  Likewise on the
receiver side.

> @@ -155,6 +157,104 @@
>      message->msg_controllen = clen;
>    memcpy (message->msg_control, cdata, message->msg_controllen);
>  
> +  error_t check_auth (process_t proc, auth_t auth, __pid_t pid,
> +                   __uid_t euid, __uid_t auid,
> +                   __gid_t egid, __gid_t agid,
> +                   int ngroups, __gid_t *groups)
> +  {
> +    error_t err;
> +    pid_t rpid, rppid;
> +    int orphaned;
> +    uid_t euids_buf[CMGROUP_MAX], auids_buf[CMGROUP_MAX];
> +    uid_t *euids = euids_buf, *auids = auids_buf;
> +    gid_t egids_buf[CMGROUP_MAX], agids_buf[CMGROUP_MAX];
> +    gid_t *egids = egids_buf, *agids = agids_buf;
> +    size_t neuids = CMGROUP_MAX, nauids = CMGROUP_MAX;
> +    size_t negids = CMGROUP_MAX, nagids = CMGROUP_MAX;
> +
> +    /* FIXME: In this the right lock? */
> +    /* FIXME: EPERM and/or EACCESS? */
> +    HURD_CRITICAL_BEGIN;
> +    __mutex_lock (&_hurd_id.lock);
> +    err = _hurd_check_ids();
> +    if (err)
> +      goto finish;
> +
> +    err = __USEPORT (PROC, __proc_getpids (proc, &rpid, &rppid, &orphaned));
> +    if (err)
> +      goto finish;
> +    if (rpid != pid )
> +      {

So here is the security concern: this is asking the proc port of the
sender what is the pid of sender. But what if the proc server of the
sender is a fake and tells us lies?  You'd need to check that the
process behind `proc` is the same as the process behind the receiver's
proc server.

Yes your code is asking the proc server of the sender, not the proc
server of the receiver: see the comment above HURD_PORT_USE: if you
want to use the proc server of the receiver, you need to use `port`
(defined by the HURD_PORT_USE macro). But then you'll get the pid of the
receiver, not of the sender.

Now, I wonder the implications of the sender giving its proc port to the
receiver.  Doesn't that allow the receiver to do nasty things with it?
such as calling proc_getprivports() and get access to the host_priv and
device_master ports when the sender is root?

So I'd say a completely different way is needed to check the pid of the
sender. The matter here is that only pflocal has a port to the sender,
the receiver doesn't have one. Another noticeable thing is that the
receiver trusts pflocal, so if pflocal provides information about the
sender (such as a task port of the sender), the receiver can trust it,
and safely use proc_task2pid etc. to get information about it from its
own proc and auth servers.  So probably adding something to pflocal can
provide a solution.

> +     err = EPERM;
> +     goto finish;
> +      }
> +
> +    err = __USEPORT (AUTH, __auth_getids
> +                  (auth,
> +                   &euids, &neuids, &auids, &nauids,
> +                   &egids, &negids, &agids, &nagids));

Same here, you are asking the sender's auth server about what the sender
is.  What if the sender provided a fake auth server port?

Samuel