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

[Bryan Christianson <bryan@xxxxxxxxxxxxx>]

> On 5/11/2015, at 1:13 AM, Miroslav Lichvar <mlichvar@xxxxxxxxxx> wrote:
> 
> 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.

No problem - I expected that and thought it was better to get something underway.

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

PRV_BindSocket() has to create the socket, unless we create the socket in the daemon, pass it in, bind() and then send it back. I'm not sure sending the socket in 2 directions is a good idea. That's why I call nio_bind_socket() from the privileged helper rather just factoring out the call to bind().
 
> 
> 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.

I had wondered about that but wasn't sure quite how to do it. Is there an array of descriptors I can iterate?

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

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

Ah - the nuances of style :) No problem.

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

Sure - no problem.

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

Should the code I factored out in ntp_io.c as nio_bind_socket() be moved to privops.c ? I had thought it was better to leave it where it was and just expose it through PRV_bind_socket.
> 
> 
>> --- /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"?

Yeah - I should have caught that one

> 
>> +/* 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.

We do have the case where some of the function arguments are optionally NULL (adjtime() and settimeofday()). Currently I set up a single operator for each function call, but maybe I should use a separate operator for each combination of args. The union for adjtime would then have either one or two struct timeval members.

e.g. 
	op_ADJTIME_first ==> adjtime(tv, NULL);
	op_ADJTIME_second ==> adjtime(NULL, res); // not on MacOS - it segfaults
	op_ADJTIME_both ==> adjtime(tv, res);

and similarly for settimeofday();

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

OK - I think as long as the request and response are each less that 512 bytes, we'll get the whole struct in a single IO.

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

OK

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

Ooops - had intended to do that but it got overlooked. Thanks.

--
Bryan


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