[libvirt] [PATCH 5/7] Convert remote daemon & acl code to use polkit API

[John Ferlan]

On 09/10/2014 10:20 AM, Daniel P. Berrange wrote:
> Convert the remote daemon auth check and the access control
> code to use the common polkit API for checking auth.
> 
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> ---
>  daemon/remote.c                    | 235 ++-----------------------------------
>  src/access/viraccessdriverpolkit.c |  91 ++++++--------
>  2 files changed, 45 insertions(+), 281 deletions(-)
> 

<...snip...>

Since this was pushed yesterday - my morning Coverity picked up on an
issue this morning...

> -static char *
> -virAccessDriverPolkitFormatProcess(const char *actionid)
> +static int
> +virAccessDriverPolkitGetCaller(const char *actionid,
> +                               pid_t *pid,
> +                               unsigned long long *startTime,
> +                               uid_t *uid)
>  {
>      virIdentityPtr identity = virIdentityGetCurrent();
> -    pid_t pid;
> -    unsigned long long startTime;
> -    uid_t uid;
> -    char *ret = NULL;
> -#ifndef PKCHECK_SUPPORTS_UID
> -    static bool polkitInsecureWarned;
> -#endif
> +    int ret = -1;
>  
>      if (!identity) {
>          virAccessError(VIR_ERR_ACCESS_DENIED,
>                         _("Policy kit denied action %s from <anonymous>"),
>                         actionid);
> -        return NULL;
> +        return -1;
>      }
> -    if (virIdentityGetUNIXProcessID(identity, &pid) < 0)
> +    if (virIdentityGetUNIXProcessID(identity, pid) < 0)

(1) Event deref_ptr_in_call: 	Dereferencing pointer "pid". [details]
Also see events: 	[check_after_deref]


>          goto cleanup;
> -    if (virIdentityGetUNIXProcessTime(identity, &startTime) < 0)
> +    if (virIdentityGetUNIXProcessTime(identity, startTime) < 0)
>          goto cleanup;
> -    if (virIdentityGetUNIXUserID(identity, &uid) < 0)
> +    if (virIdentityGetUNIXUserID(identity, uid) < 0)
>          goto cleanup;
>  
>      if (!pid) {

(2) Event check_after_deref: 	Null-checking "pid" suggests that it may
be null, but it has already been dereferenced on all paths leading to
the check.
Also see events: 	[deref_ptr_in_call]

Should this reference now be "(!*pid)" ?

John

> @@ -101,25 +99,14 @@ virAccessDriverPolkitFormatProcess(const char *actionid)
>                         _("No UNIX process ID available"));
>          goto cleanup;
>      }
> -    if (!startTime) {
> -        virAccessError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("No UNIX process start time available"));
> -        goto cleanup;
> -    }
>  
> -#ifdef PKCHECK_SUPPORTS_UID
> -    if (virAsprintf(&ret, "%llu,%llu,%llu",
> -                    (unsigned long long)pid, startTime, (unsigned long long)uid) < 0)
> +    if (virIdentityGetUNIXProcessTime(identity, startTime) < 0)
>          goto cleanup;
> -#else
> -    if (!polkitInsecureWarned) {
> -        VIR_WARN("No support for caller UID with pkcheck. This deployment is known to be insecure.");
> -        polkitInsecureWarned = true;
> -    }
> -    if (virAsprintf(&ret, "%llu,%llu",
> -                    (unsigned long long)pid, startTime) < 0)
> +
> +    if (virIdentityGetUNIXUserID(identity, uid) < 0)
>          goto cleanup;
> -#endif
> +
> +    ret = 0;
>  
>   cleanup:
>      virObjectUnref(identity);