| 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.
[adding back fsdevel] On Fri, Sep 15, 2023 at 7:04 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Fri, 2023-09-15 at 06:21 +0300, Amir Goldstein wrote: > > On Thu, Sep 14, 2023 at 11:01 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > > > Hi Amir, Goldwyn, > > > > > > FYI, > > > 1. ima_file_free() is called to check whether the file is in policy. > > > 2. ima_check_last_writer() is called to determine whether or not the > > > file hash stored as an xattr needs to be updated. > > > > > > 3. As ima_update_xattr() is not being called, I assume there is no > > > appraise rule. I asked on the thread which policy rules are being used > > > and for the boot command line, but assume that they're specifying > > > 'ima_policy=tcb" on the boot command line. > > > > Yes, here is the kconfig from syzbot report dashboard > > https://syzkaller.appspot.com/x/.config?x=df91a3034fe3f122 > > > > > > > > 4. Is commit db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the > > > i_version") the problem? > > > > IIUC, this commit is responsible for the ovl_getattr() call in the stack > > trace. syzbot did not bisect the bug yet, but now that it has found > > a reproducer, it is just a matter of time. > > However, all the bug reports in the dashboard are only from upstream, > > so I think that means that this bug was not found on any stable kernels. > > > > > > > > 5. ima_file_free() is being called twice. We should not be seeing > > > ima_get_current_hash_algo() in the trace. > > > > > > [ 66.991195][ T5030] ovl_getattr+0x1b1/0xf70 > > > [ 66.995635][ T5030] ? ovl_setattr+0x4e0/0x4e0 > > > [ 67.000229][ T5030] ? trace_raw_output_contention_end+0xd0/0xd0 > > > [ 67.006387][ T5030] ? rcu_is_watching+0x15/0xb0 > > > [ 67.011154][ T5030] ? rcu_is_watching+0x15/0xb0 > > > [ 67.015920][ T5030] ? trace_contention_end+0x3c/0xf0 > > > [ 67.021122][ T5030] ? __mutex_lock_common+0x42d/0x2530 > > > [ 67.026506][ T5030] ? lock_release+0xbf/0x9d0 > > > [ 67.031126][ T5030] ? read_lock_is_recursive+0x20/0x20 > > > [ 67.036719][ T5030] ? ima_file_free+0x17c/0x4b0 > > > [ 67.041578][ T5030] ? __lock_acquire+0x7f70/0x7f70 > > > [ 67.046615][ T5030] ? locks_remove_file+0x429/0x1040 > > > [ 67.051820][ T5030] ? mutex_lock_io_nested+0x60/0x60 > > > [ 67.057030][ T5030] ? _raw_spin_unlock+0x40/0x40 > > > [ 67.061894][ T5030] ? __asan_memset+0x23/0x40 > > > [ 67.066577][ T5030] ima_file_free+0x26e/0x4b0 > > > [ 67.071279][ T5030] ? ima_get_current_hash_algo+0x10/0x10 > > > [ 67.076929][ T5030] ? __rwlock_init+0x150/0x150 > > > [ 67.081694][ T5030] ? __lock_acquire+0x7f70/0x7f70 > > > [ 67.086727][ T5030] __fput+0x36a/0x910 > > > [ 67.090728][ T5030] task_work_run+0x24a/0x300 > > > > > > > > > Were you able to duplicate this locally? > > > > > > > I did not try. Honestly, I don't know how to enable IMA. > > Is the only thing that I need to do is set the IMA policy > > in the kernel command line? > > > > Does IMA need to be enabled per fs? per sb? > > > > If so, I can run the overlay test suite with IMA enabled and > > see what happens. > > Yes, you'll definitely will be able to see the measurement list. > > [Setting up the system to verify file signatures is a bit more > difficult: files need to be signed, keys need to be loaded. Finally > CentOS and RHEL 9.3 will have file signatures and will publish the IMA > code signing key.] > > Assuming IMA is configured, just add "ima_policy=tcb" to the command > line. This will measure all files executed, mmap'ed, kernel modules, > firmware, and all files opened by root. Normally the builtin policy is > replaced with a finer grained one. > > Below are a few commands, but Ken Goldman is writing documentation - > https://ima-doc.readthedocs.io/en/latest/ > > 1. Display the IMA measurement list: > # cat /sys/kernel/security/ima/ascii_runtime_measurements > # cat /sys/kernel/security/ima/binary_runtime_measurements > > 2. Display the IMA policy (or append to the policy) > # cat /sys/kernel/security/ima/policy > > 3. Display number of measurements > # cat /sys/kernel/security/ima/runtime_measurements_count > Nice. This seems to work fine and nothing pops up when running fstests unionmount tests of overlayfs over xfs. What strikes me as strange is that there are measurements of files in xfs and in overlayfs, but no measurements of files in tmpfs. I suppose that is because no one can tamper with the storage of tmpfs, but following the same logic, nobody can tamper with the storage of overlayfs files without tampering with storage of underlying fs (e.g. xfs), so measuring overlayfs files should not bring any extra security to the system. Especially, since if files are signed they are signed in the real storage (e.g. xfs) and not in overlayfs. So in theory, we should never ever measure files in the "virtual" overlayfs and only measure them in the real fs. The only problem is the the IMA hooks when executing, mmaping, reading files from overlayfs, don't work on the real fs. fsnotify also was not working correctly in that respect, because fs operations on overlayfs did not always trigger fsnotify events on the underlying real fs. This was fixed in 6.5 by commit bc2473c90fca ("ovl: enable fsnotify events on underlying real files") and the file_real_path() infrastructure was added to enable this. This is why I say, that in most likelihood, IMA hook should always use file_real_path() and file_dentry() to perform the measurements and record the path of the real fs when overlayfs is performing the actual read/mmap on the real fs and IMA hooks should ideally do nothing at all (as in tmpfs) when the operation is performed on the "virtual" overlayfs object. Thanks, Amir.
On Fri, 2023-09-15 at 12:57 +0300, Amir Goldstein wrote: > [adding back fsdevel] > > On Fri, Sep 15, 2023 at 7:04 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > On Fri, 2023-09-15 at 06:21 +0300, Amir Goldstein wrote: > > > On Thu, Sep 14, 2023 at 11:01 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > > > > > Hi Amir, Goldwyn, > > > > > > > > FYI, > > > > 1. ima_file_free() is called to check whether the file is in policy. > > > > 2. ima_check_last_writer() is called to determine whether or not the > > > > file hash stored as an xattr needs to be updated. > > > > > > > > 3. As ima_update_xattr() is not being called, I assume there is no > > > > appraise rule. I asked on the thread which policy rules are being used > > > > and for the boot command line, but assume that they're specifying > > > > 'ima_policy=tcb" on the boot command line. > > > > > > Yes, here is the kconfig from syzbot report dashboard > > > https://syzkaller.appspot.com/x/.config?x=df91a3034fe3f122 > > > > > > > > > > > 4. Is commit db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the > > > > i_version") the problem? > > > > > > IIUC, this commit is responsible for the ovl_getattr() call in the stack > > > trace. syzbot did not bisect the bug yet, but now that it has found > > > a reproducer, it is just a matter of time. > > > However, all the bug reports in the dashboard are only from upstream, > > > so I think that means that this bug was not found on any stable kernels. > > > > > > > > > > > 5. ima_file_free() is being called twice. We should not be seeing > > > > ima_get_current_hash_algo() in the trace. > > > > > > > > [ 66.991195][ T5030] ovl_getattr+0x1b1/0xf70 > > > > [ 66.995635][ T5030] ? ovl_setattr+0x4e0/0x4e0 > > > > [ 67.000229][ T5030] ? trace_raw_output_contention_end+0xd0/0xd0 > > > > [ 67.006387][ T5030] ? rcu_is_watching+0x15/0xb0 > > > > [ 67.011154][ T5030] ? rcu_is_watching+0x15/0xb0 > > > > [ 67.015920][ T5030] ? trace_contention_end+0x3c/0xf0 > > > > [ 67.021122][ T5030] ? __mutex_lock_common+0x42d/0x2530 > > > > [ 67.026506][ T5030] ? lock_release+0xbf/0x9d0 > > > > [ 67.031126][ T5030] ? read_lock_is_recursive+0x20/0x20 > > > > [ 67.036719][ T5030] ? ima_file_free+0x17c/0x4b0 > > > > [ 67.041578][ T5030] ? __lock_acquire+0x7f70/0x7f70 > > > > [ 67.046615][ T5030] ? locks_remove_file+0x429/0x1040 > > > > [ 67.051820][ T5030] ? mutex_lock_io_nested+0x60/0x60 > > > > [ 67.057030][ T5030] ? _raw_spin_unlock+0x40/0x40 > > > > [ 67.061894][ T5030] ? __asan_memset+0x23/0x40 > > > > [ 67.066577][ T5030] ima_file_free+0x26e/0x4b0 > > > > [ 67.071279][ T5030] ? ima_get_current_hash_algo+0x10/0x10 > > > > [ 67.076929][ T5030] ? __rwlock_init+0x150/0x150 > > > > [ 67.081694][ T5030] ? __lock_acquire+0x7f70/0x7f70 > > > > [ 67.086727][ T5030] __fput+0x36a/0x910 > > > > [ 67.090728][ T5030] task_work_run+0x24a/0x300 > > > > > > > > > > > > Were you able to duplicate this locally? > > > > > > > > > > I did not try. Honestly, I don't know how to enable IMA. > > > Is the only thing that I need to do is set the IMA policy > > > in the kernel command line? > > > > > > Does IMA need to be enabled per fs? per sb? > > > > > > If so, I can run the overlay test suite with IMA enabled and > > > see what happens. > > > > Yes, you'll definitely will be able to see the measurement list. > > > > [Setting up the system to verify file signatures is a bit more > > difficult: files need to be signed, keys need to be loaded. Finally > > CentOS and RHEL 9.3 will have file signatures and will publish the IMA > > code signing key.] > > > > Assuming IMA is configured, just add "ima_policy=tcb" to the command > > line. This will measure all files executed, mmap'ed, kernel modules, > > firmware, and all files opened by root. Normally the builtin policy is > > replaced with a finer grained one. > > > > Below are a few commands, but Ken Goldman is writing documentation - > > https://ima-doc.readthedocs.io/en/latest/ > > > > 1. Display the IMA measurement list: > > # cat /sys/kernel/security/ima/ascii_runtime_measurements > > # cat /sys/kernel/security/ima/binary_runtime_measurements > > > > 2. Display the IMA policy (or append to the policy) > > # cat /sys/kernel/security/ima/policy > > > > 3. Display number of measurements > > # cat /sys/kernel/security/ima/runtime_measurements_count > > > > Nice. > This seems to work fine and nothing pops up when running > fstests unionmount tests of overlayfs over xfs. > > What strikes me as strange is that there are measurements > of files in xfs and in overlayfs, but no measurements of files in tmpfs. tmpfs is excluded from policy, since there is no way of storing the file signature in the initramfs (CPIO). There have been a number of attempts of extending the initramfs CPIO format. As you know Al's reasons for not using some other format persist today. "cat /sys/kernel/security/ima/policy" will list the current policy. The rules is based on the fsmagic labels. The builtin policy can be replaced with a custom policy. From include/uapi/linux/magic.h: #define TMPFS_MAGIC 0x01021994 > I suppose that is because no one can tamper with the storage > of tmpfs, but following the same logic, nobody can tamper with > the storage of overlayfs files without tampering with storage of > underlying fs (e.g. xfs), so measuring overlayfs files should not > bring any extra security to the system. Sorry, files, especially random name files, can be stored on tmpfs and do need to be measured and appraised. > Especially, since if files are signed they are signed in the real > storage (e.g. xfs) and not in overlayfs. IMA-appraisal needs to prevent executing files that aren't properly signed no matter the filesystem. We can't just ignore the upper filesystem. What happens if file metadata - ower, group, modes - changes. Is the file data and metadata copied up to the overlay? If the file data remains the same, then the file signature should still verify. So it isn't as simple as saying the upper layer shouldn't ever be verified. > So in theory, we should never ever measure files in the > "virtual" overlayfs and only measure them in the real fs. > The only problem is the the IMA hooks when executing, > mmaping, reading files from overlayfs, don't work on the real fs. Right, if the file data changed, then signature verification would fail with the existing signature. And in most situations that is exactly what is desired. It's possible someone would want to sign files on upper layer with their own key, but let's ignore that use case scenario for now. > fsnotify also was not working correctly in that respect, because > fs operations on overlayfs did not always trigger fsnotify events > on the underlying real fs. Yes, that will probably address detecting file change on the real inode when accessing the lower overlay. Not sure that it will address the upper overlay issues. > This was fixed in 6.5 by commit bc2473c90fca ("ovl: enable fsnotify > events on underlying real files") and the file_real_path() infrastructure > was added to enable this. Ok, will take a closer look later. > This is why I say, that in most likelihood, IMA hook should always use > file_real_path() and file_dentry() to perform the measurements > and record the path of the real fs when overlayfs is performing the > actual read/mmap on the real fs and IMA hooks should ideally > do nothing at all (as in tmpfs) when the operation is performed > on the "virtual" overlayfs object. An IMA policy rule could be defined to ignore the upper layer, but it can't be the default. If the policy is always measure and appraise executables and mmap'ed libraries regardless of the filesystem, then we can't just ignore the upper layer. Shannah tova! Mimi
On Fri, Sep 15, 2023 at 2:36 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Fri, 2023-09-15 at 12:57 +0300, Amir Goldstein wrote: > > [adding back fsdevel] > > > > On Fri, Sep 15, 2023 at 7:04 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > > > On Fri, 2023-09-15 at 06:21 +0300, Amir Goldstein wrote: > > > > On Thu, Sep 14, 2023 at 11:01 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > > > > > > > Hi Amir, Goldwyn, > > > > > > > > > > FYI, > > > > > 1. ima_file_free() is called to check whether the file is in policy. > > > > > 2. ima_check_last_writer() is called to determine whether or not the > > > > > file hash stored as an xattr needs to be updated. > > > > > > > > > > 3. As ima_update_xattr() is not being called, I assume there is no > > > > > appraise rule. I asked on the thread which policy rules are being used > > > > > and for the boot command line, but assume that they're specifying > > > > > 'ima_policy=tcb" on the boot command line. > > > > > > > > Yes, here is the kconfig from syzbot report dashboard > > > > https://syzkaller.appspot.com/x/.config?x=df91a3034fe3f122 > > > > > > > > > > > > > > 4. Is commit db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the > > > > > i_version") the problem? > > > > > > > > IIUC, this commit is responsible for the ovl_getattr() call in the stack > > > > trace. syzbot did not bisect the bug yet, but now that it has found > > > > a reproducer, it is just a matter of time. > > > > However, all the bug reports in the dashboard are only from upstream, > > > > so I think that means that this bug was not found on any stable kernels. > > > > > > > > > > > > > > 5. ima_file_free() is being called twice. We should not be seeing > > > > > ima_get_current_hash_algo() in the trace. > > > > > > > > > > [ 66.991195][ T5030] ovl_getattr+0x1b1/0xf70 > > > > > [ 66.995635][ T5030] ? ovl_setattr+0x4e0/0x4e0 > > > > > [ 67.000229][ T5030] ? trace_raw_output_contention_end+0xd0/0xd0 > > > > > [ 67.006387][ T5030] ? rcu_is_watching+0x15/0xb0 > > > > > [ 67.011154][ T5030] ? rcu_is_watching+0x15/0xb0 > > > > > [ 67.015920][ T5030] ? trace_contention_end+0x3c/0xf0 > > > > > [ 67.021122][ T5030] ? __mutex_lock_common+0x42d/0x2530 > > > > > [ 67.026506][ T5030] ? lock_release+0xbf/0x9d0 > > > > > [ 67.031126][ T5030] ? read_lock_is_recursive+0x20/0x20 > > > > > [ 67.036719][ T5030] ? ima_file_free+0x17c/0x4b0 > > > > > [ 67.041578][ T5030] ? __lock_acquire+0x7f70/0x7f70 > > > > > [ 67.046615][ T5030] ? locks_remove_file+0x429/0x1040 > > > > > [ 67.051820][ T5030] ? mutex_lock_io_nested+0x60/0x60 > > > > > [ 67.057030][ T5030] ? _raw_spin_unlock+0x40/0x40 > > > > > [ 67.061894][ T5030] ? __asan_memset+0x23/0x40 > > > > > [ 67.066577][ T5030] ima_file_free+0x26e/0x4b0 > > > > > [ 67.071279][ T5030] ? ima_get_current_hash_algo+0x10/0x10 > > > > > [ 67.076929][ T5030] ? __rwlock_init+0x150/0x150 > > > > > [ 67.081694][ T5030] ? __lock_acquire+0x7f70/0x7f70 > > > > > [ 67.086727][ T5030] __fput+0x36a/0x910 > > > > > [ 67.090728][ T5030] task_work_run+0x24a/0x300 > > > > > > > > > > > > > > > Were you able to duplicate this locally? > > > > > > > > > > > > > I did not try. Honestly, I don't know how to enable IMA. > > > > Is the only thing that I need to do is set the IMA policy > > > > in the kernel command line? > > > > > > > > Does IMA need to be enabled per fs? per sb? > > > > > > > > If so, I can run the overlay test suite with IMA enabled and > > > > see what happens. > > > > > > Yes, you'll definitely will be able to see the measurement list. > > > > > > [Setting up the system to verify file signatures is a bit more > > > difficult: files need to be signed, keys need to be loaded. Finally > > > CentOS and RHEL 9.3 will have file signatures and will publish the IMA > > > code signing key.] > > > > > > Assuming IMA is configured, just add "ima_policy=tcb" to the command > > > line. This will measure all files executed, mmap'ed, kernel modules, > > > firmware, and all files opened by root. Normally the builtin policy is > > > replaced with a finer grained one. > > > > > > Below are a few commands, but Ken Goldman is writing documentation - > > > https://ima-doc.readthedocs.io/en/latest/ > > > > > > 1. Display the IMA measurement list: > > > # cat /sys/kernel/security/ima/ascii_runtime_measurements > > > # cat /sys/kernel/security/ima/binary_runtime_measurements > > > > > > 2. Display the IMA policy (or append to the policy) > > > # cat /sys/kernel/security/ima/policy > > > > > > 3. Display number of measurements > > > # cat /sys/kernel/security/ima/runtime_measurements_count > > > > > > > Nice. > > This seems to work fine and nothing pops up when running > > fstests unionmount tests of overlayfs over xfs. > > > > What strikes me as strange is that there are measurements > > of files in xfs and in overlayfs, but no measurements of files in tmpfs. > > tmpfs is excluded from policy, since there is no way of storing the > file signature in the initramfs (CPIO). There have been a number of > attempts of extending the initramfs CPIO format. As you know Al's > reasons for not using some other format persist today. > > "cat /sys/kernel/security/ima/policy" will list the current policy. > The rules is based on the fsmagic labels. The builtin policy can be > replaced with a custom policy. > > From include/uapi/linux/magic.h: > #define TMPFS_MAGIC 0x01021994 > > > I suppose that is because no one can tamper with the storage > > of tmpfs, but following the same logic, nobody can tamper with > > the storage of overlayfs files without tampering with storage of > > underlying fs (e.g. xfs), so measuring overlayfs files should not > > bring any extra security to the system. > > Sorry, files, especially random name files, can be stored on tmpfs and > do need to be measured and appraised. > > > Especially, since if files are signed they are signed in the real > > storage (e.g. xfs) and not in overlayfs. > > IMA-appraisal needs to prevent executing files that aren't properly > signed no matter the filesystem. We can't just ignore the upper > filesystem. What happens if file metadata - ower, group, modes - > changes. Is the file data and metadata copied up to the overlay? If > the file data remains the same, then the file signature should still > verify. So it isn't as simple as saying the upper layer shouldn't > ever be verified. > > But that is not what I am saying. I wasn't able to make my point, so I will need to use better terminology. Overlayfs files/inodes are *virutal unions* of lower and upper *real* files/inodes. You are talking about the aspect that on copy up from real lower to real upper, the IMA signature needs to be copied to the real upper file, but that is unrelated to the point I am trying to make. My point is that any read/mmap/exec of an overlay file object is *always* delegated to either real upper file or real lower file which are stored in some *real* fs (e.g. xfs). IMA already verifies signatures on access to the real files, either lower or upper. There is no added security in verifying the signature again on the *union* file, which is just a *virtual* accessor to the *real* files. IOW, the only way to tamper with overlayfs file meta/data is to tamper with the meta/data of the real files in either upper or lower layer. > > So in theory, we should never ever measure files in the > > "virtual" overlayfs and only measure them in the real fs. > > The only problem is the the IMA hooks when executing, > > mmaping, reading files from overlayfs, don't work on the real fs. > > Right, if the file data changed, then signature verification would fail > with the existing signature. And in most situations that is exactly > what is desired. > > It's possible someone would want to sign files on upper layer with > their own key, but let's ignore that use case scenario for now. > > > fsnotify also was not working correctly in that respect, because > > fs operations on overlayfs did not always trigger fsnotify events > > on the underlying real fs. > > Yes, that will probably address detecting file change on the real inode > when accessing the lower overlay. Not sure that it will address the > upper overlay issues. > upper and lower files are both real. > > This was fixed in 6.5 by commit bc2473c90fca ("ovl: enable fsnotify > > events on underlying real files") and the file_real_path() infrastructure > > was added to enable this. > > Ok, will take a closer look later. > > > This is why I say, that in most likelihood, IMA hook should always use > > file_real_path() and file_dentry() to perform the measurements > > and record the path of the real fs when overlayfs is performing the > > actual read/mmap on the real fs and IMA hooks should ideally > > do nothing at all (as in tmpfs) when the operation is performed > > on the "virtual" overlayfs object. > > An IMA policy rule could be defined to ignore the upper layer, but it > can't be the default. If the policy is always measure and appraise > executables and mmap'ed libraries regardless of the filesystem, then we > can't just ignore the upper layer. > It should not ignore the upper layer. It should ignore the virtual union of lower+upper which is called overlayfs. > Shannah tova! > Shana Tova! Amir.
On Fri, 2023-09-15 at 12:57 +0300, Amir Goldstein wrote: > > Assuming IMA is configured, just add "ima_policy=tcb" to the command > > line. This will measure all files executed, mmap'ed, kernel modules, > > firmware, and all files opened by root. Normally the builtin policy is > > replaced with a finer grained one. > > > > Below are a few commands, but Ken Goldman is writing documentation - > > https://ima-doc.readthedocs.io/en/latest/ > > > > 1. Display the IMA measurement list: > > # cat /sys/kernel/security/ima/ascii_runtime_measurements > > # cat /sys/kernel/security/ima/binary_runtime_measurements > > > > 2. Display the IMA policy (or append to the policy) > > # cat /sys/kernel/security/ima/policy > > > > 3. Display number of measurements > > # cat /sys/kernel/security/ima/runtime_measurements_count > > > > Nice. > This seems to work fine and nothing pops up when running > fstests unionmount tests of overlayfs over xfs. > > What strikes me as strange is that there are measurements > of files in xfs and in overlayfs, but no measurements of files in tmpfs. > I suppose that is because no one can tamper with the storage > of tmpfs, but following the same logic, nobody can tamper with > the storage of overlayfs files without tampering with storage of > underlying fs (e.g. xfs), so measuring overlayfs files should not > bring any extra security to the system. > > Especially, since if files are signed they are signed in the real > storage (e.g. xfs) and not in overlayfs. > > So in theory, we should never ever measure files in the > "virtual" overlayfs and only measure them in the real fs. > The only problem is the the IMA hooks when executing, > mmaping, reading files from overlayfs, don't work on the real fs. > > fsnotify also was not working correctly in that respect, because > fs operations on overlayfs did not always trigger fsnotify events > on the underlying real fs. > > This was fixed in 6.5 by commit bc2473c90fca ("ovl: enable fsnotify > events on underlying real files") and the file_real_path() infrastructure > was added to enable this. > > This is why I say, that in most likelihood, IMA hook should always use > file_real_path() and file_dentry() to perform the measurements > and record the path of the real fs when overlayfs is performing the > actual read/mmap on the real fs and IMA hooks should ideally > do nothing at all (as in tmpfs) when the operation is performed > on the "virtual" overlayfs object. tmpfs is excluded from the builtin policy, since there is no way of storing the file signature in the initramfs (CPIO). There have been a number of attempts at extending the initramfs CPIO format, but none have been upstreamed. Agreed, IMA should always use the real file for both the lower and the upper overlayfs.
On Mon, Sep 18, 2023 at 2:00 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Fri, 2023-09-15 at 12:57 +0300, Amir Goldstein wrote: > > > > Assuming IMA is configured, just add "ima_policy=tcb" to the command > > > line. This will measure all files executed, mmap'ed, kernel modules, > > > firmware, and all files opened by root. Normally the builtin policy is > > > replaced with a finer grained one. > > > > > > Below are a few commands, but Ken Goldman is writing documentation - > > > https://ima-doc.readthedocs.io/en/latest/ > > > > > > 1. Display the IMA measurement list: > > > # cat /sys/kernel/security/ima/ascii_runtime_measurements > > > # cat /sys/kernel/security/ima/binary_runtime_measurements > > > > > > 2. Display the IMA policy (or append to the policy) > > > # cat /sys/kernel/security/ima/policy > > > > > > 3. Display number of measurements > > > # cat /sys/kernel/security/ima/runtime_measurements_count > > > > > > > Nice. > > This seems to work fine and nothing pops up when running > > fstests unionmount tests of overlayfs over xfs. > > > > What strikes me as strange is that there are measurements > > of files in xfs and in overlayfs, but no measurements of files in tmpfs. > > I suppose that is because no one can tamper with the storage > > of tmpfs, but following the same logic, nobody can tamper with > > the storage of overlayfs files without tampering with storage of > > underlying fs (e.g. xfs), so measuring overlayfs files should not > > bring any extra security to the system. > > > > Especially, since if files are signed they are signed in the real > > storage (e.g. xfs) and not in overlayfs. > > > > So in theory, we should never ever measure files in the > > "virtual" overlayfs and only measure them in the real fs. > > The only problem is the the IMA hooks when executing, > > mmaping, reading files from overlayfs, don't work on the real fs. > > > > fsnotify also was not working correctly in that respect, because > > fs operations on overlayfs did not always trigger fsnotify events > > on the underlying real fs. > > > > This was fixed in 6.5 by commit bc2473c90fca ("ovl: enable fsnotify > > events on underlying real files") and the file_real_path() infrastructure > > was added to enable this. > > > > This is why I say, that in most likelihood, IMA hook should always use > > file_real_path() and file_dentry() to perform the measurements > > and record the path of the real fs when overlayfs is performing the > > actual read/mmap on the real fs and IMA hooks should ideally > > do nothing at all (as in tmpfs) when the operation is performed > > on the "virtual" overlayfs object. > > tmpfs is excluded from the builtin policy, since there is no way of > storing the file signature in the initramfs (CPIO). There have been a > number of attempts at extending the initramfs CPIO format, but none > have been upstreamed. > > Agreed, IMA should always use the real file for both the lower and the > upper overlayfs. > I took a quick look at some IMA security hooks and I think it's not going to be trivial to fix IMA over overlayfs. Simply adding a bunch of file_real_path() is not going to solve all cases. I still think that my patch is correct, but in order to fix the syzbot crash and other issues, a developer will need to run all the IMA test cases over overlayfs and examine every case more closely. If it is acceptable I would recommend to opt-out of IMA measure/appraise of overlayfs files for the default policy, but that means that underlying real files will not be measure/appraise as well. This way we at least shut up syzbot, because we know that this configuration is broken. Anyway, syzbot has just confirmed that the regressing commit is "IMA: use vfs_getattr_nosec to get the i_version" Thanks, Amir.
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(-)