Re: [chrony-dev] [PATCH] Privilege Separation - Version 2 - Add helper process

[Bryan Christianson <bryan@xxxxxxxxxxxxx>]

> On 19/11/2015, at 5:47 AM, Miroslav Lichvar <mlichvar@xxxxxxxxxx> wrote:
> 
> On Mon, Nov 16, 2015 at 01:42:27PM +1300, Bryan Christianson wrote:
>> I removed all the stuff to actually compile privops, figuring it would show up when the priv helper is used with a driver.
>> 
>> I have a couple of queries about the next step
>> 
>> 1. PRIVOPS_ALL is true when any of the function defines is added to config, I think PRV_Initialise() should be called
>> from SYS_DropRoot() (guarded by PRIVOPS_ALL) - or would you prefer a call from each driver SYS_xxxxx_DropRoot() ?
> 
> If it was used only for root privileges, SYS_DropRoot() would make
> sense. If we wanted to use the helper also for getaddrinfo() on Linux
> with seccomp filter, it would be running without root privileges
> though. I'd leave that up to the system drivers, at least for now.
> 
> Instead of defining PRIVOPS_ALL in the header, it could be defined in
> the configure script together with the addition of privops.o to
> EXTRA_OBJECTS.
> 
> Could it be named PRIVOPS_HELPER or something like that?

No problem - minor change

> 
>> 2. Not sure where (or even if) PRV_Finalise() should be called.
> 
>> From the system drivers.
> 
> Couple more comments on the patch are below.
> 
>>> Changes since version 1
>>> 1. Fix variable/struct/function names as suggested
>>> 2. Avoid calling LOG_FATAL from the helper
>>> 3. Added #define PRIVOPS_xxxxx for each privileged function
>>> 4. Transfer fd request and data in single I/O
>>> 5. helper_main() restructured as suggested.
>>> 6. remove privops.o from Makefile.in
> 
> Nice.
> 
>>> +#include "sysincl.h"
>>> +#include "conf.h"
>>> +#include "logging.h"
>>> +#include "privops.h"
>>> +#include "util.h"
>>> +
>>> +#if PRIVOPS_ANY
> 
> If the file will be compiled only when the helper is needed, this #if
> doesn't have to be here.
> 
>>> +typedef struct {
>>> +  int sock;
> 
> This field probably shouldn't be here as it's sent in the control message.
> 
>>> +  socklen_t sa_len;
>>> +  union sockaddr_in46 sa;
>>> +} ReqBindSocket;
> 
>>> +static int
>>> +send_with_fd(int fd, int fd_to_send, const void *data, int data_len)
> 
> Instead of hardcoding the case with sending socket over SCM_RIGHTS
> control message, could this function take a pointer to cmsghdr?
> Functions that prepare requests, but don't need a control message, would
> pass NULL.

OK - will change

> 
>>> +  cmsg->cmsg_level = SOL_SOCKET;
>>> +  cmsg->cmsg_type = SCM_RIGHTS;
>>> +  cmsg->cmsg_len = CMSG_LEN(sizeof(int));
>>> +
>>> +  ptr_send_fd = CMSG_DATA(cmsg);
>>> +  memcpy(ptr_send_fd, &fd_to_send, sizeof(int));
> 
> Can this be a direct assignment?

I was concerned about alignment - not sure if its valid to assume that CMSG_DATA() will be aligned on an integer boundary. Looking at the macro on MacOS it is correctly aligned, so will do direct assignment.

> 
>>> +static int
>>> +receive_with_fd(int fd, void *data, int data_size, int *fd_recvd)
> 
> Here it would be a pointer to a cmsg buffer.
> 
>>> +  /* extract transferred descriptor (if any) */
>>> +  for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
>>> +    if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS)
>>> +      memcpy(fd_recvd, CMSG_DATA(cmsg), sizeof(int));
>>> +  }
> 
> This would be in do_bindsocket().

OK

