Now that security_post_load_data() is wired up, use it instead
of the NULL file argument style of security_post_read_file(),
and update the security_kernel_load_data() call to indicate that a
security_kernel_post_load_data() call is expected.
Wire up the IMA check to match earlier logic. Perhaps a generalized
change to ima_post_load_data() might look something like this:
return process_buffer_measurement(buf, size,
kernel_load_data_id_str(load_id),
read_idmap[load_id] ?: FILE_CHECK,
0, NULL);
Signed-off-by: Kees Cook <[email protected]>
---
drivers/base/firmware_loader/fallback.c | 8 ++++----
.../base/firmware_loader/fallback_platform.c | 7 ++++++-
security/integrity/ima/ima_main.c | 20 +++++++++----------
3 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index a196aacce22c..7cfdfdcb819c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -272,9 +272,9 @@ static ssize_t firmware_loading_store(struct device *dev,
dev_err(dev, "%s: map pages failed\n",
__func__);
else
- rc = security_kernel_post_read_file(NULL,
- fw_priv->data, fw_priv->size,
- READING_FIRMWARE);
+ rc = security_kernel_post_load_data(fw_priv->data,
+ fw_priv->size,
+ LOADING_FIRMWARE);
/*
* Same logic as fw_load_abort, only the DONE bit
@@ -613,7 +613,7 @@ static bool fw_run_sysfs_fallback(u32 opt_flags)
return false;
/* Also permit LSMs and IMA to fail firmware sysfs fallback */
- ret = security_kernel_load_data(LOADING_FIRMWARE, false);
+ ret = security_kernel_load_data(LOADING_FIRMWARE, true);
if (ret < 0)
return false;
diff --git a/drivers/base/firmware_loader/fallback_platform.c b/drivers/base/firmware_loader/fallback_platform.c
index a12c79d47efc..4d1157af0e86 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -17,7 +17,7 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32 opt_flags)
if (!(opt_flags & FW_OPT_FALLBACK_PLATFORM))
return -ENOENT;
- rc = security_kernel_load_data(LOADING_FIRMWARE, false);
+ rc = security_kernel_load_data(LOADING_FIRMWARE, true);
if (rc)
return rc;
@@ -27,6 +27,11 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32 opt_flags)
if (fw_priv->data && size > fw_priv->allocated_size)
return -ENOMEM;
+
+ rc = security_kernel_post_load_data((u8 *)data, size, LOADING_FIRMWARE);
+ if (rc)
+ return rc;
+
if (!fw_priv->data)
fw_priv->data = vmalloc(size);
if (!fw_priv->data)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 85000dc8595c..1a7bc4c7437d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -648,15 +648,6 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
enum ima_hooks func;
u32 secid;
- if (!file && read_id == READING_FIRMWARE) {
- if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
- (ima_appraise & IMA_APPRAISE_ENFORCE)) {
- pr_err("Prevent firmware loading_store.\n");
- return -EACCES; /* INTEGRITY_UNKNOWN */
- }
- return 0;
- }
-
/* permit signed certs */
if (!file && read_id == READING_X509_CERTIFICATE)
return 0;
@@ -706,7 +697,7 @@ int ima_load_data(enum kernel_load_data_id id, bool contents)
}
break;
case LOADING_FIRMWARE:
- if (ima_enforce && (ima_appraise & IMA_APPRAISE_FIRMWARE)) {
+ if (ima_enforce && (ima_appraise & IMA_APPRAISE_FIRMWARE) && !contents) {
pr_err("Prevent firmware sysfs fallback loading.\n");
return -EACCES; /* INTEGRITY_UNKNOWN */
}
@@ -739,6 +730,15 @@ int ima_load_data(enum kernel_load_data_id id, bool contents)
*/
int ima_post_load_data(char *buf, loff_t size, enum kernel_load_data_id load_id)
{
+ if (load_id == LOADING_FIRMWARE) {
+ if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
+ (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+ pr_err("Prevent firmware loading_store.\n");
+ return -EACCES; /* INTEGRITY_UNKNOWN */
+ }
+ return 0;
+ }
+
return 0;
}
--
2.25.1
On Fri, 2020-07-24 at 14:36 -0700, Kees Cook wrote:
> Now that security_post_load_data() is wired up, use it instead
> of the NULL file argument style of security_post_read_file(),
> and update the security_kernel_load_data() call to indicate that a
> security_kernel_post_load_data() call is expected.
>
> Wire up the IMA check to match earlier logic. Perhaps a generalized
> change to ima_post_load_data() might look something like this:
>
> return process_buffer_measurement(buf, size,
> kernel_load_data_id_str(load_id),
> read_idmap[load_id] ?: FILE_CHECK,
> 0, NULL);
>
> Signed-off-by: Kees Cook <[email protected]>
process_measurement() measures, verifies a file signature - both
signatures stored as an xattr and as an appended buffer signature -
and augments audit records with the file hash. (Support for measuring,
augmenting audit records, and/or verifying fs-verity signatures has
yet to be added.)
As explained in my response to 11/19, the file descriptor provides the
file pathname associated with the buffer data. In addition, IMA
policy rules may be defined in terms of other file descriptor info -
uid, euid, uuid, etc.
Recently support was added for measuring the kexec boot command line,
certificates being loaded onto a keyring, and blacklisted file hashes
(limited to appended signatures). None of these buffers are signed.
process_buffer_measurement() was added for this reason and as a
result is limited to just measuring the buffer data.
Whether process_measurement() or process_buffer_measurement() should
be modified, needs to be determined. In either case to support the
init_module syscall, would at minimum require the associated file
pathname.
Mimi
On Mon, Jul 27, 2020 at 06:57:45AM -0400, Mimi Zohar wrote:
> On Fri, 2020-07-24 at 14:36 -0700, Kees Cook wrote:
> > Now that security_post_load_data() is wired up, use it instead
> > of the NULL file argument style of security_post_read_file(),
> > and update the security_kernel_load_data() call to indicate that a
> > security_kernel_post_load_data() call is expected.
> >
> > Wire up the IMA check to match earlier logic. Perhaps a generalized
> > change to ima_post_load_data() might look something like this:
> >
> > return process_buffer_measurement(buf, size,
> > kernel_load_data_id_str(load_id),
> > read_idmap[load_id] ?: FILE_CHECK,
> > 0, NULL);
> >
> > Signed-off-by: Kees Cook <[email protected]>
>
> process_measurement() measures, verifies a file signature - ?both
> signatures stored as an xattr and as an appended buffer signature -
> and augments audit records with the file hash. (Support for measuring,
> augmenting audit records, and/or verifying fs-verity signatures has
> yet to be added.)
>
> As explained in my response to 11/19, the file descriptor provides the
> file pathname associated with the buffer data. ?In addition, IMA
> policy rules may be defined in terms of other file descriptor info -
> uid, euid, uuid, etc.
>
> Recently support was added for measuring the kexec boot command line,
> certificates being loaded onto a keyring, and blacklisted file hashes
> (limited to appended signatures). ?None of these buffers are signed.
> ?process_buffer_measurement() was added for this reason and as a
> result is limited to just measuring the buffer data.
>
> Whether process_measurement() or process_buffer_measurement() should
> be modified, needs to be determined. ?In either case to support the
> init_module syscall, would at minimum require the associated file
> pathname.
Right -- I don't intend to make changes to the init_module() syscall
since it's deprecated, so this hook is more of a "fuller LSM coverage
for old syscalls" addition.
IMA can happily continue to ignore it, which is what I have here, but I
thought I'd at least show what it *might* look like. Perhaps BPF LSM is
a better example.
Does anything need to change for this patch?
--
Kees Cook
On Tue, 2020-07-28 at 12:43 -0700, Kees Cook wrote:
> On Mon, Jul 27, 2020 at 06:57:45AM -0400, Mimi Zohar wrote:
> > On Fri, 2020-07-24 at 14:36 -0700, Kees Cook wrote:
> > > Now that security_post_load_data() is wired up, use it instead
> > > of the NULL file argument style of security_post_read_file(),
> > > and update the security_kernel_load_data() call to indicate that a
> > > security_kernel_post_load_data() call is expected.
> > >
> > > Wire up the IMA check to match earlier logic. Perhaps a generalized
> > > change to ima_post_load_data() might look something like this:
> > >
> > > return process_buffer_measurement(buf, size,
> > > kernel_load_data_id_str(load_id),
> > > read_idmap[load_id] ?: FILE_CHECK,
> > > 0, NULL);
> > >
> > > Signed-off-by: Kees Cook <[email protected]>
> >
> > process_measurement() measures, verifies a file signature - both
> > signatures stored as an xattr and as an appended buffer signature -
> > and augments audit records with the file hash. (Support for measuring,
> > augmenting audit records, and/or verifying fs-verity signatures has
> > yet to be added.)
> >
> > As explained in my response to 11/19, the file descriptor provides the
> > file pathname associated with the buffer data. In addition, IMA
> > policy rules may be defined in terms of other file descriptor info -
> > uid, euid, uuid, etc.
> >
> > Recently support was added for measuring the kexec boot command line,
> > certificates being loaded onto a keyring, and blacklisted file hashes
> > (limited to appended signatures). None of these buffers are signed.
> > process_buffer_measurement() was added for this reason and as a
> > result is limited to just measuring the buffer data.
> >
> > Whether process_measurement() or process_buffer_measurement() should
> > be modified, needs to be determined. In either case to support the
> > init_module syscall, would at minimum require the associated file
> > pathname.
>
> Right -- I don't intend to make changes to the init_module() syscall
> since it's deprecated, so this hook is more of a "fuller LSM coverage
> for old syscalls" addition.
>
> IMA can happily continue to ignore it, which is what I have here, but I
> thought I'd at least show what it *might* look like. Perhaps BPF LSM is
> a better example.
>
> Does anything need to change for this patch?
I wasn't aware that init_syscall was deprecated. From your original comments,
it sounded like you wanted a new LSM for verifying kernel module signatures, as
they're currently supported via init_module().
I was mistaken. Without a file descriptor, security_post_load_data() will
measure the firmware, as Scott confirmed, but won't be able to verify the
signature, whether he signed it using evmctl or not.
Mimi
On Wed, 2020-07-29 at 12:29 -0400, Mimi Zohar wrote:
> On Tue, 2020-07-28 at 12:43 -0700, Kees Cook wrote:
> > On Mon, Jul 27, 2020 at 06:57:45AM -0400, Mimi Zohar wrote:
> > > On Fri, 2020-07-24 at 14:36 -0700, Kees Cook wrote:
> > > > Now that security_post_load_data() is wired up, use it instead
> > > > of the NULL file argument style of security_post_read_file(),
> > > > and update the security_kernel_load_data() call to indicate that a
> > > > security_kernel_post_load_data() call is expected.
> > > >
> > > > Wire up the IMA check to match earlier logic. Perhaps a generalized
> > > > change to ima_post_load_data() might look something like this:
> > > >
> > > > return process_buffer_measurement(buf, size,
> > > > kernel_load_data_id_str(load_id),
> > > > read_idmap[load_id] ?: FILE_CHECK,
> > > > 0, NULL);
> > > >
> > > > Signed-off-by: Kees Cook <[email protected]>
> > >
> > > process_measurement() measures, verifies a file signature - both
> > > signatures stored as an xattr and as an appended buffer signature -
> > > and augments audit records with the file hash. (Support for measuring,
> > > augmenting audit records, and/or verifying fs-verity signatures has
> > > yet to be added.)
> > >
> > > As explained in my response to 11/19, the file descriptor provides the
> > > file pathname associated with the buffer data. In addition, IMA
> > > policy rules may be defined in terms of other file descriptor info -
> > > uid, euid, uuid, etc.
> > >
> > > Recently support was added for measuring the kexec boot command line,
> > > certificates being loaded onto a keyring, and blacklisted file hashes
> > > (limited to appended signatures). None of these buffers are signed.
> > > process_buffer_measurement() was added for this reason and as a
> > > result is limited to just measuring the buffer data.
> > >
> > > Whether process_measurement() or process_buffer_measurement() should
> > > be modified, needs to be determined. In either case to support the
> > > init_module syscall, would at minimum require the associated file
> > > pathname.
> >
> > Right -- I don't intend to make changes to the init_module() syscall
> > since it's deprecated, so this hook is more of a "fuller LSM coverage
> > for old syscalls" addition.
> >
> > IMA can happily continue to ignore it, which is what I have here, but I
> > thought I'd at least show what it *might* look like. Perhaps BPF LSM is
> > a better example.
> >
> > Does anything need to change for this patch?
>
> I wasn't aware that init_syscall was deprecated. From your original comments,
> it sounded like you wanted a new LSM for verifying kernel module signatures,
> as
> they're currently supported via init_module().
>
> I was mistaken. Without a file descriptor, security_post_load_data() will
> measure the firmware, as Scott confirmed, but won't be able to verify the
> signature, whether he signed it using evmctl or not,
Actually, the partial firmware read should be calling
security_kernel_read_file(). The sysfs firmware fallback is calling
security_kernel_load_data(). Which firmware is calling
security_kernel_post_load_data()?
thanks,
Mimi
On Wed, Jul 29, 2020 at 02:10:18PM -0400, Mimi Zohar wrote:
> Actually, the partial firmware read should be calling
> security_kernel_read_file().
Yup, it does[1], and when "whole_file" is true, it will call
security_kernel_post_read_file() with the buffer contents at the end.
> The sysfs firmware fallback is calling security_kernel_load_data().
Correct[2]; it has no file associated with it (same as the EFI platform
source).
> Which firmware is calling security_kernel_post_load_data()?
sysfs and platform both call it[2], matched with their
security_kernel_load_data() calls.
-Kees
[1] v4 patch 14: "fs/kernel_file_read: Add "offset" arg for partial reads"
https://lore.kernel.org/lkml/[email protected]/T/#iZ2e.:..:20200729175845.1745471-15-keescook::40chromium.org:0fs:kernel_read_file.c
[2] v4 patch 10: "firmware_loader: Use security_post_load_data()"
https://lore.kernel.org/lkml/[email protected]/T/#iZ2e.:..:20200729175845.1745471-11-keescook::40chromium.org:0drivers:base:firmware_loader:fallback.c
--
Kees Cook