socketpair(2) and getpeereid(3)

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

socketpair(2) and getpeereid(3)

Sebastien Marie-3
Hi,

While running testsuite on third-party program, I found some weird
behaviour on us side regarding socketpair(2) and getpeereid(3).

I ported the unit test to C (it was Rust) to check more easily.

It just creates an UNIX domain socket using socketpair(2), and next
check the euid/egid of the peer connected socket. The expected result is
that euid of the peer is the euid of the process.

$ cat test.c
#include <sys/types.h>
#include <sys/socket.h>

#include <err.h>
#include <stdlib.h>
#include <stdio.h>

int
main(int argc, char *argv[])
{
        int sv[2];
        uid_t euid;
        gid_t egid;

        if (socketpair(AF_UNIX, SOCK_STREAM, 0, sv) == -1)
                err(EXIT_FAILURE, "socketpair");

        if (getpeereid(sv[0], &euid, &egid) == -1)
                err(EXIT_FAILURE, "getpeereid");

        printf("euid = %d\negid = %d\n", euid, egid);

        return EXIT_SUCCESS;
}

But getpeereuid(3) returns ENOTCONN instead of printing peer information.

$ cc test.c && ./a.out
a.out: getpeereid: Socket is not connected


I dig a bit inside kernel.

getpeereid(3) is implemented using getsockopt(SO_PEERCRED) inside libc.
getsockopt(2) code returns the peer information from `unp->unp_connid'
if unp_flag UNP_FEIDS is set.

kern/uipc_socket.c
  1870                  case SO_PEERCRED:
  1871                          if (so->so_proto->pr_protocol == AF_UNIX) {
  1872                                  struct unpcb *unp = sotounpcb(so);
  1873
  1874                                  if (unp->unp_flags & UNP_FEIDS) {
  1875                                          m->m_len = sizeof(unp->unp_connid);
  1876                                          memcpy(mtod(m, caddr_t),
  1877                                              &(unp->unp_connid), m->m_len);
  1878                                          break;
  1879                                  }
  1880                                  return (ENOTCONN);
  1881                          }
  1882                          return (EOPNOTSUPP);


The peer information is setted inside `unp_connect()' function.

kern/uipc_usrreq.c
   517                  if (unp2->unp_addr)
   518                          unp3->unp_addr =
   519                              m_copym(unp2->unp_addr, 0, M_COPYALL, M_NOWAIT);
   520                  unp3->unp_connid.uid = p->p_ucred->cr_uid;
   521                  unp3->unp_connid.gid = p->p_ucred->cr_gid;
   522                  unp3->unp_connid.pid = p->p_p->ps_pid;
   523                  unp3->unp_flags |= UNP_FEIDS;
   524                  so2 = so3;
   525                  if (unp2->unp_flags & UNP_FEIDSBIND) {
   526                          unp->unp_connid = unp2->unp_connid;
   527                          unp->unp_flags |= UNP_FEIDS;
   528                  }
   529          }
   530          error = unp_connect2(so, so2);

