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

[Bryan Christianson <bryan@xxxxxxxxxxxxx>]

> On 20/11/2015, at 8:49 PM, Miroslav Lichvar <mlichvar@xxxxxxxxxx> wrote:
> 
> On Fri, Nov 20, 2015 at 03:35:24PM +1300, Bryan Christianson wrote:
> 
>> +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?

I think that PRV_AdjustTime, PRV_BindSocket, PRV_SetTime should return -1 on error otherwise the caller will have to handle 2 different return conventions, depending on whether its compiled to use the helper or the native call.

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

Should this be a fatal error? I was assuming that bind() would fail and then errno on the reponse would report the error back to the daemon.

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

We're reporting a fatal error so it won't really be a leak :) No problem to add a close() tho.

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

I modelled this on send_packet() from ntp_io.c - I think they can be removed.

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

Good point

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