EVM: Permission denied with overlayfs

EVM: Permission denied with overlayfs

[Ignaz Forster]

Hi,

as a follow up to my attempts to use overlayfs on an IMA protected 
system[1] I've now tried to also enable EVM. From what I understand this 
should - at least in theory - be possible: EVM will call 
d_backing_inode(dentry), which I thought would get the inode of the 
underlying file system[2], and use that for HMAC verification.

In practice simply trying to access an existing file will fail with 
"Permission denied" already. In the corresponding audit log I can see 
the file access (failed with "invalid-HMAC"), but with an inode number 
unknown to me - stat returns a completely different number for the file 
in the lower and target dir.

For testing purposes I added a new hashing algorithm to 
evm_ima_xattr_type which will not add the file system specific 
attributes (inode number, generation, file system uuid) to the hash - 
just like EVM_XATTR_PORTABLE_DIGSIG, but with the hashes generated by 
the kernel. Files created with this signature can be read correctly, 
though writing the files will still fail.

Unfortunately I'm out of ideas what is happening here. If anybody wants 
to have a look at this: Any help would be appreciated.

Kind Regards,
Ignaz

[1] https://www.spinics.net/lists/linux-integrity/msg03593.html
[2] https://www.kernel.org/doc/htmldocs/filesystems/API-d-backing-inode.html

Re: EVM: Permission denied with overlayfs

[Mimi Zohar]

Hi Ignaz,

On Tue, 2018-12-18 at 20:49 +0100, Ignaz Forster wrote:
> Hi,
> 
> as a follow up to my attempts to use overlayfs on an IMA protected 
> system[1] I've now tried to also enable EVM. From what I understand this 
> should - at least in theory - be possible: EVM will call 
> d_backing_inode(dentry), which I thought would get the inode of the 
> underlying file system[2], and use that for HMAC verification.
> 
> In practice simply trying to access an existing file will fail with 
> "Permission denied" already. In the corresponding audit log I can see 
> the file access (failed with "invalid-HMAC"), but with an inode number 
> unknown to me - stat returns a completely different number for the file 
> in the lower and target dir.
> 
> For testing purposes I added a new hashing algorithm to 
> evm_ima_xattr_type which will not add the file system specific 
> attributes (inode number, generation, file system uuid) to the hash - 
> just like EVM_XATTR_PORTABLE_DIGSIG, but with the hashes generated by 
> the kernel. Files created with this signature can be read correctly, 
> though writing the files will still fail.
> 
> Unfortunately I'm out of ideas what is happening here. If anybody wants 
> to have a look at this: Any help would be appreciated.
> 
> Kind Regards,
> Ignaz
> 
> [1] https://www.spinics.net/lists/linux-integrity/msg03593.html
> [2] https://www.kernel.org/doc/htmldocs/filesystems/API-d-backing-inode.html
> 

After creating a file on the overlay, I wasn't able to access it from
the overlay, but was able to access it from "upper".  Both "stat" and
"getfattr -m ^security" returned exactly the same things for both
pathnames.  However, the ino in the audit log was different.

After modifying evm_calc_hmac_or_hash(), replacing d_backing_inode()
with d_real_inode(), the hmac properly calculated for both the overlay
and the upper pathnames.

Something must have changed in d_backing_inode().

Mimi

Re: EVM: Permission denied with overlayfs

[Mimi Zohar]

On Tue, 2018-12-18 at 18:00 -0500, Mimi Zohar wrote:
> Hi Ignaz,
> 
> On Tue, 2018-12-18 at 20:49 +0100, Ignaz Forster wrote:
> > Hi,
> > 
> > as a follow up to my attempts to use overlayfs on an IMA protected 
> > system[1] I've now tried to also enable EVM. From what I understand this 
> > should - at least in theory - be possible: EVM will call 
> > d_backing_inode(dentry), which I thought would get the inode of the 
> > underlying file system[2], and use that for HMAC verification.
> > 
> > In practice simply trying to access an existing file will fail with 
> > "Permission denied" already. In the corresponding audit log I can see 
> > the file access (failed with "invalid-HMAC"), but with an inode number 
> > unknown to me - stat returns a completely different number for the file 
> > in the lower and target dir.
> > 
> > For testing purposes I added a new hashing algorithm to 
> > evm_ima_xattr_type which will not add the file system specific 
> > attributes (inode number, generation, file system uuid) to the hash - 
> > just like EVM_XATTR_PORTABLE_DIGSIG, but with the hashes generated by 
> > the kernel. Files created with this signature can be read correctly, 
> > though writing the files will still fail.
> > 
> > Unfortunately I'm out of ideas what is happening here. If anybody wants 
> > to have a look at this: Any help would be appreciated.
> > 
> > Kind Regards,
> > Ignaz
> > 
> > [1] https://www.spinics.net/lists/linux-integrity/msg03593.html
> > [2] https://www.kernel.org/doc/htmldocs/filesystems/API-d-backing-inode.html
> > 
> 
> After creating a file on the overlay, I wasn't able to access it from
> the overlay, but was able to access it from "upper".  Both "stat" and
> "getfattr -m ^security" returned exactly the same things for both
> pathnames.  However, the ino in the audit log was different.
> 
> After modifying evm_calc_hmac_or_hash(), replacing d_backing_inode()
> with d_real_inode(), the hmac properly calculated for both the overlay
> and the upper pathnames.
> 
> Something must have changed in d_backing_inode().

Confirmed, in linux-4.18.y d_backing_inode returns the real i_ino, but
newer kernels do not.  This is a problem for EVM as the i_ino is
included in the HMAC calculation.

Mimi

Re: EVM: Permission denied with overlayfs

[Amir Goldstein]

