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; }
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
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;@@ -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;} -- 2.27.GIT
---end quoted text---
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 forreading.\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;@@ -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;}
-- 2.27.GIT
---end quoted text---
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.
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
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
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.
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
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
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
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
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
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
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
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
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
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
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 ;-/
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
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
linux-stable-mirror@lists.linaro.org
-
Al Viro -
Christoph Hellwig -
Linus Torvalds -
Mimi Zohar -
Roberto Sassu -
Theodore Y. Ts'o