| Message ID | 20181004203007.217320-3-mjg59@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] VFS: Add a call to obtain a file's hash | expand |
On Thu, 2018-10-04 at 13:30 -0700, Matthew Garrett wrote: > 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. Make use of this by default, but provide a parameter > to force recalculation rather than trusting the filesystem. > > Signed-off-by: Matthew Garrett <mjg59@google.com> Support for not calculating the file hash would need to be finer grained than this, probably on a per mount basis. The default should be for IMA to always calculate the file hash, unless explicitly told not to. > --- > Documentation/admin-guide/kernel-parameters.txt | 5 +++++ > security/integrity/ima/ima_crypto.c | 11 +++++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index 92eb1f42240d..617ae0f83b14 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -1612,6 +1612,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_crypto.c b/security/integrity/ima/ima_crypto.c > index 7e7e7e7c250a..f25259b2b6ec 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; > @@ -431,6 +435,13 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) > return -EINVAL; > } > > + if (!ima_force_hash) { IMA should never skip the file hash calculation if the filesystem is an untrusted mount (eg. SB_I_UNTRUSTED_MOUNTER). Mimi > + hash->length = hash_digest_size[hash->algo]; > + rc = vfs_get_hash(file, hash->algo, hash->digest, hash->length); > + if (!rc) > + return 0; > + } > + > i_size = i_size_read(file_inode(file)); > > if (ima_ahash_minsize && i_size >= ima_ahash_minsize) {
On Thu, Oct 11, 2018 at 8:23 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Thu, 2018-10-04 at 13:30 -0700, Matthew Garrett wrote: > > 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. Make use of this by default, but provide a parameter > > to force recalculation rather than trusting the filesystem. > > > > Signed-off-by: Matthew Garrett <mjg59@google.com> > > Support for not calculating the file hash would need to be finer > grained than this, probably on a per mount basis. The default should > be for IMA to always calculate the file hash, unless explicitly told > not to. Ok, should this just be part of the IMA policy? > IMA should never skip the file hash calculation if the filesystem is > an untrusted mount (eg. SB_I_UNTRUSTED_MOUNTER). Ok.
On Thu, 2018-10-11 at 13:30 -0700, Matthew Garrett wrote: > On Thu, Oct 11, 2018 at 8:23 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > On Thu, 2018-10-04 at 13:30 -0700, Matthew Garrett wrote: > > > 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. Make use of this by default, but provide a parameter > > > to force recalculation rather than trusting the filesystem. > > > > > > Signed-off-by: Matthew Garrett <mjg59@google.com> > > > > Support for not calculating the file hash would need to be finer > > grained than this, probably on a per mount basis. The default should > > be for IMA to always calculate the file hash, unless explicitly told > > not to. > > Ok, should this just be part of the IMA policy? How would you be able to differentiate between different FUSE filesystems for example? > > > IMA should never skip the file hash calculation if the filesystem is > > an untrusted mount (eg. SB_I_UNTRUSTED_MOUNTER). > > Ok. >
On Thu, Oct 11, 2018 at 4:03 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > On Thu, 2018-10-11 at 13:30 -0700, Matthew Garrett wrote: > > Ok, should this just be part of the IMA policy? > > How would you be able to differentiate between different FUSE > filesystems for example? There's a couple of ways. We could extend the filesystem type matching logic to also check the subtype - you'd then need to enforce that at the LSM level in order to protect against untrusted filesystems spoofing the filesystem type. Alternatively, we could add an additional policy match type for mount point and iterate through s_mounts on the superblock - if any match, we could define the policy there?
On Fri, 2018-10-12 at 11:31 -0700, Matthew Garrett wrote: > On Thu, Oct 11, 2018 at 4:03 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Thu, 2018-10-11 at 13:30 -0700, Matthew Garrett wrote: > > > > Ok, should this just be part of the IMA policy? > > > > How would you be able to differentiate between different FUSE > > filesystems for example? > > There's a couple of ways. We could extend the filesystem type matching > logic to also check the subtype - you'd then need to enforce that at > the LSM level in order to protect against untrusted filesystems > spoofing the filesystem type. Alternatively, we could add an > additional policy match type for mount point and iterate through > s_mounts on the superblock - if any match, we could define the policy > there? The first method differentiates between different subtypes of FUSE filesystems, while the second method allows differentiating between the same type and subtype on different mount points. Both criteria are needed, but instead of the second method based on a mount point, perhaps based instead on a mount flag? Trusted mount of permitted filesystem type and subtype, that is mounted with the defined mount flag. Mimi
On Sun, Oct 14, 2018 at 6:38 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > On Fri, 2018-10-12 at 11:31 -0700, Matthew Garrett wrote: > > There's a couple of ways. We could extend the filesystem type matching > > logic to also check the subtype - you'd then need to enforce that at > > the LSM level in order to protect against untrusted filesystems > > spoofing the filesystem type. Alternatively, we could add an > > additional policy match type for mount point and iterate through > > s_mounts on the superblock - if any match, we could define the policy > > there? > > The first method differentiates between different subtypes of FUSE > filesystems, while the second method allows differentiating between > the same type and subtype on different mount points. Both criteria > are needed, but instead of the second method based on a mount point, > perhaps based instead on a mount flag? Patch 3 already requires that the allow_gethash option be passed for this to work - I can restrict that to CAP_SYS_ADMIN? > Trusted mount of permitted filesystem type and subtype, that is > mounted with the defined mount flag. Ok, I'll write up a patch that allows policy matching of filesystem subtype as well as type and try to get that posted this week so we can discuss it in Edinburgh?
On Mon, 2018-10-15 at 11:46 -0700, Matthew Garrett wrote: > On Sun, Oct 14, 2018 at 6:38 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Fri, 2018-10-12 at 11:31 -0700, Matthew Garrett wrote: > > > There's a couple of ways. We could extend the filesystem type matching > > > logic to also check the subtype - you'd then need to enforce that at > > > the LSM level in order to protect against untrusted filesystems > > > spoofing the filesystem type. Alternatively, we could add an > > > additional policy match type for mount point and iterate through > > > s_mounts on the superblock - if any match, we could define the policy > > > there? > > > > The first method differentiates between different subtypes of FUSE > > filesystems, while the second method allows differentiating between > > the same type and subtype on different mount points. Both criteria > > are needed, but instead of the second method based on a mount point, > > perhaps based instead on a mount flag? > > Patch 3 already requires that the allow_gethash option be passed for > this to work - I can restrict that to CAP_SYS_ADMIN? In the case of FUSE filesystems, using "gethash" should be limited to trusted mounts, not fileystems mounted with SB_I_UNTRUSTED_MOUNTER. So requiring CAP_SYS_ADMIN seems unnecessary. The difference in the approaches is that root has CAP_SYS_ADMIN, while providing a mount flag requires intention. > > > Trusted mount of permitted filesystem type and subtype, that is > > mounted with the defined mount flag. > > Ok, I'll write up a patch that allows policy matching of filesystem > subtype as well as type and try to get that posted this week so we can > discuss it in Edinburgh? Sounds good. Hopefully I'll have time to review it before Edinburgh. Mimi
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 92eb1f42240d..617ae0f83b14 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -1612,6 +1612,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_crypto.c b/security/integrity/ima/ima_crypto.c index 7e7e7e7c250a..f25259b2b6ec 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; @@ -431,6 +435,13 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) return -EINVAL; } + if (!ima_force_hash) { + hash->length = hash_digest_size[hash->algo]; + rc = vfs_get_hash(file, hash->algo, hash->digest, hash->length); + if (!rc) + return 0; + } + i_size = i_size_read(file_inode(file)); if (ima_ahash_minsize && i_size >= ima_ahash_minsize) {
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. Make use of this by default, but provide a parameter to force recalculation rather than trusting the filesystem. Signed-off-by: Matthew Garrett <mjg59@google.com> --- Documentation/admin-guide/kernel-parameters.txt | 5 +++++ security/integrity/ima/ima_crypto.c | 11 +++++++++++ 2 files changed, 16 insertions(+)