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

[Josh Boyer]

On Mon, Jun 02, 2014 at 02:40:28PM +0300, Dmitry Kasatkin wrote:
> On 2 June 2014 14:33, Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx> wrote:
> > On Mon, 2014-06-02 at 13:48 +0300, Dmitry Kasatkin wrote:
> >> 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....
> >
> > Currently only the builtin keys are on the system keyring, but once
> > David and Josh's UEFI patches are upstreamed, the UEFI keys will also be
> > on the system keyring.  At that point, we would want to differentiate
> > between the builtin keys and the remaining keys on the system keyring.
> > Splitting this patch definitely helps clarify what's happening and, more
> > importantly, why.
> >
> > Mimi
> >
> 
> Ok. May be would should focus on patches for existing functionality.
> If something new comes from other side, it can be addressed by new
> another set of patches.

I agree.  I appreciate taking the UEFI key patches into account, but
they're held up and won't be hitting mainline in the next release or
two.  Focus on the changes you need to make against mainline.

josh
--
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/