Re: [PATCH v1 6/7] ima: measure and appraise the IMA policy itself
On Sun, Dec 20, 2015 at 12:11:04AM -0500, Mimi Zohar wrote:
> On Thu, 2015-12-17 at 23:03 +0100, Luis R. Rodriguez wrote:
> > On Tue, Dec 08, 2015 at 01:01:23PM -0500, Mimi Zohar wrote:
> > > diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> > > index 8a45576..4d149c9 100644
> > > --- a/security/integrity/iint.c
> > > +++ b/security/integrity/iint.c
> > > @@ -222,6 +223,11 @@ int integrity_read_file(const char *path, char
> > > **data)
> > > return rc;
> > > }
> > >
> > > + if (!S_ISREG(file_inode(file)->i_mode)) {
> > > + rc = -EACCES;
> > > + goto out;
> > > + }
> > > +
> > > size = i_size_read(file_inode(file));
> > > if (size <= 0)
> > > goto out;
> >
> > This hunk seems to be unrelated to this patch? If so can it be split out?
>
> Yes, sure. Up to now, 'cat' was used to load the IMA policy. A lot
> of the problems related to opening and reading a file were hidden. So
> besides making sure that only a regular file is opened, what other
> things should we be checking? For example, do we permit the kernel to
> read NFS mounted files? Should the kernel be limited to opening only
> local files? Answering these questions becomes important as we move to
> a single kernel file read function.
Answering this properly should include effort to study and consolidate other
kernel read routines. From the little that I've so far reviewed these we don't
have much differences in requirements even between this IMA one and the sound
one
you just pointed out, the small changes for correctness however are important to
capture for all. Because of this we should be able to still provide a generic
read routine that takes all considerations into account, enables flexibility
but more importantly shares the best practices for correctness.
I can understand you might want to not wait for that yet, and I think that's
fine, but lets work in parallel to make that happen.
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 6/7] ima: measure and appraise the IMA policy itself
On Thu, 2015-12-17 at 23:03 +0100, Luis R. Rodriguez wrote:
> On Tue, Dec 08, 2015 at 01:01:23PM -0500, Mimi Zohar wrote:
> > diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> > index 8a45576..4d149c9 100644
> > --- a/security/integrity/iint.c
> > +++ b/security/integrity/iint.c
> > @@ -222,6 +223,11 @@ int integrity_read_file(const char *path, char **data)
> > return rc;
> > }
> >
> > + if (!S_ISREG(file_inode(file)->i_mode)) {
> > + rc = -EACCES;
> > + goto out;
> > + }
> > +
> > size = i_size_read(file_inode(file));
> > if (size <= 0)
> > goto out;
>
> This hunk seems to be unrelated to this patch? If so can it be split out?
Yes, sure. Up to now, 'cat' was used to load the IMA policy. A lot
of the problems related to opening and reading a file were hidden. So
besides making sure that only a regular file is opened, what other
things should we be checking? For example, do we permit the kernel to
read NFS mounted files? Should the kernel be limited to opening only
local files? Answering these questions becomes important as we move to
a single kernel file read function.
> > @@ -232,13 +238,18 @@ int integrity_read_file(const char *path, char **data)
> > goto out;
> > }
> >
> > - rc = integrity_kernel_read(file, 0, buf, size);
> > + rc = ima_read_and_process_file(file, read_func, buf, size);
> > + if (rc == -EOPNOTSUPP) {
> > + rc = integrity_kernel_read(file, 0, buf, size);
> > + if (rc > 0 && rc != size)
> > + rc = -EIO;
> > + }
> > if (rc < 0)
> > kfree(buf);
> > - else if (rc != size)
> > - rc = -EIO;
> > - else
> > + else {
> > + rc = size;
> > *data = buf;
> > + }
> > out:
> > fput(file);
> > return rc;
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index 548b258..40a24c3 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -180,6 +180,7 @@ int ima_policy_show(struct seq_file *m, void *v);
> > #define IMA_APPRAISE_LOG 0x04
> > #define IMA_APPRAISE_MODULES 0x08
> > #define IMA_APPRAISE_FIRMWARE 0x10
> > +#define IMA_APPRAISE_POLICY0x20
> >
> > #ifdef CONFIG_IMA_APPRAISE
> > int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
> > diff --git a/security/integrity/ima/ima_appraise.c
> > b/security/integrity/ima/ima_appraise.c
> > index b83049b..1e1a759 100644
> > --- a/security/integrity/ima/ima_appraise.c
> > +++ b/security/integrity/ima/ima_appraise.c
> > @@ -79,6 +79,7 @@ enum integrity_status ima_get_cache_status(struct
> > integrity_iint_cache *iint,
> > case FIRMWARE_CHECK:
> > case KEXEC_CHECK:
> > case INITRAMFS_CHECK:
> > + case POLICY_CHECK:
> > return iint->ima_read_status;
> > case FILE_CHECK:
> > default:
>
> Hrm this uses an int for the func.
>
> > @@ -102,6 +103,7 @@ static void ima_set_cache_status(struct
> > integrity_iint_cache *iint,
> > case FIRMWARE_CHECK:
> > case KEXEC_CHECK:
> > case INITRAMFS_CHECK:
> > + case POLICY_CHECK:
> > iint->ima_read_status = status;
> > break;
> > case FILE_CHECK:
>
> This uses an enum.
>
> > @@ -126,6 +128,7 @@ static void ima_cache_flags(struct integrity_iint_cache
> > *iint, int func)
> > case FIRMWARE_CHECK:
> > case KEXEC_CHECK:
> > case INITRAMFS_CHECK:
> > + case POLICY_CHECK:
> > iint->flags |= (IMA_READ_APPRAISED | IMA_APPRAISED);
> > break;
> > case FILE_CHECK:
>
> This uses an enum.
>
> All of these have a common set of funcs that will do similar things, what
> about
> just OR'ing them up in one place? That would make future additions one line
> instead of 3.
Ok
Mimi
--
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 6/7] ima: measure and appraise the IMA policy itself
On Tue, Dec 08, 2015 at 01:01:23PM -0500, Mimi Zohar wrote:
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 8a45576..4d149c9 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -222,6 +223,11 @@ int integrity_read_file(const char *path, char **data)
> return rc;
> }
>
> + if (!S_ISREG(file_inode(file)->i_mode)) {
> + rc = -EACCES;
> + goto out;
> + }
> +
> size = i_size_read(file_inode(file));
> if (size <= 0)
> goto out;
This hunk seems to be unrelated to this patch? If so can it be split out?
> @@ -232,13 +238,18 @@ int integrity_read_file(const char *path, char **data)
> goto out;
> }
>
> - rc = integrity_kernel_read(file, 0, buf, size);
> + rc = ima_read_and_process_file(file, read_func, buf, size);
> + if (rc == -EOPNOTSUPP) {
> + rc = integrity_kernel_read(file, 0, buf, size);
> + if (rc > 0 && rc != size)
> + rc = -EIO;
> + }
> if (rc < 0)
> kfree(buf);
> - else if (rc != size)
> - rc = -EIO;
> - else
> + else {
> + rc = size;
> *data = buf;
> + }
> out:
> fput(file);
> return rc;
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 548b258..40a24c3 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -180,6 +180,7 @@ int ima_policy_show(struct seq_file *m, void *v);
> #define IMA_APPRAISE_LOG 0x04
> #define IMA_APPRAISE_MODULES 0x08
> #define IMA_APPRAISE_FIRMWARE0x10
> +#define IMA_APPRAISE_POLICY 0x20
>
> #ifdef CONFIG_IMA_APPRAISE
> int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index b83049b..1e1a759 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -79,6 +79,7 @@ enum integrity_status ima_get_cache_status(struct
> integrity_iint_cache *iint,
> case FIRMWARE_CHECK:
> case KEXEC_CHECK:
> case INITRAMFS_CHECK:
> + case POLICY_CHECK:
> return iint->ima_read_status;
> case FILE_CHECK:
> default:
Hrm this uses an int for the func.
> @@ -102,6 +103,7 @@ static void ima_set_cache_status(struct
> integrity_iint_cache *iint,
> case FIRMWARE_CHECK:
> case KEXEC_CHECK:
> case INITRAMFS_CHECK:
> + case POLICY_CHECK:
> iint->ima_read_status = status;
> break;
> case FILE_CHECK:
This uses an enum.
> @@ -126,6 +128,7 @@ static void ima_cache_flags(struct integrity_iint_cache
> *iint, int func)
> case FIRMWARE_CHECK:
> case KEXEC_CHECK:
> case INITRAMFS_CHECK:
> + case POLICY_CHECK:
> iint->flags |= (IMA_READ_APPRAISED | IMA_APPRAISED);
> break;
> case FILE_CHECK:
This uses an enum.
All of these have a common set of funcs that will do similar things, what about
just OR'ing them up in one place? That would make future additions one line
instead of 3.
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
