[RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Roberto Sassu]

Commit a1f9b1c0439db ("integrity/ima: switch to using __kernel_read")
replaced the __vfs_read() call in integrity_kernel_read() with
__kernel_read(), a new helper introduced by commit 61a707c543e2a ("fs: add
a __kernel_read helper").

Since the new helper requires that also the FMODE_CAN_READ flag is set in
file->f_mode, this patch saves the original f_mode and sets the flag if the
the file descriptor has the necessary file operation. Lastly, it restores
the original f_mode at the end of ima_calc_file_hash().

Cc: stable@vger.kernel.org # 5.8.x
Fixes: a1f9b1c0439db ("integrity/ima: switch to using __kernel_read")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_crypto.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 21989fa0c107..22ed86a0c964 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -537,6 +537,7 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
 	loff_t i_size;
 	int rc;
 	struct file *f = file;
+	fmode_t saved_mode;
 	bool new_file_instance = false, modified_mode = false;
 
 	/*
@@ -550,7 +551,7 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
 	}
 
 	/* Open a new file instance in O_RDONLY if we cannot read */
-	if (!(file->f_mode & FMODE_READ)) {
+	if (!(file->f_mode & FMODE_READ) || !(file->f_mode & FMODE_CAN_READ)) {
 		int flags = file->f_flags & ~(O_WRONLY | O_APPEND |
 				O_TRUNC | O_CREAT | O_NOCTTY | O_EXCL);
 		flags |= O_RDONLY;
@@ -562,7 +563,10 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
 			 */
 			pr_info_ratelimited("Unable to reopen file for reading.\n");
 			f = file;
+			saved_mode = f->f_mode;
 			f->f_mode |= FMODE_READ;
+			if (likely(file->f_op->read || file->f_op->read_iter))
+				f->f_mode |= FMODE_CAN_READ;
 			modified_mode = true;
 		} else {
 			new_file_instance = true;
@@ -582,7 +586,7 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
 	if (new_file_instance)
 		fput(f);
 	else if (modified_mode)
-		f->f_mode &= ~FMODE_READ;
+		f->f_mode = saved_mode;
 	return rc;
 }
 
-- 
2.27.GIT

Re: [RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Mimi Zohar]

Hi Roberto,

On Fri, 2020-11-13 at 09:01 +0100, Roberto Sassu wrote:
> Commit a1f9b1c0439db ("integrity/ima: switch to using __kernel_read")
> replaced the __vfs_read() call in integrity_kernel_read() with
> __kernel_read(), a new helper introduced by commit 61a707c543e2a ("fs: add
> a __kernel_read helper").
> 
> Since the new helper requires that also the FMODE_CAN_READ flag is set in
> file->f_mode, this patch saves the original f_mode and sets the flag if the
> the file descriptor has the necessary file operation. Lastly, it restores
> the original f_mode at the end of ima_calc_file_hash().
> 
> Cc: stable@vger.kernel.org # 5.8.x
> Fixes: a1f9b1c0439db ("integrity/ima: switch to using __kernel_read")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Thanks!  It's now queued in next-integrity-testing.

Mimi

Re: [RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Christoph Hellwig]

On Fri, Nov 13, 2020 at 09:01:32AM +0100, Roberto Sassu wrote:
> Commit a1f9b1c0439db ("integrity/ima: switch to using __kernel_read")
> replaced the __vfs_read() call in integrity_kernel_read() with
> __kernel_read(), a new helper introduced by commit 61a707c543e2a ("fs: add
> a __kernel_read helper").
> 
> Since the new helper requires that also the FMODE_CAN_READ flag is set in
> file->f_mode, this patch saves the original f_mode and sets the flag if the
> the file descriptor has the necessary file operation. Lastly, it restores
> the original f_mode at the end of ima_calc_file_hash().

This looks bogus.  FMODE_CAN_READ has a pretty clear definition and
you can't just go and read things if it is not set.  Also f_mode
manipulations on a life file are racy.

> 
> Cc: stable@vger.kernel.org # 5.8.x
> Fixes: a1f9b1c0439db ("integrity/ima: switch to using __kernel_read")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/integrity/ima/ima_crypto.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 21989fa0c107..22ed86a0c964 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -537,6 +537,7 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>  	loff_t i_size;
>  	int rc;
>  	struct file *f = file;
> +	fmode_t saved_mode;
>  	bool new_file_instance = false, modified_mode = false;
>  
>  	/*
> @@ -550,7 +551,7 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>  	}
>  
>  	/* Open a new file instance in O_RDONLY if we cannot read */
> -	if (!(file->f_mode & FMODE_READ)) {
> +	if (!(file->f_mode & FMODE_READ) || !(file->f_mode & FMODE_CAN_READ)) {
>  		int flags = file->f_flags & ~(O_WRONLY | O_APPEND |
>  				O_TRUNC | O_CREAT | O_NOCTTY | O_EXCL);
>  		flags |= O_RDONLY;
> @@ -562,7 +563,10 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>  			 */
>  			pr_info_ratelimited("Unable to reopen file for reading.\n");
>  			f = file;
> +			saved_mode = f->f_mode;
>  			f->f_mode |= FMODE_READ;
> +			if (likely(file->f_op->read || file->f_op->read_iter))
> +				f->f_mode |= FMODE_CAN_READ;
>  			modified_mode = true;
>  		} else {
>  			new_file_instance = true;
> @@ -582,7 +586,7 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>  	if (new_file_instance)
>  		fput(f);
>  	else if (modified_mode)
> -		f->f_mode &= ~FMODE_READ;
> +		f->f_mode = saved_mode;
>  	return rc;
>  }
>  
> -- 
> 2.27.GIT
> 
---end quoted text---

RE: [RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Roberto Sassu]

> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Saturday, November 14, 2020 12:11 PM
> On Fri, Nov 13, 2020 at 09:01:32AM +0100, Roberto Sassu wrote:
> > Commit a1f9b1c0439db ("integrity/ima: switch to using __kernel_read")
> > replaced the __vfs_read() call in integrity_kernel_read() with
> > __kernel_read(), a new helper introduced by commit 61a707c543e2a ("fs:
> add
> > a __kernel_read helper").
> >
> > Since the new helper requires that also the FMODE_CAN_READ flag is set
> in
> > file->f_mode, this patch saves the original f_mode and sets the flag if the
> > the file descriptor has the necessary file operation. Lastly, it restores
> > the original f_mode at the end of ima_calc_file_hash().
> 
> This looks bogus.  FMODE_CAN_READ has a pretty clear definition and
> you can't just go and read things if it is not set.  Also f_mode
> manipulations on a life file are racy.

FMODE_CAN_READ was not set because f_mode does not have
FMODE_READ. In the patch, I check if the former can be set
similarly to the way it is done in file_table.c and open.c.

Is there a better way to read a file when the file was not opened
for reading and a new file descriptor cannot be created?

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

> > Cc: stable@vger.kernel.org # 5.8.x
> > Fixes: a1f9b1c0439db ("integrity/ima: switch to using __kernel_read")
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  security/integrity/ima/ima_crypto.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_crypto.c
> b/security/integrity/ima/ima_crypto.c
> > index 21989fa0c107..22ed86a0c964 100644
> > --- a/security/integrity/ima/ima_crypto.c
> > +++ b/security/integrity/ima/ima_crypto.c
> > @@ -537,6 +537,7 @@ int ima_calc_file_hash(struct file *file, struct
> ima_digest_data *hash)
> >  	loff_t i_size;
> >  	int rc;
> >  	struct file *f = file;
> > +	fmode_t saved_mode;
> >  	bool new_file_instance = false, modified_mode = false;
> >
> >  	/*
> > @@ -550,7 +551,7 @@ int ima_calc_file_hash(struct file *file, struct
> ima_digest_data *hash)
> >  	}
> >
> >  	/* Open a new file instance in O_RDONLY if we cannot read */
> > -	if (!(file->f_mode & FMODE_READ)) {
> > +	if (!(file->f_mode & FMODE_READ) || !(file->f_mode &
> FMODE_CAN_READ)) {
> >  		int flags = file->f_flags & ~(O_WRONLY | O_APPEND |
> >  				O_TRUNC | O_CREAT | O_NOCTTY |
> O_EXCL);
> >  		flags |= O_RDONLY;
> > @@ -562,7 +563,10 @@ int ima_calc_file_hash(struct file *file, struct
> ima_digest_data *hash)
> >  			 */
> >  			pr_info_ratelimited("Unable to reopen file for
> reading.\n");
> >  			f = file;
> > +			saved_mode = f->f_mode;
> >  			f->f_mode |= FMODE_READ;
> > +			if (likely(file->f_op->read || file->f_op->read_iter))
> > +				f->f_mode |= FMODE_CAN_READ;
> >  			modified_mode = true;
> >  		} else {
> >  			new_file_instance = true;
> > @@ -582,7 +586,7 @@ int ima_calc_file_hash(struct file *file, struct
> ima_digest_data *hash)
> >  	if (new_file_instance)
> >  		fput(f);
> >  	else if (modified_mode)
> > -		f->f_mode &= ~FMODE_READ;
> > +		f->f_mode = saved_mode;
> >  	return rc;
> >  }
> >
> > --
> > 2.27.GIT
> >
> ---end quoted text---

Re: [RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Christoph Hellwig]

On Mon, Nov 16, 2020 at 08:52:19AM +0000, Roberto Sassu wrote:
> FMODE_CAN_READ was not set because f_mode does not have
> FMODE_READ. In the patch, I check if the former can be set
> similarly to the way it is done in file_table.c and open.c.
> 
> Is there a better way to read a file when the file was not opened
> for reading and a new file descriptor cannot be created?

You can't open a file not open for reading.  The file system or device
driver might have to prepare read-specific resources in ->open to
support reads.  So what you'll have to do is to open a new instance
of the file that is open for reading.

Re: [RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Mimi Zohar]

On Mon, 2020-11-16 at 16:22 +0000, Christoph Hellwig wrote:
> On Mon, Nov 16, 2020 at 08:52:19AM +0000, Roberto Sassu wrote:
> > FMODE_CAN_READ was not set because f_mode does not have
> > FMODE_READ. In the patch, I check if the former can be set
> > similarly to the way it is done in file_table.c and open.c.
> > 
> > Is there a better way to read a file when the file was not opened
> > for reading and a new file descriptor cannot be created?
> 
> You can't open a file not open for reading.  The file system or device
> driver might have to prepare read-specific resources in ->open to
> support reads.  So what you'll have to do is to open a new instance
> of the file that is open for reading.

This discussion seems to be going down the path of requiring an IMA
filesystem hook for reading the file, again.  That solution was
rejected, not by me.  What is new this time?

Mimi

Re: [RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Linus Torvalds]

On Mon, Nov 16, 2020 at 8:47 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> This discussion seems to be going down the path of requiring an IMA
> filesystem hook for reading the file, again.  That solution was
> rejected, not by me.  What is new this time?

You can't read a non-read-opened file. Not even IMA can.

So don't do that then.

IMA is doing something wrong. Why would you ever read a file that can't be read?

Fix whatever "open" function instead of trying to work around the fact
that you opened it wrong.

             Linus

Re: [RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Christoph Hellwig]

On Mon, Nov 16, 2020 at 09:37:32AM -0800, Linus Torvalds wrote:
> > This discussion seems to be going down the path of requiring an IMA
> > filesystem hook for reading the file, again.  That solution was
> > rejected, not by me.  What is new this time?
> 
> You can't read a non-read-opened file. Not even IMA can.
> 
> So don't do that then.
> 
> IMA is doing something wrong. Why would you ever read a file that can't be read?
> 
> Fix whatever "open" function instead of trying to work around the fact
> that you opened it wrong.

The "issue" with IMA is that it uses security hooks to hook into the
VFS and then wants to read every file that gets opened on a real file
system to "measure" the contents vs a hash stashed away somewhere.
Which has always been rather sketchy.

Re: [RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Al Viro]

On Mon, Nov 16, 2020 at 09:37:32AM -0800, Linus Torvalds wrote:
> On Mon, Nov 16, 2020 at 8:47 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > This discussion seems to be going down the path of requiring an IMA
> > filesystem hook for reading the file, again.  That solution was
> > rejected, not by me.  What is new this time?
> 
> You can't read a non-read-opened file. Not even IMA can.
> 
> So don't do that then.
> 
> IMA is doing something wrong. Why would you ever read a file that can't be read?
>
> Fix whatever "open" function instead of trying to work around the fact
> that you opened it wrong.

IMA pulls that crap on _every_ open(2), including O_WRONLY.  As far as I'm
concerned, the only sane answer is not enabling that thing on your builds;
they are deeply special and I hadn't been able to reason with them no
matter how much I tried ;-/

Re: [RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Linus Torvalds]

On Mon, Nov 16, 2020 at 9:41 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> The "issue" with IMA is that it uses security hooks to hook into the
> VFS and then wants to read every file that gets opened on a real file
> system to "measure" the contents vs a hash stashed away somewhere.

Well, but that's easy enough to handle: if the open isn't a read open,
then the old contents don't matter, so you shouldn't bother to measure
the file.

So this literally sounds like a "doctor, doctor, it hurts when I hit
my head with a hammer" situation..

         Linus

Re: [RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Mimi Zohar]

On Mon, 2020-11-16 at 17:41 +0000, Christoph Hellwig wrote:
> On Mon, Nov 16, 2020 at 09:37:32AM -0800, Linus Torvalds wrote:
> > > This discussion seems to be going down the path of requiring an IMA
> > > filesystem hook for reading the file, again.  That solution was
> > > rejected, not by me.  What is new this time?
> > 
> > You can't read a non-read-opened file. Not even IMA can.
> > 
> > So don't do that then.
> > 
> > IMA is doing something wrong. Why would you ever read a file that can't be read?
> > 
> > Fix whatever "open" function instead of trying to work around the fact
> > that you opened it wrong.
> 
> The "issue" with IMA is that it uses security hooks to hook into the
> VFS and then wants to read every file that gets opened on a real file
> system to "measure" the contents vs a hash stashed away somewhere.
> Which has always been rather sketchy.

There are security hooks, where IMA is co-located, but there are also
IMA hooks where there isn't an IMA hook (e.g. ima_file_check).  In all
cases, the file needs to be read in order to calculate the file hash,
which is then used for verifying file signatures (equivalent of secure
boot) and extending the TPM (equivalent of trusted boot).  Only after
measuring and verifying the file integrity, should access be granted to
the file.

Whether filesystems can and should be trusted to provide the real file
hashes is a separate issue.

The decision as to which files should be measured or the signature
verified is based on policy.

thanks,

Mimi

Re: [RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Mimi Zohar]

On Mon, 2020-11-16 at 10:09 -0800, Linus Torvalds wrote:
> On Mon, Nov 16, 2020 at 9:41 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > The "issue" with IMA is that it uses security hooks to hook into the
> > VFS and then wants to read every file that gets opened on a real file
> > system to "measure" the contents vs a hash stashed away somewhere.
> 
> Well, but that's easy enough to handle: if the open isn't a read open,
> then the old contents don't matter, so you shouldn't bother to measure
> the file.
> 
> So this literally sounds like a "doctor, doctor, it hurts when I hit
> my head with a hammer" situation..

We need to differentiate between signed files, which by definition are
immutable, and those that are mutable.  Appending to a mutable file,
for example, would result in the file hash not being updated. 
Subsequent reads would fail.

Mimi

Re: [RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Mimi Zohar]

On Mon, 2020-11-16 at 18:08 +0000, Al Viro wrote:
> On Mon, Nov 16, 2020 at 09:37:32AM -0800, Linus Torvalds wrote:
> > On Mon, Nov 16, 2020 at 8:47 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > >
> > > This discussion seems to be going down the path of requiring an IMA
> > > filesystem hook for reading the file, again.  That solution was
> > > rejected, not by me.  What is new this time?
> > 
> > You can't read a non-read-opened file. Not even IMA can.
> > 
> > So don't do that then.
> > 
> > IMA is doing something wrong. Why would you ever read a file that can't be read?
> >
> > Fix whatever "open" function instead of trying to work around the fact
> > that you opened it wrong.
> 
> IMA pulls that crap on _every_ open(2), including O_WRONLY.  As far as I'm
> concerned, the only sane answer is not enabling that thing on your builds;
> they are deeply special and I hadn't been able to reason with them no
> matter how much I tried ;-/

The builtin IMA policies are only meant to be used until a custom can
be loaded.  The decision as to what should be measured or verified is
left up to the system owner.

In terms of the architecture specific policy rules, there are rules to:
- measure the kexec kernel image and kernel modules
- verify the kexec kernel image and kernel modules appended signatures

These rules should be pretty straight forward to verify.

Mimi

RE: [RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Roberto Sassu]

> From: Al Viro [mailto:viro@ftp.linux.org.uk] On Behalf Of Al Viro
> Sent: Monday, November 16, 2020 7:09 PM
> On Mon, Nov 16, 2020 at 09:37:32AM -0800, Linus Torvalds wrote:
> > On Mon, Nov 16, 2020 at 8:47 AM Mimi Zohar <zohar@linux.ibm.com>
> wrote:
> > >
> > > This discussion seems to be going down the path of requiring an IMA
> > > filesystem hook for reading the file, again.  That solution was
> > > rejected, not by me.  What is new this time?
> >
> > You can't read a non-read-opened file. Not even IMA can.
> >
> > So don't do that then.
> >
> > IMA is doing something wrong. Why would you ever read a file that can't
> be read?
> >
> > Fix whatever "open" function instead of trying to work around the fact
> > that you opened it wrong.
> 
> IMA pulls that crap on _every_ open(2), including O_WRONLY.  As far as I'm
> concerned, the only sane answer is not enabling that thing on your builds;
> they are deeply special and I hadn't been able to reason with them no
> matter how much I tried ;-/

A file-based protection mechanism against offline attacks would require
to verify the current HMAC also before writing and to update the HMAC
after the write.

One of the reasons why dentry_open() cannot be used and IMA switches
to the old method of changing the mode of the current file descriptor is
that the current process does not have enough privileges to do the
operation.

If we find a way to read the file that always works, without reducing the
security, the old method can be removed.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

Re: [RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Linus Torvalds]

On Mon, Nov 16, 2020 at 10:35 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> We need to differentiate between signed files, which by definition are
> immutable, and those that are mutable.  Appending to a mutable file,
> for example, would result in the file hash not being updated.
> Subsequent reads would fail.

Why would that require any reading of the file at all AT WRITE TIME?

Don't do it. Really.

When opening the file write-only, you just invalidate the hash. It
doesn't matter anyway - you're only writing.

Later on, when reading, only at that point does the hash matter, and
then you can do the verification.

Although honestly, I don't even see the point. You know the hash won't
match, if you wrote to the file.

           Linus

Re: [RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Theodore Y. Ts'o]

On Tue, Nov 17, 2020 at 10:23:58AM -0800, Linus Torvalds wrote:
> On Mon, Nov 16, 2020 at 10:35 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > We need to differentiate between signed files, which by definition are
> > immutable, and those that are mutable.  Appending to a mutable file,
> > for example, would result in the file hash not being updated.
> > Subsequent reads would fail.
> 
> Why would that require any reading of the file at all AT WRITE TIME?
> 
> Don't do it. Really.
> 
> When opening the file write-only, you just invalidate the hash. It
> doesn't matter anyway - you're only writing.
> 
> Later on, when reading, only at that point does the hash matter, and
> then you can do the verification.
> 
> Although honestly, I don't even see the point. You know the hash won't
> match, if you wrote to the file.

I think the use case the IMA folks might be thinking about is where
they want to validate the file at open time, *before* the userspace
application starts writing to the file, since there might be some
subtle attacks where Boris changes the first part of the file before
Alice appends "I agree" to said file.

Of course, Boris will be able to modify the file after Alice has
modified it, so it's a bit of a moot point, but one could imagine a
scenario where the file is modified while the system is not running
(via an evil hotel maid) and then after Alice modifies the file, of
*course* the hash will be invalid, so no one would notice.  A sane
application would have read the file to make sure it contained the
proper contents before appending "I agree" to said file, so it's a bit
of an esoteric point.

The other case I could imagine is if the file is marked execute-only,
without read access, and IMA wanted to be able to read the file to
check the hash.  But we already make an execption for allowing the
file to be read during page faults, so that's probably less
controversial.

						- Ted

Re: [RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Mimi Zohar]

On Tue, 2020-11-17 at 10:23 -0800, Linus Torvalds wrote:
> On Mon, Nov 16, 2020 at 10:35 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > We need to differentiate between signed files, which by definition are
> > immutable, and those that are mutable.  Appending to a mutable file,
> > for example, would result in the file hash not being updated.
> > Subsequent reads would fail.
> 
> Why would that require any reading of the file at all AT WRITE TIME?

On the (last) file close, the file hash is re-calculated and written
out as security.ima.  The EVM hmac is re-calculated and written out as
security.evm.

> 
> Don't do it. Really.

I really wish it wasn't needed.

> 
> When opening the file write-only, you just invalidate the hash. It
> doesn't matter anyway - you're only writing.
> 
> Later on, when reading, only at that point does the hash matter, and
> then you can do the verification.
> 
> Although honestly, I don't even see the point. You know the hash won't
> match, if you wrote to the file.

On the local system, as Roberto mentioned, before updating a file, the
existing file's data and metadata (EVM) should be verified to protect
from an offline attack.

The above scenario assumes calculating the file hash is only being used
for verifying the integrity of the file (security.ima), but there are
other reasons for calculating the file hash.  For example depending on
the IMA measurement policy, just accessing a file could require
including the file hash in the measurement list.  True that measurement
will only be valid at the time of measurement, but it provides a base
value.

Mimi

Re: [RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Linus Torvalds]

On Tue, Nov 17, 2020 at 3:24 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> I really wish it wasn't needed.

Seriously, I get the feeling that IMA is completely mis-designed, and
is doing actively bad things.

Who uses this "feature", and who cares? Because I would suggest you
just change the policy and be done with it.

            Linus

Re: [RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Linus Torvalds]

On Tue, Nov 17, 2020 at 3:29 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Tue, Nov 17, 2020 at 3:24 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > I really wish it wasn't needed.
>
> Seriously, I get the feeling that IMA is completely mis-designed, and
> is doing actively bad things.
>
> Who uses this "feature", and who cares? Because I would suggest you
> just change the policy and be done with it.

Another alternative is to change the policy and say "any write-only
open gets turned into a read-write open".

But it needs to be done at *OPEN* time, not randomly afterwards by
just lying to the 'struct file'.

Why? Because the open has told the filesystem that it's only for
writing, and a filesystem could validly do things that make reading
invalid. The simplest example of this is a network filesystem, where
the server might simply not *allow* reads, because the open was for
writing only.

See? IMA really does something fundamentally quite wrong when it tries
to read from a non-readable file. It might "work" by accident, but I
really do think that commit a1f9b1c0439db didn't "break" IMA - it
showed that IMA was doing something fundamentally wrong.

           Linus

Re: [RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Mimi Zohar]

On Tue, 2020-11-17 at 15:36 -0800, Linus Torvalds wrote:
> Another alternative is to change the policy and say "any write-only
> open gets turned into a read-write open".
> 
> But it needs to be done at *OPEN* time, not randomly afterwards by
> just lying to the 'struct file'.

The ima_file_check hook is at open, but it is immediately after
vfs_open().   Only after the file is opened can we determine if the
file is in policy.  If the file was originally opened without read
permission, a new file instance (dentry_open) with read permission is
opened.  Would limiting opening a new file instance with read
permission to just the ima_file_check hook be acceptable?

thanks,

Mimi

RE: [RESEND][PATCH] ima: Set and clear FMODE_CAN_READ in ima_calc_file_hash()

[Roberto Sassu]

> From: Linus Torvalds [mailto:torvalds@linux-foundation.org]
> Sent: Wednesday, November 18, 2020 12:37 AM
> On Tue, Nov 17, 2020 at 3:29 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Tue, Nov 17, 2020 at 3:24 PM Mimi Zohar <zohar@linux.ibm.com>
> wrote:
> > >
> > > I really wish it wasn't needed.
> >
> > Seriously, I get the feeling that IMA is completely mis-designed, and
> > is doing actively bad things.
> >
> > Who uses this "feature", and who cares? Because I would suggest you
> > just change the policy and be done with it.
> 
> Another alternative is to change the policy and say "any write-only
> open gets turned into a read-write open".

One issue that would arise from doing it is that security policies need
to be modified to grant the additional read permission. If the open
flag is added early, the LSM hook security_file_open() will see it.

This solution seems not optimal, as we are giving to processes a
permission that they wouldn't really take advantage of, since the
content read remains in kernel space. And an additional permission
is a permission that can be exploited.

As Mimi said, we already have a second open with dentry_open() when
the original file descriptor is not suitable. The only problem, which is
why changing the mode is still there, is that a process still might not
have the privilege to read, and this is a legitimate case.

We could assign a more powerful credential to the process, since
dentry_open() accepts a credential as an argument. We could obtain
such powerful credential from prepare_kernel_cred(). This option
has better chances to work without modifying existing security policies
as likely those policies already assigned the required privilege to the
kernel. However, doing so might not be what LSM people recommend.

Any suggestion?

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli