Re: [PATCH] ima: Fix return value of ima_write_policy()

[Mimi Zohar]

On Thu, 2020-04-23 at 09:39 +0000, Roberto Sassu wrote:
> > On Tue, 2020-04-21 at 11:04 +0200, Roberto Sassu wrote:
> > > Return datalen instead of zero if there is a rule to appraise the policy
> > > but that rule is not enforced.
> > >
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Fixes: 19f8a84713edc ("ima: measure and appraise the IMA policy itself")
> > > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > > ---
> > >  security/integrity/ima/ima_fs.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/security/integrity/ima/ima_fs.c
> > b/security/integrity/ima/ima_fs.c
> > > index a71e822a6e92..2c2ea814b954 100644
> > > --- a/security/integrity/ima/ima_fs.c
> > > +++ b/security/integrity/ima/ima_fs.c
> > > @@ -340,6 +340,8 @@ static ssize_t ima_write_policy(struct file *file,
> > const char __user *buf,
> > >  				    1, 0);
> > >  		if (ima_appraise & IMA_APPRAISE_ENFORCE)
> > >  			result = -EACCES;
> > > +		else
> > > +			result = datalen;
> > 
> > In all other cases, where the IMA_APPRAISE_ENFORCE is not enabled we
> > allow the action. ÂHere we prevent loading the policy, but don't
> > return an error. ÂOne option, as you did, is return some indication
> > that the policy was not loaded. ÂAnother option would be to allow
> > loading the policy in LOG or FIX mode, but I don't think that would be
> > productive. ÂPerhaps differentiate between the LOG and FIX modes from
> > the OFF mode. ÂFor the LOG and FIX modes, perhaps return -EACCES as
> > well. ÂFor the OFF case, loading a policy with appraise rules should
> > not be permitted.
> 
> In LOG or FIX mode, loading a policy with absolute path will succeed.

Yes

> Maybe we should just keep the same behavior also when the policy
> is directly written to securityfs.

The purpose of LOG mode is to learn about the filesystem behavior in
order to label it properly. ÂFIX mode is for re-calculating and fixing
existing file hashes. ÂIf we permit directly writing the policy to
securityfs, even if the existing policy requires the policy to be
signed, then it is behaving differently than it would in enforcing
mode. ÂI'm ok with that, as long as the error message is emitted.

> Ok for the OFF mode, but probably this should be a separate patch.

Agreed

Mimi

> 
> > >  	} else {
> > >  		result = ima_parse_add_rule(data);
> > >  	}
>