[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: RFC: [PATCH] SCM_CREDS support



On Thu, 2013-10-24 at 14:34 +0200, Samuel Thibault wrote:
> 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.

Well, you give the explanation below. I'll add a proper description when
all is cleared out.

> > 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.

How could it point to other proc/auth servers? The receiver is using the
ports of the same proc server. Are you considering more than one
instance running? This is communication on a local socket, and the
socket read/write mode is controlling the access to it. In the
implementation only the same user and root could send. for other users
the socket permission has to be changed from srw-r--r-- to srw-r--rw-
Tested by sending as another user.

> > +  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.

This is related to the second patch I mentioned, sending with -nz
option. If that case this option is not going to be implemented, of
course there is no reason to do this. I tried to indicate that.

> > @@ -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?

If so they have to guess exactly which ports are used by the running
instance of proc and auth, or??
 
> > +      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.

I have never seen any application using that. If so, I have to check if
it works on kFreeBSD first.

> > +    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.

How?

What about the _hurd_check_ids() call?
/* Check that _hurd_id.{gen,aux} are valid and update them if not.
   Expects _hurd_id.lock to be held and does not release it.  */

> 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.

Yes I know, I've tried that.

> 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?

Might be so, you are the expert, I'm not. Of course this has to be
avoided by all means.

> 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.

Can you elaborate? I've tried with proc_reauthenticate() and
auth_user_authenticate() but they did not trigger any problems, so I
removed that code. I should have tried harder to test faked servers. How
to modify proc and auth servers (or write simple code) to fake stuff?

Maybe making an alternate S_io_reauthenticate not trashing the IDs is a
way forward?

> > +	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?

See above.



Reply to: