Re: [RFC PATCH v4 4/4] KEYS: define an owner trusted keyring

[Dmitry Kasatkin]

On 1 June 2014 05:14, Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx> wrote:
> On Sat, 2014-05-31 at 01:37 +0300, Dmitry Kasatkin wrote:
>> On 28 May 2014 18:09, Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx> wrote:
>> > (UEFI) secure boot provides a signature chain of trust rooted in
>> > hardware. The signature chain of trust includes the Machine Owner
>> > Keys(MOKs), which cannot be modified without physical presence.
>> >
>> > Instead of allowing public keys, with certificates signed by any
>> > key on the system trusted keyring, to be added to a trusted
>> > keyring, this patch further restricts the certificates to those
>> > signed by the machine owner's key or chosen key.
>> >
>> > This patch defines an owner trusted keyring, defines a new boot
>> > command line option 'keyring=' to designate the machine owner's
>> > chosen key, and renames the function get_system_trusted_keyring()
>> > to get_system_or_owner_trusted_keyring().
>> >
>> > This patch permits the machine owner to safely identify their own
>> > or chosen key, without requiring it to be builtin the kernel.
>> >
>> > Signed-off-by: Mimi Zohar<zohar@xxxxxxxxxxxxxxxxxx>
>> > ---
>> >  crypto/asymmetric_keys/x509_public_key.c |  3 +-
>> >  include/keys/system_keyring.h            | 15 ++++--
>> >  include/linux/key.h                      |  4 ++
>> >  kernel/system_keyring.c                  | 85 ++++++++++++++++++++++++++++++++
>> >  security/keys/key.c                      | 20 ++++++++
>> >  5 files changed, 121 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
>> > index 1af8a30..6d52790 100644
>> > --- a/crypto/asymmetric_keys/x509_public_key.c
>> > +++ b/crypto/asymmetric_keys/x509_public_key.c
>> > @@ -237,7 +237,8 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
>> >                 if (ret < 0)
>> >                         goto error_free_cert;
>> >         } else {
>> > -               ret = x509_validate_trust(cert, get_system_trusted_keyring());
>> > +               ret = x509_validate_trust(cert,
>> > +                                         get_system_or_owner_trusted_keyring());
>> >                 if (!ret)
>> >                         prep->trusted = 1;
>> >         }
>> > diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
>> > index 72665eb..f665c33 100644
>> > --- a/include/keys/system_keyring.h
>> > +++ b/include/keys/system_keyring.h
>> > @@ -17,15 +17,20 @@
>> >  #include <linux/key.h>
>> >
>> >  extern struct key *system_trusted_keyring;
>> > -static inline struct key *get_system_trusted_keyring(void)
>> > -{
>> > -       return system_trusted_keyring;
>> > -}
>> > +extern struct key *owner_trusted_keyring;
>> > +
>> > +extern struct key *get_system_or_owner_trusted_keyring(void);
>> > +extern void load_owner_identified_uefi_key(key_ref_t key);
>> > +
>> >  #else
>> > -static inline struct key *get_system_trusted_keyring(void)
>> > +static inline struct key *get_system_or_owner_trusted_keyring(void)
>> >  {
>> >         return NULL;
>> >  }
>> > +
>> > +static void load_owner_identified_uefi_key(key_ref_t key)
>> > +{
>> > +}
>> >  #endif
>> >
>> >  #endif /* _KEYS_SYSTEM_KEYRING_H */
>> > diff --git a/include/linux/key.h b/include/linux/key.h
>> > index cd0abb8..861843a 100644
>> > --- a/include/linux/key.h
>> > +++ b/include/linux/key.h
>> > @@ -267,6 +267,10 @@ extern int wait_for_key_construction(struct key *key, bool intr);
>> >
>> >  extern int key_validate(const struct key *key);
>> >
>> > +extern int key_match(key_ref_t key,
>> > +                    const char *type,
>> > +                    const char *description);
>> > +
>> >  extern key_ref_t key_create_or_update(key_ref_t keyring,
>> >                                       const char *type,
>> >                                       const char *description,
>> > diff --git a/kernel/system_keyring.c b/kernel/system_keyring.c
>> > index 52ebc70..e9b14ac 100644
>> > --- a/kernel/system_keyring.c
>> > +++ b/kernel/system_keyring.c
>> > @@ -19,11 +19,75 @@
>> >  #include "module-internal.h"
>> >
>> >  struct key *system_trusted_keyring;
>> > +struct key *owner_trusted_keyring;
>> >  EXPORT_SYMBOL_GPL(system_trusted_keyring);
>> >
>> >  extern __initconst const u8 system_certificate_list[];
>> >  extern __initconst const unsigned long system_certificate_list_size;
>> >
>> > +static int use_owner_trusted_keyring;
>> > +
>> > +static char *owner_keyid;
>> > +static int builtin_keyring;
>> > +static int __init default_keyring_set(char *str)
>> > +{
>> > +       if (!str)       /* default: builtin */
>> > +               return 1;
>> > +
>> > +       if (strcmp(str, "system") == 0)  /* use system keyring */
>> > +               ;
>> > +       else if (strcmp(str, "builtin") == 0)  /* only builtin keys */
>> > +               builtin_keyring = 1;
>> > +       else
>> > +               owner_keyid = str;   /* owner local key 'id:xxxxxx' */
>> > +       return 1;
>> > +}
>> > +__setup("keyring=", default_keyring_set);
>> > +
>> > +/*
>> > + * Load the owner identified key on the 'owner' trusted keyring.
>> > + */
>> > +void load_owner_identified_uefi_key(key_ref_t key)
>> > +{
>> > +       if (!owner_keyid || use_owner_trusted_keyring)
>> > +               return;
>> > +
>> > +       if (!key_match(key, "asymmetric", owner_keyid))
>> > +               return;
>> > +
>> > +       if (key_link(owner_trusted_keyring, key_ref_to_ptr(key)) == 0) {
>> > +               set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
>> > +               use_owner_trusted_keyring = 1;
>>
>> This is a bit strange...
>> Linking any key forces to use owner trusted keyring...
>
> Wouldn't it be stranger to identify a specific key on the system keyring
> and add it to the owner keyring, but then not use it?   I don't
> understand your concern.  What is the problem?

This patch technically specifies to use all keys from the system
keyring or the one specified
by the owner_keyid. Why the owner keyring is needed then at all?
owner_keyid can identify the key to use from the system keyring....


>
>> > +               pr_notice("Loaded X.509 cert '%s' on .owner_keyring\n",
>> > +                                 key_ref_to_ptr(key)->description);
>> > +       }
>> > +}
>> > +
>> > +static void load_owner_identified_builtin_key(key_ref_t key)
>> > +{
>> > +       if (!owner_keyid && !builtin_keyring)
>> > +               return;
>> > +
>>
>> It looks like builtin_keyring is useless...
>> Is it like linking all .system keys to owners keyring???
>>
>> Why not just to return .system keyring in get_system_or_owner_trusted_keyring()
>> based on owner_keyid?
>
> Before David and Josh's proposed UEFI secure boot patches, only the
> builtin keys are on the system keyring. After those patches, the UEFI
> secure boot keys, including the MOK keys, are there as well.
>
> I think splitting this patch up into before and after adding the secure
> boot keys to the system keyring, would help clarify this patch.
>

> Mimi
>
>>
>>
>> > +       if (!builtin_keyring && !key_match(key, "asymmetric", owner_keyid))
>> > +               return;
>> > +
>> > +       if (key_link(owner_trusted_keyring, key_ref_to_ptr(key)) == 0) {
>> > +               set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
>> > +               use_owner_trusted_keyring = 1;
>>
>> The same...
>> Linking any key forces to use owner trusted keyring...
>>
>>
>> > +               pr_notice("Loaded X.509 cert '%s' on .owner_keyring\n",
>> > +                                 key_ref_to_ptr(key)->description);
>> > +       }
>> > +}
>> > +
>> > +/*
>> > + * Use the owner_trusted_keyring if available
>> > + */
>> > +struct key *get_system_or_owner_trusted_keyring(void)
>> > +{
>> > +       return use_owner_trusted_keyring ? owner_trusted_keyring :
>> > +               system_trusted_keyring;
>> > +}
>> > +
>> >  /*
>> >   * Load the compiled-in keys
>> >   */
>> > @@ -50,6 +114,25 @@ static __init int system_trusted_keyring_init(void)
>> >  device_initcall(system_trusted_keyring_init);
>> >
>> >  /*
>> > + * Load the owner trusted key
>> > + */
>> > +static __init int owner_trusted_keyring_init(void)
>> > +{
>> > +       pr_notice("Initialize the owner trusted keyring\n");
>> > +
>> > +       owner_trusted_keyring =
>> > +               keyring_alloc(".owner_keyring",
>> > +                             KUIDT_INIT(0), KGIDT_INIT(0), current_cred(),
>> > +                             ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
>> > +                             KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH),
>> > +                             KEY_ALLOC_NOT_IN_QUOTA, NULL);
>> > +       if (IS_ERR(owner_trusted_keyring))
>> > +               panic("Can't allocate owner trusted keyring\n");
>> > +       return 0;
>> > +}
>> > +device_initcall(owner_trusted_keyring_init);
>> > +
>> > +/*
>> >   * Load the compiled-in list of X.509 certificates.
>> >   */
>> >  static __init int load_system_certificate_list(void)
>> > @@ -91,6 +174,8 @@ static __init int load_system_certificate_list(void)
>> >                 } else {
>> >                         pr_notice("Loaded X.509 cert '%s'\n",
>> >                                   key_ref_to_ptr(key)->description);
>> > +
>> > +                       load_owner_identified_builtin_key(key);
>> >                         key_ref_put(key);
>> >                 }
>> >                 p += plen;
>> > diff --git a/security/keys/key.c b/security/keys/key.c
>> > index 2048a11..b448ab1 100644
>> > --- a/security/keys/key.c
>> > +++ b/security/keys/key.c
>> > @@ -701,6 +701,26 @@ void key_type_put(struct key_type *ktype)
>> >         up_read(&key_types_sem);
>> >  }
>> >
>> > +/*
>> > + * Use the key type's match function to compare the key's id.
>> > + */
>> > +int key_match(key_ref_t key, const char *type, const char *description)
>> > +{
>> > +       struct keyring_index_key index_key;
>> > +       int ret = 0;
>> > +
>> > +       index_key.type = key_type_lookup(type);
>> > +       if (IS_ERR(index_key.type))
>> > +               goto out;
>> > +
>> > +       if (index_key.type->match &&
>> > +           index_key.type->match(key_ref_to_ptr(key), description))
>> > +               ret = 1;
>> > +       key_type_put(index_key.type);
>> > +out:
>> > +       return ret;
>> > +}
>> > +
>> >  /*
>> >   * Attempt to update an existing key.
>> >   *
>
>



-- 
Thanks,
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/