On Wed, Dec 19, 2018 at 5:39 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Tue, 2018-12-18 at 18:00 -0500, Mimi Zohar wrote:
> > Hi Ignaz,
> >
> > On Tue, 2018-12-18 at 20:49 +0100, Ignaz Forster wrote:
> > > Hi,
> > >
> > > as a follow up to my attempts to use overlayfs on an IMA protected
> > > system[1] I've now tried to also enable EVM. From what I understand this
> > > should - at least in theory - be possible: EVM will call
> > > d_backing_inode(dentry), which I thought would get the inode of the
> > > underlying file system[2], and use that for HMAC verification.
> > >
> > > In practice simply trying to access an existing file will fail with
> > > "Permission denied" already. In the corresponding audit log I can see
> > > the file access (failed with "invalid-HMAC"), but with an inode number
> > > unknown to me - stat returns a completely different number for the file
> > > in the lower and target dir.
> > >
> > > For testing purposes I added a new hashing algorithm to
> > > evm_ima_xattr_type which will not add the file system specific
> > > attributes (inode number, generation, file system uuid) to the hash -
> > > just like EVM_XATTR_PORTABLE_DIGSIG, but with the hashes generated by
> > > the kernel. Files created with this signature can be read correctly,
> > > though writing the files will still fail.
> > >
> > > Unfortunately I'm out of ideas what is happening here. If anybody wants
> > > to have a look at this: Any help would be appreciated.
> > >
> > > Kind Regards,
> > > Ignaz
> > >
> > > [1] https://www.spinics.net/lists/linux-integrity/msg03593.html
> > > [2] https://www.kernel.org/doc/htmldocs/filesystems/API-d-backing-inode.html
> > >
> >
> > After creating a file on the overlay, I wasn't able to access it from
> > the overlay, but was able to access it from "upper".  Both "stat" and
> > "getfattr -m ^security" returned exactly the same things for both
> > pathnames.  However, the ino in the audit log was different.
> >
> > After modifying evm_calc_hmac_or_hash(), replacing d_backing_inode()
> > with d_real_inode(), the hmac properly calculated for both the overlay
> > and the upper pathnames.
> >
> > Something must have changed in d_backing_inode().
>
> Confirmed, in linux-4.18.y d_backing_inode returns the real i_ino, but
> newer kernels do not.  This is a problem for EVM as the i_ino is
> included in the HMAC calculation.
>

Hi Mimi,

v4.19 has a big change that removes many VFS hacks in favor of
overlayfs stacked file operations.

The major implication for VFS code is that file_inode(file) is now the overlayfs
inode and not the real inode. Therefore, file_dentry(file) is also the overlayfs
dentry and not the real dentry.

I am not sure how that change impacts EVM ?
FWIW, d_backing_inode(dentry) was never anything more than d_inode(dentry).

Can you please try to describe in more details for someone who has no clue what
EVM does how exactly the v4.19 change is manifested in the EVM use case.

AFAIKT, evm_calc_hmac_or_hash() would get the overlayfs dentry both in
v4.18 and v4.19 and therefore d_backing_inode(dentry) should be the
overlayfs inode in both kernels (?).

The value of overlayfs inode i_ino can be identical to i_ino of the real inode
under some conditions, but far from always and the value of overlayfs inode
i_generation is almost guaranteed to not match that of the real inode.

Ignaz, can you add some more debug prints to shed some light on what
exactly has changed, between the two kernels?
If the calculated hashes do not match in two different execution paths,
please provide two sample stack traces that see different i_ino, so we can
examine them.

Thanks,
Amir.

Re: EVM: Permission denied with overlayfs

[James Bottomley]

On Wed, 2018-12-19 at 10:39 -0500, Mimi Zohar wrote:
> Confirmed, in linux-4.18.y d_backing_inode returns the real i_ino,
> but newer kernels do not.

Just so we're clear, this isn't an issue with d_backing_inode(), which
hasn't changed since its introduction in 2015 and which always returns
dentry->d_inode (it was originally a helper for unionfs which got
merged even though unionfs didn't, which makes it and the comment about
upper/lower totally misleading).  The problem is that overlayfs has
changed the inode it places into d_inode.

>   This is a problem for EVM as the i_ino is included in the HMAC
> calculation.

Isn't the solution always to use portable signatures for containers? 
It's problematic to include inode and generation with an overlay
because if you change the metadata it gets copied up => new inode
number and generation on the upper filesystem but if we were always
using the underlying inode number and generation, the signature would
then be wrong on the copied up file.

At base, most container images are sets of tar files, which are not
inode number preserving anyway, so even if we find a convoluted way to
fix the above, the EVM signature has to be portable because otherwise
its always wrong for container images.

James

Re: EVM: Permission denied with overlayfs

[Mimi Zohar]

On Wed, 2018-12-19 at 08:56 -0800, James Bottomley wrote:
> On Wed, 2018-12-19 at 10:39 -0500, Mimi Zohar wrote:
> > Confirmed, in linux-4.18.y d_backing_inode returns the real i_ino,
> > but newer kernels do not.
> 
> Just so we're clear, this isn't an issue with d_backing_inode(), which
> hasn't changed since its introduction in 2015 and which always returns
> dentry->d_inode (it was originally a helper for unionfs which got
> merged even though unionfs didn't, which makes it and the comment about
> upper/lower totally misleading).  The problem is that overlayfs has
> changed the inode it places into d_inode.
> 
> >   This is a problem for EVM as the i_ino is included in the HMAC
> > calculation.
> 
> Isn't the solution always to use portable signatures for containers? 
> It's problematic to include inode and generation with an overlay
> because if you change the metadata it gets copied up => new inode
> number and generation on the upper filesystem but if we were always
> using the underlying inode number and generation, the signature would
> then be wrong on the copied up file.
> 
> At base, most container images are sets of tar files, which are not
> inode number preserving anyway, so even if we find a convoluted way to
> fix the above, the EVM signature has to be portable because otherwise
> its always wrong for container images.

Ignaz's use case was mutable files, not immutable files with file
signatures.  Prior to 4.19, EVM was calculating and verifying the file
HMAC properly.  With 4.19, it stopped working because the i_ino used
in calculating the HMAC value stored in security.evm, is not the same
 when verifying the HMAC value.

Mimi

Re: EVM: Permission denied with overlayfs

[Mimi Zohar]

