Re: Fwd: [PATCH] ima: fix wrong dereferences of file->f_path

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[adding back fsdevel]

On Fri, Sep 15, 2023 at 7:04 AM Mimi Zohar <zohar@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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.




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux