PROBLEM: IMA xattrs not written on overlayfs

PROBLEM: IMA xattrs not written on overlayfs

[Ignaz Forster]

Hi,

I'm currently experimenting with IMA / EVM on overlayfs, however those 
don't seem to work together very well.

With kernel 4.18 it was possible to at least partially use IMA. As long 
as the O_TRUNC attribute was not set during a copy_up operation 
everything seemed to work so far.

Now when applying the changes from 
https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git/tag/?h=ovl-update-4.19 
(or using kernel 4.19-rc2) every file contents modification or creation 
of a new file will fail, probably because the inode reported by 
overlayfs does not match the physical inode number any more (and thus 
the IMA hashes won't be generated).


A small example for reproduction (on a system with IMA appraisal):
# OVERLAYFS_TEST_DIR=`mktemp -d`
# mkdir "${OVERLAYFS_TEST_DIR}/upper"
# mkdir "${OVERLAYFS_TEST_DIR}/work"
# mount -t overlay -o lowerdir=/etc,upperdir="${OVERLAYFS_TEST_DIR} 
/upper",workdir="${OVERLAYFS_TEST_DIR}/work" overlay /etc
#
# rm -f /etc/test.txt
# echo Test > /etc/test.txt
# cat /etc/test.txt
cat: /etc/test.txt: Permission denied
# ls -s /etc/test.txt
4 /etc/test.txt # <- The contents are there
# getfattr -m . -d /etc/test.txt
# # <- The hash isn't


After some debugging I'm not sure on how to continue from here. My 
assumption is that overlayfs will have to be modified, however I fail to 
see where to start.

Ignaz

Re: PROBLEM: IMA xattrs not written on overlayfs

[Mimi Zohar]

On Fri, 2018-09-07 at 18:49 +0200, Ignaz Forster wrote:
> Hi,
> 
> I'm currently experimenting with IMA / EVM on overlayfs, however those 
> don't seem to work together very well.
> 
> With kernel 4.18 it was possible to at least partially use IMA. As long 
> as the O_TRUNC attribute was not set during a copy_up operation 
> everything seemed to work so far.
> 
> Now when applying the changes from 
> https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git/tag/?h=ovl-update-4.19 
> (or using kernel 4.19-rc2) every file contents modification or creation 
> of a new file will fail, probably because the inode reported by 
> overlayfs does not match the physical inode number any more (and thus 
> the IMA hashes won't be generated).
> 
> 
> A small example for reproduction (on a system with IMA appraisal):
> # OVERLAYFS_TEST_DIR=`mktemp -d`
> # mkdir "${OVERLAYFS_TEST_DIR}/upper"
> # mkdir "${OVERLAYFS_TEST_DIR}/work"
> # mount -t overlay -o lowerdir=/etc,upperdir="${OVERLAYFS_TEST_DIR} 
> /upper",workdir="${OVERLAYFS_TEST_DIR}/work" overlay /etc
> #
> # rm -f /etc/test.txt
> # echo Test > /etc/test.txt
> # cat /etc/test.txt
> cat: /etc/test.txt: Permission denied
> # ls -s /etc/test.txt
> 4 /etc/test.txt # <- The contents are there
> # getfattr -m . -d /etc/test.txt
> # # <- The hash isn't
> 

Thank you for providing the example.  Also on a linux-4.18.0-rcX test
kernel, the file hash isn't being written out either.  The builtin
"appraise_tcb" policy (eg. specified as "ima_policy=appraise_tcb" on
the boot command) has a tmpfs dont_appraise rule.

> After some debugging I'm not sure on how to continue from here. My 
> assumption is that overlayfs will have to be modified, however I fail to 
> see where to start.

Please make sure that you're comparing the results based on using the
same IMA policy.

Mimi

Re: PROBLEM: IMA xattrs not written on overlayfs

[Ignaz Forster]

Am 07.09.18 um 20:45 schrieb Mimi Zohar:
>> A small example for reproduction (on a system with IMA appraisal):
>> # OVERLAYFS_TEST_DIR=`mktemp -d`
>> # mkdir "${OVERLAYFS_TEST_DIR}/upper"
>> # mkdir "${OVERLAYFS_TEST_DIR}/work"
>> # mount -t overlay -o lowerdir=/etc,upperdir="${OVERLAYFS_TEST_DIR}
>> /upper",workdir="${OVERLAYFS_TEST_DIR}/work" overlay /etc
>> #
>> # rm -f /etc/test.txt
>> # echo Test > /etc/test.txt
>> # cat /etc/test.txt
>> cat: /etc/test.txt: Permission denied
>> # ls -s /etc/test.txt
>> 4 /etc/test.txt # <- The contents are there
>> # getfattr -m . -d /etc/test.txt
>> # # <- The hash isn't
>>
> 
> Thank you for providing the example.  Also on a linux-4.18.0-rcX test
> kernel, the file hash isn't being written out either.  The builtin
> "appraise_tcb" policy (eg. specified as "ima_policy=appraise_tcb" on
> the boot command) has a tmpfs dont_appraise rule.

Putting the mount point into /tmp may have been a bad example, however 
at least on my system /tmp is mounted from a btrfs subvolume. Same with 
/var, which I'm using for my personal tests.

Ignaz

Re: PROBLEM: IMA xattrs not written on overlayfs

[Mimi Zohar]

On Mon, 2018-09-10 at 11:17 +0200, Ignaz Forster wrote:
> Am 07.09.18 um 20:45 schrieb Mimi Zohar:
> >> A small example for reproduction (on a system with IMA appraisal):
> >> # OVERLAYFS_TEST_DIR=`mktemp -d`
> >> # mkdir "${OVERLAYFS_TEST_DIR}/upper"
> >> # mkdir "${OVERLAYFS_TEST_DIR}/work"
> >> # mount -t overlay -o lowerdir=/etc,upperdir="${OVERLAYFS_TEST_DIR}
> >> /upper",workdir="${OVERLAYFS_TEST_DIR}/work" overlay /etc
> >> #
> >> # rm -f /etc/test.txt
> >> # echo Test > /etc/test.txt
> >> # cat /etc/test.txt
> >> cat: /etc/test.txt: Permission denied
> >> # ls -s /etc/test.txt
> >> 4 /etc/test.txt # <- The contents are there
> >> # getfattr -m . -d /etc/test.txt
> >> # # <- The hash isn't
> >>
> > 
> > Thank you for providing the example.  Also on a linux-4.18.0-rcX test
> > kernel, the file hash isn't being written out either.  The builtin
> > "appraise_tcb" policy (eg. specified as "ima_policy=appraise_tcb" on
> > the boot command) has a tmpfs dont_appraise rule.
> 
> Putting the mount point into /tmp may have been a bad example, however 
> at least on my system /tmp is mounted from a btrfs subvolume. Same with 
> /var, which I'm using for my personal tests.

The file size is still 0, when ima_check_last_writer() calls
ima_update_xattr(), which tries to calculate the file hash and write
it out an security.ima.

Mimi

Re: PROBLEM: IMA xattrs not written on overlayfs

[Ignaz Forster]

[-- Attachment #1: Type: text/plain, Size: 1240 bytes --]

Am 28.09.18 um 18:54 schrieb Mimi Zohar:
> On Mon, 2018-09-10 at 11:17 +0200, Ignaz Forster wrote:
>> Am 07.09.18 um 20:45 schrieb Mimi Zohar:
>>>> A small example for reproduction (on a system with IMA appraisal):
>>>> # OVERLAYFS_TEST_DIR=`mktemp -d`
>>>> # mkdir "${OVERLAYFS_TEST_DIR}/upper"
>>>> # mkdir "${OVERLAYFS_TEST_DIR}/work"
>>>> # mount -t overlay -o lowerdir=/etc,upperdir="${OVERLAYFS_TEST_DIR}
>>>> /upper",workdir="${OVERLAYFS_TEST_DIR}/work" overlay /etc
>>>> #
>>>> # rm -f /etc/test.txt
>>>> # echo Test > /etc/test.txt
>>>> # cat /etc/test.txt
>>>> cat: /etc/test.txt: Permission denied
>>>> # ls -s /etc/test.txt
>>>> 4 /etc/test.txt # <- The contents are there
>>>> # getfattr -m . -d /etc/test.txt
>>>> # # <- The hash isn't
>>>>
> The file size is still 0, when ima_check_last_writer() calls
> ima_update_xattr(), which tries to calculate the file hash and write
> it out an security.ima.

We found out that when forcibly setting the read flag in 
ovl_open_realfile as seen in the attached patch the IMA attributes will 
be set correctly again. It seems IMA cannot read the file contents and 
thus cannot create the hash any more.

This is obviously not ready for production, but the best I currently have.

Ignaz

[-- Attachment #2: 01_ima_overlay_new_file.patch --]
[-- Type: text/x-patch, Size: 737 bytes --]

diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index aeaefd2a551b..b96663a2fbac 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -23,13 +23,15 @@ static char ovl_whatisit(struct inode *inode, struct inode *realinode)
 		return 'm';
 }
 
-static struct file *ovl_open_realfile(const struct file *file,
+static struct file *ovl_open_realfile(struct file *file,
 				      struct inode *realinode)
 {
 	struct inode *inode = file_inode(file);
 	struct file *realfile;
 	const struct cred *old_cred;
 
+	file->f_flags &= ~O_WRONLY;
+	file->f_flags |= O_RDWR;
 	old_cred = ovl_override_creds(inode->i_sb);
 	realfile = open_with_fake_path(&file->f_path, file->f_flags | O_NOATIME,
 				       realinode, current_cred());

Re: PROBLEM: IMA xattrs not written on overlayfs

[Ignaz Forster]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: multipart/mixed; boundary="------------BC1E6C429149E8EDD60E2DAE", Size: 1364 bytes --]

Am 28.09.18 um 18:54 schrieb Mimi Zohar:
> On Mon, 2018-09-10 at 11:17 +0200, Ignaz Forster wrote:
>> Am 07.09.18 um 20:45 schrieb Mimi Zohar:
>>>> A small example for reproduction (on a system with IMA appraisal):
>>>> # OVERLAYFS_TEST_DIR=`mktemp -d`
>>>> # mkdir "${OVERLAYFS_TEST_DIR}/upper"
>>>> # mkdir "${OVERLAYFS_TEST_DIR}/work"
>>>> # mount -t overlay -o lowerdir=/etc,upperdir="${OVERLAYFS_TEST_DIR}
>>>> /upper",workdir="${OVERLAYFS_TEST_DIR}/work" overlay /etc
>>>> #
>>>> # rm -f /etc/test.txt
>>>> # echo Test > /etc/test.txt
>>>> # cat /etc/test.txt
>>>> cat: /etc/test.txt: Permission denied
>>>> # ls -s /etc/test.txt
>>>> 4 /etc/test.txt # <- The contents are there
>>>> # getfattr -m . -d /etc/test.txt
>>>> # # <- The hash isn't
>>>>
> The file size is still 0, when ima_check_last_writer() calls
> ima_update_xattr(), which tries to calculate the file hash and write
> it out an security.ima.

We found out that when forcibly setting the read flag in 
ovl_open_realfile as seen in the attached patch the IMA attributes will 
be set correctly again. It seems IMA cannot read the file contents and 
thus cannot create the hash any more.

This is obviously not ready for production, but the best I currently have.

Ignaz


    [ Part 2, Text/X-PATCH (Name: "01_ima_overlay_new_file.patch") 22 ]
    [ lines. ]
    [ Unable to print this part. ]

Re: PROBLEM: IMA xattrs not written on overlayfs

[Mimi Zohar]

On Fri, 2018-09-28 at 20:24 +0200, Ignaz Forster wrote:
> Am 28.09.18 um 18:54 schrieb Mimi Zohar:
> > On Mon, 2018-09-10 at 11:17 +0200, Ignaz Forster wrote:
> >> Am 07.09.18 um 20:45 schrieb Mimi Zohar:
> >>>> A small example for reproduction (on a system with IMA appraisal):
> >>>> # OVERLAYFS_TEST_DIR=`mktemp -d`
> >>>> # mkdir "${OVERLAYFS_TEST_DIR}/upper"
> >>>> # mkdir "${OVERLAYFS_TEST_DIR}/work"
> >>>> # mount -t overlay -o lowerdir=/etc,upperdir="${OVERLAYFS_TEST_DIR}
> >>>> /upper",workdir="${OVERLAYFS_TEST_DIR}/work" overlay /etc
> >>>> #
> >>>> # rm -f /etc/test.txt
> >>>> # echo Test > /etc/test.txt
> >>>> # cat /etc/test.txt
> >>>> cat: /etc/test.txt: Permission denied
> >>>> # ls -s /etc/test.txt
> >>>> 4 /etc/test.txt # <- The contents are there
> >>>> # getfattr -m . -d /etc/test.txt
> >>>> # # <- The hash isn't
> >>>>
> > The file size is still 0, when ima_check_last_writer() calls
> > ima_update_xattr(), which tries to calculate the file hash and write
> > it out an security.ima.
> 
> We found out that when forcibly setting the read flag in 
> ovl_open_realfile as seen in the attached patch the IMA attributes will 
> be set correctly again. It seems IMA cannot read the file contents and 
> thus cannot create the hash any more.
> 
> This is obviously not ready for production, but the best I currently have.

Yeah, having file read access is definitely required to calculate a
file hash.  If it helps, you could make this change in IMA and then
revert it after calculating the file hash.

Mimi

Re: PROBLEM: IMA xattrs not written on overlayfs

[Mimi Zohar]

On Fri, 2018-09-28 at 20:24 +0200, Ignaz Forster wrote:
> Am 28.09.18 um 18:54 schrieb Mimi Zohar:
> > On Mon, 2018-09-10 at 11:17 +0200, Ignaz Forster wrote:
> >> Am 07.09.18 um 20:45 schrieb Mimi Zohar:
> >>>> A small example for reproduction (on a system with IMA appraisal):
> >>>> # OVERLAYFS_TEST_DIR=`mktemp -d`
> >>>> # mkdir "${OVERLAYFS_TEST_DIR}/upper"
> >>>> # mkdir "${OVERLAYFS_TEST_DIR}/work"
> >>>> # mount -t overlay -o lowerdir=/etc,upperdir="${OVERLAYFS_TEST_DIR}
> >>>> /upper",workdir="${OVERLAYFS_TEST_DIR}/work" overlay /etc
> >>>> #
> >>>> # rm -f /etc/test.txt
> >>>> # echo Test > /etc/test.txt
> >>>> # cat /etc/test.txt
> >>>> cat: /etc/test.txt: Permission denied
> >>>> # ls -s /etc/test.txt
> >>>> 4 /etc/test.txt # <- The contents are there
> >>>> # getfattr -m . -d /etc/test.txt
> >>>> # # <- The hash isn't
> >>>>
> > The file size is still 0, when ima_check_last_writer() calls
> > ima_update_xattr(), which tries to calculate the file hash and write
> > it out an security.ima.
> 
> We found out that when forcibly setting the read flag in 
> ovl_open_realfile as seen in the attached patch the IMA attributes will 
> be set correctly again. It seems IMA cannot read the file contents and 
> thus cannot create the hash any more.
> 
> This is obviously not ready for production, but the best I currently have.

Yeah, having file read access is definitely required to calculate a
file hash.  If it helps, you could make this change in IMA and then
revert it after calculating the file hash.

Mimi

Re: PROBLEM: IMA xattrs not written on overlayfs

[Fabian Vogt]

Hi,

I'm the other person Ignaz refers to when he wrote "we".

Am Freitag, 28. September 2018, 20:24:47 CEST schrieb Ignaz Forster:
> Am 28.09.18 um 18:54 schrieb Mimi Zohar:
> > On Mon, 2018-09-10 at 11:17 +0200, Ignaz Forster wrote:
> >> Am 07.09.18 um 20:45 schrieb Mimi Zohar:
> >>>> A small example for reproduction (on a system with IMA appraisal):
> >>>> # OVERLAYFS_TEST_DIR=`mktemp -d`
> >>>> # mkdir "${OVERLAYFS_TEST_DIR}/upper"
> >>>> # mkdir "${OVERLAYFS_TEST_DIR}/work"
> >>>> # mount -t overlay -o lowerdir=/etc,upperdir="${OVERLAYFS_TEST_DIR}
> >>>> /upper",workdir="${OVERLAYFS_TEST_DIR}/work" overlay /etc
> >>>> #
> >>>> # rm -f /etc/test.txt
> >>>> # echo Test > /etc/test.txt
> >>>> # cat /etc/test.txt
> >>>> cat: /etc/test.txt: Permission denied
> >>>> # ls -s /etc/test.txt
> >>>> 4 /etc/test.txt # <- The contents are there
> >>>> # getfattr -m . -d /etc/test.txt
> >>>> # # <- The hash isn't
> >>>>
> > The file size is still 0, when ima_check_last_writer() calls
> > ima_update_xattr(), which tries to calculate the file hash and write
> > it out an security.ima.
> 
> We found out that when forcibly setting the read flag in 
> ovl_open_realfile as seen in the attached patch the IMA attributes will 
> be set correctly again. It seems IMA cannot read the file contents and 
> thus cannot create the hash any more.

In ima_crypto.c, ima_calc_file_hash_{atfm,tfm} we can find this hack (?),
which allows to read even through a WRONLY file:

        if (!(file->f_mode & FMODE_READ)) {
                file->f_mode |= FMODE_READ;
                read = 1;
        }

        integrity_kernel_read(file, ...);

        if (read)
                file->f_mode &= ~FMODE_READ;

In overlayfs since 4.19, it has a private struct file* for the internal filp
pointing to either lower or upper, which is not affected by this change to
the overlayfs' file's f_mode. Thus we get -EBADF back from
integrity_kernel_read -> ... -> ovl_read_iter -> vfs_iter_read.
That was not visible in any logs as IMA ignores -EBADF returned from
ima_calc_file_hash.

Before 4.19, overlayfs used d_real so there was no layer between
integrity_kernel_read and the real filp which means that the f_mode toggle in
ima had the desired effect.

> This is obviously not ready for production, but the best I currently have.

A better alternative for this is to set FMODE_READ temporarily in the private
filp during the call to vfs_iter_read in ovl_read_iter but IIRC we tried that
once and it was not enough. Maybe the test was just incomplete though.

Thanks,
Fabian

> Ignaz

Re: PROBLEM: IMA xattrs not written on overlayfs

[Miklos Szeredi]

On Fri, Sep 28, 2018 at 9:37 PM, Fabian Vogt <fvogt@suse.de> wrote:
> Hi,
>
> I'm the other person Ignaz refers to when he wrote "we".
>
> Am Freitag, 28. September 2018, 20:24:47 CEST schrieb Ignaz Forster:
>> Am 28.09.18 um 18:54 schrieb Mimi Zohar:
>> > On Mon, 2018-09-10 at 11:17 +0200, Ignaz Forster wrote:
>> >> Am 07.09.18 um 20:45 schrieb Mimi Zohar:
>> >>>> A small example for reproduction (on a system with IMA appraisal):
>> >>>> # OVERLAYFS_TEST_DIR=`mktemp -d`
>> >>>> # mkdir "${OVERLAYFS_TEST_DIR}/upper"
>> >>>> # mkdir "${OVERLAYFS_TEST_DIR}/work"
>> >>>> # mount -t overlay -o lowerdir=/etc,upperdir="${OVERLAYFS_TEST_DIR}
>> >>>> /upper",workdir="${OVERLAYFS_TEST_DIR}/work" overlay /etc
>> >>>> #
>> >>>> # rm -f /etc/test.txt
>> >>>> # echo Test > /etc/test.txt
>> >>>> # cat /etc/test.txt
>> >>>> cat: /etc/test.txt: Permission denied
>> >>>> # ls -s /etc/test.txt
>> >>>> 4 /etc/test.txt # <- The contents are there
>> >>>> # getfattr -m . -d /etc/test.txt
>> >>>> # # <- The hash isn't
>> >>>>
>> > The file size is still 0, when ima_check_last_writer() calls
>> > ima_update_xattr(), which tries to calculate the file hash and write
>> > it out an security.ima.
>>
>> We found out that when forcibly setting the read flag in
>> ovl_open_realfile as seen in the attached patch the IMA attributes will
>> be set correctly again. It seems IMA cannot read the file contents and
>> thus cannot create the hash any more.
>
> In ima_crypto.c, ima_calc_file_hash_{atfm,tfm} we can find this hack (?),
> which allows to read even through a WRONLY file:
>
>         if (!(file->f_mode & FMODE_READ)) {
>                 file->f_mode |= FMODE_READ;
>                 read = 1;
>         }
>
>         integrity_kernel_read(file, ...);
>
>         if (read)
>                 file->f_mode &= ~FMODE_READ;

It's not just a hack, it's a security hole: what prevents a read(2) on
that file from userspace exploiting the window while the f_mode is
changed?

Thanks,
Miklos

Re: PROBLEM: IMA xattrs not written on overlayfs

[Mimi Zohar]

Hi Miklos,

On Mon, 2018-10-01 at 11:05 +0200, Miklos Szeredi wrote:
> On Fri, Sep 28, 2018 at 9:37 PM, Fabian Vogt <fvogt@suse.de> wrote:
> > Hi,
> >
> > I'm the other person Ignaz refers to when he wrote "we".
> >
> > Am Freitag, 28. September 2018, 20:24:47 CEST schrieb Ignaz Forster:
> >> Am 28.09.18 um 18:54 schrieb Mimi Zohar:
> >> > On Mon, 2018-09-10 at 11:17 +0200, Ignaz Forster wrote:
> >> >> Am 07.09.18 um 20:45 schrieb Mimi Zohar:
> >> >>>> A small example for reproduction (on a system with IMA appraisal):
> >> >>>> # OVERLAYFS_TEST_DIR=`mktemp -d`
> >> >>>> # mkdir "${OVERLAYFS_TEST_DIR}/upper"
> >> >>>> # mkdir "${OVERLAYFS_TEST_DIR}/work"
> >> >>>> # mount -t overlay -o lowerdir=/etc,upperdir="${OVERLAYFS_TEST_DIR}
> >> >>>> /upper",workdir="${OVERLAYFS_TEST_DIR}/work" overlay /etc
> >> >>>> #
> >> >>>> # rm -f /etc/test.txt
> >> >>>> # echo Test > /etc/test.txt
> >> >>>> # cat /etc/test.txt
> >> >>>> cat: /etc/test.txt: Permission denied
> >> >>>> # ls -s /etc/test.txt
> >> >>>> 4 /etc/test.txt # <- The contents are there
> >> >>>> # getfattr -m . -d /etc/test.txt
> >> >>>> # # <- The hash isn't
> >> >>>>
> >> > The file size is still 0, when ima_check_last_writer() calls
> >> > ima_update_xattr(), which tries to calculate the file hash and write
> >> > it out an security.ima.
> >>
> >> We found out that when forcibly setting the read flag in
> >> ovl_open_realfile as seen in the attached patch the IMA attributes will
> >> be set correctly again. It seems IMA cannot read the file contents and
> >> thus cannot create the hash any more.
> >
> > In ima_crypto.c, ima_calc_file_hash_{atfm,tfm} we can find this hack (?),
> > which allows to read even through a WRONLY file:
> >
> >         if (!(file->f_mode & FMODE_READ)) {
> >                 file->f_mode |= FMODE_READ;
> >                 read = 1;
> >         }
> >
> >         integrity_kernel_read(file, ...);
> >
> >         if (read)
> >                 file->f_mode &= ~FMODE_READ;
> 
> It's not just a hack, it's a security hole: what prevents a read(2) on
> that file from userspace exploiting the window while the f_mode is
> changed?

IMA calculates the file hash either on the last write close (__fput)
or when verifying the file hash on file open.  The discussion here is
about calculating the file hash on __fput.  Before the file can be
read, the IMA file hash or signature would have to be verified first
on file open. 

Unless I'm missing something, which is totally possible, I'm not sure
there is a problem.

Mimi

Re: PROBLEM: IMA xattrs not written on overlayfs

[Mimi Zohar]

Hi Miklos,

On Mon, 2018-10-01 at 11:05 +0200, Miklos Szeredi wrote:
> On Fri, Sep 28, 2018 at 9:37 PM, Fabian Vogt <fvogt@suse.de> wrote:
> > Hi,
> >
> > I'm the other person Ignaz refers to when he wrote "we".
> >
> > Am Freitag, 28. September 2018, 20:24:47 CEST schrieb Ignaz Forster:
> >> Am 28.09.18 um 18:54 schrieb Mimi Zohar:
> >> > On Mon, 2018-09-10 at 11:17 +0200, Ignaz Forster wrote:
> >> >> Am 07.09.18 um 20:45 schrieb Mimi Zohar:
> >> >>>> A small example for reproduction (on a system with IMA appraisal):
> >> >>>> # OVERLAYFS_TEST_DIR=`mktemp -d`
> >> >>>> # mkdir "${OVERLAYFS_TEST_DIR}/upper"
> >> >>>> # mkdir "${OVERLAYFS_TEST_DIR}/work"
> >> >>>> # mount -t overlay -o lowerdir=/etc,upperdir="${OVERLAYFS_TEST_DIR}
> >> >>>> /upper",workdir="${OVERLAYFS_TEST_DIR}/work" overlay /etc
> >> >>>> #
> >> >>>> # rm -f /etc/test.txt
> >> >>>> # echo Test > /etc/test.txt
> >> >>>> # cat /etc/test.txt
> >> >>>> cat: /etc/test.txt: Permission denied
> >> >>>> # ls -s /etc/test.txt
> >> >>>> 4 /etc/test.txt # <- The contents are there
> >> >>>> # getfattr -m . -d /etc/test.txt
> >> >>>> # # <- The hash isn't
> >> >>>>
> >> > The file size is still 0, when ima_check_last_writer() calls
> >> > ima_update_xattr(), which tries to calculate the file hash and write
> >> > it out an security.ima.
> >>
> >> We found out that when forcibly setting the read flag in
> >> ovl_open_realfile as seen in the attached patch the IMA attributes will
> >> be set correctly again. It seems IMA cannot read the file contents and
> >> thus cannot create the hash any more.
> >
> > In ima_crypto.c, ima_calc_file_hash_{atfm,tfm} we can find this hack (?),
> > which allows to read even through a WRONLY file:
> >
> >         if (!(file->f_mode & FMODE_READ)) {
> >                 file->f_mode |= FMODE_READ;
> >                 read = 1;
> >         }
> >
> >         integrity_kernel_read(file, ...);
> >
> >         if (read)
> >                 file->f_mode &= ~FMODE_READ;
> 
> It's not just a hack, it's a security hole: what prevents a read(2) on
> that file from userspace exploiting the window while the f_mode is
> changed?

IMA calculates the file hash either on the last write close (__fput)
or when verifying the file hash on file open.  The discussion here is
about calculating the file hash on __fput.  Before the file can be
read, the IMA file hash or signature would have to be verified first
on file open. 

Unless I'm missing something, which is totally possible, I'm not sure
there is a problem.

Mimi

Re: PROBLEM: IMA xattrs not written on overlayfs

[Miklos Szeredi]

On Wed, Oct 3, 2018 at 11:18 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> Hi Miklos,
>
> On Mon, 2018-10-01 at 11:05 +0200, Miklos Szeredi wrote:
>> On Fri, Sep 28, 2018 at 9:37 PM, Fabian Vogt <fvogt@suse.de> wrote:
>> > Hi,
>> >
>> > I'm the other person Ignaz refers to when he wrote "we".
>> >
>> > Am Freitag, 28. September 2018, 20:24:47 CEST schrieb Ignaz Forster:
>> >> Am 28.09.18 um 18:54 schrieb Mimi Zohar:
>> >> > On Mon, 2018-09-10 at 11:17 +0200, Ignaz Forster wrote:
>> >> >> Am 07.09.18 um 20:45 schrieb Mimi Zohar:
>> >> >>>> A small example for reproduction (on a system with IMA appraisal):
>> >> >>>> # OVERLAYFS_TEST_DIR=`mktemp -d`
>> >> >>>> # mkdir "${OVERLAYFS_TEST_DIR}/upper"
>> >> >>>> # mkdir "${OVERLAYFS_TEST_DIR}/work"
>> >> >>>> # mount -t overlay -o lowerdir=/etc,upperdir="${OVERLAYFS_TEST_DIR}
>> >> >>>> /upper",workdir="${OVERLAYFS_TEST_DIR}/work" overlay /etc
>> >> >>>> #
>> >> >>>> # rm -f /etc/test.txt
>> >> >>>> # echo Test > /etc/test.txt
>> >> >>>> # cat /etc/test.txt
>> >> >>>> cat: /etc/test.txt: Permission denied
>> >> >>>> # ls -s /etc/test.txt
>> >> >>>> 4 /etc/test.txt # <- The contents are there
>> >> >>>> # getfattr -m . -d /etc/test.txt
>> >> >>>> # # <- The hash isn't
>> >> >>>>
>> >> > The file size is still 0, when ima_check_last_writer() calls
>> >> > ima_update_xattr(), which tries to calculate the file hash and write
>> >> > it out an security.ima.
>> >>
>> >> We found out that when forcibly setting the read flag in
>> >> ovl_open_realfile as seen in the attached patch the IMA attributes will
>> >> be set correctly again. It seems IMA cannot read the file contents and
>> >> thus cannot create the hash any more.
>> >
>> > In ima_crypto.c, ima_calc_file_hash_{atfm,tfm} we can find this hack (?),
>> > which allows to read even through a WRONLY file:
>> >
>> >         if (!(file->f_mode & FMODE_READ)) {
>> >                 file->f_mode |= FMODE_READ;
>> >                 read = 1;
>> >         }
>> >
>> >         integrity_kernel_read(file, ...);
>> >
>> >         if (read)
>> >                 file->f_mode &= ~FMODE_READ;
>>
>> It's not just a hack, it's a security hole: what prevents a read(2) on
>> that file from userspace exploiting the window while the f_mode is
>> changed?
>
> IMA calculates the file hash either on the last write close (__fput)
> or when verifying the file hash on file open.  The discussion here is
> about calculating the file hash on __fput.  Before the file can be
> read, the IMA file hash or signature would have to be verified first
> on file open.
>
> Unless I'm missing something, which is totally possible, I'm not sure
> there is a problem.

Right, if it's done from last fput() then it's at least not a security hole.

This hack may work for some filesystems, but as you noticed, it won't
work for overlayfs.  And  if probably won't work for a number of other
filesystems as well: the fs can assume that f_mode & FMODE_READ will
remain off if it was off at open time.

The proper way to handle it generally is to open a separate instance
of the same file from IMA with O_RDONLY and use that to calculate the
hash.   There's really no point in reusing the same file and hacking
the f_mode bits.

Thanks,
Miklos


>
> Mimi
>

Re: PROBLEM: IMA xattrs not written on overlayfs

[Mimi Zohar]

On Thu, 2018-10-04 at 00:35 +0200, Miklos Szeredi wrote:

> Right, if it's done from last fput() then it's at least not a security hole.
> 
> This hack may work for some filesystems, but as you noticed, it won't
> work for overlayfs.  And  if probably won't work for a number of other
> filesystems as well: the fs can assume that f_mode & FMODE_READ will
> remain off if it was off at open time.
> 
> The proper way to handle it generally is to open a separate instance
> of the same file from IMA with O_RDONLY and use that to calculate the
> hash.   There's really no point in reusing the same file and hacking
> the f_mode bits.

Is there an appropriate low level kernel call for creating a new file
descriptor that would be appropriate?  dentry_open(), like the call in
file_clone_open(), has a lot of overhead, including the LSM calls.
 Calculating the file hash always needs to work.

Mimi

Re: PROBLEM: IMA xattrs not written on overlayfs

[Mimi Zohar]

On Thu, 2018-10-04 at 00:35 +0200, Miklos Szeredi wrote:

> Right, if it's done from last fput() then it's at least not a security hole.
> 
> This hack may work for some filesystems, but as you noticed, it won't
> work for overlayfs.  And  if probably won't work for a number of other
> filesystems as well: the fs can assume that f_mode & FMODE_READ will
> remain off if it was off at open time.
> 
> The proper way to handle it generally is to open a separate instance
> of the same file from IMA with O_RDONLY and use that to calculate the
> hash.   There's really no point in reusing the same file and hacking
> the f_mode bits.

Is there an appropriate low level kernel call for creating a new file
descriptor that would be appropriate?  dentry_open(), like the call in
file_clone_open(), has a lot of overhead, including the LSM calls.
 Calculating the file hash always needs to work.

Mimi

Re: PROBLEM: IMA xattrs not written on overlayfs

[Goldwyn Rodrigues]

On 11:52 04/10, Mimi Zohar wrote:
> On Thu, 2018-10-04 at 00:35 +0200, Miklos Szeredi wrote:
> 
> > Right, if it's done from last fput() then it's at least not a security hole.
> > 
> > This hack may work for some filesystems, but as you noticed, it won't
> > work for overlayfs.  And  if probably won't work for a number of other
> > filesystems as well: the fs can assume that f_mode & FMODE_READ will
> > remain off if it was off at open time.
> > 
> > The proper way to handle it generally is to open a separate instance
> > of the same file from IMA with O_RDONLY and use that to calculate the
> > hash.   There's really no point in reusing the same file and hacking
> > the f_mode bits.
> 
> Is there an appropriate low level kernel call for creating a new file
> descriptor that would be appropriate? �dentry_open(), like the call in
> file_clone_open(), has a lot of overhead, including the LSM calls.
> �Calculating the file hash always needs to work.
> 

Mimi,

I have formulated a patch which is working for me on overlayfs. I am
using dentry_open(), which I agree may have overhead. While this
opens up the possibility of using it for files opened with O_DIRECT
which may end up getting the file into pagecache.

Subject: [PATCH] Open new file instance O_RDONLY to calculate hash

Using the open struct file to calculate the hash does not work
with overlayfs, because the real struct file is hidden behind the
overlays struct file. So, any file->f_mode manipulations are not
reflected on the real struct file. So, open the file again, read and
calculate the hash.

As a byproduct, we can remove all instance of f_mode manipulations
and can work with O_DIRECT as well.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 7e7e7e7c250a..87e5fedc2162 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -210,7 +210,7 @@ static int ima_calc_file_hash_atfm(struct file *file,
 {
 	loff_t i_size, offset;
 	char *rbuf[2] = { NULL, };
-	int rc, read = 0, rbuf_len, active = 0, ahash_rc = 0;
+	int rc, rbuf_len, active = 0, ahash_rc = 0;
 	struct ahash_request *req;
 	struct scatterlist sg[1];
 	struct crypto_wait wait;
@@ -257,11 +257,6 @@ static int ima_calc_file_hash_atfm(struct file *file,
 					  &rbuf_size[1], 0);
 	}
 
-	if (!(file->f_mode & FMODE_READ)) {
-		file->f_mode |= FMODE_READ;
-		read = 1;
-	}
-
 	for (offset = 0; offset < i_size; offset += rbuf_len) {
 		if (!rbuf[1] && offset) {
 			/* Not using two buffers, and it is not the first
@@ -300,8 +295,6 @@ static int ima_calc_file_hash_atfm(struct file *file,
 	/* wait for the last update request to complete */
 	rc = ahash_wait(ahash_rc, &wait);
 out3:
-	if (read)
-		file->f_mode &= ~FMODE_READ;
 	ima_free_pages(rbuf[0], rbuf_size[0]);
 	ima_free_pages(rbuf[1], rbuf_size[1]);
 out2:
@@ -336,7 +329,7 @@ static int ima_calc_file_hash_tfm(struct file *file,
 {
 	loff_t i_size, offset = 0;
 	char *rbuf;
-	int rc, read = 0;
+	int rc;
 	SHASH_DESC_ON_STACK(shash, tfm);
 
 	shash->tfm = tfm;
@@ -357,11 +350,6 @@ static int ima_calc_file_hash_tfm(struct file *file,
 	if (!rbuf)
 		return -ENOMEM;
 
-	if (!(file->f_mode & FMODE_READ)) {
-		file->f_mode |= FMODE_READ;
-		read = 1;
-	}
-
 	while (offset < i_size) {
 		int rbuf_len;
 
@@ -378,8 +366,6 @@ static int ima_calc_file_hash_tfm(struct file *file,
 		if (rc)
 			break;
 	}
-	if (read)
-		file->f_mode &= ~FMODE_READ;
 	kfree(rbuf);
 out:
 	if (!rc)
@@ -420,26 +406,21 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
 {
 	loff_t i_size;
 	int rc;
+	struct file *f = dentry_open(&file->f_path, O_LARGEFILE | O_RDONLY,
+			current_cred());
 
-	/*
-	 * For consistency, fail file's opened with the O_DIRECT flag on
-	 * filesystems mounted with/without DAX option.
-	 */
-	if (file->f_flags & O_DIRECT) {
-		hash->length = hash_digest_size[ima_hash_algo];
-		hash->algo = ima_hash_algo;
-		return -EINVAL;
-	}
-
-	i_size = i_size_read(file_inode(file));
+	i_size = i_size_read(file_inode(f));
 
 	if (ima_ahash_minsize && i_size >= ima_ahash_minsize) {
-		rc = ima_calc_file_ahash(file, hash);
+		rc = ima_calc_file_ahash(f, hash);
 		if (!rc)
-			return 0;
+			goto out;
 	}
 
-	return ima_calc_file_shash(file, hash);
+	rc = ima_calc_file_shash(f, hash);
+out:
+	fput(f);
+	return rc;
 }
 
 /*

-- 
Goldwyn

Re: PROBLEM: IMA xattrs not written on overlayfs

[Mimi Zohar]

On Thu, 2018-10-04 at 21:57 -0500, Goldwyn Rodrigues wrote:
> On 11:52 04/10, Mimi Zohar wrote:
> > On Thu, 2018-10-04 at 00:35 +0200, Miklos Szeredi wrote:
> > 
> > > Right, if it's done from last fput() then it's at least not a security hole.
> > > 
> > > This hack may work for some filesystems, but as you noticed, it won't
> > > work for overlayfs.  And  if probably won't work for a number of other
> > > filesystems as well: the fs can assume that f_mode & FMODE_READ will
> > > remain off if it was off at open time.
> > > 
> > > The proper way to handle it generally is to open a separate instance
> > > of the same file from IMA with O_RDONLY and use that to calculate the
> > > hash.   There's really no point in reusing the same file and hacking
> > > the f_mode bits.
> > 
> > Is there an appropriate low level kernel call for creating a new file
> > descriptor that would be appropriate?  dentry_open(), like the call in
> > file_clone_open(), has a lot of overhead, including the LSM calls.
> >  Calculating the file hash always needs to work.
> > 
> 
> Mimi,
> 
> I have formulated a patch which is working for me on overlayfs. I am
> using dentry_open(), which I agree may have overhead. While this
> opens up the possibility of using it for files opened with O_DIRECT
> which may end up getting the file into pagecache.
> 
> Subject: [PATCH] Open new file instance O_RDONLY to calculate hash
> 
> Using the open struct file to calculate the hash does not work
> with overlayfs, because the real struct file is hidden behind the
> overlays struct file. So, any file->f_mode manipulations are not
> reflected on the real struct file. So, open the file again, read and
> calculate the hash.
> 
> As a byproduct, we can remove all instance of f_mode manipulations
> and can work with O_DIRECT as well.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

By "overhead", I didn't mean it from a performance perspective, but
was concerned about the dentry_open() failing.  If "dentry_open" fails
for any reason, the file hash will not be re-calculated, causing
future file opens to fail.  Missing is the test for dentry_open()
failure.

By removing the "read" check, re-opening the file is now for all
files, not just those opened without "read".  From a testing
perspective, it will catch problems faster, but ...

I've had a patch queued that removes the O_DIRECT test, but haven't
had a chance to test it on ALL filesystems.  I would probably re-open
the file with the original flags, plus read, as much as possible.
 
Mimi


> 
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 7e7e7e7c250a..87e5fedc2162 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -210,7 +210,7 @@ static int ima_calc_file_hash_atfm(struct file *file,
>  {
>  	loff_t i_size, offset;
>  	char *rbuf[2] = { NULL, };
> -	int rc, read = 0, rbuf_len, active = 0, ahash_rc = 0;
> +	int rc, rbuf_len, active = 0, ahash_rc = 0;
>  	struct ahash_request *req;
>  	struct scatterlist sg[1];
>  	struct crypto_wait wait;
> @@ -257,11 +257,6 @@ static int ima_calc_file_hash_atfm(struct file *file,
>  					  &rbuf_size[1], 0);
>  	}
>  
> -	if (!(file->f_mode & FMODE_READ)) {
> -		file->f_mode |= FMODE_READ;
> -		read = 1;
> -	}
> -
>  	for (offset = 0; offset < i_size; offset += rbuf_len) {
>  		if (!rbuf[1] && offset) {
>  			/* Not using two buffers, and it is not the first
> @@ -300,8 +295,6 @@ static int ima_calc_file_hash_atfm(struct file *file,
>  	/* wait for the last update request to complete */
>  	rc = ahash_wait(ahash_rc, &wait);
>  out3:
> -	if (read)
> -		file->f_mode &= ~FMODE_READ;
>  	ima_free_pages(rbuf[0], rbuf_size[0]);
>  	ima_free_pages(rbuf[1], rbuf_size[1]);
>  out2:
> @@ -336,7 +329,7 @@ static int ima_calc_file_hash_tfm(struct file *file,
>  {
>  	loff_t i_size, offset = 0;
>  	char *rbuf;
> -	int rc, read = 0;
> +	int rc;
>  	SHASH_DESC_ON_STACK(shash, tfm);
>  
>  	shash->tfm = tfm;
> @@ -357,11 +350,6 @@ static int ima_calc_file_hash_tfm(struct file *file,
>  	if (!rbuf)
>  		return -ENOMEM;
>  
> -	if (!(file->f_mode & FMODE_READ)) {
> -		file->f_mode |= FMODE_READ;
> -		read = 1;
> -	}
> -
>  	while (offset < i_size) {
>  		int rbuf_len;
>  
> @@ -378,8 +366,6 @@ static int ima_calc_file_hash_tfm(struct file *file,
>  		if (rc)
>  			break;
>  	}
> -	if (read)
> -		file->f_mode &= ~FMODE_READ;
>  	kfree(rbuf);
>  out:
>  	if (!rc)
> @@ -420,26 +406,21 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>  {
>  	loff_t i_size;
>  	int rc;
> +	struct file *f = dentry_open(&file->f_path, O_LARGEFILE | O_RDONLY,
> +			current_cred());
>  
> -	/*
> -	 * For consistency, fail file's opened with the O_DIRECT flag on
> -	 * filesystems mounted with/without DAX option.
> -	 */
> -	if (file->f_flags & O_DIRECT) {
> -		hash->length = hash_digest_size[ima_hash_algo];
> -		hash->algo = ima_hash_algo;
> -		return -EINVAL;
> -	}
> -
> -	i_size = i_size_read(file_inode(file));
> +	i_size = i_size_read(file_inode(f));
>  
>  	if (ima_ahash_minsize && i_size >= ima_ahash_minsize) {
> -		rc = ima_calc_file_ahash(file, hash);
> +		rc = ima_calc_file_ahash(f, hash);
>  		if (!rc)
> -			return 0;
> +			goto out;
>  	}
>  
> -	return ima_calc_file_shash(file, hash);
> +	rc = ima_calc_file_shash(f, hash);
> +out:
> +	fput(f);
> +	return rc;
>  }
>  
>  /*
> 

Re: PROBLEM: IMA xattrs not written on overlayfs

[Mimi Zohar]

On Thu, 2018-10-04 at 21:57 -0500, Goldwyn Rodrigues wrote:
> On 11:52 04/10, Mimi Zohar wrote:
> > On Thu, 2018-10-04 at 00:35 +0200, Miklos Szeredi wrote:
> > 
> > > Right, if it's done from last fput() then it's at least not a security hole.
> > > 
> > > This hack may work for some filesystems, but as you noticed, it won't
> > > work for overlayfs.  And  if probably won't work for a number of other
> > > filesystems as well: the fs can assume that f_mode & FMODE_READ will
> > > remain off if it was off at open time.
> > > 
> > > The proper way to handle it generally is to open a separate instance
> > > of the same file from IMA with O_RDONLY and use that to calculate the
> > > hash.   There's really no point in reusing the same file and hacking
> > > the f_mode bits.
> > 
> > Is there an appropriate low level kernel call for creating a new file
> > descriptor that would be appropriate?  dentry_open(), like the call in
> > file_clone_open(), has a lot of overhead, including the LSM calls.
> >  Calculating the file hash always needs to work.
> > 
> 
> Mimi,
> 
> I have formulated a patch which is working for me on overlayfs. I am
> using dentry_open(), which I agree may have overhead. While this
> opens up the possibility of using it for files opened with O_DIRECT
> which may end up getting the file into pagecache.
> 
> Subject: [PATCH] Open new file instance O_RDONLY to calculate hash
> 
> Using the open struct file to calculate the hash does not work
> with overlayfs, because the real struct file is hidden behind the
> overlays struct file. So, any file->f_mode manipulations are not
> reflected on the real struct file. So, open the file again, read and
> calculate the hash.
> 
> As a byproduct, we can remove all instance of f_mode manipulations
> and can work with O_DIRECT as well.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

By "overhead", I didn't mean it from a performance perspective, but
was concerned about the dentry_open() failing.  If "dentry_open" fails
for any reason, the file hash will not be re-calculated, causing
future file opens to fail.  Missing is the test for dentry_open()
failure.

By removing the "read" check, re-opening the file is now for all
files, not just those opened without "read".  From a testing
perspective, it will catch problems faster, but ...

I've had a patch queued that removes the O_DIRECT test, but haven't
had a chance to test it on ALL filesystems.  I would probably re-open
the file with the original flags, plus read, as much as possible.
 
Mimi


> 
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 7e7e7e7c250a..87e5fedc2162 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -210,7 +210,7 @@ static int ima_calc_file_hash_atfm(struct file *file,
>  {
>  	loff_t i_size, offset;
>  	char *rbuf[2] = { NULL, };
> -	int rc, read = 0, rbuf_len, active = 0, ahash_rc = 0;
> +	int rc, rbuf_len, active = 0, ahash_rc = 0;
>  	struct ahash_request *req;
>  	struct scatterlist sg[1];
>  	struct crypto_wait wait;
> @@ -257,11 +257,6 @@ static int ima_calc_file_hash_atfm(struct file *file,
>  					  &rbuf_size[1], 0);
>  	}
>  
> -	if (!(file->f_mode & FMODE_READ)) {
> -		file->f_mode |= FMODE_READ;
> -		read = 1;
> -	}
> -
>  	for (offset = 0; offset < i_size; offset += rbuf_len) {
>  		if (!rbuf[1] && offset) {
>  			/* Not using two buffers, and it is not the first
> @@ -300,8 +295,6 @@ static int ima_calc_file_hash_atfm(struct file *file,
>  	/* wait for the last update request to complete */
>  	rc = ahash_wait(ahash_rc, &wait);
>  out3:
> -	if (read)
> -		file->f_mode &= ~FMODE_READ;
>  	ima_free_pages(rbuf[0], rbuf_size[0]);
>  	ima_free_pages(rbuf[1], rbuf_size[1]);
>  out2:
> @@ -336,7 +329,7 @@ static int ima_calc_file_hash_tfm(struct file *file,
>  {
>  	loff_t i_size, offset = 0;
>  	char *rbuf;
> -	int rc, read = 0;
> +	int rc;
>  	SHASH_DESC_ON_STACK(shash, tfm);
>  
>  	shash->tfm = tfm;
> @@ -357,11 +350,6 @@ static int ima_calc_file_hash_tfm(struct file *file,
>  	if (!rbuf)
>  		return -ENOMEM;
>  
> -	if (!(file->f_mode & FMODE_READ)) {
> -		file->f_mode |= FMODE_READ;
> -		read = 1;
> -	}
> -
>  	while (offset < i_size) {
>  		int rbuf_len;
>  
> @@ -378,8 +366,6 @@ static int ima_calc_file_hash_tfm(struct file *file,
>  		if (rc)
>  			break;
>  	}
> -	if (read)
> -		file->f_mode &= ~FMODE_READ;
>  	kfree(rbuf);
>  out:
>  	if (!rc)
> @@ -420,26 +406,21 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>  {
>  	loff_t i_size;
>  	int rc;
> +	struct file *f = dentry_open(&file->f_path, O_LARGEFILE | O_RDONLY,
> +			current_cred());
>  
> -	/*
> -	 * For consistency, fail file's opened with the O_DIRECT flag on
> -	 * filesystems mounted with/without DAX option.
> -	 */
> -	if (file->f_flags & O_DIRECT) {
> -		hash->length = hash_digest_size[ima_hash_algo];
> -		hash->algo = ima_hash_algo;
> -		return -EINVAL;
> -	}
> -
> -	i_size = i_size_read(file_inode(file));
> +	i_size = i_size_read(file_inode(f));
>  
>  	if (ima_ahash_minsize && i_size >= ima_ahash_minsize) {
> -		rc = ima_calc_file_ahash(file, hash);
> +		rc = ima_calc_file_ahash(f, hash);
>  		if (!rc)
> -			return 0;
> +			goto out;
>  	}
>  
> -	return ima_calc_file_shash(file, hash);
> +	rc = ima_calc_file_shash(f, hash);
> +out:
> +	fput(f);
> +	return rc;
>  }
>  
>  /*
> 

Re: PROBLEM: IMA xattrs not written on overlayfs

[Goldwyn Rodrigues]

On  6:33 05/10, Mimi Zohar wrote:
> On Thu, 2018-10-04 at 21:57 -0500, Goldwyn Rodrigues wrote:
> > On 11:52 04/10, Mimi Zohar wrote:
> > > On Thu, 2018-10-04 at 00:35 +0200, Miklos Szeredi wrote:
> > > 
> > > > Right, if it's done from last fput() then it's at least not a security hole.
> > > > 
> > > > This hack may work for some filesystems, but as you noticed, it won't
> > > > work for overlayfs.  And  if probably won't work for a number of other
> > > > filesystems as well: the fs can assume that f_mode & FMODE_READ will
> > > > remain off if it was off at open time.
> > > > 
> > > > The proper way to handle it generally is to open a separate instance
> > > > of the same file from IMA with O_RDONLY and use that to calculate the
> > > > hash.   There's really no point in reusing the same file and hacking
> > > > the f_mode bits.
> > > 
> > > Is there an appropriate low level kernel call for creating a new file
> > > descriptor that would be appropriate? �dentry_open(), like the call in
> > > file_clone_open(), has a lot of overhead, including the LSM calls.
> > > �Calculating the file hash always needs to work.
> > > 
> > 
> > Mimi,
> > 
> > I have formulated a patch which is working for me on overlayfs. I am
> > using dentry_open(), which I agree may have overhead. While this
> > opens up the possibility of using it for files opened with O_DIRECT
> > which may end up getting the file into pagecache.
> > 
> > Subject: [PATCH] Open new file instance O_RDONLY to calculate hash
> > 
> > Using the open struct file to calculate the hash does not work
> > with overlayfs, because the real struct file is hidden behind the
> > overlays struct file. So, any file->f_mode manipulations are not
> > reflected on the real struct file. So, open the file again, read and
> > calculate the hash.
> > 
> > As a byproduct, we can remove all instance of f_mode manipulations
> > and can work with O_DIRECT as well.
> > 
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> By "overhead", I didn't mean it from a performance perspective, but
> was concerned about the dentry_open() failing. �If "dentry_open" fails
> for any reason, the file hash will not be re-calculated, causing
> future file opens to fail. �Missing is the test for dentry_open()
> failure.

Yes, I realized that after sending the patch. I added the check.

> 
> By removing the "read" check, re-opening the file is now for all
> files, not just those opened without "read". �From a testing
> perspective, it will catch problems faster, but ...

Okay. Modified to open a new file only if we don't have read
permissions...

> 
> I've had a patch queued that removes the O_DIRECT test, but haven't
> had a chance to test it on ALL filesystems. �I would probably re-open
> the file with the original flags, plus read, as much as possible.

Okay, I have kept the O_DIRECT for now.

I will send the updated patch after some more testing.

Thanks,

-- 
Goldwyn

Re: PROBLEM: IMA xattrs not written on overlayfs

[Goldwyn Rodrigues]

On  6:33 05/10, Mimi Zohar wrote:
> On Thu, 2018-10-04 at 21:57 -0500, Goldwyn Rodrigues wrote:
> > On 11:52 04/10, Mimi Zohar wrote:
> > > On Thu, 2018-10-04 at 00:35 +0200, Miklos Szeredi wrote:
> > > 
> > > > Right, if it's done from last fput() then it's at least not a security hole.
> > > > 
> > > > This hack may work for some filesystems, but as you noticed, it won't
> > > > work for overlayfs.  And  if probably won't work for a number of other
> > > > filesystems as well: the fs can assume that f_mode & FMODE_READ will
> > > > remain off if it was off at open time.
> > > > 
> > > > The proper way to handle it generally is to open a separate instance
> > > > of the same file from IMA with O_RDONLY and use that to calculate the
> > > > hash.   There's really no point in reusing the same file and hacking
> > > > the f_mode bits.
> > > 
> > > Is there an appropriate low level kernel call for creating a new file
> > > descriptor that would be appropriate?  dentry_open(), like the call in
> > > file_clone_open(), has a lot of overhead, including the LSM calls.
> > >  Calculating the file hash always needs to work.
> > > 
> > 
> > Mimi,
> > 
> > I have formulated a patch which is working for me on overlayfs. I am
> > using dentry_open(), which I agree may have overhead. While this
> > opens up the possibility of using it for files opened with O_DIRECT
> > which may end up getting the file into pagecache.
> > 
> > Subject: [PATCH] Open new file instance O_RDONLY to calculate hash
> > 
> > Using the open struct file to calculate the hash does not work
> > with overlayfs, because the real struct file is hidden behind the
> > overlays struct file. So, any file->f_mode manipulations are not
> > reflected on the real struct file. So, open the file again, read and
> > calculate the hash.
> > 
> > As a byproduct, we can remove all instance of f_mode manipulations
> > and can work with O_DIRECT as well.
> > 
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> By "overhead", I didn't mean it from a performance perspective, but
> was concerned about the dentry_open() failing.  If "dentry_open" fails
> for any reason, the file hash will not be re-calculated, causing
> future file opens to fail.  Missing is the test for dentry_open()
> failure.

Yes, I realized that after sending the patch. I added the check.

> 
> By removing the "read" check, re-opening the file is now for all
> files, not just those opened without "read".  From a testing
> perspective, it will catch problems faster, but ...

Okay. Modified to open a new file only if we don't have read
permissions...

> 
> I've had a patch queued that removes the O_DIRECT test, but haven't
> had a chance to test it on ALL filesystems.  I would probably re-open
> the file with the original flags, plus read, as much as possible.

Okay, I have kept the O_DIRECT for now.

I will send the updated patch after some more testing.

Thanks,

-- 
Goldwyn

Re: PROBLEM: IMA xattrs not written on overlayfs

[Goldwyn Rodrigues]

On  6:33 05/10, Mimi Zohar wrote:
> On Thu, 2018-10-04 at 21:57 -0500, Goldwyn Rodrigues wrote:
> > On 11:52 04/10, Mimi Zohar wrote:
> > > On Thu, 2018-10-04 at 00:35 +0200, Miklos Szeredi wrote:
> > > 
> > > > Right, if it's done from last fput() then it's at least not a security hole.
> > > > 
> > > > This hack may work for some filesystems, but as you noticed, it won't
> > > > work for overlayfs.  And  if probably won't work for a number of other
> > > > filesystems as well: the fs can assume that f_mode & FMODE_READ will
> > > > remain off if it was off at open time.
> > > > 
> > > > The proper way to handle it generally is to open a separate instance
> > > > of the same file from IMA with O_RDONLY and use that to calculate the
> > > > hash.   There's really no point in reusing the same file and hacking
> > > > the f_mode bits.
> > > 
> > > Is there an appropriate low level kernel call for creating a new file
> > > descriptor that would be appropriate?  dentry_open(), like the call in
> > > file_clone_open(), has a lot of overhead, including the LSM calls.
> > >  Calculating the file hash always needs to work.
> > > 
> > 
> > Mimi,
> > 
> > I have formulated a patch which is working for me on overlayfs. I am
> > using dentry_open(), which I agree may have overhead. While this
> > opens up the possibility of using it for files opened with O_DIRECT
> > which may end up getting the file into pagecache.
> > 
> > Subject: [PATCH] Open new file instance O_RDONLY to calculate hash
> > 
> > Using the open struct file to calculate the hash does not work
> > with overlayfs, because the real struct file is hidden behind the
> > overlays struct file. So, any file->f_mode manipulations are not
> > reflected on the real struct file. So, open the file again, read and
> > calculate the hash.
> > 
> > As a byproduct, we can remove all instance of f_mode manipulations
> > and can work with O_DIRECT as well.
> > 
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> By "overhead", I didn't mean it from a performance perspective, but
> was concerned about the dentry_open() failing.  If "dentry_open" fails
> for any reason, the file hash will not be re-calculated, causing
> future file opens to fail.  Missing is the test for dentry_open()
> failure.

Yes, I realized that after sending the patch. I added the check.

> 
> By removing the "read" check, re-opening the file is now for all
> files, not just those opened without "read".  From a testing
> perspective, it will catch problems faster, but ...

Okay. Modified to open a new file only if we don't have read
permissions...

> 
> I've had a patch queued that removes the O_DIRECT test, but haven't
> had a chance to test it on ALL filesystems.  I would probably re-open
> the file with the original flags, plus read, as much as possible.

Okay, I have kept the O_DIRECT for now.

I will send the updated patch after some more testing.

Thanks,

-- 
Goldwyn

Re: PROBLEM: IMA xattrs not written on overlayfs

[Amir Goldstein]

On Fri, Oct 5, 2018 at 1:34 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Thu, 2018-10-04 at 21:57 -0500, Goldwyn Rodrigues wrote:
> > On 11:52 04/10, Mimi Zohar wrote:
> > > On Thu, 2018-10-04 at 00:35 +0200, Miklos Szeredi wrote:
> > >
> > > > Right, if it's done from last fput() then it's at least not a security hole.
> > > >
> > > > This hack may work for some filesystems, but as you noticed, it won't
> > > > work for overlayfs.  And  if probably won't work for a number of other
> > > > filesystems as well: the fs can assume that f_mode & FMODE_READ will
> > > > remain off if it was off at open time.
> > > >
> > > > The proper way to handle it generally is to open a separate instance
> > > > of the same file from IMA with O_RDONLY and use that to calculate the
> > > > hash.   There's really no point in reusing the same file and hacking
> > > > the f_mode bits.
> > >
> > > Is there an appropriate low level kernel call for creating a new file
> > > descriptor that would be appropriate?  dentry_open(), like the call in
> > > file_clone_open(), has a lot of overhead, including the LSM calls.
> > >  Calculating the file hash always needs to work.
> > >
> >
> > Mimi,
> >
> > I have formulated a patch which is working for me on overlayfs. I am
> > using dentry_open(), which I agree may have overhead. While this
> > opens up the possibility of using it for files opened with O_DIRECT
> > which may end up getting the file into pagecache.
> >
> > Subject: [PATCH] Open new file instance O_RDONLY to calculate hash
> >
> > Using the open struct file to calculate the hash does not work
> > with overlayfs, because the real struct file is hidden behind the
> > overlays struct file. So, any file->f_mode manipulations are not
> > reflected on the real struct file. So, open the file again, read and
> > calculate the hash.
> >
> > As a byproduct, we can remove all instance of f_mode manipulations
> > and can work with O_DIRECT as well.
> >
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>
> By "overhead", I didn't mean it from a performance perspective, but
> was concerned about the dentry_open() failing.  If "dentry_open" fails
> for any reason, the file hash will not be re-calculated, causing
> future file opens to fail.

open_with_fake_path() looks like a better fit here.
Skips irrelevant security checks and less likely to fail.

Thanks,
Amir.

Re: PROBLEM: IMA xattrs not written on overlayfs

[Mimi Zohar]

On Sun, 2018-10-07 at 11:22 +0300, Amir Goldstein wrote:
> On Fri, Oct 5, 2018 at 1:34 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Thu, 2018-10-04 at 21:57 -0500, Goldwyn Rodrigues wrote:
> > > On 11:52 04/10, Mimi Zohar wrote:
> > > > On Thu, 2018-10-04 at 00:35 +0200, Miklos Szeredi wrote:
> > > >
> > > > > Right, if it's done from last fput() then it's at least not a security hole.
> > > > >
> > > > > This hack may work for some filesystems, but as you noticed, it won't
> > > > > work for overlayfs.  And  if probably won't work for a number of other
> > > > > filesystems as well: the fs can assume that f_mode & FMODE_READ will
> > > > > remain off if it was off at open time.
> > > > >
> > > > > The proper way to handle it generally is to open a separate instance
> > > > > of the same file from IMA with O_RDONLY and use that to calculate the
> > > > > hash.   There's really no point in reusing the same file and hacking
> > > > > the f_mode bits.
> > > >
> > > > Is there an appropriate low level kernel call for creating a new file
> > > > descriptor that would be appropriate?  dentry_open(), like the call in
> > > > file_clone_open(), has a lot of overhead, including the LSM calls.
> > > >  Calculating the file hash always needs to work.
> > > >
> > >
> > > Mimi,
> > >
> > > I have formulated a patch which is working for me on overlayfs. I am
> > > using dentry_open(), which I agree may have overhead. While this
> > > opens up the possibility of using it for files opened with O_DIRECT
> > > which may end up getting the file into pagecache.
> > >
> > > Subject: [PATCH] Open new file instance O_RDONLY to calculate hash
> > >
> > > Using the open struct file to calculate the hash does not work
> > > with overlayfs, because the real struct file is hidden behind the
> > > overlays struct file. So, any file->f_mode manipulations are not
> > > reflected on the real struct file. So, open the file again, read and
> > > calculate the hash.
> > >
> > > As a byproduct, we can remove all instance of f_mode manipulations
> > > and can work with O_DIRECT as well.
> > >
> > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >
> > By "overhead", I didn't mean it from a performance perspective, but
> > was concerned about the dentry_open() failing.  If "dentry_open" fails
> > for any reason, the file hash will not be re-calculated, causing
> > future file opens to fail.
> 
> open_with_fake_path() looks like a better fit here.
> Skips irrelevant security checks and less likely to fail.

It skips the "cred" checking, but everything else seems to be the same
including the security_ checks.

Mimi