Allowing empty keys? or: setting attributes on keys safely

Allowing empty keys? or: setting attributes on keys safely

[Linus Heckemann]

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.

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?

Cheers
Linus

[1]: https://lore.kernel.org/all/alpine.LNX.2.00.1603281843250.15978@sisyphus/

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

[Jarkko Sakkinen]

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?

> 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.]
 
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;

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

BR, Jarkko

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

[Linus Heckemann]

"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

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

[Jarkko Sakkinen]

On Tue Feb 20, 2024 at 9:28 AM UTC, Linus Heckemann wrote:
> "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.

What is the actual problem then, other than feelings?

BR, Jarkko

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

[Jarkko Sakkinen]

On Tue Feb 20, 2024 at 5:49 PM UTC, Jarkko Sakkinen wrote:
> On Tue Feb 20, 2024 at 9:28 AM UTC, Linus Heckemann wrote:
> > "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.
>
> What is the actual problem then, other than feelings?

I mean API semantics almost never should not been changed, even if
it is "stupid" one way or another. So threshold to do anything at 
all should have bar set to quite high...

BR, Jarkko

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

[Maximilian Bosch]

Hi!

> Something prevents you setting invalid payload first, and appropriate
> one later with keyctl_update?

When I look at the implementation of `key_update`[1], it seems as if the
expiry is reset. Is there any way I'm missing which allows me to either
atomically create a key in a keyring with a timeout or to update a key
while retaining the timeout?

Cheers!

Max / ma27

[1] https://github.com/torvalds/linux/blob/v6.8/security/keys/key.c#L1083