When using socketpair(2), it is `soconnect2()' which is called, so the
unp_connid struct is never setted.

kern/uipc_syscalls.c
   458          error = socreate(SCARG(uap, domain), &so1, type, SCARG(uap, protocol));
   459          if (error)
   460                  return (error);
   461          error = socreate(SCARG(uap, domain), &so2, type, SCARG(uap, protocol));
   462          if (error)
   463                  goto free1;
   464
   465          error = soconnect2(so1, so2);
   466          if (error != 0)
   467                  goto free2;


Could I have confirmation if it is a bug or not ? I am unsure if
socketpair(2) should set the peer information or not. But at least,
ENOTCONN is wrong: socketpair(2) returns connected sockets.

Thanks.
--
Sebastien Marie

Reply | Threaded
Open this post in threaded view
|

Re: socketpair(2) and getpeereid(3)

Philip Guenther
On Sat, 7 Apr 2018, Sebastien Marie wrote:
> While running testsuite on third-party program, I found some weird
> behaviour on us side regarding socketpair(2) and getpeereid(3).
>
> I ported the unit test to C (it was Rust) to check more easily.
>
> It just creates an UNIX domain socket using socketpair(2), and next
> check the euid/egid of the peer connected socket. The expected result is
> that euid of the peer is the euid of the process.
...
> Could I have confirmation if it is a bug or not ? I am unsure if
> socketpair(2) should set the peer information or not. But at least,
> ENOTCONN is wrong: socketpair(2) returns connected sockets.

That's a bug, IMO: socketpair(2) should support use of getpeereid(3).


Philip Guenther

Reply | Threaded
Open this post in threaded view
|

Re: socketpair(2) and getpeereid(3)

Theo de Raadt-2
Philip Guenther <[hidden email]> wrote:

> On Sat, 7 Apr 2018, Sebastien Marie wrote:
> > While running testsuite on third-party program, I found some weird
> > behaviour on us side regarding socketpair(2) and getpeereid(3).
> >
> > I ported the unit test to C (it was Rust) to check more easily.
> >
> > It just creates an UNIX domain socket using socketpair(2), and next
> > check the euid/egid of the peer connected socket. The expected result is
> > that euid of the peer is the euid of the process.
> ...
> > Could I have confirmation if it is a bug or not ? I am unsure if
> > socketpair(2) should set the peer information or not. But at least,
> > ENOTCONN is wrong: socketpair(2) returns connected sockets.
>
> That's a bug, IMO: socketpair(2) should support use of getpeereid(3).

Yes, support should be added.

Reply | Threaded
Open this post in threaded view
|

Re: socketpair(2) and getpeereid(3)

Sebastien Marie-3
On Sat, Apr 07, 2018 at 09:29:30PM -0600, Theo de Raadt wrote:

> Philip Guenther <[hidden email]> wrote:
>
> > On Sat, 7 Apr 2018, Sebastien Marie wrote:
> > > While running testsuite on third-party program, I found some weird
> > > behaviour on us side regarding socketpair(2) and getpeereid(3).
> > >
> > > I ported the unit test to C (it was Rust) to check more easily.
> > >
> > > It just creates an UNIX domain socket using socketpair(2), and next
> > > check the euid/egid of the peer connected socket. The expected result is
> > > that euid of the peer is the euid of the process.
> > ...
> > > Could I have confirmation if it is a bug or not ? I am unsure if
> > > socketpair(2) should set the peer information or not. But at least,
> > > ENOTCONN is wrong: socketpair(2) returns connected sockets.
> >
> > That's a bug, IMO: socketpair(2) should support use of getpeereid(3).
>
> Yes, support should be added.

Here a diff to add getpeerid(3) support inside socketpair(2).

It fills unp_connid on the two sockets (there are connected each other).

I think it is the more simple way to achieve it. Moving the related code
from unp_connect() to unp_connect2() should be possible (only few direct
callers of {so,unp_}connect2() ), but unp_connid will not be copied on
the two sockets.

--
Sebastien Marie


Index: kern/uipc_syscalls.c
===================================================================
RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
retrieving revision 1.168
diff -u -p -r1.168 uipc_syscalls.c
--- kern/uipc_syscalls.c 28 Mar 2018 09:54:00 -0000 1.168
+++ kern/uipc_syscalls.c 8 Apr 2018 04:57:14 -0000
@@ -448,6 +448,7 @@ sys_socketpair(struct proc *p, void *v,
  struct filedesc *fdp = p->p_fd;
  struct file *fp1, *fp2;
  struct socket *so1, *so2;
+ struct unpcb *unp1, *unp2;
  int type, cloexec, nonblock, fflag, error, sv[2];
 
  type  = SCARG(uap, type) & ~(SOCK_CLOEXEC | SOCK_NONBLOCK);
@@ -461,6 +462,14 @@ sys_socketpair(struct proc *p, void *v,
  error = socreate(SCARG(uap, domain), &so2, type, SCARG(uap, protocol));
  if (error)
  goto free1;
+
+ unp1 = sotounpcb(so1);
+ unp2 = sotounpcb(so2);
+ unp1->unp_connid.uid = unp2->unp_connid.uid = p->p_ucred->cr_uid;
+ unp1->unp_connid.gid = unp2->unp_connid.gid = p->p_ucred->cr_gid;
+ unp1->unp_connid.pid = unp2->unp_connid.pid = p->p_p->ps_pid;
+ unp1->unp_flags |= UNP_FEIDS;
+ unp2->unp_flags |= UNP_FEIDS;
 
  error = soconnect2(so1, so2);
  if (error != 0)

Reply | Threaded
Open this post in threaded view
|

Re: socketpair(2) and getpeereid(3)

Alexander Bluhm
On Sun, Apr 08, 2018 at 07:10:27AM +0200, Sebastien Marie wrote:
> I think it is the more simple way to achieve it. Moving the related code
> from unp_connect() to unp_connect2() should be possible (only few direct
> callers of {so,unp_}connect2() ), but unp_connid will not be copied on
> the two sockets.

This is layer violation.  The socket layer does not know that the
domain is AF_UNIX.  You must check that before calling sotounpcb().

I think it would be best to implement this in unp_connect2().
Although sogetopt(SO_PEERCRED) also does a layer violation, but at
least it checks for AF_UNIX.

bluhm