Re: [PATCH v8 7/8] ima: check against blacklisted hashes for files with modsig
On Sun, 2019-10-20 at 12:06 -0400, Mimi Zohar wrote: > On Sat, 2019-10-19 at 14:06 -0400, Nayna Jain wrote: > > Asymmetric private keys are used to sign multiple files. The kernel > > currently support checking against blacklisted keys. However, if the > > public key is blacklisted, any file signed by the blacklisted key will > > automatically fail signature verification. We might not want to blacklist > > all the files signed by a particular key, but just a single file. > > Blacklisting the public key is not fine enough granularity. > > > > This patch adds support for checking against the blacklisted hash of the > > file based on the IMA policy. The blacklisted hash is the file hash > > without the appended signature. Defined is a new policy option > > "appraise_flag=check_blacklist". > > Please add an example of how to blacklist a file with an appended > signature. The simplest example that works on x86 as well as Power > would be blacklisting a kernel module. The example should include > calculating the kernel module hash without the appended signature, > enabling the Kconfig option (CONFIG_SYSTEM_BLACKLIST_HASH_LIST), and > the blacklist hash format (eg. "bin:"). And of course, the IMA appraise kernel module policy rule containing "appraise_flag=check_blacklist". thanks, Mimi
Re: [PATCH v8 7/8] ima: check against blacklisted hashes for files with modsig
On Sat, 2019-10-19 at 14:06 -0400, Nayna Jain wrote: > Asymmetric private keys are used to sign multiple files. The kernel > currently support checking against blacklisted keys. However, if the > public key is blacklisted, any file signed by the blacklisted key will > automatically fail signature verification. We might not want to blacklist > all the files signed by a particular key, but just a single file. > Blacklisting the public key is not fine enough granularity. > > This patch adds support for checking against the blacklisted hash of the > file based on the IMA policy. The blacklisted hash is the file hash > without the appended signature. Defined is a new policy option > "appraise_flag=check_blacklist". Please add an example of how to blacklist a file with an appended signature. The simplest example that works on x86 as well as Power would be blacklisting a kernel module. The example should include calculating the kernel module hash without the appended signature, enabling the Kconfig option (CONFIG_SYSTEM_BLACKLIST_HASH_LIST), and the blacklist hash format (eg. "bin:"). thanks, Mimi
Re: [PATCH v8 3/8] powerpc: detect the trusted boot state of the system
On Sat, 2019-10-19 at 14:06 -0400, Nayna Jain wrote:
> While secure boot permits only properly verified signed kernels to be
> booted, trusted boot takes a measurement of the kernel image prior to
> boot that can be subsequently compared against good known values via
> attestation services.
>
Instead of "takes a measurement", either "stores a measurement" or
"calculates the file hash of the kernel image and stores the
measurement prior to boot, that".
> This patch reads the trusted boot state of a PowerNV system. The state
> is used to conditionally enable additional measurement rules in the IMA
> arch-specific policies.
>
> Signed-off-by: Nayna Jain
> ---
> arch/powerpc/include/asm/secure_boot.h | 6 ++
> arch/powerpc/kernel/secure_boot.c | 24
> 2 files changed, 30 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/secure_boot.h
> b/arch/powerpc/include/asm/secure_boot.h
> index 07d0fe0ca81f..a2ff556916c6 100644
> --- a/arch/powerpc/include/asm/secure_boot.h
> +++ b/arch/powerpc/include/asm/secure_boot.h
>
> diff --git a/arch/powerpc/kernel/secure_boot.c
> b/arch/powerpc/kernel/secure_boot.c
> index 99bba7915629..9753470ab08a 100644
> --- a/arch/powerpc/kernel/secure_boot.c
> +++ b/arch/powerpc/kernel/secure_boot.c
> @@ -7,6 +7,17 @@
> #include
> #include
>
> +static struct device_node *get_ppc_fw_sb_node(void)
> +{
> + static const struct of_device_id ids[] = {
> + { .compatible = "ibm,secureboot-v1", },
> + { .compatible = "ibm,secureboot-v2", },
> + {},
> + };
> +
scripts/checkpatch.pl is complaining that secureboot-v1, secureboot-v2
are not documented in the device tree bindings.
Mimi
Re: [PATCH v8 5/8] ima: make process_buffer_measurement() generic
On Sat, 2019-10-19 at 14:06 -0400, Nayna Jain wrote:
> process_buffer_measurement() is limited to measuring the kexec boot
> command line. This patch makes process_buffer_measurement() more
> generic, allowing it to measure other types of buffer data (e.g.
> blacklisted binary hashes or key hashes).
based on "func".
>
> This patch modifies the function to conditionally retrieve the policy
> defined pcr and template based on the func.
This would be done in a subsequent patch, not here.
> @@ -642,19 +642,38 @@ static void process_buffer_measurement(const void *buf,
> int size,
> .filename = eventname,
> .buf = buf,
> .buf_len = size};
> - struct ima_template_desc *template_desc = NULL;
> + struct ima_template_desc *template = NULL;
> struct {
> struct ima_digest_data hdr;
> char digest[IMA_MAX_DIGEST_SIZE];
> } hash = {};
> int violation = 0;
> - int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
> int action = 0;
> + u32 secid;
>
> - action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr,
> - &template_desc);
> - if (!(action & IMA_MEASURE))
> - return;
> + if (func) {
> + security_task_getsecid(current, &secid);
> + action = ima_get_action(NULL, current_cred(), secid, 0, func,
> + &pcr, &template);
> + if (!(action & IMA_MEASURE))
> + return;
> + }
> +
Initially there is no need to test "func". A specific "func" test
would be added as needed.
Mimi
Re: [PATCH v8 7/8] ima: check against blacklisted hashes for files with modsig
On Sat, 2019-10-19 at 14:06 -0400, Nayna Jain wrote:
> diff --git a/Documentation/ABI/testing/ima_policy
> b/Documentation/ABI/testing/ima_policy
> index 29ebe9afdac4..4c97afcc0f3c 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -25,6 +25,7 @@ Description:
> lsm:[[subj_user=] [subj_role=] [subj_type=]
>[obj_user=] [obj_role=] [obj_type=]]
> option: [[appraise_type=]] [template=] [permit_directio]
> + [appraise_flag=[check_blacklist]]
Like the other options, only "[[appraise_flag=]]" should be defined
here. The values should be defined in the "option:" section.
> base: func:=
> [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
> [FIRMWARE_CHECK]
>
> [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index 136ae4e0ee92..7a002b08dde8 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -303,6 +304,36 @@ static int modsig_verify(enum ima_hooks func, const
> struct modsig *modsig,
> return rc;
> }
>
> +/*
> + * ima_blacklist_measurement - Checks whether the binary is blacklisted. If
Please update the function name to reflect the actual function name.
> + * yes, then adds the hash of the blacklisted binary to the measurement list.
Refer to Documentation/process/coding-style.rst section "8)
Commenting" on how to format function comments. Don't start a
sentence with "If yes,".
> + *
> + * Returns -EPERM if the hash is blacklisted.
> + */
> +int ima_check_blacklist(struct integrity_iint_cache *iint,
> + const struct modsig *modsig, int pcr)
> +{
> + enum hash_algo hash_algo;
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c
> index 5380aca2b351..bfaae7a8443a 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1172,6 +1173,11 @@ static int ima_parse_rule(char *rule, struct
> ima_rule_entry *entry)
> else
> result = -EINVAL;
> break;
> + case Opt_appraise_flag:
> + ima_log_string(ab, "appraise_flag", args[0].from);
> + if (strstr(args[0].from, "blacklist"))
> + entry->flags |= IMA_CHECK_BLACKLIST;
> + break;
When adding a new policy rule option, ima_policy_show() needs to be
updated as well.
Mimi
> case Opt_permit_directio:
> entry->flags |= IMA_PERMIT_DIRECTIO;
> break;
>
Re: [PATCH v8 2/8] powerpc/ima: add support to initialize ima policy rules
On Sat, 2019-10-19 at 14:06 -0400, Nayna Jain wrote:
> index ..65d82ee74ea4
> --- /dev/null
> +++ b/arch/powerpc/kernel/ima_arch.c
> @@ -0,0 +1,39 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Nayna Jain
> + */
> +
> +#include
> +#include
> +
> +bool arch_ima_get_secureboot(void)
> +{
> + return is_ppc_secureboot_enabled();
> +}
> +
> +/*
> + * The "secure_rules" are enabled only on "secureboot" enabled systems.
> + * These rules verify the file signatures against known good values.
> + * The "appraise_type=imasig|modsig" option allows the known good signature
> + * to be stored as an xattr or as an appended signature.
Please add another sentence or two as a separate paragraph with an
explanation why the kernel module rule is conditional (eg. Only verify
the appended kernel module signatures once.)
> + */
> +static const char *const secure_rules[] = {
> + "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
> +#ifndef CONFIG_MODULE_SIG_FORCE
> + "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> +#endif
> + NULL
> +};
> +
Mimi
Re: [PATCH v8 4/8] powerpc/ima: add measurement rules to ima arch specific policy
On Sat, 2019-10-19 at 14:06 -0400, Nayna Jain wrote:
> This patch adds the measurement rules to the arch specific policies on
> trusted boot enabled systems.
This version does not add rules to the existing arch specific policy,
but defines an arch specific trusted boot only policy and a combined
secure and trusted boot policy.
>
> Signed-off-by: Nayna Jain
> ---
> arch/powerpc/kernel/ima_arch.c | 34 +-
> 1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
> index 65d82ee74ea4..710872ea8f35 100644
> --- a/arch/powerpc/kernel/ima_arch.c
> +++ b/arch/powerpc/kernel/ima_arch.c
> @@ -26,6 +26,32 @@ static const char *const secure_rules[] = {
> NULL
> };
>
> +/*
> + * The "measure_rules" are enabled only on "trustedboot" enabled systems.
Please update the policy name to reflect the new "trusted_rules" name.
> + * These rules add the kexec kernel image and kernel modules file hashes to
> + * the IMA measurement list.
> + */
> +static const char *const trusted_rules[] = {
> + "measure func=KEXEC_KERNEL_CHECK",
> + "measure func=MODULE_CHECK",
> + NULL
> +};
> +
> +/*
> + * The "secure_and_trusted_rules" contains rules for both the secure boot and
> + * trusted boot. The "template=ima-modsig" option includes the appended
> + * signature, when available, in the IMA measurement list.
> + */
> +static const char *const secure_and_trusted_rules[] = {
> + "measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
> + "measure func=MODULE_CHECK template=ima-modsig",
> + "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
> +#ifndef CONFIG_MODULE_SIG_FORCE
> + "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> +#endif
> + NULL
> +};
> +
> /*
> * Returns the relevant IMA arch-specific policies based on the system secure
> * boot state.
> @@ -33,7 +59,13 @@ static const char *const secure_rules[] = {
> const char *const *arch_get_ima_policy(void)
> {
> if (is_ppc_secureboot_enabled())
> - return secure_rules;
> + if (is_ppc_trustedboot_enabled())
> + return secure_and_trusted_rules;
> + else
> + return secure_rules;
> + else
> + if (is_ppc_trustedboot_enabled())
No need for the "if" statement to be on a separate line. Please
combine the "else" and "if" statements.
Mimi
> + return trusted_rules;
>
> return NULL;
> }
Re: [PATCH v7 7/8] ima: check against blacklisted hashes for files with modsig
On Mon, 2019-10-07 at 21:14 -0400, Nayna Jain wrote:
> Asymmetric private keys are used to sign multiple files. The kernel
> currently support checking against the blacklisted keys. However, if the
> public key is blacklisted, any file signed by the blacklisted key will
> automatically fail signature verification. We might not want to blacklist
> all the files signed by a particular key, but just a single file.
> Blacklisting the public key is not fine enough granularity.
>
> This patch adds support for blacklisting binaries with appended signatures,
> based on the IMA policy. Defined is a new policy option
> "appraise_flag=check_blacklist".
The blacklisted hash is not the same as the file hash, but is the file
hash without the appended signature. Are there tools for calculating
the blacklisted hash? Can you provide an example?
>
> Signed-off-by: Nayna Jain
> ---
> Documentation/ABI/testing/ima_policy | 1 +
> security/integrity/ima/ima.h | 9 +++
> security/integrity/ima/ima_appraise.c | 39 +++
> security/integrity/ima/ima_main.c | 12 ++---
> security/integrity/ima/ima_policy.c | 10 +--
> security/integrity/integrity.h| 1 +
> 6 files changed, 66 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy
> b/Documentation/ABI/testing/ima_policy
> index 29ebe9afdac4..4c97afcc0f3c 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -25,6 +25,7 @@ Description:
> lsm:[[subj_user=] [subj_role=] [subj_type=]
>[obj_user=] [obj_role=] [obj_type=]]
> option: [[appraise_type=]] [template=] [permit_directio]
> + [appraise_flag=[check_blacklist]]
> base: func:=
> [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
> [FIRMWARE_CHECK]
> [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index ed86c1f70d7f..63e20ccc91ce 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -256,6 +256,8 @@ int ima_policy_show(struct seq_file *m, void *v);
> #define IMA_APPRAISE_KEXEC 0x40
>
> #ifdef CONFIG_IMA_APPRAISE
> +int ima_check_blacklist(struct integrity_iint_cache *iint,
> + const struct modsig *modsig, int action, int pcr);
> int ima_appraise_measurement(enum ima_hooks func,
>struct integrity_iint_cache *iint,
>struct file *file, const unsigned char *filename,
> @@ -271,6 +273,13 @@ int ima_read_xattr(struct dentry *dentry,
> struct evm_ima_xattr_data **xattr_value);
>
> #else
> +static inline int ima_check_blacklist(struct integrity_iint_cache *iint,
> + const struct modsig *modsig, int action,
> + int pcr)
> +{
> + return 0;
> +}
> +
> static inline int ima_appraise_measurement(enum ima_hooks func,
> struct integrity_iint_cache *iint,
> struct file *file,
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index 136ae4e0ee92..fe34d64a684c 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -12,6 +12,7 @@
> #include
> #include
> #include
> +#include
>
> #include "ima.h"
>
> @@ -303,6 +304,44 @@ static int modsig_verify(enum ima_hooks func, const
> struct modsig *modsig,
> return rc;
> }
>
> +/*
> + * ima_blacklist_measurement - Checks whether the binary is blacklisted. If
> + * yes, then adds the hash of the blacklisted binary to the measurement list.
> + *
> + * Returns -EPERM if the hash is blacklisted.
> + */
> +int ima_check_blacklist(struct integrity_iint_cache *iint,
> + const struct modsig *modsig, int action, int pcr)
> +{
> + enum hash_algo hash_algo;
> + const u8 *digest = NULL;
> + u32 digestsize = 0;
> + u32 secid;
> + int rc = 0;
> + struct ima_template_desc *template_desc;
> +
> + template_desc = lookup_template_desc("ima-buf");
> + template_desc_init_fields(template_desc->fmt, &(template_desc->fields),
> + &(template_desc->num_fields));
Before using template_desc, make sure that template_desc isn't NULL.
For completeness, check the return code of
template_desc_init_fields()
> +
> + if (!(iint->flags & IMA_CHECK_BLACKLIST))
> + return 0;
Move this check before getting the template_desc and make sure that
modsig isn't NULL.
> +
> + if (iint->flags & IMA_MODSIG_ALLOWED) {
> + security_task_getsecid(current, &secid);
secid isn't being used.
> + ima_get_modsig_digest(mods
Re: [PATCH v7 8/8] powerpc/ima: update ima arch policy to check for blacklist
On Mon, 2019-10-07 at 21:14 -0400, Nayna Jain wrote:
> This patch updates the arch specific policies for PowernV systems
> to add check against blacklisted binary hashes before doing the
> verification.
This sentence explains how you're doing something. A simple tweak in
the wording provides the motivation.
^to make sure that the binary hash is not blacklisted.
>
> Signed-off-by: Nayna Jain
Reviewed-by: Mimi Zohar
> ---
> arch/powerpc/kernel/ima_arch.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
> index 88bfe4a1a9a5..4fa41537b846 100644
> --- a/arch/powerpc/kernel/ima_arch.c
> +++ b/arch/powerpc/kernel/ima_arch.c
> @@ -25,9 +25,9 @@ bool arch_ima_get_secureboot(void)
> static const char *const arch_rules[] = {
> "measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
> "measure func=MODULE_CHECK template=ima-modsig",
> - "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
> + "appraise func=KEXEC_KERNEL_CHECK appraise_flag=check_blacklist
> appraise_type=imasig|modsig",
> #if !IS_ENABLED(CONFIG_MODULE_SIG_FORCE)
> - "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> + "appraise func=MODULE_CHECK appraise_flag=check_blacklist
> appraise_type=imasig|modsig",
> #endif
> NULL
> };
Re: [PATCH v7 6/8] certs: add wrapper function to check blacklisted binary hash
On Mon, 2019-10-07 at 21:14 -0400, Nayna Jain wrote:
> The existing is_hash_blacklisted() function returns -EKEYREJECTED
> error code for both the blacklisted keys and binaries.
>
> This patch adds a wrapper function is_binary_blacklisted() to check
> against binary hashes and returns -EPERM.
>
> Signed-off-by: Nayna Jain
This patch description describes what you're doing, not the
motivation.
Reviewed-by: Mimi Zohar
> ---
> certs/blacklist.c | 9 +
> include/keys/system_keyring.h | 6 ++
> 2 files changed, 15 insertions(+)
>
> diff --git a/certs/blacklist.c b/certs/blacklist.c
> index ec00bf337eb6..6514f9ebc943 100644
> --- a/certs/blacklist.c
> +++ b/certs/blacklist.c
> @@ -135,6 +135,15 @@ int is_hash_blacklisted(const u8 *hash, size_t hash_len,
> const char *type)
> }
> EXPORT_SYMBOL_GPL(is_hash_blacklisted);
>
> +int is_binary_blacklisted(const u8 *hash, size_t hash_len)
> +{
> + if (is_hash_blacklisted(hash, hash_len, "bin") == -EKEYREJECTED)
> + return -EPERM;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(is_binary_blacklisted);
> +
> /*
> * Initialise the blacklist
> */
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index c1a96fdf598b..fb8b07daa9d1 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -35,12 +35,18 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
> extern int mark_hash_blacklisted(const char *hash);
> extern int is_hash_blacklisted(const u8 *hash, size_t hash_len,
> const char *type);
> +extern int is_binary_blacklisted(const u8 *hash, size_t hash_len);
> #else
> static inline int is_hash_blacklisted(const u8 *hash, size_t hash_len,
> const char *type)
> {
> return 0;
> }
> +
> +static inline int is_binary_blacklisted(const u8 *hash, size_t hash_len)
> +{
> + return 0;
> +}
> #endif
>
> #ifdef CONFIG_IMA_BLACKLIST_KEYRING
Re: [PATCH v7 5/8] ima: make process_buffer_measurement() generic
[Cc'ing Prakhar Srivastava]
On Mon, 2019-10-07 at 21:14 -0400, Nayna Jain wrote:
> An additional measurement record is needed to indicate the blacklisted
> binary. The record will measure the blacklisted binary hash.
>
> This patch makes the function process_buffer_measurement() generic to be
> called by the blacklisting function. It modifies the function to handle
> more than just the KEXEC_CMDLINE.
The purpose of this patch is to make process_buffer_measurement() more
generic. The patch description should simply say,
process_buffer_measurement() is limited to measuring the kexec boot
command line. This patch makes process_buffer_measurement() more
generic, allowing it to measure other types of buffer data (eg.
blacklisted binary hashes).
Mimi
>
> Signed-off-by: Nayna Jain
> ---
> security/integrity/ima/ima.h | 3 +++
> security/integrity/ima/ima_main.c | 29 ++---
> 2 files changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 3689081aaf38..ed86c1f70d7f 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -217,6 +217,9 @@ void ima_store_measurement(struct integrity_iint_cache
> *iint, struct file *file,
> struct evm_ima_xattr_data *xattr_value,
> int xattr_len, const struct modsig *modsig, int pcr,
> struct ima_template_desc *template_desc);
> +void process_buffer_measurement(const void *buf, int size,
> + const char *eventname, int pcr,
> + struct ima_template_desc *template_desc);
> void ima_audit_measurement(struct integrity_iint_cache *iint,
> const unsigned char *filename);
> int ima_alloc_init_template(struct ima_event_data *event_data,
> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index 60027c643ecd..77115e884496 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -626,14 +626,14 @@ int ima_load_data(enum kernel_load_data_id id)
> * @buf: pointer to the buffer that needs to be added to the log.
> * @size: size of buffer(in bytes).
> * @eventname: event name to be used for the buffer entry.
> - * @cred: a pointer to a credentials structure for user validation.
> - * @secid: the secid of the task to be validated.
> + * @pcr: pcr to extend the measurement
> + * @template_desc: template description
> *
> * Based on policy, the buffer is measured into the ima log.
> */
> -static void process_buffer_measurement(const void *buf, int size,
> -const char *eventname,
> -const struct cred *cred, u32 secid)
> +void process_buffer_measurement(const void *buf, int size,
> + const char *eventname, int pcr,
> + struct ima_template_desc *template_desc)
> {
> int ret = 0;
> struct ima_template_entry *entry = NULL;
> @@ -642,19 +642,11 @@ static void process_buffer_measurement(const void *buf,
> int size,
> .filename = eventname,
> .buf = buf,
> .buf_len = size};
> - struct ima_template_desc *template_desc = NULL;
> struct {
> struct ima_digest_data hdr;
> char digest[IMA_MAX_DIGEST_SIZE];
> } hash = {};
> int violation = 0;
> - int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
> - int action = 0;
> -
> - action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr,
> - &template_desc);
> - if (!(action & IMA_MEASURE))
> - return;
>
> iint.ima_hash = &hash.hdr;
> iint.ima_hash->algo = ima_hash_algo;
> @@ -686,12 +678,19 @@ static void process_buffer_measurement(const void *buf,
> int size,
> */
> void ima_kexec_cmdline(const void *buf, int size)
> {
> + int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
> + struct ima_template_desc *template_desc = NULL;
> + int action;
> u32 secid;
>
> if (buf && size != 0) {
> security_task_getsecid(current, &secid);
> - process_buffer_measurement(buf, size, "kexec-cmdline",
> -current_cred(), secid);
> + action = ima_get_action(NULL, current_cred(), secid, 0,
> + KEXEC_CMDLINE, &pcr, &template_desc);
> + if (!(action & IMA_MEASURE))
> + return;
> + process_buffer_measurement(buf, size, "kexec-cmdline", pcr,
> +template_desc);
> }
> }
>
Re: [PATCH v7 2/8] powerpc: add support to initialize ima policy rules
On Mon, 2019-10-07 at 21:14 -0400, Nayna Jain wrote: > PowerNV systems uses kernel based bootloader, thus its secure boot > implementation uses kernel IMA security subsystem to verify the kernel > before kexec. ^use a Linux based bootloader, which rely on the IMA subsystem to enforce different secure boot modes. > Since the verification policy might differ based on the > secure boot mode of the system, the policies are defined at runtime. ^the policies need to be defined at runtime. > > This patch implements the arch-specific support to define the IMA policy > rules based on the runtime secure boot mode of the system. > > This patch provides arch-specific IMA policies if PPC_SECURE_BOOT > config is enabled. > > Signed-off-by: Nayna Jain > --- > arch/powerpc/Kconfig | 2 ++ > arch/powerpc/kernel/Makefile | 2 +- > arch/powerpc/kernel/ima_arch.c | 33 + > include/linux/ima.h| 3 ++- > 4 files changed, 38 insertions(+), 2 deletions(-) > create mode 100644 arch/powerpc/kernel/ima_arch.c > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index b4a221886fcf..deb19ec6ba3d 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -938,6 +938,8 @@ config PPC_SECURE_BOOT > prompt "Enable secure boot support" > bool > depends on PPC_POWERNV > + depends on IMA > + depends on IMA_ARCH_POLICY As IMA_ARCH_POLICY is dependent on IMA, I don't see a need for depending on both IMA and IMA_ARCH_POLICY. Mimi
Re: [PATCH v6 6/9] ima: make process_buffer_measurement() non static
[Cc'ing Prakhar] On Fri, 2019-09-27 at 10:25 -0400, Nayna Jain wrote: > To add the support for checking against blacklist, it would be needed > to add an additional measurement record that identifies the record > as blacklisted. > > This patch modifies the process_buffer_measurement() and makes it > non static to be used by blacklist functionality. It modifies the > function to handle more than just the KEXEC_CMDLINE. > > Signed-off-by: Nayna Jain Making process_buffer_measurement() non static is the end result, not the reason for the change. The reason for changing process_buffer_measurement() is to make it more generic. The blacklist measurement record is the usecase. Please rewrite the patch description. thanks, Mimi
Re: [PATCH v6 3/9] powerpc: add support to initialize ima policy rules
On Tue, 2019-10-01 at 12:07 -0400, Nayna wrote:
>
> On 09/30/2019 09:04 PM, Thiago Jung Bauermann wrote:
> > Hello,
>
> Hi,
>
> >
> >> diff --git a/arch/powerpc/kernel/ima_arch.c
> >> b/arch/powerpc/kernel/ima_arch.c
> >> new file mode 100644
> >> index ..39401b67f19e
> >> --- /dev/null
> >> +++ b/arch/powerpc/kernel/ima_arch.c
> >> @@ -0,0 +1,33 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (C) 2019 IBM Corporation
> >> + * Author: Nayna Jain
> >> + */
> >> +
> >> +#include
> >> +#include
> >> +
> >> +bool arch_ima_get_secureboot(void)
> >> +{
> >> + return is_powerpc_os_secureboot_enabled();
> >> +}
> >> +
> >> +/* Defines IMA appraise rules for secureboot */
> >> +static const char *const arch_rules[] = {
> >> + "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
> >> +#if !IS_ENABLED(CONFIG_MODULE_SIG)
> >> + "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> >> +#endif
> >> + NULL
> >> +};
> >> +
> >> +/*
> >> + * Returns the relevant IMA arch policies based on the system secureboot
> >> state.
> >> + */
> >> +const char *const *arch_get_ima_policy(void)
> >> +{
> >> + if (is_powerpc_os_secureboot_enabled())
> >> + return arch_rules;
> >> +
> >> + return NULL;
> >> +}
> > If CONFIG_MODULE_SIG is enabled but module signatures aren't enforced,
> > then IMA won't enforce module signature either. x86's
> > arch_get_ima_policy() calls set_module_sig_enforced(). Doesn't the
> > powerpc version need to do that as well?
> >
> > On the flip side, if module signatures are enforced by the module
> > subsystem then IMA will verify the signature a second time since there's
> > no sharing of signature verification results between the module
> > subsystem and IMA (this was observed by Mimi).
> >
> > IMHO this is a minor issue, since module loading isn't a hot path and
> > the duplicate work shouldn't impact anything. But it could be avoided by
> > having a NULL entry in arch_rules, which arch_get_ima_policy() would
> > dynamically update with the "appraise func=MODULE_CHECK" rule if
> > is_module_sig_enforced() is true.
>
> Thanks Thiago for reviewing. I am wondering that this will give two
> meanings for NULL. Can we do something like below, there are possibly
> two options ?
>
> 1. Set IMA_APPRAISED in the iint->flags if is_module_sig_enforced().
>
> OR
>
> 2. Let ima_get_action() check for is_module_sig_enforced() when policy
> is appraise and func is MODULE_CHECK.
I'm a bit hesitant about mixing the module subsystem signature
verification method with the IMA measure "template=ima-modsig" rules.
Does it actually work?
We can at least limit verifying the same appended signature twice to
when "module.sig_enforce" is specified on the boot command line, by
changing "!IS_ENABLED(CONFIG_MODULE_SIG)" to test
"CONFIG_MODULE_SIG_FORCE".
Mimi
Re: [PATCH v6 8/9] ima: deprecate permit_directio, instead use appraise_flag
Hi Nayna, On Fri, 2019-09-27 at 10:25 -0400, Nayna Jain wrote: > This patch deprecates the existing permit_directio flag, instead adds > it as possible value to appraise_flag parameter. > For eg. > appraise_flag=permit_directio Defining a generic "appraise_flag=", which supports different options, is the right direction. I would really like to depreciate the "permit_directio" flag, not just change the policy syntax. For now, let's drop this change. Mimi
Re: [PATCH v6 7/9] ima: check against blacklisted hashes for files with modsig
On Fri, 2019-09-27 at 10:25 -0400, Nayna Jain wrote:
> Asymmetric private keys are used to sign multiple files. The kernel
> currently support checking against the blacklisted keys. However, if the
> public key is blacklisted, any file signed by the blacklisted key will
> automatically fail signature verification. We might not want to blacklist
> all the files signed by a particular key, but just a single file.
> Blacklisting the public key is not fine enough granularity.
>
> This patch adds support for blacklisting binaries with appended signatures,
> based on the IMA policy. Defined is a new policy option
> "appraise_flag=check_blacklist".
>
> Signed-off-by: Nayna Jain
> ---
> Documentation/ABI/testing/ima_policy | 1 +
> security/integrity/ima/ima.h | 12 +
> security/integrity/ima/ima_appraise.c | 35 +++
> security/integrity/ima/ima_main.c | 8 --
> security/integrity/ima/ima_policy.c | 10 ++--
> security/integrity/integrity.h| 1 +
> 6 files changed, 63 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy
> b/Documentation/ABI/testing/ima_policy
> index 29ebe9afdac4..4c97afcc0f3c 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -25,6 +25,7 @@ Description:
> lsm:[[subj_user=] [subj_role=] [subj_type=]
>[obj_user=] [obj_role=] [obj_type=]]
> option: [[appraise_type=]] [template=] [permit_directio]
> + [appraise_flag=[check_blacklist]]
> base: func:=
> [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
> [FIRMWARE_CHECK]
> [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 9bf509217e8e..2c034728b239 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -254,6 +254,9 @@ int ima_policy_show(struct seq_file *m, void *v);
> #define IMA_APPRAISE_KEXEC 0x40
>
> #ifdef CONFIG_IMA_APPRAISE
> +int ima_blacklist_measurement(struct integrity_iint_cache *iint,
> + const struct modsig *modsig, int action,
> + int pcr, struct ima_template_desc *template_desc);
> int ima_appraise_measurement(enum ima_hooks func,
>struct integrity_iint_cache *iint,
>struct file *file, const unsigned char *filename,
> @@ -269,6 +272,15 @@ int ima_read_xattr(struct dentry *dentry,
> struct evm_ima_xattr_data **xattr_value);
>
> #else
> +static inline int ima_blacklist_measurement(struct integrity_iint_cache
> *iint,
> + const struct modsig *modsig,
> + int action, int pcr,
> + struct ima_template_desc
> + *template_desc)
> +{
> + return 0;
> +}
> +
> static inline int ima_appraise_measurement(enum ima_hooks func,
> struct integrity_iint_cache *iint,
> struct file *file,
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index 136ae4e0ee92..a5a82e870e24 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -12,6 +12,7 @@
> #include
> #include
> #include
> +#include
>
> #include "ima.h"
>
> @@ -303,6 +304,40 @@ static int modsig_verify(enum ima_hooks func, const
> struct modsig *modsig,
> return rc;
> }
>
> +/*
> + * ima_blacklist_measurement - checks if the file measurement is blacklisted
> + *
> + * Returns -EKEYREJECTED if the hash is blacklisted.
> + */
In the function comment, please mention that blacklisted binaries are
included in the IMA measurement list.
> +int ima_blacklist_measurement(struct integrity_iint_cache *iint,
> + const struct modsig *modsig, int action, int pcr,
> + struct ima_template_desc *template_desc)
> +{
> + enum hash_algo hash_algo;
> + const u8 *digest = NULL;
> + u32 digestsize = 0;
> + u32 secid;
> + int rc = 0;
> +
> + if (!(iint->flags & IMA_CHECK_BLACKLIST))
> + return 0;
> +
> + if (iint->flags & IMA_MODSIG_ALLOWED) {
> + security_task_getsecid(current, &secid);
> + ima_get_modsig_digest(modsig, &hash_algo, &digest, &digestsize);
> +
> + rc = is_hash_blacklisted(digest, digestsize, "bin");
> +
> + /* Returns -EKEYREJECTED on blacklisted hash found */
is_hash_blacklisted() returning -EKEYREJECTED makes sense if the key
is blacklisted, not so much for a binary. It would make more sense to
define is_binary_blacklis
Re: [PATCH v6 5/9] powerpc/ima: add measurement rules to ima arch specific policy
On Fri, 2019-09-27 at 10:25 -0400, Nayna Jain wrote:
> This patch adds the measurement rules to the arch specific policies for the
> systems with trusted boot.
>
on trusted boot enabled systems.
> Signed-off-by: Nayna Jain
Minor comment correction below.
Reviewed-by: Mimi Zohar
> ---
> arch/powerpc/kernel/ima_arch.c | 44 +++---
> 1 file changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
> index 39401b67f19e..77c61b142042 100644
> --- a/arch/powerpc/kernel/ima_arch.c
> +++ b/arch/powerpc/kernel/ima_arch.c
> @@ -12,8 +12,18 @@ bool arch_ima_get_secureboot(void)
> return is_powerpc_os_secureboot_enabled();
> }
>
> -/* Defines IMA appraise rules for secureboot */
> +/*
> + * The "arch_rules" contains both the securebot and trustedboot rules for
> adding
> + * the kexec kernel image and kernel modules file hashes to the IMA
> measurement
> + * list and verifying the file signatures against known good values.
> + *
> + * The "appraise_type=imasig|modsig" option allows the good signature to be
> + * stored as an xattr or as an appended signature. The "template=ima-modsig"
> + * option includes the appended signature in the IMA measurement list.
includes the appended signature, when available, in the IMA
measurement list.
> + */
> static const char *const arch_rules[] = {
> + "measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
> + "measure func=MODULE_CHECK template=ima-modsig",
> "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
> #if !IS_ENABLED(CONFIG_MODULE_SIG)
> "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> @@ -22,12 +32,40 @@ static const char *const arch_rules[] = {
> };
>
> /*
> - * Returns the relevant IMA arch policies based on the system secureboot
> state.
> + * The "measure_rules" are enabled only on "trustedboot" enabled systems.
> + * These rules add the kexec kernel image and kernel modules file hashes to
> + * the IMA measurement list.
> + */
> +static const char *const measure_rules[] = {
> + "measure func=KEXEC_KERNEL_CHECK",
> + "measure func=MODULE_CHECK",
> + NULL
> +};
> +
> +/*
> + * Returns the relevant IMA arch policies based on the system secureboot
> + * and trustedboot state.
> */
> const char *const *arch_get_ima_policy(void)
> {
> - if (is_powerpc_os_secureboot_enabled())
> + const char *const *rules;
> + int offset = 0;
> +
> + for (rules = arch_rules; *rules != NULL; rules++) {
> + if (strncmp(*rules, "appraise", 8) == 0)
> + break;
> + offset++;
> + }
> +
> + if (is_powerpc_os_secureboot_enabled()
> + && is_powerpc_trustedboot_enabled())
> return arch_rules;
>
> + if (is_powerpc_os_secureboot_enabled())
> + return arch_rules + offset;
> +
> + if (is_powerpc_trustedboot_enabled())
> + return measure_rules;
> +
> return NULL;
> }
Re: [PATCH v3 4/4] powerpc: load firmware trusted keys/hashes into kernel keyring
On Mon, 2019-08-26 at 09:23 -0400, Nayna Jain wrote:
> The keys used to verify the Host OS kernel are managed by firmware as
> secure variables. This patch loads the verification keys into the .platform
> keyring and revocation hashes into .blacklist keyring. This enables
> verification and loading of the kernels signed by the boot time keys which
> are trusted by firmware.
>
> Signed-off-by: Nayna Jain
Feel free to add my tag after addressing the formatting issues.
Reviewed-by: Mimi Zohar
> diff --git a/security/integrity/platform_certs/load_powerpc.c
> b/security/integrity/platform_certs/load_powerpc.c
> new file mode 100644
> index ..359d5063d4da
> --- /dev/null
> +++ b/security/integrity/platform_certs/load_powerpc.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Nayna Jain
> + *
> + * - loads keys and hashes stored and controlled by the firmware.
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "keyring_handler.h"
> +
> +/*
> + * Get a certificate list blob from the named secure variable.
> + */
> +static __init void *get_cert_list(u8 *key, unsigned long keylen, uint64_t
> *size)
> +{
> + int rc;
> + void *db;
> +
> + rc = secvar_ops->get(key, keylen, NULL, size);
> + if (rc) {
> + pr_err("Couldn't get size: %d\n", rc);
> + return NULL;
> + }
> +
> + db = kmalloc(*size, GFP_KERNEL);
> + if (!db)
> + return NULL;
> +
> + rc = secvar_ops->get(key, keylen, db, size);
> + if (rc) {
> + kfree(db);
> + pr_err("Error reading db var: %d\n", rc);
> + return NULL;
> + }
> +
> + return db;
> +}
> +
> +/*
> + * Load the certs contained in the keys databases into the platform trusted
> + * keyring and the blacklisted X.509 cert SHA256 hashes into the blacklist
> + * keyring.
> + */
> +static int __init load_powerpc_certs(void)
> +{
> + void *db = NULL, *dbx = NULL;
> + uint64_t dbsize = 0, dbxsize = 0;
> + int rc = 0;
> +
> + if (!secvar_ops)
> + return -ENODEV;
> +
> + /* Get db, and dbx. They might not exist, so it isn't
> + * an error if we can't get them.
> + */
> + db = get_cert_list("db", 3, &dbsize);
> + if (!db) {
> + pr_err("Couldn't get db list from firmware\n");
> + } else {
> + rc = parse_efi_signature_list("powerpc:db",
> + db, dbsize, get_handler_for_db);
> + if (rc)
> + pr_err("Couldn't parse db signatures: %d\n",
> + rc);
There's no need to split this line.
> + kfree(db);
> + }
> +
> + dbx = get_cert_list("dbx", 3, &dbxsize);
> + if (!dbx) {
> + pr_info("Couldn't get dbx list from firmware\n");
> + } else {
> + rc = parse_efi_signature_list("powerpc:dbx",
> + dbx, dbxsize,
> + get_handler_for_dbx);
Formatting of this line is off.
> + if (rc)
> + pr_err("Couldn't parse dbx signatures: %d\n", rc);
> + kfree(dbx);
> + }
> +
> + return rc;
> +}
> +late_initcall(load_powerpc_certs);
Re: [PATCH v3 3/4] x86/efi: move common keyring handler functions to new file
(Cc'ing Josh Boyer, David Howells) On Mon, 2019-09-02 at 21:55 +1000, Michael Ellerman wrote: > Nayna Jain writes: > > > The handlers to add the keys to the .platform keyring and blacklisted > > hashes to the .blacklist keyring is common for both the uefi and powerpc > > mechanisms of loading the keys/hashes from the firmware. > > > > This patch moves the common code from load_uefi.c to keyring_handler.c > > > > Signed-off-by: Nayna Jain Acked-by: Mimi Zohar > > --- > > security/integrity/Makefile | 3 +- > > .../platform_certs/keyring_handler.c | 80 +++ > > .../platform_certs/keyring_handler.h | 32 > > security/integrity/platform_certs/load_uefi.c | 67 +--- > > 4 files changed, 115 insertions(+), 67 deletions(-) > > create mode 100644 security/integrity/platform_certs/keyring_handler.c > > create mode 100644 security/integrity/platform_certs/keyring_handler.h > > This has no acks from security folks, though I'm not really clear on who > maintains those files. I upstreamed David's, Josh's, and Nayna's patches, so that's probably me. > Do I take it because it's mostly just code movement people are OK with > it going in via the powerpc tree? Yes, the only reason for splitting load_uefi.c is for powerpc. These patches should be upstreamed together. Mimi
Re: [PATCH 2/2 v2] efi: print appropriate status message when loading certificates
On Sun, 2019-03-24 at 08:26 +0800, Lee, Chun-Yi wrote:
> When loading certificates list from UEFI variable, the original error
> message direct shows the efi status code from UEFI firmware. It looks
> ugly:
>
> [2.335031] Couldn't get size: 0x800e
> [2.335032] Couldn't get UEFI MokListRT
> [2.339985] Couldn't get size: 0x800e
> [2.339987] Couldn't get UEFI dbx list
>
> So, this patch shows the status string instead of status code.
>
> On the other hand, the "Couldn't get UEFI" message doesn't need
> to be exposed when db/dbx/mok variable do not exist. So, this
> patch set the message level to debug.
>
> v2.
> Setting the MODSIGN messagse level to debug.
>
> Link:
> https://forums.opensuse.org/showthread.php/535324-MODSIGN-Couldn-t-get-UEFI-db-list?p=2897516#post2897516
> Cc: James Morris
> Cc: Serge E. Hallyn"
> Cc: David Howells
> Cc: Nayna Jain
> Cc: Josh Boyer
> Cc: Mimi Zohar
> Signed-off-by: "Lee, Chun-Yi"
> ---
> security/integrity/platform_certs/load_uefi.c | 13 -
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/security/integrity/platform_certs/load_uefi.c
> b/security/integrity/platform_certs/load_uefi.c
> index 81b19c52832b..e65244b31f04 100644
> --- a/security/integrity/platform_certs/load_uefi.c
> +++ b/security/integrity/platform_certs/load_uefi.c
> @@ -48,7 +48,9 @@ static __init void *get_cert_list(efi_char16_t *name,
> efi_guid_t *guid,
>
> 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);
> + if (status != EFI_NOT_FOUND)
> + pr_err("Couldn't get size: %s\n",
> + efi_status_to_str(status));
> return NULL;
> }
>
> @@ -59,7 +61,8 @@ static __init void *get_cert_list(efi_char16_t *name,
> efi_guid_t *guid,
> 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: %s\n",
> + efi_status_to_str(status));
> return NULL;
> }
>
> @@ -155,7 +158,7 @@ static int __init load_uefi_certs(void)
> if (!uefi_check_ignore_db()) {
> db = get_cert_list(L"db", &secure_var, &dbsize);
> if (!db) {
> - pr_err("MODSIGN: Couldn't get UEFI db list\n");
> + pr_debug("MODSIGN: Couldn't get UEFI db list\n");
Sure, this is fine.
> } else {
> rc = parse_efi_signature_list("UEFI:db",
> db, dbsize, get_handler_for_db);
> @@ -168,7 +171,7 @@ static int __init load_uefi_certs(void)
>
> mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
> if (!mok) {
> - pr_info("Couldn't get UEFI MokListRT\n");
> + pr_debug("Couldn't get UEFI MokListRT\n");
This is fine too.
> } else {
> rc = parse_efi_signature_list("UEFI:MokListRT",
> mok, moksize, get_handler_for_db);
> @@ -179,7 +182,7 @@ static int __init load_uefi_certs(void)
>
> dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
> if (!dbx) {
> - pr_info("Couldn't get UEFI dbx list\n");
> + pr_debug("Couldn't get UEFI dbx list\n");
If there isn't a db or moklist, then this is fine. My concern is not
having an indication that the dbx wasn't installed, when it should
have been.
Perhaps similar to the "Loading compiled-in X.509 certificates"
informational message there should informational messages for db, mok,
and dbx as well.
Mimi
> } else {
> rc = parse_efi_signature_list("UEFI:dbx",
> dbx, dbxsize,
Re: [PATCH 1/2] efi: add a function for transferring status to string
On Wed, 2019-03-27 at 19:58 +0100, Ard Biesheuvel wrote:
> On Sun, 24 Mar 2019 at 01:26, Lee, Chun-Yi wrote:
> >
> > This function can be used to transfer EFI status code to string
> > for printing out debug message. Using this function can improve
> > the readability of log.
Maybe instead of "for transferring status" use "to convert the status
value to a" in the Subject line and here in the patch description.
> >
> > Cc: Ard Biesheuvel
> > Cc: Kees Cook
> > Cc: Anton Vorontsov
> > Cc: Colin Cross
> > Cc: Tony Luck
> > Signed-off-by: "Lee, Chun-Yi"
> > ---
> > include/linux/efi.h | 28
> > 1 file changed, 28 insertions(+)
> >
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 54357a258b35..a43cb0dc37af 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -1768,4 +1768,32 @@ struct linux_efi_memreserve {
> > #define EFI_MEMRESERVE_COUNT(size) (((size) - sizeof(struct
> > linux_efi_memreserve)) \
> > / sizeof(((struct linux_efi_memreserve *)0)->entry[0]))
> >
> > +#define EFI_STATUS_STR(_status) \
> > +case EFI_##_status: \
> > + return "EFI_" __stringify(_status);
> > +
> > +static inline char *
> > +efi_status_to_str(efi_status_t status)
> > +{
> > + switch (status) {
> > + EFI_STATUS_STR(SUCCESS)
> > + EFI_STATUS_STR(LOAD_ERROR)
> > + EFI_STATUS_STR(INVALID_PARAMETER)
> > + EFI_STATUS_STR(UNSUPPORTED)
> > + EFI_STATUS_STR(BAD_BUFFER_SIZE)
> > + EFI_STATUS_STR(BUFFER_TOO_SMALL)
> > + EFI_STATUS_STR(NOT_READY)
> > + EFI_STATUS_STR(DEVICE_ERROR)
> > + EFI_STATUS_STR(WRITE_PROTECTED)
> > + EFI_STATUS_STR(OUT_OF_RESOURCES)
> > + EFI_STATUS_STR(NOT_FOUND)
> > + EFI_STATUS_STR(ABORTED)
> > + EFI_STATUS_STR(SECURITY_VIOLATION)
> > + default:
> > + pr_warn("Unknown efi status: 0x%lx", status);
> > + }
> > +
> > + return "Unknown efi status";
> > +}
> > +
> > #endif /* _LINUX_EFI_H */
> > --
> > 2.16.4
> >
>
> Please turn this into a proper function so that not every calling
> object has to duplicate all these strings.
Hi Ard,
Keeping the status values and strings in sync is difficult. I was
going to suggest moving the macro immediately after the status value
definitions.
Mimi
Re: [PATCH 3/3] x86/ima: retry detecting secure boot mode
On Fri, 2019-03-08 at 09:51 -0800, Matthew Garrett wrote: > On Fri, Mar 8, 2019 at 5:40 AM Mimi Zohar wrote: > > > > On Thu, 2019-03-07 at 14:50 -0800, Matthew Garrett wrote: > > > Is the issue that it gives incorrect results on the first read, or is > > > the issue that it gives incorrect results before ExitBootServices() is > > > called? If the former then we should read twice in the boot stub, if > > > the latter then we should figure out a way to do this immediately > > > after ExitBootServices() instead. > > > > Detecting the secure boot mode isn't the problem. On boot, I am > > seeing "EFI stub: UEFI Secure Boot is enabled", but setup_arch() emits > > "Secure boot could not be determined". > > > > In efi_main() the secure_boot mode is initially unset, so > > efi_get_secureboot() is called. efi_get_secureboot() returns the > > secure_boot mode correctly as enabled. The problem seems to be in > > saving the secure_boot mode for later use. > > Hm. And this only happens on certain firmware versions? If something's > stepping on boot_params then we have bigger problems. I was seeing this problem before and after updating the system firmware on my laptop last summer. If updating the firmware had resolved the problem, I wouldn't have included this patch. Mimi
Re: [PATCH 3/3] x86/ima: retry detecting secure boot mode
On Fri, 2019-03-08 at 09:51 -0800, Matthew Garrett wrote: > On Fri, Mar 8, 2019 at 5:40 AM Mimi Zohar wrote: > > > > On Thu, 2019-03-07 at 14:50 -0800, Matthew Garrett wrote: > > > Is the issue that it gives incorrect results on the first read, or is > > > the issue that it gives incorrect results before ExitBootServices() is > > > called? If the former then we should read twice in the boot stub, if > > > the latter then we should figure out a way to do this immediately > > > after ExitBootServices() instead. > > > > Detecting the secure boot mode isn't the problem. On boot, I am > > seeing "EFI stub: UEFI Secure Boot is enabled", but setup_arch() emits > > "Secure boot could not be determined". > > > > In efi_main() the secure_boot mode is initially unset, so > > efi_get_secureboot() is called. efi_get_secureboot() returns the > > secure_boot mode correctly as enabled. The problem seems to be in > > saving the secure_boot mode for later use. > > Hm. And this only happens on certain firmware versions? If something's > stepping on boot_params then we have bigger problems. FYI, efi_printk() works before exit_boot(), but not afterwards. The system hangs. Mimi
Re: [PATCH 3/3] x86/ima: retry detecting secure boot mode
On Thu, 2019-03-07 at 14:50 -0800, Matthew Garrett wrote: > On Thu, Mar 7, 2019 at 2:48 PM Mimi Zohar wrote: > > I added this last attempt because I'm seeing this on my laptop, with > > some older, buggy firmware. > > Is the issue that it gives incorrect results on the first read, or is > the issue that it gives incorrect results before ExitBootServices() is > called? If the former then we should read twice in the boot stub, if > the latter then we should figure out a way to do this immediately > after ExitBootServices() instead. Detecting the secure boot mode isn't the problem. On boot, I am seeing "EFI stub: UEFI Secure Boot is enabled", but setup_arch() emits "Secure boot could not be determined". In efi_main() the secure_boot mode is initially unset, so efi_get_secureboot() is called. efi_get_secureboot() returns the secure_boot mode correctly as enabled. The problem seems to be in saving the secure_boot mode for later use. Mimi
Re: [PATCH 3/3] x86/ima: retry detecting secure boot mode
On Thu, 2019-03-07 at 14:44 -0800, Matthew Garrett wrote: > On Thu, Mar 7, 2019 at 2:38 PM Justin Forbes wrote: > > On Thu, Mar 7, 2019 at 4:29 PM Matthew Garrett wrote: > >> > >> On Mon, Nov 19, 2018 at 11:57 AM Mimi Zohar wrote: > >> > > >> > The secure boot mode may not be detected on boot for some reason (eg. > >> > buggy firmware). This patch attempts one more time to detect the > >> > secure boot mode. > >> > >> Do we have cases where this has actually been seen? I'm not sure what > >> the circumstances are that would result in this behaviour. > > > > > > We have never seen it in practice, though we only ever do anything with it > > with x86, so it is possible that some other platforms maybe? > > I'm not sure that it buys us anything to check this in both the boot > stub and the running kernel. If a platform *is* giving us different > results, anything else relying on the information from the boot stub > is also going to be broken, so we should do this centrally rather than > in the IMA code. I added this last attempt because I'm seeing this on my laptop, with some older, buggy firmware. Mimi
Re: linux-next: Tree for Feb 20
> Fixes for this have already been proposed, and should appear in -next shortly > > The EFI one is here > https://mail.google.com/mail/u/0/#label/linux-efi/FMfcgxwBVgrQRjglPkWRqRqVclGgVDnB > > Not sure about the IMA one, Mimi should be able to comment ... I've already commented on the other patch and was expecting to see a revised patch. Mimi
Re: [PATCH v3 1/2] ima: fix build error redeclaration of enumerator
On Thu, 2019-02-14 at 12:28 -0500, Mimi Zohar wrote:
> On Wed, 2019-02-13 at 23:16 +0100, Anders Roxell wrote:
> > Commit a893ea15d764 ("tpm: move tpm_chip definition to
> > include/linux/tpm.h") introduced a build error when both ima and efi is
> > enabled. What happens is that both headers (ima.h and efi.h) defines the
> > same 'NONE' constant, and it broke when they started getting included
> > from the same file.
> >
> > In file included from ../security/integrity/ima/ima_fs.c:30:
> > ../security/integrity/ima/ima.h:176:7: error: redeclaration of enumerator
> > "NONE"
> > hook(NONE) \
> >^~~~
> > ../security/integrity/ima/ima.h:188:34: note: in definition of macro
> > "__ima_hook_enumify"
> > #define __ima_hook_enumify(ENUM) ENUM,
> > ^~~~
> > ../security/integrity/ima/ima.h:191:2: note: in expansion of macro
> > "__ima_hooks"
> > __ima_hooks(__ima_hook_enumify)
> > ^~~
> > In file included from ../arch/arm64/include/asm/acpi.h:15,
> > from ../include/acpi/acpi_io.h:7,
> > from ../include/linux/acpi.h:47,
> > from ../include/linux/tpm.h:26,
> > from ../security/integrity/ima/ima.h:25,
> > from ../security/integrity/ima/ima_fs.c:30:
> > ../include/linux/efi.h:1723:2: note: previous definition of "NONE" was here
> > NONE,
> > ^~~~
> > make[4]: *** [../scripts/Makefile.build:277:
> > security/integrity/ima/ima_fs.o] Error 1
> >
> > Rework to prefix the ima enum with 'IMA_*'.
> >
> > Reviewed-by: Andy Shevchenko
> > Signed-off-by: Anders Roxell
>
> Ok, this looks reasonable, but will have a minor clash with Gustavo's
> "security: mark expected switch fall-throughs and add a missing
> break".
This patch correctly didn't modify the IMA policy keywords, just the
enumeration, but now the policy file incorrectly displays
the "ima_" prefixed names.
Instead of maintaining an enumeration and the corresponding
stringified version of the enumeration, there is a single list with
two macros. One of these macros needs to be modified. Instead of
modifying the the hook names directly, I would probably modify the
enumeration macro.
#define __ima_hook_enumify(ENUM)ENUM,
enum ima_hooks {
__ima_hooks(__ima_hook_enumify)
};
and
#define __ima_hook_stringify(str) (#str),
static const char *const func_tokens[] = {
__ima_hooks(__ima_hook_stringify)
};
Mimi
Re: [PATCH v3 1/2] ima: fix build error redeclaration of enumerator
On Wed, 2019-02-13 at 23:16 +0100, Anders Roxell wrote:
> Commit a893ea15d764 ("tpm: move tpm_chip definition to
> include/linux/tpm.h") introduced a build error when both ima and efi is
> enabled. What happens is that both headers (ima.h and efi.h) defines the
> same 'NONE' constant, and it broke when they started getting included
> from the same file.
>
> In file included from ../security/integrity/ima/ima_fs.c:30:
> ../security/integrity/ima/ima.h:176:7: error: redeclaration of enumerator
> "NONE"
> hook(NONE) \
>^~~~
> ../security/integrity/ima/ima.h:188:34: note: in definition of macro
> "__ima_hook_enumify"
> #define __ima_hook_enumify(ENUM) ENUM,
> ^~~~
> ../security/integrity/ima/ima.h:191:2: note: in expansion of macro
> "__ima_hooks"
> __ima_hooks(__ima_hook_enumify)
> ^~~
> In file included from ../arch/arm64/include/asm/acpi.h:15,
> from ../include/acpi/acpi_io.h:7,
> from ../include/linux/acpi.h:47,
> from ../include/linux/tpm.h:26,
> from ../security/integrity/ima/ima.h:25,
> from ../security/integrity/ima/ima_fs.c:30:
> ../include/linux/efi.h:1723:2: note: previous definition of "NONE" was here
> NONE,
> ^~~~
> make[4]: *** [../scripts/Makefile.build:277: security/integrity/ima/ima_fs.o]
> Error 1
>
> Rework to prefix the ima enum with 'IMA_*'.
>
> Reviewed-by: Andy Shevchenko
> Signed-off-by: Anders Roxell
Ok, this looks reasonable, but will have a minor clash with Gustavo's
"security: mark expected switch fall-throughs and add a missing
break".
James, are you picking up Gastavo's v2 patch from Friday?
Mimi
> ---
>
> No change since v2.
>
> security/integrity/ima/ima.h | 24 +++
> security/integrity/ima/ima_api.c | 3 +-
> security/integrity/ima/ima_appraise.c | 40 ++--
> security/integrity/ima/ima_main.c | 30 -
> security/integrity/ima/ima_policy.c | 92 +--
> 5 files changed, 95 insertions(+), 94 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index d213e835c498..89ceb61f279c 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -173,18 +173,18 @@ static inline unsigned long ima_hash_key(u8 *digest)
> }
>
> #define __ima_hooks(hook)\
> - hook(NONE) \
> - hook(FILE_CHECK)\
> - hook(MMAP_CHECK)\
> - hook(BPRM_CHECK)\
> - hook(CREDS_CHECK) \
> - hook(POST_SETATTR) \
> - hook(MODULE_CHECK) \
> - hook(FIRMWARE_CHECK)\
> - hook(KEXEC_KERNEL_CHECK)\
> - hook(KEXEC_INITRAMFS_CHECK) \
> - hook(POLICY_CHECK) \
> - hook(MAX_CHECK)
> + hook(IMA_NONE) \
> + hook(IMA_FILE_CHECK)\
> + hook(IMA_MMAP_CHECK)\
> + hook(IMA_BPRM_CHECK)\
> + hook(IMA_CREDS_CHECK) \
> + hook(IMA_POST_SETATTR) \
> + hook(IMA_MODULE_CHECK) \
> + hook(IMA_FIRMWARE_CHECK)\
> + hook(IMA_KEXEC_KERNEL_CHECK)\
> + hook(IMA_KEXEC_INITRAMFS_CHECK) \
> + hook(IMA_POLICY_CHECK) \
> + hook(IMA_MAX_CHECK)
> #define __ima_hook_enumify(ENUM) ENUM,
>
> enum ima_hooks {
> diff --git a/security/integrity/ima/ima_api.c
> b/security/integrity/ima/ima_api.c
> index c7505fb122d4..81e705423894 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -168,7 +168,8 @@ void ima_add_violation(struct file *file, const unsigned
> char *filename,
> * The policy is defined in terms of keypairs:
> * subj=, obj=, type=, func=, mask=, fsmagic=
> * subj,obj, and type: are LSM specific.
> - * func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK
> + * func: IMA_FILE_CHECK | IMA_BPRM_CHECK | IMA_CREDS_CHECK \
> + * | IMA_MMAP_CHECK | IMA_MODULE_CHECK
> * mask: contains the permission mask
> * fsmagic: hex value
> *
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index a2baa85ea2f5..c527cf3f37d3 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -86,16 +86,16 @@ enum integrity_status ima_get_cache_status(struct
> integrity_iint_cache *iint,
> enum ima_hooks func)
> {
> switch (func) {
> - case MMAP_CHECK:
> + case IMA_MMAP_CHECK:
> return iint->ima_mmap_status;
> - case BPRM_CHECK:
> + case IMA_BPRM_CHECK:
> return iint->ima_bprm_status;
> - case CREDS_CHECK:
> + case IMA_CREDS_CHECK:
> return iint->ima_creds_status;
> - case FILE_CHECK:
> - case POST_SETATTR:
> + case IMA_FILE_CHE
Re: [PATCH v2 7/7] ima: Support platform keyring for kernel appraisal
On Wed, 2018-12-12 at 16:14 -0200, Thiago Jung Bauermann wrote: [snip] > Subject: [PATCH] ima: Only use the platform keyring if it's enabled > > Signed-off-by: Thiago Jung Bauermann Good catch! Thanks. Mimi > --- > security/integrity/ima/ima_appraise.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/security/integrity/ima/ima_appraise.c > b/security/integrity/ima/ima_appraise.c > index e8f520450895..f6ac405daabb 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -297,7 +297,8 @@ int ima_appraise_measurement(enum ima_hooks func, > status = INTEGRITY_UNKNOWN; > break; > } > - if (rc && func == KEXEC_KERNEL_CHECK) > + if (IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING) && rc && > + func == KEXEC_KERNEL_CHECK) > rc = integrity_digsig_verify(INTEGRITY_KEYRING_PLATFORM, >(const char *)xattr_value, >xattr_len,
Re: [PATCH v2 0/7] add platform/firmware keys support for kernel verification by IMA
Hi Nayna, On Sun, 2018-12-09 at 01:56 +0530, Nayna Jain wrote: > On secure boot enabled systems, a verified kernel may need to kexec > additional kernels. For example, it may be used as a bootloader needing > to kexec a target kernel or it may need to kexec a crashdump kernel. > In such cases, it may want to verify the signature of the next kernel > image. > > It is possible that the new kernel image is signed with third party keys > which are stored as platform or firmware keys in the 'db' variable. The > kernel, however, can not directly verify these platform keys, and an > administrator may therefore not want to trust them for arbitrary usage. > In order to differentiate platform keys from other keys and provide the > necessary separation of trust the kernel needs an additional keyring to > store platform/firmware keys. > > The secure boot key database is expected to store the keys as EFI > Signature List(ESL). The patch set uses David Howells and Josh Boyer's > patch to access and parse the ESL to extract the certificates and load > them onto the platform keyring. > > The last patch in this patch set adds support for IMA-appraisal to > verify the kexec'ed kernel image based on keys stored in the platform > keyring. Thanks! This patch set is now in the #next-integrity branch. https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git/ Mimi
Re: [PATCH 0/7] add platform/firmware keys support for kernel verification by IMA
Hi Nayna, On Sun, 2018-11-25 at 20:44 +0530, Nayna Jain wrote: > On secure boot enabled systems, a verified kernel may need to kexec > additional kernels. For example, it may be used as a bootloader needing > to kexec a target kernel or it may need to kexec a crashdump kernel. > In such cases, it may want to verify the signature of the next kernel > image. > > It is possible that the new kernel image is signed with third party keys > which are stored as platform or firmware keys in the 'db' variable. The > kernel, however, can not directly verify these platform keys, and an > administrator may therefore not want to trust them for arbitrary usage. > In order to differentiate platform keys from other keys and provide the > necessary separation of trust the kernel needs an additional keyring to > store platform/firmware keys. > > The secure boot key database is expected to store the keys as EFI > Signature List(ESL). The patch set uses David Howells and Josh Boyer's > patch to access and parse the ESL to extract the certificates and load > them onto the platform keyring. > > The last patch in this patch set adds support for IMA-appraisal to > verify the kexec'ed kernel image based on keys stored in the platform > keyring. > > Changelog: > > v0: > - The original patches loaded the certificates onto the secondary > trusted keyring. This patch set defines a new keyring named > ".platform" and adds the certificates to this new keyring > - removed CONFIG EFI_SIGNATURE_LIST_PARSER and LOAD_UEFI_KEYS > - moved files from certs/ to security/integrity/platform_certs/ This patch set is looking really good! There are a couple of checkpatch.pl warnings that need to be addressed before these patches can be upstreamed. I'd also like to see some Reviews/Acks for them as well. For the time being these patches are queued in the #next-integrity- queued branch. https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity. git/ thanks! Mimi > > Dave Howells (2): > efi: Add EFI signature data types > efi: Add an EFI signature blob parser > > Josh Boyer (2): > efi: Import certificates from UEFI Secure Boot > efi: Allow the "db" UEFI variable to be suppressed > > Nayna Jain (3): > integrity: define a trusted platform keyring > integrity: load certs to the platform keyring > ima: support platform keyring for kernel appraisal > > include/linux/efi.h| 34 > security/integrity/Kconfig | 11 ++ > security/integrity/Makefile| 5 + > security/integrity/digsig.c| 115 > security/integrity/ima/ima_appraise.c | 11 +- > security/integrity/integrity.h | 23 ++- > security/integrity/platform_certs/efi_parser.c | 112 > security/integrity/platform_certs/load_uefi.c | 192 > + > .../integrity/platform_certs/platform_keyring.c| 62 +++ > 9 files changed, 527 insertions(+), 38 deletions(-) > create mode 100644 security/integrity/platform_certs/efi_parser.c > create mode 100644 security/integrity/platform_certs/load_uefi.c > create mode 100644 security/integrity/platform_certs/platform_keyring.c >
Re: [PATCH 4/7] efi: Add an EFI signature blob parser
On Sun, 2018-11-25 at 20:44 +0530, Nayna Jain wrote: > From: Dave Howells > > Add a function to parse an EFI signature blob looking for elements of > interest. A list is made up of a series of sublists, where all the > elements in a sublist are of the same type, but sublists can be of > different types. > > For each sublist encountered, the function pointed to by the > get_handler_for_guid argument is called with the type specifier GUID and > returns either a pointer to a function to handle elements of that type or > NULL if the type is not of interest. > > If the sublist is of interest, each element is passed to the handler > function in turn. There are a few checkpatch.pl warnings that need to be addressed, including the missing SPDX license. Mimi > > Signed-off-by: David Howells > Signed-off-by: Nayna Jain > --- > Changelog: > > v0: > - removed the CONFIG EFI_SIGNATURE_LIST_PARSER > - moved efi_parser.c from certs to security/integrity/platform_certs > directory > > include/linux/efi.h| 9 ++ > security/integrity/Makefile| 3 +- > security/integrity/platform_certs/efi_parser.c | 112 > + > 3 files changed, 123 insertions(+), 1 deletion(-) > create mode 100644 security/integrity/platform_certs/efi_parser.c > > diff --git a/include/linux/efi.h b/include/linux/efi.h > index 99cba6fe1234..2016145e2d6d 100644 > --- a/include/linux/efi.h > +++ b/include/linux/efi.h > @@ -1138,6 +1138,15 @@ extern int efi_memattr_apply_permissions(struct > mm_struct *mm, > char * __init efi_md_typeattr_format(char *buf, size_t size, >const efi_memory_desc_t *md); > > + > +typedef void (*efi_element_handler_t)(const char *source, > + const void *element_data, > + size_t element_size); > +extern int __init parse_efi_signature_list( > + const char *source, > + const void *data, size_t size, > + efi_element_handler_t (*get_handler_for_guid)(const efi_guid_t *)); > + > /** > * efi_range_is_wc - check the WC bit on an address range > * @start: starting kvirt address > diff --git a/security/integrity/Makefile b/security/integrity/Makefile > index 046ffc1bb42d..6ee9058866cd 100644 > --- a/security/integrity/Makefile > +++ b/security/integrity/Makefile > @@ -9,7 +9,8 @@ integrity-y := iint.o > integrity-$(CONFIG_INTEGRITY_AUDIT) += integrity_audit.o > integrity-$(CONFIG_INTEGRITY_SIGNATURE) += digsig.o > integrity-$(CONFIG_INTEGRITY_ASYMMETRIC_KEYS) += digsig_asymmetric.o > -integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) += > platform_certs/platform_keyring.o > +integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) += > platform_certs/platform_keyring.o \ > + platform_certs/efi_parser.o > > subdir-$(CONFIG_IMA) += ima > obj-$(CONFIG_IMA)+= ima/ > diff --git a/security/integrity/platform_certs/efi_parser.c > b/security/integrity/platform_certs/efi_parser.c > new file mode 100644 > index ..4e396f98f5c7 > --- /dev/null > +++ b/security/integrity/platform_certs/efi_parser.c > @@ -0,0 +1,112 @@ > +/* EFI signature/key/certificate list parser > + * > + * Copyright (C) 2012, 2016 Red Hat, Inc. All Rights Reserved. > + * Written by David Howells (dhowe...@redhat.com) > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public Licence > + * as published by the Free Software Foundation; either version > + * 2 of the Licence, or (at your option) any later version. > + */ > + > +#define pr_fmt(fmt) "EFI: "fmt > +#include > +#include > +#include > +#include > + > +/** > + * parse_efi_signature_list - Parse an EFI signature list for certificates > + * @source: The source of the key > + * @data: The data blob to parse > + * @size: The size of the data blob > + * @get_handler_for_guid: Get the handler func for the sig type (or NULL) > + * > + * Parse an EFI signature list looking for elements of interest. A list is > + * made up of a series of sublists, where all the elements in a sublist are > of > + * the same type, but sublists can be of different types. > + * > + * For each sublist encountered, the @get_handler_for_guid function is called > + * with the type specifier GUID and returns either a pointer to a function to > + * handle elements of that type or NULL if the type is not of interest. > + * > + * If the sublist is of interest, each element is passed to the handler > + * function in turn. > + * > + * Error EBADMSG is returned if the list doesn't parse correctly and 0 is > + * returned if the list was parsed correctly. No error can be returned from > + * the @get_handler_for_guid function or the element handler function it > + * returns. > + */ > +int __init parse_efi_signature_list( > + const char *source, > + const void *data, size_t size, > +
Re: [PATCH 5/7] efi: Import certificates from UEFI Secure Boot
On Sun, 2018-11-25 at 20:44 +0530, Nayna Jain wrote: > From: Josh Boyer > > New Patch Description: > == > > Secure Boot stores a list of allowed certificates in the 'db' variable. > This patch imports those certificates into the platform keyring. The shim > UEFI bootloader has a similar certificate list stored in the 'MokListRT' > variable. We import those as well. > > Secure Boot also maintains a list of disallowed certificates in the 'dbx' > variable. We load those certificates into the system blacklist keyring > and forbid any kernel signed with those from loading. > > Original Patch Description: > > > Secure Boot stores a list of allowed certificates in the 'db' variable. > This imports those certificates into the system trusted keyring. This > allows for a third party signing certificate to be used in conjunction > with signed modules. By importing the public certificate into the 'db' > variable, a user can allow a module signed with that certificate to > load. The shim UEFI bootloader has a similar certificate list stored > in the 'MokListRT' variable. We import those as well. > > Secure Boot also maintains a list of disallowed certificates in the 'dbx' > variable. We load those certificates into the newly introduced system > blacklist keyring and forbid any module signed with those from loading and > forbid the use within the kernel of any key with a matching hash. > > This facility is enabled by setting CONFIG_LOAD_UEFI_KEYS. There are quite a few checkpatch.pl warnings that need to be addressed, including the missing SPDX license. Mimi
[PATCH 3/3] x86/ima: retry detecting secure boot mode
The secure boot mode may not be detected on boot for some reason (eg.
buggy firmware). This patch attempts one more time to detect the
secure boot mode.
Signed-off-by: Mimi Zohar
---
arch/x86/kernel/Makefile | 2 ++
arch/x86/kernel/ima_arch.c | 46 --
include/linux/ima.h| 2 +-
3 files changed, 47 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index f0910a1e1db7..eb51b0e1189c 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -151,4 +151,6 @@ ifeq ($(CONFIG_X86_64),y)
obj-y += vsmp_64.o
endif
+ifdef CONFIG_EFI
obj-$(CONFIG_IMA) += ima_arch.o
+endif
diff --git a/arch/x86/kernel/ima_arch.c b/arch/x86/kernel/ima_arch.c
index 6c248616ee57..e47cd9390ab4 100644
--- a/arch/x86/kernel/ima_arch.c
+++ b/arch/x86/kernel/ima_arch.c
@@ -7,10 +7,52 @@
extern struct boot_params boot_params;
+static enum efi_secureboot_mode get_sb_mode(void)
+{
+ efi_char16_t efi_SecureBoot_name[] = L"SecureBoot";
+ efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
+ efi_status_t status;
+ unsigned long size;
+ u8 secboot;
+
+ size = sizeof(secboot);
+
+ /* Get variable contents into buffer */
+ status = efi.get_variable(efi_SecureBoot_name, &efi_variable_guid,
+ NULL, &size, &secboot);
+ if (status == EFI_NOT_FOUND) {
+ pr_info("ima: secureboot mode disabled\n");
+ return efi_secureboot_mode_disabled;
+ }
+
+ if (status != EFI_SUCCESS) {
+ pr_info("ima: secureboot mode unknown\n");
+ return efi_secureboot_mode_unknown;
+ }
+
+ if (secboot == 0) {
+ pr_info("ima: secureboot mode disabled\n");
+ return efi_secureboot_mode_disabled;
+ }
+
+ pr_info("ima: secureboot mode enabled\n");
+ return efi_secureboot_mode_enabled;
+}
+
bool arch_ima_get_secureboot(void)
{
- if (efi_enabled(EFI_BOOT) &&
- (boot_params.secure_boot == efi_secureboot_mode_enabled))
+ static enum efi_secureboot_mode sb_mode;
+ static bool initialized;
+
+ if (!initialized && efi_enabled(EFI_BOOT)) {
+ sb_mode = boot_params.secure_boot;
+
+ if (sb_mode == efi_secureboot_mode_unset)
+ sb_mode = get_sb_mode();
+ initialized = true;
+ }
+
+ if (sb_mode == efi_secureboot_mode_enabled)
return true;
else
return false;
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 5ab9134d4fd7..b5e16b8c50b7 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -30,7 +30,7 @@ extern void ima_post_path_mknod(struct dentry *dentry);
extern void ima_add_kexec_buffer(struct kimage *image);
#endif
-#ifdef CONFIG_X86
+#if defined(CONFIG_X86) && defined(CONFIG_EFI)
extern bool arch_ima_get_secureboot(void);
extern const char * const *arch_get_ima_policy(void);
#else
--
2.7.5
[PATCH 2/3] selftests/ima: kexec_load syscall test
The kernel CONFIG_KEXEC_VERIFY_SIG option is limited to verifying a
kernel image's signature, when loaded via the kexec_file_load syscall.
There is no method for verifying a kernel image's signature loaded
via the kexec_load syscall.
This test verifies loading the kernel image via the kexec_load syscall
fails when the kernel CONFIG_KEXEC_VERIFY_SIG option is enabled on
systems with secureboot enabled[1].
[1] Detecting secureboot enabled is architecture specific.
Signed-off-by: Mimi Zohar
---
tools/testing/selftests/Makefile | 1 +
tools/testing/selftests/ima/Makefile | 11 ++
tools/testing/selftests/ima/config | 4 ++
tools/testing/selftests/ima/test_kexec_load.sh | 54 ++
4 files changed, 70 insertions(+)
create mode 100644 tools/testing/selftests/ima/Makefile
create mode 100644 tools/testing/selftests/ima/config
create mode 100755 tools/testing/selftests/ima/test_kexec_load.sh
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index f1fe492c8e17..552c447af03d 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -13,6 +13,7 @@ TARGETS += firmware
TARGETS += ftrace
TARGETS += futex
TARGETS += gpio
+TARGETS += ima
TARGETS += intel_pstate
TARGETS += ipc
TARGETS += kcmp
diff --git a/tools/testing/selftests/ima/Makefile
b/tools/testing/selftests/ima/Makefile
new file mode 100644
index ..0b3adf5444b6
--- /dev/null
+++ b/tools/testing/selftests/ima/Makefile
@@ -0,0 +1,11 @@
+# Makefile for kexec_load
+
+uname_M := $(shell uname -m 2>/dev/null || echo not)
+ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/x86/ -e s/x86_64/x86/)
+
+ifeq ($(ARCH),x86)
+TEST_PROGS := test_kexec_load.sh
+
+include ../lib.mk
+
+endif
diff --git a/tools/testing/selftests/ima/config
b/tools/testing/selftests/ima/config
new file mode 100644
index ..6bc86d4d9bb4
--- /dev/null
+++ b/tools/testing/selftests/ima/config
@@ -0,0 +1,4 @@
+CONFIG_IMA_APPRAISE
+CONFIG_IMA_ARCH_POLICY
+CONFIG_SECURITYFS
+CONFIG_KEXEC_VERIFY_SIG
diff --git a/tools/testing/selftests/ima/test_kexec_load.sh
b/tools/testing/selftests/ima/test_kexec_load.sh
new file mode 100755
index ..3aafc6f6a400
--- /dev/null
+++ b/tools/testing/selftests/ima/test_kexec_load.sh
@@ -0,0 +1,54 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# Loading a kernel image via the kexec_load syscall should fail
+# when the kerne is CONFIG_KEXEC_VERIFY_SIG enabled and the system
+# is booted in secureboot mode.
+
+TEST="$0"
+EFIVARFS="/sys/firmware/efi/efivars"
+rc=0
+
+# Kselftest framework requirement - SKIP code is 4.
+ksft_skip=4
+
+# kexec requires root privileges
+if [ $UID != 0 ]; then
+ echo "$TEST: must be run as root" >&2
+ exit $ksft_skip
+fi
+
+# Make sure that efivars is mounted in the normal location
+if ! grep -q "^\S\+ $EFIVARFS efivarfs" /proc/mounts; then
+ echo "$TEST: efivars is not mounted on $EFIVARFS" >&2
+ exit $ksft_skip
+fi
+
+# Get secureboot mode
+file="$EFIVARFS/SecureBoot-*"
+if [ ! -e $file ]; then
+ echo "$TEST: unknown secureboot mode" >&2
+ exit $ksft_skip
+fi
+secureboot=`hexdump $file | awk '{print substr($4,length($4),1)}'`
+
+# kexec_load should fail in secure boot mode
+KERNEL_IMAGE="/boot/vmlinuz-`uname -r`"
+kexec -l $KERNEL_IMAGE &>> /dev/null
+if [ $? == 0 ]; then
+ kexec -u
+ if [ "$secureboot" == "1" ]; then
+ echo "$TEST: kexec_load succeeded [FAIL]"
+ rc=1
+ else
+ echo "$TEST: kexec_load succeeded [PASS]"
+ fi
+else
+ if [ "$secureboot" == "1" ]; then
+ echo "$TEST: kexec_load failed [PASS]"
+ else
+ echo "$TEST: kexec_load failed [FAIL]"
+ rc=1
+ fi
+fi
+
+exit $rc
--
2.7.5
[PATCH 1/3] ima: add error mesage to kexec_load
Reject the kexec_load syscall with some indication of the problem.
Signed-off-by: Mimi Zohar
---
security/integrity/ima/ima_main.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index 41e4771980d5..df0b2ee49fa2 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -513,8 +513,10 @@ int ima_load_data(enum kernel_load_data_id id)
switch (id) {
case LOADING_KEXEC_IMAGE:
if (IS_ENABLED(CONFIG_KEXEC_VERIFY_SIG)
- && arch_ima_get_secureboot())
+ && arch_ima_get_secureboot()) {
+ pr_err("impossible to appraise a kernel image without a
file descriptor; try using kexec_file_load syscall.\n");
return -EACCES;
+ }
if (ima_enforce && (ima_appraise & IMA_APPRAISE_KEXEC)) {
pr_err("impossible to appraise a kernel image without a
file descriptor; try using kexec_file_load syscall.\n");
--
2.7.5
[PATCH 0/3] selftest/ima: fail kexec_load syscall
The "ima: add support for arch specific policies" patch set introduced architecture specific policies, including an x86 policy which prevents loading a kernel image via the kexec_load syscall. This patch set preq's that patch set, adding a missing kexec_load syscall failure message, extending the existing support for detecting secureboot mode, and defining a kexec_load syscall selftest to simplify testing. To run the kexec_load test requires root privileges. Execute: "sudo make TARGETS=ima kselftest". With secure boot enabled, the kexec_load fails, but the test succeeds. selftests: ima: test_kexec_load.sh ./test_kexec_load.sh: kexec_load failed [PASS] ok 1..1 selftests: ima: test_kexec_load.sh [PASS] Mimi Mimi Zohar (3): ima: add error mesage to kexec_load selftests/ima: kexec_load syscall test x86/ima: retry detecting secure boot mode arch/x86/kernel/Makefile | 2 + arch/x86/kernel/ima_arch.c | 46 +- include/linux/ima.h| 2 +- security/integrity/ima/ima_main.c | 4 +- tools/testing/selftests/Makefile | 1 + tools/testing/selftests/ima/Makefile | 11 ++ tools/testing/selftests/ima/config | 4 ++ tools/testing/selftests/ima/test_kexec_load.sh | 54 ++ 8 files changed, 120 insertions(+), 4 deletions(-) create mode 100644 tools/testing/selftests/ima/Makefile create mode 100644 tools/testing/selftests/ima/config create mode 100755 tools/testing/selftests/ima/test_kexec_load.sh -- 2.7.5
Re: [PATCH v5 0/6] Add support for architecture specific IMA policies
On Fri, 2018-10-05 at 23:10 +0530, Nayna Jain wrote: > From: Nayna Jain > > The architecture specific policy, introduced in this patch set, permits > different architectures to define IMA policy rules based on kernel > configuration and system runtime information. > > For example, on x86, there are two methods of verifying the kexec'ed kernel > image signature - CONFIG_KEXEC_VERIFY_SIG and IMA appraisal policy > KEXEC_KERNEL_CHECK. CONFIG_KEXEC_VERIFY_SIG enforces the kexec_file_load > syscall to verify file signatures, but does not prevent the kexec_load > syscall. The IMA KEXEC_KERNEL_CHECK policy rule verifies the kexec'ed > kernel image, loaded via the kexec_file_load syscall, is validly signed and > prevents loading a kernel image via the kexec_load syscall. When secure > boot is enabled, the kexec'ed kernel image needs to be signed and the > signature verified. In this environment, either method of verifying the > kexec'ed kernel image is acceptable, as long as the kexec_load syscall is > disabled. > > The previous version of this patchset introduced a new IMA policy rule to > disable the kexec_load syscall, when CONFIG_KEXEC_VERIFY_SIG was enabled, > however that is removed from this version by introducing a different > mechanism, as described below. > > The patchset defines an arch_ima_get_secureboot() function to retrieve the > secureboot state of the system. If secureboot is enabled and > CONFIG_KEXEC_VERIFY_SIG is configured, it denies permission to kexec_load > syscall. > > To support architecture specific policies, a new function > arch_get_ima_policy() is defined. This patch set defines IMA > KERNEL_KEXEC_POLICY rules for x86 *only* if CONFIG_KEXEC_VERIFY_SIG is > disabled and secure boot is enabled. > > This patch set includes a patch, which refactors ima_init_policy() to > remove code duplication. Other than a couple of #ifdef's in .c files, which should be converted to use IS_ENABLED(), the patch set is looking really good. thanks! Mimi
Re: [PATCH v4 3/6] ima: refactor ima_init_policy()
Hi Nayna,
On Wed, 2018-09-26 at 17:52 +0530, Nayna Jain wrote:
> +static void add_rules(struct ima_rule_entry *entries, int count,
> + enum policy_rule_list file)
Using "file" to refer to the policy_rule_list enumeration is unusual.
Please change the variable name to something more appropriate.
Mimi
> +{
> + int i = 0;
> +
> + for (i = 0; i < count; i++) {
> + struct ima_rule_entry *entry;
> +
> + if (file & IMA_DEFAULT_POLICY)
> + list_add_tail(&entries[i].list, &ima_default_rules);
> +
> + if (file & IMA_CUSTOM_POLICY) {
> + entry = kmemdup(&entries[i], sizeof(*entry),
> + GFP_KERNEL);
> + if (!entry)
> + continue;
> +
> + INIT_LIST_HEAD(&entry->list);
> + list_add_tail(&entry->list, &ima_policy_rules);
> + }
> + if (entries[i].action == APPRAISE)
> + temp_ima_appraise |= ima_appraise_flag(entries[i].func);
> + if (entries[i].func == POLICY_CHECK)
> + temp_ima_appraise |= IMA_APPRAISE_POLICY;
> + }
> +}
> +
Re: [PATCH v4 6/6] x86/ima: define arch_get_ima_policy() for x86
Hi Eric, Nayna,
On Wed, 2018-09-26 at 17:52 +0530, Nayna Jain wrote:
> From: Eric Richter
> This patch implements an example arch-specific IMA policy for x86 to
> enable measurement and appraisal of any kernel image loaded for kexec,
> when CONFIG_KEXEC_VERIFY_SIG is not enabled.
>
> For systems with CONFIG_KEXEC_VERIFY_SIG enabled, only the measurement
> rule is enabled, not the IMA-appraisal rule.
The patch itself looks good, but this patch description explains
"what" the patch is doing, not "why". Missing is the motivation for
the patch.
Mimi
>
> Signed-off-by: Eric Richter
> - Removed the policy KEXEC_ORIG_KERNEL_CHECK which was defined to
> disable the kexec_load syscall.
> - arch_get_ima_policy() uses arch_ima_get_secureboot() to get secureboot
> state
> Signed-off-by: Nayna Jain
> ---
> arch/x86/kernel/ima_arch.c | 18 ++
> include/linux/ima.h| 4
> security/integrity/ima/Kconfig | 8
> 3 files changed, 30 insertions(+)
>
> diff --git a/arch/x86/kernel/ima_arch.c b/arch/x86/kernel/ima_arch.c
> index bb5a88d2b271..245976e49a55 100644
> --- a/arch/x86/kernel/ima_arch.c
> +++ b/arch/x86/kernel/ima_arch.c
> @@ -15,3 +15,21 @@ bool arch_ima_get_secureboot(void)
> else
> return false;
> }
> +
> +/* arch rules for audit and user mode */
> +static const char * const sb_arch_rules[] = {
> +#ifndef CONFIG_KEXEC_VERIFY_SIG
> + "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig",
> +#endif /* CONFIG_KEXEC_VERIFY_SIG */
> + "measure func=KEXEC_KERNEL_CHECK",
> + NULL
> +};
> +
> +#ifdef CONFIG_IMA_ARCH_POLICY
> +const char * const *arch_get_ima_policy(void)
> +{
> + if (arch_ima_get_secureboot())
> + return sb_arch_rules;
> + return NULL;
> +}
> +#endif
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 350fa957f8a6..dabd3abdf671 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -39,10 +39,14 @@ static inline bool arch_ima_get_secureboot(void)
> }
> #endif
>
> +#if defined(CONFIG_X86) && defined(CONFIG_IMA_ARCH_POLICY)
> +extern const char * const *arch_get_ima_policy(void);
> +#else
> static inline const char * const *arch_get_ima_policy(void)
> {
> return NULL;
> }
> +#endif
>
> #else
> static inline int ima_bprm_check(struct linux_binprm *bprm)
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 13b446328dda..97609a76aa14 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -157,6 +157,14 @@ config IMA_APPRAISE
>
> If unsure, say N.
>
> +config IMA_ARCH_POLICY
> +bool "Enable loading an IMA architecture specific policy"
> +depends on KEXEC_VERIFY_SIG || IMA_APPRAISE &&
> INTEGRITY_ASYMMETRIC_KEYS
> +default n
> +help
> + This option enables loading an IMA architecture specific policy
> + based on run time secure boot flags.
> +
> config IMA_APPRAISE_BUILD_POLICY
> bool "IMA build time configured policy rules"
> depends on IMA_APPRAISE && INTEGRITY_ASYMMETRIC_KEYS
Re: [PATCH v4 4/6] ima: add support for arch specific policies
On Wed, 2018-09-26 at 17:52 +0530, Nayna Jain wrote:
> Builtin IMA policies can be enabled on the boot command line, and replaced
> with a custom policy, normally during early boot in the initramfs. Build
> time IMA policy rules were recently added. These rules are automatically
> enabled on boot and persist after loading a custom policy.
>
> There is a need for yet another type of policy, an architecture specific
> policy, which is derived at runtime during kernel boot, based on the
> runtime secure boot flags. Like the build time policy rules, these rules
> persist after loading a custom policy.
>
> This patch adds support for loading an architecture specific IMA policy.
Thanks! Just a couple of minor comments/changes.
>
> Signed-off-by: Nayna Jain
> - Defined function to convert the arch policy strings to an array of
> ima_entry_rules. The memory can then be freed after loading a custom
> policy.
> - Rename ima_get_arch_policy to arch_get_ima_policy.
> Signed-off-by: Mimi Zohar
> - Modified ima_init_arch_policy() and ima_init_policy() to use add_rules()
> from previous patch.
> Signed-off-by: Nayna Jain
> ---
> include/linux/ima.h | 5 +++
> security/integrity/ima/ima_policy.c | 70
> +++--
> 2 files changed, 73 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 4852255aa4f4..350fa957f8a6 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -39,6 +39,11 @@ static inline bool arch_ima_get_secureboot(void)
> }
> #endif
>
> +static inline const char * const *arch_get_ima_policy(void)
> +{
> + return NULL;
> +}
> +
> #else
> static inline int ima_bprm_check(struct linux_binprm *bprm)
> {
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c
> index d5b327320d3a..5fb4b0c123a3 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -20,6 +20,7 @@
> #include
> #include
> #include
> +#include
>
> #include "ima.h"
>
> @@ -195,6 +196,9 @@ static struct ima_rule_entry secure_boot_rules[]
> __ro_after_init = {
>.flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
> };
>
> +/* An array of architecture specific rules */
> +struct ima_rule_entry *arch_policy_entry __ro_after_init;
> +
> static LIST_HEAD(ima_default_rules);
> static LIST_HEAD(ima_policy_rules);
> static LIST_HEAD(ima_temp_rules);
> @@ -492,7 +496,6 @@ static void add_rules(struct ima_rule_entry *entries, int
> count,
> if (!entry)
> continue;
>
> - INIT_LIST_HEAD(&entry->list);
> list_add_tail(&entry->list, &ima_policy_rules);
Assuming that INIT_LIST_HEAD isn't needed, removing it belongs in
"[PATCH v4 3/6] ima: refactor ima_init_policy()".
> }
> if (entries[i].action == APPRAISE)
> @@ -502,6 +505,48 @@ static void add_rules(struct ima_rule_entry *entries,
> int count,
> }
> }
>
> +static int ima_parse_rule(char *rule, struct ima_rule_entry *entry);
> +
> +static int __init ima_init_arch_policy(void)
> +{
> + const char * const *arch_rules;
> + const char * const *rules;
> + int arch_entries = 0;
> + int i = 0;
> +
> + arch_rules = arch_get_ima_policy();
> + if (!arch_rules)
> + return arch_entries;
> +
> + /* Get number of rules */
> + for (rules = arch_rules; *rules != NULL; rules++)
> + arch_entries++;
> +
> + arch_policy_entry = kcalloc(arch_entries + 1,
> + sizeof(*arch_policy_entry), GFP_KERNEL);
> + if (!arch_policy_entry)
> + return 0;
> +
> + /* Convert each policy string rules to struct ima_rule_entry format */
> + for (rules = arch_rules, i = 0; *rules != NULL; rules++) {
> + char rule[255];
> + int result;
> +
> + result = strlcpy(rule, *rules, sizeof(rule));
> +
> + INIT_LIST_HEAD(&arch_policy_entry[i].list);
> + result = ima_parse_rule(rule, &arch_policy_entry[i]);
> + if (result) {
> + pr_warn("Skipping unknown architecture policy rule:
> %s\n", rule);
> + memset(&arch_policy_entry[i], 0,
> +sizeof(*arch_policy_entry));
> + continue;
> + }
> + i++;
> + }
> + return i;
> +}
> +
> /**
> * ima_init_policy - initialize the default me
Re: [PATCH v4 5/6] ima: add support for external setting of ima_appraise
Hi Nayna,
On Wed, 2018-09-26 at 17:52 +0530, Nayna Jain wrote:
> The "ima_appraise" mode defaults to enforcing, unless configured to allow
> the boot command line "ima_appraise" option. This patch explicitly sets the
> "ima_appraise" mode for the arch specific policy setting.
Eventually this patch might be needed if/when we need to differentiate
between different secure boot modes.
Only if CONFIG_IMA_APPRAISE_BOOTPARAM is enabled, can the IMA appraise
mode be modified on the boot command line. Instead of this patch, how
about making the ability to change the IMA appraise mode also
dependent on CONFIG_IMA_ARCH_POLICY not being enabled?
Mimi
>
> Signed-off-by: Nayna Jain
> ---
> security/integrity/ima/ima.h | 5 +
> security/integrity/ima/ima_appraise.c | 11 +--
> security/integrity/ima/ima_policy.c | 5 -
> 3 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 588e4813370c..6e5fa7c42809 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -248,6 +248,7 @@ enum hash_algo ima_get_hash_algo(struct
> evm_ima_xattr_data *xattr_value,
>int xattr_len);
> int ima_read_xattr(struct dentry *dentry,
> struct evm_ima_xattr_data **xattr_value);
> +void set_ima_appraise(char *str);
>
> #else
> static inline int ima_appraise_measurement(enum ima_hooks func,
> @@ -290,6 +291,10 @@ static inline int ima_read_xattr(struct dentry *dentry,
> return 0;
> }
>
> +static inline void set_ima_appraise(char *str)
> +{
> +}
> +
> #endif /* CONFIG_IMA_APPRAISE */
>
> /* LSM based policy rules require audit */
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index 8bd7a0733e51..e061613bcb87 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -18,15 +18,22 @@
>
> #include "ima.h"
>
> -static int __init default_appraise_setup(char *str)
> +void set_ima_appraise(char *str)
> {
> -#ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
> if (strncmp(str, "off", 3) == 0)
> ima_appraise = 0;
> else if (strncmp(str, "log", 3) == 0)
> ima_appraise = IMA_APPRAISE_LOG;
> else if (strncmp(str, "fix", 3) == 0)
> ima_appraise = IMA_APPRAISE_FIX;
> + else if (strncmp(str, "enforce", 7) == 0)
> + ima_appraise = IMA_APPRAISE_ENFORCE;
> +}
> +
> +static int __init default_appraise_setup(char *str)
> +{
> +#ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
> + set_ima_appraise(str);
> #endif
> return 1;
> }
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c
> index 5fb4b0c123a3..410fee31b162 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -585,9 +585,12 @@ void __init ima_init_policy(void)
> arch_entries = ima_init_arch_policy();
> if (!arch_entries)
> pr_info("No architecture policies found\n");
> - else
> + else {
> add_rules(arch_policy_entry, arch_entries,
> IMA_DEFAULT_POLICY | IMA_CUSTOM_POLICY);
> + if (temp_ima_appraise)
> + set_ima_appraise("enforce");
> + }
>
> /*
>* Insert the builtin "secure_boot" policy rules requiring file
Re: [PATCH v4 3/6] ima: refactor ima_init_policy()
On Wed, 2018-09-26 at 17:52 +0530, Nayna Jain wrote:
> This patch removes the code duplication in ima_init_policy() by defining
> a new function named add_rules().
Thanks! The patch looks good, but let's expand on this just a bit.
Rules can be added to the initial IMA policy, the custom policy or
both, based on a mask (IMA_DEFAULT_POLICY, IMA_CUSTOM_POLICY).
Mimi
>
> Signed-off-by: Nayna Jain
> ---
> security/integrity/ima/ima_policy.c | 98
> +
> 1 file changed, 57 insertions(+), 41 deletions(-)
>
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c
> index 8c9499867c91..d5b327320d3a 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -58,6 +58,8 @@ enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE,
> LSM_OBJ_TYPE,
>
> enum policy_types { ORIGINAL_TCB = 1, DEFAULT_TCB };
>
> +enum policy_rule_list { IMA_DEFAULT_POLICY = 1, IMA_CUSTOM_POLICY };
> +
> struct ima_rule_entry {
> struct list_head list;
> int action;
> @@ -473,6 +475,33 @@ static int ima_appraise_flag(enum ima_hooks func)
> return 0;
> }
>
> +static void add_rules(struct ima_rule_entry *entries, int count,
> + enum policy_rule_list file)
> +{
> + int i = 0;
> +
> + for (i = 0; i < count; i++) {
> + struct ima_rule_entry *entry;
> +
> + if (file & IMA_DEFAULT_POLICY)
> + list_add_tail(&entries[i].list, &ima_default_rules);
> +
> + if (file & IMA_CUSTOM_POLICY) {
> + entry = kmemdup(&entries[i], sizeof(*entry),
> + GFP_KERNEL);
> + if (!entry)
> + continue;
> +
> + INIT_LIST_HEAD(&entry->list);
> + list_add_tail(&entry->list, &ima_policy_rules);
> + }
> + if (entries[i].action == APPRAISE)
> + temp_ima_appraise |= ima_appraise_flag(entries[i].func);
> + if (entries[i].func == POLICY_CHECK)
> + temp_ima_appraise |= IMA_APPRAISE_POLICY;
> + }
> +}
> +
> /**
> * ima_init_policy - initialize the default measure rules.
> *
> @@ -481,28 +510,23 @@ static int ima_appraise_flag(enum ima_hooks func)
> */
> void __init ima_init_policy(void)
> {
> - int i, measure_entries, appraise_entries, secure_boot_entries;
> -
> - /* if !ima_policy set entries = 0 so we load NO default rules */
> - measure_entries = ima_policy ? ARRAY_SIZE(dont_measure_rules) : 0;
> - appraise_entries = ima_use_appraise_tcb ?
> - ARRAY_SIZE(default_appraise_rules) : 0;
> - secure_boot_entries = ima_use_secure_boot ?
> - ARRAY_SIZE(secure_boot_rules) : 0;
> + int build_appraise_entries;
>
> - for (i = 0; i < measure_entries; i++)
> - list_add_tail(&dont_measure_rules[i].list, &ima_default_rules);
> + /* if !ima_policy, we load NO default rules */
> + if (ima_policy)
> + add_rules(dont_measure_rules, ARRAY_SIZE(dont_measure_rules),
> + IMA_DEFAULT_POLICY);
>
> switch (ima_policy) {
> case ORIGINAL_TCB:
> - for (i = 0; i < ARRAY_SIZE(original_measurement_rules); i++)
> - list_add_tail(&original_measurement_rules[i].list,
> - &ima_default_rules);
> + add_rules(original_measurement_rules,
> + ARRAY_SIZE(original_measurement_rules),
> + IMA_DEFAULT_POLICY);
> break;
> case DEFAULT_TCB:
> - for (i = 0; i < ARRAY_SIZE(default_measurement_rules); i++)
> - list_add_tail(&default_measurement_rules[i].list,
> - &ima_default_rules);
> + add_rules(default_measurement_rules,
> + ARRAY_SIZE(default_measurement_rules),
> + IMA_DEFAULT_POLICY);
> default:
> break;
> }
> @@ -511,38 +535,30 @@ void __init ima_init_policy(void)
>* Insert the builtin "secure_boot" policy rules requiring file
>* signatures, prior to any other appraise rules.
>*/
> - for (i = 0; i < secure_boot_entries; i++) {
> - list_add_tail(&secure_boot_rules[i].list, &ima_default_rules);
> - temp_ima_appraise |=
> - ima_appraise_flag(secure_boot_rules[i].func);
> - }
> + if (ima_use_secure_boot)
> + add_rules(secure_boot_rules, ARRAY_SIZE(secure_boot_rules),
> + IMA_DEFAULT_POLICY);
>
> /*
>* Insert the build time appraise rules requiring file signatures
>* for both the initial and custom policies, prior to other appraise
> - * rules.
> + * rules. As the secure boot rules includes all of the build time
> +
Re: [PATCH v4 2/6] ima: prevent kexec_load syscall based on runtime secureboot flag
[Cc'ing the kexec mailing list, and Seth]
On Wed, 2018-09-26 at 17:52 +0530, Nayna Jain wrote:
> When CONFIG_KEXEC_VERIFY_SIG is enabled, the kexec_file_load syscall
> requires the kexec'd kernel image to be signed. Distros are concerned
> about totally disabling the kexec_load syscall. As a compromise, the
> kexec_load syscall will only be disabled when CONFIG_KEXEC_VERIFY_SIG
> is configured and the system is booted with secureboot enabled.
>
> This patch disables the kexec_load syscall only for systems booted with
> secureboot enabled.
>
> Signed-off-by: Nayna Jain
Nice!
Mimi
> ---
> security/integrity/ima/ima_main.c | 17 +++--
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index dce0a8a217bb..bdb6e5563d05 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -505,20 +505,24 @@ int ima_post_read_file(struct file *file, void *buf,
> loff_t size,
> */
> int ima_load_data(enum kernel_load_data_id id)
> {
> - bool sig_enforce;
> + bool ima_enforce, sig_enforce;
>
> - if ((ima_appraise & IMA_APPRAISE_ENFORCE) != IMA_APPRAISE_ENFORCE)
> - return 0;
> + ima_enforce =
> + (ima_appraise & IMA_APPRAISE_ENFORCE) == IMA_APPRAISE_ENFORCE;
>
> switch (id) {
> case LOADING_KEXEC_IMAGE:
> - if (ima_appraise & IMA_APPRAISE_KEXEC) {
> +#ifdef CONFIG_KEXEC_VERIFY_SIG
> + if (arch_ima_get_secureboot())
> + return -EACCES;
> +#endif
> + if (ima_enforce && (ima_appraise & IMA_APPRAISE_KEXEC)) {
> pr_err("impossible to appraise a kernel image without a
> file descriptor; try using kexec_file_load syscall.\n");
> return -EACCES; /* INTEGRITY_UNKNOWN */
> }
> break;
> case LOADING_FIRMWARE:
> - if (ima_appraise & IMA_APPRAISE_FIRMWARE) {
> + if (ima_enforce && (ima_appraise & IMA_APPRAISE_FIRMWARE)) {
> pr_err("Prevent firmware sysfs fallback loading.\n");
> return -EACCES; /* INTEGRITY_UNKNOWN */
> }
> @@ -526,7 +530,8 @@ int ima_load_data(enum kernel_load_data_id id)
> case LOADING_MODULE:
> sig_enforce = is_module_sig_enforced();
>
> - if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES)) {
> + if (ima_enforce && (!sig_enforce
> + && (ima_appraise & IMA_APPRAISE_MODULES))) {
> pr_err("impossible to appraise a module without a file
> descriptor. sig_enforce kernel parameter might help\n");
> return -EACCES; /* INTEGRITY_UNKNOWN */
> }
Re: [PATCH v4 1/6] x86/ima: define arch_ima_get_secureboot
[Cc'ing the kexec mailing list, and Seth]
On Wed, 2018-09-26 at 17:52 +0530, Nayna Jain wrote:
> Distros are concerned about totally disabling the kexec_load syscall.
> As a compromise, the kexec_load syscall will only be disabled when
> CONFIG_KEXEC_VERIFY_SIG is configured and the system is booted with
> secureboot enabled.
>
> This patch defines the new arch specific function called
> arch_ima_get_secureboot() to retrieve the secureboot state of the system.
>
> Signed-off-by: Nayna Jain
> Suggested-by: Seth Forshee
Nice!
Mimi
> ---
> arch/x86/kernel/Makefile | 2 ++
> arch/x86/kernel/ima_arch.c | 17 +
> include/linux/ima.h| 9 +
> 3 files changed, 28 insertions(+)
> create mode 100644 arch/x86/kernel/ima_arch.c
>
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 02d6f5cf4e70..f32406e51424 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -149,3 +149,5 @@ ifeq ($(CONFIG_X86_64),y)
> obj-$(CONFIG_MMCONF_FAM10H) += mmconf-fam10h_64.o
> obj-y += vsmp_64.o
> endif
> +
> +obj-$(CONFIG_IMA)+= ima_arch.o
> diff --git a/arch/x86/kernel/ima_arch.c b/arch/x86/kernel/ima_arch.c
> new file mode 100644
> index ..bb5a88d2b271
> --- /dev/null
> +++ b/arch/x86/kernel/ima_arch.c
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2018 IBM Corporation
> + */
> +#include
> +#include
> +
> +extern struct boot_params boot_params;
> +
> +bool arch_ima_get_secureboot(void)
> +{
> + if (efi_enabled(EFI_BOOT) &&
> + (boot_params.secure_boot == efi_secureboot_mode_enabled))
> + return true;
> + else
> + return false;
> +}
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 84806b54b50a..4852255aa4f4 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -30,6 +30,15 @@ extern void ima_post_path_mknod(struct dentry *dentry);
> extern void ima_add_kexec_buffer(struct kimage *image);
> #endif
>
> +#ifdef CONFIG_X86
> +extern bool arch_ima_get_secureboot(void);
> +#else
> +static inline bool arch_ima_get_secureboot(void)
> +{
> + return false;
> +}
> +#endif
> +
> #else
> static inline int ima_bprm_check(struct linux_binprm *bprm)
> {
Re: [PATCH 3/4] ima: add support for KEXEC_ORIG_KERNEL_CHECK
On Fri, 2018-08-03 at 11:16 -0500, Seth Forshee wrote: > On Fri, Aug 03, 2018 at 10:54:59AM -0400, Mimi Zohar wrote: > > On Fri, 2018-08-03 at 08:11 -0500, Seth Forshee wrote: > > > On Wed, Jul 25, 2018 at 06:31:59PM -0500, Eric Richter wrote: > > > > IMA can verify the signature of kernel images loaded with > > > > kexec_file_load, > > > > but can not verify images loaded with the regular kexec_load syscall. > > > > Therefore, the appraisal will automatically fail during kexec_load when > > > > an > > > > appraise policy rule is set for func=KEXEC_KERNEL_CHECK. This can be > > > > used > > > > to effectively disable the kexec_load syscall, while still allowing the > > > > kexec_file_load to operate so long as the target kernel image is signed. > > > > > > > > However, this conflicts with CONFIG_KEXEC_VERIFY_SIG. If that option is > > > > enabled and there is an appraise rule set, then the target kernel would > > > > have to be verifiable by both IMA and the architecture specific kernel > > > > verification procedure. > > > > > > > > This patch adds a new func= for IMA appraisal specifically for the > > > > original > > > > kexec_load syscall. Therefore, the kexec_load syscall can be effectively > > > > disabled via IMA policy, leaving the kexec_file_load syscall able to do > > > > its > > > > own signature verification, and not require it to be signed via IMA. To > > > > retain compatibility, the existing func=KEXEC_KERNEL_CHECK flag is > > > > unchanged, and thus enables appraisal for both kexec syscalls. > > > > > > This seems like a roundabout way to disallow the kexec_load syscall. > > > Wouldn't it make more sense to simply disallow kexec_load any time > > > CONFIG_KEXEC_VERIFY_SIG is enabled, since it effectively renders that > > > option impotent? Or has that idea already been rejected? > > > > Agreed! We can modify the "case LOADING_KEXEC_IMAGE" in > > ima_load_data() to prevent the kexec_load based on > > CONFIG_KEXEC_VERIFY_SIG. > > > > The architecture specific policy would only include the IMA appraise > > rule if CONFIG_KEXEC_VERIFY_SIG was not defined. > > After looking at this some more I'm having second thoughts about my > suggestion. As a distro we produce a kernel that needs to be flexible > enough for a variety of scenarios, and if we completely close off the > ability to load an unsigned kernel for kexec that's almost certainly > going to end up breaking some use cases. > > So I think it is necessary to make this a run-time decision rather than > a compile-time decision. The patch as provided does this based on > whether or not the kernel was booted under secure boot. That might be > reasonable, though I still find this mechanism kind of awkward. Right, the above change is almost right. Instead of preventing the kexec_load syscall based on CONFIG_KEXEC_VERIFY_SIG it should be based on a runtime secure boot flag. Only if there is an arch specific secure boot function and the secure boot flag is enabled, would the kexec_load be disabled. Without an architecture specific secure boot function, the existing IMA code would fail the kexec_load syscall. > It seems > like ideally there would instead be some logic that would accept the > image if the KEXEC_VERIFY_SIG verification had passed, and otherwise > require IMA signature verification. True, but for now to coordinate between the two signature verification methods, only if CONFIG_KEXEC_VERIFY_SIG is not enabled would an IMA architecture specific rule be defined. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] ima: add support for KEXEC_ORIG_KERNEL_CHECK
On Fri, 2018-08-03 at 08:11 -0500, Seth Forshee wrote: > On Wed, Jul 25, 2018 at 06:31:59PM -0500, Eric Richter wrote: > > IMA can verify the signature of kernel images loaded with kexec_file_load, > > but can not verify images loaded with the regular kexec_load syscall. > > Therefore, the appraisal will automatically fail during kexec_load when an > > appraise policy rule is set for func=KEXEC_KERNEL_CHECK. This can be used > > to effectively disable the kexec_load syscall, while still allowing the > > kexec_file_load to operate so long as the target kernel image is signed. > > > > However, this conflicts with CONFIG_KEXEC_VERIFY_SIG. If that option is > > enabled and there is an appraise rule set, then the target kernel would > > have to be verifiable by both IMA and the architecture specific kernel > > verification procedure. > > > > This patch adds a new func= for IMA appraisal specifically for the original > > kexec_load syscall. Therefore, the kexec_load syscall can be effectively > > disabled via IMA policy, leaving the kexec_file_load syscall able to do its > > own signature verification, and not require it to be signed via IMA. To > > retain compatibility, the existing func=KEXEC_KERNEL_CHECK flag is > > unchanged, and thus enables appraisal for both kexec syscalls. > > This seems like a roundabout way to disallow the kexec_load syscall. > Wouldn't it make more sense to simply disallow kexec_load any time > CONFIG_KEXEC_VERIFY_SIG is enabled, since it effectively renders that > option impotent? Or has that idea already been rejected? Agreed! We can modify the "case LOADING_KEXEC_IMAGE" in ima_load_data() to prevent the kexec_load based on CONFIG_KEXEC_VERIFY_SIG. The architecture specific policy would only include the IMA appraise rule if CONFIG_KEXEC_VERIFY_SIG was not defined. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
On Thu, 2018-05-03 at 22:23 +, Luis R. Rodriguez wrote: > On Tue, May 01, 2018 at 03:27:27PM -0400, Mimi Zohar wrote: > > On Tue, 2018-05-01 at 21:11 +0200, Hans de Goede wrote: > > > Only the pre hook? I believe the post-hook should still be called too, > > > right? So that we've hashes of all loaded firmwares in the IMA core. > > > > Good catch! Right, if IMA-measurement is enabled, then we would want > > to add the measurement. > > Mimi, just a heads up, we only use the post hook for the syfs fallback > mechanism, ie, we don't even use the post hook for direct fs lookup. > Do we want that there? The direct fs lookup calls kernel_read_file_from_path(), which calls the security_kernel_read_file() and security_kernel_post_read_file() hooks. So there is no need to add a direct call to either of these security calls. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
On Tue, 2018-05-01 at 21:11 +0200, Hans de Goede wrote:
> Hi,
>
> On 01-05-18 16:36, Mimi Zohar wrote:
> > [Cc'ing linux-security]
> >
> > On Sun, 2018-04-29 at 11:35 +0200, Hans de Goede wrote:
> > [...]
> >> diff --git a/drivers/base/firmware_loader/fallback_efi.c
> >> b/drivers/base/firmware_loader/fallback_efi.c
> >> new file mode 100644
> >> index ..82ba82f48a79
> >> --- /dev/null
> >> +++ b/drivers/base/firmware_loader/fallback_efi.c
> >> @@ -0,0 +1,51 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +
> >> +#include "fallback.h"
> >> +#include "firmware.h"
> >> +
> >> +int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv,
> >> + enum fw_opt *opt_flags, int ret)
> >> +{
> >> + enum kernel_read_file_id id = READING_FIRMWARE;
> >
> > Please define a new kernel_read_file_id for this (eg.
> > READING_FIRMWARE_EFI_EMBEDDED).
>
> Are you sure, I wonder how useful it is to add a new
> kernel_read_file_id every time a new way to get firmware
> comes up?
>
> I especially wonder about the sense in adding a new id
> given that the quite old FIRMWARE_PREALLOC_BUFFER is
> still not supported / checked properly by the security code.
I posted patches earlier today[1], which address this. Patch 5/6 just
makes it equivalent to READING_FIRMWARE. Patch 6/6 questions whether
the device has access to the pre-allocated buffer *before* the
signature has been verified.
[1] kernsec.org/pipermail/linux-security-module-archive/2018-May/006639.html
>
> Anyways I can add a new id if you want me to, what about
> when fw_get_efi_embedded_fw is reading into a driver allocated
> buffer, do you want a separate EADING_FIRMWARE_EFI_EMBEDDED_PREALLOC_BUFFER
> for that ?
Without the kernel being able to verify the firmware's signature, I'm
not sure it makes much of a difference.
>
> >
> >> + size_t size, max = INT_MAX;
> >> + int rc;
> >> +
> >> + if (!dev)
> >> + return ret;
> >> +
> >> + if (!device_property_read_bool(dev, "efi-embedded-firmware"))
> >> + return ret;
> >
> > Instead of calling security_kernel_post_read_file(), either in
> > device_property_read_bool() or here call security_kernel_read_file().
> >
> > The pre read call is for deciding whether to allow this call
> > independent of the firmware being loaded, whereas the post security
> > call is currently being used by IMA-appraisal for verifying a
> > signature. There might be other LSMs using the post hook as well. As
> > there is no kernel signature associated with this firmware, use the
> > security pre read_file hook.
>
> Only the pre hook? I believe the post-hook should still be called too,
> right? So that we've hashes of all loaded firmwares in the IMA core.
Good catch! Right, if IMA-measurement is enabled, then we would want
to add the measurement.
Mimi
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
[Cc'ing linux-security]
On Sun, 2018-04-29 at 11:35 +0200, Hans de Goede wrote:
[...]
> diff --git a/drivers/base/firmware_loader/fallback_efi.c
> b/drivers/base/firmware_loader/fallback_efi.c
> new file mode 100644
> index ..82ba82f48a79
> --- /dev/null
> +++ b/drivers/base/firmware_loader/fallback_efi.c
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include "fallback.h"
> +#include "firmware.h"
> +
> +int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv,
> +enum fw_opt *opt_flags, int ret)
> +{
> + enum kernel_read_file_id id = READING_FIRMWARE;
Please define a new kernel_read_file_id for this (eg.
READING_FIRMWARE_EFI_EMBEDDED).
> + size_t size, max = INT_MAX;
> + int rc;
> +
> + if (!dev)
> + return ret;
> +
> + if (!device_property_read_bool(dev, "efi-embedded-firmware"))
> + return ret;
Instead of calling security_kernel_post_read_file(), either in
device_property_read_bool() or here call security_kernel_read_file().
The pre read call is for deciding whether to allow this call
independent of the firmware being loaded, whereas the post security
call is currently being used by IMA-appraisal for verifying a
signature. There might be other LSMs using the post hook as well. As
there is no kernel signature associated with this firmware, use the
security pre read_file hook.
thanks,
Mimi
> +
> + *opt_flags |= FW_OPT_NO_WARN | FW_OPT_NOCACHE | FW_OPT_NOFALLBACK;
> +
> + /* Already populated data member means we're loading into a buffer */
> + if (fw_priv->data) {
> + id = READING_FIRMWARE_PREALLOC_BUFFER;
> + max = fw_priv->allocated_size;
> + }
> +
> + rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data, &size, max);
> + if (rc) {
> + dev_warn(dev, "Firmware %s not in EFI\n", fw_priv->fw_name);
> + return ret;
> + }
> +
> + rc = security_kernel_post_read_file(NULL, fw_priv->data, size, id);
> + if (rc) {
> + if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
> + vfree(fw_priv->data);
> + fw_priv->data = NULL;
> + }
> + return rc;
> + }
> +
> + dev_dbg(dev, "using efi-embedded fw %s\n", fw_priv->fw_name);
> + fw_priv->size = size;
> + fw_state_done(fw_priv);
> + return 0;
> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/5] efi: Add embedded peripheral firmware support
On Tue, 2018-04-24 at 23:42 +, Luis R. Rodriguez wrote:
> On Tue, Apr 24, 2018 at 12:07:01PM -0400, Mimi Zohar wrote:
> > On Tue, 2018-04-24 at 17:09 +0200, Hans de Goede wrote:
> > > Hi,
> > >
> > > On 23-04-18 23:11, Luis R. Rodriguez wrote:
> > > > Hans, please see use of READING_FIRMWARE_PREALLOC_BUFFER, we'll need a
> > > > new ID
> > > > and security for this type of request so IMA can reject it if the
> > > > policy is
> > > > configured for it.
> > >
> > > Hmm, interesting, actually it seems like the whole existence
> > > of READING_FIRMWARE_PREALLOC_BUFFER is a mistake,
>
> request_firmware_into_buf() was merged without my own review, however,
> the ID thing did get review from Mimi:
>
> https://patchwork.kernel.org/patch/9074611/
>
> The ID is not for IMA alone, its for any LSM to decide what to do.
> Note Mimi asked for READING_FIRMWARE_DMA if such buffer was in DMA,
> otherise READING_FIRMWARE_PREALLOC_BUFFER was suggested.
>
> > > the IMA
> > > framework really does not care if we are loading the firmware
> > > into memory allocated by the firmware-loader code, or into
> > > memory allocated by the device-driver requesting the firmware.
>
> That's up to LSM folks to decide. We have these so far:
>
> #define __kernel_read_file_id(id) \
>
> id(UNKNOWN, unknown)\
>
> id(FIRMWARE, firmware) \
>
> id(FIRMWARE_PREALLOC_BUFFER, firmware) \
>
> id(MODULE, kernel-module) \
>
> id(KEXEC_IMAGE, kexec-image)\
>
> id(KEXEC_INITRAMFS, kexec-initramfs)\
>
> id(POLICY, security-policy) \
>
> id(X509_CERTIFICATE, x509-certificate) \
>
> id(MAX_ID, )
>
> The first type of IDs added was about type of files the kernel
> LSMs may want to do different things for.
>
> Mimi why did you want a separate ID for it back before?
The point of commit a098ecd2fa7d ("firmware: support loading into a
pre-allocated buffer") is to avoid reading the firmware into kernel
memory and then copying it "to it's final resting place". My concern
is that if the device driver has access to the buffer, it could access
the buffer prior to the firmware's signature having been verified by
the kernel.
In tightly controlled environments interested in limiting which signed
firmware version is loaded, require's the device driver not having
access to the buffer until after the signature has been verified by
the kernel (eg. IMA-appraisal).
>
> I should note now that request_firmware_into_buf() and its
> READING_FIRMWARE_PREALLOC_BUFFER was to enable a driver on memory constrained
> devices. The files are large (commit says 16 MiB).
>
> I've heard of larger possible files with remoteproc and with Android using
> the custom fallback mechanism -- which could mean a proprietary tool
> fetching firmware from a random special place on a device.
>
> I could perhaps imagine an LSM which may be aware of such type of
> arrangement may want to do its own vetting of some sort, but this
> would not be specific to READING_FIRMWARE_PREALLOC_BUFFER, but rather
> the custom fallback mechaism.
>
> Whether or not the buffer was preallocated by the driver seems a little
> odd for security folks to do something different with it. Security LSM
> folks please chime in.
>
> I could see a bit more of a use case for an ID for firmware scraped
> from EFI, which Hans' patch will provide. But that *also* should get
> good review from other LSM folks.
>
> One of the issues with accepting more IDs loosely is where do we
> stop though? If no one really is using READING_FIRMWARE_PREALLOC_BUFFER
> I'd say lets remove it. Likewise, for this EFI thing I'd like an idea
> if we really are going to have users for it.
>
> If its of any help --
>
> drivers/soc/qcom/mdt_loader.c is the only driver currently using
> request_firmware_into_buf() however I'll note qcom_mdt_load() is used in many
> other drivers so they are wrappers around request_firmware_into_buf():
>
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c: * adreno_request_fw() handles this,
> but qcom_mdt_load() does
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c: ret = qcom_mdt_load(dev, fw,
> fwname, GPU_PAS_ID
Re: [PATCH v3 2/5] efi: Add embedded peripheral firmware support
On Tue, 2018-04-24 at 17:09 +0200, Hans de Goede wrote:
> Hi,
>
> On 23-04-18 23:11, Luis R. Rodriguez wrote:
> > Hans, please see use of READING_FIRMWARE_PREALLOC_BUFFER, we'll need a new
> > ID
> > and security for this type of request so IMA can reject it if the policy is
> > configured for it.
>
> Hmm, interesting, actually it seems like the whole existence
> of READING_FIRMWARE_PREALLOC_BUFFER is a mistake, the IMA
> framework really does not care if we are loading the firmware
> into memory allocated by the firmware-loader code, or into
> memory allocated by the device-driver requesting the firmware.
>
> As such the current IMA code (from v4.17-rc2) actually does
> not handle READING_FIRMWARE_PREALLOC_BUFFER at all,
Right, it doesn't yet address READING_FIRMWARE_PREALLOC_BUFFER, but
should.
Depending on whether the device requesting the firmware has access to
the DMA memory, before the signature verification, will determine how
IMA-appraisal addresses READING_FIRMWARE_PREALLOC_BUFFER.
Mimi
> here
> are bits of code from: security/integrity/ima/ima_main.c:
>
> static int read_idmap[READING_MAX_ID] = {
> [READING_FIRMWARE] = FIRMWARE_CHECK,
> [READING_MODULE] = MODULE_CHECK,
> [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
> [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
> [READING_POLICY] = POLICY_CHECK
> };
>
> int ima_post_read_file(struct file *file, void *buf, loff_t size,
> ...
> if (!file && read_id == READING_FIRMWARE) {
> if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
> (ima_appraise & IMA_APPRAISE_ENFORCE))
> return -EACCES; /* INTEGRITY_UNKNOWN */
> return 0;
> }
>
> Which show that the IMA code is not handling
> READING_FIRMWARE_PREALLOC_BUFFER as it should (I believe it
> should handle it the same as READING_FIRMWARE).
>
> Now we could fix that, but the only user of
> READING_FIRMWARE_PREALLOC_BUFFER is the code which originally
> introduced it:
>
> https://patchwork.kernel.org/patch/9162011/
>
> So I believe it might be better to instead replace it
> with just READING_FIRMWARE and find another way to tell
> kernel_read_file() that there is a pre-allocated buffer,
> perhaps the easiest way there is that *buf must be
> NULL when the caller wants kernel_read_file() to
> vmalloc the mem. This would of course require auditing
> all callers that the buf which the pass in is initialized
> to NULL.
>
> Either way adding a third READING_FIRMWARE_FOO to the
> kernel_read_file_id enum seems like a bad idea, from
> the IMA pov firmware is firmware.
>
> What this whole exercise has shown me though is that
> I need to call security_kernel_post_read_file() when
> loading EFI embedded firmware. I will add a call to
> security_kernel_post_read_file() for v4 of the patch-set.
>
> > Please Cc Kees in future patches.
>
> Will do.
>
> Regards,
>
> Hans
>
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: An actual suggestion (Re: [GIT PULL] Kernel lockdown for secure boot)
On Thu, 2018-04-05 at 10:16 +0800, joeyli wrote: > Hi David, > > On Wed, Apr 04, 2018 at 05:17:24PM +0100, David Howells wrote: > > Andy Lutomirski wrote: > > > > > Since this thread has devolved horribly, I'm going to propose a solution. > > > > > > 1. Split the "lockdown" state into three levels: (please don't > > > bikeshed about the names right now.) > > > > > > LOCKDOWN_NONE: normal behavior > > > > > > LOCKDOWN_PROTECT_INTEGREITY: kernel tries to keep root from writing to > > > kernel memory > > > > > > LOCKDOWN_PROTECT_INTEGRITY_AND_SECRECY: kernel tries to keep root from > > > reading or writing kernel memory. > > > > In theory, it's good idea, but in practice it's not as easy to implement as > > I > > think you think. > > > > Let me list here the things that currently get restricted by lockdown: > > > [...snip] > > (5) Kexec. > > > > About IMA with kernel module signing and kexec(not on x86_64 yet)... Only carrying the measurement list across kexec is architecture specific, but everything else should work. > Because IMA can be used to verify the integrity of kernel module or even > the image for kexec. I think that the > IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY must be enabled at runtime > when kernel is locked-down. I think we need to understand the problem a bit better. Is the problem that you're using the secondary keyring and loading the UEFI keys onto the secondary keyring? > Because the root can enroll master key to keyring then IMA trusts the ima key > derived from master key. It causes that the arbitrary signed module can be > loaded > when the root compromised. With only the builtin keyring, only keys signed by a builtin key can be added to the IMA keyring. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/9] KEYS: Blacklisting & UEFI database load
On Sun, 2018-03-11 at 11:20 +0800, joeyli wrote: > On Wed, Mar 07, 2018 at 07:28:37AM -0800, James Bottomley wrote: > > On Wed, 2018-03-07 at 08:18 -0500, Mimi Zohar wrote: > > > On Tue, 2018-03-06 at 15:05 +0100, Jiri Slaby wrote: > > > > what's the status of this please? Distributors (I checked SUSE, > > > > RedHat and Ubuntu) have to carry these patches and every of them > > > > have to forward-port the patches to new kernels. So are you going > > > > to resend the PR to have this merged? > > [...] > > > Just because I trust the platform keys prior to booting the kernel, > > > doesn't mean that I *want* to trust those keys once booted. There > > > are, however, places where we need access to those keys to verify a > > > signature (eg. kexec kernel image). > > > > Which is essentially the reason I always give when these patches come > > back > > > > Josh Boyer's "MODSIGN: Allow the "db" UEFI variable to be suppressed" > patch checks MokIgnoreDB variable to ignore db: > > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-uefi&id=7c395b30a33a617c5cc2cdd419300af71277b79a > > I think that we can consider to use MokAllowDB. Which means that kernel > ignores DB by default. Not all systems have a shim layer. This design is really x86 specific. Allowing shim keys, but ignoring DB, does not address those systems. > > > Nayna Jain's "certs: define a trusted platform keyring" patch set > > > introduces a new, separate keyring for these platform keys. > > > > Perhaps, to break the deadlock, we should ask Jiří what the reason is > > the distros want these keys to be trusted. Apart from the Microsoft > > key, it will also give you an OEM key in your trusted keyring. Is it > > something to do with OEM supplied modules? > > > > As I remember that some manufacturers uses certificate in db to > sign their kernel module. We need to discuss with them for switching > to mok. Currently I do not know all use cases for using db. > > There have some benefits for using db: > > - User does not need to deal with shim-mokmanager to enroll mok. >Target machine doesn't need to reboot and user doesn't need to >face to mokmanager UI. The reason for trusting enrolled shim keys is because it requires physical presence. (I kind of remember hearing that this changed. There is some method of accepting enrolled keys that does not require physical presence.) > - The db is a authenticated variable, it's still secure when secure >boot is disabled. >The db is a authenticated variable that it can only be modified >by manufacturer's key. Kernel can trust it when secure boot >is disabled. It's useful for we do not need to taint kernel >for loading a manufacturer's kernel module even secure boot is >disabled. > > - Do not need to worry about the space of NVRAM and the EFI firmware >implementation for writing a boot time variable. > > But I also agree that we should not trust all keys (like Microsoft key) > in db by default. Between requiring a shim layer and relying on physical presence, I'm not convinced this is the best solution. Do we really want to support different methods for different architectures? Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/9] KEYS: Blacklisting & UEFI database load
On Tue, 2018-03-06 at 15:05 +0100, Jiri Slaby wrote: > On 11/16/2016, 07:10 PM, David Howells wrote: > > Here are two sets of patches. Firstly, the first three patches provide a > > blacklist, making the following changes: > ... > > Secondly, the remaining patches allow the UEFI database to be used to load > > the system keyrings: > ... > > Dave Howells (2): > > efi: Add EFI signature data types > > efi: Add an EFI signature blob parser > > > > David Howells (5): > > KEYS: Add a system blacklist keyring > > X.509: Allow X.509 certs to be blacklisted > > PKCS#7: Handle blacklisted certificates > > KEYS: Allow unrestricted boot-time addition of keys to secondary > > keyring > > efi: Add SHIM and image security database GUID definitions > > > > Josh Boyer (2): > > MODSIGN: Import certificates from UEFI Secure Boot > > MODSIGN: Allow the "db" UEFI variable to be suppressed > > Hi, > > what's the status of this please? Distributors (I checked SUSE, RedHat > and Ubuntu) have to carry these patches and every of them have to > forward-port the patches to new kernels. So are you going to resend the > PR to have this merged? With secure boot enabled, we establish a signature chain of trust, rooted in HW, up to the kernel and then transition from those keys to a new set of keys builtin the kernel and loaded onto the builtin_trusted_keys (builtin). Enabling the secondary_builtin_keys (secondary) allows keys signed by a key on the builtin keyring to be added to the secondary keyring. Any key, signed by a key on either the builtin or secondary keyring, can be added to the IMA trusted keyring. The "KEYS: Allow unrestricted boot-time addition of keys to secondary keyring" patch loads the platform keys directly onto the secondary keyring, without requiring them to be signed by a key on the builtin or secondary keyring. With this change, any key signed by a platfrom key on the secondary, can be loaded onto the .ima trusted keyring. Just because I trust the platform keys prior to booting the kernel, doesn't mean that I *want* to trust those keys once booted. There are, however, places where we need access to those keys to verify a signature (eg. kexec kernel image). Nayna Jain's "certs: define a trusted platform keyring" patch set introduces a new, separate keyring for these platform keys. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown
On Wed, 2017-11-15 at 21:46 +0100, Luis R. Rodriguez wrote:
> On Wed, Nov 15, 2017 at 02:56:57PM -0500, Mimi Zohar wrote:
> > On Wed, 2017-11-15 at 18:52 +0100, Luis R. Rodriguez wrote:
> > > On Wed, Nov 15, 2017 at 06:49:57AM -0500, Mimi Zohar wrote:
> > > > On Tue, 2017-11-14 at 21:50 +0100, Luis R. Rodriguez wrote:
> > > >
> > > > > Johannes made cfg80211 recently just use request_firmware() now via
> > > > > commit on
> > > > > linux-next 90a53e4432 ("cfg80211: implement regdb signature
> > > > > checking") [0] as
> > > > > he got tired of waiting firmware signing, but note he implemented a
> > > > > signature
> > > > > checking on its own so he open codes verify_pkcs7_signature() after
> > > > > the
> > > > > request_firmware() call. If we are happy to live with this, then so
> > > > > be it.
> > > > >
> > > > > [0]
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=90a53e4432b12288316efaa5f308adafb8d304b0
> > > >
> > > > Johannes was tired of waiting? Commit 5a9196d "ima: add support for
> > > > measuring and appraising firmware" has been in the kernel since linux-
> > > > 3.17.
> > > >
> > > > The original firmware hook for verifying firmware signatures were
> > > > replaced with the common LSM pre and post kernel_read_file() hooks
> > > > in linux-4.6.y.
> > > >
> > > > Even if you wanted to support firmware signature verification without
> > > > IMA-appraisal, it should be using the LSM hooks.
> > >
> > > request_firmware() uses kernel_read_file_from_path() underneath the hood,
> > > and so its used for both:
> > >
> > > /lib/firmware/regulatory.db
> > > /lib/firmware/regulatory.db.p7s
> >
> > The firmware signature validation should occur as part of
> > kernel_read_file_from_path(), not as a stand alone verification.
> >
> > Why not extend kernel_read_file_from_path() to pass the detached signature?
> > Since the signature would only be used for the verification, there's no need
> > to return the open file descriptor.
>
> This goes along with the question if there were an other users who wanted it,
> or more importantly -- if firmware signing was desirable for any reason, a
> modified kernel_read_file_from_path_signed() could in turn be used, *or* an
> LSM
> added to handle READING_FIRMWARE and READING_FIRMWARE_PREALLOC_BUFFER. The
> above use case was one example outside of the typical firmware use. I've long
> pointed out that we no longer use the firmware API for just firmware, and the
> above is now a very good example of it. I've been suggesting uses of the
> firmware API for non-firmware had already happened and that more uses were on
> its way. Trusted boot has nothing to do with these uses as such the gains of
> systems pegged with "trusted boot" have nothing to do validation of these
> files
> through hardware.
No, it has nothing to do with other users wanting it. It has to do
with extending an API to support detach signatures.
There's no reason to define a new function named
kernel_read_file_from_path_signed(). To prevent code duplication, the
existing functions would turn into wrappers. It's not like there are
that many users. A quick search returned:
kernel_read_file_from_fd: 2
kernel_read_file_from_path: 5
LSMs: 3 loadpin, selinux, + ima
Mimi
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown
On Wed, 2017-11-15 at 18:52 +0100, Luis R. Rodriguez wrote:
> On Wed, Nov 15, 2017 at 06:49:57AM -0500, Mimi Zohar wrote:
> > On Tue, 2017-11-14 at 21:50 +0100, Luis R. Rodriguez wrote:
> >
> > > Johannes made cfg80211 recently just use request_firmware() now via
> > > commit on
> > > linux-next 90a53e4432 ("cfg80211: implement regdb signature checking")
> > > [0] as
> > > he got tired of waiting firmware signing, but note he implemented a
> > > signature
> > > checking on its own so he open codes verify_pkcs7_signature() after the
> > > request_firmware() call. If we are happy to live with this, then so be it.
> > >
> > > [0]
> > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=90a53e4432b12288316efaa5f308adafb8d304b0
> >
> > Johannes was tired of waiting? Commit 5a9196d "ima: add support for
> > measuring and appraising firmware" has been in the kernel since linux-
> > 3.17.
> >
> > The original firmware hook for verifying firmware signatures were
> > replaced with the common LSM pre and post kernel_read_file() hooks
> > in linux-4.6.y.
> >
> > Even if you wanted to support firmware signature verification without
> > IMA-appraisal, it should be using the LSM hooks.
>
> request_firmware() uses kernel_read_file_from_path() underneath the hood,
> and so its used for both:
>
> /lib/firmware/regulatory.db
> /lib/firmware/regulatory.db.p7s
The firmware signature validation should occur as part of
kernel_read_file_from_path(), not as a stand alone verification.
Why not extend kernel_read_file_from_path() to pass the detached signature?
Since the signature would only be used for the verification, there's no need to
return the open file descriptor.
Or if you prefer, call kernel_read_file_from_path() for the detached signature
first, and then call it again for the firmware with a pointer to the detached
signature.
>
> The later only if CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, which defaults
> to y anyway.
>
> What I meant was that net/wireless/reg.c now open codes firmware signature
> validation on its own rather than using a helper. IMA appraisal will still
> be used if enabled given kernel_read_file_from_path() is used.
>
> The open coding of the firmware signature check is what I wanted to highlight.
How are the keys in the CFG80211_EXTRA_REGDB_KEYDIR verified? The
call to key_create_or_update() with the KEY_ALLOC_BYPASS_RESTRICTION
option by-passes any requirement that the keys in this directory are
signed. This by-passes the concept of extending the secure boot
signature chain of trust. To safely validate the keys use the
restrict_link_by_builtin_trusted option.
Mimi
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown
On Tue, 2017-11-14 at 21:50 +0100, Luis R. Rodriguez wrote:
> Johannes made cfg80211 recently just use request_firmware() now via commit on
> linux-next 90a53e4432 ("cfg80211: implement regdb signature checking") [0] as
> he got tired of waiting firmware signing, but note he implemented a signature
> checking on its own so he open codes verify_pkcs7_signature() after the
> request_firmware() call. If we are happy to live with this, then so be it.
>
> [0]
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=90a53e4432b12288316efaa5f308adafb8d304b0
Johannes was tired of waiting? Commit 5a9196d "ima: add support for
measuring and appraising firmware" has been in the kernel since linux-
3.17.
The original firmware hook for verifying firmware signatures were
replaced with the common LSM pre and post kernel_read_file() hooks
in linux-4.6.y.
Even if you wanted to support firmware signature verification without
IMA-appraisal, it should be using the LSM hooks.
Mimi
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown
On Tue, 2017-11-14 at 13:38 +0100, Greg Kroah-Hartman wrote: > On Tue, Nov 14, 2017 at 07:21:38AM -0500, Mimi Zohar wrote: > > On Mon, 2017-11-13 at 14:09 -0800, Linus Torvalds wrote: > > > On Mon, Nov 13, 2017 at 1:44 PM, David Howells > > > wrote: > > > > > > > > Whilst that may be true, we either have to check signatures on every > > > > bit of > > > > firmware that the appropriate driver doesn't say is meant to be signed > > > > or not > > > > bother. > > > > > > I vote for "not bother". > > > > > > Seriously, if you have firmware in /lib/firmware, and you don't trust > > > it, what the hell are you doing? > > > > I might "trust" the files in /lib/firmware, but I also want to make > > sure that they haven't changed. File signatures provide file > > provenance and integrity guarantees. > > Then "verify" them with signatures that you generate yourself. Like > dm-verify does for the partition that you put the firmware on. The discussion, here, is in the context of the "lockdown" patch set, without IMA-appraisal configured. Kernel modules and the kexec kernel image require file signatures in lockdown mode. An equivalent method of requiring file signatures for firmware (without IMA-appraisal) does not exist. I posted the patch [RFC PATCH v2] "fw_lockdown: new micro LSM module to prevent loading unsigned firmware". The patch and discussion can be found here - (https://lkml.org/lkml/2017/11/13/217). Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown
On Mon, 2017-11-13 at 14:09 -0800, Linus Torvalds wrote: > On Mon, Nov 13, 2017 at 1:44 PM, David Howells wrote: > > > > Whilst that may be true, we either have to check signatures on every bit of > > firmware that the appropriate driver doesn't say is meant to be signed or > > not > > bother. > > I vote for "not bother". > > Seriously, if you have firmware in /lib/firmware, and you don't trust > it, what the hell are you doing? I might "trust" the files in /lib/firmware, but I also want to make sure that they haven't changed. File signatures provide file provenance and integrity guarantees. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown
On Sat, 2017-11-11 at 02:32 +, Alan Cox wrote: > > My assumption here is: > > 1) there are some less important and so security-insensitive firmwares, > >by which I mean that such firmwares won't be expected to be signed in > >terms of vulnerability or integrity. > >(I can't give you examples though.) > > 2) firmware's signature will be presented separately from the firmware > >blob itself. Say, "firmware.bin.p7s" for "firmware.bin" > > For x86 at least any firmware on any system modern enough to support > 'secure' boot should already be signed. The only major exception is > likely to be for things like random USB widgets. Does this mean that the firmware signature is contained within the firmware? On Linux, is anything verifying this signature? Or is this the hw verifying the firmware's signature? > Even things like input controller firmware loaded over i2c or spi > is usually signed because you could do fun things with input faking > otherwise. > > The other usual exception is FPGAs, but since the point of an FPGA is > usually the fact it *can* be reprogrammed it's not clear that signing > FPGA firmware makes sense unless it is designed to be fixed function. > > You can't subvert the bus protocols on the x86 FPGA I am aware of as > those bits are signed (or hard IP), but without IOMMU I am not sure FPGA > and 'secure' boot is completely compatible. Ok, but why differentiate between "fixed" and research/development FPGA functions? In both cases, the FPGA firmware would need to be signed locally. Wouldn't this be similar to kernel developers building their own kernels and signing the kernel modules? Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown
On Fri, 2017-11-10 at 02:46 +0100, Luis R. Rodriguez wrote:
> On Thu, Nov 09, 2017 at 10:48:43AM +0900, AKASHI, Takahiro wrote:
> > On Wed, Nov 08, 2017 at 08:46:26PM +0100, Luis R. Rodriguez wrote:
> > > But perhaps I'm not understanding the issue well, let me know.
> >
> > My point is quite simple:
> > my_deviceA_init() {
> > err = request_firmware(&fw, "deviceA"); <--- (a)
> > if (err)
> > goto err_request;
> >
> > err = verify_firmware(fw); <--- (b)
> > if (err)
> > goto err_verify;
> >
> > load_fw_to_deviceA(fw); <--- (c)
> > ...
> > }
> >
> > As legacy device drivers does not have (b), there is no chance to
> > prevent loading a firmware at (c) for locked-down kernel.
>
> Ah, I think your example requires another piece of code to make it clearer.
> Here is an example legacy driver:
>
> my_legacy_deviceB_init() {
> err = request_firmware(&fw, "deviceB"); <--- (a)
> if (err)
> goto err_request;
>
> load_fw_to_deviceA(fw); <--- (c)
> ...
> }
>
> There is no verify_firmware() call here, and as such the approach Linus
> suggested a while ago cannot possibly fail on a "locked down kernel", unless
> *very* legacy API call gets a verify_firmware() sprinkled.
>
> One sensible thing to say here is then that all request_firmware() calls
> should
> just fail on a "locked down kernel", however if this were true then even calls
> which *did* issue a subsequent verify_firmware() would fail earlier therefore
> making verify_firmware() pointless on new drivers.
As long as these "*very* legacy API calls", are calling
kernel_read_file_from_path() to read the firmware, there shouldn't be
a problem.
Mimi
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown
On Thu, 2017-11-09 at 13:46 +0900, AKASHI, Takahiro wrote:
> Mimi,
>
> On Wed, Nov 08, 2017 at 09:17:37PM -0500, Mimi Zohar wrote:
> > > > IMHO that should just fail then, ie, a "locked down" kernel should not
> > > > want to
> > > > *pass* a firmware signature if such thing could not be done.
> > > >
> > > > Its no different than trying to verify a signed module on a "locked
> > > > down" for
> > > > which it has no signature.
> > > >
> > > > But perhaps I'm not understanding the issue well, let me know.
> > >
> > > My point is quite simple:
> > > my_deviceA_init() {
> > > err = request_firmware(&fw, "deviceA"); <--- (a)
> > > if (err)
> > > goto err_request;
> > >
> > > err = verify_firmware(fw); <--- (b)
> > > if (err)
> > > goto err_verify;
> > >
> > > load_fw_to_deviceA(fw); <--- (c)
> > > ...
> > > }
> > >
> > > As legacy device drivers does not have (b), there is no chance to
> > > prevent loading a firmware at (c) for locked-down kernel.
> > >
> > > If you allow me to bring in yet another function, say
> > > request_firmware_signable(), which should be used in place of (a)
> > > for all verification-aware drivers, that would be fine.
> >
> > I really don't understand why you need a new function. The
> > request_firmware() eventually calls kernel_read_file_from_path(),
> > which already calls the pre and post LSM hooks.
>
> My assumption here is:
> 1) there are some less important and so security-insensitive firmwares,
>by which I mean that such firmwares won't be expected to be signed in
>terms of vulnerability or integrity.
>(I can't give you examples though.)
Differentiating between firmware can be achieved based on LSM labels,
or other file metadata. Whether this is acceptable for "lockdown"
mode is a separate issue.
> 2) firmware's signature will be presented separately from the firmware
>blob itself. Say, "firmware.bin.p7s" for "firmware.bin"
Currently there are appended signatures and signatures stored as
xattrs. (Thiago Bauermann posted patches adding appending signature
support within IMA.) Adding support for detached signatures could be
added as well, but it should be defined within the existing integrity
framework.
Mimi
> I don't think that the current security_kernel(_post)_read_file() scheme
> fit with this assumption very well.
>
> Thanks,
> -Takahiro AKASHI
>
>
> > IMA-appraisal is already on these hooks verifying the requested
> > firmware's signature. For systems with "lockdown" enabled, but
> > without IMA-appraisal enabled, define a small, builtin LSM that sits
> > on these LSM hooks and denies the unsigned firmware requests.
> >
> > Mimi
> >
> > > In this case, all the invocation of request_firmware() in legacy code
> > > could be forced to fail in locked-down kernel.
> > >
> > > But I think that "signable" should be allowed to be combined with other
> > > features of request_firmware variants like _(no)wait or _direct.
> > >
> > > -Takahiro AKASHI
> >
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-security-module" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown
> > IMHO that should just fail then, ie, a "locked down" kernel should not want
> > to
> > *pass* a firmware signature if such thing could not be done.
> >
> > Its no different than trying to verify a signed module on a "locked down"
> > for
> > which it has no signature.
> >
> > But perhaps I'm not understanding the issue well, let me know.
>
> My point is quite simple:
> my_deviceA_init() {
> err = request_firmware(&fw, "deviceA"); <--- (a)
> if (err)
> goto err_request;
>
> err = verify_firmware(fw); <--- (b)
> if (err)
> goto err_verify;
>
> load_fw_to_deviceA(fw); <--- (c)
> ...
> }
>
> As legacy device drivers does not have (b), there is no chance to
> prevent loading a firmware at (c) for locked-down kernel.
>
> If you allow me to bring in yet another function, say
> request_firmware_signable(), which should be used in place of (a)
> for all verification-aware drivers, that would be fine.
I really don't understand why you need a new function. The
request_firmware() eventually calls kernel_read_file_from_path(),
which already calls the pre and post LSM hooks.
IMA-appraisal is already on these hooks verifying the requested
firmware's signature. For systems with "lockdown" enabled, but
without IMA-appraisal enabled, define a small, builtin LSM that sits
on these LSM hooks and denies the unsigned firmware requests.
Mimi
> In this case, all the invocation of request_firmware() in legacy code
> could be forced to fail in locked-down kernel.
>
> But I think that "signable" should be allowed to be combined with other
> features of request_firmware variants like _(no)wait or _direct.
>
> -Takahiro AKASHI
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown
> > Or reflect that IMA-appraisal, if enabled, will enforce firmware being > > validly signed. > > But FWICT lockdown is a built-in kernel thingy, unless lockdown implies IMA > it would not be the place to refer to it. > > It seems the documentation was proposed to help users if an error was caught. > That error should cover only what is being addressed in code on the kernel. Enabling "lockdown" needs to take into account IMA-appraisal to prevent breaking systems with it enabled. An IMA builtin "secure_boot" policy was already upstreamed (commit 503ceaef8e2e "ima: define a set of appraisal rules requiring file signatures"). An additional patch, automatically enables the "secure_boot" policy in "lockdown" mode. Refer to this discussion and patch: http://kernsec.org/pipermail/linux-security-module-archive/2017-October/003913.html http://kernsec.org/pipermail/linux-security-module-archive/2017-October/003910.html thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/27] Enforce module signatures if the kernel is locked down
On Thu, 2017-11-02 at 22:01 +, David Howells wrote:
> Mimi Zohar wrote:
>
> > Right, it would never get here if the IMA signature verification
> > fails. If sig_enforce is not enabled, then it will also work. So the
> > only case is if sig_enforced is enabled and there is no key.
> >
> > eg.
> > else if (can_do_ima_check && is_ima_appraise_enabled())
> > err = 0;
>
> I'm not sure where you want to put that, but I can't just do this:
>
> /* Not having a signature is only an error if we're strict. */
> if (err == -ENOKEY && !sig_enforce &&
> (!can_do_ima_check || !is_ima_appraise_enabled()) &&
The above IMA checks aren't needed here.
> !kernel_is_locked_down("Loading of unsigned modules"))
> err = 0;
> else if (can_do_ima_check && is_ima_appraise_enabled())
> err = 0;
>
> because that'll print out a message in lockdown mode saying that you're not
> allowed to do that and then maybe do it anyway.
Then at least for now, document that even though kernel modules might
be signed and verified by IMA-appraisal, that in lockdown mode they
also require an appended signature.
Mimi
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown
On Thu, 2017-11-02 at 22:04 +, David Howells wrote: > Mimi Zohar wrote: > > > > Only validly signed device firmware may be loaded. > > > > fw_get_filesystem_firmware() calls kernel_read_file_from_path() to > > read the firmware, which calls into the security hooks. Is there > > another place that validates the firmware signatures. I'm not seeing > > which patch requires firmware to be signed? > > Luis has a set of patches for this. However, I'm not sure if that's going > anywhere at the moment. Possibly I should remove this from the manpage for > the moment. Or reflect that IMA-appraisal, if enabled, will enforce firmware being validly signed. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/27] security, efi: Add kernel lockdown
Hi David, >From the man page: > Only validly signed modules may be loaded. > .P > Only validly signed binaries may be kexec'd. > .P > Only validly signed device firmware may be loaded. fw_get_filesystem_firmware() calls kernel_read_file_from_path() to read the firmware, which calls into the security hooks. Is there another place that validates the firmware signatures. I'm not seeing which patch requires firmware to be signed? Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/27] Enforce module signatures if the kernel is locked down
On Thu, 2017-11-02 at 21:30 +, David Howells wrote: > Mimi Zohar wrote: > > > By this point, IMA-appraisal has already verified the kernel module > > signature back in kernel_read_file_from_fd(), if it was required. > > Having a key with which to verify the appended signature or requiring > > an appended signature, should not be required as well. > > I guess I don't need to put in any support for IMA here, then, and you've > taken care of it in your patchset such that it won't actually go into > module_sig_check() in that case (or will at least return immediately). Right, it would never get here if the IMA signature verification fails. If sig_enforce is not enabled, then it will also work. So the only case is if sig_enforced is enabled and there is no key. eg. else if (can_do_ima_check && is_ima_appraise_enabled()) err = 0; Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/27] Enforce module signatures if the kernel is locked down
On Thu, 2017-11-02 at 17:22 +, David Howells wrote:
> #ifdef CONFIG_MODULE_SIG
> -static int module_sig_check(struct load_info *info, int flags)
> +static int module_sig_check(struct load_info *info, int flags,
> + bool can_do_ima_check)
> {
> int err = -ENOKEY;
> const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
> @@ -2781,13 +2783,16 @@ static int module_sig_check(struct load_info *info,
> int flags)
> }
>
> /* Not having a signature is only an error if we're strict. */
> - if (err == -ENOKEY && !sig_enforce)
> + if (err == -ENOKEY && !sig_enforce &&
> + (!can_do_ima_check || !is_ima_appraise_enabled()) &&
> + !kernel_is_locked_down("Loading of unsigned modules"))
By this point, IMA-appraisal has already verified the kernel module
signature back in kernel_read_file_from_fd(), if it was required.
Having a key with which to verify the appended signature or requiring
an appended signature, should not be required as well.
Mimi
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/27] Enforce module signatures if the kernel is locked down
[Corrected Matthew Garrett's email address. Cc'ed Bruno Meneguele] On Mon, 2017-10-30 at 17:00 +, David Howells wrote: > Mimi Zohar wrote: > > > This kernel_is_locked_down() check is being called for both the > > original and new module_load syscalls. We need to be able > > differentiate them. This is fine for the original syscall, but for > > the new syscall we would need an additional IMA check - > > !is_ima_appraise_enabled(). > > IMA can only be used with finit_module()? Yes, without the file descriptor, IMA-appraisal can't access the xattrs. You should really look at Bruno's patches, which are in my next branch: 8168913c50d5 "ima: check signature enforcement against cmdline param instead of CONFIG" 404090509894 module: export module signature enforcement status Can we get an Ack on the module one? Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set
On Mon, 2017-10-30 at 15:49 +, David Howells wrote: > Mimi Zohar wrote: > > > Huh?! With the "secure_boot" policy enabled on the boot command line, > > IMA-appraisal would verify the kexec kernel image, firmware, kernel > > modules, and custom IMA policy signatures. > > What happens if the "secure_boot" policy isn't enabled on the boot command > line? Can you sum up both cases in a paragraph I can add to the patch > description? The other patch automatically enables "secure_boot" for lockdown mode. So there is no need to specify "secure_boot" on the boot command line. Reordering the patches so that the other patch comes before any call to is_ima_appraise_enabled() will simplify this patch description. > > Other patches in this patch series need to be updated as well to check > > if IMA-appraisal is enabled. > > Which exactly? I've added your "!is_ima_appraise_enabled() &&" line to > kexec_file() and module_sig_check(). Anything else? load_module(), which calls module_sig_check(), is called by both the old and new kernel module syscalls. IMA is only on the new syscall. Did you differentiate between the kernel module syscalls? There doesn't seem to be any other patches affected. That said, the IMA "secure_boot" policy is more stringent than what you have without it. For example, with the "secure_boot" policy enabled, firwmware needs to be signed as well. At some point, we'll want to also require the initramfs be signed as well. Both methods work independently of each other, but there needs to be better coordination for when both methods are enabled at the same time (eg. are both signatures required?). For testing purposes, you can use the same certs/signing_key to sign the kexec image, kernel modules and firmware, by loading the signing_key on the .ima keyring. Using evmctl, sign the files (eg. evmctl ima_sign -a sha256 -k certs/signing_key.pem --imasig /boot/). Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set
On Mon, 2017-10-30 at 09:00 +, David Howells wrote: > Mimi Zohar wrote: > > > Yes, that works. Thanks! Remember is_ima_appraise_enabled() is > > dependent on the "ima: require secure_boot rules in lockdown mode" > > patch - http://kernsec.org/pipermail/linux-security-module-archive/201 > > 7-October/003910.html. > > What happens if the file in question is being accessed from a filesystem that > doesn't have xattrs and doesn't provide support for appraisal? Is it rejected > outright or just permitted? IMA-appraisal returns -EACCES for any error, including lack of xattr support. Thiago Bauermann posted the "Appended signatures support for IMA appraisal" patch set. This patch set allows the current kernel module appended signature format to be used for verifying the kernel image. Once that patch set is upstreamed, we'll be able to update the IMA "secure_boot" policy to permit appended signatures. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set
On Sat, 2017-10-28 at 16:34 +0800, joeyli wrote: > On Fri, Oct 27, 2017 at 03:32:26PM -0400, Mimi Zohar wrote: > > On Thu, 2017-10-26 at 10:17 -0400, Mimi Zohar wrote: > > > On Thu, 2017-10-26 at 15:42 +0800, joeyli wrote: > > > > Hi Mimi, > > > > > > > > Thank you for reviewing. > > > > > > > > On Mon, Oct 23, 2017 at 11:54:43AM -0400, Mimi Zohar wrote: > > > > > On Thu, 2017-10-19 at 15:51 +0100, David Howells wrote: > > > > > > From: Chun-Yi Lee > > > > > > > > > > > > When KEXEC_VERIFY_SIG is not enabled, kernel should not loads image > > > > > > through kexec_file systemcall if securelevel has been set. > > > > > > > > > > The patch title and description needs to be updated to refer to > > > > > lockdown, not securelevel. > > > > > > > > > > As previously mentioned the last time these patches were posted, this > > > > > leaves out testing to see if the integrity subsystem is enabled. > > > > > > > > > > Commit 503ceaef8e2e "ima: define a set of appraisal rules requiring > > > > > file signatures" was upstreamed. An additional patch could force > > > > > these rules to be added to the custom policy, if lockdown is enabled. > > > > > This and other patches in this series could then check to see if > > > > > is_ima_appraise_enabled() is true. > > > > > > > > > > Mimi > > > > > > > > > > > > > I have updated the patch title and description, and I also added > > > > is_ima_appraise_enabled() as the following. Is it good to you? > > > > > > Yes, that works. Thanks! Remember is_ima_appraise_enabled() is > > > dependent on the "ima: require secure_boot rules in lockdown mode" > > > patch - http://kernsec.org/pipermail/linux-security-module-archive/201 > > > 7-October/003910.html. > > > > > > The IMA "secure_boot" policy can be specified on the boot command line > > > as ima_policy="secure_boot". It requires kernel modules, firmware, > > > kexec kernel image and the IMA custom policy to be signed. In > > > lockdown mode, these rules are enabled by default and added to the > > > custom policy. > > > > > > > On the other hand, I am not good on IMA. I have traced the code path > > > > in kimage_file_prepare_segments(). Looks that the READING_KEXEC_IMAGE > > > > doesn't show in selinux_kernel_read_file(). Where is the exact code > > > > in IMA for checking the signature when loading crash kernel file? > > > > > > kernel_read_file_from_fd() calls the security_kernel_read_file() and > > > security_kernel_post_read_file() hooks, which call ima_read_file() and > > > ima_post_read_file() respectively. > > > > Hm, with "lockdown" enabled on the boot command line, I'm now able to > > do the kexec load, but not the unload. :/ After the kexec load with > > I have tried on Qemu with OVMF, I can load and unload second kernel by > kexec tool (on openSUSE is in kexec-tools RPM): > > # kexec -u -s Thanks, I left off the "-s" option, causing it to fail. This is the correct behavior. So both with/without the "-s" option are working properly. > I add -s for using kexec-load-file, and I signed kernel by pesign. > > > the "--reuse-cmdline" option, the system reboots, but isn't in > > "lockdown" mode. > > > > Either enabling secure boot in EFI firmware or using _lockdown_ kernel > parameter, the second kernel can be locked down on my OVMF VM. > > I used following commands: > > # kexec -s -l /boot/vmlinuz-4.14.0-rc2-default+ --append="$(cat > /proc/cmdline)" --initrd=/boot/initrd-4.14.0-rc2-default+ > # umount -a; mount -o remount,ro /I' > # kexec -e > > The kernel source is from David's linux-fs git with lockdown-20171026 tag. > The kernel is also signed by pesign. Yes, based on the patches in David's tree, "lockdown" is being carried to the target OS properly. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set
On Thu, 2017-10-26 at 10:17 -0400, Mimi Zohar wrote: > On Thu, 2017-10-26 at 15:42 +0800, joeyli wrote: > > Hi Mimi, > > > > Thank you for reviewing. > > > > On Mon, Oct 23, 2017 at 11:54:43AM -0400, Mimi Zohar wrote: > > > On Thu, 2017-10-19 at 15:51 +0100, David Howells wrote: > > > > From: Chun-Yi Lee > > > > > > > > When KEXEC_VERIFY_SIG is not enabled, kernel should not loads image > > > > through kexec_file systemcall if securelevel has been set. > > > > > > The patch title and description needs to be updated to refer to > > > lockdown, not securelevel. > > > > > > As previously mentioned the last time these patches were posted, this > > > leaves out testing to see if the integrity subsystem is enabled. > > > > > > Commit 503ceaef8e2e "ima: define a set of appraisal rules requiring > > > file signatures" was upstreamed. An additional patch could force > > > these rules to be added to the custom policy, if lockdown is enabled. > > > This and other patches in this series could then check to see if > > > is_ima_appraise_enabled() is true. > > > > > > Mimi > > > > > > > I have updated the patch title and description, and I also added > > is_ima_appraise_enabled() as the following. Is it good to you? > > Yes, that works. Thanks! Remember is_ima_appraise_enabled() is > dependent on the "ima: require secure_boot rules in lockdown mode" > patch - http://kernsec.org/pipermail/linux-security-module-archive/201 > 7-October/003910.html. > > The IMA "secure_boot" policy can be specified on the boot command line > as ima_policy="secure_boot". It requires kernel modules, firmware, > kexec kernel image and the IMA custom policy to be signed. In > lockdown mode, these rules are enabled by default and added to the > custom policy. > > > On the other hand, I am not good on IMA. I have traced the code path > > in kimage_file_prepare_segments(). Looks that the READING_KEXEC_IMAGE > > doesn't show in selinux_kernel_read_file(). Where is the exact code > > in IMA for checking the signature when loading crash kernel file? > > kernel_read_file_from_fd() calls the security_kernel_read_file() and > security_kernel_post_read_file() hooks, which call ima_read_file() and > ima_post_read_file() respectively. Hm, with "lockdown" enabled on the boot command line, I'm now able to do the kexec load, but not the unload. :/ After the kexec load with the "--reuse-cmdline" option, the system reboots, but isn't in "lockdown" mode. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set
On Thu, 2017-10-26 at 10:17 -0400, Mimi Zohar wrote: > On Thu, 2017-10-26 at 15:42 +0800, joeyli wrote: > > Hi Mimi, > > > > Thank you for reviewing. > > > > On Mon, Oct 23, 2017 at 11:54:43AM -0400, Mimi Zohar wrote: > > > On Thu, 2017-10-19 at 15:51 +0100, David Howells wrote: > > > > From: Chun-Yi Lee > > > > > > > > When KEXEC_VERIFY_SIG is not enabled, kernel should not loads image > > > > through kexec_file systemcall if securelevel has been set. > > > > > > The patch title and description needs to be updated to refer to > > > lockdown, not securelevel. > > > > > > As previously mentioned the last time these patches were posted, this > > > leaves out testing to see if the integrity subsystem is enabled. > > > > > > Commit 503ceaef8e2e "ima: define a set of appraisal rules requiring > > > file signatures" was upstreamed. An additional patch could force > > > these rules to be added to the custom policy, if lockdown is enabled. > > > This and other patches in this series could then check to see if > > > is_ima_appraise_enabled() is true. > > > > > > Mimi > > > > > > > I have updated the patch title and description, and I also added > > is_ima_appraise_enabled() as the following. Is it good to you? > > Yes, that works. Thanks! Remember is_ima_appraise_enabled() is > dependent on the "ima: require secure_boot rules in lockdown mode" > patch - http://kernsec.org/pipermail/linux-security-module-archive/201 > 7-October/003910.html. > > The IMA "secure_boot" policy can be specified on the boot command line > as ima_policy="secure_boot". It requires kernel modules, firmware, > kexec kernel image and the IMA custom policy to be signed. In > lockdown mode, these rules are enabled by default and added to the > custom policy. > > > On the other hand, I am not good on IMA. I have traced the code path > > in kimage_file_prepare_segments(). Looks that the READING_KEXEC_IMAGE > > doesn't show in selinux_kernel_read_file(). Where is the exact code > > in IMA for checking the signature when loading crash kernel file? > > kernel_read_file_from_fd() calls the security_kernel_read_file() and > security_kernel_post_read_file() hooks, which call ima_read_file() and > ima_post_read_file() respectively. Hm, with "lockdown" enabled on the boot command line, I'm now able to do the kexec load, but not the unload. :/ After the kexec load with the "--reuse-cmdline" option, the system reboots, but isn't in "lockdown" mode. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/27] Enforce module signatures if the kernel is locked down
On Thu, 2017-10-19 at 15:50 +0100, David Howells wrote:
> If the kernel is locked down, require that all modules have valid
> signatures that we can verify.
>
> Signed-off-by: David Howells
> ---
>
> kernel/module.c |3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index de66ec825992..3d9a3270c179 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2781,7 +2781,8 @@ static int module_sig_check(struct load_info *info, int
> flags)
> }
>
> /* Not having a signature is only an error if we're strict. */
> - if (err == -ENOKEY && !sig_enforce)
> + if (err == -ENOKEY && !sig_enforce &&
> + !kernel_is_locked_down("Loading of unsigned modules"))
This kernel_is_locked_down() check is being called for both the
original and new module_load syscalls. We need to be able
differentiate them. This is fine for the original syscall, but for
the new syscall we would need an additional IMA check -
!is_ima_appraise_enabled().
Mimi
> err = 0;
>
> return err;
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Thu, 2017-10-26 at 17:37 +0100, David Howells wrote: > Hi James, > > Can you pull this patchset into security/next please? > > It adds kernel lockdown support for EFI secure boot. Note that it doesn't yet > cover: > > bpf - No agreement as to how > ftrace - Recently suggested, query sent to maintainer > perf- Not looked at yet. > > and there are some changes recently proposed that make it work with IMA that > I'll pass on as a follow up when we've fully worked them out. There's a major difference between leaving out support and preventing properly signed code from working properly. We're already at -rc6. I'm just not sure how there will be time to include the patches, test, and send James a subsequent pull request before the next open window? Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set
[Cc'ing Matthew Garrett]
On Thu, 2017-10-26 at 16:02 +0100, David Howells wrote:
> joeyli wrote:
>
> > + if (!IS_ENABLED(CONFIG_KEXEC_VERIFY_SIG) &&
> > + !is_ima_appraise_enabled() &&
> > + kernel_is_locked_down("kexec of unsigned images"))
>
> This doesn't seem right. It seems that you can then kexec unsigned images
> into a locked-down kernel if IMA appraise is enabled.
Huh?! With the "secure_boot" policy enabled on the boot command line,
IMA-appraisal would verify the kexec kernel image, firmware, kernel
modules, and custom IMA policy signatures. With the "ima: require
secure_boot rules in lockdown mode" patch, the "lockdown" mode would
enable IMA-appraisal's secure_boot policy, without requiring the boot
command line option. It would also add the secure_boot rules to the
custom policy, so that if the builtin policy is replaced with a custom
policy, the "secure_boot" policy would still be enforced.
Other patches in this patch series need to be updated as well to check
if IMA-appraisal is enabled.
Mimi
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set
On Thu, 2017-10-26 at 15:42 +0800, joeyli wrote: > Hi Mimi, > > Thank you for reviewing. > > On Mon, Oct 23, 2017 at 11:54:43AM -0400, Mimi Zohar wrote: > > On Thu, 2017-10-19 at 15:51 +0100, David Howells wrote: > > > From: Chun-Yi Lee > > > > > > When KEXEC_VERIFY_SIG is not enabled, kernel should not loads image > > > through kexec_file systemcall if securelevel has been set. > > > > The patch title and description needs to be updated to refer to > > lockdown, not securelevel. > > > > As previously mentioned the last time these patches were posted, this > > leaves out testing to see if the integrity subsystem is enabled. > > > > Commit 503ceaef8e2e "ima: define a set of appraisal rules requiring > > file signatures" was upstreamed. An additional patch could force > > these rules to be added to the custom policy, if lockdown is enabled. > > This and other patches in this series could then check to see if > > is_ima_appraise_enabled() is true. > > > > Mimi > > > > I have updated the patch title and description, and I also added > is_ima_appraise_enabled() as the following. Is it good to you? Yes, that works. Thanks! Remember is_ima_appraise_enabled() is dependent on the "ima: require secure_boot rules in lockdown mode" patch - http://kernsec.org/pipermail/linux-security-module-archive/201 7-October/003910.html. The IMA "secure_boot" policy can be specified on the boot command line as ima_policy="secure_boot". It requires kernel modules, firmware, kexec kernel image and the IMA custom policy to be signed. In lockdown mode, these rules are enabled by default and added to the custom policy. > On the other hand, I am not good on IMA. I have traced the code path > in kimage_file_prepare_segments(). Looks that the READING_KEXEC_IMAGE > doesn't show in selinux_kernel_read_file(). Where is the exact code > in IMA for checking the signature when loading crash kernel file? kernel_read_file_from_fd() calls the security_kernel_read_file() and security_kernel_post_read_file() hooks, which call ima_read_file() and ima_post_read_file() respectively. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set
On Thu, 2017-10-19 at 15:51 +0100, David Howells wrote:
> From: Chun-Yi Lee
>
> When KEXEC_VERIFY_SIG is not enabled, kernel should not loads image
> through kexec_file systemcall if securelevel has been set.
The patch title and description needs to be updated to refer to
lockdown, not securelevel.
As previously mentioned the last time these patches were posted, this
leaves out testing to see if the integrity subsystem is enabled.
Commit 503ceaef8e2e "ima: define a set of appraisal rules requiring
file signatures" was upstreamed. An additional patch could force
these rules to be added to the custom policy, if lockdown is enabled.
This and other patches in this series could then check to see if
is_ima_appraise_enabled() is true.
Mimi
> This code was showed in Matthew's patch but not in git:
> https://lkml.org/lkml/2015/3/13/778
>
> Cc: Matthew Garrett
> Signed-off-by: Chun-Yi Lee
> Signed-off-by: David Howells
> cc: ke...@lists.infradead.org
> ---
>
> kernel/kexec_file.c |7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 9f48f4412297..ff6523f2dcc2 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -255,6 +255,13 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int,
> initrd_fd,
> if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
> return -EPERM;
>
> + /* Don't permit images to be loaded into trusted kernels if we're not
> + * going to verify the signature on them
> + */
> + if (!IS_ENABLED(CONFIG_KEXEC_VERIFY_SIG) &&
> + kernel_is_locked_down("kexec of unsigned images"))
> + return -EPERM;
> +
> /* Make sure we have a legal set of flags */
> if (flags != (flags & KEXEC_FILE_FLAGS))
> return -EINVAL;
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-security-module" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH v1] efivarfs: define integrity_read method
This patch defines simple_read_iter_from_buffer(), replaces the
existing efivarfs ->read method with ->read_iter method, and defines
an ->integrity_read file operation method to read data for integrity
hash collection.
(Posting separately for review, before being squashed with the others.)
Changelog v1:
- totally re-written based on Al's comments, containing source code.
Signed-off-by: Mimi Zohar
---
fs/efivarfs/file.c | 12 +++-
fs/libfs.c | 32
include/linux/fs.h | 2 ++
3 files changed, 41 insertions(+), 5 deletions(-)
diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index 5f22e74bbade..17955a92a5b3 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -64,9 +64,10 @@ static ssize_t efivarfs_file_write(struct file *file,
return bytes;
}
-static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
- size_t count, loff_t *ppos)
+static ssize_t efivarfs_file_read_iter(struct kiocb *iocb,
+ struct iov_iter *iter)
{
+ struct file *file = iocb->ki_filp;
struct efivar_entry *var = file->private_data;
unsigned long datasize = 0;
u32 attributes;
@@ -96,8 +97,8 @@ static ssize_t efivarfs_file_read(struct file *file, char
__user *userbuf,
goto out_free;
memcpy(data, &attributes, sizeof(attributes));
- size = simple_read_from_buffer(userbuf, count, ppos,
- data, datasize + sizeof(attributes));
+ size = simple_read_iter_from_buffer(iocb, iter, data,
+ datasize + sizeof(attributes));
out_free:
kfree(data);
@@ -174,8 +175,9 @@ efivarfs_file_ioctl(struct file *file, unsigned int cmd,
unsigned long p)
const struct file_operations efivarfs_file_operations = {
.open = simple_open,
- .read = efivarfs_file_read,
+ .read_iter = efivarfs_file_read_iter,
.write = efivarfs_file_write,
.llseek = no_llseek,
.unlocked_ioctl = efivarfs_file_ioctl,
+ .integrity_read = efivarfs_file_read_iter,
};
diff --git a/fs/libfs.c b/fs/libfs.c
index a04395334bb1..e1b4f8695013 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -16,6 +16,7 @@
#include
#include
#include /* sync_mapping_buffers */
+#include
#include
@@ -676,6 +677,37 @@ ssize_t simple_write_to_buffer(void *to, size_t available,
loff_t *ppos,
EXPORT_SYMBOL(simple_write_to_buffer);
/**
+ * simple_read_iter_from_buffer - copy data from the buffer to user space
+ * @iocb: struct containing the file, the current position and other info
+ * @to: the user space buffer to read to
+ * @from: the buffer to read from
+ * @available: the size of the buffer
+ *
+ * The simple_read_iter_from_buffer() function reads up to @available bytes
+ * from the current buffer into the user space buffer.
+ *
+ * On success, the current buffer offset is advanced by the number of bytes
+ * read, or a negative value is returned on error.
+ **/
+ssize_t simple_read_iter_from_buffer(struct kiocb *iocb, struct iov_iter *to,
+const void *from, size_t available)
+{
+ loff_t pos = iocb->ki_pos;
+ size_t ret;
+
+ if (pos < 0)
+ return -EINVAL;
+ if (pos >= available)
+ return 0;
+ ret = copy_to_iter(from + pos, available - pos, to);
+ if (!ret && iov_iter_count(to))
+ return -EFAULT;
+ iocb->ki_pos = pos + ret;
+ return ret;
+}
+EXPORT_SYMBOL(simple_read_iter_from_buffer);
+
+/**
* memory_read_from_buffer - copy data from the buffer
* @to: the kernel space buffer to read to
* @count: the maximum number of bytes to read
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 36edfe84c4bf..d85d2c43afd9 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3012,6 +3012,8 @@ extern void simple_release_fs(struct vfsmount **mount,
int *count);
extern ssize_t simple_read_from_buffer(void __user *to, size_t count,
loff_t *ppos, const void *from, size_t available);
+extern ssize_t simple_read_iter_from_buffer(struct kiocb *iocb,
+ struct iov_iter *to, const void *from, size_t available);
extern ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
const void __user *from, size_t count);
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH] efivarfs: define integrity_read method
This patch defines an ->integrity_read file operation method to read data for
integrity hash collection.
(Posting separately for review, before being squashed with the others.)
Signed-off-by: Mimi Zohar
---
fs/efivarfs/file.c | 29 +
1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index 5f22e74bbade..b687c982e0a1 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include
#include "internal.h"
@@ -64,8 +65,9 @@ static ssize_t efivarfs_file_write(struct file *file,
return bytes;
}
-static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
- size_t count, loff_t *ppos)
+static ssize_t __efivarfs_file_read(struct file *file, char __user *userbuf,
+ size_t count, loff_t *ppos,
+ struct iov_iter *iter)
{
struct efivar_entry *var = file->private_data;
unsigned long datasize = 0;
@@ -96,14 +98,32 @@ static ssize_t efivarfs_file_read(struct file *file, char
__user *userbuf,
goto out_free;
memcpy(data, &attributes, sizeof(attributes));
- size = simple_read_from_buffer(userbuf, count, ppos,
- data, datasize + sizeof(attributes));
+
+ if (!iter)
+ size = simple_read_from_buffer(userbuf, count, ppos, data,
+ datasize + sizeof(attributes));
+ else
+ size = copy_to_iter(data, datasize + sizeof(attributes), iter);
out_free:
kfree(data);
return size;
}
+static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
+ size_t count, loff_t *ppos)
+{
+ return __efivarfs_file_read(file, userbuf, count, ppos, NULL);
+}
+
+static ssize_t efivarfs_file_read_iter(struct kiocb *iocb,
+ struct iov_iter *iter)
+{
+ struct file *file = iocb->ki_filp;
+
+ return __efivarfs_file_read(file, NULL, 0, NULL, iter);
+}
+
static int
efivarfs_ioc_getxflags(struct file *file, void __user *arg)
{
@@ -178,4 +198,5 @@ const struct file_operations efivarfs_file_operations = {
.write = efivarfs_file_write,
.llseek = no_llseek,
.unlocked_ioctl = efivarfs_file_ioctl,
+ .integrity_read = efivarfs_file_read_iter,
};
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/24] kexec_file: Disable at runtime if securelevel has been set
Hi David, On Mon, 2017-04-10 at 14:19 +0100, David Howells wrote: > Mimi Zohar wrote: > > > From an IMA perspective, either a file hash or signature are valid, > > but for this usage it must be a signature. > > Not necessarily. If IMA can guarantee that a module is the same based on its > hash rather than on a key, I would've thought that should be fine. File hashes can be modified on the running system, so they're normally used, in conjunction with EVM, to detect off line modification of mutable files and prevent their usage. These patches https://lkml.org/lkml/2017/5/2/465 should provide some of the missing functionality. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/24] kexec_file: Disable at runtime if securelevel has been set
On Fri, 2017-04-07 at 10:17 +0100, David Howells wrote: > Mimi Zohar wrote: > > > > Okay, fair enough. I can stick in an OR with an IS_ENABLED on some IMA > > > symbol. CONFIG_IMA_KEXEC maybe? And also require IMA be enabled? > > > > Not quite, since as Dave pointed out, IMA is policy driven. As a > > policy is installed, we could set a flag. > > Does such a flag exist as yet? Not exactly what is needed. There's a flag named ima_appraise, which is used internally in IMA. A temporary flag is created, while validating the rules. if (default_appraise_rules[i].func == POLICY_CHECK) temp_ima_appraise |= IMA_APPRAISE_POLICY; if (!result && (entry->action == UNKNOWN)) result = -EINVAL; else if (entry->func == MODULE_CHECK) temp_ima_appraise |= IMA_APPRAISE_MODULES; else if (entry->func == FIRMWARE_CHECK) temp_ima_appraise |= IMA_APPRAISE_FIRMWARE; else if (entry->func == POLICY_CHECK) temp_ima_appraise |= IMA_APPRAISE_POLICY; If the entire policy is valid, ima_update_policy_flag() sets the ima_appraise flag. ima_appraise |= temp_ima_appraise; >From an IMA perspective, either a file hash or signature are valid, but for this usage it must be a signature. So in addition to testing entry->func, above, entry->flags would need to be tested as well to detect if IMA_DIGSIG_REQUIRED is set. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/24] kexec_file: Disable at runtime if securelevel has been set
On Fri, 2017-04-07 at 15:41 +0800, Dave Young wrote: > On 04/07/17 at 08:07am, David Howells wrote: > > Dave Young wrote: > > > > > > > > + /* Don't permit images to be loaded into trusted kernels if > > > > > > we're not > > > > > > +* going to verify the signature on them > > > > > > +*/ > > > > > > + if (!IS_ENABLED(CONFIG_KEXEC_VERIFY_SIG) && > > > > > > kernel_is_locked_down()) > > > > > > + return -EPERM; > > > > > > + > > > > > > > > > > > > > > IMA can be used to verify file signatures too, based on the LSM hooks > > > > in kernel_read_file_from_fd(). CONFIG_KEXEC_VERIFY_SIG should not be > > > > required. > > > > > > Mimi, I remember we talked somthing before about the two signature > > > verification. One can change IMA policy in initramfs userspace, > > > also there are kernel cmdline param to disable IMA, so it can break the > > > lockdown? Suppose kexec boot with ima disabled cmdline param and then > > > kexec reboot again.. > > > > I guess I should lock down the parameter to disable IMA too. > > That is one thing, user can change IMA policy in initramfs userspace, > I'm not sure if IMA enforce the signed policy now, if no it will be also > a problem. I'm not sure how this relates to the question of whether IMA verifies the kexec kernel image signature, as the test would not be based on a Kconfig option, but on a runtime variable. To answer your question, the rule for requiring the policy to be signed is: appraise func=POLICY_CHECK appraise_type=imasig When the ability to append rules is Kconfig enabled, the builtin policy requires the new policy or additional rules to be signed. Unfortunately, always requiring the policy to be signed, would have broken userspace. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/24] kexec_file: Disable at runtime if securelevel has been set
On Fri, 2017-04-07 at 08:09 +0100, David Howells wrote: > Mimi Zohar wrote: > > > > > + if (!IS_ENABLED(CONFIG_KEXEC_VERIFY_SIG) && > > > > kernel_is_locked_down()) > > > > + return -EPERM; > > > > + > > > > > > > > IMA can be used to verify file signatures too, based on the LSM hooks > > in kernel_read_file_from_fd(). CONFIG_KEXEC_VERIFY_SIG should not be > > required. > > Okay, fair enough. I can stick in an OR with an IS_ENABLED on some IMA > symbol. CONFIG_IMA_KEXEC maybe? And also require IMA be enabled? Not quite, since as Dave pointed out, IMA is policy driven. As a policy is installed, we could set a flag. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/24] kexec_file: Disable at runtime if securelevel has been set
On Fri, 2017-04-07 at 14:19 +0800, Dave Young wrote: > On 04/06/17 at 11:49pm, Mimi Zohar wrote: > > On Fri, 2017-04-07 at 11:05 +0800, Dave Young wrote: > > > On 04/05/17 at 09:15pm, David Howells wrote: > > > > From: Chun-Yi Lee > > > > > > > > When KEXEC_VERIFY_SIG is not enabled, kernel should not loads image > > > > through kexec_file systemcall if securelevel has been set. > > > > > > > > This code was showed in Matthew's patch but not in git: > > > > https://lkml.org/lkml/2015/3/13/778 I specifically checked to make sure that either kexec_file() signature verification was acceptable and would have commented then, if it had not been included. > > > > Cc: Matthew Garrett > > > > Signed-off-by: Chun-Yi Lee > > > > Signed-off-by: David Howells > > > > cc: ke...@lists.infradead.org > > > > --- > > > > > > > > kernel/kexec_file.c |6 ++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > > > > index b118735fea9d..f6937eecd1eb 100644 > > > > --- a/kernel/kexec_file.c > > > > +++ b/kernel/kexec_file.c > > > > @@ -268,6 +268,12 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, > > > > int, initrd_fd, > > > > if (!capable(CAP_SYS_BOOT) || kexec_load_disabled) > > > > return -EPERM; > > > > > > > > + /* Don't permit images to be loaded into trusted kernels if > > > > we're not > > > > +* going to verify the signature on them > > > > +*/ > > > > + if (!IS_ENABLED(CONFIG_KEXEC_VERIFY_SIG) && > > > > kernel_is_locked_down()) > > > > + return -EPERM; > > > > + > > > > > > > > IMA can be used to verify file signatures too, based on the LSM hooks > > in kernel_read_file_from_fd(). CONFIG_KEXEC_VERIFY_SIG should not be > > required. > > Mimi, I remember we talked somthing before about the two signature > verification. One can change IMA policy in initramfs userspace, > also there are kernel cmdline param to disable IMA, so it can break the > lockdown? Suppose kexec boot with ima disabled cmdline param and then > kexec reboot again.. Right, we discussed that the same method of measuring the kexec image and initramfs, for extending trusted boot to the OS, could also be used for verifying the kexec image and initramfs signatures, for extending secure boot to the OS. The file hash would be calculated once for both. All of your concerns could be addressed with very minor changes to IMA. (Continued in response to David.) > > > > > /* Make sure we have a legal set of flags */ > > > > if (flags != (flags & KEXEC_FILE_FLAGS)) > > > > return -EINVAL; > > > > > > > > > > > > ___ > > > > kexec mailing list > > > > ke...@lists.infradead.org > > > > http://lists.infradead.org/mailman/listinfo/kexec > > > > > > Acked-by: Dave Young > > > > > > Thanks > > > Dave > > > -- > > > To unsubscribe from this list: send the line "unsubscribe > > > linux-security-module" in > > > the body of a message to majord...@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/24] kexec_file: Disable at runtime if securelevel has been set
On Fri, 2017-04-07 at 11:05 +0800, Dave Young wrote: > On 04/05/17 at 09:15pm, David Howells wrote: > > From: Chun-Yi Lee > > > > When KEXEC_VERIFY_SIG is not enabled, kernel should not loads image > > through kexec_file systemcall if securelevel has been set. > > > > This code was showed in Matthew's patch but not in git: > > https://lkml.org/lkml/2015/3/13/778 > > > > Cc: Matthew Garrett > > Signed-off-by: Chun-Yi Lee > > Signed-off-by: David Howells > > cc: ke...@lists.infradead.org > > --- > > > > kernel/kexec_file.c |6 ++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > > index b118735fea9d..f6937eecd1eb 100644 > > --- a/kernel/kexec_file.c > > +++ b/kernel/kexec_file.c > > @@ -268,6 +268,12 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, > > initrd_fd, > > if (!capable(CAP_SYS_BOOT) || kexec_load_disabled) > > return -EPERM; > > > > + /* Don't permit images to be loaded into trusted kernels if we're not > > +* going to verify the signature on them > > +*/ > > + if (!IS_ENABLED(CONFIG_KEXEC_VERIFY_SIG) && kernel_is_locked_down()) > > + return -EPERM; > > + > > IMA can be used to verify file signatures too, based on the LSM hooks in kernel_read_file_from_fd(). CONFIG_KEXEC_VERIFY_SIG should not be required. Mimi > /* Make sure we have a legal set of flags */ > > if (flags != (flags & KEXEC_FILE_FLAGS)) > > return -EINVAL; > > > > > > ___ > > kexec mailing list > > ke...@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/kexec > > Acked-by: Dave Young > > Thanks > Dave > -- > To unsubscribe from this list: send the line "unsubscribe > linux-security-module" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/12] One more attempt at useful kernel lockdown
On Tue, 2013-09-10 at 12:44 -0700, H. Peter Anvin wrote: > On 09/10/2013 12:17 PM, David Lang wrote: > >> > >> In theory these blobs are traceable to a manufacturer. It's not really > >> an indication that it's "safe" more than it's an indication that it > >> hasn't been changed. But I haven't chased this very hard yet because > >> of below... > > > > well, not if you are trying to defend against root breaking in to the > > machine. > > > > And we have at least some drivers where we even have the firmware in the > Linux kernel tree, and thus aren't opaque blobs at all. > > I suspect we'll need, at some point, a way for vendors that aren't > already doing signatures on their firmware in a device-specific way to > do so in a kernel-supported way. The easiest (in terms of getting > vendors to play along, not necessarily technically) might be a PGP > signature (either inline or standalone) and have the public key as part > of the driver? Why invent yet another method of verifying the integrity of a file based on a signature? Why not use the existing method for appraising files? Just create a new integrity hook at the appropriate place. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/12] One more attempt at useful kernel lockdown
On Tue, 2013-09-10 at 16:48 -0700, H. Peter Anvin wrote: > On 09/10/2013 04:43 PM, Mimi Zohar wrote: > > > > Why invent yet another method of verifying the integrity of a file based > > on a signature? Why not use the existing method for appraising files? > > Just create a new integrity hook at the appropriate place. > > > > What would the deliverables be from the hardware vendor and what tools > would you expect them to need on their end? The package installer needs to not only install files, but file metadata as well. Elena Reshetova (Intel) has already added rpm hooks to write security xattrs. The next step, yet to be done, is to include and write the signatures as part of the rpm install process. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/12] One more attempt at useful kernel lockdown
On Mon, 2013-09-09 at 11:49 -0400, Matthew Garrett wrote: > Some use cases require the ability to ensure that anything running in ring 0 > is trusted code. We have support for signing the kernel and kernel modules, > but there's still a range of exported kernel interfaces that make it easy to > modify the running kernel. Previous attempts to implement a generic interface > to restrict this have included a new capability (breaks existing userspace) > and tying it to a requirement for signed modules (breaks assumptions in > certain situations where userspace is already running with restricted > privileges). > > So, this is my final attempt at providing the functionality I'm interested > in without inherently tying it to Secure Boot. There's strong parallels > between the functionality that I'm interested in and the BSD securelevel > interface, so here's a trivial implementation. Thank you for not tying this functionality to UEFI secure boot. There are, and will continue to be, many systems out there that don't support UEFI secure boot, yet would be interested in this functionality. As David Lang aptly said, "... security is not a binary thing, just because there is one hole, it isn't worthless to close another one." Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/12] Security: Add CAP_COMPROMISE_KERNEL
On Wed, 2013-03-20 at 20:37 +, Matthew Garrett wrote: > On Wed, 2013-03-20 at 15:16 -0400, Mimi Zohar wrote: > > On Wed, 2013-03-20 at 18:12 +, Matthew Garrett wrote: > > > Well, in the absence of hardcoded in-kernel policy, there needs to be > > > some mechanism for ensuring the integrity of a policy. Shipping a signed > > > policy initramfs fragment and having any Secure Boot bootloaders pass a > > > flag in bootparams indicating that the kernel should panic if that > > > fragment isn't present would seem to be the easiest way of doing that. > > > Or have I misunderstood the question? > > > > Ok, I was confused by the term "fragmented" initramfs. So once you have > > verified the "early" fragmented initramfs signature, this initramfs will > > load the "trusted" public keys and could also load the MAC policy. (I > > realize that dracut is currently loading the MAC policy, not the > > initramfs.) The MAC policy would then be trusted, right? Could we then > > use the LSM labels for defining an integrity policy for kexec? > > Right, that'd be the rough idea. Any further runtime policy updates > would presumably need to be signed with a trusted key. I'm really sorry to belabor this point, but can kexec rely on an LSM label to identify a specific file, out of all the files being executed, in a secure boot environment? The SELinux integrity rule for kexec would then look something like, appraise func=BPRM_CHECK obj_type=kdump_exec_t appraise_type=imasig We could then follow this up with Serge's idea of, "a capset akin to the bounding set, saying you can only have the caps in this set if the running binary was a signed one." kexec already requires CAP_SYS_BOOT. thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/12] Security: Add CAP_COMPROMISE_KERNEL
On Wed, 2013-03-20 at 18:12 +, Matthew Garrett wrote: > On Wed, 2013-03-20 at 14:01 -0400, Mimi Zohar wrote: > > > Sorry, I'm not sure to which work you're referring. If you're referring > > to Dmitry's "initramfs with digital signature protection" patches, then > > we're speaking about enforcing integrity, not MAC security. > > Well, in the absence of hardcoded in-kernel policy, there needs to be > some mechanism for ensuring the integrity of a policy. Shipping a signed > policy initramfs fragment and having any Secure Boot bootloaders pass a > flag in bootparams indicating that the kernel should panic if that > fragment isn't present would seem to be the easiest way of doing that. > Or have I misunderstood the question? Ok, I was confused by the term "fragmented" initramfs. So once you have verified the "early" fragmented initramfs signature, this initramfs will load the "trusted" public keys and could also load the MAC policy. (I realize that dracut is currently loading the MAC policy, not the initramfs.) The MAC policy would then be trusted, right? Could we then use the LSM labels for defining an integrity policy for kexec? thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/12] Security: Add CAP_COMPROMISE_KERNEL
On Wed, 2013-03-20 at 16:49 +, Matthew Garrett wrote: > On Wed, 2013-03-20 at 12:41 -0400, Mimi Zohar wrote: > > > Matthrew, perhaps you could clarify whether this will be tied to MAC > > security. Based on the kexec thread, I'm under the impression that is > > not the intention, or at least not for kexec. As root isn't trusted, > > neither is the boot command line, nor any policy that is loaded by root, > > including those for MAC. > > The work done on signed initramfs fragments would seem to be the best > option here so far? Sorry, I'm not sure to which work you're referring. If you're referring to Dmitry's "initramfs with digital signature protection" patches, then we're speaking about enforcing integrity, not MAC security. thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/12] Security: Add CAP_COMPROMISE_KERNEL
On Tue, 2013-03-19 at 15:47 +1100, James Morris wrote: > On Mon, 18 Mar 2013, Matthew Garrett wrote: > > > This patch introduces CAP_COMPROMISE_KERNEL. > > I'd like to see this named CAP_MODIFY_KERNEL, which is more accurate and > less emotive. Otherwise I think core kernel developers will be scratching > their head over where to sprinkle this. > > Apart from that, I like the idea, especially when it's wired up to MAC > security. Matthrew, perhaps you could clarify whether this will be tied to MAC security. Based on the kexec thread, I'm under the impression that is not the intention, or at least not for kexec. As root isn't trusted, neither is the boot command line, nor any policy that is loaded by root, including those for MAC. thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
