Re: [PATCH v4] KEYS: encrypted: Instantiate key with user-provided decrypted data

[Sumit Garg]

On Fri, 14 Jan 2022 at 00:45, Yael Tiomkin <yaelt@xxxxxxxxxx> wrote:
>
> On Fri, Jan 7, 2022 at 8:32 AM Sumit Garg <sumit.garg@xxxxxxxxxx> wrote:
> >
> > On Fri, 7 Jan 2022 at 18:23, Yael Tiomkin <yaelt@xxxxxxxxxx> wrote:
> > >
> > > Hi Sumit,
> > >
> > > On Fri, Jan 7, 2022 at 12:15 AM Sumit Garg <sumit.garg@xxxxxxxxxx> wrote:
> > > >
> > > > Hi Yael,
> > > >
> > > > On Thu, 6 Jan 2022 at 01:48, Yael Tiomkin <yaelt@xxxxxxxxxx> wrote:
> > > > >
> > > > > Hi Sumit,
> > > > >
> > > > > On Mon, Jan 3, 2022 at 1:51 AM Sumit Garg <sumit.garg@xxxxxxxxxx> wrote:
> > > > > >
> > > > > > Hi Mimi,
> > > > > >
> > > > > > Apologies for the delayed reply as I was on leave for a long new year weekend.
> > > > > >
> > > > > > On Thu, 30 Dec 2021 at 18:59, Mimi Zohar <zohar@xxxxxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > Hi Sumit,
> > > > > > >
> > > > > > > On Thu, 2021-12-30 at 15:37 +0530, Sumit Garg wrote:
> > > > > > > > + Jan, Ahmad
> > > > > > > >
> > > > > > > > On Thu, 30 Dec 2021 at 03:24, Yael Tiomkin <yaelt@xxxxxxxxxx> wrote:
> > > > > > > > >
> > > > > > > > > The encrypted.c class supports instantiation of encrypted keys with
> > > > > > > > > either an already-encrypted key material, or by generating new key
> > > > > > > > > material based on random numbers. This patch defines a new datablob
> > > > > > > > > format: [<format>] <master-key name> <decrypted data length>
> > > > > > > > > <decrypted data> that allows to instantiate encrypted keys using
> > > > > > > > > user-provided decrypted data, and therefore allows to perform key
> > > > > > > > > encryption from userspace. The decrypted key material will be
> > > > > > > > > inaccessible from userspace.
> > > > > > > >
> > > > > > > > This type of user-space key import feature has already been discussed
> > > > > > > > at large in the context of trusted keys here [1]. So what makes it
> > > > > > > > special in case of encrypted keys such that it isn't a "UNSAFE_IMPORT"
> > > > > > > > or "DEBUGGING_IMPORT" or "DEVELOPMENT_IMPORT", ...?
> > > > > > > >
> > > > > > > > [1] https://lore.kernel.org/linux-integrity/74830d4f-5a76-8ba8-aad0-0d79f7c01af9@xxxxxxxxxxxxxx/
> > > > > > > >
> > > > > > > > -Sumit
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Reviewed-by: Mimi Zohar <zohar@xxxxxxxxxxxxx>
> > > > > > > > > Signed-off-by: Yael Tiomkin <yaelt@xxxxxxxxxx>
> > > > > > >
> > > > > > > There is a difference between trusted and encrypted keys.
> > > > > >
> > > > > > Yeah I understand the implementation differences.
> > > > > >
> > > > > > >  So in
> > > > > > > addition to pointing to the rather long discussion thread, please
> > > > > > > summarize the conclusion and, assuming you agree, include why in once
> > > > > > > case it was acceptable and in the other it wasn't to provide userspace
> > > > > > > key data.
> > > > > >
> > > > > > My major concern with importing user-space key data in *plain* format
> > > > > > is that if import is *not* done in a safe (manufacturing or
> > > > > > production) environment then the plain key data is susceptible to
> > > > > > user-space compromises when the device is in the field.
> > > > >
> > > > > I agree this can happen. Key distribution in any scenario needs to be
> > > > > secure and this could also potentially be an issue if the key is first
> > > > > encrypted and then imported.
> > > >
> > > > Currently its not the case with encrypted keys. These are random keys
> > > > generated within the kernel and encrypted with master key within the
> > > > kernel and then exposed to user-space as encrypted blob only.
> > >
> > >
> > > There are two different ways to create encrypted keys. One is to have
> > > them generated within the kernel using random numbers, and the other
> > > is by importing them in their encrypted form from user-space.
> > > I was referring to the latter in my previous statement.
> > >
> >
> > So, from a key distribution security point of view, encrypted key
> > user-space import is **not equal to** plain key user-space import.
> > That's why we need to have a separate compile time option as every
> > device may come with its own threat model and may choose to enable or
> > disable this user-space plain key import feature.
> >
> > -Sumit
> >
> > > >
> > > > > We can make sure the documentation
> > > > > highlights the safety requirement.
> > > > >
> > > >
> > > > IMO, you should enable this feature as a compile time option. The help
> > > > text for that config option should highlight the use-case along with a
> > > > safety warning.
> > > >
> > > > -Sumit
> > > >
> > > > > >
> > > > > > And it sounds like we are diverting from basic definition [1] of encrypted keys:
> > > > > >
> > > > > > "Trusted and Encrypted Keys are two new key types added to the
> > > > > > existing kernel key ring service. Both of these new types are variable
> > > > > > length symmetric keys, and in both cases all keys are created in the
> > > > > > kernel, and **user space sees, stores, and loads** only encrypted
> > > > > > blobs."
> > > > > >
> > > > > > Also, as Jarrko mentioned earlier the use-case is still not clear to
> > > > > > me as well. Isn't user logon keys an alternative option for
> > > > > > non-readable user-space keys?
> > > > >
> > > > > The goal in this change is to allow key encryption from userspace,
> > > > > using user-provided decrypted data. This cannot be achieved in logon
> > > > > keys, which as you mentioned, are simply non-readable user type keys.
> > > > >
> > > > >
> > > > > >
> > > > > > [1] https://www.kernel.org/doc/html/v4.13/security/keys/trusted-encrypted.html
> > > > > >
> > > > > > -Sumit
> > > > > >
> > > > > > >
> > > > > > > thanks,
> > > > > > >
> > > > > > > Mimi
> > > > > > >
> > > > >
> > > > > Yael
>
> Hi Sumit,
> The encrypted-keys module is already controlled by the
> CONFIG_ENCRYPTED_KEYS option, which I think might give sufficient
> granularity to control the behavior. Do you still think a feature
> dedicated option is needed?
>

Since this feature will change security properties for encrypted keys
which can be considered as a regression for existing users during
uprev. So it needs to be an optional key import feature with a clear
user visible warning. Thinking more about this, you can even have a
module param to turn it on with a runtime warning as well.

-Sumit

> Yael