Re: [chrony-dev] [PATCH] MacOSX - Drop root privileges

[Miroslav Lichvar <mlichvar@xxxxxxxxxx>]

On Sun, Nov 01, 2015 at 06:29:06PM +1300, Bryan Christianson wrote:
> Before dropping root privileges, chronyd forks and runs a helper
> process that will remain privileged and perform bind(), adjtime() and
> settimeofday() operations on behalf of the soon to be non-privileged
> chronyd.  Use an AF_UNIX socket pair as a full duplex channel between
> the privileged helper and chronyd.

I like the idea of privilege separation. This will be a very nice
feature for systems where it's not possible to drop root privileges
completely. I think it could be even useful on systems where it is
possible. For instance on Linux when using the seccomp filtering,
getaddrinfo() (which is difficult to contain) could be called in the
less restricted process. I'm not sure how threads would fit into this,
but we can figure that out later.

>  12 files changed, 783 insertions(+), 62 deletions(-)

This is a quite a lot of new code. I'd like to see some changes in the
design. I'm not completely sure how the result should look like, so it
might take a couple of iterations before we get it right, please be
patient.

I think it would be good if the new privileged code was clearly
separated from the rest of the code. In the existing code it should be
just a replacement of the adjtime(), settimeofday(), bind() calls with
functions like PRV_AdjustTime(), PRV_SetTime(), PRV_BindSocket(). When
the feature is disabled (both at build time and runtime), the code
should work exactly as before if possible.

The code that runs in the privileged process should be small, readable
and secure. As one of the first things it should probably close all
file descriptors beside the socket that it uses for communication with
the main process. There should be a mechanism that will select which
privileged operations are supported, maybe some macros that are set by
the system-specific macros. Some arguments could be checked if they
are sane, e.g. if the port number is equal to CNF_GetNTPPort(), to
limit the damage when the main process is compromised.

Please follow the function naming scheme as the rest of the code, i.e.
imperative form, CamelCase after the initial ???_ for public
functions, lowercase and underscores for private functions. Maybe it
wasn't the best choice, but it should be consistent.

I'd like this to be at least two separate patches. First adding the
general support for privilege separation and second adding the Mac OS
support. When sending a new version a patch or patchset, it's good to
change the prefix to "PATCHv2", and increment it later, so it's clear
there was a change and what response belongs to what version of the
patch.

> +++ b/logging.h
> @@ -100,6 +100,7 @@ typedef enum {
>    LOGF_Keys,
>    LOGF_Logging,
>    LOGF_Nameserv,
> +  LOGF_Priv,

Would LOG_PrivOps be better?

> +void
> +NIO_setup_priv_helper(int enable)
> +{
> +  if (enable) {
> +    bind_socket = PRV_bind_socket;
> +  } else {
> +    bind_socket = nio_bind_socket;
> +  }
> +}

I'd rather see the selection between making a call to the privileged
process or calling the function directly in PRV_BindSocket() and
ntp_io.c always calling that function.

> --- /dev/null
> +++ b/privops.c

> +#include <errno.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <sys/time.h>

Could this be replaced with "sysincl.h"?

> +/* daemon request struct */
> +#define ARG_SIZE 64
> +typedef struct {
> +  int op;
> +  int arg_size;
> +  char args[ARG_SIZE];
> +} priv_request_t;

I'd rather avoid the opaque field in the request and response
declarations and use an union like in candm.h/cmdmon/client. It would
simplify reading and writing of the messages. If all members are
short, the whole structure could be sent, the message size would be
constant and easily validated.

> +static int
> +readrequest(int fd, priv_request_t *req)
> +{
> +  int op;
> +  int size;
> +  int nbytes;
> +
> +  nbytes = read(fd, &op, sizeof(op));
> +  if (nbytes < sizeof(op)) {
> +    return -1;
> +  }
> +  nbytes = read(fd, &size, sizeof(size));
> +  if (nbytes < sizeof(size)) {
> +    return -1;
> +  }
> +  nbytes = read(fd, req->args, size);
> +  if (nbytes < size) {
> +    return -1;
> +  }

This should be a single read of the request structure.

> +  if (sendrequest(fd_unix, &req) != 0) {
> +    return -1;
> +  }
> +
> +  if (readresponse(fd_unix, &res) != 0) {
> +    /* something bad happened */
> +    return -1;
> +  }

This should be a single call of a function that will send the request,
read response and check if it's valid.

> +void
> +PRV_start(void)
> +{
> +  pid_t pid;
> +  int sock_pair[2];
> +
> +  if (fd_unix >= 0) {
> +    LOG_FATAL(LOGF_Priv, "PRV_start, helper is already running");
> +  }
> +
> +  if (socketpair(AF_UNIX, SOCK_STREAM, 0, sock_pair) != 0) {
> +    LOG_FATAL(LOGF_Priv, "PRV_start, socketpair() failed : %s", strerror(errno));
> +  }

Shouldn't be the CLOEXEC flag set on the descriptors here? There is
the UTI_FdSetCloexec function for that.

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