| Message ID | 20230913073755.3489676-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: fix wrong dereferences of file->f_path | expand |
On Wed, Sep 13, 2023 at 10:38 AM Amir Goldstein <amir73il@gmail.com> wrote: > > When storing IMA xattr on an overlayfs inode, the xattr is actually > stored in the inode of the underlying (a.k.a real) filesystem, so there > is an ambiguity whether this IMA xattr describes the integrity of the > overlayfs inode or the real inode. > > For this reason and other reasons, IMA is not supported on overlayfs, > in the sense that integrity checking on the overlayfs inode/file/path > do not work correctly and have undefined behavior and the IMA xattr > always describes the integrity of the real inode. > > When a user operates on an overlayfs file, whose underlying real file > has IMA enabled, IMA should always operate on the real path and not > on the overlayfs path. > > IMA code already uses the helper file_dentry() to get the dentry > of the real file. Dereferencing file->f_path directly means that IMA > will operate on the overlayfs inode, which is wrong. > > Therefore, all dereferences to f_path were converted to use the > file_real_path() helper. > > Reported-by: syzbot+a67fc5321ffb4b311c98@syzkaller.appspotmail.com > Closes: https://lore.kernel.org/linux-unionfs/0000000000005bd097060530b758@google.com/ > Fixes: db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version") > Cc: Christian Brauner <brauner@kernel.org> > Cc: Jeff Layton <jlayton@kernel.org> > Cc: Goldwyn Rodrigues <rgoldwyn@suse.com> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > > Mimi, > > Some of the wrong f_path dereferences are much older than the Fixes > commit, but they did not have as big an impact as the wrong f_path > dereference that the Fixes commit introduced. > > For example, commit a408e4a86b36 ("ima: open a new file instance if no > read permissions") worked because reading the content of the overlayfs > file has the same result as reading the content of the real file, but it > is actually the real file integrity that we want to verify. > > Anyway, the real path information, that is now available via the > file_real_path() helper, was not available in IMA integrity check context > at the time that commit a408e4a86b36 was merged. > Only problem is that fix did not resolve the syzbot bug, which seems to do the IMA integrity check on overlayfs file (not sure). I am pretty sure that this patch fixes "a bug" when IMA is on the filesystem under overlayfs and this is a pretty important use case. But I guess there are still issues with IMA over overlayfs and this is not the only one. Is this really a use case that needs to be supported? Isn't the newly added SB_I_IMA_UNVERIFIABLE_SIGNATURE flag a hint that IMA on overlayfs is not a good idea at all? Thanks, Amir. > > security/integrity/ima/ima_api.c | 4 ++-- > security/integrity/ima/ima_crypto.c | 2 +- > security/integrity/ima/ima_main.c | 10 +++++----- > 3 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c > index 452e80b541e5..badf3784a1a0 100644 > --- a/security/integrity/ima/ima_api.c > +++ b/security/integrity/ima/ima_api.c > @@ -268,8 +268,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, > * to an initial measurement/appraisal/audit, but was modified to > * assume the file changed. > */ > - result = vfs_getattr_nosec(&file->f_path, &stat, STATX_CHANGE_COOKIE, > - AT_STATX_SYNC_AS_STAT); > + result = vfs_getattr_nosec(file_real_path(file), &stat, > + STATX_CHANGE_COOKIE, AT_STATX_SYNC_AS_STAT); > if (!result && (stat.result_mask & STATX_CHANGE_COOKIE)) > i_version = stat.change_cookie; > hash.hdr.algo = algo; > diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c > index 51ad29940f05..e6c52f3c8f37 100644 > --- a/security/integrity/ima/ima_crypto.c > +++ b/security/integrity/ima/ima_crypto.c > @@ -555,7 +555,7 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) > int flags = file->f_flags & ~(O_WRONLY | O_APPEND | > O_TRUNC | O_CREAT | O_NOCTTY | O_EXCL); > flags |= O_RDONLY; > - f = dentry_open(&file->f_path, flags, file->f_cred); > + f = dentry_open(file_real_path(file), flags, file->f_cred); > if (IS_ERR(f)) > return PTR_ERR(f); > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 365db0e43d7c..87c13effbdf4 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -94,7 +94,7 @@ static int mmap_violation_check(enum ima_hooks func, struct file *file, > inode = file_inode(file); > > if (!*pathbuf) /* ima_rdwr_violation possibly pre-fetched */ > - *pathname = ima_d_path(&file->f_path, pathbuf, > + *pathname = ima_d_path(file_real_path(file), pathbuf, > filename); > integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, *pathname, > "mmap_file", "mmapped_writers", rc, 0); > @@ -142,7 +142,7 @@ static void ima_rdwr_violation_check(struct file *file, > if (!send_tomtou && !send_writers) > return; > > - *pathname = ima_d_path(&file->f_path, pathbuf, filename); > + *pathname = ima_d_path(file_real_path(file), pathbuf, filename); > > if (send_tomtou) > ima_add_violation(file, *pathname, iint, > @@ -168,7 +168,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint, > update = test_and_clear_bit(IMA_UPDATE_XATTR, > &iint->atomic_flags); > if ((iint->flags & IMA_NEW_FILE) || > - vfs_getattr_nosec(&file->f_path, &stat, > + vfs_getattr_nosec(file_real_path(file), &stat, > STATX_CHANGE_COOKIE, > AT_STATX_SYNC_AS_STAT) || > !(stat.result_mask & STATX_CHANGE_COOKIE) || > @@ -347,7 +347,7 @@ static int process_measurement(struct file *file, const struct cred *cred, > goto out_locked; > > if (!pathbuf) /* ima_rdwr_violation possibly pre-fetched */ > - pathname = ima_d_path(&file->f_path, &pathbuf, filename); > + pathname = ima_d_path(file_real_path(file), &pathbuf, filename); > > if (action & IMA_MEASURE) > ima_store_measurement(iint, file, pathname, > @@ -487,7 +487,7 @@ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot) > result = -EPERM; > > file = vma->vm_file; > - pathname = ima_d_path(&file->f_path, &pathbuf, filename); > + pathname = ima_d_path(file_real_path(file), &pathbuf, filename); > integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, pathname, > "collect_data", "failed-mprotect", result, 0); > if (pathbuf) > -- > 2.34.1 >
On Wed, 2023-09-13 at 15:09 +0300, Amir Goldstein wrote: > On Wed, Sep 13, 2023 at 10:38 AM Amir Goldstein <amir73il@gmail.com> wrote: > > > > When storing IMA xattr on an overlayfs inode, the xattr is actually > > stored in the inode of the underlying (a.k.a real) filesystem, so there > > is an ambiguity whether this IMA xattr describes the integrity of the > > overlayfs inode or the real inode. > > > > For this reason and other reasons, IMA is not supported on overlayfs, > > in the sense that integrity checking on the overlayfs inode/file/path > > do not work correctly and have undefined behavior and the IMA xattr > > always describes the integrity of the real inode. > > > > When a user operates on an overlayfs file, whose underlying real file > > has IMA enabled, IMA should always operate on the real path and not > > on the overlayfs path. > > > > IMA code already uses the helper file_dentry() to get the dentry > > of the real file. Dereferencing file->f_path directly means that IMA > > will operate on the overlayfs inode, which is wrong. > > > > Therefore, all dereferences to f_path were converted to use the > > file_real_path() helper. Thanks, Amir. This sounds right. > > > > Reported-by: syzbot+a67fc5321ffb4b311c98@syzkaller.appspotmail.com > > Closes: https://lore.kernel.org/linux-unionfs/0000000000005bd097060530b758@google.com/ > > Fixes: db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version") > > Cc: Christian Brauner <brauner@kernel.org> > > Cc: Jeff Layton <jlayton@kernel.org> > > Cc: Goldwyn Rodrigues <rgoldwyn@suse.com> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > > > Mimi, > > > > Some of the wrong f_path dereferences are much older than the Fixes > > commit, but they did not have as big an impact as the wrong f_path > > dereference that the Fixes commit introduced. > > > > For example, commit a408e4a86b36 ("ima: open a new file instance if no > > read permissions") worked because reading the content of the overlayfs > > file has the same result as reading the content of the real file, but it > > is actually the real file integrity that we want to verify. > > > > Anyway, the real path information, that is now available via the > > file_real_path() helper, was not available in IMA integrity check context > > at the time that commit a408e4a86b36 was merged. > > Only problem is that fix did not resolve the syzbot bug, which > seems to do the IMA integrity check on overlayfs file (not sure). > > I am pretty sure that this patch fixes "a bug" when IMA is on the filesystem > under overlayfs and this is a pretty important use case. Agreed. > But I guess there are still issues with IMA over overlayfs and this is not > the only one. Sigh > Is this really a use case that needs to be supported? > Isn't the newly added SB_I_IMA_UNVERIFIABLE_SIGNATUREh flag > a hint that IMA on overlayfs is not a good idea at all? With SB_I_IMA_UNVERIFIABLE_SIGNATURE enabled for overlayfs, signature verification will then fail immediately for all overlayfs files in policy. I don't think that's the right solution. Verification should be limited to when the overlayfs file is the same as the underlying backing store, the real inode, not the overlay upper files.
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index 452e80b541e5..badf3784a1a0 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -268,8 +268,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, * to an initial measurement/appraisal/audit, but was modified to * assume the file changed. */ - result = vfs_getattr_nosec(&file->f_path, &stat, STATX_CHANGE_COOKIE, - AT_STATX_SYNC_AS_STAT); + result = vfs_getattr_nosec(file_real_path(file), &stat, + STATX_CHANGE_COOKIE, AT_STATX_SYNC_AS_STAT); if (!result && (stat.result_mask & STATX_CHANGE_COOKIE)) i_version = stat.change_cookie; hash.hdr.algo = algo; diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c index 51ad29940f05..e6c52f3c8f37 100644 --- a/security/integrity/ima/ima_crypto.c +++ b/security/integrity/ima/ima_crypto.c @@ -555,7 +555,7 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) int flags = file->f_flags & ~(O_WRONLY | O_APPEND | O_TRUNC | O_CREAT | O_NOCTTY | O_EXCL); flags |= O_RDONLY; - f = dentry_open(&file->f_path, flags, file->f_cred); + f = dentry_open(file_real_path(file), flags, file->f_cred); if (IS_ERR(f)) return PTR_ERR(f); diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 365db0e43d7c..87c13effbdf4 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -94,7 +94,7 @@ static int mmap_violation_check(enum ima_hooks func, struct file *file, inode = file_inode(file); if (!*pathbuf) /* ima_rdwr_violation possibly pre-fetched */ - *pathname = ima_d_path(&file->f_path, pathbuf, + *pathname = ima_d_path(file_real_path(file), pathbuf, filename); integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, *pathname, "mmap_file", "mmapped_writers", rc, 0); @@ -142,7 +142,7 @@ static void ima_rdwr_violation_check(struct file *file, if (!send_tomtou && !send_writers) return; - *pathname = ima_d_path(&file->f_path, pathbuf, filename); + *pathname = ima_d_path(file_real_path(file), pathbuf, filename); if (send_tomtou) ima_add_violation(file, *pathname, iint, @@ -168,7 +168,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint, update = test_and_clear_bit(IMA_UPDATE_XATTR, &iint->atomic_flags); if ((iint->flags & IMA_NEW_FILE) || - vfs_getattr_nosec(&file->f_path, &stat, + vfs_getattr_nosec(file_real_path(file), &stat, STATX_CHANGE_COOKIE, AT_STATX_SYNC_AS_STAT) || !(stat.result_mask & STATX_CHANGE_COOKIE) || @@ -347,7 +347,7 @@ static int process_measurement(struct file *file, const struct cred *cred, goto out_locked; if (!pathbuf) /* ima_rdwr_violation possibly pre-fetched */ - pathname = ima_d_path(&file->f_path, &pathbuf, filename); + pathname = ima_d_path(file_real_path(file), &pathbuf, filename); if (action & IMA_MEASURE) ima_store_measurement(iint, file, pathname, @@ -487,7 +487,7 @@ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot) result = -EPERM; file = vma->vm_file; - pathname = ima_d_path(&file->f_path, &pathbuf, filename); + pathname = ima_d_path(file_real_path(file), &pathbuf, filename); integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, pathname, "collect_data", "failed-mprotect", result, 0); if (pathbuf)
When storing IMA xattr on an overlayfs inode, the xattr is actually stored in the inode of the underlying (a.k.a real) filesystem, so there is an ambiguity whether this IMA xattr describes the integrity of the overlayfs inode or the real inode. For this reason and other reasons, IMA is not supported on overlayfs, in the sense that integrity checking on the overlayfs inode/file/path do not work correctly and have undefined behavior and the IMA xattr always describes the integrity of the real inode. When a user operates on an overlayfs file, whose underlying real file has IMA enabled, IMA should always operate on the real path and not on the overlayfs path. IMA code already uses the helper file_dentry() to get the dentry of the real file. Dereferencing file->f_path directly means that IMA will operate on the overlayfs inode, which is wrong. Therefore, all dereferences to f_path were converted to use the file_real_path() helper. Reported-by: syzbot+a67fc5321ffb4b311c98@syzkaller.appspotmail.com Closes: https://lore.kernel.org/linux-unionfs/0000000000005bd097060530b758@google.com/ Fixes: db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version") Cc: Christian Brauner <brauner@kernel.org> Cc: Jeff Layton <jlayton@kernel.org> Cc: Goldwyn Rodrigues <rgoldwyn@suse.com> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- Mimi, Some of the wrong f_path dereferences are much older than the Fixes commit, but they did not have as big an impact as the wrong f_path dereference that the Fixes commit introduced. For example, commit a408e4a86b36 ("ima: open a new file instance if no read permissions") worked because reading the content of the overlayfs file has the same result as reading the content of the real file, but it is actually the real file integrity that we want to verify. Anyway, the real path information, that is now available via the file_real_path() helper, was not available in IMA itegrity check context at the time that commit a408e4a86b36 was merged. Thanks, Amir. security/integrity/ima/ima_api.c | 4 ++-- security/integrity/ima/ima_crypto.c | 2 +- security/integrity/ima/ima_main.c | 10 +++++----- 3 files changed, 8 insertions(+), 8 deletions(-)