On Wed, 2018-12-19 at 18:38 +0200, Amir Goldstein wrote:
> On Wed, Dec 19, 2018 at 5:39 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Tue, 2018-12-18 at 18:00 -0500, Mimi Zohar wrote:
> > > Hi Ignaz,
> > >
> > > On Tue, 2018-12-18 at 20:49 +0100, Ignaz Forster wrote:
> > > > Hi,
> > > >
> > > > as a follow up to my attempts to use overlayfs on an IMA protected
> > > > system[1] I've now tried to also enable EVM. From what I understand this
> > > > should - at least in theory - be possible: EVM will call
> > > > d_backing_inode(dentry), which I thought would get the inode of the
> > > > underlying file system[2], and use that for HMAC verification.
> > > >
> > > > In practice simply trying to access an existing file will fail with
> > > > "Permission denied" already. In the corresponding audit log I can see
> > > > the file access (failed with "invalid-HMAC"), but with an inode number
> > > > unknown to me - stat returns a completely different number for the file
> > > > in the lower and target dir.
> > > >
> > > > For testing purposes I added a new hashing algorithm to
> > > > evm_ima_xattr_type which will not add the file system specific
> > > > attributes (inode number, generation, file system uuid) to the hash -
> > > > just like EVM_XATTR_PORTABLE_DIGSIG, but with the hashes generated by
> > > > the kernel. Files created with this signature can be read correctly,
> > > > though writing the files will still fail.
> > > >
> > > > Unfortunately I'm out of ideas what is happening here. If anybody wants
> > > > to have a look at this: Any help would be appreciated.
> > > >
> > > > Kind Regards,
> > > > Ignaz
> > > >
> > > > [1] https://www.spinics.net/lists/linux-integrity/msg03593.html
> > > > [2] https://www.kernel.org/doc/htmldocs/filesystems/API-d-backing-inode.html
> > > >
> > >
> > > After creating a file on the overlay, I wasn't able to access it from
> > > the overlay, but was able to access it from "upper".  Both "stat" and
> > > "getfattr -m ^security" returned exactly the same things for both
> > > pathnames.  However, the ino in the audit log was different.
> > >
> > > After modifying evm_calc_hmac_or_hash(), replacing d_backing_inode()
> > > with d_real_inode(), the hmac properly calculated for both the overlay
> > > and the upper pathnames.
> > >
> > > Something must have changed in d_backing_inode().
> >
> > Confirmed, in linux-4.18.y d_backing_inode returns the real i_ino, but
> > newer kernels do not.  This is a problem for EVM as the i_ino is
> > included in the HMAC calculation.
> >
> 
> Hi Mimi,
> 
> v4.19 has a big change that removes many VFS hacks in favor of
> overlayfs stacked file operations.
> 
> The major implication for VFS code is that file_inode(file) is now the overlayfs
> inode and not the real inode. Therefore, file_dentry(file) is also the overlayfs
> dentry and not the real dentry.
> 
> I am not sure how that change impacts EVM ?
> FWIW, d_backing_inode(dentry) was never anything more than d_inode(dentry).
> 
> Can you please try to describe in more details for someone who has no clue what
> EVM does how exactly the v4.19 change is manifested in the EVM use case.

IMA calculates and stores a file hash/signature on the file data
(security.ima).  EVM calculates and stores an HMAC/signature on the
file metadata (security.evm).  Some data needs to be included in the
HMAC/signature that binds the file metadata with the file data.  That
data is the inode's ino, generation, uid, gid, mode and the uuid.

> 
> AFAIKT, evm_calc_hmac_or_hash() would get the overlayfs dentry both in
> v4.18 and v4.19 and therefore d_backing_inode(dentry) should be the
> overlayfs inode in both kernels (?).
> 
> The value of overlayfs inode i_ino can be identical to i_ino of the real inode
> under some conditions, but far from always and the value of overlayfs inode
> i_generation is almost guaranteed to not match that of the real inode.
> 
> Ignaz, can you add some more debug prints to shed some light on what
> exactly has changed, between the two kernels?
> If the calculated hashes do not match in two different execution paths,
> please provide two sample stack traces that see different i_ino, so we can
> examine them.

Assuming you've created and overlay mounted the lower, upper, work,
and merged directories, accessing files only in the merged directory
fails.

diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index 4f9126ebfbf4..d0ffa08d4b23 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -193,6 +193,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
 				 uint8_t type, struct evm_digest *data)
 {
 	struct inode *inode = d_backing_inode(dentry);
+	struct inode *inode1 = d_real_inode(dentry);
 	struct xattr_list *xattr;
 	struct shash_desc *desc;
 	size_t xattr_size = 0;
@@ -241,6 +242,9 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
 		if (is_ima)
 			ima_present = true;
 	}
+	if (inode != inode1)
+		pr_info("ino: %lu %lu %lu %s\n", inode->i_ino, inode1->i_ino,
+				dentry->d_inode->i_ino, dentry->d_name.name);
 	hmac_add_misc(desc, inode, type, data->digest);
 
 	/* Portable EVM signatures must include an IMA hash */
-- 

Mimi

Re: EVM: Permission denied with overlayfs

[James Bottomley]

On Wed, 2018-12-19 at 13:15 -0500, Mimi Zohar wrote:
> On Wed, 2018-12-19 at 08:56 -0800, James Bottomley wrote:
> > On Wed, 2018-12-19 at 10:39 -0500, Mimi Zohar wrote:
> > > Confirmed, in linux-4.18.y d_backing_inode returns the real
> > > i_ino, but newer kernels do not.
> > 
> > Just so we're clear, this isn't an issue with d_backing_inode(),
> > which hasn't changed since its introduction in 2015 and which
> > always returns dentry->d_inode (it was originally a helper for
> > unionfs which got merged even though unionfs didn't, which makes it
> > and the comment about upper/lower totally misleading).  The problem
> > is that overlayfs has changed the inode it places into d_inode.
> > 
> > >   This is a problem for EVM as the i_ino is included in the HMAC
> > > calculation.
> > 
> > Isn't the solution always to use portable signatures for
> > containers? 
> > It's problematic to include inode and generation with an overlay
> > because if you change the metadata it gets copied up => new inode
> > number and generation on the upper filesystem but if we were always
> > using the underlying inode number and generation, the signature
> > would then be wrong on the copied up file.
> > 
> > At base, most container images are sets of tar files, which are not
> > inode number preserving anyway, so even if we find a convoluted way
> > to fix the above, the EVM signature has to be portable because
> > otherwise its always wrong for container images.
> 
> Ignaz's use case was mutable files, not immutable files with file
> signatures.

