| Message ID | 20190226215034.68772-4-matthewgarrett@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2,1/4] VFS: Add a call to obtain a file's hash | expand |
On Tue, 2019-02-26 at 13:50 -0800, Matthew Garrett wrote: > From: Matthew Garrett <mjg59@google.com> > > Some filesystems may be able to provide hashes in an out of band manner, > and allowing them to do so is a performance win. This is especially true > of FUSE-based filesystems where otherwise we recalculate the hash on > every measurement. To be more correct, please limit this statement to trusted mounted fuse filesystem. > Make use of this if policy says we should, but provide > a parameter to force recalculation rather than trusting the filesystem. > > Signed-off-by: Matthew Garrett <mjg59@google.com> > --- > Documentation/ABI/testing/ima_policy | 5 +++- > .../admin-guide/kernel-parameters.txt | 5 ++++ > security/integrity/ima/ima.h | 3 ++- > security/integrity/ima/ima_api.c | 5 +++- > security/integrity/ima/ima_crypto.c | 23 ++++++++++++++++++- > security/integrity/ima/ima_policy.c | 8 ++++++- > security/integrity/ima/ima_template_lib.c | 2 +- > security/integrity/integrity.h | 1 + > 8 files changed, 46 insertions(+), 6 deletions(-) > > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy > index 09a5def7e28a..6a517282068d 100644 > --- a/Documentation/ABI/testing/ima_policy > +++ b/Documentation/ABI/testing/ima_policy > @@ -24,7 +24,8 @@ Description: > [euid=] [fowner=] [fsname=] [subtype=]] > lsm: [[subj_user=] [subj_role=] [subj_type=] > [obj_user=] [obj_role=] [obj_type=]] > - option: [[appraise_type=]] [permit_directio] > + option: [[appraise_type=] [permit_directio] > + [trust_vfs]] Let's generalize "trust_vfs" a bit. How about introducing "collect_type=", with the default being reading and calculating the file hash? > > base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK] > [FIRMWARE_CHECK] > @@ -41,6 +42,8 @@ Description: > lsm: are LSM specific > option: appraise_type:= [imasig] > pcr:= decimal value > + permit_directio:= allow directio accesses > + trust_vfs:= trust VFS-provided hash values > > default policy: > # PROC_SUPER_MAGIC > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index 858b6c0b9a15..d3054a67e700 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -1640,6 +1640,11 @@ > different crypto accelerators. This option can be used > to achieve best performance for particular HW. > > + ima.force_hash= [IMA] Force hash calculation in IMA > + Format: <bool> > + Always calculate hashes rather than trusting the > + filesystem to provide them to us. Unless the IMA policy specifically defines rules with "trust_vfs", the default is to read the file, calculating the file hash. I don't see a need for this boot command line option. I do think that "trust_vfs" needs to be limited. Perhaps limiting it to a specific mounted filesystem? Perhaps limiting it to systems requiring signed IMA policies? Mimi > + > init= [KNL] > Format: <full_path> > Run specified binary instead of /sbin/init as init > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index cc12f3449a72..24d9b3a3bfc0 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -133,7 +133,8 @@ int ima_fs_init(void); > int ima_add_template_entry(struct ima_template_entry *entry, int violation, > const char *op, struct inode *inode, > const unsigned char *filename); > -int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash); > +int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash, > + bool trust_vfs); > int ima_calc_buffer_hash(const void *buf, loff_t len, > struct ima_digest_data *hash); > int ima_calc_field_array_hash(struct ima_field_data *field_data, > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c > index c7505fb122d4..0def9cf43549 100644 > --- a/security/integrity/ima/ima_api.c > +++ b/security/integrity/ima/ima_api.c > @@ -206,6 +206,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, > int length; > void *tmpbuf; > u64 i_version; > + bool trust_vfs; > struct { > struct ima_digest_data hdr; > char digest[IMA_MAX_DIGEST_SIZE]; > @@ -225,10 +226,12 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, > /* Initialize hash digest to 0's in case of failure */ > memset(&hash.digest, 0, sizeof(hash.digest)); > > + trust_vfs = iint->flags & IMA_TRUST_VFS; > + > if (buf) > result = ima_calc_buffer_hash(buf, size, &hash.hdr); > else > - result = ima_calc_file_hash(file, &hash.hdr); > + result = ima_calc_file_hash(file, &hash.hdr, trust_vfs); > > if (result && result != -EBADF && result != -EINVAL) > goto out; > diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c > index acf2c7df7145..94105089ad41 100644 > --- a/security/integrity/ima/ima_crypto.c > +++ b/security/integrity/ima/ima_crypto.c > @@ -32,6 +32,10 @@ static unsigned long ima_ahash_minsize; > module_param_named(ahash_minsize, ima_ahash_minsize, ulong, 0644); > MODULE_PARM_DESC(ahash_minsize, "Minimum file size for ahash use"); > > +static bool ima_force_hash; > +module_param_named(force_hash, ima_force_hash, bool_enable_only, 0644); > +MODULE_PARM_DESC(force_hash, "Always calculate hashes"); > + > /* default is 0 - 1 page. */ > static int ima_maxorder; > static unsigned int ima_bufsize = PAGE_SIZE; > @@ -402,11 +406,14 @@ static int ima_calc_file_shash(struct file *file, struct ima_digest_data *hash) > * shash for the hash calculation. If ahash fails, it falls back to using > * shash. > */ > -int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) > +int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash, > + bool trust_vfs) > { > loff_t i_size; > int rc; > struct file *f = file; > + struct dentry *dentry = file_dentry(file); > + struct inode *inode = d_backing_inode(dentry); > bool new_file_instance = false, modified_flags = false; > > /* > @@ -419,6 +426,20 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) > return -EINVAL; > } > > + /* > + * If policy says we should trust VFS-provided hashes, and > + * we're not configured to force hashing, and if the > + * filesystem is trusted, ask the VFS if it can obtain the > + * hash without us having to calculate it ourself. > + */ > + if (trust_vfs && !ima_force_hash && > + !(inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER)) { > + hash->length = hash_digest_size[hash->algo]; > + rc = vfs_get_hash(file, hash->algo, hash->digest, hash->length); > + if (!rc) > + return 0; > + } > + > /* Open a new file instance in O_RDONLY if we cannot read */ > if (!(file->f_mode & FMODE_READ)) { > int flags = file->f_flags & ~(O_WRONLY | O_APPEND | > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index dcecb6aae5ec..af632c31f856 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -683,7 +683,7 @@ enum { > Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt, > Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt, > Opt_appraise_type, Opt_permit_directio, > - Opt_pcr, Opt_err > + Opt_pcr, Opt_trust_vfs, Opt_err > }; > > static const match_table_t policy_tokens = { > @@ -718,6 +718,7 @@ static const match_table_t policy_tokens = { > {Opt_appraise_type, "appraise_type=%s"}, > {Opt_permit_directio, "permit_directio"}, > {Opt_pcr, "pcr=%s"}, > + {Opt_trust_vfs, "trust_vfs"}, > {Opt_err, NULL} > }; > > @@ -1060,6 +1061,9 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) > case Opt_permit_directio: > entry->flags |= IMA_PERMIT_DIRECTIO; > break; > + case Opt_trust_vfs: > + entry->flags |= IMA_TRUST_VFS; > + break; > case Opt_pcr: > if (entry->action != MEASURE) { > result = -EINVAL; > @@ -1356,6 +1360,8 @@ int ima_policy_show(struct seq_file *m, void *v) > seq_puts(m, "appraise_type=imasig "); > if (entry->flags & IMA_PERMIT_DIRECTIO) > seq_puts(m, "permit_directio "); > + if (entry->flags & IMA_TRUST_VFS) > + seq_puts(m, "trust_vfs "); > rcu_read_unlock(); > seq_puts(m, "\n"); > return 0; > diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c > index 43752002c222..a36cdc6651b7 100644 > --- a/security/integrity/ima/ima_template_lib.c > +++ b/security/integrity/ima/ima_template_lib.c > @@ -290,7 +290,7 @@ int ima_eventdigest_init(struct ima_event_data *event_data, > inode = file_inode(event_data->file); > hash.hdr.algo = ima_template_hash_algo_allowed(ima_hash_algo) ? > ima_hash_algo : HASH_ALGO_SHA1; > - result = ima_calc_file_hash(event_data->file, &hash.hdr); > + result = ima_calc_file_hash(event_data->file, &hash.hdr, false); > if (result) { > integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, > event_data->filename, "collect_data", > diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h > index 7de59f44cba3..a03f859c1602 100644 > --- a/security/integrity/integrity.h > +++ b/security/integrity/integrity.h > @@ -36,6 +36,7 @@ > #define IMA_NEW_FILE 0x04000000 > #define EVM_IMMUTABLE_DIGSIG 0x08000000 > #define IMA_FAIL_UNVERIFIABLE_SIGS 0x10000000 > +#define IMA_TRUST_VFS 0x20000000 > > #define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \ > IMA_HASH | IMA_APPRAISE_SUBMASK)
> > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy > > index 09a5def7e28a..6a517282068d 100644 > > --- a/Documentation/ABI/testing/ima_policy > > +++ b/Documentation/ABI/testing/ima_policy > > @@ -24,7 +24,8 @@ Description: > > [euid=] [fowner=] [fsname=] [subtype=]] > > lsm: [[subj_user=] [subj_role=] [subj_type=] > > [obj_user=] [obj_role=] [obj_type=]] > > - option: [[appraise_type=]] [permit_directio] > > + option: [[appraise_type=] [permit_directio] > > + [trust_vfs]] > > Let's generalize "trust_vfs" a bit. How about introducing > "collect_type=", with the default being reading and calculating the > file hash? The naming might be based on the VFS name (e.g vfs_read, vfs_get_hash) or on the file_operations name (eg. read, get_hash). Mimi
On Thu, Feb 28, 2019 at 10:05 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy > > > index 09a5def7e28a..6a517282068d 100644 > > > --- a/Documentation/ABI/testing/ima_policy > > > +++ b/Documentation/ABI/testing/ima_policy > > > @@ -24,7 +24,8 @@ Description: > > > [euid=] [fowner=] [fsname=] [subtype=]] > > > lsm: [[subj_user=] [subj_role=] [subj_type=] > > > [obj_user=] [obj_role=] [obj_type=]] > > > - option: [[appraise_type=]] [permit_directio] > > > + option: [[appraise_type=] [permit_directio] > > > + [trust_vfs]] > > > > Let's generalize "trust_vfs" a bit. How about introducing > > "collect_type=", with the default being reading and calculating the > > file hash? > > The naming might be based on the VFS name (e.g vfs_read, vfs_get_hash) > or on the file_operations name (eg. read, get_hash). If collect_type=get_hash and the filesystem doesn't support the get_hash type, should the behaviour be to fall back to read?
On Thu, 2019-02-28 at 13:41 -0800, Matthew Garrett wrote: > On Thu, Feb 28, 2019 at 10:05 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > > > > > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy > > > > index 09a5def7e28a..6a517282068d 100644 > > > > --- a/Documentation/ABI/testing/ima_policy > > > > +++ b/Documentation/ABI/testing/ima_policy > > > > @@ -24,7 +24,8 @@ Description: > > > > [euid=] [fowner=] [fsname=] [subtype=]] > > > > lsm: [[subj_user=] [subj_role=] [subj_type=] > > > > [obj_user=] [obj_role=] [obj_type=]] > > > > - option: [[appraise_type=]] [permit_directio] > > > > + option: [[appraise_type=] [permit_directio] > > > > + [trust_vfs]] > > > > > > Let's generalize "trust_vfs" a bit. How about introducing > > > "collect_type=", with the default being reading and calculating the > > > file hash? > > > > The naming might be based on the VFS name (e.g vfs_read, vfs_get_hash) > > or on the file_operations name (eg. read, get_hash). > > If collect_type=get_hash and the filesystem doesn't support the > get_hash type, should the behaviour be to fall back to read? "get_hash" should be limited to a specific filesystem type and subtype. Based on the filesystem type and subtype, couldn't a warning be emitted at policy load time. Mimi
On Thu, Feb 28, 2019 at 1:59 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > On Thu, 2019-02-28 at 13:41 -0800, Matthew Garrett wrote: > > If collect_type=get_hash and the filesystem doesn't support the > > get_hash type, should the behaviour be to fall back to read? > > "get_hash" should be limited to a specific filesystem type and > subtype. Based on the filesystem type and subtype, couldn't a warning > be emitted at policy load time. The policy may be loaded before the filesystem is mounted, so even if we added a capabilities mechanism we wouldn't be able to verify it. There's also potentially cases where a filesystem could support hash retrieval for some files but not others, and in that case we'd probably want to fall back to reading the file.
On Thu, Feb 28, 2019 at 2:38 PM Matthew Garrett <mjg59@google.com> wrote: > > On Thu, Feb 28, 2019 at 1:59 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Thu, 2019-02-28 at 13:41 -0800, Matthew Garrett wrote: > > > If collect_type=get_hash and the filesystem doesn't support the > > > get_hash type, should the behaviour be to fall back to read? > > > > "get_hash" should be limited to a specific filesystem type and > > subtype. Based on the filesystem type and subtype, couldn't a warning > > be emitted at policy load time. > > The policy may be loaded before the filesystem is mounted, so even if > we added a capabilities mechanism we wouldn't be able to verify it. > There's also potentially cases where a filesystem could support hash > retrieval for some files but not others, and in that case we'd > probably want to fall back to reading the file. To be clear, I'm entirely happy to make this change - I'd just like to ensure that I do it the right way!
On Mon, 2019-03-04 at 11:52 -0800, Matthew Garrett wrote: > On Thu, Feb 28, 2019 at 2:38 PM Matthew Garrett <mjg59@google.com> wrote: > > > > On Thu, Feb 28, 2019 at 1:59 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > On Thu, 2019-02-28 at 13:41 -0800, Matthew Garrett wrote: > > > > If collect_type=get_hash and the filesystem doesn't support the > > > > get_hash type, should the behaviour be to fall back to read? > > > > > > "get_hash" should be limited to a specific filesystem type and > > > subtype. Based on the filesystem type and subtype, couldn't a warning > > > be emitted at policy load time. > > > > The policy may be loaded before the filesystem is mounted, so even if > > we added a capabilities mechanism we wouldn't be able to verify it. > > There's also potentially cases where a filesystem could support hash > > retrieval for some files but not others, and in that case we'd > > probably want to fall back to reading the file. > > To be clear, I'm entirely happy to make this change - I'd just like to > ensure that I do it the right way! Falling back to reading the file is fine. So we're assuming that the person signing a policy containing "get_hash" understands the ramifications. And yes, only signed policies containing "get_hash" should be loaded. I'd really appreciate a regression test (eg. ltp, xfstests, or kselftests). Mimi
On Mon, Mar 4, 2019 at 12:32 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > On Mon, 2019-03-04 at 11:52 -0800, Matthew Garrett wrote: > > To be clear, I'm entirely happy to make this change - I'd just like to > > ensure that I do it the right way! > > Falling back to reading the file is fine. So we're assuming that the > person signing a policy containing "get_hash" understands the > ramifications. And yes, only signed policies containing "get_hash" > should be loaded. I'm not clear on why requiring signed policies is helpful here. If you allow FUSE mounts at all then you need to trust the FUSE filesystem to return good results, in which case you can trust it to return valid hashes. If you don't trust the FUSE filesystem then generating the hash via read doesn't win you anything - the filesystem can return one set of data on the initial IMA hashing, and then return a second set later. Requiring signed policy doesn't change that.
On Mon, 2019-03-04 at 14:10 -0800, Matthew Garrett wrote: > On Mon, Mar 4, 2019 at 12:32 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Mon, 2019-03-04 at 11:52 -0800, Matthew Garrett wrote: > > > To be clear, I'm entirely happy to make this change - I'd just like to > > > ensure that I do it the right way! > > > > Falling back to reading the file is fine. So we're assuming that the > > person signing a policy containing "get_hash" understands the > > ramifications. And yes, only signed policies containing "get_hash" > > should be loaded. > > I'm not clear on why requiring signed policies is helpful here. If you > allow FUSE mounts at all then you need to trust the FUSE filesystem to > return good results, in which case you can trust it to return valid > hashes. If you don't trust the FUSE filesystem then generating the > hash via read doesn't win you anything - the filesystem can return one > set of data on the initial IMA hashing, and then return a second set > later. Requiring signed policy doesn't change that. You're defining a new generic file ops "get_hash", but are using FUSE, a specific filesystem, as an example. Requiring the IMA policy to be signed when using "get_hash", is proof of the sysadmin's agreement to bypass actually reading and calculating the file hash. Mimi
On Tue, Mar 5, 2019 at 5:19 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Mon, 2019-03-04 at 14:10 -0800, Matthew Garrett wrote: > > I'm not clear on why requiring signed policies is helpful here. If you > > allow FUSE mounts at all then you need to trust the FUSE filesystem to > > return good results, in which case you can trust it to return valid > > hashes. If you don't trust the FUSE filesystem then generating the > > hash via read doesn't win you anything - the filesystem can return one > > set of data on the initial IMA hashing, and then return a second set > > later. Requiring signed policy doesn't change that. > > You're defining a new generic file ops "get_hash", but are using FUSE, > a specific filesystem, as an example. Requiring the IMA policy to be > signed when using "get_hash", is proof of the sysadmin's agreement to > bypass actually reading and calculating the file hash. We can trust in-kernel filesystems to return reliable information. Network filesystems have the same issue as FUSE - we're trusting that the remote endpoint won't give us different information on successive reads. What's the threat that's blocked by requiring signed policy here?
On Tue, 2019-03-05 at 10:39 -0800, Matthew Garrett wrote: > On Tue, Mar 5, 2019 at 5:19 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > On Mon, 2019-03-04 at 14:10 -0800, Matthew Garrett wrote: > > > I'm not clear on why requiring signed policies is helpful here. If you > > > allow FUSE mounts at all then you need to trust the FUSE filesystem to > > > return good results, in which case you can trust it to return valid > > > hashes. If you don't trust the FUSE filesystem then generating the > > > hash via read doesn't win you anything - the filesystem can return one > > > set of data on the initial IMA hashing, and then return a second set > > > later. Requiring signed policy doesn't change that. > > > > You're defining a new generic file ops "get_hash", but are using FUSE, > > a specific filesystem, as an example. Requiring the IMA policy to be > > signed when using "get_hash", is proof of the sysadmin's agreement to > > bypass actually reading and calculating the file hash. > > We can trust in-kernel filesystems to return reliable information. > Network filesystems have the same issue as FUSE - we're trusting that > the remote endpoint won't give us different information on successive > reads. What's the threat that's blocked by requiring signed policy > here? Today, IMA calculates the file hash by reading the file. If "get_hash" is a generic filesystem ops, then any filesystem could implement it, properly or not. sysadmins shouldn't have to review kernel code to understand the source of the file hash, but should be able to assume that unless they explicitly authorize "get_hash" usage, IMA reads the file and calculates the file hash. Mimi
On Tue, Mar 5, 2019 at 11:51 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > On Tue, 2019-03-05 at 10:39 -0800, Matthew Garrett wrote: > > We can trust in-kernel filesystems to return reliable information. > > Network filesystems have the same issue as FUSE - we're trusting that > > the remote endpoint won't give us different information on successive > > reads. What's the threat that's blocked by requiring signed policy > > here? > > Today, IMA calculates the file hash by reading the file. If > "get_hash" is a generic filesystem ops, then any filesystem could > implement it, properly or not. sysadmins shouldn't have to review > kernel code to understand the source of the file hash, but should be > able to assume that unless they explicitly authorize "get_hash" usage, > IMA reads the file and calculates the file hash. But what's the threat? If an attacker is in a position to inject additional IMA policy then in general they're already in a position to violate other security assumptions. Admins who have a threat model that includes an attacker being able to do this are already requiring signed policy. What's the threat that requiring signed policy for this specific option mitigates?
On Tue, 2019-03-05 at 12:27 -0800, Matthew Garrett wrote: > On Tue, Mar 5, 2019 at 11:51 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Tue, 2019-03-05 at 10:39 -0800, Matthew Garrett wrote: > > > We can trust in-kernel filesystems to return reliable information. > > > Network filesystems have the same issue as FUSE - we're trusting that > > > the remote endpoint won't give us different information on successive > > > reads. What's the threat that's blocked by requiring signed policy > > > here? > > > > Today, IMA calculates the file hash by reading the file. If > > "get_hash" is a generic filesystem ops, then any filesystem could > > implement it, properly or not. sysadmins shouldn't have to review > > kernel code to understand the source of the file hash, but should be > > able to assume that unless they explicitly authorize "get_hash" usage, > > IMA reads the file and calculates the file hash. > > But what's the threat? If an attacker is in a position to inject > additional IMA policy then in general they're already in a position to > violate other security assumptions. Admins who have a threat model > that includes an attacker being able to do this are already requiring > signed policy. What's the threat that requiring signed policy for this > specific option mitigates? That might be true, but this "feature" isn't a minor change. It totally changes the IMA measurement list meaning, without any indication of the change in meaning. Mimi
On Wed, Mar 6, 2019 at 4:30 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Tue, 2019-03-05 at 12:27 -0800, Matthew Garrett wrote: > > But what's the threat? If an attacker is in a position to inject > > additional IMA policy then in general they're already in a position to > > violate other security assumptions. Admins who have a threat model > > that includes an attacker being able to do this are already requiring > > signed policy. What's the threat that requiring signed policy for this > > specific option mitigates? > > That might be true, but this "feature" isn't a minor change. It > totally changes the IMA measurement list meaning, without any > indication of the change in meaning. Ok. Would annotating the audit message to indicate that the hash was provided directly by the filesystem be sufficient? I'm not clear on why an admin would set this flag without having read the documentation for it - like many security features, enabling an inappropriate combination of them may result in bad things happening. I'm not keen on tying it to signing because: a) There are multiple configurations where requiring signed policy doesn't give a security benefit - if the IMA policy is part of a verified or measured initramfs, we already have integrity guarantees and adding an additional layer of signing doesn't win us anything (eg, in this configuration the IMA key may be loaded from the initramfs as well, so an attacker able to modify policy could add an additional signing key). b) Users who are already using signed policy won't get the additional hint that you think is necessary. I'm happy to add this if there's a real threat model around it, but requiring signing for something other than security reasons seems like it's conflating unrelated issues.
On Wed, 2019-03-06 at 10:31 -0800, Matthew Garrett wrote: > On Wed, Mar 6, 2019 at 4:30 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > On Tue, 2019-03-05 at 12:27 -0800, Matthew Garrett wrote: > > > But what's the threat? If an attacker is in a position to inject > > > additional IMA policy then in general they're already in a position to > > > violate other security assumptions. Admins who have a threat model > > > that includes an attacker being able to do this are already requiring > > > signed policy. What's the threat that requiring signed policy for this > > > specific option mitigates? > > > > That might be true, but this "feature" isn't a minor change. It > > totally changes the IMA measurement list meaning, without any > > indication of the change in meaning. > > Ok. Would annotating the audit message to indicate that the hash was > provided directly by the filesystem be sufficient? The audit log doesn't have the same security properties as the TPM quote and IMA measurement list. Nor does the attestation server necessarily have access to it. > I'm not clear on > why an admin would set this flag without having read the documentation > for it - like many security features, enabling an inappropriate > combination of them may result in bad things happening. I'm not keen > on tying it to signing because: > > a) There are multiple configurations where requiring signed policy > doesn't give a security benefit - if the IMA policy is part of a > verified or measured initramfs, we already have integrity guarantees > and adding an additional layer of signing doesn't win us anything (eg, > in this configuration the IMA key may be loaded from the initramfs as > well, so an attacker able to modify policy could add an additional > signing key). Agreed, as long as there is no possibility of additional files being installed/downloaded to the rootfs or files on other filesystems being accessed before the IMA keyring is locked or a custom policy is installed, a verified or measured initramfs might be enough. This implies that both the custom policy and the keys loaded onto the IMA keyring are included in the initramfs, which isn't what is currently being done today. My preference would be to remove the original method of loading unsigned IMA policies, but that could/would break existing systems. > b) Users who are already using signed policy won't get the additional > hint that you think is necessary. But they would have to knowingly add "get_hash" to the IMA policy and have signed it. > I'm happy to add this if there's a real threat model around it, but > requiring signing for something other than security reasons seems like > it's conflating unrelated issues. A colleague said, relying on the filesystem to provide the file hash extends the TCB to include filesystems. Mimi
On Wed, Mar 6, 2019 at 2:39 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Wed, 2019-03-06 at 10:31 -0800, Matthew Garrett wrote: > > Ok. Would annotating the audit message to indicate that the hash was > > provided directly by the filesystem be sufficient? > > The audit log doesn't have the same security properties as the TPM > quote and IMA measurement list. Nor does the attestation server > necessarily have access to it. Ok. > > I'm not clear on > > why an admin would set this flag without having read the documentation > > for it - like many security features, enabling an inappropriate > > combination of them may result in bad things happening. I'm not keen > > on tying it to signing because: > > > > a) There are multiple configurations where requiring signed policy > > doesn't give a security benefit - if the IMA policy is part of a > > verified or measured initramfs, we already have integrity guarantees > > and adding an additional layer of signing doesn't win us anything (eg, > > in this configuration the IMA key may be loaded from the initramfs as > > well, so an attacker able to modify policy could add an additional > > signing key). > > Agreed, as long as there is no possibility of additional files being > installed/downloaded to the rootfs or files on other filesystems being > accessed before the IMA keyring is locked or a custom policy is > installed, a verified or measured initramfs might be enough. > > This implies that both the custom policy and the keys loaded onto the > IMA keyring are included in the initramfs, which isn't what is > currently being done today. My preference would be to remove the > original method of loading unsigned IMA policies, but that could/would > break existing systems. It's the way we handle IMA configuration - the policy is loaded and further policy loads are disabled due to IMA_WRITE_POLICY being default n. We're not currently making use of signing, but longer term plan is for the keys to be loaded at the same time. > > b) Users who are already using signed policy won't get the additional > > hint that you think is necessary. > > But they would have to knowingly add "get_hash" to the IMA policy and > have signed it. But in the non-signed case they'd still need to knowingly add "get_hash" to the IMA policy. Why does signing indicate stronger understanding of policy? If my understanding of ima_match_policy() correct, if there's already a measurement rule that applies to a filesystem then adding an additional trust_vfs rule will be ignored, so once the initial policy is loaded it's not possible for someone to transition a filesystem from a full read to using the vfs call. IE, a policy like: measure measure fsmagic=0x46555345 trust_vfs is still going to perform the full measurement even on FUSE. > > I'm happy to add this if there's a real threat model around it, but > > requiring signing for something other than security reasons seems like > > it's conflating unrelated issues. > > A colleague said, relying on the filesystem to provide the file hash > extends the TCB to include filesystems. The TCB already includes filesystems - IMA's measurements are only accurate if the filesystem returns the same data on subsequent reads (assuming i_version hasn't been updated). We assert that this is true, but it the filesystem is outside the TCB then that assertion is invalid.
On Wed, 2019-03-06 at 15:36 -0800, Matthew Garrett wrote: > > But they would have to knowingly add "get_hash" to the IMA policy and > > have signed it. > > But in the non-signed case they'd still need to knowingly add > "get_hash" to the IMA policy. Why does signing indicate stronger > understanding of policy? Nobody is suggesting that signing the policy is a stronger indication of understanding the policy. Signing the policy simply limits which policies may be loaded. > If my understanding of ima_match_policy() > correct, if there's already a measurement rule that applies to a > filesystem then adding an additional trust_vfs rule will be ignored, > so once the initial policy is loaded it's not possible for someone to > transition a filesystem from a full read to using the vfs call. IE, a > policy like: > > measure > measure fsmagic=0x46555345 trust_vfs > > is still going to perform the full measurement even on FUSE. This scenario assumes that a custom policy has already been loaded and that there are default catchall rules. I really do hope that anybody enabling support for loading multiple policies requires those policies to be signed, like the builtin appraise policy. > > > > I'm happy to add this if there's a real threat model around it, but > > > requiring signing for something other than security reasons seems like > > > it's conflating unrelated issues. > > > > A colleague said, relying on the filesystem to provide the file hash > > extends the TCB to include filesystems. > > The TCB already includes filesystems - IMA's measurements are only > accurate if the filesystem returns the same data on subsequent reads > (assuming i_version hasn't been updated). We assert that this is true, > but it the filesystem is outside the TCB then that assertion is > invalid. There is also a difference between trusting the filesystem "read" and the filesystem "get_hash" implementation, that have yet to be written. Mimi
On Wed, Mar 6, 2019 at 5:54 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Wed, 2019-03-06 at 15:36 -0800, Matthew Garrett wrote: > > > > But they would have to knowingly add "get_hash" to the IMA policy and > > > have signed it. > > > > But in the non-signed case they'd still need to knowingly add > > "get_hash" to the IMA policy. Why does signing indicate stronger > > understanding of policy? > > Nobody is suggesting that signing the policy is a stronger indication > of understanding the policy. Signing the policy simply limits which > policies may be loaded. I'm not sure I understand the additional risk, though. Either a filesystem is already being measured using a full read, in which case adding an additional rule won't change behaviour, or it's not being measured at all, in which case there's no incentive for an attacker to add a new rule. > > > > I'm happy to add this if there's a real threat model around it, but > > > > requiring signing for something other than security reasons seems like > > > > it's conflating unrelated issues. > > > > > > A colleague said, relying on the filesystem to provide the file hash > > > extends the TCB to include filesystems. > > > > The TCB already includes filesystems - IMA's measurements are only > > accurate if the filesystem returns the same data on subsequent reads > > (assuming i_version hasn't been updated). We assert that this is true, > > but it the filesystem is outside the TCB then that assertion is > > invalid. > > There is also a difference between trusting the filesystem "read" and > the filesystem "get_hash" implementation, that have yet to be written. In both cases we're placing trust in the filesystem's correctness. It's certainly possible for the get_hash call to be broken, but that's something we can put additional testing into. The same argument implies that enabling appraisal is increasing the amount of trust we place in the filesystem - without it we don't need getxattr() to work, and without digital signatures we don't need the kernel's RSA code to work. Most new features result in us relying on more code, and in some cases we're not even able to audit that code (eg, if you're not using an encrypted filesystem then we're depending on the correctness of your disk's firmware, and we know that there are implementations that don't verify updates or which provide debug commands that allow modification at runtime in a way that could be used to defeat IMA)
On Wed, 2019-03-06 at 20:19 -0800, Matthew Garrett wrote: > On Wed, Mar 6, 2019 at 5:54 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > On Wed, 2019-03-06 at 15:36 -0800, Matthew Garrett wrote: > > > > > > But they would have to knowingly add "get_hash" to the IMA policy and > > > > have signed it. > > > > > > But in the non-signed case they'd still need to knowingly add > > > "get_hash" to the IMA policy. Why does signing indicate stronger > > > understanding of policy? > > > > Nobody is suggesting that signing the policy is a stronger indication > > of understanding the policy. Signing the policy simply limits which > > policies may be loaded. > > I'm not sure I understand the additional risk, though. Either a > filesystem is already being measured using a full read, in which case > adding an additional rule won't change behaviour, or it's not being > measured at all, in which case there's no incentive for an attacker to > add a new rule. In an environment where the initramfs contains everything, including the IMA policy, and is verified, then what you're saying is true - the IMA policy doesn't need to be signed. However, the existing dracut and systemd IMA modules read the IMA policy not from the initramfs, but from real root. In this environment where the IMA policy is on real root, an attacker could modify the IMA policy. I realize this is an existing problem, not particular to "get_hash'. Unlike other policy changes, the attestation server would have no way of detecting this particular change. > > There is also a difference between trusting the filesystem "read" and > > the filesystem "get_hash" implementation, that have yet to be written. > > In both cases we're placing trust in the filesystem's correctness. > It's certainly possible for the get_hash call to be broken, but that's > something we can put additional testing into. The call itself wouldn't be broken, but determining if the filesystem is actually calculating/re-calculating the file hash properly or simply returning a stored/cached value. I do see the benefit of the filesystems being responsible for the file hash. Perhaps I'm just being overly cautious. I'd like to hear other people's opinions. Mimi
On Thu, Mar 7, 2019 at 12:48 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Wed, 2019-03-06 at 20:19 -0800, Matthew Garrett wrote: > > I'm not sure I understand the additional risk, though. Either a > > filesystem is already being measured using a full read, in which case > > adding an additional rule won't change behaviour, or it's not being > > measured at all, in which case there's no incentive for an attacker to > > add a new rule. > > In an environment where the initramfs contains everything, including > the IMA policy, and is verified, then what you're saying is true - the > IMA policy doesn't need to be signed. However, the existing dracut > and systemd IMA modules read the IMA policy not from the initramfs, > but from real root. In this environment where the IMA policy is on > real root, an attacker could modify the IMA policy. I realize this is > an existing problem, not particular to "get_hash'. Unlike other > policy changes, the attestation server would have no way of detecting > this particular change. Ah, got you. Would measuring the policy be sufficient here? > > In both cases we're placing trust in the filesystem's correctness. > > It's certainly possible for the get_hash call to be broken, but that's > > something we can put additional testing into. > > The call itself wouldn't be broken, but determining if the filesystem > is actually calculating/re-calculating the file hash properly or > simply returning a stored/cached value. I do see the benefit of the > filesystems being responsible for the file hash. Perhaps I'm just > being overly cautious. I'd like to hear other people's opinions. Yup, happy to get further feedback on this.
On Thu, Mar 7, 2019 at 2:41 PM Matthew Garrett <mjg59@google.com> wrote:
> Yup, happy to get further feedback on this.
Anyone other than me and Mimi with thoughts here? :)
On Thu, 2019-04-04 at 14:46 -0700, Matthew Garrett wrote: > On Thu, Mar 7, 2019 at 2:41 PM Matthew Garrett <mjg59@google.com> > wrote: > > Yup, happy to get further feedback on this. > > Anyone other than me and Mimi with thoughts here? :) The obvious other thought is integration with fs-verity, which is a filesystem maintained possibly signed merkel tree hash. The problem here is what does vfs_get_hash() actually mean? The assumption seems to be that it is the flat hash of the entire file which doesn't work for merkle trees. However, if it could be a representative hash of the file which is produced however the filesystem decides, it could work (well, unless the file is copied on to a different fs, of course ...). James
On Thu, Apr 4, 2019 at 3:18 PM James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > The obvious other thought is integration with fs-verity, which is a > filesystem maintained possibly signed merkel tree hash. The problem > here is what does vfs_get_hash() actually mean? The assumption seems > to be that it is the flat hash of the entire file which doesn't work > for merkle trees. However, if it could be a representative hash of the > file which is produced however the filesystem decides, it could work > (well, unless the file is copied on to a different fs, of course ...). We could always use fs-verity to store additional verifiable metadata including actual hashes for consistency?
On Thu, 2019-04-04 at 15:26 -0700, Matthew Garrett wrote: > On Thu, Apr 4, 2019 at 3:18 PM James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > The obvious other thought is integration with fs-verity, which is a > > filesystem maintained possibly signed merkel tree hash. The > > problem here is what does vfs_get_hash() actually mean? The > > assumption seems to be that it is the flat hash of the entire file > > which doesn't work for merkle trees. However, if it could be a > > representative hash of the file which is produced however the > > filesystem decides, it could work (well, unless the file is copied > > on to a different fs, of course ...). > > We could always use fs-verity to store additional verifiable metadata > including actual hashes for consistency? Redundant information is always possible, but it can become inconsistent and, because the hashes can't be derived from each other, it's hard to tell if it is inconsistent without redoing the whole hash with each method. I was more wondering what, if any, problems would follow if we did let the filesystem choose the hash method and simply used the top merkle hash in place of the usual IMA hash? James
On Thu, Apr 4, 2019 at 3:35 PM James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > Redundant information is always possible, but it can become > inconsistent and, because the hashes can't be derived from each other, > it's hard to tell if it is inconsistent without redoing the whole hash > with each method. Part of the problem here is that IMA is effectively used for two related but different purposes - measurement and appraisal. You generally want measurements to be comparable across filesystems, whereas appraisal doesn't need to be. So if we don't have comparable measurements, there's less benefit in performing measurement (we have no real idea what the expected measurements would be in advance). That's less important for appraisal, but arguably we don't care about appraisal of stuff on fs-verity backed filesystems to begin with because we can just attest that they're legitimate? > I was more wondering what, if any, problems would follow if we did let > the filesystem choose the hash method and simply used the top merkle > hash in place of the usual IMA hash? We could definitely just pass it through as a separate hash type, and my initial thinking was that fs-verity might be a reasonable use case for that, but I'm not sure that it buys us much in the IMA case.
On Thu, 2019-04-04 at 18:50 -0700, Matthew Garrett wrote: > On Thu, Apr 4, 2019 at 3:35 PM James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > Redundant information is always possible, but it can become > > inconsistent and, because the hashes can't be derived from each > > other, it's hard to tell if it is inconsistent without redoing the > > whole hash with each method. > > Part of the problem here is that IMA is effectively used for two > related but different purposes - measurement and appraisal. You > generally want measurements to be comparable across filesystems, > whereas appraisal doesn't need to be. Sure, but I think the only requirement for measurement is knowing how to reproduce them. As long as you know the algorithm the filesystem is using ... i.e. it's recorded in the IMA log, you should be able to verify them. > So if we don't have comparable measurements, there's less benefit in > performing measurement (we have no real idea what the expected > measurements would be in advance). As long as the algorithm used for the measurement is recorded, I don't think there's a problem. The IMA log currently records the hash algorithm and the actual hash, so if we take shaX to be a flat hash, we could use shaX-merkle for fs-verity and everything would work. > That's less important for appraisal, but arguably we don't care about > appraisal of stuff on fs-verity backed filesystems to begin with > because we can just attest that they're legitimate? I think Ted mentioned they did like to sign the merkle tree to prove the apk being installed was legitimate, so I think both measurement and appraisal are relevant. > > I was more wondering what, if any, problems would follow if we did > > let the filesystem choose the hash method and simply used the top > > merkle hash in place of the usual IMA hash? > > We could definitely just pass it through as a separate hash type, and > my initial thinking was that fs-verity might be a reasonable use case > for that, but I'm not sure that it buys us much in the IMA case. Unifying the interfaces for measurement and appraisal sounds like a desirable thing. IMA has just been debating measurement on mmap and the per-page hashes of fs-verity seem to be tailor made for this. Note: I'm not insisting on this, you just asked for other feedback and I think it's a useful discussion. James
On Thu, Apr 4, 2019 at 7:27 PM James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > > On Thu, 2019-04-04 at 18:50 -0700, Matthew Garrett wrote: > > On Thu, Apr 4, 2019 at 3:35 PM James Bottomley > > <James.Bottomley@hansenpartnership.com> wrote: > > > Redundant information is always possible, but it can become > > > inconsistent and, because the hashes can't be derived from each > > > other, it's hard to tell if it is inconsistent without redoing the > > > whole hash with each method. > > > > Part of the problem here is that IMA is effectively used for two > > related but different purposes - measurement and appraisal. You > > generally want measurements to be comparable across filesystems, > > whereas appraisal doesn't need to be. > > Sure, but I think the only requirement for measurement is knowing how > to reproduce them. As long as you know the algorithm the filesystem is > using ... i.e. it's recorded in the IMA log, you should be able to > verify them. Mm. I think this is use-case dependent, but there are certainly use cases where this would be sufficient. I think this would work on the VFS side, but we'd need to extend IMA to allow you to write a policy that specified the use of the fs-verity data on the appropriate filesystems (right now IMA uses one hash type globally) - if anyone's interested in deploying that, I'm happy to add support for it.
Mimi, anything else I can do here?
[Cc'ing Roberto]
Hi Matthew,
On Mon, 2019-04-29 at 15:51 -0700, Matthew Garrett wrote:
> Mimi, anything else I can do here?
Trying to remember where we were ... The last issue, as I recall, is
somehow annotating the measurement list to indicate the source of the
file hash.
One solution might be:
Suppose instead of re-using the "d-ng" for the vfs hash, you defined a
new field named d-vfs. Instead of the "ima-ng" or "d-ng|n-ng", the
template name could be "d-vfs|n-ng".
Intermixing of template formats is not a problem. IMA already
supports multiple templates in the same list for carrying the
measurement list across kexec. (There are no guarantees that the
current measurement list and the kexec'ed kernel will be the same
template format.) The template format is currently defined at compile
time, with a run time option of changing it.
The issue then becomes how to dynamically switch between template formats, based on fields.
Mimi
On Thu, May 2, 2019 at 1:25 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > Suppose instead of re-using the "d-ng" for the vfs hash, you defined a > new field named d-vfs. Instead of the "ima-ng" or "d-ng|n-ng", the > template name could be "d-vfs|n-ng". Is it legitimate to redefine d-ng such that if the hash comes from the filesystem it adds an additional prefix? This will only occur if the admin has explicitly enabled the trusted_vfs option, so we wouldn't break any existing configurations. Otherwise, I'll look for the cleanest approach for making this dynamic.
On Thu, 2019-05-02 at 15:37 -0700, Matthew Garrett wrote: > On Thu, May 2, 2019 at 1:25 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > Suppose instead of re-using the "d-ng" for the vfs hash, you defined a > > new field named d-vfs. Instead of the "ima-ng" or "d-ng|n-ng", the > > template name could be "d-vfs|n-ng". > > Is it legitimate to redefine d-ng such that if the hash comes from the > filesystem it adds an additional prefix? This will only occur if the > admin has explicitly enabled the trusted_vfs option, so we wouldn't > break any existing configurations. Otherwise, I'll look for the > cleanest approach for making this dynamic. I would assume modifying d-ng would break existing attestation servers. Perhaps instead of making the template format dynamic based on fields, as I suggested above, define a per policy rule template format option. Mimi
On 5/3/2019 1:02 AM, Mimi Zohar wrote: > On Thu, 2019-05-02 at 15:37 -0700, Matthew Garrett wrote: >> On Thu, May 2, 2019 at 1:25 PM Mimi Zohar <zohar@linux.ibm.com> wrote: >>> Suppose instead of re-using the "d-ng" for the vfs hash, you defined a >>> new field named d-vfs. Instead of the "ima-ng" or "d-ng|n-ng", the >>> template name could be "d-vfs|n-ng". >> >> Is it legitimate to redefine d-ng such that if the hash comes from the >> filesystem it adds an additional prefix? This will only occur if the >> admin has explicitly enabled the trusted_vfs option, so we wouldn't >> break any existing configurations. Otherwise, I'll look for the >> cleanest approach for making this dynamic. > > I would assume modifying d-ng would break existing attestation > servers. Yes, I would also prefer to avoid modification of d-ng. > Perhaps instead of making the template format dynamic based on fields, > as I suggested above, define a per policy rule template format option. This should not be too complicated. The template to use will be returned by ima_get_action() to process_measurement(). Roberto
On 5/3/2019 8:51 AM, Roberto Sassu wrote: > On 5/3/2019 1:02 AM, Mimi Zohar wrote: >> On Thu, 2019-05-02 at 15:37 -0700, Matthew Garrett wrote: >>> On Thu, May 2, 2019 at 1:25 PM Mimi Zohar <zohar@linux.ibm.com> wrote: >>>> Suppose instead of re-using the "d-ng" for the vfs hash, you defined a >>>> new field named d-vfs. Instead of the "ima-ng" or "d-ng|n-ng", the >>>> template name could be "d-vfs|n-ng". >>> >>> Is it legitimate to redefine d-ng such that if the hash comes from the >>> filesystem it adds an additional prefix? This will only occur if the >>> admin has explicitly enabled the trusted_vfs option, so we wouldn't >>> break any existing configurations. Otherwise, I'll look for the >>> cleanest approach for making this dynamic. >> >> I would assume modifying d-ng would break existing attestation >> servers. > > Yes, I would also prefer to avoid modification of d-ng. > > >> Perhaps instead of making the template format dynamic based on fields, >> as I suggested above, define a per policy rule template format option. > > This should not be too complicated. The template to use will be returned > by ima_get_action() to process_measurement(). Some time ago I made some patches: https://sourceforge.net/p/linux-ima/mailman/message/31655784/ Roberto
On Fri, 2019-05-03 at 10:17 +0200, Roberto Sassu wrote: > On 5/3/2019 8:51 AM, Roberto Sassu wrote: > > On 5/3/2019 1:02 AM, Mimi Zohar wrote: > >> Perhaps instead of making the template format dynamic based on fields, > >> as I suggested above, define a per policy rule template format option. > > > > This should not be too complicated. The template to use will be returned > > by ima_get_action() to process_measurement(). > > Some time ago I made some patches: > > https://sourceforge.net/p/linux-ima/mailman/message/31655784/ Thank you for the reference! In addition to Matthew's VFS hash use case, Thiago's appended signature support would benefit from a per policy rule template. Do you want, and have time, to add this support? Mimi
On 5/3/2019 2:47 PM, Mimi Zohar wrote: > On Fri, 2019-05-03 at 10:17 +0200, Roberto Sassu wrote: >> On 5/3/2019 8:51 AM, Roberto Sassu wrote: >>> On 5/3/2019 1:02 AM, Mimi Zohar wrote: > >>>> Perhaps instead of making the template format dynamic based on fields, >>>> as I suggested above, define a per policy rule template format option. >>> >>> This should not be too complicated. The template to use will be returned >>> by ima_get_action() to process_measurement(). >> >> Some time ago I made some patches: >> >> https://sourceforge.net/p/linux-ima/mailman/message/31655784/ > > Thank you for the reference! In addition to Matthew's VFS hash use > case, Thiago's appended signature support would benefit from a per > policy rule template. Do you want, and have time, to add this > support? No problem. At the moment I'm busy with the digest lists patch set. I will see if I have time later. Roberto
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy index 09a5def7e28a..6a517282068d 100644 --- a/Documentation/ABI/testing/ima_policy +++ b/Documentation/ABI/testing/ima_policy @@ -24,7 +24,8 @@ Description: [euid=] [fowner=] [fsname=] [subtype=]] lsm: [[subj_user=] [subj_role=] [subj_type=] [obj_user=] [obj_role=] [obj_type=]] - option: [[appraise_type=]] [permit_directio] + option: [[appraise_type=] [permit_directio] + [trust_vfs]] base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK] [FIRMWARE_CHECK] @@ -41,6 +42,8 @@ Description: lsm: are LSM specific option: appraise_type:= [imasig] pcr:= decimal value + permit_directio:= allow directio accesses + trust_vfs:= trust VFS-provided hash values default policy: # PROC_SUPER_MAGIC diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 858b6c0b9a15..d3054a67e700 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -1640,6 +1640,11 @@ different crypto accelerators. This option can be used to achieve best performance for particular HW. + ima.force_hash= [IMA] Force hash calculation in IMA + Format: <bool> + Always calculate hashes rather than trusting the + filesystem to provide them to us. + init= [KNL] Format: <full_path> Run specified binary instead of /sbin/init as init diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index cc12f3449a72..24d9b3a3bfc0 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -133,7 +133,8 @@ int ima_fs_init(void); int ima_add_template_entry(struct ima_template_entry *entry, int violation, const char *op, struct inode *inode, const unsigned char *filename); -int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash); +int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash, + bool trust_vfs); int ima_calc_buffer_hash(const void *buf, loff_t len, struct ima_digest_data *hash); int ima_calc_field_array_hash(struct ima_field_data *field_data, diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index c7505fb122d4..0def9cf43549 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -206,6 +206,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, int length; void *tmpbuf; u64 i_version; + bool trust_vfs; struct { struct ima_digest_data hdr; char digest[IMA_MAX_DIGEST_SIZE]; @@ -225,10 +226,12 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, /* Initialize hash digest to 0's in case of failure */ memset(&hash.digest, 0, sizeof(hash.digest)); + trust_vfs = iint->flags & IMA_TRUST_VFS; + if (buf) result = ima_calc_buffer_hash(buf, size, &hash.hdr); else - result = ima_calc_file_hash(file, &hash.hdr); + result = ima_calc_file_hash(file, &hash.hdr, trust_vfs); if (result && result != -EBADF && result != -EINVAL) goto out; diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c index acf2c7df7145..94105089ad41 100644 --- a/security/integrity/ima/ima_crypto.c +++ b/security/integrity/ima/ima_crypto.c @@ -32,6 +32,10 @@ static unsigned long ima_ahash_minsize; module_param_named(ahash_minsize, ima_ahash_minsize, ulong, 0644); MODULE_PARM_DESC(ahash_minsize, "Minimum file size for ahash use"); +static bool ima_force_hash; +module_param_named(force_hash, ima_force_hash, bool_enable_only, 0644); +MODULE_PARM_DESC(force_hash, "Always calculate hashes"); + /* default is 0 - 1 page. */ static int ima_maxorder; static unsigned int ima_bufsize = PAGE_SIZE; @@ -402,11 +406,14 @@ static int ima_calc_file_shash(struct file *file, struct ima_digest_data *hash) * shash for the hash calculation. If ahash fails, it falls back to using * shash. */ -int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) +int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash, + bool trust_vfs) { loff_t i_size; int rc; struct file *f = file; + struct dentry *dentry = file_dentry(file); + struct inode *inode = d_backing_inode(dentry); bool new_file_instance = false, modified_flags = false; /* @@ -419,6 +426,20 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) return -EINVAL; } + /* + * If policy says we should trust VFS-provided hashes, and + * we're not configured to force hashing, and if the + * filesystem is trusted, ask the VFS if it can obtain the + * hash without us having to calculate it ourself. + */ + if (trust_vfs && !ima_force_hash && + !(inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER)) { + hash->length = hash_digest_size[hash->algo]; + rc = vfs_get_hash(file, hash->algo, hash->digest, hash->length); + if (!rc) + return 0; + } + /* Open a new file instance in O_RDONLY if we cannot read */ if (!(file->f_mode & FMODE_READ)) { int flags = file->f_flags & ~(O_WRONLY | O_APPEND | diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index dcecb6aae5ec..af632c31f856 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -683,7 +683,7 @@ enum { Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt, Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt, Opt_appraise_type, Opt_permit_directio, - Opt_pcr, Opt_err + Opt_pcr, Opt_trust_vfs, Opt_err }; static const match_table_t policy_tokens = { @@ -718,6 +718,7 @@ static const match_table_t policy_tokens = { {Opt_appraise_type, "appraise_type=%s"}, {Opt_permit_directio, "permit_directio"}, {Opt_pcr, "pcr=%s"}, + {Opt_trust_vfs, "trust_vfs"}, {Opt_err, NULL} }; @@ -1060,6 +1061,9 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) case Opt_permit_directio: entry->flags |= IMA_PERMIT_DIRECTIO; break; + case Opt_trust_vfs: + entry->flags |= IMA_TRUST_VFS; + break; case Opt_pcr: if (entry->action != MEASURE) { result = -EINVAL; @@ -1356,6 +1360,8 @@ int ima_policy_show(struct seq_file *m, void *v) seq_puts(m, "appraise_type=imasig "); if (entry->flags & IMA_PERMIT_DIRECTIO) seq_puts(m, "permit_directio "); + if (entry->flags & IMA_TRUST_VFS) + seq_puts(m, "trust_vfs "); rcu_read_unlock(); seq_puts(m, "\n"); return 0; diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c index 43752002c222..a36cdc6651b7 100644 --- a/security/integrity/ima/ima_template_lib.c +++ b/security/integrity/ima/ima_template_lib.c @@ -290,7 +290,7 @@ int ima_eventdigest_init(struct ima_event_data *event_data, inode = file_inode(event_data->file); hash.hdr.algo = ima_template_hash_algo_allowed(ima_hash_algo) ? ima_hash_algo : HASH_ALGO_SHA1; - result = ima_calc_file_hash(event_data->file, &hash.hdr); + result = ima_calc_file_hash(event_data->file, &hash.hdr, false); if (result) { integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, event_data->filename, "collect_data", diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 7de59f44cba3..a03f859c1602 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -36,6 +36,7 @@ #define IMA_NEW_FILE 0x04000000 #define EVM_IMMUTABLE_DIGSIG 0x08000000 #define IMA_FAIL_UNVERIFIABLE_SIGS 0x10000000 +#define IMA_TRUST_VFS 0x20000000 #define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \ IMA_HASH | IMA_APPRAISE_SUBMASK)