Re: [PATCH v3 20/22] ima: load policy using path

[Petko Manolov <petkan@mip-labs.com>]

On 16-02-08 16:12:16, Mimi Zohar wrote:
> On Mon, 2016-02-08 at 10:45 +0000, Dmitry Kasatkin wrote:
> 
> > > > @@ -286,9 +322,12 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
> > > >       result = mutex_lock_interruptible(&ima_write_mutex);
> > > >       if (result < 0)
> > > >               goto out_free;
> > > > -     result = ima_parse_add_rule(data);
> > > > -     mutex_unlock(&ima_write_mutex);
> > > >
> > > > +     if (data[0] == '/')
> > >
> > > >It seems that if we feed relative path to ima_policy the update will fail...
> > >
> > > Yes, i think it is always a good idea to pass absolute path.
> > 
> > What if we at least emit a warning so people know what's wrong?
> 
> The next patch "ima: measure and appraise the IMA policy itself" adds
> the following.  Is a failure message enough?

That would be the wrong message.  The above code does not handle relative paths 
so any attempt to load the policy by "./ima_policy_file" or 
"../../ima_policy_file" will fail.

Isn't there a kernel function that checks if given string is a path-name?

> +       else if (ima_appraise & IMA_APPRAISE_POLICY) {
> +               pr_err("IMA: signed policy required\n");
> +               integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
> +                                   "policy_update", "signed policy
> required",
> +                                   1, 0);
> +               if (ima_appraise & IMA_APPRAISE_ENFORCE)
> +                       result = -EACCES;
> +       } else
>                 result = ima_parse_add_rule(data);
> 
> > 
> >                 Petko
> > 
> > DK: May be a good idea to print that loading policy by path or not.
> 
> Are we including the pathname?  Are you suggesting a log or audit message?

I guess log is more appropriate.