The word "mutable" is problematic in terms of overlays.  Only the upper
layer is mutable, so if your EVM signed file is anywhere other than in
the top layer it's technically immutable.  What you get when you mutate
it is a copy up.  the VFS guarantee is that inode numbers are stable
only for the current mount and may change on a remount.  Most disk
backed filesystems have inode numbers encoded in their on disk inodes,
which is why they have far more stability than the simple VFS
requirement, but some filesystems can't have long term stable inode
numbers.  We recognise this problem in EVM with so called "portable
signatures" that don't include the inode number and generation.

I'm interested in discussing two points

   1. Can we actually come up with semantics where the EVM signature is
      always valid for overlays?
   2. If the answer to 1. is "no" should we not always use portable
      signatures for overlay cases?

I think there are a load of nasty corner cases in 1. that make 2. far
more preferable, but perhaps we can begin with the use case that
requires non-portable signatures with overlays, because I don't think
I've heard it yet.

>   Prior to 4.19, EVM was calculating and verifying the file
> HMAC properly.  With 4.19, it stopped working because the i_ino used
> in calculating the HMAC value stored in security.evm, is not the same
>  when verifying the HMAC value.

Yes, but the same thing would happen on cifs or NFS or any other
filesystem that constructs inode numbers on the fly.  If we're not
proposing trying to fix them, why fix overlayfs?

James

Re: EVM: Permission denied with overlayfs

[Amir Goldstein]

On Wed, Dec 19, 2018 at 9:34 PM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Wed, 2018-12-19 at 13:15 -0500, Mimi Zohar wrote:
> > On Wed, 2018-12-19 at 08:56 -0800, James Bottomley wrote:
> > > On Wed, 2018-12-19 at 10:39 -0500, Mimi Zohar wrote:
> > > > Confirmed, in linux-4.18.y d_backing_inode returns the real
> > > > i_ino, but newer kernels do not.
> > >
> > > Just so we're clear, this isn't an issue with d_backing_inode(),
> > > which hasn't changed since its introduction in 2015 and which
> > > always returns dentry->d_inode (it was originally a helper for
> > > unionfs which got merged even though unionfs didn't, which makes it
> > > and the comment about upper/lower totally misleading).  The problem
> > > is that overlayfs has changed the inode it places into d_inode.
> > >
> > > >   This is a problem for EVM as the i_ino is included in the HMAC
> > > > calculation.
> > >
> > > Isn't the solution always to use portable signatures for
> > > containers?
> > > It's problematic to include inode and generation with an overlay
> > > because if you change the metadata it gets copied up => new inode
> > > number and generation on the upper filesystem but if we were always
> > > using the underlying inode number and generation, the signature
> > > would then be wrong on the copied up file.
> > >
> > > At base, most container images are sets of tar files, which are not
> > > inode number preserving anyway, so even if we find a convoluted way
> > > to fix the above, the EVM signature has to be portable because
> > > otherwise its always wrong for container images.
> >
> > Ignaz's use case was mutable files, not immutable files with file
> > signatures.
>
> The word "mutable" is problematic in terms of overlays.  Only the upper
> layer is mutable, so if your EVM signed file is anywhere other than in
> the top layer it's technically immutable.  What you get when you mutate
> it is a copy up.  the VFS guarantee is that inode numbers are stable
> only for the current mount and may change on a remount.  Most disk
> backed filesystems have inode numbers encoded in their on disk inodes,
> which is why they have far more stability than the simple VFS
> requirement, but some filesystems can't have long term stable inode
> numbers.  We recognise this problem in EVM with so called "portable
> signatures" that don't include the inode number and generation.
>
> I'm interested in discussing two points
>
>    1. Can we actually come up with semantics where the EVM signature is
>       always valid for overlays?

What you are looking for is a persistent and stable identifier for
the inode object. Such a property already exists for filesystems that
support NFS file handles.

As I wrote, you can *sometimes* rely on overlay i_ino to be consistent
and stable (it will usually have the value of the pre copy up lower inode
even after copy up), bug i_generation is nonsense.

You can *always* rely on overlay file handle to be a unique indetifier
of the inode (even after mount cycle) as long as nfs_export=on and
overlayfs supports NFS file handles (as well as the underlying layers).


>    2. If the answer to 1. is "no" should we not always use portable
>       signatures for overlay cases?

You can determine if overlayfs has exportfs ops and choose portable
non-portable signatures per instance.

>
> I think there are a load of nasty corner cases in 1. that make 2. far
> more preferable, but perhaps we can begin with the use case that
> requires non-portable signatures with overlays, because I don't think
> I've heard it yet.
>
> >   Prior to 4.19, EVM was calculating and verifying the file
> > HMAC properly.  With 4.19, it stopped working because the i_ino used
> > in calculating the HMAC value stored in security.evm, is not the same
> >  when verifying the HMAC value.
>
> Yes, but the same thing would happen on cifs or NFS or any other
> filesystem that constructs inode numbers on the fly.  If we're not
> proposing trying to fix them, why fix overlayfs?
>

NFS file handles as inode identifier would work for those as well.

Thanks,
Amir.

Re: EVM: Permission denied with overlayfs

[Amir Goldstein]

