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

[Miroslav Lichvar <mlichvar@xxxxxxxxxx>]

On Sat, Nov 07, 2015 at 07:33:00AM +1300, Bryan Christianson wrote:
> Privileged helper that will perform adjtime(), settimeofday(), bind() on
> behalf of chronyd when running as non-root user.

Sounds good.

> -OBJS = array.o cmdparse.o conf.o local.o logging.o main.o memory.o \
> +OBJS = array.o cmdparse.o conf.o local.o logging.o main.o memory.o privops.o \

Maybe instead of always compiling the file, add it to EXTRA_OBJECTS in
the configure script when the feature is enabled?

>    LOGF_Nameserv,
> +  LOG_PrivOps,

That should be LOGF_PrivOps. I see I wrote LOG_PrivOps in my other
mail, sorry for the confusion.

> +typedef struct {
> +  struct timeval tv;
> +} req_adjust;

Please use CamelCase for typedefs, e.g. ReqAdjustTime.

> +typedef struct {
> +  socklen_t sa_len;
> +  union sockaddr_in46 sa;
> +} req_sockaddr;

ReqBindSocket?

> +static int fd_unix = -1;

helper_fd?

> +
> +static inline int
> +havehelper(void)

have_helper() ? Please use underscores in names of private functions.

> +static int
> +send_fd(int fd, int fdtowrite)
> +{

> +  ssize_t bytesSent;

> +  bytesSent = sendmsg(fd, &msg, 0);
> +  if (bytesSent == sizeof(kDummyData)) {

No need to keep a variable for this. The result can be compared
directly to the expected value if it's not needed elsewhere.

> +    nbytes = read(fd, &ack, sizeof(ack));
> +    if (nbytes != sizeof(ack)) {

For this too.

> +  struct {
> +    struct cmsghdr  hdr;
> +    int fd;
> +  } control;

This could be replaced with an array of chars, like in ntp_io.c, and
then use the CMSG_* macros to work on that.

> +static int
> +readrequest(int fd, priv_request_t *req)

read_request :)

> +{
> +  int nbytes;
> +
> +  nbytes = read(fd, req, sizeof(priv_request_t));
> +  if (nbytes < sizeof(priv_request_t)) {
> +    return -1;
> +  }

No need to wrap in curly braces if it's not expected more statements
will be added later and the commit would update the if line. We are
loosely following the Linux kernel coding style.

> +static int
> +do_adjtime(int fd, const struct timeval *delta)

The file descriptor shouldn't be passed here. I think the main
function should be sending the reponse and the parameters here should
be just the request and the reponse data. Similarly for the other
functions.

> +{
> +  int rc;

> +  rc = adjtime(delta, &olddelta);
> +  res.rc = rc;

No need to use the extra variable for this?

> +static void
> +helpermain(int fd)

> +    switch (req.op) {
> +    default:
> +      LOG_FATAL(LOG_PrivOps, "Unexpected operator : %d", req.op);

Hm, will the message get to syslogd when we now close all descriptors?
I guess it should be reopen when the helper is starting. Or maybe
don't make any log messages in the helper. I'm not sure.

> +    case op_BINDSOCKET:
> +      {
> +        priv_response_t res;
> +        int sock;
> +
> +        struct sockaddr *sa = &req.u.sa.sa.u;
> +        socklen_t sa_len = req.u.sa.sa_len;
> +
> +        // wakeup the daemon then wait for a socket descriptor

Please use C-style comments.

> +        memset(&res, 0, sizeof(res));
> +        sendresponse(fd, &res);
> +
> +        sock = receive_fd(fd);
> +
> +        do_bindsocket(fd, sock, sa, sa_len);

I don't like this very much. Could be the functions that send and
receive the helper requests extended to allow a control message and
send both sockaddr and the descriptor in the same message?

I'm still wondering if this operation is worth the trouble,
considering it will be needed only when the server port is open later
with a chronyc command.

> +static int
> +getportfromaddress(const struct sockaddr *address, socklen_t address_len, int *ip_port)

There is a similar function for that, UTI_SockaddrToIPAndPort.

>  if (port != 0 && port != CNF_GetNTPPort()) {
>    return 0;
>  }

This should be checked in the helper. The main process could be
compromised.

> +int
> +PRV_BindSocket(int sock, const struct sockaddr *address, socklen_t address_len)
> +{
> +  priv_request_t req;
> +  priv_response_t res;
> +  int port;
> +
> +  if (!getportfromaddress(address, address_len, &port)) {
> +    LOG_FATAL(LOG_PrivOps, "PRV_BindSocket, invalid socket address");
> +  }
> +
> +  if (!havehelper() || port == 0 || port >= 1024) {
> +    /* dont use the helper if not required */

Is that 1024 a safe assumption? Considering it's not a time critical
code, maybe always call the helper if it's running and later modify
ntp_io to not call PRV_BindSocket for client-only sockets?

> +#ifndef _PRIVOPS_H_
> +#define _PRIVOPS_H_

GOT_PRIVOPS_H to follow the other headers.

> +#if defined(FEAT_PRIVHELPER)
> +/* use privileged function wrappers */
> +int PRV_AdjustTime(const struct timeval *delta, struct timeval *olddelta);
> +int PRV_BindSocket(int sock, const struct sockaddr *address, socklen_t address_len);
> +int PRV_SetTime(const struct timeval *tp, const struct timezone *tzp);

We might need to be able to enable/disable each function separately,
so I'd suggest to use macros like PRIVOPS_ADJUSTTIME and enable them
in the configure script as needed for each system.

> +#endif /* defined(FEAT_PRIVHELPER) */
> +
> +#endif
> \ No newline at end of file

Please add a newline :).

Thanks,

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