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

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

On Mon, 2014-06-09 at 17:06 +0300, Dmitry Kasatkin wrote:
> On 9 June 2014 16:58, Dmitry Kasatkin <dmitry.kasatkin@gmail.com> wrote:
> > On 9 June 2014 16:48, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >> On Mon, 2014-06-09 at 16:05 +0300, Dmitry Kasatkin wrote:
> >>> On 09/06/14 15:51, Mimi Zohar wrote:
> >>> > On Mon, 2014-06-09 at 15:13 +0300, Dmitry Kasatkin wrote:
> >>> >> On 03/06/14 20:58, Mimi Zohar wrote:
> >>> >>> 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 a particular key on the system keyring.
> >>> >>>
> >>> >>> When the UEFI secure boot keys are added to the system keyring, the
> >>> >>> platform owner will be able to load their key in one of the UEFI DBs
> >>> >>> (eg. Machine Owner Key(MOK) list) and select their key, without
> >>> >>> having to rebuild the kernel.
> >>> >>>
> >>> >>> This patch defines an owner trusted keyring, a new boot command
> >>> >>> line option 'keys_ownerid=', and defines a new function
> >>> >>> get_system_or_owner_trusted_keyring().
> >>> >> Hello,
> >>> >>
> >>> >> The functionality of this entire patch can be replaced by only ~2 lines
> >>> >> of code in x509_request_asymmetric_key()
> >>> >>
> >>> >> if (keys_ownerid || strcmp(keys_ownerid, id))
> >>> >>      return -EPERM;
> >>> >>
> >>> >> Right?
> >>> > Are you suggesting only add the one matching key to the system keyring?
> >>>
> >>> No. I am not suggesting this.
> >>>
> >>> All built in keys are allocated with KEY_ALLOC_TRUSTED flag and
> >>> prep.trusted is set to "true".
> >>>
> >>> So  the following statement has no effect.
> >>
> >> Ok, so it has no affect on adding builtin keys to the system keyring.
> >>
> >>>
> >>> #ifdef CONFIG_SYSTEM_TRUSTED_KEYRING
> >>>         ret = x509_validate_trust(cert, system_trusted_keyring);
> >>>         if (!ret)
> >>>             prep->trusted = 1;
> >>> #endif
> >>
> >> The last patch set changes the test to:
> >>      ret = x509_validate_trust(cert, get_system_or_owner_trusted_keyring());
> >>
> >
> > It does not really mater. I just copied original code to my response.
> >
> >>> Keys which come from user-space will check for
> >>>
> >>> if (keys_ownerid && strcmp(keys_ownerid, id))
> >>>      return -EPERM;
> >>>
> >>>
> >>> So 2 lines patch works fine..
> >>
> >> It works based on the assumption, that you would ever only want a single
> >> key on the 'owner' keyring, which is probably not the case.
> >>
> >
> > There is no any assumption here. I am discussing functionality of this patch.
> > That is exactly what this patch does - loads single key on the owners keyring.
> >
> > There is no need for additional keyring for a single key. That is just
> > enough to limit verification to the owners key id.
> >
> 
> There is no reason to have advanced bloated implementation for unsure,
> may be never coming use-cases.
> 
> It is always very easy to make new patches for the future cases.

Normally I would agree with you, but in this case we already have
multiple use case scenarios (eg. builtin keys, transitioning from one
MOK key to another), but are simply waiting for the proposed UEFI key
patches to be upstreamed.  Basically, we need the UEFI key patches
functionality, if not these specific patches, to safely identify the
owner's key(s), without requiring the end user to rebuild the kernel to
include their key(s).

Mimi