> 
>>> +/* HELPER - send response to the fd */
>>> +
>>> +static inline int
> 
> Why inline? I'd leave that up to the compiler.

OK

> 
>>> +static void
>>> +do_adjtime(const PrvRequest *req, PrvResponse *res)
> 
> Wouldn't the code be a bit more readable if req was the operation
> specific request type? Too bad the response has to be the generic type
> to allow returning the fatal error.

OK

> 
>>> +{
>>> +  const struct timeval *delta = &req->u.adj_tv.tv;
>>> +  struct timeval olddelta;
>>> +
>>> +  memset(res, 0, sizeof(PrvResponse));
> 
> memset() calls can be moved to the main function.

OK

> 
>>> +  while (1) {
>>> +    if (receive_with_fd(fd, &req, sizeof(PrvRequest), &recvd_fd) < sizeof(PrvRequest))
>>> +      /* read error or closed input - we cannot recover - give up */
>>> +      return;
>>> +
>>> +    switch (req.op) {
>>> +    default:
>>> +      res_fatal(&res, "Unexpected operator : %d", req.op);
>>> +      break;
> 
> Can you please move the default case to the end of the switch
> statement? I didn't even know this syntax was valid :).

OK :)

> 
>>> +    case op_BINDSOCKET:
>>> +      req.u.bind_sock.sock = recvd_fd; /* use transferred descriptor */
>>> +      do_bindsocket(&req, &res);
> 
> There would be a pointer to the cmsg buffer as an extra argument.
> 
>>> +PRV_BindSocket(int sock, struct sockaddr *address, socklen_t address_len)
>>> +{
>>> +  PrvRequest req;
>>> +  PrvResponse res;
>>> +  IPAddr ip;
>>> +  unsigned short port;
>>> +
>>> +  if (address->sa_len > address_len)
> 
> On some systems there is no sa_len in struct sockaddr. Does it have to
> be checked?

Easy to drop the check

> 
>>> +void
>>> +PRV_Initialise(void)
>>> +{
>>> +  pid_t pid;
>>> +  int sock_pair[2];
>>> +
>>> +  if (have_helper())
>>> +    LOG_FATAL(LOGF_PrivOps, "PRV_start, helper is already running");
>>> +
>>> +  if (socketpair(AF_UNIX, SOCK_STREAM, 0, sock_pair) != 0)
> 
> Shouldn't this actually be SOCK_DGRAM to preserve message boundaries?
> Or is that not a problem with such small messages? I suspect it
> would be more difficult to detect that the other end of the socket was
> closed. SOCK_SEQPACKET doesn't seem portable.

OK - will test with SOCK_DGRAM  I think SOCK_STREAM is OK here because messages less than 512 bytes will be forwarded immediately (POSIX) and maybe longer messages OS dependent.

> 
>>> +    /* close other descriptors inherited from the parent process */
>>> +    for (fd = 0; fd < 1024; fd++) {
>>> +      if (fd != sock_pair[1])
>>> +        close(fd);
>>> +    }
>>> +
>>> +    helper_main(sock_pair[1]);
>>> +
>>> +    close(sock_pair[1]);
>>> +    exit(0);
> 
> The two lines could be moved to helper_main().

OK

> 
>>> +PRV_Finalise(void)
>>> +{
>>> +  int status;
>>> +  close(helper_fd);
>>> +  helper_fd = -1;
>>> +  wait(&status);
> 
> wait() seems to require <sys/wait.h>.

Hmmm - ok - It compiles OK on MacOS - that header must be implicitly included by one of the other headers in sysincl.h. I'll update sysincl.h to explicitly include.


B
--
To unsubscribe email chrony-dev-request@xxxxxxxxxxxxxxxxxxxx with "unsubscribe" in the subject.
For help email chrony-dev-request@xxxxxxxxxxxxxxxxxxxx with "help" in the subject.
Trouble?  Email listmaster@xxxxxxxxxxxxxxxxxxxx.