On 16-02-08 16:12:16, Mimi Zohar wrote:

On Mon, 2016-02-08 at 10:45 +0000, Dmitry Kasatkin wrote:

quoted

quoted

quoted

@@ -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] == '/')

quoted

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);

quoted

                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.