Re: [Linux-ima-devel] [PATCH v2 4/7] ima: measure and appraise kexec image and initramfs
Hi, Mimi On 12/28/15 at 07:51am, Mimi Zohar wrote: > On Mon, 2015-12-28 at 10:08 +0800, Dave Young wrote: > > On 12/25/15 at 09:45am, Mimi Zohar wrote: > > > IMA calculates the file hash, in this case, based on the buffer > > > contents. The hash is calculated once and used for both measurement > > > and appraisal. If the file integrity appraisal fails (eg. hash > > > comparison or signature failure), IMA prevents the kexec files from > > > being used. > > > > > > > Ok, thanks for the explanatioin. But I have another question, why do we > > need a special hook for KEXEC? Shouldn't all files use same way to do the > > measurement and appraisal? > > "By all files" are you referring to all files read by the kernel or all > files opened, executed or mmapped by the system? Hmm, I means any kind of files read by the kernel. > > Currently IMA allocates a page sized buffer, reads a file a page chunk > at a time calculating the file hash as it does so, and then frees the > buffer before returning to the caller. This method of calculating the > file hash is used for measuring and appraising files opened > (FILE_CHECK), executed (BPRM_CHECK) or mmapped (MMAP_CHECK) by the > system. > > This patch set addresses files being read by kernel. A single new > generic hook named ima_hash_and_process_file() is defined to not only > measure and appraise the kexec image and initramfs, but firmware and the > IMA policy. As we identify other places that the kernel is reading > files, this hook would be called in those places as well. What I can not understand is why IMA need know the caller information and why cann't introduce a generic interface. kexec and firmware and other caller all read files, so a common file based interface should be better? Thanks Dave -- 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: [Linux-ima-devel] [PATCH v2 4/7] ima: measure and appraise kexec image and initramfs
Hi, Mimi
CCing kexec list, not all kexec people subscribed to IMA list.
I just subscribed to it since Vivek CCed me last time about the V1 of this
series.
On 12/23/15 at 06:55pm, Mimi Zohar wrote:
> This patch defines a new IMA hook ima_hash_and_process_file() for
> measuring and appraising files read by the kernel. The caller loads
> the file into memory before calling this function, which calculates
> the hash followed by the normal IMA policy based processing.
>
> Two new IMA policy functions named KEXEC_CHECK and INITRAMFS_CHECK
> are defined for measuring, appraising or auditing the kexec image
> and initramfs.
Could you help us understand why do we need it first.
I think I do not really understand the purpose of the IMA handling
about kexec kernel and initramfs.
* Does the files in disk space have already contains some hash values
and when kernel load it IMA functions will do some checking? But seems I do not
see such handling..
* Does it try to calculate the hash of the file buffer after copying,
and IMA will avoid future modification based on the hash calculated?
If this is the purpose I think it should be wrong because kexe file buffers
will be freed at the end of kexec_file_load syscall.
>
> Changelog v2:
> - Calculate the file hash from the in memory buffer (suggested by Dave Young)
> - Rename ima_read_and_process_file() to ima_hash_and_process_file()
> - replace individual case statements with range:
> KEXEC_CHECK ... IMA_MAX_READ_CHECK - 1
> v1:
> - Instead of ima_read_and_process_file() allocating memory, the caller
> allocates and frees the memory.
> - Moved the kexec measurement/appraisal call to copy_file_from_fd(). The
> same call now measures and appraises both the kexec image and initramfs.
>
> Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
> ---
> Documentation/ABI/testing/ima_policy | 2 +-
> include/linux/ima.h | 16 ++
> kernel/kexec_file.c | 24
> security/integrity/iint.c | 1 +
> security/integrity/ima/ima.h | 21 --
> security/integrity/ima/ima_api.c | 6 +++--
> security/integrity/ima/ima_appraise.c | 11 --
> security/integrity/ima/ima_main.c | 41
> ---
> security/integrity/ima/ima_policy.c | 38
> security/integrity/integrity.h| 7 --
> 10 files changed, 127 insertions(+), 40 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy
> b/Documentation/ABI/testing/ima_policy
> index 0a378a8..e80f767 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -26,7 +26,7 @@ Description:
> option: [[appraise_type=]] [permit_directio]
>
> base: func:=
> [BPRM_CHECK][MMAP_CHECK][FILE_CHECK][MODULE_CHECK]
> - [FIRMWARE_CHECK]
> + [FIRMWARE_CHECK] [KEXEC_CHECK] [INITRAMFS_CHECK]
> mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
> [[^]MAY_EXEC]
> fsmagic:= hex value
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 120ccc5..020de0f 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -13,6 +13,12 @@
> #include
> struct linux_binprm;
>
> +enum ima_read_hooks {
> + KEXEC_CHECK = 1,
> + INITRAMFS_CHECK,
> + IMA_MAX_READ_CHECK
> +};
> +
> #ifdef CONFIG_IMA
> extern int ima_bprm_check(struct linux_binprm *bprm);
> extern int ima_file_check(struct file *file, int mask, int opened);
> @@ -20,6 +26,9 @@ extern void ima_file_free(struct file *file);
> extern int ima_file_mmap(struct file *file, unsigned long prot);
> extern int ima_module_check(struct file *file);
> extern int ima_fw_from_file(struct file *file, char *buf, size_t size);
> +extern int ima_hash_and_process_file(struct file *file,
> + void *buf, size_t size,
> + enum ima_read_hooks read_func);
>
> #else
> static inline int ima_bprm_check(struct linux_binprm *bprm)
> @@ -52,6 +61,13 @@ static inline int ima_fw_from_file(struct file *file, char
> *buf, size_t size)
> return 0;
> }
>
> +static inline int ima_hash_and_process_file(struct file *file,
> + void *buf, size_t size,
> + enum ima_read_hooks read_func)
> +{
> + return 0;
> +}
> +
> #endif /* CONFIG_IMA */
>
> #ifdef CONFIG_IMA_APPRAISE
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index b70ada0..1d0d998
Re: [PATCH v1 4/7] ima: measure and appraise kexec image and initramfs
On 12/08/15 at 02:15pm, Mimi Zohar wrote:
> On Tue, 2015-12-08 at 13:32 -0500, Vivek Goyal wrote:
> > On Tue, Dec 08, 2015 at 01:01:21PM -0500, Mimi Zohar wrote:
> >
> > [..]
> > > #ifdef CONFIG_IMA_APPRAISE
> > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > index b70ada0..18c4a84 100644
> > > --- a/kernel/kexec_file.c
> > > +++ b/kernel/kexec_file.c
> > > @@ -18,6 +18,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -33,7 +34,8 @@ size_t __weak kexec_purgatory_size = 0;
> > >
> > > static int kexec_calculate_store_digests(struct kimage *image);
> > >
> > > -static int copy_file_from_fd(int fd, void **buf, unsigned long *buf_len)
> > > +static int copy_file_from_fd(int fd, enum ima_read_hooks read_func,
> > > + void **buf, unsigned long *buf_len)
> > > {
> > > struct fd f = fdget(fd);
> > > int ret;
> > > @@ -65,14 +67,17 @@ static int copy_file_from_fd(int fd, void **buf,
> > > unsigned long *buf_len)
> > > goto out;
> > > }
> > >
> > > + ret = ima_read_and_process_file(f.file, read_func, *buf, stat.size);
> > > + if (ret != -EOPNOTSUPP)
> > > + goto out_free;
> > > +
> >
> > Hi Mimi,
> >
> > I am wondering why are we designing this function to also read the file.
>
> > Looks like we just want an hook which calls into ima so that ima can
> > apply its *additional* policies on kernel and initramfs. If caller is
> > allocating the buffer, then caller can continue to read the file as well.
> > In fact that simplifies the code. Now caller which need to check
> > -EOPNOTSUPP and continue to read the file anyway.
>
> IMA is calculating the file hash as it reads the file. The file hash is
> then used for normal IMA processing - adding the measurement to the
> measurement list and verifying the file's integrity.
>
> > IOW, if caller still has to maintain the code to read the file, then it
> > is better that ima exports a hook which callers calls after reading the
> > file and ima can do its own verification.
>
> There's no sense in reading the file twice. The file hash should be
> calculated as the file is being read. Either the existing function to
> read the file needs to support calculating the file hash or it should
> call IMA.
Can IMA provide a function to calculate the hash from a buffer?
>
> There's a lot of code duplication for reading a file by the kernel (eg.
> kexec, firmware, kernel modules, ...). Each place does it just a bit
> differently than the other. There should be a single function for
> reading and calculating the file hash at the same time.
If you want to address the duplication for reading file, IMHO you can
introduce a general interface to read file in kernel space. But I do not
think the reading + calculating only interface is a good way.
>
> > Also why do we call second parameter as "read_func". I am really not
> > passing a function read function. May be something lile file_type might
> > be better.
>
> The read_func identifies the caller of ima_read_and_process_file().
> The IMA policy would then look like:
>
> measure func=KEXEC_CHECK
> appraise func=KEXEC_CHECK appraise_type=imasig
> #
> measure func=INITRAMFS_CHECK
> appraise func=INITRAMFS_CHECK appraise_type=imasig
> #
> measure func=FIRMWARE_CHECK
> appraise func=FIRMWARE_CHECK appraise_type=imasig
> #
> measure func=POLICY_CHECK
> appraise func=POLICY_CHECK appraise_type=imasig
>
> > So how does this additional IMA specific policies help (on top of existing
> > signature verification mechanism we have for kernel).
>
> The existing kexec signature verification is limited to verifying the
> kexec image on x86_64. It does not verify the kexec image on other
> architectures, nor does it verify the initramfs. The original method
> for verifying a files' integrity was IMA. We have need for verifying
> both the kexec image and initramfs.
>
> Mimi
>
Thanks
Dave
--
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