> > Hi Mimi,
> >
> > v4.19 has a big change that removes many VFS hacks in favor of
> > overlayfs stacked file operations.
> >
> > The major implication for VFS code is that file_inode(file) is now the overlayfs
> > inode and not the real inode. Therefore, file_dentry(file) is also the overlayfs
> > dentry and not the real dentry.
> >
> > I am not sure how that change impacts EVM ?
> > FWIW, d_backing_inode(dentry) was never anything more than d_inode(dentry).
> >
> > Can you please try to describe in more details for someone who has no clue what
> > EVM does how exactly the v4.19 change is manifested in the EVM use case.
>
> IMA calculates and stores a file hash/signature on the file data
> (security.ima).  EVM calculates and stores an HMAC/signature on the
> file metadata (security.evm).  Some data needs to be included in the
> HMAC/signature that binds the file metadata with the file data.  That
> data is the inode's ino, generation, uid, gid, mode and the uuid.
>
> >
> > AFAIKT, evm_calc_hmac_or_hash() would get the overlayfs dentry both in
> > v4.18 and v4.19 and therefore d_backing_inode(dentry) should be the
> > overlayfs inode in both kernels (?).
> >
> > The value of overlayfs inode i_ino can be identical to i_ino of the real inode
> > under some conditions, but far from always and the value of overlayfs inode
> > i_generation is almost guaranteed to not match that of the real inode.
> >
> > Ignaz, can you add some more debug prints to shed some light on what
> > exactly has changed, between the two kernels?
> > If the calculated hashes do not match in two different execution paths,
> > please provide two sample stack traces that see different i_ino, so we can
> > examine them.
>
> Assuming you've created and overlay mounted the lower, upper, work,
> and merged directories, accessing files only in the merged directory
> fails.
>

Mimi,

