Re: [PATCH v1 3/7] ima: load policy using path

[Luis R. Rodriguez]

On Thu, Dec 17, 2015 at 11:33 AM, Luis R. Rodriguez  wrote:
> Please no, instead of adding yet-another kernel file-loading facility which is
> likely error prone we should consolidate *all kernel file-loading facilities*
> into a *common generic shared one*. So please work to make that happen since 
> you
> need yet-another user for it.m.com
>
> Since you need yet-naother kernel file-loader please do the work to generalize
> it, or at least try it.

As per review in another thread with Mimi we determined they're not
adding a *new* reader, but using an existing one. The possible issues
with early read and pivot_root() as well as possible considerations
for a common user mode helper are still relevant for when we
generalize a common kernel loader. Mimi has also pointed out a few
other kernel loaders. It seems we'll try to tackle this after the
holidays. To help keep track of progress and consolidate notes on this
I've stuffed details about this on this wiki:

http://kernelnewbies.org/KernelProjects/common-kernel-loader

  Luis
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/7] ima: load policy using path

[Luis R. Rodriguez]

The subject should probably be: ima: add support to load policy from path

Cc'ing Roberts who originally wanted SELinux file policy signing capabilities.
Also Greg, who is reviewing the sysdata file changes I had proposed which would
provide a generic file loading facility for modules, and later a generic file
signing checker. Cc'ing Linus for any feedback on the possible issues he might
foresee if we all go gung-ho on "kernel file loading", as this is where it
seems some folks might be going. I *think* the user hint might help close the
semantic gap observed on using a file loader on firmware_class, but perhaps he
or other might have some other corner cases in mind we should also consider.

Note that the crux between using a generic kernel file loader and the common
"sysdata" file loader will be that sysdata file users (wireless would be one
getting rid of CRDA) and a core kernel file loader (IMA could be one, likewise
perhaps SELinux if it follows suit in design as in this patch) is that the core
kernel file loader would allow arbitrary file paths, and the sysdata users
would have stuff in /lib/firmware/ paths (and therefore also have things in the
linux-firmware tree).

On Tue, Dec 08, 2015 at 01:01:20PM -0500, Mimi Zohar wrote:
> From: Dmitry Kasatkin 
> 
> Currently the IMA policy is loaded by writing the policy rules to
> '/ima/policy'.

> That way the policy cannot be measured or appraised. This patch extends the
> policy loading interface with the possibility to load the policy using a
> pathname. The policy can be loaded like:
> 
> echo /etc/ima/ima_policy > /sys/kernel/security/ima/policy

I don't think this is as clear, you can just say something like:

-
We currently cannot do appraisal or signature vetting of IMA policies
since we currently can only load IMA policies by writing the contents
of the polify directly in, as follows:

cat policy-file > /ima/policy

If we provide the kernel the path to the IMA policy so it can load
the policy itself it'd be able to later appraise or vet for the file
signature if it had one. This adds support to load IMA policies
with a given path as follows:

echo /etc/ima/ima_policy > /sys/kernel/security/ima/policy
-

> 
> Signed-off-by: Dmitry Kasatkin 
> Signed-off-by: Mimi Zohar 
> ---
>  security/integrity/iint.c   |  4 +---
>  security/integrity/ima/ima_fs.c | 39 ++-
>  security/integrity/integrity.h  |  2 +-
>  3 files changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 2de9c82..54b51a4 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -203,10 +203,8 @@ int integrity_kernel_read(struct file *file, loff_t 
> offset,
>   * This is function opens a file, allocates the buffer of required
>   * size, read entire file content to the buffer and closes the file
>   *
> - * It is used only by init code.
> - *
>   */
> -int __init integrity_read_file(const char *path, char **data)
> +int integrity_read_file(const char *path, char **data)
>  {
>   struct file *file;
>   loff_t size;
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index eebb985..f902b6b 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -258,6 +258,40 @@ static const struct file_operations 
> ima_ascii_measurements_ops = {
>   .release = seq_release,
>  };
>  
> +static ssize_t ima_read_policy(char *path)
> +{
> + char *data, *datap;
> + int rc, size, pathlen = strlen(path);
> + char *p;
> +
> + /* remove \n */
> + datap = path;
> + strsep(, "\n");
> +
> + rc = integrity_read_file(path, );
> + if (rc < 0)
> + return rc;
> +
> + size = rc;
> + datap = data;
> +
> + while (size > 0 && (p = strsep(, "\n"))) {
> + pr_debug("rule: %s\n", p);
> + rc = ima_parse_add_rule(p);
> + if (rc < 0)
> + break;
> + size -= rc;
> + }
> +
> + kfree(data);
> + if (rc < 0)
> + return rc;
> + else if (size)
> + return -EINVAL;
> + else
> + return pathlen;
> +}
> +

Please no, instead of adding yet-another kernel file-loading facility which is
likely error prone we should consolidate *all kernel file-loading facilities*
into a *common generic shared one*. So please work to make that happen since you
need yet-another user for it. I've done some initial homework already on a few
prominent common users. I say "initial" homework as my search was rather crude 
on
'git grep' and not done using semantic parsers to see if I "search for file
loaders" through semantic means. This later search may be optional, but it
would help