* [PATCH v7 0/5] KEYS: Measure keys when they are created or updated
From: Lakshmi Ramasubramanian @ 2019-11-14 3:11 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
Keys created or updated in the system are currently not measured.
Therefore an attestation service, for instance, would not be able to
attest whether or not the trusted keys keyring(s), for instance, contain
only known good (trusted) keys.
IMA measures system files, command line arguments passed to kexec,
boot aggregate, etc. It can be used to measure keys as well.
But there is no mechanism available in the kernel for IMA to
know when a key is created or updated.
This change aims to address measuring keys created or updated
in the system.
To achieve the above the following changes have been made:
- Added a new IMA hook namely, ima_post_key_create_or_update, which
measures the key. This IMA hook is called from key_create_or_update
function. The key measurement can be controlled through IMA policy.
A new IMA policy function KEY_CHECK has been added to measure keys.
"keyrings=" option can be specified for KEY_CHECK to limit
measuring the keys loaded onto the specified keyrings only.
# measure keys loaded onto any keyring
measure func=KEY_CHECK
# measure keys loaded onto the IMA keyring only
measure func=KEY_CHECK keyring=".ima"
# measure keys on the BUILTIN and IMA keyrings into a different PCR
measure func=KEY_CHECK keyring=".builtin_trusted_keys|.ima" pcr=11
Testing performed:
* Booted the kernel with this change.
* When KEY_CHECK policy is set IMA measures keys loaded
onto any keyring (keyrings= option not specified).
* Keys are not measured when KEY_CHECK is not set.
* When keyrings= option is specified for KEY_CHECK then only the keys
loaded onto a keyring specified in the option is measured.
* Added a new key to a keyring.
=> Added keys to .ima and .evm keyrings.
* Added the same key again.
=> Add the same key to .ima and .evm keyrings.
Change Log:
v7:
=> Removed CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS option and used
CONFIG_KEYS instead for ima_asymmetric_keys.c
=> Added the patches related to "keyrings=" option support to
this patch set.
v6:
=> Rebased the changes to v5.4-rc7
=> Renamed KEYRING_CHECK to KEY_CHECK per Mimi's suggestion.
=> Excluded the patches that add support for limiting key
measurement to specific keyrings ("keyrings=" option
for "measure func=KEY_CHECK" in the IMA policy).
Also, excluded the patches that add support for deferred
processing of keys (queue support).
These patches will be added in separate patch sets later.
v5:
=> Reorganized the patches to add measurement of keys through
the IMA hook without any queuing and then added queuing support.
=> Updated the queuing functions to minimize code executed inside mutex.
=> Process queued keys after custom IMA policies have been applied.
v4:
=> Rebased the changes to v5.4-rc3
=> Applied the following dependent patch set first
and then added new changes.
https://lore.kernel.org/linux-integrity/1572492694-6520-1-git-send-email-zohar@linux.ibm.com
=> Refactored the patch set to separate out changes related to
func KEYRING_CHECK and options keyrings into different patches.
=> Moved the functions to queue and dequeue keys for measurement
from ima_queue.c to a new file ima_asymmetric_keys.c.
=> Added a new config namely CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
to compile ima_asymmetric_keys.c
v3:
=> Added KEYRING_CHECK for measuring keys. This can optionally specify
keyrings to measure.
=> Updated ima_get_action() and related functions to return
the keyrings if specified in the policy.
=> process_buffer_measurement() function is updated to take keyring
as a parameter. The key will be measured if the policy includes
the keyring in the list of measured keyrings. If the policy does not
specify any keyrings then all keys are measured.
v2:
=> Per suggestion from Mimi reordered the patch set to first
enable measuring keys added or updated in the system.
And, then scope the measurement to keys added to
builtin_trusted_keys keyring through ima policy.
=> Removed security_key_create_or_update function and instead
call ima hook, to measure the key, directly from
key_create_or_update function.
v1:
=> LSM function for key_create_or_update. It calls ima.
=> Added ima hook for measuring keys
=> ima measures keys based on ima policy.
v0:
=> Added LSM hook for key_create_or_update.
=> Measure keys added to builtin or secondary trusted keys keyring.
Lakshmi Ramasubramanian (5):
IMA: Add KEY_CHECK func to measure keys
IMA: Define an IMA hook to measure keys
KEYS: Call the IMA hook to measure keys
IMA: Add support to limit measuring keys
IMA: Read keyrings= option from the IMA policy
Documentation/ABI/testing/ima_policy | 17 ++++-
include/linux/ima.h | 13 ++++
security/integrity/ima/Makefile | 1 +
security/integrity/ima/ima.h | 9 ++-
security/integrity/ima/ima_api.c | 8 ++-
security/integrity/ima/ima_appraise.c | 4 +-
security/integrity/ima/ima_asymmetric_keys.c | 57 ++++++++++++++++
security/integrity/ima/ima_main.c | 9 +--
security/integrity/ima/ima_policy.c | 70 ++++++++++++++++++--
security/keys/key.c | 7 ++
10 files changed, 175 insertions(+), 20 deletions(-)
create mode 100644 security/integrity/ima/ima_asymmetric_keys.c
--
2.17.1
^ permalink raw reply
* [PATCH v7 3/5] KEYS: Call the IMA hook to measure keys
From: Lakshmi Ramasubramanian @ 2019-11-14 3:12 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191114031202.18012-1-nramas@linux.microsoft.com>
Call the IMA hook from key_create_or_update function to measure
the key when a new key is created or an existing key is updated.
This patch adds the call to the IMA hook from key_create_or_update
function to measure the key on key create or update.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
include/linux/ima.h | 13 +++++++++++++
security/keys/key.c | 7 +++++++
2 files changed, 20 insertions(+)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 6d904754d858..0af88b781ab6 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -25,6 +25,12 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
extern void ima_post_path_mknod(struct dentry *dentry);
extern void ima_kexec_cmdline(const void *buf, int size);
+#ifdef CONFIG_KEYS
+extern void ima_post_key_create_or_update(struct key *keyring,
+ struct key *key,
+ unsigned long flags, bool create);
+#endif
+
#ifdef CONFIG_IMA_KEXEC
extern void ima_add_kexec_buffer(struct kimage *image);
#endif
@@ -101,6 +107,13 @@ static inline void ima_add_kexec_buffer(struct kimage *image)
{}
#endif
+#ifndef CONFIG_KEYS
+static inline void ima_post_key_create_or_update(struct key *keyring,
+ struct key *key,
+ unsigned long flags,
+ bool create) {}
+#endif
+
#ifdef CONFIG_IMA_APPRAISE
extern bool is_ima_appraise_enabled(void);
extern void ima_inode_post_setattr(struct dentry *dentry);
diff --git a/security/keys/key.c b/security/keys/key.c
index 764f4c57913e..a0f1e7b3b8b9 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -13,6 +13,7 @@
#include <linux/security.h>
#include <linux/workqueue.h>
#include <linux/random.h>
+#include <linux/ima.h>
#include <linux/err.h>
#include "internal.h"
@@ -936,6 +937,8 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
goto error_link_end;
}
+ ima_post_key_create_or_update(keyring, key, flags, true);
+
key_ref = make_key_ref(key, is_key_possessed(keyring_ref));
error_link_end:
@@ -965,6 +968,10 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
}
key_ref = __key_update(key_ref, &prep);
+
+ if (!IS_ERR(key_ref))
+ ima_post_key_create_or_update(keyring, key, flags, false);
+
goto error_free_prep;
}
EXPORT_SYMBOL(key_create_or_update);
--
2.17.1
^ permalink raw reply related
* [PATCH v7 1/5] IMA: Add KEY_CHECK func to measure keys
From: Lakshmi Ramasubramanian @ 2019-11-14 3:11 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191114031202.18012-1-nramas@linux.microsoft.com>
Measure keys loaded onto any keyring.
This patch defines a new IMA policy func namely KEY_CHECK to
measure keys. Updated ima_match_rules() to check for KEY_CHECK
and ima_parse_rule() to handle KEY_CHECK.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
Documentation/ABI/testing/ima_policy | 7 ++++++-
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_policy.c | 4 +++-
3 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 29aaedf33246..3823c27894c5 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -29,7 +29,7 @@ Description:
base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
[FIRMWARE_CHECK]
[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
- [KEXEC_CMDLINE]
+ [KEXEC_CMDLINE] [KEY_CHECK]
mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
[[^]MAY_EXEC]
fsmagic:= hex value
@@ -113,3 +113,8 @@ Description:
Example of appraise rule allowing modsig appended signatures:
appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig
+
+ Example of measure rule using KEY_CHECK to measure all keys:
+
+ measure func=KEY_CHECK
+
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index df4ca482fb53..fe6c698617bd 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -193,6 +193,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
hook(KEXEC_INITRAMFS_CHECK) \
hook(POLICY_CHECK) \
hook(KEXEC_CMDLINE) \
+ hook(KEY_CHECK) \
hook(MAX_CHECK)
#define __ima_hook_enumify(ENUM) ENUM,
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index f19a895ad7cd..1525a28fd705 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -373,7 +373,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
{
int i;
- if (func == KEXEC_CMDLINE) {
+ if ((func == KEXEC_CMDLINE) || (func == KEY_CHECK)) {
if ((rule->flags & IMA_FUNC) && (rule->func == func))
return true;
return false;
@@ -997,6 +997,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
entry->func = POLICY_CHECK;
else if (strcmp(args[0].from, "KEXEC_CMDLINE") == 0)
entry->func = KEXEC_CMDLINE;
+ else if (strcmp(args[0].from, "KEY_CHECK") == 0)
+ entry->func = KEY_CHECK;
else
result = -EINVAL;
if (!result)
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v7 4/5] IMA: Add support to limit measuring keys
From: Mimi Zohar @ 2019-11-14 14:37 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, dhowells, matthewgarrett, sashal,
jamorris, linux-integrity, linux-security-module, keyrings,
linux-kernel
In-Reply-To: <20191114031202.18012-5-nramas@linux.microsoft.com>
On Wed, 2019-11-13 at 19:12 -0800, Lakshmi Ramasubramanian wrote:
> +/**
> + * ima_match_keyring - determine whether the keyring matches the measure rule
> + * @rule: a pointer to a rule
> + * @keyring: name of the keyring to match against the measure rule
> + *
> + * If the measure action for KEY_CHECK does not specify keyrings=
> + * option then return true (Measure all keys).
> + * Else, return true if the given keyring name is present in
> + * the keyrings= option. False, otherwise.
> + */
> +static bool ima_match_keyring(struct ima_rule_entry *rule,
> + const char *keyring)
> +{
> + if ((keyring == NULL) || (rule->keyrings == NULL)
> + return true;
If the policy requires matching a specific keyring, then the "keyring"
needs to match. The logic, here, isn't quite right.
> + else
> + return (strstr(rule->keyrings, keyring) != NULL);
if (rule->keyrings) {
if (!keyring)
return false;
return (strstr(rule->keyrings, keyring) != NULL);
}
return true;
Keyrings may be created by userspace with any name (e.g. foo, foobar,
...). A keyring name might be a subset of another keyring name. For
example, with the policy "keyrings=foobar", keys being loaded on "foo"
would also be measured. Using strstr() will not achieve what is
needed.
Mimi
> +}
> +
> /**
> * ima_match_rules - determine whether an inode matches the measure rule.
> * @rule: a pointer to a rule
> @@ -364,18 +384,23 @@ int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
> * @secid: the secid of the task to be validated
> * @func: LIM hook identifier
> * @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC)
> + * @keyring: keyring name to check in policy for KEY_CHECK func
> *
> * Returns true on rule match, false on failure.
> */
> static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> const struct cred *cred, u32 secid,
> - enum ima_hooks func, int mask)
> + enum ima_hooks func, int mask,
> + const char *keyring)
> {
> int i;
>
> if ((func == KEXEC_CMDLINE) || (func == KEY_CHECK)) {
> - if ((rule->flags & IMA_FUNC) && (rule->func == func))
> + if ((rule->flags & IMA_FUNC) && (rule->func == func)) {
> + if (func == KEY_CHECK)
> + return ima_match_keyring(rule, keyring);
> return true;
> + }
> return false;
> }
> if ((rule->flags & IMA_FUNC) &&
^ permalink raw reply
* Re: [PATCH v7 3/5] KEYS: Call the IMA hook to measure keys
From: Mimi Zohar @ 2019-11-14 14:54 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, dhowells, matthewgarrett, sashal,
jamorris, linux-integrity, linux-security-module, keyrings,
linux-kernel
In-Reply-To: <20191114031202.18012-4-nramas@linux.microsoft.com>
On Wed, 2019-11-13 at 19:12 -0800, Lakshmi Ramasubramanian wrote:
> Call the IMA hook from key_create_or_update function to measure
> the key when a new key is created or an existing key is updated.
>
> This patch adds the call to the IMA hook from key_create_or_update
> function to measure the key on key create or update.
>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
No need to Cc David Howells on the entire patch set. Just Cc him,
here, after your tag.
With this patch, keys are now being measured. With the boot command
line, we can verify the measurement entry against /proc/cmdline. How
can the key measurement entry be verified? Please include that
information, here, in this patch description.
Also, can the key data, now included in the measurement list, be used
to verify signatures in the ima-sig or ima-modsig templates? Is there
a way of correlating a signature with a key? Perhaps include a
kselftest as an example.
Mimi
> ---
> include/linux/ima.h | 13 +++++++++++++
> security/keys/key.c | 7 +++++++
> 2 files changed, 20 insertions(+)
>
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 6d904754d858..0af88b781ab6 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -25,6 +25,12 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
> extern void ima_post_path_mknod(struct dentry *dentry);
> extern void ima_kexec_cmdline(const void *buf, int size);
>
> +#ifdef CONFIG_KEYS
> +extern void ima_post_key_create_or_update(struct key *keyring,
> + struct key *key,
> + unsigned long flags, bool create);
> +#endif
> +
> #ifdef CONFIG_IMA_KEXEC
> extern void ima_add_kexec_buffer(struct kimage *image);
> #endif
> @@ -101,6 +107,13 @@ static inline void ima_add_kexec_buffer(struct kimage *image)
> {}
> #endif
>
> +#ifndef CONFIG_KEYS
> +static inline void ima_post_key_create_or_update(struct key *keyring,
> + struct key *key,
> + unsigned long flags,
> + bool create) {}
> +#endif
> +
> #ifdef CONFIG_IMA_APPRAISE
> extern bool is_ima_appraise_enabled(void);
> extern void ima_inode_post_setattr(struct dentry *dentry);
> diff --git a/security/keys/key.c b/security/keys/key.c
> index 764f4c57913e..a0f1e7b3b8b9 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -13,6 +13,7 @@
> #include <linux/security.h>
> #include <linux/workqueue.h>
> #include <linux/random.h>
> +#include <linux/ima.h>
> #include <linux/err.h>
> #include "internal.h"
>
> @@ -936,6 +937,8 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
> goto error_link_end;
> }
>
> + ima_post_key_create_or_update(keyring, key, flags, true);
> +
> key_ref = make_key_ref(key, is_key_possessed(keyring_ref));
>
> error_link_end:
> @@ -965,6 +968,10 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
> }
>
> key_ref = __key_update(key_ref, &prep);
> +
> + if (!IS_ERR(key_ref))
> + ima_post_key_create_or_update(keyring, key, flags, false);
> +
> goto error_free_prep;
> }
> EXPORT_SYMBOL(key_create_or_update);
^ permalink raw reply
* Re: [PATCH] tpm_tis: Move setting of TPM_CHIP_FLAG_IRQ into tpm_tis_probe_irq_single
From: Jarkko Sakkinen @ 2019-11-14 16:41 UTC (permalink / raw)
To: Stefan Berger
Cc: linux-integrity, jsnitsel, linux-kernel, linux-security-module,
Stefan Berger
In-Reply-To: <20191112202725.3009814-1-stefanb@linux.vnet.ibm.com>
On Tue, Nov 12, 2019 at 03:27:25PM -0500, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
>
> Move the setting of the TPM_CHIP_FLAG_IRQ for irq probing into
> tpm_tis_probe_irq_single before calling tpm_tis_gen_interrupt.
> This move handles error conditions better that may arise if anything
> before fails in tpm_tis_probe_irq_single.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Suggested-by: Jerry Snitselaar <jsnitsel@redhat.com>
What about just changing the condition?
/Jarkko
^ permalink raw reply
* Re: [PATCH] tpm_tis: Move setting of TPM_CHIP_FLAG_IRQ into tpm_tis_probe_irq_single
From: Jarkko Sakkinen @ 2019-11-14 16:44 UTC (permalink / raw)
To: Stefan Berger
Cc: linux-integrity, jsnitsel, linux-kernel, linux-security-module,
Stefan Berger
In-Reply-To: <20191114164151.GB9528@linux.intel.com>
On Thu, Nov 14, 2019 at 06:41:51PM +0200, Jarkko Sakkinen wrote:
> On Tue, Nov 12, 2019 at 03:27:25PM -0500, Stefan Berger wrote:
> > From: Stefan Berger <stefanb@linux.ibm.com>
> >
> > Move the setting of the TPM_CHIP_FLAG_IRQ for irq probing into
> > tpm_tis_probe_irq_single before calling tpm_tis_gen_interrupt.
> > This move handles error conditions better that may arise if anything
> > before fails in tpm_tis_probe_irq_single.
> >
> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > Suggested-by: Jerry Snitselaar <jsnitsel@redhat.com>
>
> What about just changing the condition?
Also cannot take this since it is not a bug (no fixes tag).
/Jarkko
^ permalink raw reply
* Re: [PATCH v7 4/5] IMA: Add support to limit measuring keys
From: Lakshmi Ramasubramanian @ 2019-11-14 18:18 UTC (permalink / raw)
To: Mimi Zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <1573742237.4793.30.camel@linux.ibm.com>
On 11/14/2019 6:37 AM, Mimi Zohar wrote:
> Keyrings may be created by userspace with any name (e.g. foo, foobar,
> ...). A keyring name might be a subset of another keyring name. For
> example, with the policy "keyrings=foobar", keys being loaded on "foo"
> would also be measured. Using strstr() will not achieve what is
> needed.
>
> Mimi
Very good catch - I missed that :(
Will fix and send an update.
thanks,
-lakshmi
^ permalink raw reply
* Re: [PATCH v7 3/5] KEYS: Call the IMA hook to measure keys
From: Lakshmi Ramasubramanian @ 2019-11-14 18:24 UTC (permalink / raw)
To: Mimi Zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <1573743267.4793.43.camel@linux.ibm.com>
On 11/14/2019 6:54 AM, Mimi Zohar wrote:
> No need to Cc David Howells on the entire patch set. Just Cc him,
> here, after your tag.
ok
> With this patch, keys are now being measured. With the boot command
> line, we can verify the measurement entry against /proc/cmdline. How
> can the key measurement entry be verified? Please include that
> information, here, in this patch description.
Glad you could verify measurement of keys. Thanks a lot for trying it.
Will add information on testing\validating the feature.
> Also, can the key data, now included in the measurement list, be used
> to verify signatures in the ima-sig or ima-modsig templates? Is there
> a way of correlating a signature with a key? Perhaps include a
> kselftest as an example.
>
> Mimi
I am not familiar with kselftest. Will take a look and see if it'd be
possible to correlate a signature with a key.
thanks,
-lakshmi
^ permalink raw reply
* Re: [PATCH v7 2/5] IMA: Define an IMA hook to measure keys
From: Lakshmi Ramasubramanian @ 2019-11-14 18:30 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191114031202.18012-3-nramas@linux.microsoft.com>
On 11/13/19 7:11 PM, Lakshmi Ramasubramanian wrote:
> The IMA hook is defined in a new file namely ima_asymmetric_keys.c
> which is built only if CONFIG_KEYS is enabled.
I think instead of CONFIG_KEYS I should use
CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE for ima_asymmetric_keys.c since
this config is enabled only when both CONFIG_ASYMMETRIC_KEY_TYPE and
CONFIG_KEYS are enabled.
Please see below taken from "crypto/asymmetric_keys/Kconfig"
# SPDX-License-Identifier: GPL-2.0
menuconfig ASYMMETRIC_KEY_TYPE
bool "Asymmetric (public-key cryptographic) key type"
depends on KEYS
help
This option provides support for a key type that holds the
data for
the asymmetric keys used for public key cryptographic
operations such
as encryption, decryption, signature generation and signature
verification.
if ASYMMETRIC_KEY_TYPE
config ASYMMETRIC_PUBLIC_KEY_SUBTYPE
tristate "Asymmetric public-key crypto algorithm subtype"
endif # ASYMMETRIC_KEY_TYPE
thanks,
-lakshmi
^ permalink raw reply
* Investment opportunity
From: Peter Wong @ 2019-11-15 11:50 UTC (permalink / raw)
To: linux-security-module
Greetings,
Find attached email very confidential. reply for more details
Thanks.
Peter Wong
----------------------------------------------------
This email was sent by the shareware version of Postman Professional.
^ permalink raw reply
* [PATCH -next] x86/efi: remove unused variables
From: YueHaibing @ 2019-11-15 13:08 UTC (permalink / raw)
To: jmorris, serge, nayna, zohar, dhowells, jwboyer, yuehaibing
Cc: linux-security-module, linux-kernel
commit ad723674d675 ("x86/efi: move common keyring
handler functions to new file") leave this unused.
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
security/integrity/platform_certs/load_uefi.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
index 4369204..111898a 100644
--- a/security/integrity/platform_certs/load_uefi.c
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -11,11 +11,6 @@
#include "../integrity.h"
#include "keyring_handler.h"
-static efi_guid_t efi_cert_x509_guid __initdata = EFI_CERT_X509_GUID;
-static efi_guid_t efi_cert_x509_sha256_guid __initdata =
- EFI_CERT_X509_SHA256_GUID;
-static efi_guid_t efi_cert_sha256_guid __initdata = EFI_CERT_SHA256_GUID;
-
/*
* Look to see if a UEFI variable called MokIgnoreDB exists and return true if
* it does.
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v7 3/5] KEYS: Call the IMA hook to measure keys
From: Mimi Zohar @ 2019-11-15 13:14 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, dhowells, matthewgarrett, sashal,
jamorris, linux-integrity, linux-security-module, keyrings,
linux-kernel
In-Reply-To: <24262d82-c90b-b64d-f237-9ef038f38d0e@linux.microsoft.com>
On Thu, 2019-11-14 at 10:24 -0800, Lakshmi Ramasubramanian wrote:
> On 11/14/2019 6:54 AM, Mimi Zohar wrote:
> > With this patch, keys are now being measured. With the boot command
> > line, we can verify the measurement entry against /proc/cmdline. How
> > can the key measurement entry be verified? Please include that
> > information, here, in this patch description.
>
> Glad you could verify measurement of keys. Thanks a lot for trying it.
>
> Will add information on testing\validating the feature.
Thank you.
>
> > Also, can the key data, now included in the measurement list, be used
> > to verify signatures in the ima-sig or ima-modsig templates? Is there
> > a way of correlating a signature with a key? Perhaps include a
> > kselftest as an example.
>
> I am not familiar with kselftest. Will take a look and see if it'd be
> possible to correlate a signature with a key.
I'd like the measurement list to be self contained, allowing a
regression test, for example, to walk the measurement list, verifying
the file signatures stored in the measurement list based on the key
measurement(s).
It isn't so much about Kselftest, but implementing a regression test
(eg. Kselftest, LTP, ima-evm-utils, ...) as a PoC, in order to know
that the key measurement contains everything needed to identify the
key (eg. keyid, certificate serial number, ...) and verify file
signatures.
Mimi
^ permalink raw reply
* Re: [pipe] d60337eff1: BUG:kernel_NULL_pointer_dereference,address
From: David Howells @ 2019-11-15 13:28 UTC (permalink / raw)
To: kernel test robot
Cc: dhowells, torvalds, Rasmus Villemoes, Greg Kroah-Hartman,
Peter Zijlstra, nicolas.dichtel, raven, Christian Brauner,
keyrings, linux-usb, linux-block, linux-security-module,
linux-fsdevel, linux-api, linux-kernel, lkp
In-Reply-To: <20191110031348.GE29418@shao2-debian>
kernel test robot <lkp@intel.com> wrote:
> [ 9.423019] BUG: kernel NULL pointer dereference, address: 0000000000000008
> [ 9.425646] #PF: supervisor read access in kernel mode
> [ 9.427714] #PF: error_code(0x0000) - not-present page
> [ 9.429851] PGD 80000001fb937067 P4D 80000001fb937067 PUD 1739e1067 PMD 0
> [ 9.432468] Oops: 0000 [#1] SMP PTI
> [ 9.434064] CPU: 0 PID: 178 Comm: cat Not tainted 5.4.0-rc5-00353-gd60337eff18a3 #1
> [ 9.437139] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
> [ 9.440439] RIP: 0010:iov_iter_get_pages_alloc+0x2a8/0x400
Can you tell me if the following change fixes it for you?
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -404,7 +404,7 @@ static size_t copy_page_to_iter_pipe(struct page *page, size_t offset, size_t by
buf->offset = offset;
buf->len = bytes;
- pipe->head = i_head;
+ pipe->head = i_head + 1;
i->iov_offset = offset + bytes;
i->head = i_head;
out:
Attached is a test program that can induce some a bug in
copy_page_to_iter_pipe() where I forgot to increment the new head when
assigning it to pipe->head.
David
---
#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>
#include <err.h>
#include <sys/wait.h>
static char buf[256 * 1024] __attribute__((aligned(512)));
static char *filename;
static int pipe_wfd = -1;
static void cleanup(void)
{
close(pipe_wfd);
}
static void cleanup_child(void)
{
int w;
wait(&w);
}
int child(int fd)
{
ssize_t r;
do {
r = read(fd, buf, 256 * 1024);
if (r == -1)
err(1, "read");
} while (r != 0);
if (close(fd) == -1)
err(1, "close");
return 0;
}
int main(int argc, char **argv)
{
ssize_t n;
loff_t offset;
size_t len;
pid_t pid;
int fd, pfd[2];
if (argc != 2) {
fprintf(stderr, "Format: %s <file>\n", argv[1]);
exit(2);
}
filename = argv[1];
if (pipe(pfd) == -1)
err(1, "pipe");
pipe_wfd = pfd[1];
pid = fork();
switch (pid) {
case -1:
err(1, "fork");
case 0:
close(pfd[1]);
return child(pfd[0]);
default:
close(pfd[0]);
atexit(cleanup_child);
break;
}
fd = open(filename, O_RDONLY);
if (fd == -1)
err(1, "%s", filename);
atexit(cleanup);
len = 256 * 1024;
offset = 0;
do {
n = splice(fd, &offset, pfd[1], NULL, 256 * 1024, 0);
if (n == -1)
err(1, "splice");
} while (len -= n, len > 0);
if (close(pfd[1]) == -1)
err(1, "close/p");
if (close(fd) == -1)
err(1, "close/f");
return 0;
}
^ permalink raw reply
* Re: [pipe] d60337eff1: BUG:kernel_NULL_pointer_dereference,address
From: David Howells @ 2019-11-15 16:22 UTC (permalink / raw)
To: kernel test robot
Cc: dhowells, torvalds, Rasmus Villemoes, Greg Kroah-Hartman,
Peter Zijlstra, nicolas.dichtel, raven, Christian Brauner,
keyrings, linux-usb, linux-block, linux-security-module,
linux-fsdevel, linux-api, linux-kernel, lkp
In-Reply-To: <9279.1573824532@warthog.procyon.org.uk>
Actually, no, this is the fix:
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 7006b5b2106d..be2fc5793ddd 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -537,7 +537,7 @@ static size_t push_pipe(struct iov_iter *i, size_t size,
buf->ops = &default_pipe_buf_ops;
buf->page = page;
buf->offset = 0;
- buf->len = max_t(ssize_t, left, PAGE_SIZE);
+ buf->len = min_t(ssize_t, left, PAGE_SIZE);
left -= buf->len;
iter_head++;
pipe->head = iter_head;
David
^ permalink raw reply related
* Re: [PATCH] tpm_tis: Move setting of TPM_CHIP_FLAG_IRQ into tpm_tis_probe_irq_single
From: Stefan Berger @ 2019-11-16 14:32 UTC (permalink / raw)
To: Jarkko Sakkinen, Stefan Berger
Cc: linux-integrity, jsnitsel, linux-kernel, linux-security-module
In-Reply-To: <20191114164426.GC9528@linux.intel.com>
On 11/14/19 11:44 AM, Jarkko Sakkinen wrote:
> On Thu, Nov 14, 2019 at 06:41:51PM +0200, Jarkko Sakkinen wrote:
>> On Tue, Nov 12, 2019 at 03:27:25PM -0500, Stefan Berger wrote:
>>> From: Stefan Berger <stefanb@linux.ibm.com>
>>>
>>> Move the setting of the TPM_CHIP_FLAG_IRQ for irq probing into
>>> tpm_tis_probe_irq_single before calling tpm_tis_gen_interrupt.
>>> This move handles error conditions better that may arise if anything
>>> before fails in tpm_tis_probe_irq_single.
>>>
>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>> Suggested-by: Jerry Snitselaar <jsnitsel@redhat.com>
>> What about just changing the condition?
> Also cannot take this since it is not a bug (no fixes tag).
I'll repost but will wait until Jerry has tested it on that machine.
Stefan
>
> /Jarkko
^ permalink raw reply
* Investment opportunity
From: Peter Wong @ 2019-11-17 14:58 UTC (permalink / raw)
To: linux-security-module
Greetings,
Find attached email very confidential. reply for more details
Thanks.
Peter Wong
----------------------------------------------------
This email was sent by the shareware version of Postman Professional.
^ permalink raw reply
* Re: [PATCH] tpm_tis: Move setting of TPM_CHIP_FLAG_IRQ into tpm_tis_probe_irq_single
From: Jerry Snitselaar @ 2019-11-17 18:08 UTC (permalink / raw)
To: Stefan Berger
Cc: Jarkko Sakkinen, Stefan Berger, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <185664a9-58f2-2a4b-4e6b-8d7750a35690@linux.ibm.com>
On Sat Nov 16 19, Stefan Berger wrote:
>On 11/14/19 11:44 AM, Jarkko Sakkinen wrote:
>>On Thu, Nov 14, 2019 at 06:41:51PM +0200, Jarkko Sakkinen wrote:
>>>On Tue, Nov 12, 2019 at 03:27:25PM -0500, Stefan Berger wrote:
>>>>From: Stefan Berger <stefanb@linux.ibm.com>
>>>>
>>>>Move the setting of the TPM_CHIP_FLAG_IRQ for irq probing into
>>>>tpm_tis_probe_irq_single before calling tpm_tis_gen_interrupt.
>>>>This move handles error conditions better that may arise if anything
>>>>before fails in tpm_tis_probe_irq_single.
>>>>
>>>>Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>>>Suggested-by: Jerry Snitselaar <jsnitsel@redhat.com>
>>>What about just changing the condition?
>>Also cannot take this since it is not a bug (no fixes tag).
>
>I'll repost but will wait until Jerry has tested it on that machine.
>
> Stefan
>
Sorry, I'm still waiting to hear back from the reporter, and logistics
for some reason has been silent on my request so far.
>
>>
>>/Jarkko
>
>
^ permalink raw reply
* Re: [LKP] Re: [pipe] d60337eff1: BUG:kernel_NULL_pointer_dereference,address
From: kernel test robot @ 2019-11-18 7:53 UTC (permalink / raw)
To: David Howells, kernel test robot
Cc: torvalds, Rasmus Villemoes, Greg Kroah-Hartman, Peter Zijlstra,
nicolas.dichtel, raven, Christian Brauner, keyrings, linux-usb,
linux-block, linux-security-module, linux-fsdevel, linux-api,
linux-kernel, lkp
In-Reply-To: <6853.1573834946@warthog.procyon.org.uk>
Hi David,
Yes, it can fix the problem.
Best Regards,
Rong Chen
On 11/16/2019 12:22 AM, David Howells wrote:
> Actually, no, this is the fix:
>
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 7006b5b2106d..be2fc5793ddd 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -537,7 +537,7 @@ static size_t push_pipe(struct iov_iter *i, size_t size,
> buf->ops = &default_pipe_buf_ops;
> buf->page = page;
> buf->offset = 0;
> - buf->len = max_t(ssize_t, left, PAGE_SIZE);
> + buf->len = min_t(ssize_t, left, PAGE_SIZE);
> left -= buf->len;
> iter_head++;
> pipe->head = iter_head;
>
> David
> _______________________________________________
> LKP mailing list -- lkp@lists.01.org
> To unsubscribe send an email to lkp-leave@lists.01.org
^ permalink raw reply
* Re: [PATCH] tpm_tis: Move setting of TPM_CHIP_FLAG_IRQ into tpm_tis_probe_irq_single
From: Jarkko Sakkinen @ 2019-11-18 17:59 UTC (permalink / raw)
To: Stefan Berger
Cc: Stefan Berger, linux-integrity, jsnitsel, linux-kernel,
linux-security-module
In-Reply-To: <185664a9-58f2-2a4b-4e6b-8d7750a35690@linux.ibm.com>
On Sat, Nov 16, 2019 at 09:32:06AM -0500, Stefan Berger wrote:
> On 11/14/19 11:44 AM, Jarkko Sakkinen wrote:
> > On Thu, Nov 14, 2019 at 06:41:51PM +0200, Jarkko Sakkinen wrote:
> > > On Tue, Nov 12, 2019 at 03:27:25PM -0500, Stefan Berger wrote:
> > > > From: Stefan Berger <stefanb@linux.ibm.com>
> > > >
> > > > Move the setting of the TPM_CHIP_FLAG_IRQ for irq probing into
> > > > tpm_tis_probe_irq_single before calling tpm_tis_gen_interrupt.
> > > > This move handles error conditions better that may arise if anything
> > > > before fails in tpm_tis_probe_irq_single.
> > > >
> > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > > Suggested-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > What about just changing the condition?
> > Also cannot take this since it is not a bug (no fixes tag).
>
> I'll repost but will wait until Jerry has tested it on that machine.
OK, great, thank you.
This is really needs some reasoning on why this was the right way to
fix the issue. In addition, a source code comment might make sense.
/Jarkko
^ permalink raw reply
* [PATCH] x86/mtrr: Require CAP_SYS_ADMIN for all access
From: Kees Cook @ 2019-11-18 21:09 UTC (permalink / raw)
To: Borislav Petkov
Cc: Thomas Gleixner, Zhang Xiaoxu, linux-kernel, x86, tyhicks,
colin.king, linux-security-module, Matthew Garrett
Zhang Xiaoxu noted that physical address locations for MTRR were
visible to non-root users, which could be considered an information
leak. In discussing[1] the options for solving this, it sounded like
just moving the capable check into open() was the first step. If this
breaks userspace, then we will have a test case for the more conservative
approaches discussed in the thread. In summary:
- MTRR should check capabilities at open time (or retain the
checks on the opener's permissions for later checks).
- changing the DAC permissions might break something that expects to
open mtrr when not uid 0.
- if we leave the DAC permissions alone and just move the capable check
to the opener, we should get the desired protection. (i.e. check
against CAP_SYS_ADMIN not just the wider uid 0.)
- if that still breaks things, as in userspace expects to be able to
read other parts of the file as non-uid-0 and non-CAP_SYS_ADMIN, then
we need to censor the contents using the opener's permissions. For
example, as done in other /proc cases, like commit 51d7b120418e
("/proc/iomem: only expose physical resource addresses to privileged
users").
[1] https://lore.kernel.org/lkml/201911110934.AC5BA313@keescook/
Reported-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
arch/x86/kernel/cpu/mtrr/if.c | 21 ++-------------------
1 file changed, 2 insertions(+), 19 deletions(-)
diff --git a/arch/x86/kernel/cpu/mtrr/if.c b/arch/x86/kernel/cpu/mtrr/if.c
index 4d36dcc1cf87..9c86aeae1b14 100644
--- a/arch/x86/kernel/cpu/mtrr/if.c
+++ b/arch/x86/kernel/cpu/mtrr/if.c
@@ -101,9 +101,6 @@ mtrr_write(struct file *file, const char __user *buf, size_t len, loff_t * ppos)
int length;
size_t linelen;
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
-
memset(line, 0, LINE_SIZE);
len = min_t(size_t, len, LINE_SIZE - 1);
@@ -226,8 +223,6 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
#ifdef CONFIG_COMPAT
case MTRRIOC32_ADD_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
err =
mtrr_file_add(sentry.base, sentry.size, sentry.type, true,
file, 0);
@@ -236,24 +231,18 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
#ifdef CONFIG_COMPAT
case MTRRIOC32_SET_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
err = mtrr_add(sentry.base, sentry.size, sentry.type, false);
break;
case MTRRIOC_DEL_ENTRY:
#ifdef CONFIG_COMPAT
case MTRRIOC32_DEL_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
err = mtrr_file_del(sentry.base, sentry.size, file, 0);
break;
case MTRRIOC_KILL_ENTRY:
#ifdef CONFIG_COMPAT
case MTRRIOC32_KILL_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
err = mtrr_del(-1, sentry.base, sentry.size);
break;
case MTRRIOC_GET_ENTRY:
@@ -279,8 +268,6 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
#ifdef CONFIG_COMPAT
case MTRRIOC32_ADD_PAGE_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
err =
mtrr_file_add(sentry.base, sentry.size, sentry.type, true,
file, 1);
@@ -289,8 +276,6 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
#ifdef CONFIG_COMPAT
case MTRRIOC32_SET_PAGE_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
err =
mtrr_add_page(sentry.base, sentry.size, sentry.type, false);
break;
@@ -298,16 +283,12 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
#ifdef CONFIG_COMPAT
case MTRRIOC32_DEL_PAGE_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
err = mtrr_file_del(sentry.base, sentry.size, file, 1);
break;
case MTRRIOC_KILL_PAGE_ENTRY:
#ifdef CONFIG_COMPAT
case MTRRIOC32_KILL_PAGE_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
err = mtrr_del_page(-1, sentry.base, sentry.size);
break;
case MTRRIOC_GET_PAGE_ENTRY:
@@ -381,6 +362,8 @@ static int mtrr_open(struct inode *inode, struct file *file)
return -EIO;
if (!mtrr_if->get)
return -ENXIO;
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
return single_open(file, mtrr_seq_show, NULL);
}
--
2.17.1
--
Kees Cook
^ permalink raw reply related
* Re: [PATCH linux-kselftest/test v2] apparmor: add AppArmor KUnit tests for policy unpack
From: Brendan Higgins @ 2019-11-19 0:34 UTC (permalink / raw)
To: Kees Cook
Cc: shuah, John Johansen, jmorris, serge, Alan Maguire, Iurii Zaikin,
David Gow, Luis Chamberlain, Theodore Ts'o,
Linux Kernel Mailing List, linux-security-module,
KUnit Development, open list:KERNEL SELFTEST FRAMEWORK,
Mike Salvatore
In-Reply-To: <20191107233337.GA191231@google.com>
On Thu, Nov 7, 2019 at 3:33 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> On Wed, Nov 06, 2019 at 09:18:27AM -0800, Kees Cook wrote:
> > On Tue, Nov 05, 2019 at 04:43:29PM -0800, Brendan Higgins wrote:
> > > From: Mike Salvatore <mike.salvatore@canonical.com>
> > >
> > > Add KUnit tests to test AppArmor unpacking of userspace policies.
> > > AppArmor uses a serialized binary format for loading policies. To find
> > > policy format documentation see
> > > Documentation/admin-guide/LSM/apparmor.rst.
> > >
> > > In order to write the tests against the policy unpacking code, some
> > > static functions needed to be exposed for testing purposes. One of the
> > > goals of this patch is to establish a pattern for which testing these
> > > kinds of functions should be done in the future.
> > >
> > > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > > Signed-off-by: Mike Salvatore <mike.salvatore@canonical.com>
> > > ---
> > > security/apparmor/Kconfig | 16 +
> > > security/apparmor/policy_unpack.c | 4 +
> > > security/apparmor/policy_unpack_test.c | 607 +++++++++++++++++++++++++
> > > 3 files changed, 627 insertions(+)
> > > create mode 100644 security/apparmor/policy_unpack_test.c
> > >
> > > diff --git a/security/apparmor/Kconfig b/security/apparmor/Kconfig
> > > index d8b1a360a6368..78a33ccac2574 100644
> > > --- a/security/apparmor/Kconfig
> > > +++ b/security/apparmor/Kconfig
> > > @@ -66,3 +66,19 @@ config SECURITY_APPARMOR_DEBUG_MESSAGES
> > > Set the default value of the apparmor.debug kernel parameter.
> > > When enabled, various debug messages will be logged to
> > > the kernel message buffer.
> > > +
> > > +config SECURITY_APPARMOR_KUNIT_TEST
> > > + bool "Build KUnit tests for policy_unpack.c"
> > > + depends on KUNIT && SECURITY_APPARMOR
> > > + help
> > > + This builds the AppArmor KUnit tests.
> > > +
> > > + KUnit tests run during boot and output the results to the debug log
> > > + in TAP format (http://testanything.org/). Only useful for kernel devs
> > > + running KUnit test harness and are not for inclusion into a
> > > + production build.
> > > +
> > > + For more information on KUnit and unit tests in general please refer
> > > + to the KUnit documentation in Documentation/dev-tools/kunit/.
> > > +
> > > + If unsure, say N.
> > > diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
> > > index 8cfc9493eefc7..37c1dd3178fc0 100644
> > > --- a/security/apparmor/policy_unpack.c
> > > +++ b/security/apparmor/policy_unpack.c
> > > @@ -1120,3 +1120,7 @@ int aa_unpack(struct aa_loaddata *udata, struct list_head *lh,
> > >
> > > return error;
> > > }
> > > +
> > > +#ifdef CONFIG_SECURITY_APPARMOR_KUNIT_TEST
> > > +#include "policy_unpack_test.c"
> > > +#endif /* CONFIG_SECURITY_APPARMOR_KUNIT_TEST */
> >
> > To make this even LESS intrusive, the ifdefs could live in ..._test.c.
>
> Less intrusive, yes, but I think I actually like the ifdef here; it
> makes it clear from the source that the test is only a part of the build
> when configured to do so. Nevertheless, I will change it if anyone feels
> strongly about it.
>
> > Also, while I *think* the kernel build system will correctly track this
> > dependency, can you double-check that changes to ..._test.c correctly
> > trigger a recompile of policy_unpack.c?
>
> Yep, just verified, first I ran the tests and everything passed. Then I
> applied the following diff:
>
> diff --git a/security/apparmor/policy_unpack_test.c b/security/apparmor/policy_unpack_test.c
> index 533137f45361c..e1b0670dbdc27 100644
> --- a/security/apparmor/policy_unpack_test.c
> +++ b/security/apparmor/policy_unpack_test.c
> @@ -161,7 +161,7 @@ static void policy_unpack_test_unpack_array_with_name(struct kunit *test)
>
> array_size = unpack_array(puf->e, name);
>
> - KUNIT_EXPECT_EQ(test, array_size, (u16)TEST_ARRAY_SIZE);
> + KUNIT_EXPECT_EQ(test, array_size + 1, (u16)TEST_ARRAY_SIZE);
> KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1);
> }
>
> and reran the tests (to trigger an incremental build) and the test
> failed as expected indicating that the dependency is properly tracked.
Hey Kees,
Since it looks like you already took a pretty close look at this,
would you mind giving me a review?
Thanks!
^ permalink raw reply
* [PATCH] efi: Only print errors about failing to get certs if EFI vars are found
From: Javier Martinez Canillas @ 2019-11-19 9:18 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Jones, Hans de Goede, Javier Martinez Canillas,
David Howells, James Morris, Josh Boyer, Mimi Zohar, Nayna Jain,
Serge E. Hallyn, linux-security-module
If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
But it just assumes that the variables will be present and prints an error
if the certs can't be loaded, even when is possible that the variables may
not exist. For example the MokListRT variable will only be present if shim
is used.
So only print an error message about failing to get the certs list from an
EFI variable if this is found. Otherwise these printed errors just pollute
the kernel ring buffer with confusing messages like the following:
[ 5.427251] Couldn't get size: 0x800000000000000e
[ 5.427261] MODSIGN: Couldn't get UEFI db list
[ 5.428012] Couldn't get size: 0x800000000000000e
[ 5.428023] Couldn't get UEFI MokListRT
Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
security/integrity/platform_certs/load_uefi.c | 31 ++++++++++---------
1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
index 81b19c52832..336fa528359 100644
--- a/security/integrity/platform_certs/load_uefi.c
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -39,16 +39,18 @@ static __init bool uefi_check_ignore_db(void)
* Get a certificate list blob from the named EFI variable.
*/
static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
- unsigned long *size)
+ unsigned long *size, efi_status_t *status)
{
- efi_status_t status;
unsigned long lsize = 4;
unsigned long tmpdb[4];
void *db;
- status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
- if (status != EFI_BUFFER_TOO_SMALL) {
- pr_err("Couldn't get size: 0x%lx\n", status);
+ *status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
+ if (*status == EFI_NOT_FOUND)
+ return NULL;
+
+ if (*status != EFI_BUFFER_TOO_SMALL) {
+ pr_err("Couldn't get size: 0x%lx\n", *status);
return NULL;
}
@@ -56,10 +58,10 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
if (!db)
return NULL;
- status = efi.get_variable(name, guid, NULL, &lsize, db);
- if (status != EFI_SUCCESS) {
+ *status = efi.get_variable(name, guid, NULL, &lsize, db);
+ if (*status != EFI_SUCCESS) {
kfree(db);
- pr_err("Error reading db var: 0x%lx\n", status);
+ pr_err("Error reading db var: 0x%lx\n", *status);
return NULL;
}
@@ -144,6 +146,7 @@ static int __init load_uefi_certs(void)
efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
void *db = NULL, *dbx = NULL, *mok = NULL;
unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
+ efi_status_t status;
int rc = 0;
if (!efi.get_variable)
@@ -153,8 +156,8 @@ static int __init load_uefi_certs(void)
* an error if we can't get them.
*/
if (!uefi_check_ignore_db()) {
- db = get_cert_list(L"db", &secure_var, &dbsize);
- if (!db) {
+ db = get_cert_list(L"db", &secure_var, &dbsize, &status);
+ if (!db && status != EFI_NOT_FOUND) {
pr_err("MODSIGN: Couldn't get UEFI db list\n");
} else {
rc = parse_efi_signature_list("UEFI:db",
@@ -166,8 +169,8 @@ static int __init load_uefi_certs(void)
}
}
- mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
- if (!mok) {
+ mok = get_cert_list(L"MokListRT", &mok_var, &moksize, &status);
+ if (!mok && status != EFI_NOT_FOUND) {
pr_info("Couldn't get UEFI MokListRT\n");
} else {
rc = parse_efi_signature_list("UEFI:MokListRT",
@@ -177,8 +180,8 @@ static int __init load_uefi_certs(void)
kfree(mok);
}
- dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
- if (!dbx) {
+ dbx = get_cert_list(L"dbx", &secure_var, &dbxsize, &status);
+ if (!dbx && status != EFI_NOT_FOUND) {
pr_info("Couldn't get UEFI dbx list\n");
} else {
rc = parse_efi_signature_list("UEFI:dbx",
--
2.23.0
^ permalink raw reply related
* Re: [PATCH] x86/mtrr: Require CAP_SYS_ADMIN for all access
From: Borislav Petkov @ 2019-11-19 10:07 UTC (permalink / raw)
To: Kees Cook
Cc: Thomas Gleixner, Zhang Xiaoxu, linux-kernel, x86, tyhicks,
colin.king, linux-security-module, Matthew Garrett
In-Reply-To: <201911181308.63F06502A1@keescook>
On Mon, Nov 18, 2019 at 01:09:21PM -0800, Kees Cook wrote:
> Zhang Xiaoxu noted that physical address locations for MTRR were
> visible to non-root users, which could be considered an information
> leak. In discussing[1] the options for solving this, it sounded like
> just moving the capable check into open() was the first step. If this
> breaks userspace, then we will have a test case for the more conservative
> approaches discussed in the thread. In summary:
>
> - MTRR should check capabilities at open time (or retain the
> checks on the opener's permissions for later checks).
>
> - changing the DAC permissions might break something that expects to
> open mtrr when not uid 0.
>
> - if we leave the DAC permissions alone and just move the capable check
> to the opener, we should get the desired protection. (i.e. check
> against CAP_SYS_ADMIN not just the wider uid 0.)
>
> - if that still breaks things, as in userspace expects to be able to
> read other parts of the file as non-uid-0 and non-CAP_SYS_ADMIN, then
> we need to censor the contents using the opener's permissions. For
> example, as done in other /proc cases, like commit 51d7b120418e
> ("/proc/iomem: only expose physical resource addresses to privileged
> users").
>
> [1] https://lore.kernel.org/lkml/201911110934.AC5BA313@keescook/
>
> Reported-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> arch/x86/kernel/cpu/mtrr/if.c | 21 ++-------------------
> 1 file changed, 2 insertions(+), 19 deletions(-)
Yap, LGTM, thanks!
Reviewed-by: Borislav Petkov <bp@suse.de>
However, as it has a user-visible impact and it is not an urgent thing
to have in the tree, I'd not queue this now but after the merge window
is done so that we have a maximum time of exposure in linux-next and we
can have ample time to addres fallout.
/me puts it on the list for after the merge window.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH] efi: Only print errors about failing to get certs if EFI vars are found
From: Hans de Goede @ 2019-11-19 10:59 UTC (permalink / raw)
To: Javier Martinez Canillas, linux-kernel
Cc: Peter Jones, David Howells, James Morris, Josh Boyer, Mimi Zohar,
Nayna Jain, Serge E. Hallyn, linux-security-module
In-Reply-To: <20191119091822.276265-1-javierm@redhat.com>
Hi,
On 19-11-2019 10:18, Javier Martinez Canillas wrote:
> If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
> from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
>
> But it just assumes that the variables will be present and prints an error
> if the certs can't be loaded, even when is possible that the variables may
> not exist. For example the MokListRT variable will only be present if shim
> is used.
>
> So only print an error message about failing to get the certs list from an
> EFI variable if this is found. Otherwise these printed errors just pollute
> the kernel ring buffer with confusing messages like the following:
>
> [ 5.427251] Couldn't get size: 0x800000000000000e
> [ 5.427261] MODSIGN: Couldn't get UEFI db list
> [ 5.428012] Couldn't get size: 0x800000000000000e
> [ 5.428023] Couldn't get UEFI MokListRT
>
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Thanks for this, I just noticed a potential issue which I missed
when you send this to me for testing:
>
> ---
>
> security/integrity/platform_certs/load_uefi.c | 31 ++++++++++---------
> 1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> index 81b19c52832..336fa528359 100644
> --- a/security/integrity/platform_certs/load_uefi.c
> +++ b/security/integrity/platform_certs/load_uefi.c
> @@ -39,16 +39,18 @@ static __init bool uefi_check_ignore_db(void)
> * Get a certificate list blob from the named EFI variable.
> */
> static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> - unsigned long *size)
> + unsigned long *size, efi_status_t *status)
> {
> - efi_status_t status;
> unsigned long lsize = 4;
> unsigned long tmpdb[4];
> void *db;
>
> - status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
> - if (status != EFI_BUFFER_TOO_SMALL) {
> - pr_err("Couldn't get size: 0x%lx\n", status);
> + *status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
> + if (*status == EFI_NOT_FOUND)
> + return NULL;
> +
> + if (*status != EFI_BUFFER_TOO_SMALL) {
> + pr_err("Couldn't get size: 0x%lx\n", *status);
> return NULL;
> }
>
> @@ -56,10 +58,10 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> if (!db)
> return NULL;
>
> - status = efi.get_variable(name, guid, NULL, &lsize, db);
> - if (status != EFI_SUCCESS) {
> + *status = efi.get_variable(name, guid, NULL, &lsize, db);
> + if (*status != EFI_SUCCESS) {
> kfree(db);
> - pr_err("Error reading db var: 0x%lx\n", status);
> + pr_err("Error reading db var: 0x%lx\n", *status);
> return NULL;
> }
>
> @@ -144,6 +146,7 @@ static int __init load_uefi_certs(void)
> efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
> void *db = NULL, *dbx = NULL, *mok = NULL;
> unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
> + efi_status_t status;
> int rc = 0;
>
> if (!efi.get_variable)
> @@ -153,8 +156,8 @@ static int __init load_uefi_certs(void)
> * an error if we can't get them.
> */
> if (!uefi_check_ignore_db()) {
> - db = get_cert_list(L"db", &secure_var, &dbsize);
> - if (!db) {
> + db = get_cert_list(L"db", &secure_var, &dbsize, &status);
> + if (!db && status != EFI_NOT_FOUND) {
> pr_err("MODSIGN: Couldn't get UEFI db list\n");
> } else {
> rc = parse_efi_signature_list("UEFI:db",
> @@ -166,8 +169,8 @@ static int __init load_uefi_certs(void)
> }
> }
>
> - mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
> - if (!mok) {
> + mok = get_cert_list(L"MokListRT", &mok_var, &moksize, &status);
> + if (!mok && status != EFI_NOT_FOUND) {
> pr_info("Couldn't get UEFI MokListRT\n");
> } else {
> rc = parse_efi_signature_list("UEFI:MokListRT",
This means that if status == EFI_NOT_FOUND we end up still
trying to parse the signature list, I guess that moksize == 0
or some such is saving us here, but I believe that
this should really be:
if (!mok) {
if (status != EFI_NOT_FOUND)
pr_info("Couldn't get UEFI MokListRT\n");
} else {
rc = parse_efi_signature_list("UEFI:MokListRT",
> @@ -177,8 +180,8 @@ static int __init load_uefi_certs(void)
> kfree(mok);
> }
>
> - dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
> - if (!dbx) {
> + dbx = get_cert_list(L"dbx", &secure_var, &dbxsize, &status);
> + if (!dbx && status != EFI_NOT_FOUND) {
> pr_info("Couldn't get UEFI dbx list\n");
> } else {
> rc = parse_efi_signature_list("UEFI:dbx",
Idem.
Regards,
Hans
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox