lock_pool drops capabilities even when running as root

[Mike Crowe]

On Thursday 04 December 2014 at 09:13:54 +0100, Werner Koch wrote:
> On Wed,  3 Dec 2014 18:57, mac at mcrowe.com said:
> 
> > We upgraded libgcrypt so we could use
> > gcry_control(GCRYCTL_DISABLE_PRIV_DROP) but this did not help (for reasons
> > that were obvious once I looked more closely.)
> 
> That was the idea.  It seems it has not been tested with capabilities.
> What do you think of this patch to make GCRYCTL_DISABLE_PRIV_DROP work
> as expected:
> 
> diff --git a/src/secmem.c b/src/secmem.c
> index cfea921..4db80a8 100644
> --- a/src/secmem.c
> +++ b/src/secmem.c
> @@ -245,15 +245,21 @@ lock_pool (void *p, size_t n)
>    {
>      cap_t cap;
>  
> -    cap = cap_from_text ("cap_ipc_lock+ep");
> -    cap_set_proc (cap);
> -    cap_free (cap);
> +    if (!no_priv_drop)
> +      {
> +        cap = cap_from_text ("cap_ipc_lock+ep");
> +        cap_set_proc (cap);
> +        cap_free (cap);
> +      }
>      err = no_mlock? 0 : mlock (p, n);
>      if (err && errno)
>        err = errno;
> -    cap = cap_from_text ("cap_ipc_lock+p");
> -    cap_set_proc (cap);
> -    cap_free(cap);
> +    if (!no_priv_drop)
> +      {
> +        cap = cap_from_text ("cap_ipc_lock+p");
> +        cap_set_proc (cap);
> +        cap_free(cap);
> +      }
>    }
>  
>    if (err)

Hi Werner,

Thanks for the reply.

It's now clear to me that my patch was wrong because it also disabled the
use of mlock. :(

I'm reasonably sure that your patch will solve the problem for us if we set
CRYCTL_DISABLE_PRIV_DROP but I don't think it is consistent with the
behaviour when USE_CAPABILITIES is not defined. In that case when
UID==EUID==0 setting GCRYCTL_DISABLE_PRIV_DROP is not required in order to
avoid dropping privileges because of the first condition in this code:

  /* Test whether we are running setuid(0).  */
  if (uid && ! geteuid ())
    {
      /* Yes, we are.  */
      if (!no_priv_drop)
        {
          /* Check that we really dropped the privs.
           * Note: setuid(0) should always fail */
          if (setuid (uid) || getuid () != geteuid () || !setuid (0))
            log_fatal ("failed to reset uid: %s\n", strerror (errno));
        }
    }

However, by looking at this code I may have spotted a more serious problem
(although it's not one that affects our use case.) Suppose I have an
application that is setuid root so that libgcrypt can use
mlock. (Presumably that's the intention of the code quoted above.)

I compile and test this program against a version of libgcrypt compiled
without capabilities support. The program loses privileges correctly as
soon as the mlocking is done via the #if defined(HAVE_MLOCK) code in
lock_pool since it does not call GCRYCTL_DISABLE_PRIV_DROP. It goes on to
allow access to files safe in the knowledge that by that point it is
running with the real UID of the user.

Someone comes along and upgrades libgcrypt to a version with capabilities
support. Now my program will lose all its capabilities but it will continue
to run eith EUID==0. All the code that follows for accessing files is now
doing so as root!

Given this situation I think that:

1. Your patch should not modify capabilities at all if EUID==UID==0 no
   matter what no_priv_drop contains.

2. The USE_CAPABILITIES code should also drop traditional privileges too in
   case the application is setuid root (taking care to do so before losing
   CAP_SETUID.)

3. The code in secmem_init also looks like it needs the same treatment.

I can prepare a patch to do this if you agree.

But, to be honest, I'm not really sure that a library like libgcrypt should
be in the business of changing privileges in this way anyway.

Thanks.

Mike.