I think I understand the problem I just don't see how that use case could have
ever worked with overlayfs.
The following patch may make your test pass,
but #1: I am not sure it is safe to do that without ovl_index_all() -
need to think about implications
but #2: For some overlayfs setups that won't be enough to
guaranty consistent i_ino
but #3: Not sure about the role of i_generation

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 30ef69715a81..8d8cd6acd71e 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -560,8 +560,7 @@ static void ovl_fill_inode(struct inode *inode,
umode_t mode, dev_t rdev,
         * ovl_new_inode(), ino arg is 0, so i_ino will be updated to real
         * upper inode i_ino on ovl_inode_init() or ovl_inode_update().
         */
-       if (inode->i_sb->s_export_op &&
-           (ovl_same_sb(inode->i_sb) || xinobits)) {
+       if ((ovl_same_sb(inode->i_sb) || xinobits)) {
                inode->i_ino = ino;
                if (xinobits && fsid && !(ino >> (64 - xinobits)))
                        inode->i_ino |= (unsigned long)fsid << (64 - xinobits);

Thanks,
Amir.

Re: EVM: Permission denied with overlayfs

[Mimi Zohar]

On Wed, 2018-12-19 at 22:12 +0200, Amir Goldstein wrote:
> On Wed, Dec 19, 2018 at 9:34 PM James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> >
> > On Wed, 2018-12-19 at 13:15 -0500, Mimi Zohar wrote:
> > > On Wed, 2018-12-19 at 08:56 -0800, James Bottomley wrote:
> > > > On Wed, 2018-12-19 at 10:39 -0500, Mimi Zohar wrote:
> > > > > Confirmed, in linux-4.18.y d_backing_inode returns the real
> > > > > i_ino, but newer kernels do not.
> > > >
> > > > Just so we're clear, this isn't an issue with d_backing_inode(),
> > > > which hasn't changed since its introduction in 2015 and which
> > > > always returns dentry->d_inode (it was originally a helper for
> > > > unionfs which got merged even though unionfs didn't, which makes it
> > > > and the comment about upper/lower totally misleading).  The problem
> > > > is that overlayfs has changed the inode it places into d_inode.
> > > >
> > > > >   This is a problem for EVM as the i_ino is included in the HMAC
> > > > > calculation.
> > > >
> > > > Isn't the solution always to use portable signatures for
> > > > containers?
> > > > It's problematic to include inode and generation with an overlay
> > > > because if you change the metadata it gets copied up => new inode
> > > > number and generation on the upper filesystem but if we were always
> > > > using the underlying inode number and generation, the signature
> > > > would then be wrong on the copied up file.
> > > >
> > > > At base, most container images are sets of tar files, which are not
> > > > inode number preserving anyway, so even if we find a convoluted way
> > > > to fix the above, the EVM signature has to be portable because
> > > > otherwise its always wrong for container images.
> > >
> > > Ignaz's use case was mutable files, not immutable files with file
> > > signatures.
> >
> > The word "mutable" is problematic in terms of overlays.  Only the upper
> > layer is mutable, so if your EVM signed file is anywhere other than in
> > the top layer it's technically immutable.  What you get when you mutate
> > it is a copy up.  the VFS guarantee is that inode numbers are stable
> > only for the current mount and may change on a remount.  Most disk
> > backed filesystems have inode numbers encoded in their on disk inodes,
> > which is why they have far more stability than the simple VFS
> > requirement, but some filesystems can't have long term stable inode
> > numbers.  We recognise this problem in EVM with so called "portable
> > signatures" that don't include the inode number and generation.

For portable signatures, to bind the file metadata with the file data,
we've replaced the inode number and generation, with the
"security.ima" xattr.  Do we want this requirement/limitation for
overlays?

The existing EVM portable signature is an asymmetric algorithm based
signature.  Would we define a "portable" HMAC?

> > I'm interested in discussing two points
> >
> >    1. Can we actually come up with semantics where the EVM signature is
> >       always valid for overlays?
> 
> What you are looking for is a persistent and stable identifier for
> the inode object. Such a property already exists for filesystems that
> support NFS file handles.
> 
> As I wrote, you can *sometimes* rely on overlay i_ino to be consistent
> and stable (it will usually have the value of the pre copy up lower inode
> even after copy up), bug i_generation is nonsense.
> 
> You can *always* rely on overlay file handle to be a unique indetifier
> of the inode (even after mount cycle) as long as nfs_export=on and
> overlayfs supports NFS file handles (as well as the underlying layers).
> 
> 
> >    2. If the answer to 1. is "no" should we not always use portable
> >       signatures for overlay cases?
> 
> You can determine if overlayfs has exportfs ops and choose portable
> non-portable signatures per instance.
> 
> >
> > I think there are a load of nasty corner cases in 1. that make 2. far
> > more preferable, but perhaps we can begin with the use case that
> > requires non-portable signatures with overlays, because I don't think
> > I've heard it yet.
> >
> > >   Prior to 4.19, EVM was calculating and verifying the file
> > > HMAC properly.  With 4.19, it stopped working because the i_ino used
> > > in calculating the HMAC value stored in security.evm, is not the same
> > >  when verifying the HMAC value.
> >
> > Yes, but the same thing would happen on cifs or NFS or any other
> > filesystem that constructs inode numbers on the fly.  If we're not
> > proposing trying to fix them, why fix overlayfs?

Both EVM and IMA have been limited to local filesystems, originally
dependent on i_version.  Only recently has there been interest in
remote file systems.  There's a lot more than just adding a portable
EVM format to support remote filesystems.

Mimi
> 
> NFS file handles as inode identifier would work for those as well.
> 
> Thanks,
> Amir.
> 

Re: EVM: Permission denied with overlayfs

[James Bottomley]

On Wed, 2018-12-19 at 16:02 -0500, Mimi Zohar wrote:
> On Wed, 2018-12-19 at 22:12 +0200, Amir Goldstein wrote:
> > On Wed, Dec 19, 2018 at 9:34 PM James Bottomley
> > <James.Bottomley@hansenpartnership.com> wrote:
> > > 
> > > On Wed, 2018-12-19 at 13:15 -0500, Mimi Zohar wrote:
> > > > On Wed, 2018-12-19 at 08:56 -0800, James Bottomley wrote:
> > > > > On Wed, 2018-12-19 at 10:39 -0500, Mimi Zohar wrote:
> > > > > > Confirmed, in linux-4.18.y d_backing_inode returns the real
> > > > > > i_ino, but newer kernels do not.
> > > > > 
> > > > > Just so we're clear, this isn't an issue with
> > > > > d_backing_inode(), which hasn't changed since its
> > > > > introduction in 2015 and which always returns dentry->d_inode 
> > > > > (it was originally a helper for unionfs which got merged even
> > > > > though unionfs didn't, which makes it and the comment about
> > > > > upper/lower totally misleading).  The problem is that
> > > > > overlayfs has changed the inode it places into d_inode.
> > > > > 
> > > > > >   This is a problem for EVM as the i_ino is included in the
> > > > > > HMAC calculation.
> > > > > 
> > > > > Isn't the solution always to use portable signatures for
> > > > > containers? It's problematic to include inode and generation
> > > > > with an overlay because if you change the metadata it gets
> > > > > copied up => new inode number and generation on the upper
> > > > > filesystem but if we were always using the underlying inode
> > > > > number and generation, the signature would then be wrong on
> > > > > the copied up file.
> > > > > 
> > > > > At base, most container images are sets of tar files, which
> > > > > are not inode number preserving anyway, so even if we find a
> > > > > convoluted way to fix the above, the EVM signature has to be
> > > > > portable because otherwise its always wrong for container
> > > > > images.
> > > > 
> > > > Ignaz's use case was mutable files, not immutable files with
> > > > file signatures.
> > > 
> > > The word "mutable" is problematic in terms of overlays.  Only the
> > > upper layer is mutable, so if your EVM signed file is anywhere
> > > other than in the top layer it's technically immutable.  What you
> > > get when you mutate it is a copy up.  the VFS guarantee is that
> > > inode numbers are stable only for the current mount and may
> > > change on a remount.  Most disk backed filesystems have inode
> > > numbers encoded in their on disk inodes, which is why they have
> > > far more stability than the simple VFS requirement, but some
> > > filesystems can't have long term stable inode numbers.  We
> > > recognise this problem in EVM with so called "portable
> > > signatures" that don't include the inode number and generation.
> 
> For portable signatures, to bind the file metadata with the file 
> data, we've replaced the inode number and generation, with the
> "security.ima" xattr.  Do we want this requirement/limitation for
> overlays?

Well, that's my question, yes.  I think there's a reasonable case for
it, but I was wondering what value the inode number and generation
brings.  Is there some reason to bind the EVM signature to a more
mutable file container (which is what inum/generation provide) rather
than a hard hash of file content (which is what the ima xattr
provides)?

> The existing EVM portable signature is an asymmetric algorithm based
> signature.  Would we define a "portable" HMAC?

Well, a signature is just an encryption of a hash.  Whether you do HMAC
with symmetric key or RSA/EC with an asymmetric one is more an
operational question.  HMAC is certainly much faster but EVM only has a
single hmac key which is problematic for the containers.  Without a use
case I can't really say.  Instinct tells me asymmetric is more suitable
to the container use case, but that's really just a guess.

James

Re: EVM: Permission denied with overlayfs

[James Bottomley]

On Wed, 2018-12-19 at 22:12 +0200, Amir Goldstein wrote:
> On Wed, Dec 19, 2018 at 9:34 PM James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > 
> > On Wed, 2018-12-19 at 13:15 -0500, Mimi Zohar wrote:
> > > On Wed, 2018-12-19 at 08:56 -0800, James Bottomley wrote:
> > > > On Wed, 2018-12-19 at 10:39 -0500, Mimi Zohar wrote:
> > > > > Confirmed, in linux-4.18.y d_backing_inode returns the real
> > > > > i_ino, but newer kernels do not.
> > > > 
> > > > Just so we're clear, this isn't an issue with
> > > > d_backing_inode(), which hasn't changed since its introduction
> > > > in 2015 and which always returns dentry->d_inode (it was
> > > > originally a helper for unionfs which got merged even though
> > > > unionfs didn't, which makes it and the comment about
> > > > upper/lower totally misleading).  The problem is that overlayfs
> > > > has changed the inode it places into d_inode.
> > > > 
> > > > >   This is a problem for EVM as the i_ino is included in the
> > > > > HMAC calculation.
> > > > 
> > > > Isn't the solution always to use portable signatures for
> > > > containers? It's problematic to include inode and generation
> > > > with an overlay because if you change the metadata it gets
> > > > copied up => new inode number and generation on the upper
> > > > filesystem but if we were always using the underlying inode
> > > > number and generation, the signature would then be wrong on the
> > > > copied up file.
> > > > 
> > > > At base, most container images are sets of tar files, which are
> > > > not inode number preserving anyway, so even if we find a
> > > > convoluted way to fix the above, the EVM signature has to be
> > > > portable because otherwise its always wrong for container
> > > > images.
> > > 
> > > Ignaz's use case was mutable files, not immutable files with file
> > > signatures.
> > 
> > The word "mutable" is problematic in terms of overlays.  Only the
> > upper layer is mutable, so if your EVM signed file is anywhere
> > other than in the top layer it's technically immutable.  What you
> > get when you mutate it is a copy up.  the VFS guarantee is that
> > inode numbers are stable only for the current mount and may change
> > on a remount.  Most disk backed filesystems have inode numbers
> > encoded in their on disk inodes, which is why they have far more
> > stability than the simple VFS requirement, but some filesystems
> > can't have long term stable inode numbers.  We recognise this
> > problem in EVM with so called "portable signatures" that don't
> > include the inode number and generation.
> > 
> > I'm interested in discussing two points
> > 
> >    1. Can we actually come up with semantics where the EVM
> > signature is
> >       always valid for overlays?
> 
> What you are looking for is a persistent and stable identifier for
> the inode object. Such a property already exists for filesystems that
> support NFS file handles.

Well, that's really what I'm asking.  I know if we want container
rather than content then filehandle is definitely the way to go for the
same reasons that NFS transitioned away from inode numbers.  However,
I'm asking the question what value is there in hashing a container
identifier instead of the real file contents?

James

Re: EVM: Permission denied with overlayfs

[Goldwyn Rodrigues]

On 18:38 19/12, Amir Goldstein wrote:
> On Wed, Dec 19, 2018 at 5:39 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Tue, 2018-12-18 at 18:00 -0500, Mimi Zohar wrote:
> > > Hi Ignaz,
> > >
> > > On Tue, 2018-12-18 at 20:49 +0100, Ignaz Forster wrote:
> > > > Hi,
> > > >
> > > > as a follow up to my attempts to use overlayfs on an IMA protected
> > > > system[1] I've now tried to also enable EVM. From what I understand this
> > > > should - at least in theory - be possible: EVM will call
> > > > d_backing_inode(dentry), which I thought would get the inode of the
> > > > underlying file system[2], and use that for HMAC verification.
> > > >
> > > > In practice simply trying to access an existing file will fail with
> > > > "Permission denied" already. In the corresponding audit log I can see
> > > > the file access (failed with "invalid-HMAC"), but with an inode number
> > > > unknown to me - stat returns a completely different number for the file
> > > > in the lower and target dir.
> > > >
> > > > For testing purposes I added a new hashing algorithm to
> > > > evm_ima_xattr_type which will not add the file system specific
> > > > attributes (inode number, generation, file system uuid) to the hash -
> > > > just like EVM_XATTR_PORTABLE_DIGSIG, but with the hashes generated by
> > > > the kernel. Files created with this signature can be read correctly,
> > > > though writing the files will still fail.
> > > >
> > > > Unfortunately I'm out of ideas what is happening here. If anybody wants
> > > > to have a look at this: Any help would be appreciated.
> > > >
> > > > Kind Regards,
> > > > Ignaz
> > > >
> > > > [1] https://www.spinics.net/lists/linux-integrity/msg03593.html
> > > > [2] https://www.kernel.org/doc/htmldocs/filesystems/API-d-backing-inode.html
> > > >
> > >
> > > After creating a file on the overlay, I wasn't able to access it from
> > > the overlay, but was able to access it from "upper".  Both "stat" and
> > > "getfattr -m ^security" returned exactly the same things for both
> > > pathnames.  However, the ino in the audit log was different.
> > >
> > > After modifying evm_calc_hmac_or_hash(), replacing d_backing_inode()
> > > with d_real_inode(), the hmac properly calculated for both the overlay
> > > and the upper pathnames.
> > >
> > > Something must have changed in d_backing_inode().
> >
> > Confirmed, in linux-4.18.y d_backing_inode returns the real i_ino, but
> > newer kernels do not.  This is a problem for EVM as the i_ino is
> > included in the HMAC calculation.
> >
> 
> Hi Mimi,
> 
> v4.19 has a big change that removes many VFS hacks in favor of
> overlayfs stacked file operations.
> 
> The major implication for VFS code is that file_inode(file) is now the overlayfs
> inode and not the real inode. Therefore, file_dentry(file) is also the overlayfs
> dentry and not the real dentry.
> 
> I am not sure how that change impacts EVM ?
> FWIW, d_backing_inode(dentry) was never anything more than d_inode(dentry).
> 
> Can you please try to describe in more details for someone who has no clue what
> EVM does how exactly the v4.19 change is manifested in the EVM use case.
> 
> AFAIKT, evm_calc_hmac_or_hash() would get the overlayfs dentry both in
> v4.18 and v4.19 and therefore d_backing_inode(dentry) should be the
> overlayfs inode in both kernels (?).
> 
> The value of overlayfs inode i_ino can be identical to i_ino of the real inode
> under some conditions, but far from always and the value of overlayfs inode
> i_generation is almost guaranteed to not match that of the real inode.

If I understand it correctly, overlay performs copy_up using a temporary inode,
and finally copies the attributes to a "real" upper inode after the copy_up is
successful. However, integrity or ima gets control for calculation and
verification of the inode during the temporary phase which is not the same as
the final "upper" inode.

-- 
Goldwyn

Re: EVM: Permission denied with overlayfs

[Amir Goldstein]

On Thu, Dec 20, 2018 at 5:42 AM Goldwyn Rodrigues <rgoldwyn@suse.com> wrote:
...
> If I understand it correctly, overlay performs copy_up using a temporary inode,
> and finally copies the attributes to a "real" upper inode after the copy_up is
> successful. However, integrity or ima gets control for calculation and
> verification of the inode during the temporary phase which is not the same as
> the final "upper" inode.
>

No, that's not ow it works.

What happens is as follows:

For brevity, let's discuss a file that was created in overlayfs, not
lower and not copied up.
The latter are just private cases that are less interesting to the
problem at hand.

When changing metadata of an overlayfs file notify_change() will first be
called on the overlay dentry and then on the real underlying dentry.
See, overlayfs completely relies on underlying fs for anything persistent,
so set_xattr on an overlay dentry will (most likely) forward the set_xattr
values to underlying dentry.

So my guess is that on the first (outer) nofity_change() security.evm is
calculated by overlayfs ino/generation and stored in real inode.
On the nested notify_change() call for the underlying dentry, security.evm
will be recalculated and stored (again) in the real inode.

On the verify path ima_file_check() and friends used to be called once pre v4.19
with an overlayfs file, whose file_inode/file_dentry is the real inode/dentry.
With v4.19 stacked file operations, ima_file_check() and friends are
called twice
just like notify_change(), once for the overlayfs file and then for
the underlying
"real" file (the "real" file has overlayfs f_path and underlying
file_inode/file_dentry).

Now according to my understanding of the EVM feature, the metadata of the
overlayfs wrapper object is utterly not interesting and measurements should only
be made on objects representing files written to local storage that
could be tampered with.
Considering the fact that any access to overlayfs object, both set and
get attributes
will always access the underlying object and go through EVM verification,
there needs to be no verification nor calculation of EVM signatures on
the overlayfs
wrapper object.

IMO the right way to fix this would be by setting an SB_NOIMA flag on
overlayfs and
skip the IMA/EVM hooks for overlayfs file_inode/file_dentry.

HOWEVER! you should take extra care to NOT access file->f_path->dentry
in integrity check code, because this is the overlayfs dentry even for "real"
files. AFAIKT, the only place where integrity code accesses file->f_path->dentry
is in Goldwyn's recent fix to ima_calc_file_hash().
                f = dentry_open(&file->f_path, flags, file->f_cred);
returns an overlayfs file with an overlayfs file_inode/file_dentry
even when called
with the "real" file.
It doesn't matter much in the context of ima_calc_file_hash(), because the inode
size and data are always guarantied to be the same for overlayfs and underlying
inode.
You may want to consider using open_with_fake_path() instead of dentry_open().
I think that is called for, but you definitely need an ACK from Al
before doing that
considering his harsh disclaimer [1].

Thanks,
Amir.

[1] commit 2abc77af89e17582db9039293c8ac881c8c96d79
Author: Al Viro <viro@zeniv.linux.org.uk>
Date:   Thu Jul 12 11:18:42 2018 -0400

    new helper: open_with_fake_path()

    open a file by given inode, faking ->f_path.  Use with shitloads
    of caution - at the very least you'd damn better make sure that
    some dentry alias of that inode is pinned down by the path in
    question.  Again, this is no general-purpose interface and I hope
    it will eventually go away.  Right now overlayfs wants something
    like that, but nothing else should.

    Any out-of-tree code with bright idea of using this one *will*
    eventually get hurt, with zero notice and great delight on my part.
    I refuse to use EXPORT_SYMBOL_GPL(), especially in situations when
    it's really EXPORT_SYMBOL_DONT_USE_IT(), but don't take that export
    as "you are welcome to use it".

    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Re: EVM: Permission denied with overlayfs

[Mimi Zohar]

On Wed, 2018-12-19 at 14:08 -0800, James Bottomley wrote:

> > For portable signatures, to bind the file metadata with the file 
> > data, we've replaced the inode number and generation, with the
> > "security.ima" xattr.  Do we want this requirement/limitation for
> > overlays?
> 
> Well, that's my question, yes.  I think there's a reasonable case for
> it, but I was wondering what value the inode number and generation
> brings.  Is there some reason to bind the EVM signature to a more
> mutable file container (which is what inum/generation provide) rather
> than a hard hash of file content (which is what the ima xattr
> provides)?

As only files in the IMA policy are labeled with security.ima, to
protect other files and directories, requires including the inode
number, generation and the UUID.

> > The existing EVM portable signature is an asymmetric algorithm based
> > signature.  Would we define a "portable" HMAC?
> 
> Well, a signature is just an encryption of a hash.  Whether you do HMAC
> with symmetric key or RSA/EC with an asymmetric one is more an
> operational question.  HMAC is certainly much faster but EVM only has a
> single hmac key which is problematic for the containers.  Without a use
> case I can't really say.  Instinct tells me asymmetric is more suitable
> to the container use case, but that's really just a guess.

One of the differences between the EVM portable signature type and the
original signature type is that the portable signatures aren't
replaced with an HMAC.  They're considered portable & immutable.

Adding kernel support for signing mutable files using an asymmetric
key is going to blur the lines between mutable/immutable files.

Mimi

Re: EVM: Permission denied with overlayfs

[James Bottomley]

On Thu, 2018-12-20 at 09:55 -0500, Mimi Zohar wrote:
> On Wed, 2018-12-19 at 14:08 -0800, James Bottomley wrote:
> 
> > > For portable signatures, to bind the file metadata with the file 
> > > data, we've replaced the inode number and generation, with the
> > > "security.ima" xattr.  Do we want this requirement/limitation for
> > > overlays?
> > 
> > Well, that's my question, yes.  I think there's a reasonable case
> > for it, but I was wondering what value the inode number and
> > generation brings.  Is there some reason to bind the EVM signature
> > to a more mutable file container (which is what inum/generation
> > provide) rather than a hard hash of file content (which is what the
> > ima xattr provides)?
> 
> As only files in the IMA policy are labeled with security.ima, to
> protect other files and directories, requires including the inode
> number, generation and the UUID.

OK, so you want to protect the container (essentially file name and
place in the tree) not the contents?  In which case, as Amir said, you
need to be using the filehandle got from something like
exportfs_encode_inode_fh().  That's guaranteed to be an immutable and
stable representative of the container not the contents.

> > > The existing EVM portable signature is an asymmetric algorithm
> > > based signature.  Would we define a "portable" HMAC?
> > 
> > Well, a signature is just an encryption of a hash.  Whether you do
> > HMAC with symmetric key or RSA/EC with an asymmetric one is more an
> > operational question.  HMAC is certainly much faster but EVM only
> > has a single hmac key which is problematic for the
> > containers.  Without a use case I can't really say.  Instinct tells
> > me asymmetric is more suitable to the container use case, but
> > that's really just a guess.
> 
> One of the differences between the EVM portable signature type and
> the original signature type is that the portable signatures aren't
> replaced with an HMAC.  They're considered portable & immutable.
> 
> Adding kernel support for signing mutable files using an asymmetric
> key is going to blur the lines between mutable/immutable files.

So don't do that ... have an additional type that simply doesn't
include the inode/generation (or move to a v2 that uses the filehandle
instead).

James