Re: Allowing empty keys? or: setting attributes on keys safely

[Linus Heckemann <linus@schreibt.jetzt>]

"Jarkko Sakkinen" <jarkko@kernel.org> writes:

> On Sat Feb 17, 2024 at 6:20 PM UTC, Linus Heckemann wrote:
>> Hi all,
>>
>> We've been fiddling with the keyring functionality; I want to set up a
>> key with an expiry time safely -- i.e. the key data should never be
>> loaded without the expiry time being set.
>
> Something prevents you setting invalid payload first, and appropriate
> one later with keyctl_update?

Nothing, and this is the approach we're taking for now, but having a
zero-length payload feels more semantically appropriate than e.g. a
single NUL byte.


>> I'd expect that I could create a user key with an empty payload, e.g.
>>
>> add_key("user", "some-key", NULL, 0, KEY_SPEC_SESSION_KEYRING);
>>
>> or
>>
>> add_key("user", "some-key", "", 0, KEY_SPEC_SESSION_KEYRING);
>>
>> in order to use keyctl_set_timeout to apply a timeout _before_ the
>> payload is populated using keyctl_update. However, both of these add_key
>> calls return -EINVAL.
>>
>> I found [1] which removed documentation that suggested that this would
>> be allowed, but the reason for not allowing an empty payload is unclear
>> to me; I think it would make sense for my exact use case, and placing a
>> dummy nonempty payload in the keyring first seems like it would be more
>> semantically weird and painful to deal with when reading from the keyring.
>>
>> Is there any reason why this restriction is in place, and is there a
>> more sensible way to apply the timeout before a payload is loaded?
>
> The function in question is user_preparse() (for reference).
>
> Unless I missed a differing key type all key types seem to have the same
> zero check in their implementations of preparse. This change would make
> user key type semantics different than for other key types.
>
> [Please correct me if there is actually key type that does allow zero
> payload.]

keyrings are a key type which not only allow but require the data length
to be 0 (see keyring_preparse). The actual data buffer pointer seems not
to be used but the manpage for add_key says it should be NULL.

> What I do find very confusing in the current call paths is that why this
> zero check does not already happen in key_instantiate_and_link() before
> preparse is called, i.e. why we don't have along the lines of:
>
> $ git diff
> diff --git a/security/keys/key.c b/security/keys/key.c
> index 5b10641debd5..7fa425ab5588 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -510,6 +510,12 @@ int key_instantiate_and_link(struct key *key,
>         prep.quotalen = key->type->def_datalen;
>         prep.expiry = TIME64_MAX;
>         if (key->type->preparse) {
> +               /* Disallow zero payload: */
> +               if (!data || !datalen) {
> +                       ret = -EINVAL;
> +                       goto error;
> +               }
> +
>                 ret = key->type->preparse(&prep);
>                 if (ret < 0)
>                         goto error;

Which answers this question I think?


>> Cheers
>> Linus
>>
>> [1]: https://lore.kernel.org/all/alpine.LNX.2.00.1603281843250.15978@sisyphus/
>
> BR, Jarkko