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

[Miroslav Lichvar <mlichvar@xxxxxxxxxx>]

On Thu, Nov 05, 2015 at 06:02:14PM +1300, Bryan Christianson wrote:
> 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().

Hm, why would the descriptor need to be sent back from the helper
after binding? If possible I'd rather keep the creation of the socket
in the main process and send it to the helper just for the binding
operation.

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

You can call close() on all numbers in range where descriptors are
expected to be (e.g. 0-1024). See go_daemon() in main.c. I'm not sure
if there is a better portable way.

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

If it's really not possible to have a privileged call that binds only
and does nothing else, then I guess yes, the code would be easier to
read I think. But the function name would need to be different, it
does much more than just binding.

> 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);

Would the second form need to be run in the helper? I think at least
on Linux it doesn't require root privileges as it's a read-only
operation. The first one is a special case of the third, so I'd just
add one operator for the third case and ignore the result if the
caller passed NULL.

> and similarly for settimeofday();

I think it's perfectly find to support only the case when tv is not
NULL and tz is NULL.

You can add assert() calls for the unsupported cases to make it clear
they are not supported and abort cleanly if some code tries to use
them.

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