[PATCH] IMA: Turn IMA_MEASURE_ASYMMETRIC_KEYS off by default

[PATCH] IMA: Turn IMA_MEASURE_ASYMMETRIC_KEYS off by default

[Lakshmi Ramasubramanian]

Enabling IMA and ASYMMETRIC_PUBLIC_KEY_SUBTYPE configs will
automatically enable the IMA hook to measure asymmetric keys. Keys
created or updated early in the boot process are queued up whether
or not a custom IMA policy is provided. Although the queued keys will
be freed if a custom IMA policy is not loaded within 5 minutes, it could
still cause significant performance impact on smaller systems.

This patch turns the config IMA_MEASURE_ASYMMETRIC_KEYS off by default.
Since a custom IMA policy that defines key measurement is required to
measure keys, systems that require key measurement can enable this
config option in addition to providing a custom IMA policy.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 security/integrity/ima/Kconfig | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 355754a6b6ca..8e678219ee9e 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -312,7 +312,19 @@ config IMA_APPRAISE_SIGNED_INIT
 	   This option requires user-space init to be signed.
 
 config IMA_MEASURE_ASYMMETRIC_KEYS
-	bool
+	bool "Enable asymmetric keys measurement on key create or update"
 	depends on IMA
 	depends on ASYMMETRIC_PUBLIC_KEY_SUBTYPE=y
-	default y
+	default n
+	help
+	   This option enables measuring asymmetric keys when the key
+	   is created or updated. Additionally a custom IMA policy that
+	   defines key measurement should also be loaded.
+
+	   If this option is enabled, keys created or updated early in
+	   the boot process are queued up. The queued keys are processed
+	   when a custom IMA policy is loaded. But if a custom IMA policy
+	   is not loaded within 5 minutes after IMA subsystem is initialized,
+	   any queued keys are just freed. Keys created or updated after
+	   a custom IMA policy is loaded will be processed immediately and
+	   not queued.
-- 
2.17.1

Re: [PATCH] IMA: Turn IMA_MEASURE_ASYMMETRIC_KEYS off by default

[James Bottomley]

On Tue, 2020-01-21 at 09:13 -0800, Lakshmi Ramasubramanian wrote:
> Enabling IMA and ASYMMETRIC_PUBLIC_KEY_SUBTYPE configs will
> automatically enable the IMA hook to measure asymmetric keys. Keys
> created or updated early in the boot process are queued up whether
> or not a custom IMA policy is provided. Although the queued keys will
> be freed if a custom IMA policy is not loaded within 5 minutes, it
> could still cause significant performance impact on smaller systems.

What exactly do you expect distributions to do with this?  I can tell
you that most of them will take the default option, so this gets set to
N and you may as well not have got the patches upstream because you
won't be able to use them in any distro with this setting.

> This patch turns the config IMA_MEASURE_ASYMMETRIC_KEYS off by
> default.  Since a custom IMA policy that defines key measurement is
> required to measure keys, systems that require key measurement can
> enable this config option in addition to providing a custom IMA
> policy.

Well, no they can't ... it's rather rare nowadays for people to build
their own kernels.  The vast majority of Linux consumers take what the
distros give them.  Think carefully before you decide a config option
is the solution to this problem.

James

Re: [PATCH] IMA: Turn IMA_MEASURE_ASYMMETRIC_KEYS off by default

[Lakshmi Ramasubramanian]

On 1/21/20 9:34 AM, James Bottomley wrote:

> What exactly do you expect distributions to do with this?  I can tell
> you that most of them will take the default option, so this gets set to
> N and you may as well not have got the patches upstream because you
> won't be able to use them in any distro with this setting.

I agree - distros that are not sure or don't care about key measurement 
are anyway not going to choose this option. Only those that really care 
will opt in.

My goal is to not burden the vast majority of the users with this 
additional overhead if they don't need it - particularly, small systems 
such as embedded devices, etc.

> 
> Well, no they can't ... it's rather rare nowadays for people to build
> their own kernels.  The vast majority of Linux consumers take what the
> distros give them.  Think carefully before you decide a config option
> is the solution to this problem.
> 
> James
> 
If you have suggestions for how I can handle it in a different way 
(other than config option), I'll be happy to try it out.

thanks,
  -lakshmi

Re: [PATCH] IMA: Turn IMA_MEASURE_ASYMMETRIC_KEYS off by default

[Mimi Zohar]

