Re: socketpair(2) and getpeereid(3)
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
Re: socketpair(2) and getpeereid(3)
On Sat, Apr 07, 2018 at 09:29:30PM -0600, Theo de Raadt wrote: > Philip Guenther <pguent...@proofpoint.com> 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.c28 Mar 2018 09:54:00 - 1.168 +++ kern/uipc_syscalls.c8 Apr 2018 04:57:14 - @@ -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), , 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)
Re: socketpair(2) and getpeereid(3)
Philip Guenther <pguent...@proofpoint.com> 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.
Re: socketpair(2) and getpeereid(3)
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
socketpair(2) and getpeereid(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 #include #include #include #include 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], , ) == -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), , type, SCARG(uap, protocol)); 459 if (error) 460 return (error); 461 error = socreate(SCARG(uap, domain), , 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