Re: [Linux-ima-user] [PATCH v3 1/2] Allows for persistent IMA policy sysfs entry;

[Mimi Z. <zo...@li...>]

On Fri, 2015-09-11 at 19:54 +0300, Petko Manolov wrote:

> On 15-09-11 09:58:22, Mimi Zohar wrote:
> > On Thu, 2015-09-10 at 14:17 +0300, Petko Manolov wrote:
> > > It is often useful to be able to add rules to the IMA policy without restarting
> > > the kernel.  This patch keeps the sysfs entry alive so the policy may be
> > > extended once new user is introduced along with corresponding credentials.
> > > 
> > > Signed-off-by: Petko Manolov <pe...@mi...>
> > 
> > Hi Petko,
> > 
> > I don't have a problem with extending the policy rules, as opposed to
> > replacing them, but my main concern is being able to do it safely.
> 
> Actually it is a hard requirement in our usecase.  The last thing we want is 
> somebody replacing the whole policy.  We may as well not have IMA enabled.

Glad to hear.   The prior policy patches that were posted replaced the
policy.

> > > ---
> > >  security/integrity/ima/Kconfig      | 15 +++++++++++++++
> > >  security/integrity/ima/ima_fs.c     |  4 ++++
> > >  security/integrity/ima/ima_policy.c | 17 +++++++++++++++++
> > >  3 files changed, 36 insertions(+)
> > > 
> > > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> > > index df30334..ebe7a907 100644
> > > --- a/security/integrity/ima/Kconfig
> > > +++ b/security/integrity/ima/Kconfig
> > > @@ -107,6 +107,21 @@ config IMA_DEFAULT_HASH
> > >  	default "sha512" if IMA_DEFAULT_HASH_SHA512
> > >  	default "wp512" if IMA_DEFAULT_HASH_WP512
> > > 
> > > +config IMA_DEV_POLICY
> > > +	bool "Enable persistent ima_policy entry in securityfs"
> > > +	depends on IMA
> > > +	default n
> > > +	help
> > > +
> > > +	  Prevents "ima_policy" removal in securityfs once the IMA policy is
> > > +	  written into the file. This allows for multiple IMA policy updates
> > > +	  while the system is running.
> > 
> > Currently, when we attempt to load a new policy, if any of the rules are 
> > malformed, loading the new policy fails.  It looks like the new rules extend 
> > the existing list.  What should happen if one of the new rules is malformed?  
> > Will the entire set of new rules be removed?
> 
> Last time i looked at this part of the code: the new policy will be accepted up 
> to the faulty rule.  The rest will be ignored.

Is that safe?  I would prefer accepting all of the rules or nothing.  I
don't think it would be too hard to implement.  Perhaps, keep a pointer
to the last rule of the existing policy.  If there is a malformed rule,
delete everything from after the last existing policy rule.

> In order to prevent this i've written user-space tool that generates 
> syntactically correct rules.  However the issue must be addressed in some other 
> way as wall.  Do you think it is worth to be more verbose when the parser runs 
> into malformed rule?

It currently isn't easy to detect which rule is malformed.  Part of the
problem is that when there are too many messages, message are dropped.
Adding more messages might aggravate the situation.

With a patch for displaying the current policy, there's no need for
displaying the policies during boot or any other time that the policy is
being extended.  We might then be able to actually see the malformed
rule.   I'm pretty sure someone has already posted an initial patch.
[cc'ing linux-ima-user mailing list] 

> > > +	  WARNING: Potential security hole - should not be used in production
> > > +	  grade kernels!
> > 
> > Agreed.  Like all other data the kernel consumes, policies should be signed 
> > and verified before being used.  I could see allowing new rules to be added, 
> > only if they're signed.  (I think Dmitry has some code that defines a new hook 
> > called POLICY_CHECK.)
> 
> POLICY_CHECK not mainlined yet.  Any plans for this to happen soon?

The last time the patches were posted, it was included in a patch bomb.
Lets try to extract the worthwhile patches.  Maybe clean them up a bit. 

> Our setup refuses to read anything that is not signed, including the new policy 
> file(s).  This, however, does hot save us from piping the rules from a script...
> 
> > > +	  If unsure, say N.
> > > +
> > >  config IMA_APPRAISE
> > >  	bool "Appraise integrity measurements"
> > >  	depends on IMA
> > > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> > > index 816d175..b0df2b8 100644
> > > --- a/security/integrity/ima/ima_fs.c
> > > +++ b/security/integrity/ima/ima_fs.c
> > > @@ -337,8 +337,12 @@ static int ima_release_policy(struct inode *inode, struct file *file)
> > >  		return 0;
> > >  	}
> > >  	ima_update_policy();
> > > +#ifndef	CONFIG_IMA_DEV_POLICY
> > >  	securityfs_remove(ima_policy);
> > >  	ima_policy = NULL;
> > > +#else
> > > +	clear_bit(IMA_FS_BUSY, &ima_fs_flags);
> > > +#endif
> > >  	return 0;
> > >  }
> > > 
> > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > > index 3997e20..eef99d6 100644
> > > --- a/security/integrity/ima/ima_policy.c
> > > +++ b/security/integrity/ima/ima_policy.c
> > > @@ -140,6 +140,19 @@ static struct list_head *ima_rules;
> > >  static DEFINE_MUTEX(ima_rules_mutex);
> > > 
> > >  static int ima_policy __initdata;
> > > +
> > > +/*
> > > + * macros to lock/unlock code that is related to dynamically
> > > + * loadable IMA policy.
> > > + */
> > > +#ifdef	CONFIG_IMA_DEV_POLICY
> > > +#define	ima_dev_policy_lock()	mutex_lock(&ima_rules_mutex)
> > > +#define	ima_dev_policy_unlock()	mutex_unlock(&ima_rules_mutex)
> > > +#else
> > > +#define	ima_dev_policy_lock()
> > > +#define	ima_dev_policy_unlock()
> > > +#endif	/* CONFIG_IMA_DEV_POLICY */
> > > +
> > 
> > As the existing policy rules never changed, we didn't need locking, With this 
> > change, the list of rules can be extended so we need locking. Instead of using 
> > a mutex, as the number of readers is far greater than writers, we should use 
> > RCU locking.
> 
> Yeah, this was stupid on my part.  I should have done something better.  Will 
> replace the mutex with proper RCU locking.

Thanks.

Mimi