Re: [PATCH] certs: Restrict blacklist updates to the secondary trusted keyring
On Tue, 2023-09-12 at 17:11 +, Eric Snowberg wrote:
>
> > On Sep 12, 2023, at 5:54 AM, Mimi Zohar wrote:
> >
> > On Tue, 2023-09-12 at 02:00 +, Eric Snowberg wrote:
> >>
> >>> On Sep 11, 2023, at 5:08 PM, Mimi Zohar wrote:
> >>>
> >>> On Mon, 2023-09-11 at 22:17 +, Eric Snowberg wrote:
> >>>>
> >>>>> On Sep 11, 2023, at 10:51 AM, Mickaël Salaün wrote:
> >>>>>
> >>>>> On Mon, Sep 11, 2023 at 09:29:07AM -0400, Mimi Zohar wrote:
> >>>>>> Hi Eric,
> >>>>>>
> >>>>>> On Fri, 2023-09-08 at 17:34 -0400, Eric Snowberg wrote:
> >>>>>>> Currently root can dynamically update the blacklist keyring if the
> >>>>>>> hash
> >>>>>>> being added is signed and vouched for by the builtin trusted keyring.
> >>>>>>> Currently keys in the secondary trusted keyring can not be used.
> >>>>>>>
> >>>>>>> Keys within the secondary trusted keyring carry the same capabilities
> >>>>>>> as
> >>>>>>> the builtin trusted keyring. Relax the current restriction for
> >>>>>>> updating
> >>>>>>> the .blacklist keyring and allow the secondary to also be referenced
> >>>>>>> as
> >>>>>>> a trust source. Since the machine keyring is linked to the secondary
> >>>>>>> trusted keyring, any key within it may also be used.
> >>>>>>>
> >>>>>>> An example use case for this is IMA appraisal. Now that IMA both
> >>>>>>> references the blacklist keyring and allows the machine owner to add
> >>>>>>> custom IMA CA certs via the machine keyring, this adds the additional
> >>>>>>> capability for the machine owner to also do revocations on a running
> >>>>>>> system.
> >>>>>>>
> >>>>>>> IMA appraisal usage example to add a revocation for /usr/foo:
> >>>>>>>
> >>>>>>> sha256sum /bin/foo | awk '{printf "bin:" $1}' > hash.txt
> >>>>>>>
> >>>>>>> openssl smime -sign -in hash.txt -inkey machine-private-key.pem \
> >>>>>>> -signer machine-certificate.pem -noattr -binary -outform DER \
> >>>>>>> -out hash.p7s
> >>>>>>>
> >>>>>>> keyctl padd blacklist "$(< hash.txt)" %:.blacklist < hash.p7s
> >>>>>>>
> >>>>>>> Signed-off-by: Eric Snowberg
> >>>>>>
> >>>>>> The secondary keyring may include both CA and code signing keys. With
> >>>>>> this change any key loaded onto the secondary keyring may blacklist a
> >>>>>> hash. Wouldn't it make more sense to limit blacklisting
> >>>>>> certificates/hashes to at least CA keys?
> >>>>>
> >>>>> Some operational constraints may limit what a CA can sign.
> >>>>
> >>>> Agreed.
> >>>>
> >>>> Is there precedents for requiring this S/MIME to be signed by a CA?
> >>>>
> >>>>> This change is critical and should be tied to a dedicated kernel config
> >>>>> (disabled by default), otherwise existing systems using this feature
> >>>>> will have their threat model automatically changed without notice.
> >>>>
> >>>> Today we have INTEGRITY_CA_MACHINE_KEYRING_MAX. This can
> >>>> be enabled to enforce CA restrictions on the machine keyring. Mimi,
> >>>> would
> >>>> this be a suitable solution for what you are after?
> >>>
> >>> There needs to be some correlation between the file hashes being added
> >>> to the blacklist and the certificate that signed them. Without that
> >>> correlation, any key on the secondary trusted keyring could add any
> >>> file hashes it wants to the blacklist.
> >>
> >> Today any key in the secondary trusted keyring can be used to validate a
> >> signed kernel module. At a later time, if a new hash is added to the
> >> blacklist
> >> keyring to revoke loading a signed kernel module, the ability to do the
> >> revocation with this additio
Re: [PATCH] integrity: powerpc: Do not select CA_MACHINE_KEYRING
On Tue, 2023-09-12 at 22:32 +0300, Jarkko Sakkinen wrote: > On Tue Sep 12, 2023 at 10:22 PM EEST, Mimi Zohar wrote: > > On Tue, 2023-09-12 at 12:49 +0300, Jarkko Sakkinen wrote: > > > On Tue Sep 12, 2023 at 10:41 AM EEST, Michal Suchánek wrote: > > > > On Mon, Sep 11, 2023 at 11:39:38PM -0400, Nayna wrote: > > > > > > > > > > On 9/7/23 13:32, Michal Suchánek wrote: > > > > > > Adding more CC's from the original patch, looks like > > > > > > get_maintainers is > > > > > > not that great for this file. > > > > > > > > > > > > On Thu, Sep 07, 2023 at 06:52:19PM +0200, Michal Suchanek wrote: > > > > > > > No other platform needs CA_MACHINE_KEYRING, either. > > > > > > > > > > > > > > This is policy that should be decided by the administrator, not > > > > > > > Kconfig > > > > > > > dependencies. > > > > > > > > > > We certainly agree that flexibility is important. However, in this > > > > > case, > > > > > this also implies that we are expecting system admins to be security > > > > > experts. As per our understanding, CA based infrastructure(PKI) is the > > > > > standard to be followed and not the policy decision. And we can only > > > > > speak > > > > > for Power. > > > > > > > > > > INTEGRITY_CA_MACHINE_KEYRING ensures that we always have CA signed > > > > > leaf > > > > > certs. > > > > > > > > And that's the problem. > > > > > > > > From a distribution point of view there are two types of leaf certs: > > > > > > > > - leaf certs signed by the distribution CA which need not be imported > > > >because the distribution CA cert is enrolled one way or another > > > > - user generated ad-hoc certificates that are not signed in any way, > > > >and enrolled by the user > > > > > > > > The latter are vouched for by the user by enrolling the certificate, and > > > > confirming that they really want to trust this certificate. Enrolling > > > > user certificates is vital for usability or secure boot. Adding extra > > > > step of creating a CA certificate stored on the same system only > > > > complicates things with no added benefit. > > > > > > This all comes down to the generic fact that kernel should not > > > proactively define what it *expects* sysadmins. > > > > > > CA based infrastructure like anything is a policy decision not > > > a decision to be enforced by kernel. > > > > Secure boot requires a signature chain of trust. IMA extends the > > secure and trusted boot concepts to the kernel. Missing from that > > signature chain of trust is the ability of allowing the end > > machine/system owner to load other certificates without recompiling the > > kernel. The introduction of the machine keyring was to address this. > > > > I'm not questioning the end user's intent on loading local or third > > party keys via the normal mechanisms. If the existing mechanism(s) for > > loading local or third party keys were full-proof, then loading a > > single certificate, self-signed or not, would be fine. However, that > > isn't the reality. The security of the two-stage approach is simply > > not equivalent to loading a single certificate. Documentation could > > help the end user/system owner to safely create (and manage) separate > > certificate signing and code signing certs. > > > > Unlike UEFI based systems, PowerVM defines two variables trustedcadb > > and moduledb, for storing certificate signing and code signing > > certificates respectively. First the certs on the trustedcadb are > > loaded and then the ones on moduledb are loaded. > > There's pragmatic reasons to make things more open than they should be > in production. As a hardware example I still possess Raspberry Pi 3B for > test workloads because it has a broken TZ implementation. The world is > really bigger than production workloads. > > It would be better to document what you said rather than enforce the > right choice IMHO (e.g. extend Kconfig documentation). PowerVM LPARs are more about production workloads than a Raspberry Pi. :) -- thanks, Mimi
Re: [PATCH] integrity: powerpc: Do not select CA_MACHINE_KEYRING
On Tue, 2023-09-12 at 12:49 +0300, Jarkko Sakkinen wrote: > On Tue Sep 12, 2023 at 10:41 AM EEST, Michal Suchánek wrote: > > On Mon, Sep 11, 2023 at 11:39:38PM -0400, Nayna wrote: > > > > > > On 9/7/23 13:32, Michal Suchánek wrote: > > > > Adding more CC's from the original patch, looks like get_maintainers is > > > > not that great for this file. > > > > > > > > On Thu, Sep 07, 2023 at 06:52:19PM +0200, Michal Suchanek wrote: > > > > > No other platform needs CA_MACHINE_KEYRING, either. > > > > > > > > > > This is policy that should be decided by the administrator, not > > > > > Kconfig > > > > > dependencies. > > > > > > We certainly agree that flexibility is important. However, in this case, > > > this also implies that we are expecting system admins to be security > > > experts. As per our understanding, CA based infrastructure(PKI) is the > > > standard to be followed and not the policy decision. And we can only speak > > > for Power. > > > > > > INTEGRITY_CA_MACHINE_KEYRING ensures that we always have CA signed leaf > > > certs. > > > > And that's the problem. > > > > From a distribution point of view there are two types of leaf certs: > > > > - leaf certs signed by the distribution CA which need not be imported > >because the distribution CA cert is enrolled one way or another > > - user generated ad-hoc certificates that are not signed in any way, > >and enrolled by the user > > > > The latter are vouched for by the user by enrolling the certificate, and > > confirming that they really want to trust this certificate. Enrolling > > user certificates is vital for usability or secure boot. Adding extra > > step of creating a CA certificate stored on the same system only > > complicates things with no added benefit. > > This all comes down to the generic fact that kernel should not > proactively define what it *expects* sysadmins. > > CA based infrastructure like anything is a policy decision not > a decision to be enforced by kernel. Secure boot requires a signature chain of trust. IMA extends the secure and trusted boot concepts to the kernel. Missing from that signature chain of trust is the ability of allowing the end machine/system owner to load other certificates without recompiling the kernel. The introduction of the machine keyring was to address this. I'm not questioning the end user's intent on loading local or third party keys via the normal mechanisms. If the existing mechanism(s) for loading local or third party keys were full-proof, then loading a single certificate, self-signed or not, would be fine. However, that isn't the reality. The security of the two-stage approach is simply not equivalent to loading a single certificate. Documentation could help the end user/system owner to safely create (and manage) separate certificate signing and code signing certs. Unlike UEFI based systems, PowerVM defines two variables trustedcadb and moduledb, for storing certificate signing and code signing certificates respectively. First the certs on the trustedcadb are loaded and then the ones on moduledb are loaded. -- thanks, Mimi
Re: [PATCH] certs: Restrict blacklist updates to the secondary trusted keyring
On Tue, 2023-09-12 at 02:00 +, Eric Snowberg wrote:
>
> > On Sep 11, 2023, at 5:08 PM, Mimi Zohar wrote:
> >
> > On Mon, 2023-09-11 at 22:17 +, Eric Snowberg wrote:
> >>
> >>> On Sep 11, 2023, at 10:51 AM, Mickaël Salaün wrote:
> >>>
> >>> On Mon, Sep 11, 2023 at 09:29:07AM -0400, Mimi Zohar wrote:
> >>>> Hi Eric,
> >>>>
> >>>> On Fri, 2023-09-08 at 17:34 -0400, Eric Snowberg wrote:
> >>>>> Currently root can dynamically update the blacklist keyring if the hash
> >>>>> being added is signed and vouched for by the builtin trusted keyring.
> >>>>> Currently keys in the secondary trusted keyring can not be used.
> >>>>>
> >>>>> Keys within the secondary trusted keyring carry the same capabilities as
> >>>>> the builtin trusted keyring. Relax the current restriction for updating
> >>>>> the .blacklist keyring and allow the secondary to also be referenced as
> >>>>> a trust source. Since the machine keyring is linked to the secondary
> >>>>> trusted keyring, any key within it may also be used.
> >>>>>
> >>>>> An example use case for this is IMA appraisal. Now that IMA both
> >>>>> references the blacklist keyring and allows the machine owner to add
> >>>>> custom IMA CA certs via the machine keyring, this adds the additional
> >>>>> capability for the machine owner to also do revocations on a running
> >>>>> system.
> >>>>>
> >>>>> IMA appraisal usage example to add a revocation for /usr/foo:
> >>>>>
> >>>>> sha256sum /bin/foo | awk '{printf "bin:" $1}' > hash.txt
> >>>>>
> >>>>> openssl smime -sign -in hash.txt -inkey machine-private-key.pem \
> >>>>> -signer machine-certificate.pem -noattr -binary -outform DER \
> >>>>> -out hash.p7s
> >>>>>
> >>>>> keyctl padd blacklist "$(< hash.txt)" %:.blacklist < hash.p7s
> >>>>>
> >>>>> Signed-off-by: Eric Snowberg
> >>>>
> >>>> The secondary keyring may include both CA and code signing keys. With
> >>>> this change any key loaded onto the secondary keyring may blacklist a
> >>>> hash. Wouldn't it make more sense to limit blacklisting
> >>>> certificates/hashes to at least CA keys?
> >>>
> >>> Some operational constraints may limit what a CA can sign.
> >>
> >> Agreed.
> >>
> >> Is there precedents for requiring this S/MIME to be signed by a CA?
> >>
> >>> This change is critical and should be tied to a dedicated kernel config
> >>> (disabled by default), otherwise existing systems using this feature
> >>> will have their threat model automatically changed without notice.
> >>
> >> Today we have INTEGRITY_CA_MACHINE_KEYRING_MAX. This can
> >> be enabled to enforce CA restrictions on the machine keyring. Mimi, would
> >> this be a suitable solution for what you are after?
> >
> > There needs to be some correlation between the file hashes being added
> > to the blacklist and the certificate that signed them. Without that
> > correlation, any key on the secondary trusted keyring could add any
> > file hashes it wants to the blacklist.
>
> Today any key in the secondary trusted keyring can be used to validate a
> signed kernel module. At a later time, if a new hash is added to the
> blacklist
> keyring to revoke loading a signed kernel module, the ability to do the
> revocation with this additional change would be more restrictive than loading
> the original module.
A public key on the secondary keyring is used to verify code that it
signed, but does not impact any other code. Allowing any public key on
the secondary keyring to blacklist any file hash is giving it more
privileges than it originally had.
This requirement isn't different than how Certificate Revocation List
(CRL) work. Not any CA can revoke a certificate.
>
> But, if you think it would be appropriate, I could add a new Kconfig
> (disabled
> by default) that validates the key being used to vouch the S/MIME encoded
> hash is a CA. That would certainly make this more complicated. With this
> addition, would the key usage field need to be referenced too?
>
> Another idea I had was changing this patch to reference only the builtin and
> the machine keyring (if configured), not the secondary keyring. Then with
> INTEGRITY_CA_MACHINE_KEYRING_MAX, only CA keys could be
> used. Let me know your thoughts on this approach. Thanks.
Better, but it doesn't address the underlying problem.
--
thanks,
Mimi
Re: [PATCH] certs: Restrict blacklist updates to the secondary trusted keyring
On Mon, 2023-09-11 at 22:17 +, Eric Snowberg wrote:
>
> > On Sep 11, 2023, at 10:51 AM, Mickaël Salaün wrote:
> >
> > On Mon, Sep 11, 2023 at 09:29:07AM -0400, Mimi Zohar wrote:
> >> Hi Eric,
> >>
> >> On Fri, 2023-09-08 at 17:34 -0400, Eric Snowberg wrote:
> >>> Currently root can dynamically update the blacklist keyring if the hash
> >>> being added is signed and vouched for by the builtin trusted keyring.
> >>> Currently keys in the secondary trusted keyring can not be used.
> >>>
> >>> Keys within the secondary trusted keyring carry the same capabilities as
> >>> the builtin trusted keyring. Relax the current restriction for updating
> >>> the .blacklist keyring and allow the secondary to also be referenced as
> >>> a trust source. Since the machine keyring is linked to the secondary
> >>> trusted keyring, any key within it may also be used.
> >>>
> >>> An example use case for this is IMA appraisal. Now that IMA both
> >>> references the blacklist keyring and allows the machine owner to add
> >>> custom IMA CA certs via the machine keyring, this adds the additional
> >>> capability for the machine owner to also do revocations on a running
> >>> system.
> >>>
> >>> IMA appraisal usage example to add a revocation for /usr/foo:
> >>>
> >>> sha256sum /bin/foo | awk '{printf "bin:" $1}' > hash.txt
> >>>
> >>> openssl smime -sign -in hash.txt -inkey machine-private-key.pem \
> >>> -signer machine-certificate.pem -noattr -binary -outform DER \
> >>> -out hash.p7s
> >>>
> >>> keyctl padd blacklist "$(< hash.txt)" %:.blacklist < hash.p7s
> >>>
> >>> Signed-off-by: Eric Snowberg
> >>
> >> The secondary keyring may include both CA and code signing keys. With
> >> this change any key loaded onto the secondary keyring may blacklist a
> >> hash. Wouldn't it make more sense to limit blacklisting
> >> certificates/hashes to at least CA keys?
> >
> > Some operational constraints may limit what a CA can sign.
>
> Agreed.
>
> Is there precedents for requiring this S/MIME to be signed by a CA?
>
> > This change is critical and should be tied to a dedicated kernel config
> > (disabled by default), otherwise existing systems using this feature
> > will have their threat model automatically changed without notice.
>
> Today we have INTEGRITY_CA_MACHINE_KEYRING_MAX. This can
> be enabled to enforce CA restrictions on the machine keyring. Mimi, would
> this be a suitable solution for what you are after?
There needs to be some correlation between the file hashes being added
to the blacklist and the certificate that signed them. Without that
correlation, any key on the secondary trusted keyring could add any
file hashes it wants to the blacklist.
Mimi
>
> I suppose root could add another key to the secondary keyring if it was
> signed by a key in the machine keyring. But then we are getting into an
> area of key usage enforcement which really only exists for things added
> to the .ima keyring.
>
> >>> ---
> >>> certs/Kconfig | 2 +-
> >>> certs/blacklist.c | 4 ++--
> >>> 2 files changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/certs/Kconfig b/certs/Kconfig
> >>> index 1f109b070877..23dc87c52aff 100644
> >>> --- a/certs/Kconfig
> >>> +++ b/certs/Kconfig
> >>> @@ -134,7 +134,7 @@ config SYSTEM_BLACKLIST_AUTH_UPDATE
> >>> depends on SYSTEM_DATA_VERIFICATION
> >>> help
> >>> If set, provide the ability to load new blacklist keys at run time if
> >>> - they are signed and vouched by a certificate from the builtin trusted
> >>> + they are signed and vouched by a certificate from the secondary
> >>> trusted
> >>
> >> If CONFIG_SECONDARY_TRUSTED_KEYRING is not enabled, it falls back to
> >> the builtin keyring. Please update the comment accordingly.
>
> I’ll fix these in the next round, thanks.
>
Re: [PATCH] certs: Restrict blacklist updates to the secondary trusted keyring
Hi Eric,
On Fri, 2023-09-08 at 17:34 -0400, Eric Snowberg wrote:
> Currently root can dynamically update the blacklist keyring if the hash
> being added is signed and vouched for by the builtin trusted keyring.
> Currently keys in the secondary trusted keyring can not be used.
>
> Keys within the secondary trusted keyring carry the same capabilities as
> the builtin trusted keyring. Relax the current restriction for updating
> the .blacklist keyring and allow the secondary to also be referenced as
> a trust source. Since the machine keyring is linked to the secondary
> trusted keyring, any key within it may also be used.
>
> An example use case for this is IMA appraisal. Now that IMA both
> references the blacklist keyring and allows the machine owner to add
> custom IMA CA certs via the machine keyring, this adds the additional
> capability for the machine owner to also do revocations on a running
> system.
>
> IMA appraisal usage example to add a revocation for /usr/foo:
>
> sha256sum /bin/foo | awk '{printf "bin:" $1}' > hash.txt
>
> openssl smime -sign -in hash.txt -inkey machine-private-key.pem \
>-signer machine-certificate.pem -noattr -binary -outform DER \
>-out hash.p7s
>
> keyctl padd blacklist "$(< hash.txt)" %:.blacklist < hash.p7s
>
> Signed-off-by: Eric Snowberg
The secondary keyring may include both CA and code signing keys. With
this change any key loaded onto the secondary keyring may blacklist a
hash. Wouldn't it make more sense to limit blacklisting
certificates/hashes to at least CA keys?
> ---
> certs/Kconfig | 2 +-
> certs/blacklist.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/certs/Kconfig b/certs/Kconfig
> index 1f109b070877..23dc87c52aff 100644
> --- a/certs/Kconfig
> +++ b/certs/Kconfig
> @@ -134,7 +134,7 @@ config SYSTEM_BLACKLIST_AUTH_UPDATE
> depends on SYSTEM_DATA_VERIFICATION
> help
> If set, provide the ability to load new blacklist keys at run time if
> - they are signed and vouched by a certificate from the builtin trusted
> + they are signed and vouched by a certificate from the secondary
> trusted
If CONFIG_SECONDARY_TRUSTED_KEYRING is not enabled, it falls back to
the builtin keyring. Please update the comment accordingly.
> keyring. The PKCS#7 signature of the description is set in the key
> payload. Blacklist keys cannot be removed.
>
> diff --git a/certs/blacklist.c b/certs/blacklist.c
> index 675dd7a8f07a..0b346048ae2d 100644
> --- a/certs/blacklist.c
> +++ b/certs/blacklist.c
> @@ -102,12 +102,12 @@ static int blacklist_key_instantiate(struct key *key,
>
> #ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
> /*
> - * Verifies the description's PKCS#7 signature against the builtin
> + * Verifies the description's PKCS#7 signature against the secondary
>* trusted keyring.
>*/
And similarly here ...
> err = verify_pkcs7_signature(key->description,
> strlen(key->description), prep->data, prep->datalen,
> - NULL, VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
> + VERIFY_USE_SECONDARY_KEYRING,
> VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
> if (err)
> return err;
> #else
--
thanks,
Mimi
Re: [PATCH 010/141] ima: Fix fall-through warnings for Clang
Hi Gustavo, On Tue, 2021-04-20 at 15:28 -0500, Gustavo A. R. Silva wrote: > Hi all, > > Friendly ping: who can take this, please? Thank you for the reminder. > > On 11/20/20 12:25, Gustavo A. R. Silva wrote: > > In preparation to enable -Wimplicit-fallthrough for Clang, fix multiple > > warnings by explicitly adding multiple break statements instead of just > > letting the code fall through to the next case. > > > > Link: https://github.com/KSPP/linux/issues/115 > > Signed-off-by: Gustavo A. R. Silva Applied to git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity thanks, Mimi
Re: [PATCH v2] integrity: Add declarations to init_once void arguments.
Hi Jiele,
On Wed, 2021-04-07 at 01:44 +, Jiele Zhao wrote:
> init_once is a callback to kmem_cache_create. The parameter
> type of this function is void *, so it's better to give a
> explicit cast here.
>
> Signed-off-by: Jiele Zhao
> ---
> security/integrity/iint.c | 2 +-
> security/integrity/ima/ima_main.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 0ba01847e836..fca8a9409e4a 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -160,7 +160,7 @@ void integrity_inode_free(struct inode *inode)
>
> static void init_once(void *foo)
> {
> - struct integrity_iint_cache *iint = foo;
> + struct integrity_iint_cache *iint = (struct integrity_iint_cache *) foo;
>
> memset(iint, 0, sizeof(*iint));
> iint->ima_file_status = INTEGRITY_UNKNOWN;
> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index 9ef748ea829f..03bef720ab44 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -482,7 +482,7 @@ int ima_bprm_check(struct linux_binprm *bprm)
> }
>
> /**
> - * ima_path_check - based on policy, collect/store measurement.
> + * ima_file_check - based on policy, collect/store measurement.
> * @file: pointer to the file to be measured
> * @mask: contains MAY_READ, MAY_WRITE, MAY_EXEC or MAY_APPEND
> *
This change was already posted as "ima: Fix function name error in
comment". I've dropped this hunk. In the future, please review your
patch line by line before posting.
Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git
next-integrity
thanks,
Mimi
Re: [PATCH v2] ima: Fix function name error in comment.
On Tue, 2021-04-06 at 02:12 +, Jiele Zhao wrote:
> The original function name was ima_path_check(). The policy parsing
> still supports PATH_CHECK. Commit 9bbb6cad0173 ("ima: rename
> ima_path_check to ima_file_check") renamed the function to
> ima_file_check(), but missed modifying the function name in the
> comment.
>
> Fixes: 9bbb6cad0173 ("ima: rename ima_path_check to ima_file_check").
>
> Signed-off-by: Jiele Zhao
Thanks, Jiele.
Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git
next-integrity
Mimi
Re: [PATCH v4 0/3] ima: kernel build support for loading the kernel module signing key
On Fri, 2021-04-09 at 10:35 -0400, Nayna Jain wrote:
> Kernel modules are currently only signed when CONFIG_MODULE_SIG is enabled.
> The kernel module signing key is a self-signed CA only loaded onto the
> .builtin_trusted_key keyring. On secure boot enabled systems with an arch
> specific IMA policy enabled, but without MODULE_SIG enabled, kernel modules
> are not signed, nor is the kernel module signing public key loaded onto the
> IMA keyring.
>
> In order to load the the kernel module signing key onto the IMA trusted
> keyring ('.ima'), the certificate needs to be signed by a CA key either on
> the builtin or secondary keyrings. The original version of this patch set
> created and loaded a kernel-CA key onto the builtin keyring. The kernel-CA
> key signed the kernel module signing key, allowing it to be loaded onto the
> IMA trusted keyring.
>
> However, missing from this version was support for the kernel-CA to sign the
> hardware token certificate. Adding that support would add additional
> complexity.
>
> Since the kernel module signing key is embedded into the Linux kernel at
> build time, instead of creating and loading a kernel-CA onto the builtin
> trusted keyring, this version makes an exception and allows the
> self-signed kernel module signing key to be loaded directly onto the
> trusted IMA keyring.
Thanks, Nayna.
Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git
next-integrity
Mimi
Re: [PATCH v2 1/2] certs: Trigger creation of RSA module signing key if it's not an RSA key
On Thu, 2021-04-08 at 15:19 -0400, Stefan Berger wrote:
> On 4/8/21 1:15 PM, Mimi Zohar wrote:
> > On Thu, 2021-04-08 at 11:24 -0400, Stefan Berger wrote:
> >> Address a kbuild issue where a developer created an ECDSA key for signing
> >> kernel modules and then builds an older version of the kernel, when bi-
> >> secting the kernel for example, that does not support ECDSA keys.
> >>
> >> Trigger the creation of an RSA module signing key if it is not an RSA key.
> >>
> >> Fixes: cfc411e7fff3 ("Move certificate handling to its own directory")
> >> Signed-off-by: Stefan Berger
> > Thanks, Stefan.
> >
> > Reviewed-by: Mimi Zohar
> >
>
> Via which tree will this go upstream? keyrings?
This patch set originally had a dependency on Nayna's v1 & v2 "ima:
kernel build support for loading the kernel module signing key" patch
set and on Herbert's "ecc" branch. With v3, the dependency on Nayna's
patch set is gone.
Jarkko, David, Herbert did you want to pick up this patch set or would
you prefer that I did? Either way is fine.
thanks,
Mimi
Re: [PATCH v2 2/2] certs: Add support for using elliptic curve keys for signing modules
On Thu, 2021-04-08 at 11:24 -0400, Stefan Berger wrote: > Add support for using elliptic curve keys for signing modules. It uses > a NIST P384 (secp384r1) key if the user chooses an elliptic curve key > and will have ECDSA support built into the kernel. > > Note: A developer choosing an ECDSA key for signing modules should still > delete the signing key (rm certs/signing_key.*) when building an older > version of a kernel that only supports RSA keys. Unless kbuild automati- > cally detects and generates a new kernel module key, ECDSA-signed kernel > modules will fail signature verification. > > Signed-off-by: Stefan Berger Thanks, Stefan. Reviewed-by: Mimi Zohar
Re: [PATCH v2 1/2] certs: Trigger creation of RSA module signing key if it's not an RSA key
On Thu, 2021-04-08 at 11:24 -0400, Stefan Berger wrote:
> Address a kbuild issue where a developer created an ECDSA key for signing
> kernel modules and then builds an older version of the kernel, when bi-
> secting the kernel for example, that does not support ECDSA keys.
>
> Trigger the creation of an RSA module signing key if it is not an RSA key.
>
> Fixes: cfc411e7fff3 ("Move certificate handling to its own directory")
> Signed-off-by: Stefan Berger
Thanks, Stefan.
Reviewed-by: Mimi Zohar
Re: [PATCH 0/2] Add support for ECDSA-signed kernel modules
On Wed, 2021-04-07 at 18:53 +0300, Jarkko Sakkinen wrote: > On Tue, Apr 06, 2021 at 02:53:38PM -0400, Stefan Berger wrote: > > This series adds support for ECDSA-signed kernel modules. > > > > The first patch in this series attempts to address the issue where a > > developer created an ECDSA key for signing modules and then falls back > > to compiling an older version of the kernel that does not support > > ECDSA keys. In this case this patch would delete that ECDSA key if it is > > in certs/signing_key.pem and trigger the creation of an RSA key. However, > > for this to work this patch would have to be applied to previous versions > > of the kernel but would also only work for the developer if he/she used a > > stable version of the kernel to which this patch was applied. So whether > > this patch actually achieves the wanted effect is not always guaranteed. > > Just wondering why the key needs to be removed in the fallback. The main concern is with bisecting the kernel. Either elliptic curve support or the first patch needs to be backported. This patch will cause the kernel module signing key to be regenerated. Mimi
Re: [PATCH] integrity/ima: Add declarations to init_once void arguments.
Hi Jiele,
On Tue, 2021-03-23 at 01:33 +, Jiele Zhao wrote:
> init_once is a callback to kmem_cache_create. The parameter
> type of this function is void *, so it's better to give a
> explicit cast here.
>
> Signed-off-by: Jiele Zhao
Please remove the "ima" in the Subject line and re-post Cc'ing the
linux-integrity mailing list.
> ---
> security/integrity/iint.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 1d20003243c3..5f3f2de997e1 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -152,7 +152,7 @@ void integrity_inode_free(struct inode *inode)
>
> static void init_once(void *foo)
> {
> - struct integrity_iint_cache *iint = foo;
> + struct integrity_iint_cache *iint = (struct integrity_iint_cache *)foo;
Like the other init_once() examples, please add a blank before foo.
thanks,
Mimi
>
> memset(iint, 0, sizeof(*iint));
> iint->ima_file_status = INTEGRITY_UNKNOWN;
[GIT PULL] integrity subsystem fix for v5.12
Hi Linus, Here's just one patch to address a NULL ptr dereferencing when there is a mismatch between the user enabled LSMs and IMA/EVM. thanks, Mimi The following changes since commit 1e28eed17697bcf343c6743f0028cc3b5dd88bf0: Linux 5.12-rc3 (2021-03-14 14:41:02 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git tags/integrity-v5.12-fix for you to fetch changes up to 92063f3ca73aab794bd5408d3361fd5b5ea33079: integrity: double check iint_cache was initialized (2021-03-22 14:54:11 -0400) integrity-v5.12-fix Mimi Zohar (1): integrity: double check iint_cache was initialized security/integrity/iint.c | 8 1 file changed, 8 insertions(+)
Re: [PATCH] ima: Fix the error code for restoring the PCR value
On Wed, 2021-03-24 at 09:00 +, Roberto Sassu wrote:
> > From: lihuafei
> > Sent: Tuesday, March 23, 2021 2:41 PM
> > ping. :-)
> >
> > On 2021/3/3 11:28, Li Huafei wrote:
> > > In ima_restore_measurement_list(), hdr[HDR_PCR].data is pointing to a
> > > buffer of type u8, which contains the dumped 32-bit pcr value.
> > > Currently, only the least significant byte is used to restore the pcr
> > > value. We should convert hdr[HDR_PCR].data to a pointer of type u32
> > > before fetching the value to restore the correct pcr value.
> > >
> > > Fixes: 47fdee60b47f ("ima: use ima_parse_buf() to parse measurements
> > headers")
> > > Signed-off-by: Li Huafei
>
> Hi Li Huafei
>
> yes, correct. Thanks for the patch.
>
> Reviewed-by: Roberto Sassu
The patch set is now queued in next-integrity-testing.
thanks,
Mimi
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
On Wed, 2021-03-24 at 09:14 -0700, James Bottomley wrote:
> On Tue, 2021-03-23 at 14:07 -0400, Mimi Zohar wrote:
> > On Tue, 2021-03-23 at 17:35 +0100, Ahmad Fatoum wrote:
> > > Hello Horia,
> > >
> > > On 21.03.21 21:48, Horia Geantă wrote:
> > > > On 3/16/2021 7:02 PM, Ahmad Fatoum wrote:
> > > > [...]
> > > > > +struct trusted_key_ops caam_trusted_key_ops = {
> > > > > + .migratable = 0, /* non-migratable */
> > > > > + .init = trusted_caam_init,
> > > > > + .seal = trusted_caam_seal,
> > > > > + .unseal = trusted_caam_unseal,
> > > > > + .exit = trusted_caam_exit,
> > > > > +};
> > > > caam has random number generation capabilities, so it's worth
> > > > using that
> > > > by implementing .get_random.
> > >
> > > If the CAAM HWRNG is already seeding the kernel RNG, why not use
> > > the kernel's?
> > >
> > > Makes for less code duplication IMO.
> >
> > Using kernel RNG, in general, for trusted keys has been discussed
> > before. Please refer to Dave Safford's detailed explanation for not
> > using it [1].
> >
> > [1]
> > https://lore.kernel.org/linux-integrity/bca04d5d9a3b764c9b7405bba4d4a3c035f2a...@alpmbapa12.e2k.ad.ge.com/
>
> I still don't think relying on one source of randomness to be
> cryptographically secure is a good idea. The fear of bugs in the
> kernel entropy pool is reasonable, but since it's widely used they're
> unlikely to persist very long. Studies have shown that some TPMs
> (notably the chinese manufactured ones) have suspicious failures in
> their RNGs:
>
> https://www.researchgate.net/publication/45934562_Benchmarking_the_True_Random_Number_Generator_of_TPM_Chips
>
> And most cryptograhpers recommend using a TPM for entropy mixing rather
> than directly:
>
> https://blog.cryptographyengineering.com/category/rngs/
>
> The TPMFail paper also shows that in spite of NIST certification
> things can go wrong with a TPM:
>
> https://tpm.fail/
We already had a lengthy discussion on replacing the TPM RNG with the
kernel RNG for trusted keys, when TEE was being introduced [2,3]. I'm
not interested in re-hashing that discussion here. The only
difference now is that CAAM is a new trust source. I suspect the same
concerns/issues persist, but at least in this case using the kernel RNG
would not be a regression.
[2] Pascal Van Leeuwen on mixing different sources of entropy and certification
-
https://lore.kernel.org/linux-integrity/mn2pr20mb29732a856a40131a671f949fca...@mn2pr20mb2973.namprd20.prod.outlook.com/
[3] Jarrko on "regression" and tpm_asym.c -
https://lore.kernel.org/linux-integrity/20191014190033.ga15...@linux.intel.com/
Mimi
Re: [RFC PATCH 2/2] integrity: double check iint_cache was initialized
On Wed, 2021-03-24 at 12:58 +0100, Dmitry Vyukov wrote: > On Wed, Mar 24, 2021 at 12:49 PM Mimi Zohar wrote: > > > > On Wed, 2021-03-24 at 12:37 +0100, Dmitry Vyukov wrote: > > > On Wed, Mar 24, 2021 at 12:21 PM Tetsuo Handa > > > wrote: > > > > > > > > On 2021/03/24 20:10, Mimi Zohar wrote: > > > > > On Wed, 2021-03-24 at 19:10 +0900, Tetsuo Handa wrote: > > > > >> On 2021/03/24 1:13, Mimi Zohar wrote: > > > > >>> On Wed, 2021-03-24 at 00:14 +0900, Tetsuo Handa wrote: > > > > >>>> On 2021/03/23 23:47, Mimi Zohar wrote: > > > > >>>>> Initially I also questioned making "integrity" an LSM. Perhaps > > > > >>>>> it's > > > > >>>>> time to reconsider. For now, it makes sense to just fix the NULL > > > > >>>>> pointer dereferencing. > > > > >>>> > > > > >>>> Do we think calling panic() as "fix the NULL pointer > > > > >>>> dereferencing" ? > > > > >>> > > > > >>> Not supplying "integrity" as an "lsm=" option is a user error. > > > > >>> There > > > > >>> are only two options - allow or deny the caller to proceed. If the > > > > >>> user is expecting the integrity subsystem to be properly working, > > > > >>> returning a NULL and allowing the system to boot (RFC patch version) > > > > >>> does not make sense. Better to fail early. > > > > >> > > > > >> What does the "user" mean? Those who load the vmlinux? > > > > >> Only the "root" user (so called administrators)? > > > > >> Any users including other than "root" user? > > > > >> > > > > >> If the user means those who load the vmlinux, that user is > > > > >> explicitly asking > > > > >> for disabling "integrity" for some reason. In that case, it is a bug > > > > >> if > > > > >> booting with "integrity" disabled is impossible. > > > > >> > > > > >> If the user means something other than those who load the vmlinux, > > > > >> is there a possibility that that user (especially non "root" users) > > > > >> is > > > > >> allowed to try to use "integrity" ? If processes other than global > > > > >> init > > > > >> process can try to use "integrity", wouldn't it be a DoS attack > > > > >> vector? > > > > >> Please explain in the descripotion why calling panic() does not cause > > > > >> DoS attack vector. > > > > > > > > > > User in this case, is anyone rebooting the system and is intentionally > > > > > changing the default values, dropping the "integrity" option on the > > > > > boot command line. > > > > > > > > OK. Then, I expect that the system boots instead of calling panic(). > > > > That user is explicitly asking for disabling "integrity" for some > > > > reason. > > > > > > That was actually my intention. The prebuilt kernel that I use for > > > things has all LSMs enabled, but then I needed to try some workload > > > with only 1 specific LSM, so I gave a different lsm= argument. > > > > IMA/EVM is dependent on "integrity". Was your intention to also > > disable IMA and EVM? > > I think, yes... or not sure. I was trying to test a bug that requires > a different major LSM and all minor LSMs are presumably irrelevant. I > dropped existing lsm= arg and added something like lsm=apparmor. > > > If so, when disabling "integrity", don't load an > > IMA policy. > > I don't really know what this means. I guess it simply comes from the > image? If so, there was no easy way to avoid loading. There are a couple of builtin IMA policies, which may be loaded on boot by specifying on the boot command line "ima_policy=". Unless the boot command line "ima_policy=" option is specified, no policy will loaded. A custom IMA policy may subsequently be loaded, normally in the initramfs, by echo'ing the file pathname to /sys/kernel/security/ima/policy. Mimi
Re: [RFC PATCH 2/2] integrity: double check iint_cache was initialized
On Wed, 2021-03-24 at 12:37 +0100, Dmitry Vyukov wrote: > On Wed, Mar 24, 2021 at 12:21 PM Tetsuo Handa > wrote: > > > > On 2021/03/24 20:10, Mimi Zohar wrote: > > > On Wed, 2021-03-24 at 19:10 +0900, Tetsuo Handa wrote: > > >> On 2021/03/24 1:13, Mimi Zohar wrote: > > >>> On Wed, 2021-03-24 at 00:14 +0900, Tetsuo Handa wrote: > > >>>> On 2021/03/23 23:47, Mimi Zohar wrote: > > >>>>> Initially I also questioned making "integrity" an LSM. Perhaps it's > > >>>>> time to reconsider. For now, it makes sense to just fix the NULL > > >>>>> pointer dereferencing. > > >>>> > > >>>> Do we think calling panic() as "fix the NULL pointer dereferencing" ? > > >>> > > >>> Not supplying "integrity" as an "lsm=" option is a user error. There > > >>> are only two options - allow or deny the caller to proceed. If the > > >>> user is expecting the integrity subsystem to be properly working, > > >>> returning a NULL and allowing the system to boot (RFC patch version) > > >>> does not make sense. Better to fail early. > > >> > > >> What does the "user" mean? Those who load the vmlinux? > > >> Only the "root" user (so called administrators)? > > >> Any users including other than "root" user? > > >> > > >> If the user means those who load the vmlinux, that user is explicitly > > >> asking > > >> for disabling "integrity" for some reason. In that case, it is a bug if > > >> booting with "integrity" disabled is impossible. > > >> > > >> If the user means something other than those who load the vmlinux, > > >> is there a possibility that that user (especially non "root" users) is > > >> allowed to try to use "integrity" ? If processes other than global init > > >> process can try to use "integrity", wouldn't it be a DoS attack vector? > > >> Please explain in the descripotion why calling panic() does not cause > > >> DoS attack vector. > > > > > > User in this case, is anyone rebooting the system and is intentionally > > > changing the default values, dropping the "integrity" option on the > > > boot command line. > > > > OK. Then, I expect that the system boots instead of calling panic(). > > That user is explicitly asking for disabling "integrity" for some reason. > > That was actually my intention. The prebuilt kernel that I use for > things has all LSMs enabled, but then I needed to try some workload > with only 1 specific LSM, so I gave a different lsm= argument. IMA/EVM is dependent on "integrity". Was your intention to also disable IMA and EVM? If so, when disabling "integrity", don't load an IMA policy. Mimi
Re: [RFC PATCH 2/2] integrity: double check iint_cache was initialized
On Wed, 2021-03-24 at 19:10 +0900, Tetsuo Handa wrote: > On 2021/03/24 1:13, Mimi Zohar wrote: > > On Wed, 2021-03-24 at 00:14 +0900, Tetsuo Handa wrote: > >> On 2021/03/23 23:47, Mimi Zohar wrote: > >>> Initially I also questioned making "integrity" an LSM. Perhaps it's > >>> time to reconsider. For now, it makes sense to just fix the NULL > >>> pointer dereferencing. > >> > >> Do we think calling panic() as "fix the NULL pointer dereferencing" ? > > > > Not supplying "integrity" as an "lsm=" option is a user error. There > > are only two options - allow or deny the caller to proceed. If the > > user is expecting the integrity subsystem to be properly working, > > returning a NULL and allowing the system to boot (RFC patch version) > > does not make sense. Better to fail early. > > What does the "user" mean? Those who load the vmlinux? > Only the "root" user (so called administrators)? > Any users including other than "root" user? > > If the user means those who load the vmlinux, that user is explicitly asking > for disabling "integrity" for some reason. In that case, it is a bug if > booting with "integrity" disabled is impossible. > > If the user means something other than those who load the vmlinux, > is there a possibility that that user (especially non "root" users) is > allowed to try to use "integrity" ? If processes other than global init > process can try to use "integrity", wouldn't it be a DoS attack vector? > Please explain in the descripotion why calling panic() does not cause > DoS attack vector. User in this case, is anyone rebooting the system and is intentionally changing the default values, dropping the "integrity" option on the boot command line. Mimi
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
On Tue, 2021-03-23 at 17:35 +0100, Ahmad Fatoum wrote:
> Hello Horia,
>
> On 21.03.21 21:48, Horia Geantă wrote:
> > On 3/16/2021 7:02 PM, Ahmad Fatoum wrote:
> > [...]
> >> +struct trusted_key_ops caam_trusted_key_ops = {
> >> + .migratable = 0, /* non-migratable */
> >> + .init = trusted_caam_init,
> >> + .seal = trusted_caam_seal,
> >> + .unseal = trusted_caam_unseal,
> >> + .exit = trusted_caam_exit,
> >> +};
> > caam has random number generation capabilities, so it's worth using that
> > by implementing .get_random.
>
> If the CAAM HWRNG is already seeding the kernel RNG, why not use the kernel's?
>
> Makes for less code duplication IMO.
Using kernel RNG, in general, for trusted keys has been discussed
before. Please refer to Dave Safford's detailed explanation for not
using it [1].
thanks,
Mimi
[1]
https://lore.kernel.org/linux-integrity/bca04d5d9a3b764c9b7405bba4d4a3c035f2a...@alpmbapa12.e2k.ad.ge.com/
Re: [RFC PATCH 2/2] integrity: double check iint_cache was initialized
On Wed, 2021-03-24 at 00:14 +0900, Tetsuo Handa wrote: > On 2021/03/23 23:47, Mimi Zohar wrote: > > Initially I also questioned making "integrity" an LSM. Perhaps it's > > time to reconsider. For now, it makes sense to just fix the NULL > > pointer dereferencing. > > Do we think calling panic() as "fix the NULL pointer dereferencing" ? Not supplying "integrity" as an "lsm=" option is a user error. There are only two options - allow or deny the caller to proceed. If the user is expecting the integrity subsystem to be properly working, returning a NULL and allowing the system to boot (RFC patch version) does not make sense. Better to fail early. Mimi
Re: [RFC PATCH 2/2] integrity: double check iint_cache was initialized
On Tue, 2021-03-23 at 23:01 +0900, Tetsuo Handa wrote:
> On 2021/03/23 22:37, Tetsuo Handa wrote:
> > On 2021/03/23 21:09, Mimi Zohar wrote:
> >> Please take a look at the newer version of this patch. Do you want to
> >> add any tags?
> >
> > Oh, I didn't know that you already posted the newer version.
> >
> >> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> >> index 1d20003243c3..0ba01847e836 100644
> >> --- a/security/integrity/iint.c
> >> +++ b/security/integrity/iint.c
> >> @@ -98,6 +98,14 @@ struct integrity_iint_cache *integrity_inode_get(struct
> >> inode *inode)
> >>struct rb_node *node, *parent = NULL;
> >>struct integrity_iint_cache *iint, *test_iint;
> >>
> >> + /*
> >> + * The integrity's "iint_cache" is initialized at security_init(),
> >> + * unless it is not included in the ordered list of LSMs enabled
> >> + * on the boot command line.
> >> + */
> >> + if (!iint_cache)
> >> + panic("%s: lsm=integrity required.\n", __func__);
> >> +
> >
> > This looks strange. If "lsm=" parameter must include "integrity",
> > it implies that nobody is allowed to disable "integrity" at boot.
Integrity isn't always required. Only when something tries to use it,
does it need to be enabled. Since both integrity and the integrity
caller are runtime dependent, it is up to the user/admin to specify
"integrity" as an "lsm=" option.
> > Then, why not unconditionally call integrity_iintcache_init() by
> > not counting on DEFINE_LSM(integrity) declaration?
Initially I also questioned making "integrity" an LSM. Perhaps it's
time to reconsider. For now, it makes sense to just fix the NULL
pointer dereferencing.
Mimi
Re: [RFC PATCH 2/2] integrity: double check iint_cache was initialized
On Tue, 2021-03-23 at 10:46 +0900, Tetsuo Handa wrote:
> On 2021/03/20 5:03, Mimi Zohar wrote:
> > The integrity's "iint_cache" is initialized at security_init(). Only
> > after an IMA policy is loaded, which is initialized at late_initcall,
> > is a file's integrity status stored in the "iint_cache".
> >
> > All integrity_inode_get() callers first verify that the IMA policy has
> > been loaded, before calling it. Yet for some reason, it is still being
> > called, causing a NULL pointer dereference.
> >
> > qemu-system-x86_64 (...snipped...) lsm=smack (...snipped...)
>
> Hmm, why are you using lsm=smack instead of security=smack ?
> Since use of lsm= overrides
> CONFIG_LSM="lockdown,yama,safesetid,integrity,tomoyo,smack,bpf" settings,
> only smack is activated, which means that integrity_iintcache_init() will not
> be called by
>
> DEFINE_LSM(integrity) = {
> .name = "integrity",
> .init = integrity_iintcache_init,
> };
>
> declaration. That's the reason iint_cache == NULL when integrity_inode_get()
> is called.
That's exactly the problem, but since we don't control how the system
is configured or which parameters are supplied on the boot command
line, the kernel needs to at least provide some explanation instead of
dereferencing a NULL pointer.
FYI, "security=" is being deprecated. From Documentation/admin-
guide/kernel-parameters.txt:
security= [SECURITY] Choose a legacy "major" security module to
enable at boot. This has been deprecated by the
"lsm=" parameter.
Please take a look at the newer version of this patch. Do you want to
add any tags?
thanks,
Mimi
Re: [PATCH 2/2] integrity: double check iint_cache was initialized
On Mon, 2021-03-22 at 09:52 -0700, Eric Biggers wrote:
> On Mon, Mar 22, 2021 at 11:42:07AM -0400, Mimi Zohar wrote:
> >
> > Reported-by: Dmitry Vyukov
> > Fixes: 79f7865d844c ("LSM: Introduce "lsm=" for boottime LSM selection")
> > Signed-off-by: Mimi Zohar
>
> Missing Cc stable?
Yes, I was waiting for some comments/review/tags, before adding it.
Mimi
Re: [PATCH 1/2] ima: don't access a file's integrity status before an IMA policy is loaded
On Mon, 2021-03-22 at 09:51 -0700, Eric Biggers wrote: > On Mon, Mar 22, 2021 at 11:42:06AM -0400, Mimi Zohar wrote: > > Only after an IMA policy is loaded, check, save, or update the cached > > file's integrity status. > > > > Signed-off-by: Mimi Zohar > > This commit message doesn't describe what the actual effect of this change is. > Is it fixing something? No, it's just short circuiting out even earlier, but isn't needed. Mimi
[PATCH 1/2] ima: don't access a file's integrity status before an IMA policy is loaded
Only after an IMA policy is loaded, check, save, or update the cached file's integrity status. Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_main.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 9ef748ea829f..9d1196f712e1 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -606,6 +606,9 @@ void ima_post_create_tmpfile(struct user_namespace *mnt_userns, struct integrity_iint_cache *iint; int must_appraise; + if (!ima_policy_flag || !S_ISREG(inode->i_mode)) + return; + must_appraise = ima_must_appraise(mnt_userns, inode, MAY_ACCESS, FILE_CHECK); if (!must_appraise) @@ -636,6 +639,9 @@ void ima_post_path_mknod(struct user_namespace *mnt_userns, struct inode *inode = dentry->d_inode; int must_appraise; + if (!ima_policy_flag || !S_ISREG(inode->i_mode)) + return; + must_appraise = ima_must_appraise(mnt_userns, inode, MAY_ACCESS, FILE_CHECK); if (!must_appraise) -- 2.27.0
[PATCH 2/2] integrity: double check iint_cache was initialized
The kernel may be built with multiple LSMs, but only a subset may be
enabled on the boot command line by specifying "lsm=". Not including
"integrity" on the ordered LSM list may result in a NULL deref.
As reported by Dmitry Vyukov:
in qemu:
qemu-system-x86_64 -enable-kvm -machine q35,nvdimm -cpu
max,migratable=off -smp 4 -m 4G,slots=4,maxmem=16G-hda
wheezy.img -kernel arch/x86/boot/bzImage -nographic -vga std
-soundhw all -usb -usbdevice tablet -bt hci -bt device:keyboard
-net user,host=10.0.2.10,hostfwd=tcp::10022-:22 -net
nic,model=virtio-net-pci -object
memory-backend-file,id=pmem1,share=off,mem-path=/dev/zero,size=64M
-device nvdimm,id=nvdimm1,memdev=pmem1 -append "console=ttyS0
root=/dev/sda earlyprintk=serial rodata=n oops=panic panic_on_warn=1
panic=86400 lsm=smack numa=fake=2 nopcid dummy_hcd.num=8" -pidfile
vm_pid -m 2G -cpu host
But it crashes on NULL deref in integrity_inode_get during boot:
Run /sbin/init as init process
BUG: kernel NULL pointer dereference, address: 001c
PGD 0 P4D 0
Oops: [#1] PREEMPT SMP KASAN
CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc2+ #97
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
rel-1.13.0-44-g88ab0c15525c-prebuilt.qemu.org 04/01/2014
RIP: 0010:kmem_cache_alloc+0x2b/0x370 mm/slub.c:2920
Code: 57 41 56 41 55 41 54 41 89 f4 55 48 89 fd 53 48 83 ec 10 44 8b
3d d9 1f 90 0b 65 48 8b 04 25 28 00 00 00 48 89 44 24 08 31 c0 <8b> 5f
1c 4cf
RSP: :c932f9d8 EFLAGS: 00010246
RAX: RBX: 888017fc4f00 RCX:
RDX: 88804022 RSI: 0c40 RDI:
RBP: R08: R09: 888019263627
R10: 83937cd1 R11: R12: 0c40
R13: 888019263538 R14: R15: 00ff
FS: () GS:88802d18() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: 001c CR3: 0b48e000 CR4: 00750ee0
DR0: DR1: DR2:
DR3: DR6: fffe0ff0 DR7: 0400
PKRU: 5554
Call Trace:
integrity_inode_get+0x47/0x260 security/integrity/iint.c:105
process_measurement+0x33d/0x17e0 security/integrity/ima/ima_main.c:237
ima_bprm_check+0xde/0x210 security/integrity/ima/ima_main.c:474
security_bprm_check+0x7d/0xa0 security/security.c:845
search_binary_handler fs/exec.c:1708 [inline]
exec_binprm fs/exec.c:1761 [inline]
bprm_execve fs/exec.c:1830 [inline]
bprm_execve+0x764/0x19a0 fs/exec.c:1792
kernel_execve+0x370/0x460 fs/exec.c:1973
try_to_run_init_process+0x14/0x4e init/main.c:1366
kernel_init+0x11d/0x1b8 init/main.c:1477
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294
Modules linked in:
CR2: 001c
---[ end trace 22d601a500de7d79 ]---
Since LSMs and IMA may be configured at build time, but not enabled at
run time, panic the system if "integrity" was not initialized before use.
Reported-by: Dmitry Vyukov
Fixes: 79f7865d844c ("LSM: Introduce "lsm=" for boottime LSM selection")
Signed-off-by: Mimi Zohar
---
security/integrity/iint.c | 8
1 file changed, 8 insertions(+)
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 1d20003243c3..0ba01847e836 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -98,6 +98,14 @@ struct integrity_iint_cache *integrity_inode_get(struct
inode *inode)
struct rb_node *node, *parent = NULL;
struct integrity_iint_cache *iint, *test_iint;
+ /*
+* The integrity's "iint_cache" is initialized at security_init(),
+* unless it is not included in the ordered list of LSMs enabled
+* on the boot command line.
+*/
+ if (!iint_cache)
+ panic("%s: lsm=integrity required.\n", __func__);
+
iint = integrity_iint_find(inode);
if (iint)
return iint;
--
2.27.0
Re: NULL deref in integrity_inode_get
On Thu, 2021-03-18 at 07:53 +0100, Dmitry Vyukov wrote: > On Thu, Mar 18, 2021 at 3:18 AM Mimi Zohar wrote: > > > > Hi Dmitry, > > > > On Mon, 2021-03-15 at 14:07 +0100, Dmitry Vyukov wrote: > > > On Mon, Mar 15, 2021 at 1:41 PM Mimi Zohar wrote: > > > > > > > > On Mon, 2021-03-15 at 11:58 +0100, Dmitry Vyukov wrote: > > > > > Hi, > > > > > > > > > > I am trying to boot 5.12-rc3 with this config: > > > > > https://github.com/google/syzkaller/blob/cc1cff8f1e1a585894796d6eae8c51eef98037e6/dashboard/config/linux/upstream-smack-kasan.config > > > > > > > > > > in qemu: > > > > > qemu-system-x86_64 -enable-kvm -machine q35,nvdimm -cpu > > > > > max,migratable=off -smp 4 -m 4G,slots=4,maxmem=16G-hda > > > > > wheezy.img -kernel arch/x86/boot/bzImage -nographic -vga std > > > > > -soundhw all -usb -usbdevice tablet -bt hci -bt device:keyboard > > > > >-net user,host=10.0.2.10,hostfwd=tcp::10022-:22 -net > > > > > nic,model=virtio-net-pci -object > > > > > memory-backend-file,id=pmem1,share=off,mem-path=/dev/zero,size=64M > > > > > -device nvdimm,id=nvdimm1,memdev=pmem1 -append "console=ttyS0 > > > > > root=/dev/sda earlyprintk=serial rodata=n oops=panic panic_on_warn=1 > > > > > panic=86400 lsm=smack numa=fake=2 nopcid dummy_hcd.num=8" -pidfile > > > > > vm_pid -m 2G -cpu host > > > > > > > > > > But it crashes on NULL deref in integrity_inode_get during boot: > > > > > > > > > > Run /sbin/init as init process > > > > > BUG: kernel NULL pointer dereference, address: 001c > > > > > #PF: supervisor read access in kernel mode > > > > > #PF: error_code(0x) - not-present page > > > > > PGD 0 P4D 0 > > > > > Oops: [#1] PREEMPT SMP KASAN > > > > > CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc2+ #97 > > > > > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS > > > > > rel-1.13.0-44-g88ab0c15525c-prebuilt.qemu.org 04/01/2014 > > > > > RIP: 0010:kmem_cache_alloc+0x2b/0x370 mm/slub.c:2920 > > > > > Code: 57 41 56 41 55 41 54 41 89 f4 55 48 89 fd 53 48 83 ec 10 44 8b > > > > > 3d d9 1f 90 0b 65 48 8b 04 25 28 00 00 00 48 89 44 24 08 31 c0 <8b> 5f > > > > > 1c 4cf > > > > > RSP: :c932f9d8 EFLAGS: 00010246 > > > > > RAX: RBX: 888017fc4f00 RCX: > > > > > RDX: 88804022 RSI: 0c40 RDI: > > > > > RBP: R08: R09: 888019263627 > > > > > R10: 83937cd1 R11: R12: 0c40 > > > > > R13: 888019263538 R14: R15: 00ff > > > > > FS: () GS:88802d18() > > > > > knlGS: > > > > > CS: 0010 DS: ES: CR0: 80050033 > > > > > CR2: 001c CR3: 0b48e000 CR4: 00750ee0 > > > > > DR0: DR1: DR2: > > > > > DR3: DR6: fffe0ff0 DR7: 0400 > > > > > PKRU: 5554 > > > > > Call Trace: > > > > > integrity_inode_get+0x47/0x260 security/integrity/iint.c:105 > > > > > process_measurement+0x33d/0x17e0 > > > > > security/integrity/ima/ima_main.c:237 > > > > > ima_bprm_check+0xde/0x210 security/integrity/ima/ima_main.c:474 > > > > > security_bprm_check+0x7d/0xa0 security/security.c:845 > > > > > search_binary_handler fs/exec.c:1708 [inline] > > > > > exec_binprm fs/exec.c:1761 [inline] > > > > > bprm_execve fs/exec.c:1830 [inline] > > > > > bprm_execve+0x764/0x19a0 fs/exec.c:1792 > > > > > kernel_execve+0x370/0x460 fs/exec.c:1973 > > > > > try_to_run_init_process+0x14/0x4e init/main.c:1366 > > > > > kernel_init+0x11d/0x1b8 init/main.c:1477 > > > > > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294 > > > > > Modules linked in: > > > > > CR2: 001c > > > > > ---[ end trace 22d601a500de7d79 ]--- > > > > > > > > It looks like integrity_inode_get() fai
[RFC PATCH 2/2] integrity: double check iint_cache was initialized
From: Test
The integrity's "iint_cache" is initialized at security_init(). Only
after an IMA policy is loaded, which is initialized at late_initcall,
is a file's integrity status stored in the "iint_cache".
All integrity_inode_get() callers first verify that the IMA policy has
been loaded, before calling it. Yet for some reason, it is still being
called, causing a NULL pointer dereference.
As reported by Dmitry Vyukov:
in qemu:
qemu-system-x86_64 -enable-kvm -machine q35,nvdimm -cpu
max,migratable=off -smp 4 -m 4G,slots=4,maxmem=16G-hda
wheezy.img -kernel arch/x86/boot/bzImage -nographic -vga std
-soundhw all -usb -usbdevice tablet -bt hci -bt device:keyboard
-net user,host=10.0.2.10,hostfwd=tcp::10022-:22 -net
nic,model=virtio-net-pci -object
memory-backend-file,id=pmem1,share=off,mem-path=/dev/zero,size=64M
-device nvdimm,id=nvdimm1,memdev=pmem1 -append "console=ttyS0
root=/dev/sda earlyprintk=serial rodata=n oops=panic panic_on_warn=1
panic=86400 lsm=smack numa=fake=2 nopcid dummy_hcd.num=8" -pidfile
vm_pid -m 2G -cpu host
But it crashes on NULL deref in integrity_inode_get during boot:
Run /sbin/init as init process
BUG: kernel NULL pointer dereference, address: 001c
PGD 0 P4D 0
Oops: [#1] PREEMPT SMP KASAN
CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc2+ #97
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
rel-1.13.0-44-g88ab0c15525c-prebuilt.qemu.org 04/01/2014
RIP: 0010:kmem_cache_alloc+0x2b/0x370 mm/slub.c:2920
Code: 57 41 56 41 55 41 54 41 89 f4 55 48 89 fd 53 48 83 ec 10 44 8b
3d d9 1f 90 0b 65 48 8b 04 25 28 00 00 00 48 89 44 24 08 31 c0 <8b> 5f
1c 4cf
RSP: :c932f9d8 EFLAGS: 00010246
RAX: RBX: 888017fc4f00 RCX:
RDX: 88804022 RSI: 0c40 RDI:
RBP: R08: R09: 888019263627
R10: 83937cd1 R11: R12: 0c40
R13: 888019263538 R14: R15: 00ff
FS: () GS:88802d18() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: 001c CR3: 0b48e000 CR4: 00750ee0
DR0: DR1: DR2:
DR3: DR6: fffe0ff0 DR7: 0400
PKRU: 5554
Call Trace:
integrity_inode_get+0x47/0x260 security/integrity/iint.c:105
process_measurement+0x33d/0x17e0 security/integrity/ima/ima_main.c:237
ima_bprm_check+0xde/0x210 security/integrity/ima/ima_main.c:474
security_bprm_check+0x7d/0xa0 security/security.c:845
search_binary_handler fs/exec.c:1708 [inline]
exec_binprm fs/exec.c:1761 [inline]
bprm_execve fs/exec.c:1830 [inline]
bprm_execve+0x764/0x19a0 fs/exec.c:1792
kernel_execve+0x370/0x460 fs/exec.c:1973
try_to_run_init_process+0x14/0x4e init/main.c:1366
kernel_init+0x11d/0x1b8 init/main.c:1477
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294
Modules linked in:
CR2: 001c
---[ end trace 22d601a500de7d79 ]---
Before calling kmem_cache_alloc(), check that the iint_cache has
been initialized.
Reported-by: Dmitry Vyukov
Signed-off-by: Mimi Zohar
---
security/integrity/iint.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 1d20003243c3..80b5ae7bb712 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -97,6 +97,15 @@ struct integrity_iint_cache *integrity_inode_get(struct
inode *inode)
struct rb_node **p;
struct rb_node *node, *parent = NULL;
struct integrity_iint_cache *iint, *test_iint;
+ static int once = 0;
+
+ if (!iint_cache) { /* shouldn't get here */
+ if (!once) {
+ dump_stack();
+ once = 1;
+ }
+ return NULL;
+ }
iint = integrity_iint_find(inode);
if (iint)
--
2.27.0
[RFC PATCH 1/2] ima: don't access a file's integrity status before an IMA policy is loaded
From: Test Only after an IMA policy is loaded, check, save, or update the cached file's integrity status. Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_main.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 9ef748ea829f..9d1196f712e1 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -606,6 +606,9 @@ void ima_post_create_tmpfile(struct user_namespace *mnt_userns, struct integrity_iint_cache *iint; int must_appraise; + if (!ima_policy_flag || !S_ISREG(inode->i_mode)) + return; + must_appraise = ima_must_appraise(mnt_userns, inode, MAY_ACCESS, FILE_CHECK); if (!must_appraise) @@ -636,6 +639,9 @@ void ima_post_path_mknod(struct user_namespace *mnt_userns, struct inode *inode = dentry->d_inode; int must_appraise; + if (!ima_policy_flag || !S_ISREG(inode->i_mode)) + return; + must_appraise = ima_must_appraise(mnt_userns, inode, MAY_ACCESS, FILE_CHECK); if (!must_appraise) -- 2.27.0
Re: NULL deref in integrity_inode_get
Hi Dmitry, On Mon, 2021-03-15 at 14:07 +0100, Dmitry Vyukov wrote: > On Mon, Mar 15, 2021 at 1:41 PM Mimi Zohar wrote: > > > > On Mon, 2021-03-15 at 11:58 +0100, Dmitry Vyukov wrote: > > > Hi, > > > > > > I am trying to boot 5.12-rc3 with this config: > > > https://github.com/google/syzkaller/blob/cc1cff8f1e1a585894796d6eae8c51eef98037e6/dashboard/config/linux/upstream-smack-kasan.config > > > > > > in qemu: > > > qemu-system-x86_64 -enable-kvm -machine q35,nvdimm -cpu > > > max,migratable=off -smp 4 -m 4G,slots=4,maxmem=16G-hda > > > wheezy.img -kernel arch/x86/boot/bzImage -nographic -vga std > > > -soundhw all -usb -usbdevice tablet -bt hci -bt device:keyboard > > >-net user,host=10.0.2.10,hostfwd=tcp::10022-:22 -net > > > nic,model=virtio-net-pci -object > > > memory-backend-file,id=pmem1,share=off,mem-path=/dev/zero,size=64M > > > -device nvdimm,id=nvdimm1,memdev=pmem1 -append "console=ttyS0 > > > root=/dev/sda earlyprintk=serial rodata=n oops=panic panic_on_warn=1 > > > panic=86400 lsm=smack numa=fake=2 nopcid dummy_hcd.num=8" -pidfile > > > vm_pid -m 2G -cpu host > > > > > > But it crashes on NULL deref in integrity_inode_get during boot: > > > > > > Run /sbin/init as init process > > > BUG: kernel NULL pointer dereference, address: 001c > > > #PF: supervisor read access in kernel mode > > > #PF: error_code(0x) - not-present page > > > PGD 0 P4D 0 > > > Oops: [#1] PREEMPT SMP KASAN > > > CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc2+ #97 > > > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS > > > rel-1.13.0-44-g88ab0c15525c-prebuilt.qemu.org 04/01/2014 > > > RIP: 0010:kmem_cache_alloc+0x2b/0x370 mm/slub.c:2920 > > > Code: 57 41 56 41 55 41 54 41 89 f4 55 48 89 fd 53 48 83 ec 10 44 8b > > > 3d d9 1f 90 0b 65 48 8b 04 25 28 00 00 00 48 89 44 24 08 31 c0 <8b> 5f > > > 1c 4cf > > > RSP: :c932f9d8 EFLAGS: 00010246 > > > RAX: RBX: 888017fc4f00 RCX: > > > RDX: 88804022 RSI: 0c40 RDI: > > > RBP: R08: R09: 888019263627 > > > R10: 83937cd1 R11: R12: 0c40 > > > R13: 888019263538 R14: R15: 00ff > > > FS: () GS:88802d18() > > > knlGS: > > > CS: 0010 DS: ES: CR0: 80050033 > > > CR2: 001c CR3: 0b48e000 CR4: 00750ee0 > > > DR0: DR1: DR2: > > > DR3: DR6: fffe0ff0 DR7: 0400 > > > PKRU: 5554 > > > Call Trace: > > > integrity_inode_get+0x47/0x260 security/integrity/iint.c:105 > > > process_measurement+0x33d/0x17e0 security/integrity/ima/ima_main.c:237 > > > ima_bprm_check+0xde/0x210 security/integrity/ima/ima_main.c:474 > > > security_bprm_check+0x7d/0xa0 security/security.c:845 > > > search_binary_handler fs/exec.c:1708 [inline] > > > exec_binprm fs/exec.c:1761 [inline] > > > bprm_execve fs/exec.c:1830 [inline] > > > bprm_execve+0x764/0x19a0 fs/exec.c:1792 > > > kernel_execve+0x370/0x460 fs/exec.c:1973 > > > try_to_run_init_process+0x14/0x4e init/main.c:1366 > > > kernel_init+0x11d/0x1b8 init/main.c:1477 > > > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294 > > > Modules linked in: > > > CR2: 001c > > > ---[ end trace 22d601a500de7d79 ]--- > > > > It looks like integrity_inode_get() fails to alloc memory. Only on > > failure to verify the integrity of a file would an error be returned. > > I think that is what you would want to happen. Without an "appraise" > > policy, this shouldn't happen. > > It happens at the very boot. I think the cache is NULL. An IMA policy had to have been loaded in order for integrity_inode_get() to have been called. If this is happening on boot, it's too early for a custom policy to have been loaded by userspace, but I don't see the builtin policy defined on the boot command line either. Any additional information would be much appreciated. thanks, Mimi
Re: NULL deref in integrity_inode_get
Hi Dmitry, On Mon, 2021-03-15 at 11:58 +0100, Dmitry Vyukov wrote: > Hi, > > I am trying to boot 5.12-rc3 with this config: > https://github.com/google/syzkaller/blob/cc1cff8f1e1a585894796d6eae8c51eef98037e6/dashboard/config/linux/upstream-smack-kasan.config > > in qemu: > qemu-system-x86_64 -enable-kvm -machine q35,nvdimm -cpu > max,migratable=off -smp 4 -m 4G,slots=4,maxmem=16G-hda > wheezy.img -kernel arch/x86/boot/bzImage -nographic -vga std > -soundhw all -usb -usbdevice tablet -bt hci -bt device:keyboard >-net user,host=10.0.2.10,hostfwd=tcp::10022-:22 -net > nic,model=virtio-net-pci -object > memory-backend-file,id=pmem1,share=off,mem-path=/dev/zero,size=64M > -device nvdimm,id=nvdimm1,memdev=pmem1 -append "console=ttyS0 > root=/dev/sda earlyprintk=serial rodata=n oops=panic panic_on_warn=1 > panic=86400 lsm=smack numa=fake=2 nopcid dummy_hcd.num=8" -pidfile > vm_pid -m 2G -cpu host > > But it crashes on NULL deref in integrity_inode_get during boot: > > Run /sbin/init as init process > BUG: kernel NULL pointer dereference, address: 001c > #PF: supervisor read access in kernel mode > #PF: error_code(0x) - not-present page > PGD 0 P4D 0 > Oops: [#1] PREEMPT SMP KASAN > CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc2+ #97 > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS > rel-1.13.0-44-g88ab0c15525c-prebuilt.qemu.org 04/01/2014 > RIP: 0010:kmem_cache_alloc+0x2b/0x370 mm/slub.c:2920 > Code: 57 41 56 41 55 41 54 41 89 f4 55 48 89 fd 53 48 83 ec 10 44 8b > 3d d9 1f 90 0b 65 48 8b 04 25 28 00 00 00 48 89 44 24 08 31 c0 <8b> 5f > 1c 4cf > RSP: :c932f9d8 EFLAGS: 00010246 > RAX: RBX: 888017fc4f00 RCX: > RDX: 88804022 RSI: 0c40 RDI: > RBP: R08: R09: 888019263627 > R10: 83937cd1 R11: R12: 0c40 > R13: 888019263538 R14: R15: 00ff > FS: () GS:88802d18() knlGS: > CS: 0010 DS: ES: CR0: 80050033 > CR2: 001c CR3: 0b48e000 CR4: 00750ee0 > DR0: DR1: DR2: > DR3: DR6: fffe0ff0 DR7: 0400 > PKRU: 5554 > Call Trace: > integrity_inode_get+0x47/0x260 security/integrity/iint.c:105 > process_measurement+0x33d/0x17e0 security/integrity/ima/ima_main.c:237 > ima_bprm_check+0xde/0x210 security/integrity/ima/ima_main.c:474 > security_bprm_check+0x7d/0xa0 security/security.c:845 > search_binary_handler fs/exec.c:1708 [inline] > exec_binprm fs/exec.c:1761 [inline] > bprm_execve fs/exec.c:1830 [inline] > bprm_execve+0x764/0x19a0 fs/exec.c:1792 > kernel_execve+0x370/0x460 fs/exec.c:1973 > try_to_run_init_process+0x14/0x4e init/main.c:1366 > kernel_init+0x11d/0x1b8 init/main.c:1477 > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294 > Modules linked in: > CR2: 001c > ---[ end trace 22d601a500de7d79 ]--- It looks like integrity_inode_get() fails to alloc memory. Only on failure to verify the integrity of a file would an error be returned. I think that is what you would want to happen. Without an "appraise" policy, this shouldn't happen. Mimi
Re: [PATCH 06/20] ima: Manual replacement of the deprecated strlcpy() with return values
On Mon, 2021-02-22 at 16:12 +0100, Romain Perier wrote:
> The strlcpy() reads the entire source buffer first, it is dangerous if
> the source buffer lenght is unbounded or possibility non NULL-terminated.
As other's have pointed out, "lenght" -> length.
> It can lead to linear read overflows, crashes, etc...
>
> As recommended in the deprecated interfaces [1], it should be replaced
> by strscpy.
>
> This commit replaces all calls to strlcpy that handle the return values
> by the corresponding strscpy calls with new handling of the return
> values (as it is quite different between the two functions).
>
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
>
> Signed-off-by: Romain Perier
> ---
> security/integrity/ima/ima_policy.c |8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c
> index 9b45d064a87d..1a905b8b064f 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -790,8 +790,14 @@ static int __init ima_init_arch_policy(void)
> for (rules = arch_rules, i = 0; *rules != NULL; rules++) {
> char rule[255];
> int result;
> + ssize_t len;
>
> - result = strlcpy(rule, *rules, sizeof(rule));
> + len = strscpy(rule, *rules, sizeof(rule));
> + if (len == -E2BIG) {
> + pr_warn("Internal copy of architecture policy rule '%s'
> "
> + "failed. Skipping.\n", *rules);
"arch_rules" is an array of hard coded strings. The generic reason
for replacing strlcpy with strscpy doesn't seem applicable; however,
the additonal warning is appropriate.
(User-visible strings are not bound to the 80 column length. Breaking
up the line like this is fine, but unnecessary.)
Acked-by: Mimi Zohar
thanks,
Mimi
> + continue;
> + }
>
> INIT_LIST_HEAD(_policy_entry[i].list);
> result = ima_parse_rule(rule, _policy_entry[i]);
>
Re: [PATCH v3 02/11] evm: Load EVM key in ima_load_x509() to avoid appraisal
Hi Roberto,
On Wed, 2020-11-11 at 10:22 +0100, Roberto Sassu wrote:
> Public keys do not need to be appraised by IMA as the restriction on the
> IMA/EVM keyrings ensures that a key can be loaded only if it is signed with
> a key in the primary or secondary keyring.
>
> However, when evm_load_x509() is called, appraisal is already enabled and
> a valid IMA signature must be added to the EVM key to pass verification.
>
> Since the restriction is applied on both IMA and EVM keyrings, it is safe
> to disable appraisal also when the EVM key is loaded. This patch calls
> evm_load_x509() inside ima_load_x509() if CONFIG_IMA_LOAD_X509 is defined.
>
> Signed-off-by: Roberto Sassu
> Reviewed-by: Mimi Zohar
> ---
> security/integrity/iint.c | 2 ++
> security/integrity/ima/ima_init.c | 4
> 2 files changed, 6 insertions(+)
>
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 1d20003243c3..7d08c31c612f 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -200,7 +200,9 @@ int integrity_kernel_read(struct file *file, loff_t
> offset,
> void __init integrity_load_keys(void)
> {
> ima_load_x509();
> +#ifndef CONFIG_IMA_LOAD_X509
> evm_load_x509();
> +#endif
Please replace the ifdef with the IS_ENABLED() equivalent.
thanks,
Mimi
Re: [PATCH v9 9/9] certs: Add support for using elliptic curve keys for signing modules
On Sat, 2021-02-27 at 11:35 +0800, yumeng wrote: > 在 2021/2/26 0:08, Stefan Berger 写道: > > From: Stefan Berger > > > > > diff --git a/certs/Makefile b/certs/Makefile > > index 3fe6b73786fa..c487d7021c54 100644 > > --- a/certs/Makefile > > +++ b/certs/Makefile > > @@ -69,6 +69,18 @@ else > > SIGNER = -signkey $(obj)/signing_key.key > > endif # CONFIG_IMA_APPRAISE_MODSIG > > > > Is there anything wrong in this patch? > I can't apply it when I use 'git am '. > errors like below: > > error: certs/Kconfig: does not match index > error: patch failed: certs/Makefile:69 > error: certs/Makefile: patch does not apply > > Thanks Nothing wrong with the patch, just a dependency. From the Change log: - This patch builds on top Nayna's series for 'kernel build support for loading the kernel module signing key'. - https://lkml.org/lkml/2021/2/18/856 thanks, Mimi
Re: [PATCH v24 04/25] IMA: avoid label collisions with stacked LSMs
On Mon, 2021-02-22 at 15:45 -0800, Casey Schaufler wrote:
> On 2/14/2021 10:21 AM, Mimi Zohar wrote:
>
> Would these changes match your suggestion?
>
> security/integrity/ima/ima_policy.c | 24
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c
> index 9ac673472781..e80956548243 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -78,11 +78,11 @@ struct ima_rule_entry {
> bool (*uid_op)(kuid_t, kuid_t);/* Handlers for operators */
> bool (*fowner_op)(kuid_t, kuid_t); /* uid_eq(), uid_gt(), uid_lt() */
> int pcr;
> + int which_lsm; /* which of the rules to use */
> struct {
> void *rules[LSMBLOB_ENTRIES]; /* LSM file metadata specific */
If each IMA policy rule may only contain a single LSM specific
LSM_OBJ_{USER | ROLE | TYPE} and LSM_SUBJ_{USER | ROLE | TYPE}, then
there is no need for rules[LSMBLOB_ENTRIES]. Leave it as "*rule".
Otherwise it looks good.
Mimi
> char *args_p; /* audit value */
> int type; /* audit type */
> - int which_lsm; /* which of the rules to use */
> } lsm[MAX_LSM_RULES];
> char *fsname;
> struct ima_rule_opt_list *keyrings; /* Measure keys added to these
> keyrings */
Re: [PATCH v2] IMA: support for duplicate data measurement
On Thu, 2021-02-18 at 14:05 -0800, Tushar Sugandhi wrote: > On 2021-02-17 12:49 p.m., Tushar Sugandhi wrote: > > On 2021-02-17 12:39 p.m., Mimi Zohar wrote: > >> On Wed, 2021-02-17 at 10:53 -0800, Tushar Sugandhi wrote: > >>> Thanks for the feedback Mimi. > >>> Appreciate it. > >>> > >>> On 2021-02-17 7:03 a.m., Mimi Zohar wrote: > >>>> Hi Tushar, > >>>> > >>>> The Subject line could be improved. Perhaps something like - "IMA: > >>>> support for duplicate measurement records" > >>>> > >>> Will do. > >>> > >>>> On Tue, 2021-02-16 at 18:46 -0800, Tushar Sugandhi wrote: > >>>>> IMA does not measure duplicate data since TPM extend is a very > >>>>> expensive > >>>>> operation. However, in some cases, the measurement of duplicate data > >>>>> is necessary to accurately determine the current state of the system. > >>>>> Eg, SELinux state changing from 'audit', to 'enforcing', and back to > >>>>> 'audit' again. In this example, currently, IMA will not measure the > >>>>> last state change to 'audit'. This limits the ability of attestation > >>>>> services to accurately determine the current state of the measurements > >>>>> on the system. > >>>> > >>>> This patch description is written from your specific usecase > >>>> perspective, but it impacts file and buffer data measurements as well, > >>>> not only critical data measurements. In all of these situations, with > >>>> this patch a new measurement record is added/appended to the > >>>> measurement list. Please re-write the patch description making it more > >>>> generic. > >>>> > >>>> For example, I would start with something like, "IMA does not include > >>>> duplicate file, buffer or critical data measurement records ..." > >>>> > >>> Agreed. > >>> I will generalize the description further and send the v3 for review. > >> > >> It would be good to boot with the ima_policy=tcb policy with/without > >> your patch and account for the different number of measurements. Are > >> all the differences related to duplicate measurements - original file > >> hash -> new file hash -> original file hash - similar to what you > >> described. > >> > > Thanks for the ima_policy=tcb pointer. > > > > I tested my patch with: > > - duplicate buffer content for "measure func=CRITICAL_DATA" > > - and reading the same file twice with "measure func=FILE_CHECK > > mask=MAY_READ" > > > > In both the above use cases, IMA is measuring the duplicate entries with > > the patch, and not measuring the duplicate entries w/o the patch. > > > > I will test the "ima_policy=tcb" boot-scenario as you suggested, before > > posting the next version. > > > > I booted the system with "ima_policy=tcb" policy with/without my patch. > I also removed /etc/ima/ima-policy for testing these use-cases. > (so that it wouldn't override the policy generated by boot param > "ima_policy=tcb"). > > I double checked the contents of the kernel policy: > #cat /sys/kernel/security/integrity/ima/policy > dont_measure fsmagic=0x9fa0 > dont_measure fsmagic=0x62656572 > dont_measure fsmagic=0x64626720 > dont_measure fsmagic=0x1021994 > dont_measure fsmagic=0x1cd1 > dont_measure fsmagic=0x42494e4d > dont_measure fsmagic=0x73636673 > dont_measure fsmagic=0xf97cff8c > dont_measure fsmagic=0x43415d53 > dont_measure fsmagic=0x27e0eb > dont_measure fsmagic=0x63677270 > dont_measure fsmagic=0x6e736673 > dont_measure fsmagic=0xde5e81e4 > measure func=MMAP_CHECK mask=MAY_EXEC > measure func=BPRM_CHECK mask=MAY_EXEC > measure func=FILE_CHECK mask=^MAY_READ euid=0 > measure func=FILE_CHECK mask=^MAY_READ uid=0 > measure func=MODULE_CHECK > measure func=FIRMWARE_CHECK > measure func=POLICY_CHECK > > And then I compared the contents of the ascii_runtime_measurements with > and without my patch. > > And here are my findings: > > (1) Files like systemd-udevd, x2go_sessions etc. get measured multiple > times with the CONFIG_IMA_DISABLE_HTABLE=y. > They only get measured once with the config "=n". > > 10 668df8723f5a1f57a0afe3b50d44054d66363
Re: [PATCH] of: error: 'const struct kimage' has no member named 'arch'
On Fri, 2021-02-19 at 11:43 -0600, Rob Herring wrote:
> On Fri, Feb 19, 2021 at 10:57 AM Lakshmi Ramasubramanian
> wrote:
> >
> > On 2/19/21 6:16 AM, Rob Herring wrote:
> > > On Thu, Feb 18, 2021 at 8:53 PM Lakshmi Ramasubramanian
> > > wrote:
> > >>
> > >> On 2/18/21 5:13 PM, Thiago Jung Bauermann wrote:
> > >>>
> > >>> Lakshmi Ramasubramanian writes:
> > >>>
> > >>>> On 2/18/21 4:07 PM, Mimi Zohar wrote:
> > >>>>
> > >>>> Hi Mimi,
> > >>>>
> > >>>>> On Thu, 2021-02-18 at 14:33 -0800, Lakshmi Ramasubramanian wrote:
> > >>>>>> of_kexec_alloc_and_setup_fdt() defined in drivers/of/kexec.c builds
> > >>>>>> a new device tree object that includes architecture specific data
> > >>>>>> for kexec system call. This should be defined only if the
> > >>>>>> architecture
> > >>>>>> being built defines kexec architecture structure "struct
> > >>>>>> kimage_arch".
> > >>>>>>
> > >>>>>> Define a new boolean config OF_KEXEC that is enabled if
> > >>>>>> CONFIG_KEXEC_FILE and CONFIG_OF_FLATTREE are enabled, and
> > >>>>>> the architecture is arm64 or powerpc64. Build drivers/of/kexec.c
> > >>>>>> if CONFIG_OF_KEXEC is enabled.
> > >>>>>>
> > >>>>>> Signed-off-by: Lakshmi Ramasubramanian
> > >>>>>> Fixes: 33488dc4d61f ("of: Add a common kexec FDT setup function")
> > >>>>>> Reported-by: kernel test robot
> > >>>>>> ---
> > >>>>>> drivers/of/Kconfig | 6 ++
> > >>>>>> drivers/of/Makefile | 7 +--
> > >>>>>> 2 files changed, 7 insertions(+), 6 deletions(-)
> > >>>>>>
> > >>>>>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> > >>>>>> index 18450437d5d5..f2e8fa54862a 100644
> > >>>>>> --- a/drivers/of/Kconfig
> > >>>>>> +++ b/drivers/of/Kconfig
> > >>>>>> @@ -100,4 +100,10 @@ config OF_DMA_DEFAULT_COHERENT
> > >>>>>> # arches should select this if DMA is coherent by
> > >>>>>> default for OF devices
> > >>>>>> bool
> > >>>>>> +config OF_KEXEC
> > >>>>>> + bool
> > >>>>>> + depends on KEXEC_FILE
> > >>>>>> + depends on OF_FLATTREE
> > >>>>>> + default y if ARM64 || PPC64
> > >>>>>> +
> > >>>>>> endif # OF
> > >>>>>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> > >>>>>> index c13b982084a3..287579dd1695 100644
> > >>>>>> --- a/drivers/of/Makefile
> > >>>>>> +++ b/drivers/of/Makefile
> > >>>>>> @@ -13,11 +13,6 @@ obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
> > >>>>>> obj-$(CONFIG_OF_RESOLVE) += resolver.o
> > >>>>>> obj-$(CONFIG_OF_OVERLAY) += overlay.o
> > >>>>>> obj-$(CONFIG_OF_NUMA) += of_numa.o
> > >>>>>> -
> > >>>>>> -ifdef CONFIG_KEXEC_FILE
> > >>>>>> -ifdef CONFIG_OF_FLATTREE
> > >>>>>> -obj-y += kexec.o
> > >>>>>> -endif
> > >>>>>> -endif
> > >>>>>> +obj-$(CONFIG_OF_KEXEC) += kexec.o
> > >>>>>> obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> > >>>>> Is it possible to reuse CONFIG_HAVE_IMA_KEXEC here?
> > >>>>>
> > >>>>
> > >>>> For ppc64 CONFIG_HAVE_IMA_KEXEC is selected when CONFIG_KEXEC_FILE is
> > >>>> enabled.
> > >>>> So I don't see a problem in reusing CONFIG_HAVE_IMA_KEXEC for ppc.
> > >>>>
> > >>>> But for arm64, CONFIG_HAVE_IMA_KEXEC is enabled in the final patch in
> > >>>> the patch
> > >>>> set (the one for carrying forward IMA log across kexec for arm64).
> > >>>> arm64 calls
> > >>>> of_kexec_alloc_and_setup_fdt() prior to enabling CONFIG_HAVE_IMA_KEXEC
> &
Re: [PATCH] certs: Add support for using elliptic curve keys for signing modules
On Fri, 2021-02-19 at 10:41 -0500, Stefan Berger wrote: > From: Stefan Berger > > This patch adds support for using elliptic curve keys for signing > modules. It uses a NIST P256 (prime256v1) key if the user chooses an > elliptic curve key. > > A developer choosing an ECDSA key for signing modules has to manually > delete the signing key (rm certs/signing_key.*) when falling back to > an older version of a kernel that only supports RSA key since otherwise > ECDSA-signed modules will not be usable when that older kernel runs. > > Signed-off-by: Stefan Berger Thanks, Stefan! Tested with this patch applied on top of "[PATCH v8 0/4] Add support for x509 certs with NIST p256 and p192" and "[PATCH v2 0/5] ima: kernel build support for loading the kernel module" patch sets. Tested-by: Mimi Zohar Reviewed-by: Mimi Zohar
Re: [PATCH] of: error: 'const struct kimage' has no member named 'arch'
On Fri, 2021-02-19 at 11:08 -0300, Thiago Jung Bauermann wrote:
> Lakshmi Ramasubramanian writes:
>
> > On 2/18/21 5:13 PM, Thiago Jung Bauermann wrote:
> >> Lakshmi Ramasubramanian writes:
> >>
> >>> On 2/18/21 4:07 PM, Mimi Zohar wrote:
> >>>
> >>> Hi Mimi,
> >>>
> >>>> On Thu, 2021-02-18 at 14:33 -0800, Lakshmi Ramasubramanian wrote:
> >>>>> of_kexec_alloc_and_setup_fdt() defined in drivers/of/kexec.c builds
> >>>>> a new device tree object that includes architecture specific data
> >>>>> for kexec system call. This should be defined only if the architecture
> >>>>> being built defines kexec architecture structure "struct kimage_arch".
> >>>>>
> >>>>> Define a new boolean config OF_KEXEC that is enabled if
> >>>>> CONFIG_KEXEC_FILE and CONFIG_OF_FLATTREE are enabled, and
> >>>>> the architecture is arm64 or powerpc64. Build drivers/of/kexec.c
> >>>>> if CONFIG_OF_KEXEC is enabled.
> >>>>>
> >>>>> Signed-off-by: Lakshmi Ramasubramanian
> >>>>> Fixes: 33488dc4d61f ("of: Add a common kexec FDT setup function")
> >>>>> Reported-by: kernel test robot
> >>>>> ---
> >>>>>drivers/of/Kconfig | 6 ++
> >>>>>drivers/of/Makefile | 7 +--
> >>>>>2 files changed, 7 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> >>>>> index 18450437d5d5..f2e8fa54862a 100644
> >>>>> --- a/drivers/of/Kconfig
> >>>>> +++ b/drivers/of/Kconfig
> >>>>> @@ -100,4 +100,10 @@ config OF_DMA_DEFAULT_COHERENT
> >>>>> # arches should select this if DMA is coherent by default for
> >>>>> OF devices
> >>>>> bool
> >>>>>+config OF_KEXEC
> >>>>> + bool
> >>>>> + depends on KEXEC_FILE
> >>>>> + depends on OF_FLATTREE
> >>>>> + default y if ARM64 || PPC64
> >>>>> +
> >>>>>endif # OF
> >>>>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> >>>>> index c13b982084a3..287579dd1695 100644
> >>>>> --- a/drivers/of/Makefile
> >>>>> +++ b/drivers/of/Makefile
> >>>>> @@ -13,11 +13,6 @@ obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
> >>>>>obj-$(CONFIG_OF_RESOLVE) += resolver.o
> >>>>>obj-$(CONFIG_OF_OVERLAY) += overlay.o
> >>>>>obj-$(CONFIG_OF_NUMA) += of_numa.o
> >>>>> -
> >>>>> -ifdef CONFIG_KEXEC_FILE
> >>>>> -ifdef CONFIG_OF_FLATTREE
> >>>>> -obj-y += kexec.o
> >>>>> -endif
> >>>>> -endif
> >>>>> +obj-$(CONFIG_OF_KEXEC) += kexec.o
> >>>>> obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> >>>> Is it possible to reuse CONFIG_HAVE_IMA_KEXEC here?
> >>>>
> >>>
> >>> For ppc64 CONFIG_HAVE_IMA_KEXEC is selected when CONFIG_KEXEC_FILE is
> >>> enabled.
> >>> So I don't see a problem in reusing CONFIG_HAVE_IMA_KEXEC for ppc.
> >>>
> >>> But for arm64, CONFIG_HAVE_IMA_KEXEC is enabled in the final patch in the
> >>> patch
> >>> set (the one for carrying forward IMA log across kexec for arm64). arm64
> >>> calls
> >>> of_kexec_alloc_and_setup_fdt() prior to enabling CONFIG_HAVE_IMA_KEXEC
> >>> and hence
> >>> breaks the build for arm64.
> >> One problem is that I believe that this patch won't placate the robot,
> >> because IIUC it generates config files at random and this change still
> >> allows hppa and s390 to enable CONFIG_OF_KEXEC.
> >
> > I enabled CONFIG_OF_KEXEC for s390. With my patch applied, CONFIG_OF_KEXEC
> > is
> > removed. So I think the robot enabling this config would not be a problem.
> >
> >> Perhaps a new CONFIG_HAVE_KIMAGE_ARCH option? Not having that option
> >> would still allow building kexec.o, but would be used inside kexec.c to
> >> avoid accessing kimage.arch members.
> >>
> >
> > I think this is a good idea - a new CONFIG_HAVE_KIMAGE_ARCH, which will be
> > selected by arm64 and ppc for now. I tried this, and it fixes the build
> > issue.
> >
> > Although, the name for the new config can be misleading since PARISC, for
> > instance, also defines "struct kimage_arch". Perhaps,
> > CONFIG_HAVE_ELF_KIMAGE_ARCH since of_kexec_alloc_and_setup_fdt() is
> > accessing ELF specific fields in "struct kimage_arch"?
>
> Ah, right. I should have digged into the code before making my
> suggestion. CONFIG_HAVE_KIMAGE_ARCH isn't appropriate, indeed.
>
> >
> > Rob/Mimi - please let us know which approach you think is better.
>
> Ah! We can actually use the existing CONFIG_HAVE_IMA_KEXEC, no? I don't
> know why I didn't think of it before.
Including kexec.o based on CONFIG_HAVE_IMA_KEXEC is a bisect issue on
ARM64, as Lakshmi pointed out. Defining a new, maybe temporary, flag
would solve the problem.
Mimi
Re: [PATCH] of: error: 'const struct kimage' has no member named 'arch'
On Thu, 2021-02-18 at 14:33 -0800, Lakshmi Ramasubramanian wrote:
> of_kexec_alloc_and_setup_fdt() defined in drivers/of/kexec.c builds
> a new device tree object that includes architecture specific data
> for kexec system call. This should be defined only if the architecture
> being built defines kexec architecture structure "struct kimage_arch".
>
> Define a new boolean config OF_KEXEC that is enabled if
> CONFIG_KEXEC_FILE and CONFIG_OF_FLATTREE are enabled, and
> the architecture is arm64 or powerpc64. Build drivers/of/kexec.c
> if CONFIG_OF_KEXEC is enabled.
>
> Signed-off-by: Lakshmi Ramasubramanian
> Fixes: 33488dc4d61f ("of: Add a common kexec FDT setup function")
> Reported-by: kernel test robot
> ---
> drivers/of/Kconfig | 6 ++
> drivers/of/Makefile | 7 +--
> 2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 18450437d5d5..f2e8fa54862a 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -100,4 +100,10 @@ config OF_DMA_DEFAULT_COHERENT
> # arches should select this if DMA is coherent by default for OF devices
> bool
>
> +config OF_KEXEC
> + bool
> + depends on KEXEC_FILE
> + depends on OF_FLATTREE
> + default y if ARM64 || PPC64
> +
> endif # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index c13b982084a3..287579dd1695 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -13,11 +13,6 @@ obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
> obj-$(CONFIG_OF_RESOLVE) += resolver.o
> obj-$(CONFIG_OF_OVERLAY) += overlay.o
> obj-$(CONFIG_OF_NUMA) += of_numa.o
> -
> -ifdef CONFIG_KEXEC_FILE
> -ifdef CONFIG_OF_FLATTREE
> -obj-y+= kexec.o
> -endif
> -endif
> +obj-$(CONFIG_OF_KEXEC) += kexec.o
>
> obj-$(CONFIG_OF_UNITTEST) += unittest-data/
Is it possible to reuse CONFIG_HAVE_IMA_KEXEC here?
Mimi
Re: [PATCH v2 5/5] ima: enable loading of build time generated key on .ima keyring
On Thu, 2021-02-18 at 17:00 -0500, Nayna Jain wrote: > The kernel currently only loads the kernel module signing key onto > the builtin trusted keyring. To support IMA, load the module signing > key selectively either onto the builtin or IMA keyring based on MODULE_SIG > or MODULE_APPRAISE_MODSIG config respectively; and loads the CA kernel > key onto the builtin trusted keyring. > > Signed-off-by: Nayna Jain Always having a CA key would simplify the code. Otherwise for the patch set, Reviewed-by: Mimi Zohar
Re: [PATCH v2] IMA: support for duplicate data measurement
On Wed, 2021-02-17 at 10:53 -0800, Tushar Sugandhi wrote: > Thanks for the feedback Mimi. > Appreciate it. > > On 2021-02-17 7:03 a.m., Mimi Zohar wrote: > > Hi Tushar, > > > > The Subject line could be improved. Perhaps something like - "IMA: > > support for duplicate measurement records" > > > Will do. > > > On Tue, 2021-02-16 at 18:46 -0800, Tushar Sugandhi wrote: > >> IMA does not measure duplicate data since TPM extend is a very expensive > >> operation. However, in some cases, the measurement of duplicate data > >> is necessary to accurately determine the current state of the system. > >> Eg, SELinux state changing from 'audit', to 'enforcing', and back to > >> 'audit' again. In this example, currently, IMA will not measure the > >> last state change to 'audit'. This limits the ability of attestation > >> services to accurately determine the current state of the measurements > >> on the system. > > > > This patch description is written from your specific usecase > > perspective, but it impacts file and buffer data measurements as well, > > not only critical data measurements. In all of these situations, with > > this patch a new measurement record is added/appended to the > > measurement list. Please re-write the patch description making it more > > generic. > > > > For example, I would start with something like, "IMA does not include > > duplicate file, buffer or critical data measurement records ..." > > > Agreed. > I will generalize the description further and send the v3 for review. It would be good to boot with the ima_policy=tcb policy with/without your patch and account for the different number of measurements. Are all the differences related to duplicate measurements - original file hash -> new file hash -> original file hash - similar to what you described. thanks, Mimi
Re: [PATCH v2] IMA: support for duplicate data measurement
Hi Tushar, The Subject line could be improved. Perhaps something like - "IMA: support for duplicate measurement records" On Tue, 2021-02-16 at 18:46 -0800, Tushar Sugandhi wrote: > IMA does not measure duplicate data since TPM extend is a very expensive > operation. However, in some cases, the measurement of duplicate data > is necessary to accurately determine the current state of the system. > Eg, SELinux state changing from 'audit', to 'enforcing', and back to > 'audit' again. In this example, currently, IMA will not measure the > last state change to 'audit'. This limits the ability of attestation > services to accurately determine the current state of the measurements > on the system. This patch description is written from your specific usecase perspective, but it impacts file and buffer data measurements as well, not only critical data measurements. In all of these situations, with this patch a new measurement record is added/appended to the measurement list. Please re-write the patch description making it more generic. For example, I would start with something like, "IMA does not include duplicate file, buffer or critical data measurement records ..." thanks, Mimi > > Update ima_add_template_entry() to support measurement of duplicate > data, driven by a Kconfig option - IMA_DISABLE_HTABLE. > > Signed-off-by: Tushar Sugandhi
[GIT PULL] integrity subsystem updates for v5.12
Hi Linus, New is IMA support for measuring kernel critical data, as per usual based on policy. The first example measures the in memory SELinux policy. The second example measures the kernel version. In addition are four bug fixes to address memory leaks and a missing "static" function declaration. [FYI: Stephen is carrying a manual merge of the pidfd tree with the integrity tree.] thanks, Mimi The following changes since commit 7c53f6b671f4aba70ff15e1b05148b10d58c2837: Linux 5.11-rc3 (2021-01-10 14:34:50 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git tags/integrity-v5.12 for you to fetch changes up to f6692213b5045dc461ce0858fb18cf46f328c202: integrity: Make function integrity_add_key() static (2021-02-12 11:11:59 -0500) integrity-v5.12 Dinghao Liu (1): evm: Fix memleak in init_desc Lakshmi Ramasubramanian (4): IMA: define a builtin critical data measurement policy selinux: include a consumer of the new IMA critical data hook ima: Free IMA measurement buffer on error ima: Free IMA measurement buffer after kexec syscall Mimi Zohar (2): Merge branch 'measure-critical-data' into next-integrity Merge branch 'ima-kexec-fixes' into next-integrity Raphael Gianotti (1): IMA: Measure kernel version in early boot Tushar Sugandhi (6): IMA: generalize keyring specific measurement constructs IMA: add support to measure buffer data hash IMA: define a hook to measure kernel integrity critical data IMA: add policy rule to measure critical data IMA: limit critical data measurement based on a label IMA: extend critical data hook to limit the measurement based on a label Wei Yongjun (1): integrity: Make function integrity_add_key() static Documentation/ABI/testing/ima_policy| 5 +- Documentation/admin-guide/kernel-parameters.txt | 5 +- include/linux/ima.h | 10 +++ include/linux/kexec.h | 5 ++ kernel/kexec_file.c | 5 ++ security/integrity/digsig.c | 4 +- security/integrity/evm/evm_crypto.c | 7 +- security/integrity/ima/ima.h| 8 +- security/integrity/ima/ima_api.c| 8 +- security/integrity/ima/ima_appraise.c | 2 +- security/integrity/ima/ima_asymmetric_keys.c| 2 +- security/integrity/ima/ima_init.c | 5 ++ security/integrity/ima/ima_kexec.c | 3 + security/integrity/ima/ima_main.c | 59 ++-- security/integrity/ima/ima_policy.c | 115 +++- security/integrity/ima/ima_queue_keys.c | 3 +- security/selinux/Makefile | 2 + security/selinux/ima.c | 44 + security/selinux/include/ima.h | 24 + security/selinux/include/security.h | 3 +- security/selinux/ss/services.c | 64 +++-- 21 files changed, 329 insertions(+), 54 deletions(-) create mode 100644 security/selinux/ima.c create mode 100644 security/selinux/include/ima.h
Re: [PATCH] integrity/ima: Provide Kconfig option for ima-modsig template
Hi Michael, On Mon, 2021-02-15 at 11:23 +0100, Michael Weiß wrote: > 'ima-modsig' was not in the list of selectable templates in Kconfig. > The missing Kconfig options were added to support the ima-modsig > template as default template. > > Signed-off-by: Michael Weiß Since 'ima-modsig' is only needed for appended signatures (e.g. kexec kernel image on powerpc, kernel modules) a per policy rule "template=" option was defined. There's also the 'ima_template=' boot command line option. Between these two options, I didn't see the need for making it a build time default option. Do you? The patch itself looks good. thanks, Mimi > --- > security/integrity/ima/Kconfig | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig > index 12e9250c1bec..32b9325f49bf 100644 > --- a/security/integrity/ima/Kconfig > +++ b/security/integrity/ima/Kconfig > @@ -78,6 +78,8 @@ choice > bool "ima-ng (default)" > config IMA_SIG_TEMPLATE > bool "ima-sig" > + config IMA_MODSIG_TEMPLATE > + bool "ima-modsig" > endchoice > > config IMA_DEFAULT_TEMPLATE > @@ -86,6 +88,7 @@ config IMA_DEFAULT_TEMPLATE > default "ima" if IMA_TEMPLATE > default "ima-ng" if IMA_NG_TEMPLATE > default "ima-sig" if IMA_SIG_TEMPLATE > + default "ima-modsig" if IMA_MODSIG_TEMPLATE > > choice > prompt "Default integrity hash algorithm"
Re: [PATCH v24 04/25] IMA: avoid label collisions with stacked LSMs
Hi Casey,
On Tue, 2021-01-26 at 08:40 -0800, Casey Schaufler wrote:
> Integrity measurement may filter on security module information
> and needs to be clear in the case of multiple active security
> modules which applies. Provide a boot option ima_rules_lsm= to
> allow the user to specify an active securty module to apply
> filters to. If not specified, use the first registered module
> that supports the audit_rule_match() LSM hook. Allow the user
> to specify in the IMA policy an lsm= option to specify the
> security module to use for a particular rule.
Thanks, Casey.
(This patch description line length seems short.)
>
> Signed-off-by: Casey Schaufler
> To: Mimi Zohar
> To: linux-integr...@vger.kernel.org
> ---
> Documentation/ABI/testing/ima_policy | 8 +++-
> security/integrity/ima/ima_policy.c | 64 ++--
> 2 files changed, 57 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy
> b/Documentation/ABI/testing/ima_policy
> index e35263f97fc1..a7943d40466f 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -25,7 +25,7 @@ Description:
> base: [[func=] [mask=] [fsmagic=] [fsuuid=] [uid=]
> [euid=] [fowner=] [fsname=]]
> lsm:[[subj_user=] [subj_role=] [subj_type=]
> - [obj_user=] [obj_role=] [obj_type=]]
> + [obj_user=] [obj_role=] [obj_type=] [lsm=]]
"[lsm=]" either requires all LSM rules types (e.g. {subj/obj}_user,
role, type) to be exactly the same for multiple LSMs or all of the LSM
rule types are applicable to only a single LSM. Supporting multiple
LSMs with exactly the same LSM labels doesn't seem worth the effort.
Keep it simple - a single rule, containing any LSM rule types, is
applicable to a single LSM.
> option: [[appraise_type=]] [template=] [permit_directio]
> [appraise_flag=] [keyrings=]
> base:
> @@ -114,6 +114,12 @@ Description:
>
> measure subj_user=_ func=FILE_CHECK mask=MAY_READ
>
> + It is possible to explicitly specify which security
> + module a rule applies to using lsm=. If the security
> + modules specified is not active on the system the rule
> + will be rejected. If lsm= is not specified the first
> + security module registered on the system will be assumed.
> +
> Example of measure rules using alternate PCRs::
>
> measure func=KEXEC_KERNEL_CHECK pcr=4
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c
> index 8002683003e6..de72b719c90c 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -82,6 +82,7 @@ struct ima_rule_entry {
> void *rules[LSMBLOB_ENTRIES]; /* LSM file metadata specific */
> char *args_p; /* audit value */
> int type; /* audit type */
> + int which_lsm; /* which of the rules to use */
> } lsm[MAX_LSM_RULES];
Even if we wanted to support multiple LSMs within the same rule having
both "rules[LSMBLOB_ENTRIES]" and "which_lsm" shouldn't be necessary.
The LSMBLOB_ENTRIES should already identify the LSM.
To support a single LSM per policy rule, "which_lsm" should be defined
outside of lsm[MAX_LSM_RULES]. This will simplify the rest of the code
(e.g. matching/freeing rules).
int which_lsm; /* which of the rules to use */
struct {
void *rule;/* LSM file metadata specific */
char *args_p; /* audit value */
int type; /* audit type */
} lsm[MAX_LSM_RULES];
> char *fsname;
> struct ima_rule_opt_list *keyrings; /* Measure keys added to these
> keyrings */
> @@ -90,17 +91,15 @@ struct ima_rule_entry {
>
> /**
> * ima_lsm_isset - Is a rule set for any of the active security modules
> - * @rules: The set of IMA rules to check
> + * @entry: the rule entry to examine
> + * @lsm_rule: the specific rule type in question
> *
> - * If a rule is set for any LSM return true, otherwise return false.
> + * If a rule is set return true, otherwise return false.
> */
> -static inline bool ima_lsm_isset(void *rules[])
> +static inline bool ima_lsm_isset(struct ima_rule_entry *entry, int lsm_rule)
> {
> - int i;
> -
> - for (i = 0; i < LSMBLOB_ENTRIES; i++)
> - if (rules[i])
> - return true;
> + if (entry->lsm[lsm_rule].rules[entry->l
Re: [PATCH 4/5] keys: define build time generated ephemeral kernel CA key
On Thu, 2021-02-11 at 17:13 -0500, Stefan Berger wrote: > On 2/11/21 2:54 PM, Nayna Jain wrote: > > Certificates being loaded onto the IMA trusted keyring must be signed by > > a key on either the builtin and secondary trusted keyring. > > > > This patch creates and includes in the kernel image an ephemeral CA > > key, at build time when IMA_APPRAISE_MODSIG is enabled. > > > > Signed-off-by: Nayna Jain > > --- > > diff --git a/certs/Makefile b/certs/Makefile > > > @@ -60,14 +78,23 @@ $(obj)/signing_key.pem: $(obj)/x509.genkey > > @$(kecho) "### needs to be run as root, and uses a hardware random" > > @$(kecho) "### number generator if one is available." > > @$(kecho) "###" > > +ifeq ($(CONFIG_IMA_APPRAISE_MODSIG),y) > > + # Generate kernel build time CA Certificate. > > + @$(Q)openssl req -new -nodes -utf8 \ > > + -$(CONFIG_MODULE_SIG_HASH) -days 36500 \ > > + -subj "/CN=Build time autogenerated kernel CA key" \ > > + -batch -x509 -config $(obj)/x509.genkey \ > > + -outform PEM -out $(CA_KEY) \ > > + -keyout $(CA_KEY) -extensions ca_ext \ > > + $($(quiet)redirect_openssl) > > +endif # CONFIG_IMA_APPRAISE_MODSIG > > $(Q)openssl req -new -nodes -utf8 \ > > -batch -config $(obj)/x509.genkey \ > > -outform PEM -out $(obj)/signing_key.csr \ > > -keyout $(obj)/signing_key.key -extensions myexts \ > > $($(quiet)redirect_openssl) > > $(Q)openssl x509 -req -days 36500 -in $(obj)/signing_key.csr \ > > - -outform PEM -out $(obj)/signing_key.crt \ > > - -signkey $(obj)/signing_key.key \ > > + -outform PEM -out $(obj)/signing_key.crt $(SIGNER) \ > > -$(CONFIG_MODULE_SIG_HASH) -extensions myexts \ > > -extfile $(obj)/x509.genkey \ > > $($(quiet)redirect_openssl) > > It may make things easier (also below) if the CA was always created and > the kernel signing key was always signed by that CA rather than doing > this only in the IMA_APPRAISE_MODSIG case. Maybe someone else has an > opinion on that? Thanks, Stefan. It would definitely simplify the code. We wanted to minimize the code change and solicit feedback, before making such a change. Mimi
Re: [PATCH v17 00/10] Carry forward IMA measurement log on kexec on ARM64
On Wed, 2021-02-10 at 15:55 -0500, Mimi Zohar wrote: > On Wed, 2021-02-10 at 14:42 -0600, Rob Herring wrote: > > On Wed, Feb 10, 2021 at 11:33 AM Lakshmi Ramasubramanian > > > Ideally, we don't apply the same patch in 2 branches. It looks like > > there's a conflict but no real dependence on the above patch (the > > ima_buffer part). The conflict seems trivial enough that Linus can > > resolve it in the merge window. > > > > Or Mimi can take the whole thing if preferred? > > How about I create a topic branch with just the two patches, allowing > both of us to merge it? There shouldn't be a problem with re-writing > next-integrity history. The 2 patches are now in the ima-kexec-fixes branch. Mimi
Re: [PATCH v17 00/10] Carry forward IMA measurement log on kexec on ARM64
On Wed, 2021-02-10 at 14:42 -0600, Rob Herring wrote:
> On Wed, Feb 10, 2021 at 11:33 AM Lakshmi Ramasubramanian
> wrote:
> >
> > On 2/10/21 9:15 AM, Rob Herring wrote:
> > > On Tue, Feb 09, 2021 at 10:21:50AM -0800, Lakshmi Ramasubramanian wrote:
> > >> On kexec file load Integrity Measurement Architecture (IMA) subsystem
> > >> may verify the IMA signature of the kernel and initramfs, and measure
> > >> it. The command line parameters passed to the kernel in the kexec call
> > >> may also be measured by IMA. A remote attestation service can verify
> > >> a TPM quote based on the TPM event log, the IMA measurement list, and
> > >> the TPM PCR data. This can be achieved only if the IMA measurement log
> > >> is carried over from the current kernel to the next kernel across
> > >> the kexec call.
> > >>
> > >> powerpc already supports carrying forward the IMA measurement log on
> > >> kexec. This patch set adds support for carrying forward the IMA
> > >> measurement log on kexec on ARM64.
> > >>
> > >> This patch set moves the platform independent code defined for powerpc
> > >> such that it can be reused for other platforms as well. A chosen node
> > >> "linux,ima-kexec-buffer" is added to the DTB for ARM64 to hold
> > >> the address and the size of the memory reserved to carry
> > >> the IMA measurement log.
> > >>
> > >> This patch set has been tested for ARM64 platform using QEMU.
> > >> I would like help from the community for testing this change on powerpc.
> > >> Thanks.
> > >>
> > >> This patch set is based on
> > >> commit 96acc833dec8 ("ima: Free IMA measurement buffer after kexec
> > >> syscall")
> > >> in
> > >> https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git
> > >> "next-integrity" branch.
> > >
> > > Is that a hard dependency still? Given this is now almost entirely
> > > deleting arch code and adding drivers/of/ code, I was going to apply it.
> > >
> >
> > I tried applying the patches in Linus' mainline branch -
> > PATCH #5 0005-powerpc-Move-ima-buffer-fields-to-struct-kimage.patch
> > doesn't apply.
> >
> > But if I apply the dependent patch set (link given below), all the
> > patches in this patch set apply fine.
> >
> > https://patchwork.kernel.org/project/linux-integrity/patch/20210204174951.25771-2-nra...@linux.microsoft.com/
>
> Ideally, we don't apply the same patch in 2 branches. It looks like
> there's a conflict but no real dependence on the above patch (the
> ima_buffer part). The conflict seems trivial enough that Linus can
> resolve it in the merge window.
>
> Or Mimi can take the whole thing if preferred?
How about I create a topic branch with just the two patches, allowing
both of us to merge it? There shouldn't be a problem with re-writing
next-integrity history.
Mimi
Re: [PATCH 0/3] support for duplicate measurement of integrity critical data
On Tue, 2021-02-09 at 10:23 -0800, Tushar Sugandhi wrote: > > On Mon, 2021-02-08 at 15:22 -0500, Mimi Zohar wrote: > >> On Fri, 2021-01-29 at 16:45 -0800, Tushar Sugandhi wrote: > >>> IMA does not measure duplicate buffer data since TPM extend is a very > >>> expensive operation. However, in some cases for integrity critical > >>> data, the measurement of duplicate data is necessary to accurately > >>> determine the current state of the system. Eg, SELinux state changing > >>> from 'audit', to 'enforcing', and back to 'audit' again. In this > >>> example, currently, IMA will not measure the last state change to > >>> 'audit'. This limits the ability of attestation services to accurately > >>> determine the current state of the integrity critical data on the > >>> system. > >>> > >>> This series addresses this gap by providing the ability to measure > >>> duplicate entries for integrity critical data, driven by policy. > >> > >> The same reason for re-measuring buffer data is equally applicable to > >> files. In both cases, the file or the buffer isn't re-measured if it > >> already exists in the htable. Please don't limit this patch set to > >> just buffer data. > > > Agreed. I wasn't sure if you wanted the support for files, or other > buffer measurement scenarios, except critical data. So I started the > implementation with supporting just critical data. Happy to extend it > to files and other buffer measurement scenarios as you suggested. > > > Instead of making the change on a per measurement rule basis, disabling > > "htable" would be the simplest way of forcing re-measurements. All > > that would be needed is a new Kconfig (e.g. CONFIG_IMA_DISABLE_HTABLE) > > and the associated test in ima_add_template_entry(). > > > Agreed. Earlier I wasn't sure if you wanted allow_dup support for all > the scenarios. Now that it is clear, I will implement it as you > suggested. Thank you so much for the pointers. Appreciate it. There are two different solutions - per measurement rule, disabling htable - being discussed. Disabling htable requires miminumal changes. Which version are you thinking of implementing? thanks, Mimi
Re: Migration to trusted keys: sealing user-provided key?
On Mon, 2021-02-08 at 15:38 +0100, Jan Lübbe wrote: > As it seems that this feature would not be appropriate for all use-cases and > threat models, I wonder if making it optional would be acceptable. Something > like: > > config TRUSTED_KEYS_IMPORT To me "IMPORT" implies from a trusted source, which this is not. Perhaps "UNSAFE_IMPORT", "DEBUGGING_IMPORT, "DEVELOPMENT_IMPORT", ... Defining a Kconfig with any of these names and the other changes below, makes it very clear using predefined key data is not recommended. My concern with extending trusted keys to new trust sources is the implication that the security/integrity is equivalent to the existing discrete TPM. > bool "Allow creating TRUSTED KEYS from existing key material" > depends on TRUSTED_KEYS Missing "default n" > help > This option adds support for creating new trusted keys from > existing > key material supplied by userspace, instead of using random numbers. > As with random trusted keys, userspace cannot extract the > plain-text Once defined, as with random trusted keys, userspace cannot ... > key material again and will only ever see encrypted blobs. > > This option should *only* be enabled for use in a trusted > environment (such as during debugging/development or in a secured > factory). Also, consider using 'keyctl padd' instead of 'keyctl > add' Even the "secured factory" is not a good idea. Please limit the usage to debugging/development. > to avoid exposing the plain-text key on the process command line. > > If you are unsure as to whether this is required, answer N. The above would be fine. thanks, Mimi
Re: [PATCH 0/3] support for duplicate measurement of integrity critical data
Hi Tushar, On Mon, 2021-02-08 at 15:22 -0500, Mimi Zohar wrote: > On Fri, 2021-01-29 at 16:45 -0800, Tushar Sugandhi wrote: > > IMA does not measure duplicate buffer data since TPM extend is a very > > expensive operation. However, in some cases for integrity critical > > data, the measurement of duplicate data is necessary to accurately > > determine the current state of the system. Eg, SELinux state changing > > from 'audit', to 'enforcing', and back to 'audit' again. In this > > example, currently, IMA will not measure the last state change to > > 'audit'. This limits the ability of attestation services to accurately > > determine the current state of the integrity critical data on the > > system. > > > > This series addresses this gap by providing the ability to measure > > duplicate entries for integrity critical data, driven by policy. > > The same reason for re-measuring buffer data is equally applicable to > files. In both cases, the file or the buffer isn't re-measured if it > already exists in the htable. Please don't limit this patch set to > just buffer data. Instead of making the change on a per measurement rule basis, disabling "htable" would be the simplest way of forcing re-measurements. All that would be needed is a new Kconfig (e.g. CONFIG_IMA_DISABLE_HTABLE) and the associated test in ima_add_template_entry(). thanks, Mimi
Re: [PATCH 1/3] IMA: add policy condition to measure duplicate critical data
Hi Tushar,
On Fri, 2021-01-29 at 16:45 -0800, Tushar Sugandhi wrote:
> IMA needs to support duplicate measurements of integrity
> critical data to accurately determine the current state of that data
> on the system. Further, since measurement of duplicate data is not
> required for all the use cases, it needs to be policy driven.
>
> Define "allow_dup", a new IMA policy condition, for the IMA func
> CRITICAL_DATA to allow duplicate buffer measurement of integrity
> critical data.
>
> Limit the ability to measure duplicate buffer data when action is
> "measure" and func is CRITICAL_DATA.
Why?!
>
> Signed-off-by: Tushar Sugandhi
> ---
>
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c
> index 9b45d064a87d..b89eb768dd05 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -35,6 +35,7 @@
> #define IMA_FSNAME 0x0200
> #define IMA_KEYRINGS 0x0400
> #define IMA_LABEL0x0800
> +#define IMA_ALLOW_DUP0x1000
>
> #define UNKNOWN 0
> #define MEASURE 0x0001 /* same as IMA_MEASURE */
> @@ -87,6 +88,7 @@ struct ima_rule_entry {
> char *fsname;
> struct ima_rule_opt_list *keyrings; /* Measure keys added to these
> keyrings */
> struct ima_rule_opt_list *label; /* Measure data grouped under this
> label */
Defining a new boolean entry shouldn't be necessary.The other
boolean values are just stored in "flags".
> struct ima_template_desc *template;
> };
thanks,
Mimi
Re: [PATCH 0/3] support for duplicate measurement of integrity critical data
Hi Tushar, On Fri, 2021-01-29 at 16:45 -0800, Tushar Sugandhi wrote: > IMA does not measure duplicate buffer data since TPM extend is a very > expensive operation. However, in some cases for integrity critical > data, the measurement of duplicate data is necessary to accurately > determine the current state of the system. Eg, SELinux state changing > from 'audit', to 'enforcing', and back to 'audit' again. In this > example, currently, IMA will not measure the last state change to > 'audit'. This limits the ability of attestation services to accurately > determine the current state of the integrity critical data on the > system. > > This series addresses this gap by providing the ability to measure > duplicate entries for integrity critical data, driven by policy. The same reason for re-measuring buffer data is equally applicable to files. In both cases, the file or the buffer isn't re-measured if it already exists in the htable. Please don't limit this patch set to just buffer data. thanks, Mimi
Re: [PATCH 3/3] IMA: add support to measure duplicate buffer for critical data hook
Hi Tushar,
On Fri, 2021-01-29 at 16:45 -0800, Tushar Sugandhi wrote:
> diff --git a/security/integrity/ima/ima_queue.c
> b/security/integrity/ima/ima_queue.c
>
> index c096ef8945c7..fbf359495fa8 100644
> --- a/security/integrity/ima/ima_queue.c
> +++ b/security/integrity/ima/ima_queue.c
> @@ -158,7 +158,7 @@ static int ima_pcr_extend(struct tpm_digest *digests_arg,
> int pcr)
> */
> int ima_add_template_entry(struct ima_template_entry *entry, int violation,
> const char *op, struct inode *inode,
> -const unsigned char *filename)
> +const unsigned char *filename, bool allow_dup)
> {
> u8 *digest = entry->digests[ima_hash_algo_idx].digest;
>
struct tpm_digestate_entry(struct ima_template_entry *entry, int
violation,
>
> mutex_lock(_extend_list_mutex);
> if (!violation) {
> - if (ima_lookup_digest_entry(digest, entry->pcr)) {
> + if (!allow_dup &&
> + ima_lookup_digest_entry(digest, entry->pcr)) {
Can't this change be simplified to "if (!violation && !allow_dup)"?
Also perhaps instead of passing another variable "allow_dup" to each of
these functions, pass a mask containing violation and allow_dup.
> audit_cause = "hash_exists";
> result = -EEXIST;
> goto out;
thanks,
Mimi
Re: [PATCH v2 1/2] ima: Free IMA measurement buffer on error
On Fri, 2021-02-05 at 09:39 -0800, Lakshmi Ramasubramanian wrote:
> On 2/5/21 2:05 AM, Greg KH wrote:
> > On Thu, Feb 04, 2021 at 09:49:50AM -0800, Lakshmi Ramasubramanian wrote:
> >> IMA allocates kernel virtual memory to carry forward the measurement
> >> list, from the current kernel to the next kernel on kexec system call,
> >> in ima_add_kexec_buffer() function. In error code paths this memory
> >> is not freed resulting in memory leak.
> >>
> >> Free the memory allocated for the IMA measurement list in
> >> the error code paths in ima_add_kexec_buffer() function.
> >>
> >> Signed-off-by: Lakshmi Ramasubramanian
> >> Suggested-by: Tyler Hicks
> >> Fixes: 7b8589cc29e7 ("ima: on soft reboot, save the measurement list")
> >> ---
> >> security/integrity/ima/ima_kexec.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >
> >
> >
> > This is not the correct way to submit patches for inclusion in the
> > stable kernel tree. Please read:
> > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > for how to do this properly.
> >
> >
> >
>
> Thanks for the info Greg.
>
> I will re-submit the two patches in the proper format.
No need. I'm testing these patches now. I'm not exactly sure what the
problem is. Stable wasn't Cc'ed. Is it that you sent the patch
directly to Greg or added "Fixes"?
thanks,
Mimi
Re: [PATCH v7 4/4] ima: Support EC keys for signature verification
On Mon, 2021-02-01 at 10:19 -0500, Stefan Berger wrote: > Add support for IMA signature verification for EC keys. Since SHA type > of hashes can be used by RSA and ECDSA signature schemes we need to > look at the key and derive from the key which signature scheme to use. > Since this can be applied to all types of keys, we change the selection > of the encoding type to be driven by the key's signature scheme rather > than by the hash type. > > Signed-off-by: Stefan Berger > Reviewed-by: Vitaly Chikunov > Reviewed-by: Tianjia Zhang Thanks, Stefan! Acked-by: Mimi Zohar
Re: Migration to trusted keys: sealing user-provided key?
On Mon, 2021-02-01 at 17:38 +0100, Jan Lübbe wrote: > On Mon, 2021-02-01 at 11:11 -0500, Mimi Zohar wrote: > > On Mon, 2021-02-01 at 16:31 +0100, Jan Lübbe wrote: > > > On Sun, 2021-01-31 at 09:29 -0500, Mimi Zohar wrote: > > > > > Usage:: > > > > > > > > keyctl add encrypted name "new [format] key-type:master-key-name > > > > keylen" > > > > ring > > > > keyctl add encrypted name "load hex_blob" ring > > > > > > 'load' (as I understand the code) only accepts an encrypted blob. > > > > > > So the only way I see to have an encrypted key with a non-random key data > > > would > > > be: > > > - create a random temporary master key and load a copy as a user key > > > - encrypt the chosen key data with the temporary master key (using a new > > > userspace reimplementation of the kernel encrypted key blob format) > > > - use keyctl add encrypted dmcrypt "load " > > > - create new trusted master key (OP-TEE or CAAM in our case) as > > > - use keyctl update to switch to the new trusted master key > > > - use keyctl pipe on the trusted and encrypted keys and store both for > > > loading > > > on later boots > > > > > > If we'd support importing a pre-existing key into a trusted or encrypted > > > key, > > > we'd do instead: > > > - use keyctl add trusted dmcrypt "import " > > > - use keyctl pipe on the trusted key and store it for loading on later > > > boots > > > > > > This way, users wouldn't need to care which backend is used by trusted > > > keys > > > (TPM/OP-TEE/CAAM/...). That would make use-cases where a random key is not > > > suitable as straight-forward as the those where a random key is OK. > > > > As I said above, the "encrypted" key update doesn't change the key data > > used for encrypting/decrypting storage in the dm-crypt case, it just > > updates the key under which it is encrypted/signed. > > Yes, that's clear. I only used it to demonstrate how a workaround for > importing > key material into an encrypted key could look like. > > > Yes, the reason for using an encrypted "trusted" key, as opposed to an > > encrypted "user" key, is that the "trusted" key is encrypted/decrypted > > by the TPM and never exposed to userspace in the clear. > > Yes, and that's the main reason I'd like to use trusted keys with dm-crypt: a > much lower chance of exposing this key somewhere it could be extracted. > > > It doesn't sound like you're wanting to update the storage key in the > > field, just the key used to encrypt/decrypt that key. So I'm still not > > clear as to why you would want an initial non-random encrypted key. > > Providing that key on the command line certaining isn't a good idea. > > Some of our customers have systems in the field which use non-mainline patches > for access to the CAAM [1], which also have the downside of exposing the > decrypted key material directly to userspace. In that thread you suggested to > use trusted keys instead. With Sumit's work that rework is finally within > reach. > :) > > > In those systems, we have data that's encrypted with a pre-existing dm-crypt > or > ecryptfs key. As we update those systems in the field to newer kernels, we > want > to get rid of those custom patches, but can't reencrypt everything. > > So the approach would be to perform a one-time migration when updating a > device: > - use our old interface to decrypt the key and 'import' it into a trusted key > - use keyctl pipe and save the re-encrypted key to disk > - destroy the old encrypted key > After this migration, the key material is no longer available to userspace > (only > to dm-crypt). > > > Another use-case for supporting key import that we want to support is > analysis > of broken devices returned from the field: > - generate an encryption key per device in the factory > - encrypt it to a private key in escrow and archive it for later use > - import it into a trusted key on the device > - keyctl pipe it to a file on the device for use on boot > > Later, when you need to do an analysis, you can get the key from escrow even > if > the device cannot boot any longer. The first use case doesn't sound like a valid reason for upstreaming such support. It's a one time update to migrate everyone to a newer kernel. That you can carry independently of upstream. In terms of the second use case, do you really want the ability and the resulting responsibility of being able to decrypt user's data? Please think this through carefully, before you decide you really want/need this feature. thanks, Mimi
Re: Migration to trusted keys: sealing user-provided key?
On Mon, 2021-02-01 at 16:31 +0100, Jan Lübbe wrote: > On Sun, 2021-01-31 at 09:29 -0500, Mimi Zohar wrote: > > On Sun, 2021-01-31 at 15:14 +0100, Jan Lübbe wrote: > > > On Sun, 2021-01-31 at 07:09 -0500, Mimi Zohar wrote: > > > > > > > > > > > > > > [1] The ima-evm-utils README contains EVM examples of "trusted" and > > > > "user" based "encrypted" keys. > > > > > > I assume you refer to > > > https://sourceforge.net/p/linux-ima/ima-evm-utils/ci/master/tree/README#l143 > > > "Generate EVM encrypted keys" and "Generate EVM trusted keys (TPM based)"? > > > > > > In both cases, the key used by EVM is a *newly generated* random key. The > > > only > > > difference is whether it's encrypted to a user key or a (random) trusted > > > key. > > > > The "encrypted" asymmetric key data doesn't change, "update" just > > changes the key under which it is encrypted/decrypted. > > > > Usage:: > > > > keyctl add encrypted name "new [format] key-type:master-key-name keylen" > > ring > > keyctl add encrypted name "load hex_blob" ring > > 'load' (as I understand the code) only accepts an encrypted blob. > > So the only way I see to have an encrypted key with a non-random key data > would > be: > - create a random temporary master key and load a copy as a user key > - encrypt the chosen key data with the temporary master key (using a new > userspace reimplementation of the kernel encrypted key blob format) > - use keyctl add encrypted dmcrypt "load " > - create new trusted master key (OP-TEE or CAAM in our case) as > - use keyctl update to switch to the new trusted master key > - use keyctl pipe on the trusted and encrypted keys and store both for loading > on later boots > > If we'd support importing a pre-existing key into a trusted or encrypted key, > we'd do instead: > - use keyctl add trusted dmcrypt "import " > - use keyctl pipe on the trusted key and store it for loading on later boots > > This way, users wouldn't need to care which backend is used by trusted keys > (TPM/OP-TEE/CAAM/...). That would make use-cases where a random key is not > suitable as straight-forward as the those where a random key is OK. As I said above, the "encrypted" key update doesn't change the key data used for encrypting/decrypting storage in the dm-crypt case, it just updates the key under which it is encrypted/signed. Yes, the reason for using an encrypted "trusted" key, as opposed to an encrypted "user" key, is that the "trusted" key is encrypted/decrypted by the TPM and never exposed to userspace in the clear. It doesn't sound like you're wanting to update the storage key in the field, just the key used to encrypt/decrypt that key. So I'm still not clear as to why you would want an initial non-random encrypted key. Providing that key on the command line certaining isn't a good idea. Mimi
Re: [PATCH v5 2/4] x509: Detect sm2 keys by their parameters OID
On Mon, 2021-02-01 at 09:35 -0500, Stefan Berger wrote: > On 2/1/21 8:23 AM, David Howells wrote: > > Stefan Berger wrote: > > > >> From: Stefan Berger > >> > >> Detect whether a key is an sm2 type of key by its OID in the parameters > >> array rather than assuming that everything under OID_id_ecPublicKey > >> is sm2, which is not the case. > >> > >> Signed-off-by: Stefan Berger > >> Cc: David Howells > >> Cc: keyri...@vger.kernel.org > > I presume these cc's are intentionally not on the first patch or the cover > > (if > > there is one)? > > No, this is not intentional. I guess this is a case of wrong use of cc: > versus mailing lists - my bad. I posted the whole series to > linux-crypto, linux-integrity, keyrings and lkml. > > V6 is at least visible here now: > > - https://lkml.org/lkml/2021/1/31/323 > > - https://marc.info/?l=linux-crypto-vger=161213604618722=2 > > - > https://lore.kernel.org/linux-integrity/20210131233301.1301787-1-stef...@linux.ibm.com/T/#mbc9fae5facb4178f64c1145e2654258c0af8fa96 > > - https://marc.info/?l=linux-keyrings=161213608818735=2 > > > > > > > Do you have a branch you want me to pull or did you want me to take just > > patches 2-4? > > Please take it from the mailing list. If there are requests for more > changes on the crypto level, I will send another series. I personally am > waiting for some sort of verdict on the crypto level... The entire patch set should be upstreamed as a single patch set, after having each of the maintainer's Ack it. In addition, the v6 version is missing some Reviewed-by tags. (Stefan will re-post a v7 patch set.) David, I don't have problem with this patch set being upstreamed via the keys subsystem, assuming it's been tested. thanks, Mimi
Re: Migration to trusted keys: sealing user-provided key?
On Sun, 2021-01-31 at 15:14 +0100, Jan Lübbe wrote: > On Sun, 2021-01-31 at 07:09 -0500, Mimi Zohar wrote: > > > > [1] The ima-evm-utils README contains EVM examples of "trusted" and > > "user" based "encrypted" keys. > > I assume you refer to > https://sourceforge.net/p/linux-ima/ima-evm-utils/ci/master/tree/README#l143 > "Generate EVM encrypted keys" and "Generate EVM trusted keys (TPM based)"? > > In both cases, the key used by EVM is a *newly generated* random key. The only > difference is whether it's encrypted to a user key or a (random) trusted key. The "encrypted" asymmetric key data doesn't change, "update" just changes the key under which it is encrypted/decrypted. Usage:: keyctl add encrypted name "new [format] key-type:master-key-name keylen" ring keyctl add encrypted name "load hex_blob" ring keyctl update keyid "update key-type:master-key-name" Mimi
Re: Migration to trusted keys: sealing user-provided key?
On Sat, 2021-01-30 at 19:53 +0200, Jarkko Sakkinen wrote: > On Thu, 2021-01-28 at 18:31 +0100, Ahmad Fatoum wrote: > > Hello, > > > > I've been looking into how a migration to using trusted/encrypted keys > > would look like (particularly with dm-crypt). > > > > Currently, it seems the the only way is to re-encrypt the partitions > > because trusted/encrypted keys always generate their payloads from > > RNG. > > > > If instead there was a key command to initialize a new trusted/encrypted > > key with a user provided value, users could use whatever mechanism they > > used beforehand to get a plaintext key and use that to initialize a new > > trusted/encrypted key. From there on, the key will be like any other > > trusted/encrypted key and not be disclosed again to userspace. > > > > What are your thoughts on this? Would an API like > > > > keyctl add trusted dmcrypt-key 'set ' # user-supplied content > > > > be acceptable? > > Maybe it's the lack of knowledge with dm-crypt, but why this would be > useful? Just want to understand the bottleneck, that's all. We upstreamed "trusted" & "encrypted" keys together in order to address this sort of problem. Instead of directly using a "trusted" key for persistent file signatures being stored as xattrs, the "encrypted" key provides one level of indirection. The "encrypted" key may be encrypted/decrypted with either a TPM based "trusted" key or with a "user" type symmetric key[1]. Instead of modifying "trusted" keys, use a "user" type "encrypted" key. Mimi [1] The ima-evm-utils README contains EVM examples of "trusted" and "user" based "encrypted" keys.
Re: [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries
On Thu, 2021-01-28 at 10:27 -0500, Mimi Zohar wrote: > Hi David, > > On Thu, 2021-01-28 at 15:16 +, David Howells wrote: > > Which tree do you envision this going through? EFI or keyrings - or are you > > going to ask Linus to pull it directly? I can pull it if it should go > > through > > the keyrings tree. > > There's one more patch, yet to be posted, which updates > asymmetric_verify(). As long as you're willing to upstream all of the > patches, that's fine with me. Oops, wrong thread. I thought this was Stefan's patch set. Mimi
Re: [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries
Hi David, On Thu, 2021-01-28 at 15:16 +, David Howells wrote: > Which tree do you envision this going through? EFI or keyrings - or are you > going to ask Linus to pull it directly? I can pull it if it should go through > the keyrings tree. There's one more patch, yet to be posted, which updates asymmetric_verify(). As long as you're willing to upstream all of the patches, that's fine with me. thanks! Mimi
Re: [PATCH v15 10/10] arm64: Add IMA log information in kimage used for kexec
On Wed, 2021-01-27 at 10:24 -0800, Lakshmi Ramasubramanian wrote:
> On 1/27/21 10:02 AM, Will Deacon wrote:
> > On Wed, Jan 27, 2021 at 09:56:53AM -0800, Lakshmi Ramasubramanian wrote:
> >> On 1/27/21 8:54 AM, Will Deacon wrote:
> >>> On Fri, Jan 15, 2021 at 09:30:17AM -0800, Lakshmi Ramasubramanian wrote:
> Address and size of the buffer containing the IMA measurement log need
> to be passed from the current kernel to the next kernel on kexec.
>
> Add address and size fields to "struct kimage_arch" for ARM64 platform
> to hold the address and size of the IMA measurement log buffer.
>
> Update CONFIG_KEXEC_FILE to select CONFIG_HAVE_IMA_KEXEC, if CONFIG_IMA
> is enabled, to indicate that the IMA measurement log information is
> present in the device tree for ARM64.
>
> Co-developed-by: Prakhar Srivastava
> Signed-off-by: Prakhar Srivastava
> Signed-off-by: Lakshmi Ramasubramanian
> Reviewed-by: Thiago Jung Bauermann
> ---
> arch/arm64/Kconfig | 1 +
> arch/arm64/include/asm/kexec.h | 5 +
> 2 files changed, 6 insertions(+)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 1d466addb078..ea7f7fe3dccd 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1094,6 +1094,7 @@ config KEXEC
> config KEXEC_FILE
> bool "kexec file based system call"
> select KEXEC_CORE
> +select HAVE_IMA_KEXEC if IMA
> help
> This is new version of kexec system call. This system call is
> file based and takes file descriptors as system call argument
> diff --git a/arch/arm64/include/asm/kexec.h
> b/arch/arm64/include/asm/kexec.h
> index d24b527e8c00..2bd19ccb6c43 100644
> --- a/arch/arm64/include/asm/kexec.h
> +++ b/arch/arm64/include/asm/kexec.h
> @@ -100,6 +100,11 @@ struct kimage_arch {
> void *elf_headers;
> unsigned long elf_headers_mem;
> unsigned long elf_headers_sz;
> +
> +#ifdef CONFIG_IMA_KEXEC
> +phys_addr_t ima_buffer_addr;
> +size_t ima_buffer_size;
> +#endif
> >>>
> >>> Why do these need to be in the arch structure instead of 'struct kimage'?
> >>>
> >>
> >> Currently, only powerpc and, with this patch set, arm64 have support for
> >> carrying forward IMA measurement list across kexec system call. The above
> >> fields are used for tracking IMA measurement list.
> >>
> >> Do you see a reason to move these fields to "struct kimage"?
> >
> > If they're gated on CONFIG_IMA_KEXEC, then it seems harmless for them to
> > be added to the shared structure. Or are you saying that there are
> > architectures which have CONFIG_IMA_KEXEC but do not want these fields?
> >
>
> As far as I know, there are no other architectures that define
> CONFIG_IMA_KEXEC, but do not use these fields.
Yes, CONFIG_IMA_KEXEC enables "carrying the IMA measurement list across
a soft boot". The only arch that currently carries the IMA
measurement across kexec is powerpc.
Mimi
Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
[Cc'ing linux-integrity] On Wed, 2021-01-27 at 11:46 +, David Howells wrote: > Jarkko Sakkinen wrote: > > > > I suppose a user space tool could be created. But wouldn’t what is > > > currently done in the kernel in this area need to be removed? > > > > Right. I don't think this was a great idea in the first place to > > do to the kernel but since it exists, I guess the patch does make > > sense. > > This information needs to be loaded from the UEFI tables before the system > starts loading any kernel modules or running any programs (if we do > verification of such, which I think IMA can do). There needs to a clear distinction between the pre-boot and post-boot keys. UEFI has its own trust model, which should be limited to UEFI. The .platform keyring was upstreamed and limited to verifying the kexec kernel image. Any other usage of the .platform keyring keys is abusing its intended purpose. The cover letter says, "Anytime the .platform keyring is used, the keys in the .blacklist keyring are referenced, if a matching key is found, the key will be rejected." I don't have a problem with loading the UEFI X509 dbx entries as long as its usage is limited to verifying the kexec kernel image. Mimi
Re: [PATCH v3] IMA: Measure kernel version in early boot
On Mon, 2021-01-25 at 16:50 -0800, Raphael Gianotti wrote: > The integrity of a kernel can be verified by the boot loader on cold > boot, and during kexec, by the current running kernel, before it is > loaded. However, it is still possible that the new kernel being > loaded is older than the current kernel, and/or has known > vulnerabilities. Therefore, it is imperative that an attestation > service be able to verify the version of the kernel being loaded on > the client, from cold boot and subsequent kexec system calls, > ensuring that only kernels with versions known to be good are loaded. > > Measure the kernel version using ima_measure_critical_data() early on > in the boot sequence, reducing the chances of known kernel > vulnerabilities being exploited. With IMA being part of the kernel, > this overall approach makes the measurement itself more trustworthy. > > To enable measuring the kernel version "ima_policy=critical_data" > needs to be added to the kernel command line arguments. > For example, > BOOT_IMAGE=/boot/vmlinuz-5.11.0-rc3+ > root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset > ima_policy=critical_data > > If runtime measurement of the kernel version is ever needed, the > following should be added to /etc/ima/ima-policy: > > measure func=CRITICAL_DATA label=kernel_info > > To extract the measured data after boot, the following command can be used: > > grep -m 1 "kernel_version" \ > /sys/kernel/security/integrity/ima/ascii_runtime_measurements > > Sample output from the command above: > > 10 a8297d408e9d5155728b619761d0dd4cedf5ef5f ima-buf > > sha256:5660e19945be0119bc19cbbf8d9c33a09935ab5d30dad48aa11f879c67d70988 > kernel_version > 352e31312e302d7263332d31363138372d676564623634666537383234342d6469727479 > > The above corresponds to the following (decoded) version string: Instead of the above, the following is clearer. The above hex-ascii string corresponds to the kernel version (e.g. xxd -r -p): > > 5.11.0-rc3-16187-gedb64fe78244-dirty > > Signed-off-by: Raphael Gianotti Assuming the above or similar change, Signed-off-by: Mimi Zohar
Re: [PATCH v2] IMA: Measure kernel version in early boot
On Fri, 2021-01-22 at 15:28 -0800, Raphael Gianotti wrote:
> The integrity of a kernel can be verified by the boot loader on cold
> boot, and during kexec, by the current running kernel, before it is
> loaded. However, it is still possible that the new kernel being
> loaded is older than the current kernel, and/or has known
> vulnerabilities. Therefore, it is imperative that an attestation
> service be able to verify the version of the kernel being loaded on
> the client, from cold boot and subsequent kexec system calls,
> ensuring that only kernels with versions known to be good are loaded.
>
> Measure the kernel version using ima_measure_critical_data() early on
> in the boot sequence, reducing the chances of known kernel
> vulnerabilities being exploited. With IMA being part of the kernel,
> this overall approach makes the measurement itself more trustworthy.
>
> To enable measuring the kernel version "ima_policy=critical_data"
> needs to be added to the kernel command line arguments.
> For example,
> BOOT_IMAGE=/boot/vmlinuz-5.11.0-rc3+
> root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset
> ima_policy=critical_data
>
> If runtime measurement of the kernel version is ever needed, the
> following should be added to /etc/ima/ima-policy:
>
> measure func=CRITICAL_DATA label=kernel_version
>
> To extract the measured data after boot, the following command can be used:
>
> grep -m 1 "kernel_version" \
> /sys/kernel/security/integrity/ima/ascii_runtime_measurements
>
> Sample output from the command above:
>
> 10 a8297d408e9d5155728b619761d0dd4cedf5ef5f ima-buf
> sha256:5660e19945be0119bc19cbbf8d9c33a09935ab5d30dad48aa11f879c67d70988
> kernel_version
> 352e31312e302d7263332d31363138372d676564623634666537383234342d6469727479
>
> The above corresponds to the following (decoded) version string:
>
> 5.11.0-rc3-16187-gedb64fe78244-dirty
>
> This patch is based on
> commit e58bb688f2e4 "Merge branch 'measure-critical-data' into next-integrity"
> in "next-integrity-testing" branch
>
> Change Log v2:
> - Changed the measurement to align with the latest version of
> ima_measure_critical_data(), without the need for queueing
> - Scoped the measurement to only measure the kernel version,
> found in UTS_RELEASE, instead of the entire linux_banner
> string
>
> Signed-off-by: Raphael Gianotti
> ---
> security/integrity/ima/ima_main.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index 6a429846f90a..0a33f570725c 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -26,6 +26,7 @@
> #include
> #include
> #include
> +#include
>
> #include "ima.h"
>
> @@ -994,8 +995,11 @@ static int __init init_ima(void)
> if (error)
> pr_warn("Couldn't register LSM notifier, error %d\n", error);
>
> - if (!error)
> + if (!error) {
> ima_update_policy_flag();
> + ima_measure_critical_data("kernel_version", "kernel_version",
> + UTS_RELEASE, strlen(UTS_RELEASE),
> false);
> + }
>
> return error;
> }
Consider defining a new critical data label grouping (e.g.
"kernel_info", ...).
Please move ima_measure_critical_data() to ima_init() and update the
critical data "label:=" in Documentation/ABI/testing/ima_policy.
thanks,
Mimi
Re: [PATCH] selinux: include a consumer of the new IMA critical data hook
On Fri, 2021-01-22 at 15:24 -0500, Paul Moore wrote: > On Thu, Jan 14, 2021 at 2:15 PM Lakshmi Ramasubramanian > wrote: > > > > SELinux stores the active policy in memory, so the changes to this data > > at runtime would have an impact on the security guarantees provided > > by SELinux. Measuring in-memory SELinux policy through IMA subsystem > > provides a secure way for the attestation service to remotely validate > > the policy contents at runtime. > > > > Measure the hash of the loaded policy by calling the IMA hook > > ima_measure_critical_data(). Since the size of the loaded policy > > can be large (several MB), measure the hash of the policy instead of > > the entire policy to avoid bloating the IMA log entry. > > > > To enable SELinux data measurement, the following steps are required: > > > > 1, Add "ima_policy=critical_data" to the kernel command line arguments > >to enable measuring SELinux data at boot time. > > For example, > > BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ > > root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset > > security=selinux ima_policy=critical_data > > > > 2, Add the following rule to /etc/ima/ima-policy > >measure func=CRITICAL_DATA label=selinux > > > > Sample measurement of the hash of SELinux policy: > > > > To verify the measured data with the current SELinux policy run > > the following commands and verify the output hash values match. > > > > sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1 > > > > grep "selinux-policy-hash" > > /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail -1 | > > cut -d' ' -f 6 > > > > Note that the actual verification of SELinux policy would require loading > > the expected policy into an identical kernel on a pristine/known-safe > > system and run the sha256sum /sys/kernel/selinux/policy there to get > > the expected hash. > > > > Signed-off-by: Lakshmi Ramasubramanian > > Suggested-by: Stephen Smalley > > Acked-by: Paul Moore > > Reviewed-by: Tyler Hicks > > --- > > Documentation/ABI/testing/ima_policy | 3 +- > > security/selinux/Makefile| 2 + > > security/selinux/ima.c | 44 +++ > > security/selinux/include/ima.h | 24 +++ > > security/selinux/include/security.h | 3 +- > > security/selinux/ss/services.c | 64 > > 6 files changed, 129 insertions(+), 11 deletions(-) > > create mode 100644 security/selinux/ima.c > > create mode 100644 security/selinux/include/ima.h > > Hi Mimi, > > Just checking as I didn't see a reply to this from you in my inbox, > you merged this into the IMA linux-next branch, yes? The patches are first staged in the linux-integrity #next-integrity- testing branch, before being staged in the #next-integrity branch, which is picked up by linux-next. Sorry, they've been staged in the next-integrity-testing branch, but are now next-integrity. Mim
Re: [PATCH v15 09/10] arm64: Call kmalloc() to allocate DTB buffer
Hi Ard,
On Fri, 2021-01-15 at 09:30 -0800, Lakshmi Ramasubramanian wrote:
> create_dtb() function allocates kernel virtual memory for
> the device tree blob (DTB). This is not consistent with other
> architectures, such as powerpc, which calls kmalloc() for allocating
> memory for the DTB.
>
> Call kmalloc() to allocate memory for the DTB, and kfree() to free
> the allocated memory.
The vmalloc() function description says, "vmalloc - allocate virtually
contiguous memory". I'd appreciate your reviewing this patch, in
particular, which replaces vmalloc() with kmalloc().
thanks,
Mimi
>
> Co-developed-by: Prakhar Srivastava
> Signed-off-by: Prakhar Srivastava
> Signed-off-by: Lakshmi Ramasubramanian
> ---
> arch/arm64/kernel/machine_kexec_file.c | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/kernel/machine_kexec_file.c
> b/arch/arm64/kernel/machine_kexec_file.c
> index 7de9c47dee7c..51c40143d6fa 100644
> --- a/arch/arm64/kernel/machine_kexec_file.c
> +++ b/arch/arm64/kernel/machine_kexec_file.c
> @@ -29,7 +29,7 @@ const struct kexec_file_ops * const kexec_file_loaders[] = {
>
> int arch_kimage_file_post_load_cleanup(struct kimage *image)
> {
> - vfree(image->arch.dtb);
> + kfree(image->arch.dtb);
> image->arch.dtb = NULL;
>
> vfree(image->arch.elf_headers);
> @@ -59,19 +59,21 @@ static int create_dtb(struct kimage *image,
> + cmdline_len + DTB_EXTRA_SPACE;
>
> for (;;) {
> - buf = vmalloc(buf_size);
> + buf = kmalloc(buf_size, GFP_KERNEL);
> if (!buf)
> return -ENOMEM;
>
> /* duplicate a device tree blob */
> ret = fdt_open_into(initial_boot_params, buf, buf_size);
> - if (ret)
> + if (ret) {
> + kfree(buf);
> return -EINVAL;
> + }
>
> ret = of_kexec_setup_new_fdt(image, buf, initrd_load_addr,
>initrd_len, cmdline);
> if (ret) {
> - vfree(buf);
> + kfree(buf);
> if (ret == -ENOMEM) {
> /* unlikely, but just in case */
> buf_size += DTB_EXTRA_SPACE;
> @@ -217,6 +219,6 @@ int load_other_segments(struct kimage *image,
> return 0;
>
> out_err:
> - vfree(dtb);
> + kfree(dtb);
> return ret;
> }
Re: [PATCH v3 09/10] certs: Allow root user to append signed hashes to the blacklist keyring
Hi Mickaël, On Thu, 2021-01-14 at 16:19 +0100, Mickaël Salaün wrote: > From: Mickaël Salaün > > Add a kernel option SYSTEM_BLACKLIST_AUTH_UPDATE to enable the root user > to dynamically add new keys to the blacklist keyring. This enables to > invalidate new certificates, either from being loaded in a keyring, or > from being trusted in a PKCS#7 certificate chain. This also enables to > add new file hashes to be denied by the integrity infrastructure. > > Being able to untrust a certificate which could have normaly been > trusted is a sensitive operation. This is why adding new hashes to the > blacklist keyring is only allowed when these hashes are signed and > vouched by the builtin trusted keyring. A blacklist hash is stored as a > key description. The PKCS#7 signature of this description must be > provided as the key payload. > > Marking a certificate as untrusted should be enforced while the system > is running. It is then forbiden to remove such blacklist keys. > > Update blacklist keyring and blacklist key access rights: > * allows the root user to search for a specific blacklisted hash, which > make sense because the descriptions are already viewable; > * forbids key update; > * restricts kernel rights on the blacklist keyring to align with the > root user rights. > > See the help in tools/certs/print-cert-tbs-hash.sh provided by a > following commit. The design looks good. I'm hoping to review/test at least this patch next week. thanks, Mimi
Re: [PATCH v10 0/8] IMA: support for measuring kernel integrity critical data
On Thu, 2021-01-07 at 20:07 -0800, Tushar Sugandhi wrote: > IMA measures files and buffer data such as keys, command-line arguments > passed to the kernel on kexec system call, etc. While these measurements > are necessary for monitoring and validating the integrity of the system, > they are not sufficient. Various data structures, policies, and states > stored in kernel memory also impact the integrity of the system. > Several kernel subsystems contain such integrity critical data - > e.g. LSMs like SELinux, AppArmor etc. or device-mapper targets like > dm-crypt, dm-verity, dm-integrity etc. These kernel subsystems help > protect the integrity of a system. Their integrity critical data is not > expected to change frequently during run-time. Some of these structures > cannot be defined as __ro_after_init, because they are initialized later. > > For a given system, various external services/infrastructure tools > (including the attestation service) interact with it - both during the > setup and during rest of the system run-time. They share sensitive data > and/or execute critical workload on that system. The external services > may want to verify the current run-time state of the relevant kernel > subsystems before fully trusting the system with business critical > data/workload. For instance, verifying that SELinux is in "enforce" mode > along with the expected policy, disks are encrypted with a certain > configuration, secure boot is enabled etc. > > This series provides the necessary IMA functionality for kernel > subsystems to ensure their configuration can be measured: > - by kernel subsystems themselves, > - in a tamper resistant way, > - and re-measured - triggered on state/configuration change. > > This patch set: > - defines a new IMA hook ima_measure_critical_data() to measure > integrity critical data, > - limits the critical data being measured based on a label, > - defines a builtin critical data measurement policy, > - and includes an SELinux consumer of the new IMA critical data hook. Thanks Tushar, Lakshmi. This patch set is queued in the next- integrity-testing branch. Mimi
Re: [PATCH v10 8/8] selinux: include a consumer of the new IMA critical data hook
On Thu, 2021-01-14 at 11:44 -0500, Mimi Zohar wrote: > [Cc'ing Sasha] > > Hi Lakshmi, > > On Thu, 2021-01-14 at 08:22 -0800, Lakshmi Ramasubramanian wrote: > > On 1/13/21 6:49 PM, Mimi Zohar wrote: > > > >>> Lakshmi is trying to address the situation where an event changes a > > >>> value, but then is restored to the original value. The original and > > >>> subsequent events are measured, but restoring to the original value > > >>> isn't re-measured. This isn't any different than when a file is > > >>> modified and then reverted. > > >>> > > >>> Instead of changing the name like this, which doesn't work for files, > > >>> allowing duplicate measurements should be generic, based on policy. > > >> > > >> Perhaps it is just the end of the day and I'm a bit tired, but I just > > >> read all of the above and I have no idea what your current thoughts > > >> are regarding this patch. > > > > > > Other than appending the timestamp, which is a hack, the patch is fine. > > > Support for re-measuring an event can be upstreamed independently. > > > > > > > Thanks for clarifying the details related to duplicate measurement > > detection and re-measuring. > > > > I will keep the timestamp for the time being, even though its a hack, as > > it helps with re-measuring state changes in SELinux. We will add support > > for "policy driven" re-measurement as a subsequent patch series. > > Once including the timestamp is upstreamed, removing it will be > difficult, especially if different userspace applications are dependent > on it. Unless everyone is on board that removing the timestamp > wouldn't be considered a regression, it cannot be upstreamed. Feel free to just re-post just this one patch. Otherwise the patch set looks good. thanks, Mimi
Re: [PATCH v10 8/8] selinux: include a consumer of the new IMA critical data hook
[Cc'ing Sasha] Hi Lakshmi, On Thu, 2021-01-14 at 08:22 -0800, Lakshmi Ramasubramanian wrote: > On 1/13/21 6:49 PM, Mimi Zohar wrote: > >>> Lakshmi is trying to address the situation where an event changes a > >>> value, but then is restored to the original value. The original and > >>> subsequent events are measured, but restoring to the original value > >>> isn't re-measured. This isn't any different than when a file is > >>> modified and then reverted. > >>> > >>> Instead of changing the name like this, which doesn't work for files, > >>> allowing duplicate measurements should be generic, based on policy. > >> > >> Perhaps it is just the end of the day and I'm a bit tired, but I just > >> read all of the above and I have no idea what your current thoughts > >> are regarding this patch. > > > > Other than appending the timestamp, which is a hack, the patch is fine. > > Support for re-measuring an event can be upstreamed independently. > > > > Thanks for clarifying the details related to duplicate measurement > detection and re-measuring. > > I will keep the timestamp for the time being, even though its a hack, as > it helps with re-measuring state changes in SELinux. We will add support > for "policy driven" re-measurement as a subsequent patch series. Once including the timestamp is upstreamed, removing it will be difficult, especially if different userspace applications are dependent on it. Unless everyone is on board that removing the timestamp wouldn't be considered a regression, it cannot be upstreamed. thanks, Mimi
Re: [PATCH v10 8/8] selinux: include a consumer of the new IMA critical data hook
On Wed, 2021-01-13 at 21:40 -0500, Paul Moore wrote: > On Wed, Jan 13, 2021 at 6:11 PM Mimi Zohar wrote: > > On Wed, 2021-01-13 at 17:10 -0500, Paul Moore wrote: > > > On Wed, Jan 13, 2021 at 4:11 PM Mimi Zohar wrote: > > > > On Wed, 2021-01-13 at 14:19 -0500, Paul Moore wrote: > > > > > On Wed, Jan 13, 2021 at 2:13 PM Mimi Zohar > > > > > wrote: > > > > > > On Tue, 2021-01-12 at 11:27 -0500, Paul Moore wrote: > > > > > > > On Thu, Jan 7, 2021 at 11:07 PM Tushar Sugandhi > > > > > > > wrote: > > > > > > > > From: Lakshmi Ramasubramanian > > > > > > > > > > > > > > > > SELinux stores the active policy in memory, so the changes to > > > > > > > > this data > > > > > > > > at runtime would have an impact on the security guarantees > > > > > > > > provided > > > > > > > > by SELinux. Measuring in-memory SELinux policy through IMA > > > > > > > > subsystem > > > > > > > > provides a secure way for the attestation service to remotely > > > > > > > > validate > > > > > > > > the policy contents at runtime. > > > > > > > > > > > > > > > > Measure the hash of the loaded policy by calling the IMA hook > > > > > > > > ima_measure_critical_data(). Since the size of the loaded > > > > > > > > policy > > > > > > > > can be large (several MB), measure the hash of the policy > > > > > > > > instead of > > > > > > > > the entire policy to avoid bloating the IMA log entry. > > > > > > > > > > > > > > > > To enable SELinux data measurement, the following steps are > > > > > > > > required: > > > > > > > > > > > > > > > > 1, Add "ima_policy=critical_data" to the kernel command line > > > > > > > > arguments > > > > > > > >to enable measuring SELinux data at boot time. > > > > > > > > For example, > > > > > > > > BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ > > > > > > > > root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset > > > > > > > > security=selinux ima_policy=critical_data > > > > > > > > > > > > > > > > 2, Add the following rule to /etc/ima/ima-policy > > > > > > > >measure func=CRITICAL_DATA label=selinux > > > > > > > > > > > > > > > > Sample measurement of the hash of SELinux policy: > > > > > > > > > > > > > > > > To verify the measured data with the current SELinux policy run > > > > > > > > the following commands and verify the output hash values match. > > > > > > > > > > > > > > > > sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1 > > > > > > > > > > > > > > > > grep "selinux-policy-hash" > > > > > > > > /sys/kernel/security/integrity/ima/ascii_runtime_measurements | > > > > > > > > tail -1 | cut -d' ' -f 6 > > > > > > > > > > > > > > > > Note that the actual verification of SELinux policy would > > > > > > > > require loading > > > > > > > > the expected policy into an identical kernel on a > > > > > > > > pristine/known-safe > > > > > > > > system and run the sha256sum /sys/kernel/selinux/policy there > > > > > > > > to get > > > > > > > > the expected hash. > > > > > > > > > > > > > > > > Signed-off-by: Lakshmi Ramasubramanian > > > > > > > > > > > > > > > > Suggested-by: Stephen Smalley > > > > > > > > Reviewed-by: Tyler Hicks > > > > > > > > --- > > > > > > > > Documentation/ABI/testing/ima_policy | 3 +- > > > > > > > > security/selinux/Makefile| 2 + > > > > > > > > security/selinux/ima.c | 64 > > > > > > > > > > > >
Re: [PATCH v10 8/8] selinux: include a consumer of the new IMA critical data hook
On Wed, 2021-01-13 at 14:19 -0500, Paul Moore wrote: > On Wed, Jan 13, 2021 at 2:13 PM Mimi Zohar wrote: > > On Tue, 2021-01-12 at 11:27 -0500, Paul Moore wrote: > > > On Thu, Jan 7, 2021 at 11:07 PM Tushar Sugandhi > > > wrote: > > > > From: Lakshmi Ramasubramanian > > > > > > > > SELinux stores the active policy in memory, so the changes to this data > > > > at runtime would have an impact on the security guarantees provided > > > > by SELinux. Measuring in-memory SELinux policy through IMA subsystem > > > > provides a secure way for the attestation service to remotely validate > > > > the policy contents at runtime. > > > > > > > > Measure the hash of the loaded policy by calling the IMA hook > > > > ima_measure_critical_data(). Since the size of the loaded policy > > > > can be large (several MB), measure the hash of the policy instead of > > > > the entire policy to avoid bloating the IMA log entry. > > > > > > > > To enable SELinux data measurement, the following steps are required: > > > > > > > > 1, Add "ima_policy=critical_data" to the kernel command line arguments > > > >to enable measuring SELinux data at boot time. > > > > For example, > > > > BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ > > > > root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset > > > > security=selinux ima_policy=critical_data > > > > > > > > 2, Add the following rule to /etc/ima/ima-policy > > > >measure func=CRITICAL_DATA label=selinux > > > > > > > > Sample measurement of the hash of SELinux policy: > > > > > > > > To verify the measured data with the current SELinux policy run > > > > the following commands and verify the output hash values match. > > > > > > > > sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1 > > > > > > > > grep "selinux-policy-hash" > > > > /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail -1 > > > > | cut -d' ' -f 6 > > > > > > > > Note that the actual verification of SELinux policy would require > > > > loading > > > > the expected policy into an identical kernel on a pristine/known-safe > > > > system and run the sha256sum /sys/kernel/selinux/policy there to get > > > > the expected hash. > > > > > > > > Signed-off-by: Lakshmi Ramasubramanian > > > > Suggested-by: Stephen Smalley > > > > Reviewed-by: Tyler Hicks > > > > --- > > > > Documentation/ABI/testing/ima_policy | 3 +- > > > > security/selinux/Makefile| 2 + > > > > security/selinux/ima.c | 64 > > > > security/selinux/include/ima.h | 24 +++ > > > > security/selinux/include/security.h | 3 +- > > > > security/selinux/ss/services.c | 64 > > > > 6 files changed, 149 insertions(+), 11 deletions(-) > > > > create mode 100644 security/selinux/ima.c > > > > create mode 100644 security/selinux/include/ima.h > > > > > > I remain concerned about the possibility of bypassing a measurement by > > > tampering with the time, but I appear to be the only one who is > > > worried about this so I'm not going to block this patch on those > > > grounds. > > > > > > Acked-by: Paul Moore > > > > Thanks, Paul. > > > > Including any unique string would cause the buffer hash to change, > > forcing a new measurement. Perhaps they were concerned with > > overflowing a counter. > > My understanding is that Lakshmi wanted to force a new measurement > each time and felt using a timestamp would be the best way to do that. > A counter, even if it wraps, would have a different value each time > whereas a timestamp is vulnerable to time adjustments. While a > properly controlled and audited system could be configured and > monitored to detect such an event (I *think*), why rely on that if it > isn't necessary? Why are you saying that even if the counter wraps a new measurement is guaranteed. I agree with the rest of what you said. Mimi
Re: [PATCH v10 5/8] IMA: limit critical data measurement based on a label
On Thu, 2021-01-07 at 20:07 -0800, Tushar Sugandhi wrote: > Integrity critical data may belong to a single subsystem or it may > arise from cross subsystem interaction. Currently there is no mechanism > to group or limit the data based on certain label. Limiting and > grouping critical data based on a label would make it flexible and > configurable to measure. > > Define "label:=", a new IMA policy condition, for the IMA func > CRITICAL_DATA to allow grouping and limiting measurement of integrity > critical data. > > Limit the measurement to the labels that are specified in the IMA > policy - CRITICAL_DATA+"label:=". If "label:=" is not provided with > the func CRITICAL_DATA, measure all the input integrity critical data. > > Signed-off-by: Tushar Sugandhi > Reviewed-by: Tyler Hicks This is looking a lot better. thanks, Mimi
Re: [PATCH v10 8/8] selinux: include a consumer of the new IMA critical data hook
On Wed, 2021-01-13 at 17:10 -0500, Paul Moore wrote: > On Wed, Jan 13, 2021 at 4:11 PM Mimi Zohar wrote: > > On Wed, 2021-01-13 at 14:19 -0500, Paul Moore wrote: > > > On Wed, Jan 13, 2021 at 2:13 PM Mimi Zohar wrote: > > > > On Tue, 2021-01-12 at 11:27 -0500, Paul Moore wrote: > > > > > On Thu, Jan 7, 2021 at 11:07 PM Tushar Sugandhi > > > > > wrote: > > > > > > From: Lakshmi Ramasubramanian > > > > > > > > > > > > SELinux stores the active policy in memory, so the changes to this > > > > > > data > > > > > > at runtime would have an impact on the security guarantees provided > > > > > > by SELinux. Measuring in-memory SELinux policy through IMA > > > > > > subsystem > > > > > > provides a secure way for the attestation service to remotely > > > > > > validate > > > > > > the policy contents at runtime. > > > > > > > > > > > > Measure the hash of the loaded policy by calling the IMA hook > > > > > > ima_measure_critical_data(). Since the size of the loaded policy > > > > > > can be large (several MB), measure the hash of the policy instead of > > > > > > the entire policy to avoid bloating the IMA log entry. > > > > > > > > > > > > To enable SELinux data measurement, the following steps are > > > > > > required: > > > > > > > > > > > > 1, Add "ima_policy=critical_data" to the kernel command line > > > > > > arguments > > > > > >to enable measuring SELinux data at boot time. > > > > > > For example, > > > > > > BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ > > > > > > root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset > > > > > > security=selinux ima_policy=critical_data > > > > > > > > > > > > 2, Add the following rule to /etc/ima/ima-policy > > > > > >measure func=CRITICAL_DATA label=selinux > > > > > > > > > > > > Sample measurement of the hash of SELinux policy: > > > > > > > > > > > > To verify the measured data with the current SELinux policy run > > > > > > the following commands and verify the output hash values match. > > > > > > > > > > > > sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1 > > > > > > > > > > > > grep "selinux-policy-hash" > > > > > > /sys/kernel/security/integrity/ima/ascii_runtime_measurements | > > > > > > tail -1 | cut -d' ' -f 6 > > > > > > > > > > > > Note that the actual verification of SELinux policy would require > > > > > > loading > > > > > > the expected policy into an identical kernel on a > > > > > > pristine/known-safe > > > > > > system and run the sha256sum /sys/kernel/selinux/policy there to get > > > > > > the expected hash. > > > > > > > > > > > > Signed-off-by: Lakshmi Ramasubramanian > > > > > > Suggested-by: Stephen Smalley > > > > > > Reviewed-by: Tyler Hicks > > > > > > --- > > > > > > Documentation/ABI/testing/ima_policy | 3 +- > > > > > > security/selinux/Makefile| 2 + > > > > > > security/selinux/ima.c | 64 > > > > > > > > > > > > security/selinux/include/ima.h | 24 +++ > > > > > > security/selinux/include/security.h | 3 +- > > > > > > security/selinux/ss/services.c | 64 > > > > > > > > > > > > 6 files changed, 149 insertions(+), 11 deletions(-) > > > > > > create mode 100644 security/selinux/ima.c > > > > > > create mode 100644 security/selinux/include/ima.h > > > > > > > > > > I remain concerned about the possibility of bypassing a measurement by > > > > > tampering with the time, but I appear to be the only one who is > > > > > worried about this so I'm not going to block this patch on those > > > > > grounds. > > > > > > > > > > Acked-by: Paul Moore > > > > > > >
Re: [PATCH v10 8/8] selinux: include a consumer of the new IMA critical data hook
On Tue, 2021-01-12 at 11:27 -0500, Paul Moore wrote:
> On Thu, Jan 7, 2021 at 11:07 PM Tushar Sugandhi
> wrote:
> > From: Lakshmi Ramasubramanian
> >
> > SELinux stores the active policy in memory, so the changes to this data
> > at runtime would have an impact on the security guarantees provided
> > by SELinux. Measuring in-memory SELinux policy through IMA subsystem
> > provides a secure way for the attestation service to remotely validate
> > the policy contents at runtime.
> >
> > Measure the hash of the loaded policy by calling the IMA hook
> > ima_measure_critical_data(). Since the size of the loaded policy
> > can be large (several MB), measure the hash of the policy instead of
> > the entire policy to avoid bloating the IMA log entry.
> >
> > To enable SELinux data measurement, the following steps are required:
> >
> > 1, Add "ima_policy=critical_data" to the kernel command line arguments
> >to enable measuring SELinux data at boot time.
> > For example,
> > BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+
> > root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset
> > security=selinux ima_policy=critical_data
> >
> > 2, Add the following rule to /etc/ima/ima-policy
> >measure func=CRITICAL_DATA label=selinux
> >
> > Sample measurement of the hash of SELinux policy:
> >
> > To verify the measured data with the current SELinux policy run
> > the following commands and verify the output hash values match.
> >
> > sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1
> >
> > grep "selinux-policy-hash"
> > /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail -1 |
> > cut -d' ' -f 6
> >
> > Note that the actual verification of SELinux policy would require loading
> > the expected policy into an identical kernel on a pristine/known-safe
> > system and run the sha256sum /sys/kernel/selinux/policy there to get
> > the expected hash.
> >
> > Signed-off-by: Lakshmi Ramasubramanian
> > Suggested-by: Stephen Smalley
> > Reviewed-by: Tyler Hicks
> > ---
> > Documentation/ABI/testing/ima_policy | 3 +-
> > security/selinux/Makefile| 2 +
> > security/selinux/ima.c | 64
> > security/selinux/include/ima.h | 24 +++
> > security/selinux/include/security.h | 3 +-
> > security/selinux/ss/services.c | 64
> > 6 files changed, 149 insertions(+), 11 deletions(-)
> > create mode 100644 security/selinux/ima.c
> > create mode 100644 security/selinux/include/ima.h
>
> I remain concerned about the possibility of bypassing a measurement by
> tampering with the time, but I appear to be the only one who is
> worried about this so I'm not going to block this patch on those
> grounds.
>
> Acked-by: Paul Moore
Thanks, Paul.
Including any unique string would cause the buffer hash to change,
forcing a new measurement. Perhaps they were concerned with
overflowing a counter.
Mimi
> > +/*
> > + * selinux_ima_measure_state - Measure hash of the SELinux policy
> > + *
> > + * @state: selinux state struct
> > + *
> > + * NOTE: This function must be called with policy_mutex held.
> > + */
> > +void selinux_ima_measure_state(struct selinux_state *state)
> > +{
> > + struct timespec64 cur_time;
> > + void *policy = NULL;
> > + char *policy_event_name = NULL;
> > + size_t policy_len;
> > + int rc = 0;
> > +
> > + /*
> > +* Measure SELinux policy only after initialization is completed.
> > +*/
> > + if (!selinux_initialized(state))
> > + return;
> > +
> > + /*
> > +* Pass a unique "event_name" to the IMA hook so that IMA subsystem
> > +* will always measure the given data.
> > +*/
> > + ktime_get_real_ts64(_time);
> > + policy_event_name = kasprintf(GFP_KERNEL, "%s-%lld:%09ld",
> > + "selinux-policy-hash",
> > + cur_time.tv_sec, cur_time.tv_nsec);
> > + if (!policy_event_name) {
> > + pr_err("SELinux: %s: event name for policy not
> > allocated.\n",
> > + __func__);
> > + goto out;
> > + }
> > +
Re: [PATCH] [v2] evm: Fix memleak in init_desc
Hi Dinghao,
On Sun, 2021-01-10 at 16:02 +0800, Dinghao Liu wrote:
> When kmalloc() fails, tmp_tfm allocated by
> crypto_alloc_shash() has not been freed, which
> leads to memleak.
>
> Fixes: d46eb3699502b ("evm: crypto hash replaced by shash")
> Signed-off-by: Dinghao Liu
This patch is now queued, with an updated patch description, in next-
integrity-testing.
thanks,
Mimi
Re: [PATCH v14 4/6] powerpc: Delete unused functions
Hi Lakshmi, On Mon, 2021-01-04 at 11:26 -0800, Lakshmi Ramasubramanian wrote: > diff --git a/arch/powerpc/kexec/file_load_64.c > b/arch/powerpc/kexec/file_load_64.c > index a05c19b3cc60..3cab318aa3b9 100644 > --- a/arch/powerpc/kexec/file_load_64.c > +++ b/arch/powerpc/kexec/file_load_64.c > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -944,7 +945,8 @@ int setup_new_fdt_ppc64(const struct kimage *image, void > *fdt, > struct crash_mem *umem = NULL, *rmem = NULL; > int i, nr_ranges, ret; > > - ret = setup_new_fdt(image, fdt, initrd_load_addr, initrd_len, cmdline); > + ret = of_kexec_setup_new_fdt(image, fdt, initrd_load_addr, initrd_len, > + cmdline); > if (ret) > goto out; > The "powerpc: Move arch independent ima kexec functions to drivers/of/kexec.c" moved setup_ima_buffer() to of_kexec_setup_new_fdt(). Defering making the change from setup_new_fdt() to of_kexec_setup_new_fdt() here, is not bisect safe. Mimi
Re: [PATCH v14 6/6] arm64: Add IMA log information in kimage used for kexec
Hi Lakshmi,
On Mon, 2021-01-04 at 11:26 -0800, Lakshmi Ramasubramanian wrote:
> Address and size of the buffer containing the IMA measurement log need
> to be passed from the current kernel to the next kernel on kexec.
>
> Any existing "linux,ima-kexec-buffer" property in the device tree
> needs to be removed and its corresponding memory reservation in
> the currently running kernel needs to be freed. The address and
> size of the current kernel's IMA measurement log need to be added
> to the device tree's IMA kexec buffer node and memory for the buffer
> needs to be reserved for the log to be carried over to the next kernel
> on the kexec call.
>
> Add address and size fields to "struct kimage_arch" for ARM64 platform
> to hold the address and size of the IMA measurement log buffer. Remove
> any existing "linux,ima-kexec-buffer" property in the device tree and
> free the corresponding memory reservation in the currently running
> kernel. Add "linux,ima-kexec-buffer" property to the device tree and
> reserve the memory for storing the IMA log that needs to be passed from
> the current kernel to the next one.
>
> Update CONFIG_KEXEC_FILE to select CONFIG_HAVE_IMA_KEXEC to indicate
> that the IMA measurement log information is present in the device tree
> for ARM64.
Perhaps for some previous version of this patch set, this patch
description was appropriate, but for the code below it's kind of
overkill.
Mimi
>
> Co-developed-by: Prakhar Srivastava
> Signed-off-by: Prakhar Srivastava
> Signed-off-by: Lakshmi Ramasubramanian
> ---
> arch/arm64/Kconfig | 1 +
> arch/arm64/include/asm/kexec.h | 5 +
> 2 files changed, 6 insertions(+)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index a6b5b7ef40ae..312b4d5ad232 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1095,6 +1095,7 @@ config KEXEC
> config KEXEC_FILE
> bool "kexec file based system call"
> select KEXEC_CORE
> + select HAVE_IMA_KEXEC if IMA
> help
> This is new version of kexec system call. This system call is
> file based and takes file descriptors as system call argument
> diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> index d24b527e8c00..2bd19ccb6c43 100644
> --- a/arch/arm64/include/asm/kexec.h
> +++ b/arch/arm64/include/asm/kexec.h
> @@ -100,6 +100,11 @@ struct kimage_arch {
> void *elf_headers;
> unsigned long elf_headers_mem;
> unsigned long elf_headers_sz;
> +
> +#ifdef CONFIG_IMA_KEXEC
> + phys_addr_t ima_buffer_addr;
> + size_t ima_buffer_size;
> +#endif
> };
>
> extern const struct kexec_file_ops kexec_image_ops;
Re: [PATCH v14 0/6] Carry forward IMA measurement log on kexec on ARM64
On Tue, 2021-01-12 at 08:42 -0600, Rob Herring wrote:
> On Mon, Jan 04, 2021 at 11:25:56AM -0800, Lakshmi Ramasubramanian wrote:
> > On kexec file load Integrity Measurement Architecture (IMA) subsystem
> > may verify the IMA signature of the kernel and initramfs, and measure
> > it. The command line parameters passed to the kernel in the kexec call
> > may also be measured by IMA. A remote attestation service can verify
> > a TPM quote based on the TPM event log, the IMA measurement list, and
> > the TPM PCR data. This can be achieved only if the IMA measurement log
> > is carried over from the current kernel to the next kernel across
> > the kexec call.
> >
> > powerpc already supports carrying forward the IMA measurement log on
> > kexec. This patch set adds support for carrying forward the IMA
> > measurement log on kexec on ARM64.
> >
> > This patch set moves the platform independent code defined for powerpc
> > such that it can be reused for other platforms as well. A chosen node
> > "linux,ima-kexec-buffer" is added to the DTB for ARM64 to hold
> > the address and the size of the memory reserved to carry
> > the IMA measurement log.
> >
> > This patch set has been tested for ARM64 platform using QEMU.
> > I would like help from the community for testing this change on powerpc.
> > Thanks.
> >
> > This patch set is based on
> > commit a29a64445089 ("powerpc: Use common of_kexec_setup_new_fdt()")
> > in https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
> > "dt/kexec" branch.
>
> This all looks good to me. I'd suggest you send the above patches out as
> part of this series because I don't plan to do so.
>
> I would like to also resolve the vmalloc vs. kmalloc difference for
> allocating the FDT. Then we can further consolidate the DT kexec code.
>
> It all needs some acks from arm64 and powerpc maintainers. As far as
> merging, I think via the integrity tree makes the most sense.
Thanks, Rob. Lakshmi, please update Rob's patches to include patch
descriptions before re-posting.
Mimi
Re: [PATCH AUTOSEL 5.7 03/30] ima: extend boot_aggregate with kernel measurements
Hi Tyler,
On Tue, 2021-01-12 at 09:35 -0600, Tyler Hicks wrote:
> On 2020-12-14 10:42:24, Tyler Hicks wrote:
> > On 2020-12-11 06:01:54, Mimi Zohar wrote:
> > > On Thu, 2020-12-10 at 21:10 -0600, Tyler Hicks wrote:
> > > > On 2020-11-29 08:17:38, Mimi Zohar wrote:
> > > > > Hi Sasha,
> > > > >
> > > > > On Wed, 2020-07-08 at 21:27 -0400, Sasha Levin wrote:
> > > > > > On Wed, Jul 08, 2020 at 12:13:13PM -0400, Mimi Zohar wrote:
> > > > > > >Hi Sasha,
> > > > > > >
> > > > > > >On Wed, 2020-07-08 at 11:40 -0400, Sasha Levin wrote:
> > > > > > >> From: Maurizio Drocco
> > > > > > >>
> > > > > > >> [ Upstream commit 20c59ce010f84300f6c655d32db2610d3433f85c ]
> > > > > > >>
> > > > > > >> Registers 8-9 are used to store measurements of the kernel and
> > > > > > >> its
> > > > > > >> command line (e.g., grub2 bootloader with tpm module enabled).
> > > > > > >> IMA
> > > > > > >> should include them in the boot aggregate. Registers 8-9 should
> > > > > > >> be
> > > > > > >> only included in non-SHA1 digests to avoid ambiguity.
> > > > > > >
> > > > > > >Prior to Linux 5.8, the SHA1 template data hashes were padded
> > > > > > >before
> > > > > > >being extended into the TPM. Support for calculating and extending
> > > > > > >the per TPM bank template data digests is only being upstreamed in
> > > > > > >Linux 5.8.
> > > > > > >
> > > > > > >How will attestation servers know whether to include PCRs 8 & 9 in
> > > > > > >the
> > > > > > >the boot_aggregate calculation? Now, there is a direct
> > > > > > >relationship
> > > > > > >between the template data SHA1 padded digest not including PCRs 8
> > > > > > >& 9,
> > > > > > >and the new per TPM bank template data digest including them.
> > > > > >
> > > > > > Got it, I'll drop it then, thank you!
> > > > >
> > > > > After re-thinking this over, I realized that the attestation server
> > > > > can
> > > > > verify the "boot_aggregate" based on the quoted PCRs without knowing
> > > > > whether padded SHA1 hashes or per TPM bank hash values were extended
> > > > > into the TPM[1], but non-SHA1 boot aggregate values [2] should always
> > > > > include PCRs 8 & 9.
> > > >
> > > > I'm still not clear on how an attestation server would know to include
> > > > PCRs 8 and 9 after this change came through a stable kernel update. It
> > > > doesn't seem like something appropriate for stable since it requires
> > > > code changes to attestation servers to handle the change.
> > > >
> > > > I know this has already been released in some stable releases, so I'm
> > > > too late, but perhaps I'm missing something.
> > >
> > > The point of adding PCRs 8 & 9 only to non-SHA1 boot_aggregate values
> > > was to avoid affecting existing attestation servers. The intention was
> > > when attestation servers added support for the non-sha1 boot_aggregate
> > > values, they'd also include PCRs 8 & 9. The existing SHA1
> > > boot_aggregate value remains PCRs 0 - 7.
> >
> > AFAIK, there's nothing that prevents the non-SHA1 TPM 2.0 PCR banks from
> > being used even before v5.8, albeit with zero padded SHA1 digests.
> > Existing attestation servers that already support that configuration are
> > broken by this stable backport.
> To wrap up this thread, I think the last thing to address is if this
> commit should be reverted from stable kernels? Do you have any thoughts
> about that, Mimi?
>
> >
> > > To prevent this or something similar from happening again, what should
> > > have been the proper way of including PCRs 8 & 9?
> >
> > I don't think that commits like 6f1a1d103b48 ("ima: Switch to
> > ima_hash_algo for boot aggregate") and 20c59ce010f8 ("ima: extend
> > boot_aggregate with kernel measurements") should be backported to
> > stable.
> >
> > Including PCRs 8 and 9 definitely makes sense to include in the
> > boot_aggregate value but limiting such a change to "starting in 5.8",
> > rather than "starting in 5.8 and 5.4.82", is the safer approach when
> > attestation server modifications are required.
As I recall, commit 6f1a1d103b48 ("ima: Switch to ima_hash_algo for
boot aggregate") was backported to address TPMs without SHA1 support,
as reported by Jerry.
Mimi
Re: [PATCH v2] evm: Fix memory leak in init_desc
Hi Dinghao, On Sun, 2021-01-10 at 11:50 +0100, Markus Elfring wrote: > > When kmalloc() fails, tmp_tfm allocated by > > crypto_alloc_shash() has not been freed, which > > leads to memleak. In the future, please conform to Documentation/process/submitting- patches.rst: - The body of the explanation, line wrapped at 75 columns, which will be copied to the permanent changelog to describe this patch. > > Do any Linux developers care for the following aspects? > > * Imperative wordings for change descriptions > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=2ff90100ace886895e4fbb2850b8d5e49d931ed6#n89 > > * Usage of the term “memory leak” (instead of an abbreviation) In general I agree, but this is a really small, obvious bug fix. Assuming Dinghao is fine with my updating the patch description, I'll fix it. Mimi
Re: [PATCH v9 3/8] IMA: define a hook to measure kernel integrity critical data
On Tue, 2021-01-05 at 12:01 -0800, Tushar Sugandhi wrote:
>
> >> data. However, various data structures, policies, and states
> >
> > Here and everywhere else, there are two blanks after a period.
> >
> I checked this patch file in multiple text editors, but couldn’t find
> any instance of period followed by two spaces. I will double check again
> all the patches for multiple spaces, and remove them if any.
There should be two blanks after a period, not one blank.
> >> + *
> >> + * Measure the kernel subsystem data, critical to the integrity of the
> >> kernel,
> >> + * into the IMA log and extend the @pcr.
> >> + *
> >> + * Use @event_name to describe the state/buffer data change.
> >> + * Examples of critical data (@buf) could be various data structures,
> >> + * policies, and states stored in kernel memory that can impact the
> >> integrity
> >> + * of the system.
> >> + *
> >> + * If @measure_buf_hash is set to true - measure hash of the buffer data,
> >> + * else measure the buffer data itself.
> >> + * @measure_buf_hash can be used to save space, if the data being measured
> >> + * is too large.
> >> + *
> >> + * The data (@buf) can only be measured, not appraised.
> >
> > The "/**" is the start of kernel-doc. Have you seen anywhere else in
> My impression was the hooks in ima_main.c e.g. ima_file_free()
> ima_file_mmap() required the double-asterisk ("/**"), and internal
> functions like ima_rdwr_violation_check() require a single-asterisk
> ("/*")
>
> kernel-doc.rst suggest the double-asterisk ("/**") for function comment
> as well.
>
> Function documentation
> --
>
> The general format of a function and function-like macro kernel-doc
> comment is::
>
>/**
> * function_name() - Brief description of function.
>
> Please let me know if you still want me to remove the double-asterisk
> ("/**") here.
Yes, of course this needs to be kernel-doc and requires "/**"
>
> > the kernel using the @ in the longer function
> > description? Have you seen this style of longer function
> > description? Refer to Documentation/doc-guide/kernel-doc.rst and other
> > code for examples.
> >
> Thanks. I will remove the prefix "@" from in the longer
> function description.
Removing the @ isn't sufficient. Please look at other
examples of longer function definitions before reposting.
thanks,
Mimi
Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.
On Tue, 2020-12-29 at 10:46 -0800, Casey Schaufler wrote:
> >> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void
> >> *lsmrule)
> >> +int security_audit_rule_match(u32 secid, u32 field, u32 op, void
> >> **lsmrule)
> >> {
> >> - return call_int_hook(audit_rule_match, 0, secid, field, op,
> >> lsmrule);
> >> + struct security_hook_list *hp;
> >> + int rc;
> >> +
> >> + hlist_for_each_entry(hp,
> >> _hook_heads.audit_rule_match, list) {
> >> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >=
> >> lsm_slot))
> >> + continue;
> >> + rc = hp->hook.audit_rule_match(secid, field, op,
> >> +
> >> [hp->lsmid->slot]);
> >> + if (rc)
> >> + return rc;
> > Suppose that there is an IMA dont_measure or dont_appraise rule, if one
> > LSM matches, then this returns true, causing any measurement or
> > integrity verification to be skipped.
> Yes, that is correct. Like the audit system, you're doing a string based
> lookup, which pretty well has to work this way. I have proposed compound
> label specifications in the past, but even if we accepted something like
> "apparmor=dates,selinux=figs" we'd still have to be compatible with the
> old style inputs.
>
> > Sample policy rules:
> > dont_measure obj_type=foo_log
> > dont_appraise obj_type=foo_log
> >>> IMA could extend the existing policy rules like "lsm=[selinux] |
> >>> [smack] | [apparmor]", but that assumes that the underlying
> >>> infrastructure supports it.
> >> Yes, but you would still need rational behavior in the
> >> case where someone has old IMA policy rules.
> > From an IMA perspective, allowing multiple LSMs to define the same
> > policy label is worse than requiring the label be constrained to a
> > particular LSM.
>
> Just to be sure we're talking about the same thing,
> the case I'm referring to is something like a file with
> two extended attributes:
>
> security.apparmor MacAndCheese
> security.SMACK64 MacAndCheese
>
> and an IMA rule that says
>
> dont_measure obj_type=MacAndCheese
>
> In this case the dont_measure will be applied to both.
> On the other hand,
>
> security.apparmor MacAndCheese
> security.SMACK64 FranksAndBeans
>
> would also apply the rule to both, which is not
> what you want. Unfortunately, there is no way to
> differentiate which LSM hit the rule.
>
> So now I'm a little confused. The case where both LSMs
> use the same label looks like it works right, where the
> case where they're different doesn't.
I'm more concerned about multiple LSMs using the same label. The
label's meaning is LSM specific.
>
> I'm beginning to think that identifying which LSMs matched
> a rule (it may be none, either or both) is the right solution.
> I don't think that audit is as sensitive to this.
If the label's meaning is LSM specific, then the rule needs to be LSM
specific.
Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.
On Mon, 2020-12-28 at 20:53 -0500, Mimi Zohar wrote:
> On Mon, 2020-12-28 at 15:20 -0800, Casey Schaufler wrote:
> > On 12/28/2020 2:14 PM, Mimi Zohar wrote:
> > > On Mon, 2020-12-28 at 12:06 -0800, Casey Schaufler wrote:
> > >> On 12/28/2020 11:24 AM, Mimi Zohar wrote:
> > >>>> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void
> > >>>> *lsmrule)
> > >>>> +int security_audit_rule_match(u32 secid, u32 field, u32 op, void
> > >>>> **lsmrule)
> > >>>> {
> > >>>> - return call_int_hook(audit_rule_match, 0, secid, field, op,
> > >>>> lsmrule);
> > >>>> + struct security_hook_list *hp;
> > >>>> + int rc;
> > >>>> +
> > >>>> + hlist_for_each_entry(hp,
> > >>>> _hook_heads.audit_rule_match, list) {
> > >>>> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >=
> > >>>> lsm_slot))
> > >>>> + continue;
> > >>>> + rc = hp->hook.audit_rule_match(secid, field, op,
> > >>>> +
> > >>>> [hp->lsmid->slot]);
> > >>>> + if (rc)
> > >>>> + return rc;
> > >>> Suppose that there is an IMA dont_measure or dont_appraise rule, if one
> > >>> LSM matches, then this returns true, causing any measurement or
> > >>> integrity verification to be skipped.
> > >> Yes, that is correct. Like the audit system, you're doing a string based
> > >> lookup, which pretty well has to work this way. I have proposed compound
> > >> label specifications in the past, but even if we accepted something like
> > >> "apparmor=dates,selinux=figs" we'd still have to be compatible with the
> > >> old style inputs.
> > >>
> > >>> Sample policy rules:
> > >>> dont_measure obj_type=foo_log
> > >>> dont_appraise obj_type=foo_log
> > > IMA could extend the existing policy rules like "lsm=[selinux] |
> > > [smack] | [apparmor]", but that assumes that the underlying
> > > infrastructure supports it.
> >
> > Yes, but you would still need rational behavior in the
> > case where someone has old IMA policy rules.
>
> From an IMA perspective, allowing multiple LSMs to define the same
> policy label is worse than requiring the label be constrained to a
> particular LSM.
If allowing multiple LSMs to define the same label is only an IMA
issue, then have security_audit_rule_init() return the number of LSMs
which define the label. IMA is already emitting a warning when an LSM
rule is not defined. Emitting an additional warning would be the
first step.
In addition, ima_parse_rule() could detect policy rules containing non
LSM specific labels. Based on policy, IMA would decide how to handle
it.
thanks,
Mimi
Re: [PATCH AUTOSEL 5.7 03/30] ima: extend boot_aggregate with kernel measurements
On Mon, 2020-12-28 at 14:28 -0500, Ken Goldman wrote: > On 12/12/2020 9:22 PM, Mimi Zohar wrote: > > Ok. Going forward, it sounds like we need to define a new > > "boot_aggregate" record. One that contains a version number and PCR > > mask. > > Just BTW, there is a TCG standard for a TPM 2.0 PCR mask that works > well. Sounds good. > > There is also a standard for an event log version number. It is > the first event of a TPM 2.0 event log. It is strange. Ok > > One useful field, though, is a mapping between the algorithm ID (e.g., > sha256 is 0x000b) and the digest size (e.g., 32 bytes). This permits > a parser to parse a log even when it encounters an unknown digest > algorithm. The template data is prefixed with the template data length. The problem is verifying the boot_aggregate, not parsing the log. thanks, Mimi
Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.
On Mon, 2020-12-28 at 15:20 -0800, Casey Schaufler wrote:
> On 12/28/2020 2:14 PM, Mimi Zohar wrote:
> > On Mon, 2020-12-28 at 12:06 -0800, Casey Schaufler wrote:
> >> On 12/28/2020 11:24 AM, Mimi Zohar wrote:
> >>> Hi Casey,
> >>>
> >>> On Fri, 2020-11-20 at 12:14 -0800, Casey Schaufler wrote:
> >>>> diff --git a/security/security.c b/security/security.c
> >>>> index 5da8b3643680..d01363cb0082 100644
> >>>> --- a/security/security.c
> >>>> +++ b/security/security.c
> >>>>
> >>>> @@ -2510,7 +2526,24 @@ int security_key_getsecurity(struct key *key,
> >>>> char **_buffer)
> >>>>
> >>>> int security_audit_rule_init(u32 field, u32 op, char *rulestr, void
> >>>> **lsmrule)
> >>>> {
> >>>> - return call_int_hook(audit_rule_init, 0, field, op, rulestr,
> >>>> lsmrule);
> >>>> + struct security_hook_list *hp;
> >>>> + bool one_is_good = false;
> >>>> + int rc = 0;
> >>>> + int trc;
> >>>> +
> >>>> + hlist_for_each_entry(hp, _hook_heads.audit_rule_init,
> >>>> list) {
> >>>> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >=
> >>>> lsm_slot))
> >>>> + continue;
> >>>> + trc = hp->hook.audit_rule_init(field, op, rulestr,
> >>>> +
> >>>> [hp->lsmid->slot]);
> >>>> + if (trc == 0)
> >>>> + one_is_good = true;
> >>>> + else
> >>>> + rc = trc;
> >>>> + }
> >>>> + if (one_is_good)
> >>>> + return 0;
> >>>> + return rc;
> >>>> }
> >>> So the same string may be defined by multiple LSMs.
> >> Yes. Any legal AppArmor label would also be a legal Smack label.
> >>
> >>>> int security_audit_rule_known(struct audit_krule *krule)
> >>>> @@ -2518,14 +2551,31 @@ int security_audit_rule_known(struct audit_krule
> >>>> *krule)
> >>>> return call_int_hook(audit_rule_known, 0, krule);
> >>>> }
> >>>>
> >>>> -void security_audit_rule_free(void *lsmrule)
> >>>> +void security_audit_rule_free(void **lsmrule)
> >>>> {
> >>>> - call_void_hook(audit_rule_free, lsmrule);
> >>>> + struct security_hook_list *hp;
> >>>> +
> >>>> + hlist_for_each_entry(hp, _hook_heads.audit_rule_free,
> >>>> list) {
> >>>> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >=
> >>>> lsm_slot))
> >>>> + continue;
> >>>> + hp->hook.audit_rule_free(lsmrule[hp->lsmid->slot]);
> >>>> + }
> >>>> }
> >>>>
> >>> If one LSM frees the string, then the string is deleted from all LSMs.
> >>> I don't understand how this safe.
> >> The audit system doesn't have a way to specify which LSM
> >> a watched label is associated with. Even if we added one,
> >> we'd still have to address the current behavior. Assigning
> >> the watch to all modules means that seeing the string
> >> in any module is sufficient to generate the event.
> > I originally thought loading a new LSM policy could not delete existing
> > LSM labels, but that isn't true. If LSM labels can come and go based
> > on policy, with this code, could loading a new policy for one LSM
> > result in deleting labels of another LSM?
>
> No. I could imagine a situation where changing policy on
> a system where audit rules have been set could result in
> confusion, but that would be true in the single LSM case.
> It would require that secids used in the old policy be
> used for different labels in the new policy. That would
> not be sane behavior. I know it's impossible for Smack.
>
> This is one of the reasons I'm switching from a single secid
> to a collection of secids. You don't want unnatural behavior
> of one LSM to impact the behavior of another.
>
>
> >
> >>>> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void
> >>>> *lsmrule)
&g
Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.
On Mon, 2020-12-28 at 11:22 -0800, Casey Schaufler wrote: > On 12/28/2020 9:54 AM, Mimi Zohar wrote: > > Hi Casey, > > > > On Fri, 2020-11-20 at 12:14 -0800, Casey Schaufler wrote: > >> When more than one security module is exporting data to > >> audit and networking sub-systems a single 32 bit integer > >> is no longer sufficient to represent the data. Add a > >> structure to be used instead. > >> > >> The lsmblob structure is currently an array of > >> u32 "secids". There is an entry for each of the > >> security modules built into the system that would > >> use secids if active. The system assigns the module > >> a "slot" when it registers hooks. If modules are > >> compiled in but not registered there will be unused > >> slots. > >> > >> A new lsm_id structure, which contains the name > >> of the LSM and its slot number, is created. There > >> is an instance for each LSM, which assigns the name > >> and passes it to the infrastructure to set the slot. > >> > >> The audit rules data is expanded to use an array of > >> security module data rather than a single instance. > >> Because IMA uses the audit rule functions it is > >> affected as well. > > This patch is quite large, even without the audit rule change. I would > > limit this patch to the new lsm_id structure changes. The audit rule > > change should be broken out as a separate patch so that the audit > > changes aren't hidden. > > Breaking up the patch in any meaningful way would require > scaffolding code that is as extensive and invasive as the > final change. I can do that if you really need it, but it > won't be any easier to read. Hidden in this patch is the new behavior of security_audit_rule_init(), security_audit_rule_free(), and security_audit_rule_match(). My concern is with label collision. Details are in a subsequent post. Can an LSM prevent label collision? > > > In addition, here are a few high level nits: > > - The (patch description) body of the explanation, line wrapped at 75 > > columns, which will be copied to the permanent changelog to describe > > this patch. (Refer Documentation/process/submitting-patches.rst.) > > Will fix. > > > - The brief kernel-doc descriptions should not have a trailing period. > > Nor should kernel-doc variable definitions have a trailing period. > > Example(s) inline below. (The existing kernel-doc is mostly correct.) > > Will fix. > > > - For some reason existing comments that span multiple lines aren't > > formatted properly. In those cases, where there is another change, > > please fix the comment and function description. > > Can you give an example? There are multiple comment styles > used in the various components. Never mind. All three examples are in tomoyo. > I don't see any comments on the ima code changes. I really > don't want to spin a new patch set that does nothing but change > two periods in comments only to find out two months from now > that the code changes are completely borked. I really don't > want to go through the process of breaking up the patch that has > been widely Acked if there's no reason to expect it would require > significant work otherwise. Understood.
Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.
On Mon, 2020-12-28 at 12:06 -0800, Casey Schaufler wrote:
> On 12/28/2020 11:24 AM, Mimi Zohar wrote:
> > Hi Casey,
> >
> > On Fri, 2020-11-20 at 12:14 -0800, Casey Schaufler wrote:
> >> diff --git a/security/security.c b/security/security.c
> >> index 5da8b3643680..d01363cb0082 100644
> >> --- a/security/security.c
> >> +++ b/security/security.c
> >>
> >> @@ -2510,7 +2526,24 @@ int security_key_getsecurity(struct key *key, char
> >> **_buffer)
> >>
> >> int security_audit_rule_init(u32 field, u32 op, char *rulestr, void
> >> **lsmrule)
> >> {
> >> - return call_int_hook(audit_rule_init, 0, field, op, rulestr,
> >> lsmrule);
> >> + struct security_hook_list *hp;
> >> + bool one_is_good = false;
> >> + int rc = 0;
> >> + int trc;
> >> +
> >> + hlist_for_each_entry(hp, _hook_heads.audit_rule_init,
> >> list) {
> >> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >=
> >> lsm_slot))
> >> + continue;
> >> + trc = hp->hook.audit_rule_init(field, op, rulestr,
> >> + [hp->lsmid->slot]);
> >> + if (trc == 0)
> >> + one_is_good = true;
> >> + else
> >> + rc = trc;
> >> + }
> >> + if (one_is_good)
> >> + return 0;
> >> + return rc;
> >> }
> > So the same string may be defined by multiple LSMs.
>
> Yes. Any legal AppArmor label would also be a legal Smack label.
>
> >> int security_audit_rule_known(struct audit_krule *krule)
> >> @@ -2518,14 +2551,31 @@ int security_audit_rule_known(struct audit_krule
> >> *krule)
> >> return call_int_hook(audit_rule_known, 0, krule);
> >> }
> >>
> >> -void security_audit_rule_free(void *lsmrule)
> >> +void security_audit_rule_free(void **lsmrule)
> >> {
> >> - call_void_hook(audit_rule_free, lsmrule);
> >> + struct security_hook_list *hp;
> >> +
> >> + hlist_for_each_entry(hp, _hook_heads.audit_rule_free,
> >> list) {
> >> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >=
> >> lsm_slot))
> >> + continue;
> >> + hp->hook.audit_rule_free(lsmrule[hp->lsmid->slot]);
> >> + }
> >> }
> >>
> > If one LSM frees the string, then the string is deleted from all LSMs.
> > I don't understand how this safe.
>
> The audit system doesn't have a way to specify which LSM
> a watched label is associated with. Even if we added one,
> we'd still have to address the current behavior. Assigning
> the watch to all modules means that seeing the string
> in any module is sufficient to generate the event.
I originally thought loading a new LSM policy could not delete existing
LSM labels, but that isn't true. If LSM labels can come and go based
on policy, with this code, could loading a new policy for one LSM
result in deleting labels of another LSM?
>
> >
> >> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
> >> +int security_audit_rule_match(u32 secid, u32 field, u32 op, void
> >> **lsmrule)
> >> {
> >> - return call_int_hook(audit_rule_match, 0, secid, field, op,
> >> lsmrule);
> >> + struct security_hook_list *hp;
> >> + int rc;
> >> +
> >> + hlist_for_each_entry(hp, _hook_heads.audit_rule_match,
> >> list) {
> >> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >=
> >> lsm_slot))
> >> + continue;
> >> + rc = hp->hook.audit_rule_match(secid, field, op,
> >> + [hp->lsmid->slot]);
> >> + if (rc)
> >> + return rc;
> > Suppose that there is an IMA dont_measure or dont_appraise rule, if one
> > LSM matches, then this returns true, causing any measurement or
> > integrity verification to be skipped.
>
> Yes, that is correct. Like the audit system, you're doing a string based
> lookup, which pretty well has to work this way. I have proposed compound
> label specifications in the past, but even if we accepted something like
> "apparmor=dates,sel
Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.
Hi Casey,
On Fri, 2020-11-20 at 12:14 -0800, Casey Schaufler wrote:
> diff --git a/security/security.c b/security/security.c
> index 5da8b3643680..d01363cb0082 100644
> --- a/security/security.c
> +++ b/security/security.c
>
> @@ -2510,7 +2526,24 @@ int security_key_getsecurity(struct key *key, char
> **_buffer)
>
> int security_audit_rule_init(u32 field, u32 op, char *rulestr, void
> **lsmrule)
> {
> - return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
> + struct security_hook_list *hp;
> + bool one_is_good = false;
> + int rc = 0;
> + int trc;
> +
> + hlist_for_each_entry(hp, _hook_heads.audit_rule_init, list) {
> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >=
> lsm_slot))
> + continue;
> + trc = hp->hook.audit_rule_init(field, op, rulestr,
> + [hp->lsmid->slot]);
> + if (trc == 0)
> + one_is_good = true;
> + else
> + rc = trc;
> + }
> + if (one_is_good)
> + return 0;
> + return rc;
> }
So the same string may be defined by multiple LSMs.
>
> int security_audit_rule_known(struct audit_krule *krule)
> @@ -2518,14 +2551,31 @@ int security_audit_rule_known(struct audit_krule
> *krule)
> return call_int_hook(audit_rule_known, 0, krule);
> }
>
> -void security_audit_rule_free(void *lsmrule)
> +void security_audit_rule_free(void **lsmrule)
> {
> - call_void_hook(audit_rule_free, lsmrule);
> + struct security_hook_list *hp;
> +
> + hlist_for_each_entry(hp, _hook_heads.audit_rule_free, list) {
> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >=
> lsm_slot))
> + continue;
> + hp->hook.audit_rule_free(lsmrule[hp->lsmid->slot]);
> + }
> }
>
If one LSM frees the string, then the string is deleted from all LSMs.
I don't understand how this safe.
> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
> +int security_audit_rule_match(u32 secid, u32 field, u32 op, void **lsmrule)
> {
> - return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
> + struct security_hook_list *hp;
> + int rc;
> +
> + hlist_for_each_entry(hp, _hook_heads.audit_rule_match, list)
> {
> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >=
> lsm_slot))
> + continue;
> + rc = hp->hook.audit_rule_match(secid, field, op,
> + [hp->lsmid->slot]);
> + if (rc)
> + return rc;
Suppose that there is an IMA dont_measure or dont_appraise rule, if one
LSM matches, then this returns true, causing any measurement or
integrity verification to be skipped.
Sample policy rules:
dont_measure obj_type=foo_log
dont_appraise obj_type=foo_log
Are there any plans to prevent label collisions or at least notify of a
label collision?
Mimi
> + }
> + return 0;
> }
> #endif /* CONFIG_AUDIT */
Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.
Hi Casey,
On Fri, 2020-11-20 at 12:14 -0800, Casey Schaufler wrote:
> When more than one security module is exporting data to
> audit and networking sub-systems a single 32 bit integer
> is no longer sufficient to represent the data. Add a
> structure to be used instead.
>
> The lsmblob structure is currently an array of
> u32 "secids". There is an entry for each of the
> security modules built into the system that would
> use secids if active. The system assigns the module
> a "slot" when it registers hooks. If modules are
> compiled in but not registered there will be unused
> slots.
>
> A new lsm_id structure, which contains the name
> of the LSM and its slot number, is created. There
> is an instance for each LSM, which assigns the name
> and passes it to the infrastructure to set the slot.
>
> The audit rules data is expanded to use an array of
> security module data rather than a single instance.
> Because IMA uses the audit rule functions it is
> affected as well.
This patch is quite large, even without the audit rule change. I would
limit this patch to the new lsm_id structure changes. The audit rule
change should be broken out as a separate patch so that the audit
changes aren't hidden.
In addition, here are a few high level nits:
- The (patch description) body of the explanation, line wrapped at 75
columns, which will be copied to the permanent changelog to describe
this patch. (Refer Documentation/process/submitting-patches.rst.)
- The brief kernel-doc descriptions should not have a trailing period.
Nor should kernel-doc variable definitions have a trailing period.
Example(s) inline below. (The existing kernel-doc is mostly correct.)
- For some reason existing comments that span multiple lines aren't
formatted properly. In those cases, where there is another change,
please fix the comment and function description.
thanks,
Mimi
>
> Acked-by: Stephen Smalley
> Acked-by: Paul Moore
> Acked-by: John Johansen
> Signed-off-by: Casey Schaufler
> Cc:
> Cc: linux-au...@redhat.com
> Cc: linux-security-mod...@vger.kernel.org
> Cc: seli...@vger.kernel.org
> ---
> diff --git a/include/linux/security.h b/include/linux/security.h
> index bc2725491560..fdb6e95c98e8 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -132,6 +132,65 @@ enum lockdown_reason {
>
> extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
>
> +/*
> + * Data exported by the security modules
> + *
> + * Any LSM that provides secid or secctx based hooks must be included.
> + */
> +#define LSMBLOB_ENTRIES ( \
> + (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_SECURITY_APPARMOR) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0))
> +
> +struct lsmblob {
> + u32 secid[LSMBLOB_ENTRIES];
> +};
> +
> +#define LSMBLOB_INVALID -1 /* Not a valid LSM slot number
> */
> +#define LSMBLOB_NEEDED -2 /* Slot requested on
> initialization */
> +#define LSMBLOB_NOT_NEEDED -3 /* Slot not requested */
> +
> +/**
> + * lsmblob_init - initialize an lsmblob structure.
Only this kernel-doc brief description is suffixed with a period.
Please remove.
> + * @blob: Pointer to the data to initialize
> + * @secid: The initial secid value
> + *
> + * Set all secid for all modules to the specified value.
> + */
> +static inline void lsmblob_init(struct lsmblob *blob, u32 secid)
> +{
> + int i;
> +
> + for (i = 0; i < LSMBLOB_ENTRIES; i++)
> + blob->secid[i] = secid;
> +}
> +
> +/**
> + * lsmblob_is_set - report if there is an value in the lsmblob
> + * @blob: Pointer to the exported LSM data
> + *
> + * Returns true if there is a secid set, false otherwise
> + */
> +static inline bool lsmblob_is_set(struct lsmblob *blob)
> +{
> + struct lsmblob empty = {};
> +
> + return !!memcmp(blob, , sizeof(*blob));
> +}
> +
> +/**
> + * lsmblob_equal - report if the two lsmblob's are equal
> + * @bloba: Pointer to one LSM data
> + * @blobb: Pointer to the other LSM data
> + *
> + * Returns true if all entries in the two are equal, false otherwise
> + */
> +static inline bool lsmblob_equal(struct lsmblob *bloba, struct lsmblob
> *blobb)
> +{
> + return !memcmp(bloba, blobb, sizeof(*bloba));
> +}
> +
> /* These functions are in security/commoncap.c */
> extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c
> index 9b5adeaa47fc..cd393aaa17d5 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> } lsm[MAX_LSM_RULES];
> @@ -88,6 +88,22 @@ struct ima_rule_entry {
> struct ima_template_desc *template;
> };
>
> +/**
> + * ima_lsm_isset - Is a rule set for any of the active security modules
> + * @rules: The set of IMA rules to check.
Nor do kernel-doc
Re: [PATCH v9 7/8] IMA: define a builtin critical data measurement policy
On Sat, 2020-12-12 at 10:02 -0800, Tushar Sugandhi wrote: > From: Lakshmi Ramasubramanian > > Define a new critical data builtin policy to allow measuring > early kernel integrity critical data before a custom IMA policy > is loaded. > > Add critical data to built-in IMA rules if the kernel command line > contains "ima_policy=critical_data". This sentence isn't really necessary. > > Update the documentation on kernel parameters to document > the new critical data builtin policy. > > Signed-off-by: Lakshmi Ramasubramanian > Reviewed-by: Tyler Hicks Otherwise, Reviewed-by: Mimi Zohar thanks, Mimi
Re: [PATCH v9 5/8] IMA: limit critical data measurement based on a label
Hi Tushar, On Sat, 2020-12-12 at 10:02 -0800, Tushar Sugandhi wrote: > System administrators should be able to limit which kernel subsystems > they want to measure the critical data for. To enable that, an IMA policy > condition to choose specific kernel subsystems is needed. This policy > condition would constrain the measurement of the critical data based on > a label for the given subsystems. Restricting which kernel integrity critical data is measured is not only of interest to system administrators. Why single them out? Limiting which critical data is measured is based on a label, making it flexible. In your use case scenario, you're grouping the label based on kernel subsystem, but is that really necessary? In the broader picture, there could be cross subsystem critical data being measured based on a single label. Please think about the broader picture and re-write the patch descirption more generically. > > Add a new IMA policy condition - "data_source:=" to the IMA func What is with "add"? You're "adding support for" or "defining" a new policy condition. Remove the single hyphen, as explained in 3/8. Please replace "data_source" with something more generic (e.g. label). thanks, Mimi > CRITICAL_DATA to allow measurement of various kernel subsystems. This > policy condition would enable the system administrators to restrict the > measurement to the labels listed in "data_source:=". > > Limit the measurement to the labels that are specified in the IMA > policy - CRITICAL_DATA+"data_source:=". If "data_sources:=" is not > provided with the func CRITICAL_DATA, the data from all the > supported kernel subsystems is measured. > > Signed-off-by: Tushar Sugandhi
Re: [PATCH v9 4/8] IMA: add policy rule to measure critical data
Hi Tushar, Please update the Subject line as, "Add policy rule support for measuring critical data". On Sat, 2020-12-12 at 10:02 -0800, Tushar Sugandhi wrote: > A new IMA policy rule is needed for the IMA hook > ima_measure_critical_data() and the corresponding func CRITICAL_DATA for > measuring the input buffer. The policy rule should ensure the buffer > would get measured only when the policy rule allows the action. The > policy rule should also support the necessary constraints (flags etc.) > for integrity critical buffer data measurements. > > Add a policy rule to define the constraints for restricting integrity > critical data measurements. > > Signed-off-by: Tushar Sugandhi This patch does not restrict measuring critical data, but adds policy rule support for measuring critical data. please update the patch description accordingly. Other than that, Reviewed-by: Mimi Zohar