On Tue, 2020-01-21 at 09:34 -0800, James Bottomley wrote:
> On Tue, 2020-01-21 at 09:13 -0800, Lakshmi Ramasubramanian wrote:
> > Enabling IMA and ASYMMETRIC_PUBLIC_KEY_SUBTYPE configs will
> > automatically enable the IMA hook to measure asymmetric keys. Keys
> > created or updated early in the boot process are queued up whether
> > or not a custom IMA policy is provided. Although the queued keys will
> > be freed if a custom IMA policy is not loaded within 5 minutes, it
> > could still cause significant performance impact on smaller systems.
> 
> What exactly do you expect distributions to do with this?  I can tell
> you that most of them will take the default option, so this gets set to
> N and you may as well not have got the patches upstream because you
> won't be able to use them in any distro with this setting.
> 
> > This patch turns the config IMA_MEASURE_ASYMMETRIC_KEYS off by
> > default.  Since a custom IMA policy that defines key measurement is
> > required to measure keys, systems that require key measurement can
> > enable this config option in addition to providing a custom IMA
> > policy.
> 
> Well, no they can't ... it's rather rare nowadays for people to build
> their own kernels.  The vast majority of Linux consumers take what the
> distros give them.  Think carefully before you decide a config option
> is the solution to this problem.

James, up until now IMA could be configured, but there wouldn't be any
performance penalty for enabling IMA until a policy was loaded.  With
IMA and asymmetric keys enabled, whether or not an IMA policy is
loaded, certificates will be queued.

My concern is:
- changing the expected behavior
- really small devices/sensors being able to queue certificates

This change permits disabling queueing certificates.  Whether the
default should be "disabled" is a separate question.  I'm open to
comments/suggestions.

Mimi

Re: [PATCH] IMA: Turn IMA_MEASURE_ASYMMETRIC_KEYS off by default

[James Bottomley]

On Tue, 2020-01-21 at 14:13 -0500, Mimi Zohar wrote:
> On Tue, 2020-01-21 at 09:34 -0800, James Bottomley wrote:
> > On Tue, 2020-01-21 at 09:13 -0800, Lakshmi Ramasubramanian wrote:
> > > Enabling IMA and ASYMMETRIC_PUBLIC_KEY_SUBTYPE configs will
> > > automatically enable the IMA hook to measure asymmetric keys.
> > > Keys created or updated early in the boot process are queued up
> > > whether or not a custom IMA policy is provided. Although the
> > > queued keys will be freed if a custom IMA policy is not loaded
> > > within 5 minutes, it could still cause significant performance
> > > impact on smaller systems.
> > 
> > What exactly do you expect distributions to do with this?  I can
> > tell you that most of them will take the default option, so this
> > gets set to N and you may as well not have got the patches upstream
> > because you won't be able to use them in any distro with this
> > setting.
> > 
> > > This patch turns the config IMA_MEASURE_ASYMMETRIC_KEYS off by
> > > default.  Since a custom IMA policy that defines key measurement
> > > is required to measure keys, systems that require key measurement
> > > can enable this config option in addition to providing a custom
> > > IMA policy.
> > 
> > Well, no they can't ... it's rather rare nowadays for people to
> > build their own kernels.  The vast majority of Linux consumers take
> > what the distros give them.  Think carefully before you decide a
> > config option is the solution to this problem.
> 
> James, up until now IMA could be configured, but there wouldn't be
> any performance penalty for enabling IMA until a policy was loaded.
>  With IMA and asymmetric keys enabled, whether or not an IMA policy
> is loaded, certificates will be queued.
> 
> My concern is:
> - changing the expected behavior

In general config options for this are a really bad idea because if the
tools only cope with one setting, no-one should ever use the other and
if they work with everything there's no need for the option.

> - really small devices/sensors being able to queue certificates

seems like the answer to this one would be don't queue.  I realise it's
after the submit design, but what about measuring when the key is added
if there's a policy otherwise measure the keyring when the policy is
added ... that way no queueing.

> This change permits disabling queueing certificates.  Whether the
> default should be "disabled" is a separate question.  I'm open to
> comments/suggestions.

I'm just giving the general rule of thumb for boolean config options. 
If it's default Y there likely shouldn't be a config option and if it's
default N the feature should likely not be in the kernel at all.

Re: [PATCH] IMA: Turn IMA_MEASURE_ASYMMETRIC_KEYS off by default

[Lakshmi Ramasubramanian]

On 1/21/2020 11:52 AM, James Bottomley wrote:

>> - really small devices/sensors being able to queue certificates
> 
> seems like the answer to this one would be don't queue.  I realise it's
> after the submit design, but what about measuring when the key is added
> if there's a policy otherwise measure the keyring when the policy is
> added ... that way no queueing.

Without the "deferred key processing" changes, only keys added at 
runtime were measured (if policy permitted).

"deferred key processing" enabled queuing keys added early in the boot 
process and measured them when the policy is loaded.

We can make this (the queuing) optional through a config, but leave the 
runtime key measurement auto-enabled (as is the config 
IMA_MEASURE_ASYMMETRIC_KEYS now).

  -lakshmi

Re: [PATCH] IMA: Turn IMA_MEASURE_ASYMMETRIC_KEYS off by default

[Mimi Zohar]

On Tue, 2020-01-21 at 12:38 -0800, Lakshmi Ramasubramanian wrote:
> On 1/21/2020 11:52 AM, James Bottomley wrote:
> 
> >> - really small devices/sensors being able to queue certificates
> > 
> > seems like the answer to this one would be don't queue.  I realise it's
> > after the submit design, but what about measuring when the key is added
> > if there's a policy otherwise measure the keyring when the policy is
> > added ... that way no queueing.
> 
> Without the "deferred key processing" changes, only keys added at 
> runtime were measured (if policy permitted).
> 
> "deferred key processing" enabled queuing keys added early in the boot 
> process and measured them when the policy is loaded.
> 
> We can make this (the queuing) optional through a config, but leave the 
> runtime key measurement auto-enabled (as is the config 
> IMA_MEASURE_ASYMMETRIC_KEYS now).

Thanks, Lakshmi.  This requires moving the code around.  Instead of
doing this on the current code base, I suggest posting a v9 version of
the entire "IMA: Deferred measurement of keys".

I suggest making the switch from spinlock to mutex, as you had it
originally, before posting v9.  The commit history will then be a lot
cleaner.

thanks,

Mimi

Re: [PATCH] IMA: Turn IMA_MEASURE_ASYMMETRIC_KEYS off by default

[Lakshmi Ramasubramanian]

On 1/22/20 12:02 PM, Mimi Zohar wrote:

> 
> Thanks, Lakshmi.  This requires moving the code around.  Instead of
> doing this on the current code base, I suggest posting a v9 version of
> the entire "IMA: Deferred measurement of keys".
> 
> I suggest making the switch from spinlock to mutex, as you had it
> originally, before posting v9.  The commit history will then be a lot
> cleaner.
> 
> thanks,
> 
> Mimi
> 

Sure Mimi - I'll post an update to the patch set shortly.

thanks,
  -lakshmi

Re: [PATCH] IMA: Turn IMA_MEASURE_ASYMMETRIC_KEYS off by default

[Mimi Zohar]

On Wed, 2020-01-22 at 12:05 -0800, Lakshmi Ramasubramanian wrote:
> On 1/22/20 12:02 PM, Mimi Zohar wrote:
> 
> > 
> > Thanks, Lakshmi.  This requires moving the code around.  Instead of
> > doing this on the current code base, I suggest posting a v9 version of
> > the entire "IMA: Deferred measurement of keys".
> > 
> > I suggest making the switch from spinlock to mutex, as you had it
> > originally, before posting v9.  The commit history will then be a lot
> > cleaner.
> > 
> > thanks,
> > 
> > Mimi
> > 
> 
> Sure Mimi - I'll post an update to the patch set shortly.

I just pushed out the next-integrity-testing branch without the
"Deferred measurement of keys" patches.  I've kept the "IMA: pre-
allocate buffer to hold keyrings string" patch, though it probably
isn't required.

thanks,

Mimi

Re: [PATCH] IMA: Turn IMA_MEASURE_ASYMMETRIC_KEYS off by default

[Mimi Zohar]

On Tue, 2020-01-21 at 11:52 -0800, James Bottomley wrote:
> On Tue, 2020-01-21 at 14:13 -0500, Mimi Zohar wrote:

> > This change permits disabling queueing certificates.  Whether the
> > default should be "disabled" is a separate question.  I'm open to
> > comments/suggestions.
> 
> I'm just giving the general rule of thumb for boolean config options. 
> If it's default Y there likely shouldn't be a config option and if it's
> default N the feature should likely not be in the kernel at all.

Thanks, James.  I'll keep this in mind when debating about defining a
new Kconfig option.

Mimi