Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;
Hi,
On Fri, Oct 16, 2015 at 10:31 PM, Petko Manolov wrote:
> IMA policy can now be updated multiple times. The new rules get appended
> to the original policy. Have in mind that the rules are scanned in FIFO
> order so be careful when you add new ones.
>
> The mutex locks are replaced with RCU, which should lead to faster policy
> traversals. The new rules are first appended to a temporary list, which
> on error gets released without disturbing the normal IMA operations.
>
> Signed-off-by: Petko Manolov
> ---
> security/integrity/ima/Kconfig | 14
> security/integrity/ima/ima_fs.c | 13 +++
> security/integrity/ima/ima_policy.c | 70
> +
> 3 files changed, 74 insertions(+), 23 deletions(-)
>
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index df30334..15264b7 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -107,6 +107,20 @@ config IMA_DEFAULT_HASH
> default "sha512" if IMA_DEFAULT_HASH_SHA512
> default "wp512" if IMA_DEFAULT_HASH_WP512
>
> +config IMA_WRITE_POLICY
> + bool "Enable multiple writes to the IMA policy"
> + depends on IMA
> + default n
> + help
> + IMA policy can now be updated multiple times. The new rules get
> + appended to the original policy. Have in mind that the rules are
> + scanned in FIFO order so be careful when you add new ones.
> +
> + WARNING: Potential security hole - should be used with care in
> + production-grade kernels!
> +
> + If unsure, say N.
> +
> config IMA_APPRAISE
> bool "Appraise integrity measurements"
> depends on IMA
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index 816d175..a3cf5c0 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -25,6 +25,8 @@
>
> #include "ima.h"
>
> +static DEFINE_MUTEX(ima_write_mutex);
> +
> static int valid_policy = 1;
> #define TMPBUFLEN 12
> static ssize_t ima_show_htable_value(char __user *buf, size_t count,
> @@ -261,6 +263,11 @@ static ssize_t ima_write_policy(struct file *file, const
> char __user *buf,
> {
> char *data = NULL;
> ssize_t result;
> + int res;
> +
> + res = mutex_lock_interruptible(&ima_write_mutex);
> + if (res)
> + return res;
>
> if (datalen >= PAGE_SIZE)
> datalen = PAGE_SIZE - 1;
> @@ -286,6 +293,8 @@ out:
> if (result < 0)
> valid_policy = 0;
> kfree(data);
> + mutex_unlock(&ima_write_mutex);
> +
> return result;
> }
>
> @@ -337,8 +346,12 @@ static int ima_release_policy(struct inode *inode,
> struct file *file)
> return 0;
> }
> ima_update_policy();
> +#ifndefCONFIG_IMA_WRITE_POLICY
> securityfs_remove(ima_policy);
> ima_policy = NULL;
> +#else
> + clear_bit(IMA_FS_BUSY, &ima_fs_flags);
> +#endif
> return 0;
> }
>
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c
> index 3997e20..7ace4e4 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -16,6 +16,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include "ima.h"
> @@ -135,11 +136,11 @@ static struct ima_rule_entry default_appraise_rules[] =
> {
>
> static LIST_HEAD(ima_default_rules);
> static LIST_HEAD(ima_policy_rules);
> +static LIST_HEAD(ima_temp_rules);
> static struct list_head *ima_rules;
>
> -static DEFINE_MUTEX(ima_rules_mutex);
> -
> static int ima_policy __initdata;
> +
> static int __init default_measure_policy_setup(char *str)
> {
> if (ima_policy)
> @@ -171,9 +172,8 @@ static int __init default_appraise_policy_setup(char *str)
> __setup("ima_appraise_tcb", default_appraise_policy_setup);
>
> /*
> - * Although the IMA policy does not change, the LSM policy can be
> - * reloaded, leaving the IMA LSM based rules referring to the old,
> - * stale LSM policy.
> + * Blocking here is not legal as we hold an RCU lock. ima_update_policy()
> + * should make it safe to walk the list at any time.
> *
> * Update the IMA LSM based rules to reflect the reloaded LSM policy.
> * We assume the rules still exist; and BUG_ON() if they don't.
> @@ -184,7 +184,6 @@ static void ima_lsm_update_rules(void)
> int result;
> int i;
>
> - mutex_lock(&ima_rules_mutex);
> list_for_each_entry_safe(entry, tmp, &ima_policy_rules, list) {
Use of "list_for_each_entry_safe" makes me having a doubt.
"safe" versions are used when entries are removed while walk.
If it is so, then entire RCU case is impossible?
Dmitry
> for (i = 0; i < MAX_LSM_RULES; i++) {
> if (!entry->lsm[i].rule)
> @@ -196,7 +195,6 @@ static void ima_lsm_update_rules(void)
>
Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;
On Sat, Oct 24, 2015 at 3:28 PM, Petko Manolov wrote: > On 15-10-23 20:13:41, Dmitry Kasatkin wrote: >> On Fri, Oct 23, 2015 at 3:29 PM, Petko Manolov wrote: >> > >> > I was actually going to get rid of IMA_FS_BUSY. It is less flexible with >> > respect to user-space tools. If the flag is up then the policy upload will >> > fail. The user script or program must check the error and repeat the >> > operation after some time. Seems kind of pointless to me, unless i am >> > missing something. >> > >> > With a semaphore the second process will block and write the policy once >> > the >> > first writer leave the operation. No need to repeat it unless there's some >> > real error. >> > >> > I was trying to be careful and not break the code in case the new >> > functionality is not selected in Kconfig. However, with your most recent >> > patch-set i guess we'll have to revisit a few things. :) >> >> Obviously in original situation it will be only a "single" policy writer - >> which IMA init script or binary. If some other script tries to write policy >> at >> the same time I would consider it as someone exploit would trying to do nasty >> things. > > You've got a point here. If we get rid of the busy flag we'll have to block > at > open() instead of write() in order to keep the "original" semantics. > No, that was not my point. The point was that there is no another/simultaneous policy writer in reality. >> With possibility to update policy I also do not see any need for multiple >> writers, when second waits first to finish update and it is not aware about >> coming another update. It would be some integrity manager doing policy >> updates >> sequentially from time to time. > > Nope, that's not the only scenario. Imagine a machine with multiple tenants > and > huge uptime. Imagine also that these tenants want to run their own software > on > this machine. Policy write may occur at any time. > No, I cannot imagine that unrelated processes can write/update policy simultaneously at any time. I can imagine that some device/system management software does policy update. May be you could give more exact use case. >> But with "reading" the policy file, I could imaging process blocking to wait >> when update/read completes. > > We don't need mutexes to safely read the policy. The code that does list > splicing is taking care of the reader either seeing the original policy or the > new one. Not a disjointed version of it. > > Whether one will see the old or the new policy is a matter of timing and > semaphores are not going to change this. > Indeed. > > Petko -- Thanks, Dmitry -- 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
[PATCH] keys, trusted: select TPM2 hash algorithm
Added 'hashalg=' option for selecting the hash algorithm.
Currently available options are:
* sha1
* sha256
* sha384
* sha512
* sm3_256
Signed-off-by: Jarkko Sakkinen
---
drivers/char/tpm/tpm.h | 5 -
drivers/char/tpm/tpm2-cmd.c | 34 ++
include/keys/trusted-type.h | 2 ++
security/keys/trusted.c | 8 +++-
4 files changed, 47 insertions(+), 2 deletions(-)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a4257a3..4c18f46 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -92,7 +92,10 @@ enum tpm2_algorithms {
TPM2_ALG_SHA1 = 0x0004,
TPM2_ALG_KEYEDHASH = 0x0008,
TPM2_ALG_SHA256 = 0x000B,
- TPM2_ALG_NULL = 0x0010
+ TPM2_ALG_SHA384 = 0x000C,
+ TPM2_ALG_SHA512 = 0x000D,
+ TPM2_ALG_NULL = 0x0010,
+ TPM2_ALG_SM3_256= 0x0012,
};
enum tpm2_command_codes {
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index bd7039f..0704bd6 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -104,6 +104,22 @@ struct tpm2_cmd {
union tpm2_cmd_params params;
} __packed;
+struct tpm2_hashalg {
+ charname[MAX_HASHALG_SIZE];
+ u32 id;
+};
+
+struct tpm2_hashalg tpm2_hashalg_map[] = {
+ {"sha1", TPM2_ALG_SHA1},
+ {"sha256", TPM2_ALG_SHA256},
+ {"sm3_256", TPM2_ALG_SM3_256},
+ {"sha384", TPM2_ALG_SHA384},
+ {"sha512", TPM2_ALG_SHA512},
+};
+
+#define TPM2_HASHALG_COUNT \
+ (sizeof(tpm2_hashalg_map) / sizeof(tpm2_hashalg_map[1]))
+
/*
* Array with one entry per ordinal defining the maximum amount
* of time the chip could take to return the result. The values
@@ -429,8 +445,26 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
{
unsigned int blob_len;
struct tpm_buf buf;
+ u32 hashalg = TPM2_ALG_SHA256;
+ int i;
int rc;
+ if (strlen(options->hashalg) > 0) {
+ for (i = 0; i < TPM2_HASHALG_COUNT; i++) {
+ if (!strcmp(options->hashalg,
+ tpm2_hashalg_map[i].name)) {
+ hashalg = tpm2_hashalg_map[i].id;
+ dev_dbg(chip->pdev, "%s: hashalg: %s 0x%08X\n",
+ __func__, tpm2_hashalg_map[i].name,
+ hashalg);
+ break;
+ }
+ }
+
+ if (i == TPM2_HASHALG_COUNT)
+ return -EINVAL;
+ }
+
rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
if (rc)
return rc;
diff --git a/include/keys/trusted-type.h b/include/keys/trusted-type.h
index f91ecd9..a545733 100644
--- a/include/keys/trusted-type.h
+++ b/include/keys/trusted-type.h
@@ -18,6 +18,7 @@
#define MAX_KEY_SIZE 128
#define MAX_BLOB_SIZE 512
#define MAX_PCRINFO_SIZE 64
+#define MAX_HASHALG_SIZE 16
struct trusted_key_payload {
struct rcu_head rcu;
@@ -36,6 +37,7 @@ struct trusted_key_options {
uint32_t pcrinfo_len;
unsigned char pcrinfo[MAX_PCRINFO_SIZE];
int pcrlock;
+ unsigned char hashalg[MAX_HASHALG_SIZE];
};
extern struct key_type key_type_trusted;
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index d3633cf..9e7564d 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -710,7 +710,8 @@ enum {
Opt_err = -1,
Opt_new, Opt_load, Opt_update,
Opt_keyhandle, Opt_keyauth, Opt_blobauth,
- Opt_pcrinfo, Opt_pcrlock, Opt_migratable
+ Opt_pcrinfo, Opt_pcrlock, Opt_migratable,
+ Opt_hashalg,
};
static const match_table_t key_tokens = {
@@ -723,6 +724,7 @@ static const match_table_t key_tokens = {
{Opt_pcrinfo, "pcrinfo=%s"},
{Opt_pcrlock, "pcrlock=%s"},
{Opt_migratable, "migratable=%s"},
+ {Opt_hashalg, "hashalg=%s"},
{Opt_err, NULL}
};
@@ -787,6 +789,10 @@ static int getoptions(char *c, struct trusted_key_payload
*pay,
return -EINVAL;
opt->pcrlock = lock;
break;
+ case Opt_hashalg:
+ strncpy(opt->hashalg, args[0].from,
+ MAX_HASHALG_SIZE - 1);
+ break;
default:
return -EINVAL;
}
--
2.5.0
--
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
Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;
On 15-10-23 20:13:41, Dmitry Kasatkin wrote: > On Fri, Oct 23, 2015 at 3:29 PM, Petko Manolov wrote: > > > > I was actually going to get rid of IMA_FS_BUSY. It is less flexible with > > respect to user-space tools. If the flag is up then the policy upload will > > fail. The user script or program must check the error and repeat the > > operation after some time. Seems kind of pointless to me, unless i am > > missing something. > > > > With a semaphore the second process will block and write the policy once > > the > > first writer leave the operation. No need to repeat it unless there's some > > real error. > > > > I was trying to be careful and not break the code in case the new > > functionality is not selected in Kconfig. However, with your most recent > > patch-set i guess we'll have to revisit a few things. :) > > Obviously in original situation it will be only a "single" policy writer - > which IMA init script or binary. If some other script tries to write policy > at > the same time I would consider it as someone exploit would trying to do nasty > things. You've got a point here. If we get rid of the busy flag we'll have to block at open() instead of write() in order to keep the "original" semantics. > With possibility to update policy I also do not see any need for multiple > writers, when second waits first to finish update and it is not aware about > coming another update. It would be some integrity manager doing policy > updates > sequentially from time to time. Nope, that's not the only scenario. Imagine a machine with multiple tenants and huge uptime. Imagine also that these tenants want to run their own software on this machine. Policy write may occur at any time. > But with "reading" the policy file, I could imaging process blocking to wait > when update/read completes. We don't need mutexes to safely read the policy. The code that does list splicing is taking care of the reader either seeing the original policy or the new one. Not a disjointed version of it. Whether one will see the old or the new policy is a matter of timing and semaphores are not going to change this. Petko -- 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
Re: [PATCHv3 1/6] integrity: define '.evm' as a builtin 'trusted' keyring
On 15-10-23 14:43:53, Mimi Zohar wrote: > On Fri, 2015-10-23 at 16:05 +0300, Petko Manolov wrote: > > On 15-10-22 21:49:25, Dmitry Kasatkin wrote: > > > > diff --git a/security/integrity/ima/Kconfig > > > b/security/integrity/ima/Kconfig > > > index df30334..a292b88 100644 > > > --- a/security/integrity/ima/Kconfig > > > +++ b/security/integrity/ima/Kconfig > > > @@ -123,14 +123,17 @@ config IMA_APPRAISE > > > If unsure, say N. > > > > > > config IMA_TRUSTED_KEYRING > > > - bool "Require all keys on the .ima keyring be signed" > > > + bool "Require all keys on the .ima keyring be signed (deprecated)" > > > depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING > > > depends on INTEGRITY_ASYMMETRIC_KEYS > > > + select INTEGRITY_TRUSTED_KEYRING > > > default y > > > help > > > This option requires that all keys added to the .ima > > > keyring be signed by a key on the system trusted keyring. > > > > > > +This option is deprecated in favor of INTEGRITY_TRUSTED_KEYRING > > > + > > > config IMA_LOAD_X509 > > > bool "Load X509 certificate onto the '.ima' trusted keyring" > > > depends on IMA_TRUSTED_KEYRING > > > > > > I guess we may as well remove this switch. Otherwise somebody have to > > remember > > to post a patch that does so a few kernel releases after this one goes > > mainline. > > There's no harm in leaving the "IMA_TRUSTED_KEYRING" Kconfig option for a > couple of releases (or perhaps until it is enabled in a long term release), > so > that the INTEGRITY_TRUSTED_KEYRING Kconfig option is enabled automatically. I have no strong opinion either way. Just saying. Let it be for the moment. Petko -- 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
