Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Roberto Sassu]

On 8/2/2019 10:23 PM, Tyler Hicks wrote:
> On 2019-08-02 22:42:26, Jarkko Sakkinen wrote:
> > On Fri, Aug 02, 2019 at 09:27:22AM -0500, Tyler Hicks wrote:
> > > On 2019-08-02 10:21:16, Roberto Sassu wrote:
> > > > On 8/1/2019 6:32 PM, Jarkko Sakkinen wrote:
> > > > > On Mon, Jul 15, 2019 at 06:44:28PM +0200, Roberto Sassu wrote:
> > > > > > According to the bug report at https://bugs.archlinux.org/task/62678,
> > > > > > the trusted module is a dependency of the ecryptfs module. We should
> > > > > > load the trusted module even if the TPM is inactive or deactivated.
> > > > > >
> > > > > > Given that commit 782779b60faa ("tpm: Actually fail on TPM errors during
> > > > > > "get random"") changes the return code of tpm_get_random(), the patch
> > > > > > should be modified to ignore the -EIO error. I will send a new version.
> > > > >
> > > > > Do you have information where this dependency comes from?
> > > >
> > > > ecryptfs retrieves the encryption key from encrypted keys (see
> > > > ecryptfs_get_encrypted_key()).
> > >
> > > That has been there for many years with any problems. It was added
> > > in 2011:
> > >
> > >   commit 1252cc3b232e582e887623dc5f70979418caaaa2
> > >   Author: Roberto Sassu <roberto.sassu@xxxxxxxxx>
> > >   Date:   Mon Jun 27 13:45:45 2011 +0200
> > >
> > >       eCryptfs: added support for the encrypted key type
> > >
> > > What's recently changed the situation is this patch:
> > >
> > >   commit 240730437deb213a58915830884e1a99045624dc
> > >   Author: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > >   Date:   Wed Feb 6 17:24:51 2019 +0100
> > >
> > >       KEYS: trusted: explicitly use tpm_chip structure from tpm_default_chip()
> > >
> > > Now eCryptfs has a hard dependency on a TPM chip that's working
> > > as expected even if eCryptfs (or the rest of the system) isn't utilizing
> > > the TPM. If the TPM behaves unexpectedly, you can't access your files.
> > > We need to get this straightened out soon.
> >
> > I agree with this conclusion that eCryptfs needs to be fixed, not
> > another workaround to trusted.ko.
>
> That wasn't the conclusion that I came to. I prefer Robert's proposed
> change to trusted.ko.
>
> How do you propose that this be fixed in eCryptfs?
>
> Removing encrypted_key support from eCryptfs is the only way that I can
> see to fix the bug in eCryptfs. That support has been there since 2011.
> I'm not sure of the number of users that would be broken by removing
> encrypted_key support. I don't think the number is high but I can't say
> that confidently.
>
> Roberto, what was your use case when you added encrypted_key support to
> eCryptfs back then? Are you aware of any users of eCryptfs +
> encrypted_keys?

Well, my use case at that time (I was at the university) was to provide
a secure storage based on TPM. I'm not aware of users of this
functionality, but I believe that it is reasonable (depending on how
trusted keys are used, the secure storage is available only on a
specific platform and if the software configuration is good). This
secure storage would be even more effective if it is coupled with
Mandatory Access Control, to release sensitive data only to the
legitimate applications.


> Jarkko, removing a long-standing feature is potentially more disruptive
> to users than adding a workaround to trusted.ko which already requires a
> similar workaround. I don't think that I agree with you on the proper
> fix here.

I also don't think it is a good idea to remove this functionality.

Jarkko, we were discussing about this issue in another thread, and your
answer then (https://lkml.org/lkml/2019/3/21/396) was that it is a
priority to fix the regression.

The patch I proposed (https://lkml.org/lkml/2019/8/2/1282, I will apply
Tyler's comments) is not invasive and fixes the issue. If the digests
pointer is still NULL, pcrlock() returns an error.

Regarding Mimi's proposal to avoid the issue by extending the PCR with
zeros, I think it also achieve the goal. The purpose of pcrlock() is to
prevent unsealing of sealed data outside the kernel. If PCR values do
not change, sealed data can be unsealed and misused by a user space
process.

Extending the PCR with any value would be sufficient, as the extend
operation (the only way to update PCRs) was designed in a way that
finding the value that would be passed to the extend operation to obtain
again one of the previous PCR values is computationally infeasible.

Roberto

--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli