| Message ID | 20231024120845.942815-1-zohar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: detect changes to the backing overlay file | expand |
On Tue, Oct 24, 2023 at 3:09 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > Commit 18b44bc5a672 ("ovl: Always reevaluate the file signature for > IMA") forced signature re-evaulation on every file access. > > IMA does not detect changes made to the backing overlay file. Instead > of always re-evaluating the file's integrity, detect a change to the > backing overlay file, by comparing the i_version, as stored in the iint, > with the backing file's i_version. > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > --- > fs/overlayfs/super.c | 2 +- > security/integrity/ima/ima_api.c | 1 + > security/integrity/ima/ima_main.c | 21 ++++++++++++++++++++- > 3 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > index def266b5e2a3..4d9137ba2293 100644 > --- a/fs/overlayfs/super.c > +++ b/fs/overlayfs/super.c > @@ -1482,7 +1482,7 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc) > ovl_trusted_xattr_handlers; > sb->s_fs_info = ofs; > sb->s_flags |= SB_POSIXACL; > - sb->s_iflags |= SB_I_SKIP_SYNC | SB_I_IMA_UNVERIFIABLE_SIGNATURE; > + sb->s_iflags |= SB_I_SKIP_SYNC; > > err = -ENOMEM; > root_dentry = ovl_get_root(sb, ctx->upper.dentry, oe); > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c > index 452e80b541e5..d1c718ef9295 100644 > --- a/security/integrity/ima/ima_api.c > +++ b/security/integrity/ima/ima_api.c > @@ -272,6 +272,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, > AT_STATX_SYNC_AS_STAT); > if (!result && (stat.result_mask & STATX_CHANGE_COOKIE)) > i_version = stat.change_cookie; > + > hash.hdr.algo = algo; > hash.hdr.length = hash_digest_size[algo]; > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 365db0e43d7c..7c8aac81d16e 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -25,6 +25,7 @@ > #include <linux/xattr.h> > #include <linux/ima.h> > #include <linux/fs.h> > +#include <linux/iversion.h> > > #include "ima.h" > > @@ -207,7 +208,7 @@ static int process_measurement(struct file *file, const struct cred *cred, > u32 secid, char *buf, loff_t size, int mask, > enum ima_hooks func) > { > - struct inode *inode = file_inode(file); > + struct inode *backing_inode, *inode = file_inode(file); > struct integrity_iint_cache *iint = NULL; > struct ima_template_desc *template_desc = NULL; > char *pathbuf = NULL; > @@ -284,6 +285,24 @@ static int process_measurement(struct file *file, const struct cred *cred, > iint->measured_pcrs = 0; > } > > + /* > + * IMA does not detect changes made to the backing overlay file. > + * If the backing file's i_version is greater than the overlay > + * file's i_version or the backing file doesn't support iversion, > + * clear the cache to force the file's integrity to be re-evaluated. > + */ > + if (inode->i_sb->s_magic == 0x794c7630 && Please don't use magic to detect overlayfs. Overlayfs is not the only fs that can have a backing file and this test is not needed. > + (action & IMA_DO_MASK) && (iint->flags & IMA_DONE_MASK)) { > + backing_inode = d_real_inode(file_dentry(file)); > + if (backing_inode != inode) { This test is sufficient, because for regular files of filesystems without ->d_real(), backing_inode will always be == inode. > + if (!IS_I_VERSION(backing_inode) || > + inode_peek_iversion(backing_inode) > iint->version) { IIUC, this checking > iint->version is not enough. The reason is that you could have recorded iint->version from i_version of a lower file, after open for write, lower file gets copied up to upper file, which is a completely different backing_inode, so there is no guarantee that the copy up will not result in a smaller or even equal i_version. For a complete solution, you will need to store the backing_inode pointer or backing dev/ino in iint to make sure that backing_inode did not change. Thanks, Amir.
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index def266b5e2a3..4d9137ba2293 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -1482,7 +1482,7 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc) ovl_trusted_xattr_handlers; sb->s_fs_info = ofs; sb->s_flags |= SB_POSIXACL; - sb->s_iflags |= SB_I_SKIP_SYNC | SB_I_IMA_UNVERIFIABLE_SIGNATURE; + sb->s_iflags |= SB_I_SKIP_SYNC; err = -ENOMEM; root_dentry = ovl_get_root(sb, ctx->upper.dentry, oe); diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index 452e80b541e5..d1c718ef9295 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -272,6 +272,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, AT_STATX_SYNC_AS_STAT); if (!result && (stat.result_mask & STATX_CHANGE_COOKIE)) i_version = stat.change_cookie; + hash.hdr.algo = algo; hash.hdr.length = hash_digest_size[algo]; diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 365db0e43d7c..7c8aac81d16e 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -25,6 +25,7 @@ #include <linux/xattr.h> #include <linux/ima.h> #include <linux/fs.h> +#include <linux/iversion.h> #include "ima.h" @@ -207,7 +208,7 @@ static int process_measurement(struct file *file, const struct cred *cred, u32 secid, char *buf, loff_t size, int mask, enum ima_hooks func) { - struct inode *inode = file_inode(file); + struct inode *backing_inode, *inode = file_inode(file); struct integrity_iint_cache *iint = NULL; struct ima_template_desc *template_desc = NULL; char *pathbuf = NULL; @@ -284,6 +285,24 @@ static int process_measurement(struct file *file, const struct cred *cred, iint->measured_pcrs = 0; } + /* + * IMA does not detect changes made to the backing overlay file. + * If the backing file's i_version is greater than the overlay + * file's i_version or the backing file doesn't support iversion, + * clear the cache to force the file's integrity to be re-evaluated. + */ + if (inode->i_sb->s_magic == 0x794c7630 && + (action & IMA_DO_MASK) && (iint->flags & IMA_DONE_MASK)) { + backing_inode = d_real_inode(file_dentry(file)); + if (backing_inode != inode) { + if (!IS_I_VERSION(backing_inode) || + inode_peek_iversion(backing_inode) > iint->version) { + iint->flags &= ~IMA_DONE_MASK; + iint->measured_pcrs = 0; + } + } + } + /* Determine if already appraised/measured based on bitmask * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED, * IMA_AUDIT, IMA_AUDITED)
Commit 18b44bc5a672 ("ovl: Always reevaluate the file signature for IMA") forced signature re-evaulation on every file access. IMA does not detect changes made to the backing overlay file. Instead of always re-evaluating the file's integrity, detect a change to the backing overlay file, by comparing the i_version, as stored in the iint, with the backing file's i_version. Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> --- fs/overlayfs/super.c | 2 +- security/integrity/ima/ima_api.c | 1 + security/integrity/ima/ima_main.c | 21 ++++++++++++++++++++- 3 files changed, 22 insertions(+), 2 deletions(-)