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

[Miroslav Lichvar <mlichvar@xxxxxxxxxx>]

On Fri, Nov 20, 2015 at 03:35:24PM +1300, Bryan Christianson wrote:
> Privileged helper that will perform adjtime(), settimeofday(), bind() on
> behalf of chronyd when running as non-root user.
> 
> Changes since version 2
> 1. All PRIVOPS_xxxx conditional compilation defines to be declared in the configure script
> 2. File decriptor transfer is merged with sending/receiving request/response
> 3. Helper functions receive contents of request union directly from helper_main()
> 4. Reorder switch in helper_main()
> 5. Close IPC file descriptor and exit from helper_main (previously helper_main returned to its caller).
> 6. Explicitly include <sys/wait.h> rather than rely on implicit inclusion

Great. I tried the binding operation on Linux and it seems to be
working nicely.

> +/* HELPER - prepare fatal error for daemon */
> +static void
> +res_fatal(PrvResponse *res, const char *fmt, ...)
> +{
> +  va_list ap;
> +  va_start(ap, fmt);
> +
> +  memset(res, 0, sizeof(PrvResponse));

This doesn't need to be here as the caller has already cleared the
memory?

> +static int
> +send_response(int fd, const PrvResponse *res)
> +{
> +  if (write(fd, res, sizeof(PrvResponse)) != sizeof(PrvResponse))
> +    return -1;

Can you please rework the return codes to follow the convention of 1
for success and 0 for fail as in the rest of the chrony code?

> +  if (req->op == op_BINDSOCKET) {
> +    /* extract transferred descriptor */
> +    for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
> +      if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS)
> +        memcpy(&req->u.bind_sock.sock, CMSG_DATA(cmsg), sizeof(int));
> +    }

I think there should be a check if the socket was really set. If the
request didn't have a SCM_RIGHTS control message, bind() would be
called on a invalid descriptor.

> +static void
> +do_adjtime(const ReqAdjustTime *req, PrvResponse *res)
> +{

This function should be probably put in #ifdef as not all systems
have adjtime().

> +  UTI_SockaddrToIPAndPort(sa, &ip, &port);
> +  if (port != 0 && port != CNF_GetNTPPort()) {
> +    res_fatal(res, "PRV_BindSocket, invalid port: %d", port);

The name of the function probably doesn't need to be included in the
messages.

The socket should be closed here as do_bindsocket() won't be called
later.

> +    return;
> +  }

> +static int
> +read_response(int fd, PrvResponse *res)
> +{
> +  if (read(fd, res, sizeof(PrvResponse)) < sizeof(PrvResponse))
> +    LOG_FATAL(LOGF_PrivOps, "Error reading from helper fd : %s", strerror(errno));

Please add a debug message here that a response was received.

> +static int
> +send_to_helper(int fd, PrvRequest *req, PrvResponse *res)
> +{

> +  if (req->op == op_BINDSOCKET) {
> +    /* send file descriptor as a control message */
> +    int *ptr_send_fd;
> +    int cmsglen;

These two variables seem to be set and read only once, can they be
removed?

> +  if (sendmsg(fd, &msg, 0) != sizeof(PrvRequest))
> +      LOG_FATAL(LOGF_PrivOps, "Error writing to helper fd : %s", strerror(errno));

Here could be a debug message that a request was sent.

> +
> +  return read_response(fd, res);
> +}

> +int
> +PRV_AdjustTime(const struct timeval *delta, struct timeval *olddelta)
> +{
> +  PrvRequest req;
> +  PrvResponse res;
> +
> +  if (!have_helper() || delta == NULL)
> +    /* helper is not running or read adjustment call */
> +    return adjtime(delta, olddelta);
> +

Should memset() be here and in the other PRV_* functions to not send
uninitialized data in the request?

> +  req.op = op_ADJTIME;
> +  req.u.adj_tv.tv = *delta;
> +
> +  if (send_to_helper(helper_fd, &req, &res) != 0)
> +    return -1;

-- 
Miroslav Lichvar

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