Re: [PATCH] KEYS: prevent KEYCTL_READ on negative key

[Eric Biggers]

On Mon, Sep 25, 2017 at 02:29:56PM +0100, David Howells wrote:
> Eric Biggers <ebiggers3@xxxxxxxxx> wrote:
> 
> > Putting the check in key_validate() would make lookups with
> > KEY_LOOKUP_PARTIAL stop returning negative keys, which would break
> > keyctl_describe(), keyctl_chown(), keyctl_setperm(), keyctl_set_timeout(),
> > keyctl_get_security() on negative keys.  I presume those are supposed to
> > work?
> 
> Lookups with KEY_LOOKUP_PARTIAL should never return a negative key by
> definition.  A negative key is instantiated with an error code, so is no longer
> under construction.

Well, that's not what KEY_LOOKUP_PARTIAL actually does.  KEY_LOOKUP_PARTIAL
allows the returned key to be uninstantiated, negatively instantiated, *or*
positively instantiated; and the callers seem to rely on that.  Perhaps a better
name might have been KEY_LOOKUP_ALLOW_PARTIAL or KEY_LOOKUP_ALLOW_NONPOSITIVE.

On the other hand, without KEY_LOOKUP_PARTIAL, the returned key is required to
be positively instantiated.  However, there is a special case.  Namely, if no
permissions are requested, the returned key is allowed to be negative (as well
as revoked, invalidated, or expired --- though those can happen at any time
anyway until you do down_read(&key->sem)).  I'm questioning whether we need that
special case.

> 
> key_get_instantiation_authkey() must fail if the key has been constructed - but
> I guess there's a potential race in keyctl_describe_key(), keyctl_set_timeout()
> and keyctl_get_security() between getting the auth token and calling
> lookup_user_key() with perm of 0 in which the key could be instantiated,
> revoked, or instantiated elsewhere, or simply expire.  This would allow the
> instantiating process a longer access window - but they do/did have a valid
> token...

Yes, by the time key_get_instantiation_authkey() returns, the key may have
already been instantiated.  But I'm not too concerned about that, since the
caller must still have had a non-revoked authorization key shortly before.

> 
> It should still be possible to describe, chown, setperm and get the security on
> negative keys by the normal access mechanism.  Changing the timeout should
> probably be denied.
> 
> > Another solution would be to remove the special case from lookup_user_key()
> > where it can return a negative/revoked/invalidated/expired key if
> > KEY_LOOKUP_PARTIAL is not specified and the 'perm' mask is 0.
> 
> There are a number of circumstances in which it lookup_user_key() is called
> with perm==0, and in each case, the caller is responsible for handling the
> security:
> 
>  (1) keyctl_invalidate_key() will do so if the caller doesn't have permission,
>      but CAP_SYS_ADMIN is set and the key is marked KEY_FLAG_ROOT_CAN_INVAL.
> 
>  (2) keyctl_keyring_clear() will do so if the caller doesn't have permission,
>      but CAP_SYS_ADMIN is set and the key is marked KEY_FLAG_ROOT_CAN_CLEAR.
> 
>  (3) keyctl_keyring_unlink() will do so on the key-to-be-removed since only the
>      keyring needs a perm check.
> 
>  (4) keyctl_read_key() always does so and then does the READ perm check and the
>      possessor-can-SEARCH can search check itself.
> 
>  (5) keyctl_describe_key(), keyctl_set_timeout() and keyctl_get_security() will
>      do so if the caller doesn't have permission, but does have a valid
>      authorisation token.  The latter requires that the key be under
>      construction.

It's not about the permission checks.  It's about whether a negative key is
allowed to be returned or not.  And I think overloading 'perm' for that is not
really appropriate, and the cause of the bug in keyctl_read_key().  See the
code, it ignores the return value of wait_for_key_construction() if 'perm' is 0:

        if (!(lflags & KEY_LOOKUP_PARTIAL)) {
                ret = wait_for_key_construction(key, true);
                switch (ret) {
                case -ERESTARTSYS:
                        goto invalid_key;
                default:
                        if (perm)
                                goto invalid_key;
                case 0:
                        break;
                }
	}

> 
> Functions that use KEY_LOOKUP_PARTIAL include:
> 
>  keyctl_describe_key()
>  keyctl_chown_key()
>  keyctl_setperm_key()
>  keyctl_set_timeout()
>  keyctl_get_security()
> 
> all of which might need to be called from the upcall program.  None of these
> should look at the payload.
> 
> > The only callers it would affect are the case in question here which is
> > clearly a bug,
> 
> keyctl_read_key() is definitely buggy.  Actually, rather than manually testing
> KEY_FLAG_NEGATIVE there, it should probably use key_validate().

It already does use key_validate().  But key_validate() is also used in
lookup_user_key(), where it is expected to accept a negative key.

The real problem seems to be that the permissions mask rather than the flags
argument is used to tell lookup_user_key() whether it can return a negative key.

> 
> > and the root-only exceptions for keyctl_invalidate() and
> > keyctl_clear().  And I suspect the latter two are unintentional as well.
> 
> I'm not sure what you think is unintentional.
> 

That root is allowed to invalidate or clear a
negative/invalidated/revoked/expired key or keyring but regular users cannot.

Again, the problem seems to be that the 'perm' argument is used for more than
just the permission check.  I think the 'lflags' argument should indicate what
state the key is allowed to be in, not 'perm'.

> > (Is root *supposed* to be able to invalidate a
> > negative/revoked/invalidated/expired key, or clear a
> > revoked/invalidated/expired keyring?)
> 
> You should be able to invalidate or unlink negative, revoked or expired keys if
> you have permission to do so.  If you're using keyrings to cache stuff, you
> need to be able to invalidate negative results as well as positive ones.
> 
> Invalidation of an invalidated key doesn't really make sense, but it shouldn't
> hurt.  I can't immediately automatically remove all links to the invalidated
> key, but have to leave it to the garbage collector to effect.
> 
> As for clearing of revoked/invalidated/expired keyrings, I'm not sure whether
> it makes sense to allow it - however, whilst keyrings are cleared upon
> revocation (since we have a definite point to do that with the key sem
> writelocked), they aren't automatically cleared upon expiry or invalidation, so
> it might make sense to permit it still.
> 

Eric