* [PATCH v5 03/10] IMA: Read keyrings= option from the IMA policy into ima_rule_entry
From: Lakshmi Ramasubramanian @ 2019-11-11 19:32 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191111193303.12781-1-nramas@linux.microsoft.com>
"keyrings=" option, if specified in the IMA policy, needs to be
stored in the list of IMA rules when the configured IMA policy is read.
This patch defines a new policy token enum namely Opt_keyrings
for reading "keyrings=" option from the IMA policy.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
security/integrity/ima/ima_policy.c | 28 +++++++++++++++++++++++++++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index a0f7ffa80736..1aee3c8b9cf6 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -768,7 +768,8 @@ enum {
Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
Opt_appraise_type, Opt_appraise_flag,
- Opt_permit_directio, Opt_pcr, Opt_template, Opt_err
+ Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
+ Opt_err
};
static const match_table_t policy_tokens = {
@@ -804,6 +805,7 @@ static const match_table_t policy_tokens = {
{Opt_permit_directio, "permit_directio"},
{Opt_pcr, "pcr=%s"},
{Opt_template, "template=%s"},
+ {Opt_keyrings, "keyrings=%s"},
{Opt_err, NULL}
};
@@ -1053,6 +1055,23 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
result = 0;
entry->flags |= IMA_FSNAME;
break;
+ case Opt_keyrings:
+ ima_log_string(ab, "keyrings", args[0].from);
+
+ if ((entry->keyrings) ||
+ (entry->action != MEASURE) ||
+ (entry->func != KEYRING_CHECK)) {
+ result = -EINVAL;
+ break;
+ }
+ entry->keyrings = kstrdup(args[0].from, GFP_KERNEL);
+ if (!entry->keyrings) {
+ result = -ENOMEM;
+ break;
+ }
+ result = 0;
+ entry->flags |= IMA_KEYRINGS;
+ break;
case Opt_fsuuid:
ima_log_string(ab, "fsuuid", args[0].from);
@@ -1428,6 +1447,13 @@ int ima_policy_show(struct seq_file *m, void *v)
seq_puts(m, " ");
}
+ if (entry->flags & IMA_KEYRINGS) {
+ if (entry->keyrings != NULL)
+ snprintf(tbuf, sizeof(tbuf), "%s", entry->keyrings);
+ seq_printf(m, pt(Opt_keyrings), tbuf);
+ seq_puts(m, " ");
+ }
+
if (entry->flags & IMA_PCR) {
snprintf(tbuf, sizeof(tbuf), "%d", entry->pcr);
seq_printf(m, pt(Opt_pcr), tbuf);
--
2.17.1
^ permalink raw reply related
* [PATCH v5 0/10] KEYS: Measure keys when they are created or updated
From: Lakshmi Ramasubramanian @ 2019-11-11 19:32 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
Problem Statement:
Keys created or updated in the system are currently not being measured.
Therefore an attestation service, for instance, would not be able to
attest whether or not the trusted keys keyring(s), for instance, contain
only known good (trusted) keys.
IMA measures system files, command line arguments passed to kexec,
boot aggregate, etc. It can be used to measure keys as well.
But there is no mechanism available in the kernel for IMA to
know when a key is created or updated.
This change aims to address measuring keys created or updated
in the system:
=> Patches #1 through #5 update IMA policy functions to handle
measurement of keys based on configured IMA policy.
=> Patches #6 and #7 add IMA hook for measuring keys and the call
to the IMA hook from key_create_or_update function.
Keys are processed immediately - no support for
deferred processing.
=> Patches #8 through #10 add support for queuing keys if
custom IMA policies have not been applied yet and process
the queued keys when custom IMA policies are applied.
To achieve the above the following changes have been made:
- Added a new IMA hook namely, ima_post_key_create_or_update, which
measures the key. This IMA hook is called from key_create_or_update
function. The key measurement can be controlled through IMA policy.
In this change set a new IMA policy function KEYRING_CHECK has been
added to measure keys. The policy can optionally specify a set of
keyrings to measure. By default all keyrings are included in
the measurement when KEYRING_CHECK policy is specified.
# measure all keys
measure func=KEYRING_CHECK
# measure keys on the IMA keyring
measure func=KEYRING_CHECK keyring=".ima"
# measure keys on the BUILTIN and IMA keyrings into a different PCR
measure func=KEYRING_CHECK keyring=".builtin_trusted_keys|.ima" pcr=11
Testing performed:
* Booted the kernel with this change.
* Executed keyctl tests from the Linux Test Project (LTP)
* All keys are measured when only KEYRING_CHECK is set.
* Only keys added to the given keyrings are measured
when keyrings option is set.
* Keys are not measured when KEYRING_CHECK is not set.
* Key is queued for measurement if IMA is not yet initialized
and processed when IMA is initialized.
* Key is measured rightaway when IMA is initialized.
* Added a new key to a keyring and verified "key create" code path.
=> In this case added a key to .ima keyring.
* Added the same key again and verified "key update" code path.
=> Add the same key to .ima keyring.
Change Log:
v5:
=> Reorganized the patches to add measurement of keys through
the IMA hook without any queuing and then added queuing support.
=> Updated the queuing functions to minimize code executed inside mutex.
=> Process queued keys after custom IMA policies have been applied.
v4:
=> Rebased the changes to v5.4-rc3
=> Applied the following dependent patch set first
and then added new changes.
https://lore.kernel.org/linux-integrity/1572492694-6520-1-git-send-email-zohar@linux.ibm.com
=> Refactored the patch set to separate out changes related to
func KEYRING_CHECK and options keyrings into different patches.
=> Moved the functions to queue and dequeue keys for measurement
from ima_queue.c to a new file ima_asymmetric_keys.c.
=> Added a new config namely CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
to compile ima_asymmetric_keys.c
v3:
=> Added KEYRING_CHECK for measuring keys. This can optionally specify
keyrings to measure.
=> Updated ima_get_action() and related functions to return
the keyrings if specified in the policy.
=> process_buffer_measurement() function is updated to take keyring
as a parameter. The key will be measured if the policy includes
the keyring in the list of measured keyrings. If the policy does not
specify any keyrings then all keys are measured.
v2:
=> Per suggestion from Mimi reordered the patch set to first
enable measuring keys added or updated in the system.
And, then scope the measurement to keys added to
builtin_trusted_keys keyring through ima policy.
=> Removed security_key_create_or_update function and instead
call ima hook, to measure the key, directly from
key_create_or_update function.
v1:
=> LSM function for key_create_or_update. It calls ima.
=> Added ima hook for measuring keys
=> ima measures keys based on ima policy.
v0:
=> Added LSM hook for key_create_or_update.
=> Measure keys added to builtin or secondary trusted keys keyring.
Background:
Currently ima measures file hashes and .ima signatures. ima signatures
are validated against keys in the ".ima" keyring. If the kernel is built
with CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY enabled,
then all keys in ".ima" keyring must be signed by a key in
".builtin_trusted_keys" or ".secondary_trusted_keys" keyrings.
Although ima supports the above configuration, not having an insight
into what keys are present in these trusted keys keyrings would prevent
an attestation service from validating a client machine.
On systems with CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
enabled, measuring keys in the ".builtin_trusted_keys" keyring provides
a mechanism to attest that the client's system binaries are indeed signed
by signers that chain to known trusted keys.
Without this change, to attest the clients one needs to maintain
an "allowed list" of file hashes of all versions of all client binaries
that are deployed on the clients in the enterprise. That is a huge
operational challenge in a large scale environment of clients with
heterogenous builds. This also limits scalability and agility of
rolling out frequent client binary updates.
Questions and concerns raised by reviewers on this patch set:
Question 1:
Is "Signed with a trusted key" equal to "Trusted file"?
Doesn't the service need the hashes of the system files to determine
whether a file is trusted or not?
"Signed with a trusted key" does not equal "Trusted"
Answer:
Agree "Signed with a trusted key" may not equal "Trusted".
To address this, the attesting service can maintain a small
manageable set of bad hashes (a "Blocked list") and a list of
trusted keys expected in client's .builtin_trusted_keys" keyring.
Using this data, the service can detect the presence of
"Disallowed (untrusted) version of client binaries".
Question 2:
Providing more data to the service (such as ".builtin_trusted_keys"),
empowers the service to deny access to clients (block clients).
IMA walks a fine line in enforcing and measuring file integrity.
This patchset breaches that fine line and in doing so brings back
the fears of trusted computing.
Answer:
Any new measurement we add in IMA will provide more data to service
and can enable it to deny access to clients. It is not clear why this patch
set would breach the fine line between measuring and enforcing.
Since this patch set is disabled by default and enabled through
CONFIG_IMA_MEASURE_TRUSTED_KEYS, only those enterprises that
require this new measurement can opt-in for it. Since it is disabled
by default, it does not restrict the autonomy of independent users
who are unaffected by attestation.
Question 3:
IMA log already contains a pointer to the IMA keys used for signature
verification. Why does the service need to care what keys were used
to sign (install) the IMA keys? What is gained by measuring the keys
in the ".builtin_trusted_keys"
Answer:
To attest the clients using the current IMA log, service needs to maintain
hashes of all the deployed versions of all the system binaries for their
enterprise. This will introduce a very high operational overhead in
a large scale environment of clients with heterogenous builds.
This limits scalability and agility of rolling out frequent client
binary updates.
On the other hand, with the current patch set, we will have IMA
validate the file signature on the clients and the service validate
that the IMA keys were installed using trusted keys.
This provides a chain of trust:
=> IMA Key validates file signature on the client
=> Built-In trusted key attests IMA key on the client
=> Attestation service attests the Built-In trusted keys
reported by the client in the IMA log
This approach, therefore, would require the service to maintain
a manageble set of trusted keys that it receives from a trusted source.
And, verify if the clients only have keys from that set of trusted keys.
Question 4:
Where will the attestation service receive the keys to validate against?
Answer:
Attestation service will receive the keys from a trusted source such as
the enterprise build services that provides the client builds.
The service will use this set of keys to verify that the keys reported by
the clients in the IMA log contains only keys from this trusted list.
Question 5:
What is changing in the IMA log through this patch set?
Answer:
This patch set does not remove any data that is currently included
in the IMA log. It only adds more data to the IMA log - the data on
".builtin_trusted_keys"
Lakshmi Ramasubramanian (10):
IMA: Added KEYRING_CHECK func in IMA policy to measure keys
IMA: Added keyrings= option in IMA policy to only measure keys added
to the specified keyrings.
IMA: Read keyrings= option from the IMA policy into ima_rule_entry
IMA: Updated IMA policy functions to return keyrings option read from
the policy
IMA: Measure key if the IMA policy allows measurement for the keyring
to which the key is linked to
IMA: Defined an IMA hook to measure keys on key create or update
KEYS: Call the IMA hook to measure key when a new key is created or an
existing key is updated
IMA: Added a flag to determine whether IMA hook can process the key
now or has to queue for processing later
IMA: Defined functions to queue and dequeue keys for measurement
IMA: Call queue and dequeue functions to measure keys.
Documentation/ABI/testing/ima_policy | 16 +-
include/linux/ima.h | 13 ++
security/integrity/ima/Kconfig | 14 ++
security/integrity/ima/Makefile | 1 +
security/integrity/ima/ima.h | 32 ++-
security/integrity/ima/ima_api.c | 8 +-
security/integrity/ima/ima_appraise.c | 4 +-
security/integrity/ima/ima_asymmetric_keys.c | 201 +++++++++++++++++++
security/integrity/ima/ima_main.c | 31 ++-
security/integrity/ima/ima_policy.c | 53 ++++-
security/keys/key.c | 9 +
11 files changed, 366 insertions(+), 16 deletions(-)
create mode 100644 security/integrity/ima/ima_asymmetric_keys.c
--
2.17.1
^ permalink raw reply
* [PATCH v5 02/10] IMA: Added keyrings= option in IMA policy to only measure keys added to the specified keyrings.
From: Lakshmi Ramasubramanian @ 2019-11-11 19:32 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191111193303.12781-1-nramas@linux.microsoft.com>
IMA policy needs to support measuring only those keys linked to
a specific set of keyrings.
This patch defines a new IMA policy option namely "keyrings=" that
can be used to specify a set of keyrings. If this option is specified
in the policy for func=KEYRING_CHECK then only the keys linked to
the keyrings given in "keyrings=" option are measured.
If "keyrings=" option is not specified for func=KEYRING_CHECK then
all keys are measured.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
Documentation/ABI/testing/ima_policy | 10 +++++++++-
security/integrity/ima/ima_policy.c | 2 ++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 341df49b5ad1..be2874fa3928 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -25,7 +25,7 @@ Description:
lsm: [[subj_user=] [subj_role=] [subj_type=]
[obj_user=] [obj_role=] [obj_type=]]
option: [[appraise_type=]] [template=] [permit_directio]
- [appraise_flag=]
+ [appraise_flag=] [keyrings=]
base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
[FIRMWARE_CHECK]
[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
@@ -43,6 +43,9 @@ Description:
appraise_flag:= [check_blacklist]
Currently, blacklist check is only for files signed with appended
signature.
+ keyrings:= list of keyrings
+ (eg, .builtin_trusted_keys|.ima). Only valid
+ when action is "measure" and func is KEYRING_CHECK.
template:= name of a defined IMA template type
(eg, ima-ng). Only valid when action is "measure".
pcr:= decimal value
@@ -119,3 +122,8 @@ Description:
all keys:
measure func=KEYRING_CHECK
+
+ Example of measure rule using KEYRING_CHECK to only measure
+ keys added to .builtin_trusted_keys or .ima keyring:
+
+ measure func=KEYRING_CHECK keyrings=.builtin_trusted_keys|.ima
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 9ca32ffaaa9d..a0f7ffa80736 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -34,6 +34,7 @@
#define IMA_EUID 0x0080
#define IMA_PCR 0x0100
#define IMA_FSNAME 0x0200
+#define IMA_KEYRINGS 0x0400
#define UNKNOWN 0
#define MEASURE 0x0001 /* same as IMA_MEASURE */
@@ -79,6 +80,7 @@ struct ima_rule_entry {
int type; /* audit type */
} lsm[MAX_LSM_RULES];
char *fsname;
+ char *keyrings; /* Measure keys added to these keyrings */
struct ima_template_desc *template;
};
--
2.17.1
^ permalink raw reply related
* [PATCH v5 05/10] IMA: Measure key if the IMA policy allows measurement for the keyring to which the key is linked to
From: Lakshmi Ramasubramanian @ 2019-11-11 19:32 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191111193303.12781-1-nramas@linux.microsoft.com>
process_buffer_measurement() needs to check if the keyring to which
the given key is linked to is listed in the keyrings option in
the IMA policy.
This patch adds a new parameter "keyring" to process_buffer_measurement().
If process_buffer_measurement() is called with func KEYRING_CHECK and
the name of the keyring to which the key is linked to, then the given
key is measured if:
1, IMA policy did not specify "keyrings=" option.
2, Or, the given keyring name is listed in the "keyrings=" option.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
security/integrity/ima/ima.h | 2 +-
security/integrity/ima/ima_appraise.c | 2 +-
security/integrity/ima/ima_main.c | 26 ++++++++++++++++++++++++--
3 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 387829afb9a2..f15199f7ff2a 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -221,7 +221,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
struct ima_template_desc *template_desc);
void process_buffer_measurement(const void *buf, int size,
const char *eventname, enum ima_hooks func,
- int pcr);
+ int pcr, const char *keyring);
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_appraise.c b/security/integrity/ima/ima_appraise.c
index 47ad4f56c0a8..a9649b04b9f1 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -330,7 +330,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
process_buffer_measurement(digest, digestsize,
"blacklisted-hash", NONE,
- pcr);
+ pcr, NULL);
}
return rc;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 68e15ff1fe8d..1bd5dbd8a077 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -632,12 +632,22 @@ int ima_load_data(enum kernel_load_data_id id)
* @eventname: event name to be used for the buffer entry.
* @func: IMA hook
* @pcr: pcr to extend the measurement
+ * @keyring: keyring for the measurement
+ *
+ * The following scenarios are possible with respect to
+ * the parameter "keyring":
+ * 1, keyring is NULL. In this case buffer is measured.
+ * 2, keyring is not NULL, but ima_get_action returned
+ * a NULL keyrings. In this case also the buffer is measured.
+ * 3, keyring is not NULL and ima_get_action returned
+ * a non-NULL keyrings. In this case measure the buffer
+ * only if the given keyring is present in the keyrings.
*
* Based on policy, the buffer is measured into the ima log.
*/
void process_buffer_measurement(const void *buf, int size,
const char *eventname, enum ima_hooks func,
- int pcr)
+ int pcr, const char *keyring)
{
int ret = 0;
struct ima_template_entry *entry = NULL;
@@ -656,6 +666,13 @@ void process_buffer_measurement(const void *buf, int size,
int action = 0;
u32 secid;
+ /*
+ * If IMA is not yet initialized or IMA policy is empty
+ * then there is no need to measure.
+ */
+ if (!ima_policy_flag)
+ return;
+
/*
* Both LSM hooks and auxilary based buffer measurements are
* based on policy. To avoid code duplication, differentiate
@@ -671,6 +688,11 @@ void process_buffer_measurement(const void *buf, int size,
return;
}
+ if ((keyring != NULL) && (keyrings != NULL)
+ && (strstr(keyrings, keyring) == NULL)) {
+ return;
+ }
+
if (!pcr)
pcr = CONFIG_IMA_MEASURE_PCR_IDX;
@@ -719,7 +741,7 @@ void ima_kexec_cmdline(const void *buf, int size)
{
if (buf && size != 0)
process_buffer_measurement(buf, size, "kexec-cmdline",
- KEXEC_CMDLINE, 0);
+ KEXEC_CMDLINE, 0, NULL);
}
static int __init init_ima(void)
--
2.17.1
^ permalink raw reply related
* [PATCH v5 06/10] IMA: Defined an IMA hook to measure keys on key create or update
From: Lakshmi Ramasubramanian @ 2019-11-11 19:32 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191111193303.12781-1-nramas@linux.microsoft.com>
Asymmetric keys used for verifying file signatures or certificates
are currently not included in the IMA measurement list.
This patch defines a new IMA hook namely ima_post_key_create_or_update()
to measure asymmetric keys.
The IMA hook is defined in a new file namely ima_asymmetric_keys.c
Note that currently IMA subsystem can be enabled without
enabling KEYS subsystem.
Adding support for measuring asymmetric keys in IMA requires KEYS
subsystem to be enabled. To handle this dependency a new config
namely CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS has been added. Enabling
this config requires the following configs to be enabled:
CONFIG_IMA, CONFIG_KEYS, CONFIG_ASYMMETRIC_KEY_TYPE, and
CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE.
The new file ima_asymmetric_keys.c is built only if
CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS is enabled.
This config is turned off by default.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
security/integrity/ima/Kconfig | 14 +++++++
security/integrity/ima/Makefile | 1 +
security/integrity/ima/ima_asymmetric_keys.c | 44 ++++++++++++++++++++
3 files changed, 59 insertions(+)
create mode 100644 security/integrity/ima/ima_asymmetric_keys.c
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 838476d780e5..c6d14884bc19 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -310,3 +310,17 @@ config IMA_APPRAISE_SIGNED_INIT
default n
help
This option requires user-space init to be signed.
+
+config IMA_MEASURE_ASYMMETRIC_KEYS
+ bool "Enable measuring asymmetric keys on key create or update"
+ depends on IMA
+ depends on KEYS
+ depends on ASYMMETRIC_KEY_TYPE
+ depends on ASYMMETRIC_PUBLIC_KEY_SUBTYPE
+ default n
+ help
+ This option enables measuring asymmetric keys when
+ the key is created or updated. Additionally, IMA policy
+ needs to be configured to either measure keys linked to
+ any keyring or only measure keys linked to the keyrings
+ specified in the IMA policy through the keyrings= option.
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index 31d57cdf2421..3e9d0ad68c7b 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -12,3 +12,4 @@ ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o
ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
+obj-$(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) += ima_asymmetric_keys.o
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
new file mode 100644
index 000000000000..7d6603bfcc06
--- /dev/null
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Microsoft Corporation
+ *
+ * Author: Lakshmi Ramasubramanian (nramas@linux.microsoft.com)
+ *
+ * File: ima_asymmetric_keys.c
+ * Defines an IMA hook to measure asymmetric keys on key
+ * create or update.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <crypto/public_key.h>
+#include <keys/asymmetric-type.h>
+#include "ima.h"
+
+/**
+ * ima_post_key_create_or_update - measure asymmetric keys
+ * @keyring: keyring to which the key is linked to
+ * @key: created or updated key
+ * @flags: key flags
+ * @create: flag indicating whether the key was created or updated
+ *
+ * Keys can only be measured, not appraised.
+ */
+void ima_post_key_create_or_update(struct key *keyring, struct key *key,
+ unsigned long flags, bool create)
+{
+ const struct public_key *pk;
+
+ /* Only asymmetric keys are handled */
+ if (key->type != &key_type_asymmetric)
+ return;
+
+ /*
+ * Get the public_key of the given asymmetric key to measure.
+ */
+ pk = key->payload.data[asym_crypto];
+ process_buffer_measurement(pk->key, pk->keylen,
+ keyring->description,
+ KEYRING_CHECK, 0,
+ keyring->description);
+}
--
2.17.1
^ permalink raw reply related
* [PATCH v5 04/10] IMA: Updated IMA policy functions to return keyrings option read from the policy
From: Lakshmi Ramasubramanian @ 2019-11-11 19:32 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191111193303.12781-1-nramas@linux.microsoft.com>
keyrings option read from the IMA policy needs to be provided to
the callers that determine the action to be performed.
This patch updates ima_get_action() and ima_match_policy() functions
to return the keyrings option specified in the IMA policy.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
security/integrity/ima/ima.h | 6 ++++--
security/integrity/ima/ima_api.c | 8 +++++---
security/integrity/ima/ima_appraise.c | 2 +-
security/integrity/ima/ima_main.c | 5 +++--
security/integrity/ima/ima_policy.c | 7 ++++++-
5 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 7f23405b2718..387829afb9a2 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -208,7 +208,8 @@ struct modsig;
/* LIM API function definitions */
int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
int mask, enum ima_hooks func, int *pcr,
- struct ima_template_desc **template_desc);
+ struct ima_template_desc **template_desc,
+ char **keyrings);
int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
int ima_collect_measurement(struct integrity_iint_cache *iint,
struct file *file, void *buf, loff_t size,
@@ -235,7 +236,8 @@ const char *ima_d_path(const struct path *path, char **pathbuf, char *filename);
/* IMA policy related functions */
int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
enum ima_hooks func, int mask, int flags, int *pcr,
- struct ima_template_desc **template_desc);
+ struct ima_template_desc **template_desc,
+ char **keyrings);
void ima_init_policy(void);
void ima_update_policy(void);
void ima_update_policy_flag(void);
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 610759fe63b8..fa2cd71ddf1a 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -169,12 +169,13 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
* @func: caller identifier
* @pcr: pointer filled in if matched measure policy sets pcr=
* @template_desc: pointer filled in if matched measure policy sets template=
+ * @keyrings: pointer filled in if matched measure policy sets keyrings=
*
* 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
- * | KEXEC_CMDLINE
+ * | KEXEC_CMDLINE | KEYRING_CHECK
* mask: contains the permission mask
* fsmagic: hex value
*
@@ -183,14 +184,15 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
*/
int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
int mask, enum ima_hooks func, int *pcr,
- struct ima_template_desc **template_desc)
+ struct ima_template_desc **template_desc,
+ char **keyrings)
{
int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE | IMA_HASH;
flags &= ima_policy_flag;
return ima_match_policy(inode, cred, secid, func, mask, flags, pcr,
- template_desc);
+ template_desc, keyrings);
}
/*
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 300c8d2943c5..47ad4f56c0a8 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -55,7 +55,7 @@ int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func)
security_task_getsecid(current, &secid);
return ima_match_policy(inode, current_cred(), secid, func, mask,
- IMA_APPRAISE | IMA_HASH, NULL, NULL);
+ IMA_APPRAISE | IMA_HASH, NULL, NULL, NULL);
}
static int ima_fix_xattr(struct dentry *dentry,
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index d7e987baf127..68e15ff1fe8d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -215,7 +215,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
* Included is the appraise submask.
*/
action = ima_get_action(inode, cred, secid, mask, func, &pcr,
- &template_desc);
+ &template_desc, NULL);
violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
(ima_policy_flag & IMA_MEASURE));
if (!action && !violation_check)
@@ -647,6 +647,7 @@ void process_buffer_measurement(const void *buf, int size,
.buf = buf,
.buf_len = size};
struct ima_template_desc *template = NULL;
+ char *keyrings = NULL;
struct {
struct ima_digest_data hdr;
char digest[IMA_MAX_DIGEST_SIZE];
@@ -665,7 +666,7 @@ void process_buffer_measurement(const void *buf, int size,
if (func) {
security_task_getsecid(current, &secid);
action = ima_get_action(NULL, current_cred(), secid, 0, func,
- &pcr, &template);
+ &pcr, &template, &keyrings);
if (!(action & IMA_MEASURE))
return;
}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 1aee3c8b9cf6..d1889eee9287 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -481,6 +481,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
* @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC)
* @pcr: set the pcr to extend
* @template_desc: the template that should be used for this rule
+ * @keyrings: set the keyrings for this rule, if specified
*
* Measure decision based on func/mask/fsmagic and LSM(subj/obj/type)
* conditions.
@@ -491,7 +492,8 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
*/
int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
enum ima_hooks func, int mask, int flags, int *pcr,
- struct ima_template_desc **template_desc)
+ struct ima_template_desc **template_desc,
+ char **keyrings)
{
struct ima_rule_entry *entry;
int action = 0, actmask = flags | (flags << 1);
@@ -527,6 +529,9 @@ int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
if ((pcr) && (entry->flags & IMA_PCR))
*pcr = entry->pcr;
+ if ((keyrings) && (entry->flags & IMA_KEYRINGS))
+ *keyrings = entry->keyrings;
+
if (template_desc && entry->template)
*template_desc = entry->template;
--
2.17.1
^ permalink raw reply related
* [PATCH v5 09/10] IMA: Defined functions to queue and dequeue keys for measurement
From: Lakshmi Ramasubramanian @ 2019-11-11 19:33 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191111193303.12781-1-nramas@linux.microsoft.com>
A key can be measured right away only if custom IMA policies
have been applied. Otherwise, the key should be queued up and
processed when custom IMA policies have been applied.
This patch defines functions to queue and dequeue keys for
measurement.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
security/integrity/ima/ima.h | 23 ++++
security/integrity/ima/ima_asymmetric_keys.c | 128 +++++++++++++++++++
2 files changed, 151 insertions(+)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index f15199f7ff2a..4e7fed8d224e 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -205,6 +205,29 @@ extern const char *const func_tokens[];
struct modsig;
+#ifdef CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
+/*
+ * To track keys that need to be measured.
+ */
+struct ima_measure_key_entry {
+ struct list_head list;
+ void *public_key;
+ u32 public_key_len;
+ char *keyring_name;
+};
+
+bool ima_queue_key_for_measurement(struct key *keyring,
+ struct key *key);
+void ima_process_queued_keys_for_measurement(void);
+#else
+static inline bool ima_queue_key_for_measurement(struct key *keyring,
+ struct key *key)
+{
+ return false;
+}
+static inline void ima_process_queued_keys_for_measurement(void) {}
+#endif /* CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS */
+
/* LIM API function definitions */
int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
int mask, enum ima_hooks func, int *pcr,
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index 61c42d06a636..4a38b4957b8c 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -7,6 +7,7 @@
* File: ima_asymmetric_keys.c
* Defines an IMA hook to measure asymmetric keys on key
* create or update.
+ * Queue and de-queue functions for measuring asymmetric keys.
*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -17,6 +18,133 @@
bool ima_process_keys_for_measurement;
+/*
+ * To synchronize access to the list of keys that need to be measured
+ */
+static DEFINE_MUTEX(ima_measure_keys_mutex);
+static LIST_HEAD(ima_measure_keys);
+
+static void ima_free_measure_key_entry(struct ima_measure_key_entry *entry)
+{
+ if (entry != NULL) {
+ if (entry->public_key != NULL)
+ kzfree(entry->public_key);
+ if (entry->keyring_name != NULL)
+ kzfree(entry->keyring_name);
+ kzfree(entry);
+ }
+}
+
+static struct ima_measure_key_entry *ima_alloc_measure_key_entry(
+ struct key *keyring,
+ struct key *key)
+{
+ int rc = 0;
+ const struct public_key *pk;
+ size_t keyring_name_len;
+ struct ima_measure_key_entry *entry = NULL;
+
+ pk = key->payload.data[asym_crypto];
+ keyring_name_len = strlen(keyring->description) + 1;
+ entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+ if (entry != NULL) {
+ entry->public_key = kzalloc(pk->keylen, GFP_KERNEL);
+ entry->keyring_name =
+ kzalloc(keyring_name_len, GFP_KERNEL);
+ }
+
+ if ((entry == NULL) || (entry->public_key == NULL) ||
+ (entry->keyring_name == NULL)) {
+ rc = -ENOMEM;
+ goto out;
+ }
+
+ strcpy(entry->keyring_name, keyring->description);
+ memcpy(entry->public_key, pk->key, pk->keylen);
+ entry->public_key_len = pk->keylen;
+ rc = 0;
+
+out:
+ if (rc) {
+ ima_free_measure_key_entry(entry);
+ entry = NULL;
+ }
+
+ return entry;
+}
+
+bool ima_queue_key_for_measurement(struct key *keyring,
+ struct key *key)
+{
+ bool queued = false;
+ struct ima_measure_key_entry *entry = NULL;
+
+ /*
+ * ima_measure_keys_mutex should be taken before checking
+ * ima_process_keys_for_measurement flag to avoid the race
+ * condition between the IMA hook checking this flag and
+ * calling ima_queue_key_for_measurement() to queue the key and
+ * ima_process_queued_keys_for_measurement() setting this flag.
+ */
+ mutex_lock(&ima_measure_keys_mutex);
+
+ if (!ima_process_keys_for_measurement) {
+ entry = ima_alloc_measure_key_entry(keyring, key);
+ if (entry != NULL) {
+ INIT_LIST_HEAD(&entry->list);
+ list_add_tail(&entry->list, &ima_measure_keys);
+ queued = true;
+ }
+ }
+
+ mutex_unlock(&ima_measure_keys_mutex);
+
+ return queued;
+}
+
+void ima_process_queued_keys_for_measurement(void)
+{
+ struct ima_measure_key_entry *entry, *tmp;
+ LIST_HEAD(temp_ima_measure_keys);
+
+ if (ima_process_keys_for_measurement)
+ return;
+
+ /*
+ * Any queued keys will be processed now. From here on
+ * keys should be processed right away.
+ */
+ ima_process_keys_for_measurement = true;
+
+ /*
+ * To avoid holding the mutex when processing queued keys,
+ * transfer the queued keys with the mutex held to a temp list,
+ * release the mutex, and then process the queued keys from
+ * the temp list.
+ *
+ * Since ima_process_keys_for_measurement is set to true above,
+ * any new key will be processed immediately and not be queued.
+ */
+ INIT_LIST_HEAD(&temp_ima_measure_keys);
+ mutex_lock(&ima_measure_keys_mutex);
+ list_for_each_entry_safe(entry, tmp, &ima_measure_keys, list) {
+ list_del(&entry->list);
+ list_add_tail(&entry->list, &temp_ima_measure_keys);
+ }
+ mutex_unlock(&ima_measure_keys_mutex);
+
+ list_for_each_entry_safe(entry, tmp,
+ &temp_ima_measure_keys, list) {
+ process_buffer_measurement(entry->public_key,
+ entry->public_key_len,
+ entry->keyring_name,
+ KEYRING_CHECK, 0,
+ entry->keyring_name);
+ list_del(&entry->list);
+ ima_free_measure_key_entry(entry);
+ }
+}
+
/**
* ima_post_key_create_or_update - measure asymmetric keys
* @keyring: keyring to which the key is linked to
--
2.17.1
^ permalink raw reply related
* [PATCH v5 07/10] KEYS: Call the IMA hook to measure key when a new key is created or an existing key is updated
From: Lakshmi Ramasubramanian @ 2019-11-11 19:33 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191111193303.12781-1-nramas@linux.microsoft.com>
key_create_or_update function needs to call the IMA hook to measure
the key when a new key is created or an existing key is updated.
This patch adds the call to the IMA hook from key_create_or_update
function.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
include/linux/ima.h | 13 +++++++++++++
security/keys/key.c | 9 +++++++++
2 files changed, 22 insertions(+)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 6d904754d858..ec5afe319ab7 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -25,6 +25,12 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
extern void ima_post_path_mknod(struct dentry *dentry);
extern void ima_kexec_cmdline(const void *buf, int size);
+#ifdef CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
+extern void ima_post_key_create_or_update(struct key *keyring,
+ struct key *key,
+ unsigned long flags, bool create);
+#endif
+
#ifdef CONFIG_IMA_KEXEC
extern void ima_add_kexec_buffer(struct kimage *image);
#endif
@@ -101,6 +107,13 @@ static inline void ima_add_kexec_buffer(struct kimage *image)
{}
#endif
+#ifndef CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
+static inline void ima_post_key_create_or_update(struct key *keyring,
+ struct key *key,
+ unsigned long flags,
+ bool create) {}
+#endif
+
#ifdef CONFIG_IMA_APPRAISE
extern bool is_ima_appraise_enabled(void);
extern void ima_inode_post_setattr(struct dentry *dentry);
diff --git a/security/keys/key.c b/security/keys/key.c
index 764f4c57913e..9782d4d046fd 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -13,6 +13,7 @@
#include <linux/security.h>
#include <linux/workqueue.h>
#include <linux/random.h>
+#include <linux/ima.h>
#include <linux/err.h>
#include "internal.h"
@@ -936,6 +937,9 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
goto error_link_end;
}
+ /* let the ima module know about the created key. */
+ ima_post_key_create_or_update(keyring, key, flags, true);
+
key_ref = make_key_ref(key, is_key_possessed(keyring_ref));
error_link_end:
@@ -965,6 +969,11 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
}
key_ref = __key_update(key_ref, &prep);
+
+ /* let the ima module know about the updated key. */
+ if (!IS_ERR(key_ref))
+ ima_post_key_create_or_update(keyring, key, flags, false);
+
goto error_free_prep;
}
EXPORT_SYMBOL(key_create_or_update);
--
2.17.1
^ permalink raw reply related
* [PATCH v5 08/10] IMA: Added a flag to determine whether IMA hook can process the key now or has to queue for processing later
From: Lakshmi Ramasubramanian @ 2019-11-11 19:33 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191111193303.12781-1-nramas@linux.microsoft.com>
Keys should be processed only if custom IMA policies have been
applied. Prior to that the keys should be queued for processing later.
This patch defines a flag namely ima_process_keys_for_measurement
to check if the key should be processed immediately or should be queued.
ima_policy_flag cannot be relied upon because ima_policy_flag will
be set to 0 when either IMA is not initialized or the IMA policy
itself is empty.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
security/integrity/ima/ima_asymmetric_keys.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index 7d6603bfcc06..61c42d06a636 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -15,6 +15,8 @@
#include <keys/asymmetric-type.h>
#include "ima.h"
+bool ima_process_keys_for_measurement;
+
/**
* ima_post_key_create_or_update - measure asymmetric keys
* @keyring: keyring to which the key is linked to
--
2.17.1
^ permalink raw reply related
* [PATCH v5 10/10] IMA: Call queue and dequeue functions to measure keys.
From: Lakshmi Ramasubramanian @ 2019-11-11 19:33 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191111193303.12781-1-nramas@linux.microsoft.com>
Keys should be queued for measurement if custom IMA policies have
not been applied. Keys queued for measurement, if any, need to be
processed when custom IMA policies have been applied.
This patch adds the call to ima_queue_key_for_measurement in the IMA hook
function and the call to ima_process_queued_keys_for_measurement when
custom IMA policies have been applied.
NOTE:
If the kernel is built with CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
then the IMA policy for measuring keys should be applied as part of
custom IMA policies. Keys will be queued up until custom policies
are applied and processed when applied.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
security/integrity/ima/ima_asymmetric_keys.c | 31 ++++++++++++++++++--
security/integrity/ima/ima_policy.c | 12 ++++++++
2 files changed, 41 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index 4a38b4957b8c..fab1d4672715 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -158,15 +158,42 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
unsigned long flags, bool create)
{
const struct public_key *pk;
+ bool key_queued = false;
- /* Only asymmetric keys are handled */
+ /* Only asymmetric keys are handled by this hook. */
if (key->type != &key_type_asymmetric)
return;
+ if (!ima_process_keys_for_measurement)
+ key_queued = ima_queue_key_for_measurement(keyring, key);
+
/*
- * Get the public_key of the given asymmetric key to measure.
+ * Need to check again if the key was queued or not because
+ * ima_process_keys_for_measurement could have flipped from
+ * false to true after it was checked above, but before the key
+ * could be queued by ima_queue_key_for_measurement().
*/
+ if (key_queued)
+ return;
+
+ /* Public key of the given asymmetric key is measured. */
pk = key->payload.data[asym_crypto];
+
+ /*
+ * keyring->description points to the name of the keyring
+ * (such as ".builtin_trusted_keys", ".ima", etc.) to
+ * which the given key is linked to.
+ *
+ * The name of the keyring is passed in the "eventname"
+ * parameter to process_buffer_measurement() and is set
+ * in the "eventname" field in ima_event_data for
+ * the key measurement IMA event.
+ *
+ * The name of the keyring is also passed in the "keyring"
+ * parameter to process_buffer_measurement() to check
+ * if the IMA policy is configured to measure a key linked
+ * to the given keyring.
+ */
process_buffer_measurement(pk->key, pk->keylen,
keyring->description,
KEYRING_CHECK, 0,
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index d1889eee9287..d130bdcbc174 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -759,6 +759,18 @@ void ima_update_policy(void)
kfree(arch_policy_entry);
}
ima_update_policy_flag();
+
+ /*
+ * Custom IMA policies have been setup.
+ * Process key(s) queued up for measurement now.
+ *
+ * NOTE:
+ * Custom IMA policies always overwrite builtin policies
+ * (policies compiled in code). If one wants measurement
+ * of asymmetric keys then it has to be configured in
+ * custom policies and updated here.
+ */
+ ima_process_queued_keys_for_measurement();
}
/* Keep the enumeration in sync with the policy_tokens! */
--
2.17.1
^ permalink raw reply related
* [PATCH v5 01/10] IMA: Added KEYRING_CHECK func in IMA policy to measure keys
From: Lakshmi Ramasubramanian @ 2019-11-11 19:32 UTC (permalink / raw)
To: zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <20191111193303.12781-1-nramas@linux.microsoft.com>
IMA policy needs to support a func to enable measurement of
asymmetric keys.
This patch defines a new IMA policy func namely KEYRING_CHECK to
measure asymmetric keys.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
Documentation/ABI/testing/ima_policy | 6 ++++++
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_policy.c | 4 +++-
3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 29aaedf33246..341df49b5ad1 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -30,6 +30,7 @@ Description:
[FIRMWARE_CHECK]
[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
[KEXEC_CMDLINE]
+ [KEYRING_CHECK]
mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
[[^]MAY_EXEC]
fsmagic:= hex value
@@ -113,3 +114,8 @@ Description:
Example of appraise rule allowing modsig appended signatures:
appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig
+
+ Example of measure rule using KEYRING_CHECK to measure
+ all keys:
+
+ measure func=KEYRING_CHECK
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index df4ca482fb53..7f23405b2718 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -193,6 +193,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
hook(KEXEC_INITRAMFS_CHECK) \
hook(POLICY_CHECK) \
hook(KEXEC_CMDLINE) \
+ hook(KEYRING_CHECK) \
hook(MAX_CHECK)
#define __ima_hook_enumify(ENUM) ENUM,
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index f19a895ad7cd..9ca32ffaaa9d 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -373,7 +373,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
{
int i;
- if (func == KEXEC_CMDLINE) {
+ if ((func == KEXEC_CMDLINE) || (func == KEYRING_CHECK)) {
if ((rule->flags & IMA_FUNC) && (rule->func == func))
return true;
return false;
@@ -997,6 +997,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
entry->func = POLICY_CHECK;
else if (strcmp(args[0].from, "KEXEC_CMDLINE") == 0)
entry->func = KEXEC_CMDLINE;
+ else if (strcmp(args[0].from, "KEYRING_CHECK") == 0)
+ entry->func = KEYRING_CHECK;
else
result = -EINVAL;
if (!result)
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] x86/mtrr: only administrator can read the configurations.
From: Kees Cook @ 2019-11-11 17:56 UTC (permalink / raw)
To: Borislav Petkov
Cc: Zhang Xiaoxu, zhangxiaoxu, mingo, hpa, x86, tyhicks, colin.king,
tglx, linux-kernel, linux-security-module, Matthew Garrett
In-Reply-To: <20191108213307.GI4503@zn.tnic>
[this wasn't being discussed on a list... CCing lkml]
On Fri, Nov 08, 2019 at 10:33:07PM +0100, Borislav Petkov wrote:
> On Fri, Nov 08, 2019 at 01:22:50PM -0800, Kees Cook wrote:
> > The correct pattern for these kinds of things is to do the checks at
> > open time, yes. (Which is why I perked up at this patch when I noticed
> > it.)
>
> I would move it there but...
>
> > Well, I'm not entirely sure what the issue here is. I saw the patch also
> > changed the DAC permissions to 0600, so wouldn't that alone fix things?
> > But the capable checks moved around... is there an "unprivileged" use of
> > this file any more? If so, why keep at capable() checks and just use
> > DAC?
>
> ... yes, that would be even better because it would kill all the checks,
> so less code.
>
> How's that?
Some recap from being accidentally offlist:
- this patch should check capabilities at open time (or retain the
checks on the opener's permissions for later checks).
- changing the DAC permissions might break something that expects to
read mtrr when not uid 0.
- if we leave the DAC permissions alone and just move the capable check
to the opener, we should get the intent of the original patch. (i.e.
check against CAP_SYS_ADMIN not just the wider uid 0.)
- *this may still break things* if userspace expects to be able to
read other parts of the file as non-uid-0 and non-CAP_SYS_ADMIN.
If *that* is the case, then we need to censor the contents using
the opener's permissions (as done in other /proc cases).
I think the most cautious way forward is something like
51d7b120418e ("/proc/iomem: only expose physical resource addresses to
privileged users"). Untested (and should likely be expanded to know
about read vs write for lockdown interaction):
diff --git a/arch/x86/kernel/cpu/mtrr/if.c b/arch/x86/kernel/cpu/mtrr/if.c
index 4d36dcc1cf87..7ccc3e290338 100644
--- a/arch/x86/kernel/cpu/mtrr/if.c
+++ b/arch/x86/kernel/cpu/mtrr/if.c
@@ -34,6 +34,11 @@ const char *mtrr_attrib_to_str(int x)
#ifdef CONFIG_PROC_FS
+static bool has_mtrr_privs(struct file *file)
+{
+ return file_ns_capable(file, &init_user_ns, CAP_SYS_ADMIN);
+}
+
static int
mtrr_file_add(unsigned long base, unsigned long size,
unsigned int type, bool increment, struct file *file, int page)
@@ -101,7 +106,7 @@ mtrr_write(struct file *file, const char __user *buf, size_t len, loff_t * ppos)
int length;
size_t linelen;
- if (!capable(CAP_SYS_ADMIN))
+ if (!has_mtrr_privs(file))
return -EPERM;
memset(line, 0, LINE_SIZE);
@@ -226,7 +231,7 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
#ifdef CONFIG_COMPAT
case MTRRIOC32_ADD_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
+ if (!has_mtrr_privs(file))
return -EPERM;
err =
mtrr_file_add(sentry.base, sentry.size, sentry.type, true,
@@ -236,7 +241,7 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
#ifdef CONFIG_COMPAT
case MTRRIOC32_SET_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
+ if (!has_mtrr_privs(file))
return -EPERM;
err = mtrr_add(sentry.base, sentry.size, sentry.type, false);
break;
@@ -244,7 +249,7 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
#ifdef CONFIG_COMPAT
case MTRRIOC32_DEL_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
+ if (!has_mtrr_privs(file))
return -EPERM;
err = mtrr_file_del(sentry.base, sentry.size, file, 0);
break;
@@ -252,7 +257,7 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
#ifdef CONFIG_COMPAT
case MTRRIOC32_KILL_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
+ if (!has_mtrr_privs(file))
return -EPERM;
err = mtrr_del(-1, sentry.base, sentry.size);
break;
@@ -279,7 +284,7 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
#ifdef CONFIG_COMPAT
case MTRRIOC32_ADD_PAGE_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
+ if (!has_mtrr_privs(file))
return -EPERM;
err =
mtrr_file_add(sentry.base, sentry.size, sentry.type, true,
@@ -289,7 +294,7 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
#ifdef CONFIG_COMPAT
case MTRRIOC32_SET_PAGE_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
+ if (!has_mtrr_privs(file))
return -EPERM;
err =
mtrr_add_page(sentry.base, sentry.size, sentry.type, false);
@@ -298,7 +303,7 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
#ifdef CONFIG_COMPAT
case MTRRIOC32_DEL_PAGE_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
+ if (!has_mtrr_privs(file))
return -EPERM;
err = mtrr_file_del(sentry.base, sentry.size, file, 1);
break;
@@ -306,7 +311,7 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
#ifdef CONFIG_COMPAT
case MTRRIOC32_KILL_PAGE_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
+ if (!has_mtrr_privs(file))
return -EPERM;
err = mtrr_del_page(-1, sentry.base, sentry.size);
break;
@@ -401,6 +406,7 @@ static int mtrr_seq_show(struct seq_file *seq, void *offset)
int i, max;
mtrr_type type;
unsigned long base, size;
+ int usage;
max = num_var_ranges;
for (i = 0; i < max; i++) {
@@ -409,6 +415,15 @@ static int mtrr_seq_show(struct seq_file *seq, void *offset)
mtrr_usage_table[i] = 0;
continue;
}
+ usage = mtrr_usage_table[i];
+ type_str = mtrr_attrib_to_str(type);
+
+ if (!has_mtrr_privs(seq->file)) {
+ base = 0;
+ size = 0;
+ usage = 0;
+ type_str = "?";
+ }
if (size < (0x100000 >> PAGE_SHIFT)) {
/* less than 1MB */
factor = 'K';
@@ -420,8 +435,7 @@ static int mtrr_seq_show(struct seq_file *seq, void *offset)
/* Base can be > 32bit */
seq_printf(seq, "reg%02i: base=0x%06lx000 (%5luMB), size=%5lu%cB, count=%d: %s\n",
i, base, base >> (20 - PAGE_SHIFT),
- size, factor,
- mtrr_usage_table[i], mtrr_attrib_to_str(type));
+ size, factor, usage, type_str);
}
return 0;
}
If we want to risk breaking stuff, here is the "just check capable at open time" patch:
diff --git a/arch/x86/kernel/cpu/mtrr/if.c b/arch/x86/kernel/cpu/mtrr/if.c
index 4d36dcc1cf87..a65e5c6686d0 100644
--- a/arch/x86/kernel/cpu/mtrr/if.c
+++ b/arch/x86/kernel/cpu/mtrr/if.c
@@ -101,9 +101,6 @@ mtrr_write(struct file *file, const char __user *buf, size_t len, loff_t * ppos)
int length;
size_t linelen;
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
-
memset(line, 0, LINE_SIZE);
len = min_t(size_t, len, LINE_SIZE - 1);
@@ -226,8 +223,6 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
#ifdef CONFIG_COMPAT
case MTRRIOC32_ADD_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
err =
mtrr_file_add(sentry.base, sentry.size, sentry.type, true,
file, 0);
@@ -236,24 +231,18 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
#ifdef CONFIG_COMPAT
case MTRRIOC32_SET_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
err = mtrr_add(sentry.base, sentry.size, sentry.type, false);
break;
case MTRRIOC_DEL_ENTRY:
#ifdef CONFIG_COMPAT
case MTRRIOC32_DEL_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
err = mtrr_file_del(sentry.base, sentry.size, file, 0);
break;
case MTRRIOC_KILL_ENTRY:
#ifdef CONFIG_COMPAT
case MTRRIOC32_KILL_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
err = mtrr_del(-1, sentry.base, sentry.size);
break;
case MTRRIOC_GET_ENTRY:
@@ -279,8 +268,6 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
#ifdef CONFIG_COMPAT
case MTRRIOC32_ADD_PAGE_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
err =
mtrr_file_add(sentry.base, sentry.size, sentry.type, true,
file, 1);
@@ -289,8 +276,6 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
#ifdef CONFIG_COMPAT
case MTRRIOC32_SET_PAGE_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
err =
mtrr_add_page(sentry.base, sentry.size, sentry.type, false);
break;
@@ -298,16 +283,12 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
#ifdef CONFIG_COMPAT
case MTRRIOC32_DEL_PAGE_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
err = mtrr_file_del(sentry.base, sentry.size, file, 1);
break;
case MTRRIOC_KILL_PAGE_ENTRY:
#ifdef CONFIG_COMPAT
case MTRRIOC32_KILL_PAGE_ENTRY:
#endif
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
err = mtrr_del_page(-1, sentry.base, sentry.size);
break;
case MTRRIOC_GET_PAGE_ENTRY:
@@ -381,6 +362,9 @@ static int mtrr_open(struct inode *inode, struct file *file)
return -EIO;
if (!mtrr_if->get)
return -ENXIO;
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
return single_open(file, mtrr_seq_show, NULL);
}
Thoughts?
-Kees
>
> ---
> From: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
> Date: Tue, 5 Nov 2019 15:17:14 +0800
> Subject: [PATCH] x86/mtrr: Restrict MTRR ranges dumping and ioctl()
>
> /proc/mtrr dumps the physical memory ranges of the variable range MTRRs
> along with their respective sizes and caching attributes. Since that
> file is world-readable, it presents a small information leak about the
> physical address ranges of a system which should be blocked.
>
> Make that file root-only and get rid of all the capability checks as
> they're not needed anymore.
>
> [ bp: rewrite commit message. ]
>
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Colin Ian King <colin.king@canonical.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Tyler Hicks <tyhicks@canonical.com>
> Cc: x86-ml <x86@kernel.org>
> Cc: zhangxiaoxu@huawei.com
> Link: https://lkml.kernel.org/r/20191105071714.27376-1-zhangxiaoxu5@huawei.com
> ---
> arch/x86/kernel/cpu/mtrr/if.c | 18 +-----------------
> 1 file changed, 1 insertion(+), 17 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mtrr/if.c b/arch/x86/kernel/cpu/mtrr/if.c
> index 4d36dcc1cf87..7ff865f2b150 100644
> --- a/arch/x86/kernel/cpu/mtrr/if.c
> +++ b/arch/x86/kernel/cpu/mtrr/if.c
> @@ -226,8 +226,6 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
> #ifdef CONFIG_COMPAT
> case MTRRIOC32_ADD_ENTRY:
> #endif
> - if (!capable(CAP_SYS_ADMIN))
> - return -EPERM;
> err =
> mtrr_file_add(sentry.base, sentry.size, sentry.type, true,
> file, 0);
> @@ -236,24 +234,18 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
> #ifdef CONFIG_COMPAT
> case MTRRIOC32_SET_ENTRY:
> #endif
> - if (!capable(CAP_SYS_ADMIN))
> - return -EPERM;
> err = mtrr_add(sentry.base, sentry.size, sentry.type, false);
> break;
> case MTRRIOC_DEL_ENTRY:
> #ifdef CONFIG_COMPAT
> case MTRRIOC32_DEL_ENTRY:
> #endif
> - if (!capable(CAP_SYS_ADMIN))
> - return -EPERM;
> err = mtrr_file_del(sentry.base, sentry.size, file, 0);
> break;
> case MTRRIOC_KILL_ENTRY:
> #ifdef CONFIG_COMPAT
> case MTRRIOC32_KILL_ENTRY:
> #endif
> - if (!capable(CAP_SYS_ADMIN))
> - return -EPERM;
> err = mtrr_del(-1, sentry.base, sentry.size);
> break;
> case MTRRIOC_GET_ENTRY:
> @@ -279,8 +271,6 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
> #ifdef CONFIG_COMPAT
> case MTRRIOC32_ADD_PAGE_ENTRY:
> #endif
> - if (!capable(CAP_SYS_ADMIN))
> - return -EPERM;
> err =
> mtrr_file_add(sentry.base, sentry.size, sentry.type, true,
> file, 1);
> @@ -289,8 +279,6 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
> #ifdef CONFIG_COMPAT
> case MTRRIOC32_SET_PAGE_ENTRY:
> #endif
> - if (!capable(CAP_SYS_ADMIN))
> - return -EPERM;
> err =
> mtrr_add_page(sentry.base, sentry.size, sentry.type, false);
> break;
> @@ -298,16 +286,12 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
> #ifdef CONFIG_COMPAT
> case MTRRIOC32_DEL_PAGE_ENTRY:
> #endif
> - if (!capable(CAP_SYS_ADMIN))
> - return -EPERM;
> err = mtrr_file_del(sentry.base, sentry.size, file, 1);
> break;
> case MTRRIOC_KILL_PAGE_ENTRY:
> #ifdef CONFIG_COMPAT
> case MTRRIOC32_KILL_PAGE_ENTRY:
> #endif
> - if (!capable(CAP_SYS_ADMIN))
> - return -EPERM;
> err = mtrr_del_page(-1, sentry.base, sentry.size);
> break;
> case MTRRIOC_GET_PAGE_ENTRY:
> @@ -436,7 +420,7 @@ static int __init mtrr_if_init(void)
> (!cpu_has(c, X86_FEATURE_CENTAUR_MCR)))
> return -ENODEV;
>
> - proc_create("mtrr", S_IWUSR | S_IRUGO, NULL, &mtrr_fops);
> + proc_create("mtrr", 0600, NULL, &mtrr_fops);
> return 0;
> }
> arch_initcall(mtrr_if_init);
> --
> 2.21.0
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
--
Kees Cook
^ permalink raw reply related
* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Mickaël Salaün @ 2019-11-08 15:39 UTC (permalink / raw)
To: Daniel Borkmann
Cc: KP Singh, Alexei Starovoitov, linux-kernel, Alexei Starovoitov,
Andy Lutomirski, Casey Schaufler, David Drysdale, Florent Revest,
James Morris, Jann Horn, John Johansen, Jonathan Corbet,
Kees Cook, Michael Kerrisk, Mickaël Salaün, Paul Moore,
Sargun Dhillon, Serge E . Hallyn, Shuah Khan, Stephen Smalley,
Tejun Heo, Tetsuo Handa, Tycho Andersen, Will Drewry, bpf,
kernel-hardening, linux-api, linux-security-module, netdev
In-Reply-To: <5d0f1dc5-5a99-bd6a-4acc-0cdcd062a0c9@iogearbox.net>
On 08/11/2019 15:34, Daniel Borkmann wrote:
> On 11/8/19 3:08 PM, Mickaël Salaün wrote:
>> On 06/11/2019 22:45, KP Singh wrote:
>>> On 06-Nov 17:55, Mickaël Salaün wrote:
>>>> On 06/11/2019 11:06, KP Singh wrote:
>>>>> On 05-Nov 11:34, Alexei Starovoitov wrote:
>>>>>> On Tue, Nov 05, 2019 at 07:01:41PM +0100, Mickaël Salaün wrote:
>>>>>>> On 05/11/2019 18:18, Alexei Starovoitov wrote:
> [...]
>>> * Use a single BPF program type; this is necessary for a key requirement
>>> of KRSI, i.e. runtime instrumentation. The upcoming prototype should
>>> illustrate how this works for KRSI - note that it’s possible to vary
>>> the context types exposed by different hooks.
>>
>> Why a single BPF program type? Do you mean *attach* types? Landlock only
>> use one program type, but will use multiple attach types.
>>
>> Why do you think it is necessary for KRSI or for runtime instrumentation?
>>
>> If it is justified, it could be a dedicated program attach type (e.g.
>> BPF_LANDLOCK_INTROSPECTION).
>>
>> What is the advantage to have the possibility to vary the context types
>> over dedicated *typed* contexts? I don't see any advantages, but at
>> least one main drawback: to require runtime checks (when helpers use
>> this generic context) instead of load time checks (thanks to static type
>> checking of the context).
>
> Lets take security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
> as one specific example here: the running kernel has its own internal
> btf_vmlinux and therefore a complete description of itself. From verifier
> side we can retrieve & introspect the security_sock_rcv_skb signatue
OK, this is indeed the signature defined by the kernel API. What happen
if this API change (e.g. if struct sock is replaced with a struct
sock_meta)?
> and
> thus know that the given BPF attachment point has struct sock and struct
> sk_buff as input arguments
How does the verifier know a given BPF attachment point for a program
without relying on its type or attach type? How and where is registered
this mapping?
To say it another way, if there is no way to differentiate two program
targeting different hook, I don't understand how the verifier could
check if a given program can legitimately call a helper which could read
the tracer and tracee fields (legitimate for a ptrace hook), whereas
this program may be attached to a sock_rcv_skb hook (and there is no way
to know that).
> which can then be accessed generically by the
> prog in order to allow sk_filter_trim_cap() to pass or to drop the skb.
> The same generic approach can be done for many of the other lsm hooks, so
> single program type would be enough there and context is derived
> automatically,
> no dedicated extra context per attach type would be needed and no runtime
> checks as you mentioned above since its still all asserted at verification
> time.
I mentioned runtime check because I though a helper should handle the
case when it doesn't make sense for a program attached to a specific
point/hook (e.g. ptrace) to use an input argument (e.g. sk) defined for
another point/hook (e.g. sock_rcv_skb).
>
> Thanks,
> Daniel
>
Thanks for this explanation Daniel.
^ permalink raw reply
* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: KP Singh @ 2019-11-08 15:27 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Alexei Starovoitov, linux-kernel, Alexei Starovoitov,
Andy Lutomirski, Casey Schaufler, Daniel Borkmann, David Drysdale,
Florent Revest, James Morris, Jann Horn, John Johansen,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Paul Moore, Sargun Dhillon,
Serge E . Hallyn, Shuah Khan, Stephen Smalley, Tejun Heo,
Tetsuo Handa, Tycho Andersen, Will Drewry, bpf, kernel-hardening,
linux-api, linux-security-module, netdev
In-Reply-To: <3e208632-e7ab-3405-5196-ab1d770e20c3@digikod.net>
On 08-Nov 15:08, Mickaël Salaün wrote:
>
> On 06/11/2019 22:45, KP Singh wrote:
> > On 06-Nov 17:55, Mickaël Salaün wrote:
> >>
> >> On 06/11/2019 11:06, KP Singh wrote:
> >>> On 05-Nov 11:34, Alexei Starovoitov wrote:
> >>>> On Tue, Nov 05, 2019 at 07:01:41PM +0100, Mickaël Salaün wrote:
> >>>>> On 05/11/2019 18:18, Alexei Starovoitov wrote:
> >>
> >> [...]
> >>
> >>>>>> I think the only way bpf-based LSM can land is both landlock and KRSI
> >>>>>> developers work together on a design that solves all use cases.
> >>>>>
> >>>>> As I said in a previous cover letter [1], that would be great. I think
> >>>>> that the current Landlock bases (almost everything from this series
> >>>>> except the seccomp interface) should meet both needs, but I would like
> >>>>> to have the point of view of the KRSI developers.
> >>>>>
> >>>>> [1] https://lore.kernel.org/lkml/20191029171505.6650-1-mic@digikod.net/
> >>>>>
> >>>>>> BPF is capable
> >>>>>> to be a superset of all existing LSMs whereas landlock and KRSI propsals today
> >>>>>> are custom solutions to specific security concerns. BPF subsystem was extended
> >>>>>> with custom things in the past. In networking we have lwt, skb, tc, xdp, sk
> >>>>>> program types with a lot of overlapping functionality. We couldn't figure out
> >>>>>> how to generalize them into single 'networking' program. Now we can and we
> >>>>>> should. Accepting two partially overlapping bpf-based LSMs would be repeating
> >>>>>> the same mistake again.
> >>>>>
> >>>>> I'll let the LSM maintainers comment on whether BPF could be a superset
> >>>>> of all LSM, but given the complexity of an access-control system, I have
> >>>>> some doubts though. Anyway, we need to start somewhere and then iterate.
> >>>>> This patch series is a first step.
> >>>>
> >>>> I would like KRSI folks to speak up. So far I don't see any sharing happening
> >>>> between landlock and KRSI. You're claiming this set is a first step. They're
> >>>> claiming the same about their patches. I'd like to set a patchset that was
> >>>> jointly developed.
> >>>
> >>> We are willing to collaborate with the Landlock developers and come up
> >>> with a common approach that would work for Landlock and KRSI. I want
> >>> to mention that this collaboration and the current Landlock approach
> >>> of using an eBPF based LSM for unprivileged sandboxing only makes sense
> >>> if unprivileged usage of eBPF is going to be ever allowed.
> >>
> >> The ability to *potentially* do unprivileged sandboxing is definitely
> >> not tied nor a blocker to the unprivileged usage of eBPF. As explained
> >> in the documentation [1] (cf. Guiding principles / Unprivileged use),
> >> Landlock is designed to be as safe as possible (from a security point of
> >> view). The impact is more complex and important than just using
> >> unprivileged eBPF, which may not be required. Unprivileged use of eBPF
> >> would be nice, but I think the current direction is to extend the Linux
> >> capabilities with one or multiple dedicated to eBPF [2] (e.g. CAP_BPF +
> >> something else), which may be even better (and a huge difference with
> >> CAP_SYS_ADMIN, a.k.a. privileged mode or root). Landlock is designed to
> >> deal with unprivileged (i.e. non-root) use cases, but of course, if the
> >> Landlock architecture may enable to do unprivileged stuff, it definitely
> >> can do privileged stuff too. However, having an architecture designed
> >> with safe unprivileged use in mind can't be achieve afterwards.
> >>
> >> [1] https://lore.kernel.org/lkml/20191104172146.30797-8-mic@digikod.net/
> >> [2] https://lore.kernel.org/bpf/20190827205213.456318-1-ast@kernel.org/
> >>
> >>
> >>>
> >>> Purely from a technical standpoint, both the current designs for
> >>> Landlock and KRSI target separate use cases and it would not be
> >>> possible to build "one on top of the other". We've tried to identify
> >>> the lowest denominator ("eBPF+LSM") requirements for both Landlock
> >>> (unprivileged sandboxing / Discretionary Access Control) and KRSI
> >>> (flexibility and unification of privileged MAC and Audit) and
> >>> prototyped an implementation based on the newly added / upcoming
> >>> features in BPF.
> >>
> >> This is not as binary as that. Sandboxing can be seen as DAC but also as
> >> MAC, depending on the subject which apply the security policy and the
> >> subjects which are enforced by this policy. If the sandboxing is applied
> >> system-wide, it is what we usually call MAC. DAC, in the Linux world,
> >> enables any user to restrict access to their files to other users.
> >>
> >> With Landlock it is not the same because a process can restrict itself
> >> but also enforce these restrictions on all its future children (which
> >> may be malicious, whatever their UID/GID). The threat and the definition
> >> of the attacker are not the same in both cases. With the Linux DAC the
> >> potentially malicious subjects are the other users, whereas with
> >> Landlock the potentially malicious subjects are (for now) the current
> >> process and all its children. Another way to explain it, and how
> >> Landlock is designed, is that a specific enforcement (i.e. a set of BPF
> >> programs) is tied to a domain, in which a set of subject are. From this
> >> perspective, this approach (subjects/tasks in a domain) is orthogonal to
> >> the DAC system (subjects/users). This design may apply to a system-wide
> >> MAC system by putting all the system tasks in one domain, and managing
> >> restrictions (by subject) with other means (e.g. task's UID,
> >> command-line strings, environment variables). In short, Landlock (in
> >> this patch series) is closer to a (potentially scoped) MAC system. But
> >> thanks to eBPF, Landlock is firstly a programmatic access-control, which
> >> means that the one who write the programs and tie them to a set of
> >> tasks, can implement their own access-control system (e.g. RBAC,
> >> time-based…), or something else (e.g. an audit system).
> >>
> >> The audit part can simply be achieve with dedicated helpers and programs
> >> that always allow accesses.
> >>
> >> Landlock evolved over multiple iterations and is now designed to be very
> >> flexible. The current way to enforce a security policy is to go through
> >> the seccomp syscall (which makes sense for multiple reasons explained
> >> and accepted before). But Landlock is designed to enable similar
> >> enforcements (or audit) with other ways to define a domain (e.g. cgroups
> >> [3], or system-wide securityfs as done in KRSI). Indeed, the only part
> >> tied to this scoped enforcement is in the domain_syscall.c file. A new
> >> file domain_fs.c could be added to implement a securityfs for a
> >> system-wide enforcement (and have other features as KRSI does).
> >>
> >
> > Given the current way landlock exposes LSM hooks, I don't think it's
> > possible to build system-wide detections.
>
> Why ?
This and some of the other questions (about the core-framework and
guiding principles) are better discussed in the context of the
new-patchset. It might take us some-time as I am on vacation for the
next few weeks and some of the other members are traveling.
>
>
> > But let’s try to come to a
> > consensus on the semantics of the how the LSM hooks are exposed to
> > BPF. At the moment I think we should:
> >
> >
> > * Bring the core interface exposed to eBPF closer to the LSM surface in
> > a way that supports both use cases. One way Landlock can still provide
> > a more abstract interface is by providing some BPF helper libraries
> > that build on top of the core framework.
>
> I still don't get why you think it is the only way or the better. I gave
> a lot of arguments and I explained why Landlock is designed the way it
> is, especially in the documentation (Guiding principles). Is there
> something similar for KRSI?
>
>
> >
> > * Use a single BPF program type; this is necessary for a key requirement
> > of KRSI, i.e. runtime instrumentation. The upcoming prototype should
> > illustrate how this works for KRSI - note that it’s possible to vary
> > the context types exposed by different hooks.
>
> Why a single BPF program type? Do you mean *attach* types? Landlock only
> use one program type, but will use multiple attach types.
>
> Why do you think it is necessary for KRSI or for runtime instrumentation?
>
> If it is justified, it could be a dedicated program attach type (e.g.
> BPF_LANDLOCK_INTROSPECTION).
>
> What is the advantage to have the possibility to vary the context types
> over dedicated *typed* contexts? I don't see any advantages, but at
> least one main drawback: to require runtime checks (when helpers use
> this generic context) instead of load time checks (thanks to static type
> checking of the context).
>
>
> > It would be nice to get the BPF maintainers’ opinion on these points.
> >
> >
> >> [3] https://lore.kernel.org/lkml/20160914072415.26021-17-mic@digikod.net/
> >>
> >> One possible important difference between Landlock and KRSI right now is
> >> the BPF program management. Both manage a list of programs per hook.
> >> However KRSI needs to be able to replace a program in these lists. This
> >> is not implemented in this Landlock patch series, first because it is
> >> not the main use-case and it is safer to have an append-only way to add
> >> restrictions (cf. seccomp-bpf model), and second because it is simpler
> >> to deal with immutable lists. However, it is worth considering extending
> >> the Landlock domain management with the ability to update the program
> >> lists. One challenge may be to identify which program should be replaced
> >> (which KRSI does with the program name). I think it would be wiser to
> >> implement this in a second step though, maybe not for the syscall
> >> interface (thanks to a new seccomp operation), but first with the
> >> securityfs one.
> >>
> >>
> >>>
> >>> We've been successfully able to prototype the use cases for KRSI
> >>> (privileged MAC and Audit) using this "eBPF+LSM" and shared our
> >>> approach at the Linux Security Summit [1]:
> >>>
> >>> * Use the new in-kernel BTF (CO-RE eBPF programs) [2] and the ability
> >>> of the BPF verifier to use the BTF information for access validation
> >>> to provide a more generic way to attach to the various LSM hooks.
> >>> This potentially saves a lot of redundant work:
> >>>
> >>> - Creation of new program types.
> >>> - Multiple types of contexts (or a single context with Unions).
> >>> - Changes to the verifier and creation of new BPF argument types
> >>> (eg. PTR_TO_TASK)
> >>
> >> As I understood from the LSS talk, KRSI's approach is to use the same
> >> hooks as LSM (cf. the securityfs). As said Alexei [4] "It must not make
> >> LSM hooks into stable ABI". Moveover, the LSM hooks may change
> >> according to internal kernel evolution, and their semantic may not make
> >
> > I think you misunderstand Alexei here. I will let him elaborate.
> >
> >> sense from a user space point of view. This is one reason for which
> >> Lanlock abstract those hooks into something that is simpler and designed
> >> to fit well with eBPF (program contexts and their attached types, as
> >> explained in the documentation).
> >>
> >> [4]
> >> https://lore.kernel.org/lkml/20191105215453.szhdkrvuekwfz6le@ast-mbp.dhcp.thefacebook.com/
> >>
> >> How does KRSI plan to deal with one LSM hook being split in two hooks in
> >> a next version of the kernel (cf. [5])?
> >
> > How often has that happened in the past? And even if it does happen,
> > it can still be handled as a part of the base framework we are trying
> > to implement.
>
> I guess the security maintainers should have an opinion on this.
>
> I don't clearly see the properties of this base framework. Could this be
> elaborated?
>
>
> >> [5] https://lore.kernel.org/lkml/20190910115527.5235-6-kpsingh@chromium.org/
> >>
> >>
> >> Another reason to have multiple different attach types/contexts (cf.
> >> landlock_domain->programs[]) is to limit useless BPF program
> >> interpretation (in addition to the non-system-wide scoped of programs).
> >> It also enables to handle and verify strict context use (which is also
> >> explain in the Guiding principles). It would be a huge wast of time to
> >> run every BPF programs for all LSM hooks. KRSI does the same but instead
> >> of relying on the program type it rely on the list tied to the
> >> securityfs file.
> >>
> >> BTF is great, but as far as I know, it's goal is to easily deal with the
> >> moving kernel ABI (e.g. task_struct layout, config/constant variables),
> >> and it is definitely useful to programs using bpf_probe_read() and
> >> similar accessors. However, I don't see how KRSI would avoid BPF types
> >> thanks to BTF.
> >>
> >
> > This should become clearer once we post our updated patch-set. Do note
> > that I am currently traveling and will be away for the next couple of
> > weeks.
> >
> >> There is only one program type for Landlock (i.e.
> >> BPF_PROG_TYPE_LANDLOCK_HOOK), and I don't see why adding new program
> >> *attach* types (e.g. BPF_LANDLOCK_PTRACE) may be an issue. The kernel
> >> will still need to be modified to implement new hooks and the new BPF
> >> helpers anyway, BTF will not change that, except maybe if the internal
> >> LSM API is exposed in a way or another to BPF (thanks to BTF), which
> >> does not seem acceptable. Am I missing something?
> >>
> >>
> >> The current KRSI approach is to allow a common set of helpers to be
> >> called by all programs (because there is no way to differentiate them
> >> with their type).
> >> How KRSI would deal with kernel objects other than the current task
> >> (e.g. a ptrace hook with a tracer and a tracee, a file open/read) with
> >> the struct krsi_ctx unions [6]?
> >>
> >> [6] https://lore.kernel.org/lkml/20190910115527.5235-7-kpsingh@chromium.org/
> >>
> >
> > The best part of BTF is that it can provide a common way to pass
> > different contexts to the various attachments points and the verifier
> > can use the BTF information to validate accesses which essentially
> > allows us to change the helpers from:
> >
> > is_running_executable(magical_krsi_ctx)
> >
> > to
> >
> > is_running_executable(inode)
> >
> >
> > which can work on any inode (ARG_PTR_TO_BTF_ID = btf_id(struct inode))
> >
> > This makes the helpers much more useful and generic. All this is
> > better explained in our upcoming patch-set.
>
> I get the usefulness of BTF for future helper evolution and for moving
> kernel API, but again, I don't see advantages over static typing check
> of (well abstract/generic) kernel object handles in the context.
>
> For instance, how BTF would replace the current BPF_LANDLOCK_PTRACE
> context and the associated helper?
>
> Does this mean that the KRSI v1 design is outdated and superseded by the
> (future) v2 because of the more important use of BTF?
Yes, that's correct and based on the feedback we got on the RFC v1
at the Linux Plumbers 2019.
Also, the changes that allow BPF verifier to use the
BTF information to validate accesses and dynamically populate the
context are very recent and have influenced the design for the
next iteration.
- KP Singh
>
>
> >> How does KRSI plan to deal with security blobs?
> >
> > The new prototype uses security blobs but does not expose them to
> > user-space. These blobs are then used in various helpers like
> > “is_running_executable” which uses blobs on the inode and the
> > task_struct. This should become clearer when the next patchset is
> > posted.
> >
> > I don’t think it’s currently possible to allow the blobs to be set
> > using eBPF programs with the main reason being that the blob will only
> > be set after the program is loaded. The answer to
> > “is_running_executable” becomes dependent on whether the file was
> > executed before the blob setting eBPF program was loaded.
> >
> > Blob management with eBPF is not possible unless we can load eBPF
> > programs that can set blobs at boot-time.
> > In short, the next KRSI version will not give eBPF
> > programs access to arbitrarily write security blobs.
>
> A previous version of Landlock enabled programs to tag inodes (cf.
> FS_GET): https://lore.kernel.org/lkml/20180227004121.3633-8-mic@digikod.net/
>
>
> >>>
> >>> * These new BPF features also alleviate the original concerns that we
> >>> raised when initially proposing KRSI and designing for precise BPF
> >>> helpers. We have some patches coming up which incorporate these new
> >>> changes and will be sharing something on the mailing list after some
> >>> cleanup.
> >>>
> >>> We can use the common "eBPF+LSM" for both privileged MAC and Audit and
> >>> unprivileged sandboxing i.e. Discretionary Access Control.
> >>> Here's what it could look like:
> >>>
> >>> * Common infrastructure allows attachment to all hooks which works well
> >>> for privileged MAC and Audit. This could be extended to provide
> >>> another attachment type for unprivileged DAC, which can restrict the
> >>> hooks that can be attached to, and also the information that is
> >>> exposed to the eBPF programs which is something that Landlock could
> >>> build.
> >>
> >> I agree that the "privileged-only" hooks should be a superset of the
> >> "security-safe-and-potentially-unprivileged" hooks. :)
> >> However, as said previously, I'm convinced it is a requirement to have
> >> abstract hooks (and associated program attach types) as defined by Landlock.
> >
> > I would like to hear the BPF maintainers’ perspective on this. I am
> > not sure they agree with you here.
> >
> > - KP Singh
> >
> >>
> >> I'm not sure what you mean by "the information that is exposed to the
> >> eBPF program". Is it the current Landlock implementation of specific
> >> contexts and attach types?
>
> …or does BTF could magically solve this?
>
>
> >>
> >>>
> >>> * This attachment could use the proposed landlock domains and attach to
> >>> the task_struct providing the discretionary access control semantics.
> >>
> >> Not task_struct but creds, yes. This is a characteristic of sandboxing,
> >> which may not be useful for the KRSI use case. It makes sense for KRSI
> >> to attach program sets (or Landlock domains) to the whole system, then
> >> using the creds does not make sense here. This difference is small and a
> >> previous version of Landlock already validated this use case with
> >> cgroups [3] (which is postponed to simplify the patch series).
> >>
> >> [3] https://lore.kernel.org/lkml/20160914072415.26021-17-mic@digikod.net/
> >>
> >>
> >>>
> >>> [1] https://static.sched.com/hosted_files/lsseu2019/a2/Kernel%20Runtime%20Security%20Instrumentation.pdf
> >>> [2] http://vger.kernel.org/bpfconf2019_talks/bpf-core.pdf
> >>>
> >>> - KP Singh
> >>
> >> I think it should be OK to first land something close to this Landlock
> >> patch series and then we could extend the domain management features and
> >> add the securityfs support that KRSI needs. The main concern seems to be
> >> about hook definitions.
> >>
> >> Another approach would be to land Landlock and KRSI as distinct LSM
> >> while trying as much as possible to mutualize code/helpers.
> >
^ permalink raw reply
* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Daniel Borkmann @ 2019-11-08 14:34 UTC (permalink / raw)
To: Mickaël Salaün, KP Singh
Cc: Alexei Starovoitov, linux-kernel, Alexei Starovoitov,
Andy Lutomirski, Casey Schaufler, David Drysdale, Florent Revest,
James Morris, Jann Horn, John Johansen, Jonathan Corbet,
Kees Cook, Michael Kerrisk, Mickaël Salaün, Paul Moore,
Sargun Dhillon, Serge E . Hallyn, Shuah Khan, Stephen Smalley,
Tejun Heo, Tetsuo Handa, Tycho Andersen, Will Drewry, bpf,
kernel-hardening, linux-api, linux-security-module, netdev
In-Reply-To: <3e208632-e7ab-3405-5196-ab1d770e20c3@digikod.net>
On 11/8/19 3:08 PM, Mickaël Salaün wrote:
> On 06/11/2019 22:45, KP Singh wrote:
>> On 06-Nov 17:55, Mickaël Salaün wrote:
>>> On 06/11/2019 11:06, KP Singh wrote:
>>>> On 05-Nov 11:34, Alexei Starovoitov wrote:
>>>>> On Tue, Nov 05, 2019 at 07:01:41PM +0100, Mickaël Salaün wrote:
>>>>>> On 05/11/2019 18:18, Alexei Starovoitov wrote:
[...]
>> * Use a single BPF program type; this is necessary for a key requirement
>> of KRSI, i.e. runtime instrumentation. The upcoming prototype should
>> illustrate how this works for KRSI - note that it’s possible to vary
>> the context types exposed by different hooks.
>
> Why a single BPF program type? Do you mean *attach* types? Landlock only
> use one program type, but will use multiple attach types.
>
> Why do you think it is necessary for KRSI or for runtime instrumentation?
>
> If it is justified, it could be a dedicated program attach type (e.g.
> BPF_LANDLOCK_INTROSPECTION).
>
> What is the advantage to have the possibility to vary the context types
> over dedicated *typed* contexts? I don't see any advantages, but at
> least one main drawback: to require runtime checks (when helpers use
> this generic context) instead of load time checks (thanks to static type
> checking of the context).
Lets take security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
as one specific example here: the running kernel has its own internal
btf_vmlinux and therefore a complete description of itself. From verifier
side we can retrieve & introspect the security_sock_rcv_skb signatue and
thus know that the given BPF attachment point has struct sock and struct
sk_buff as input arguments which can then be accessed generically by the
prog in order to allow sk_filter_trim_cap() to pass or to drop the skb.
The same generic approach can be done for many of the other lsm hooks, so
single program type would be enough there and context is derived automatically,
no dedicated extra context per attach type would be needed and no runtime
checks as you mentioned above since its still all asserted at verification
time.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Mickaël Salaün @ 2019-11-08 14:08 UTC (permalink / raw)
To: KP Singh
Cc: Alexei Starovoitov, linux-kernel, Alexei Starovoitov,
Andy Lutomirski, Casey Schaufler, Daniel Borkmann, David Drysdale,
Florent Revest, James Morris, Jann Horn, John Johansen,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Paul Moore, Sargun Dhillon,
Serge E . Hallyn, Shuah Khan, Stephen Smalley, Tejun Heo,
Tetsuo Handa, Tycho Andersen, Will Drewry, bpf, kernel-hardening,
linux-api, linux-security-module, netdev
In-Reply-To: <20191106214526.GA22244@chromium.org>
On 06/11/2019 22:45, KP Singh wrote:
> On 06-Nov 17:55, Mickaël Salaün wrote:
>>
>> On 06/11/2019 11:06, KP Singh wrote:
>>> On 05-Nov 11:34, Alexei Starovoitov wrote:
>>>> On Tue, Nov 05, 2019 at 07:01:41PM +0100, Mickaël Salaün wrote:
>>>>> On 05/11/2019 18:18, Alexei Starovoitov wrote:
>>
>> [...]
>>
>>>>>> I think the only way bpf-based LSM can land is both landlock and KRSI
>>>>>> developers work together on a design that solves all use cases.
>>>>>
>>>>> As I said in a previous cover letter [1], that would be great. I think
>>>>> that the current Landlock bases (almost everything from this series
>>>>> except the seccomp interface) should meet both needs, but I would like
>>>>> to have the point of view of the KRSI developers.
>>>>>
>>>>> [1] https://lore.kernel.org/lkml/20191029171505.6650-1-mic@digikod.net/
>>>>>
>>>>>> BPF is capable
>>>>>> to be a superset of all existing LSMs whereas landlock and KRSI propsals today
>>>>>> are custom solutions to specific security concerns. BPF subsystem was extended
>>>>>> with custom things in the past. In networking we have lwt, skb, tc, xdp, sk
>>>>>> program types with a lot of overlapping functionality. We couldn't figure out
>>>>>> how to generalize them into single 'networking' program. Now we can and we
>>>>>> should. Accepting two partially overlapping bpf-based LSMs would be repeating
>>>>>> the same mistake again.
>>>>>
>>>>> I'll let the LSM maintainers comment on whether BPF could be a superset
>>>>> of all LSM, but given the complexity of an access-control system, I have
>>>>> some doubts though. Anyway, we need to start somewhere and then iterate.
>>>>> This patch series is a first step.
>>>>
>>>> I would like KRSI folks to speak up. So far I don't see any sharing happening
>>>> between landlock and KRSI. You're claiming this set is a first step. They're
>>>> claiming the same about their patches. I'd like to set a patchset that was
>>>> jointly developed.
>>>
>>> We are willing to collaborate with the Landlock developers and come up
>>> with a common approach that would work for Landlock and KRSI. I want
>>> to mention that this collaboration and the current Landlock approach
>>> of using an eBPF based LSM for unprivileged sandboxing only makes sense
>>> if unprivileged usage of eBPF is going to be ever allowed.
>>
>> The ability to *potentially* do unprivileged sandboxing is definitely
>> not tied nor a blocker to the unprivileged usage of eBPF. As explained
>> in the documentation [1] (cf. Guiding principles / Unprivileged use),
>> Landlock is designed to be as safe as possible (from a security point of
>> view). The impact is more complex and important than just using
>> unprivileged eBPF, which may not be required. Unprivileged use of eBPF
>> would be nice, but I think the current direction is to extend the Linux
>> capabilities with one or multiple dedicated to eBPF [2] (e.g. CAP_BPF +
>> something else), which may be even better (and a huge difference with
>> CAP_SYS_ADMIN, a.k.a. privileged mode or root). Landlock is designed to
>> deal with unprivileged (i.e. non-root) use cases, but of course, if the
>> Landlock architecture may enable to do unprivileged stuff, it definitely
>> can do privileged stuff too. However, having an architecture designed
>> with safe unprivileged use in mind can't be achieve afterwards.
>>
>> [1] https://lore.kernel.org/lkml/20191104172146.30797-8-mic@digikod.net/
>> [2] https://lore.kernel.org/bpf/20190827205213.456318-1-ast@kernel.org/
>>
>>
>>>
>>> Purely from a technical standpoint, both the current designs for
>>> Landlock and KRSI target separate use cases and it would not be
>>> possible to build "one on top of the other". We've tried to identify
>>> the lowest denominator ("eBPF+LSM") requirements for both Landlock
>>> (unprivileged sandboxing / Discretionary Access Control) and KRSI
>>> (flexibility and unification of privileged MAC and Audit) and
>>> prototyped an implementation based on the newly added / upcoming
>>> features in BPF.
>>
>> This is not as binary as that. Sandboxing can be seen as DAC but also as
>> MAC, depending on the subject which apply the security policy and the
>> subjects which are enforced by this policy. If the sandboxing is applied
>> system-wide, it is what we usually call MAC. DAC, in the Linux world,
>> enables any user to restrict access to their files to other users.
>>
>> With Landlock it is not the same because a process can restrict itself
>> but also enforce these restrictions on all its future children (which
>> may be malicious, whatever their UID/GID). The threat and the definition
>> of the attacker are not the same in both cases. With the Linux DAC the
>> potentially malicious subjects are the other users, whereas with
>> Landlock the potentially malicious subjects are (for now) the current
>> process and all its children. Another way to explain it, and how
>> Landlock is designed, is that a specific enforcement (i.e. a set of BPF
>> programs) is tied to a domain, in which a set of subject are. From this
>> perspective, this approach (subjects/tasks in a domain) is orthogonal to
>> the DAC system (subjects/users). This design may apply to a system-wide
>> MAC system by putting all the system tasks in one domain, and managing
>> restrictions (by subject) with other means (e.g. task's UID,
>> command-line strings, environment variables). In short, Landlock (in
>> this patch series) is closer to a (potentially scoped) MAC system. But
>> thanks to eBPF, Landlock is firstly a programmatic access-control, which
>> means that the one who write the programs and tie them to a set of
>> tasks, can implement their own access-control system (e.g. RBAC,
>> time-based…), or something else (e.g. an audit system).
>>
>> The audit part can simply be achieve with dedicated helpers and programs
>> that always allow accesses.
>>
>> Landlock evolved over multiple iterations and is now designed to be very
>> flexible. The current way to enforce a security policy is to go through
>> the seccomp syscall (which makes sense for multiple reasons explained
>> and accepted before). But Landlock is designed to enable similar
>> enforcements (or audit) with other ways to define a domain (e.g. cgroups
>> [3], or system-wide securityfs as done in KRSI). Indeed, the only part
>> tied to this scoped enforcement is in the domain_syscall.c file. A new
>> file domain_fs.c could be added to implement a securityfs for a
>> system-wide enforcement (and have other features as KRSI does).
>>
>
> Given the current way landlock exposes LSM hooks, I don't think it's
> possible to build system-wide detections.
Why ?
> But let’s try to come to a
> consensus on the semantics of the how the LSM hooks are exposed to
> BPF. At the moment I think we should:
>
>
> * Bring the core interface exposed to eBPF closer to the LSM surface in
> a way that supports both use cases. One way Landlock can still provide
> a more abstract interface is by providing some BPF helper libraries
> that build on top of the core framework.
I still don't get why you think it is the only way or the better. I gave
a lot of arguments and I explained why Landlock is designed the way it
is, especially in the documentation (Guiding principles). Is there
something similar for KRSI?
>
> * Use a single BPF program type; this is necessary for a key requirement
> of KRSI, i.e. runtime instrumentation. The upcoming prototype should
> illustrate how this works for KRSI - note that it’s possible to vary
> the context types exposed by different hooks.
Why a single BPF program type? Do you mean *attach* types? Landlock only
use one program type, but will use multiple attach types.
Why do you think it is necessary for KRSI or for runtime instrumentation?
If it is justified, it could be a dedicated program attach type (e.g.
BPF_LANDLOCK_INTROSPECTION).
What is the advantage to have the possibility to vary the context types
over dedicated *typed* contexts? I don't see any advantages, but at
least one main drawback: to require runtime checks (when helpers use
this generic context) instead of load time checks (thanks to static type
checking of the context).
> It would be nice to get the BPF maintainers’ opinion on these points.
>
>
>> [3] https://lore.kernel.org/lkml/20160914072415.26021-17-mic@digikod.net/
>>
>> One possible important difference between Landlock and KRSI right now is
>> the BPF program management. Both manage a list of programs per hook.
>> However KRSI needs to be able to replace a program in these lists. This
>> is not implemented in this Landlock patch series, first because it is
>> not the main use-case and it is safer to have an append-only way to add
>> restrictions (cf. seccomp-bpf model), and second because it is simpler
>> to deal with immutable lists. However, it is worth considering extending
>> the Landlock domain management with the ability to update the program
>> lists. One challenge may be to identify which program should be replaced
>> (which KRSI does with the program name). I think it would be wiser to
>> implement this in a second step though, maybe not for the syscall
>> interface (thanks to a new seccomp operation), but first with the
>> securityfs one.
>>
>>
>>>
>>> We've been successfully able to prototype the use cases for KRSI
>>> (privileged MAC and Audit) using this "eBPF+LSM" and shared our
>>> approach at the Linux Security Summit [1]:
>>>
>>> * Use the new in-kernel BTF (CO-RE eBPF programs) [2] and the ability
>>> of the BPF verifier to use the BTF information for access validation
>>> to provide a more generic way to attach to the various LSM hooks.
>>> This potentially saves a lot of redundant work:
>>>
>>> - Creation of new program types.
>>> - Multiple types of contexts (or a single context with Unions).
>>> - Changes to the verifier and creation of new BPF argument types
>>> (eg. PTR_TO_TASK)
>>
>> As I understood from the LSS talk, KRSI's approach is to use the same
>> hooks as LSM (cf. the securityfs). As said Alexei [4] "It must not make
>> LSM hooks into stable ABI". Moveover, the LSM hooks may change
>> according to internal kernel evolution, and their semantic may not make
>
> I think you misunderstand Alexei here. I will let him elaborate.
>
>> sense from a user space point of view. This is one reason for which
>> Lanlock abstract those hooks into something that is simpler and designed
>> to fit well with eBPF (program contexts and their attached types, as
>> explained in the documentation).
>>
>> [4]
>> https://lore.kernel.org/lkml/20191105215453.szhdkrvuekwfz6le@ast-mbp.dhcp.thefacebook.com/
>>
>> How does KRSI plan to deal with one LSM hook being split in two hooks in
>> a next version of the kernel (cf. [5])?
>
> How often has that happened in the past? And even if it does happen,
> it can still be handled as a part of the base framework we are trying
> to implement.
I guess the security maintainers should have an opinion on this.
I don't clearly see the properties of this base framework. Could this be
elaborated?
>> [5] https://lore.kernel.org/lkml/20190910115527.5235-6-kpsingh@chromium.org/
>>
>>
>> Another reason to have multiple different attach types/contexts (cf.
>> landlock_domain->programs[]) is to limit useless BPF program
>> interpretation (in addition to the non-system-wide scoped of programs).
>> It also enables to handle and verify strict context use (which is also
>> explain in the Guiding principles). It would be a huge wast of time to
>> run every BPF programs for all LSM hooks. KRSI does the same but instead
>> of relying on the program type it rely on the list tied to the
>> securityfs file.
>>
>> BTF is great, but as far as I know, it's goal is to easily deal with the
>> moving kernel ABI (e.g. task_struct layout, config/constant variables),
>> and it is definitely useful to programs using bpf_probe_read() and
>> similar accessors. However, I don't see how KRSI would avoid BPF types
>> thanks to BTF.
>>
>
> This should become clearer once we post our updated patch-set. Do note
> that I am currently traveling and will be away for the next couple of
> weeks.
>
>> There is only one program type for Landlock (i.e.
>> BPF_PROG_TYPE_LANDLOCK_HOOK), and I don't see why adding new program
>> *attach* types (e.g. BPF_LANDLOCK_PTRACE) may be an issue. The kernel
>> will still need to be modified to implement new hooks and the new BPF
>> helpers anyway, BTF will not change that, except maybe if the internal
>> LSM API is exposed in a way or another to BPF (thanks to BTF), which
>> does not seem acceptable. Am I missing something?
>>
>>
>> The current KRSI approach is to allow a common set of helpers to be
>> called by all programs (because there is no way to differentiate them
>> with their type).
>> How KRSI would deal with kernel objects other than the current task
>> (e.g. a ptrace hook with a tracer and a tracee, a file open/read) with
>> the struct krsi_ctx unions [6]?
>>
>> [6] https://lore.kernel.org/lkml/20190910115527.5235-7-kpsingh@chromium.org/
>>
>
> The best part of BTF is that it can provide a common way to pass
> different contexts to the various attachments points and the verifier
> can use the BTF information to validate accesses which essentially
> allows us to change the helpers from:
>
> is_running_executable(magical_krsi_ctx)
>
> to
>
> is_running_executable(inode)
>
>
> which can work on any inode (ARG_PTR_TO_BTF_ID = btf_id(struct inode))
>
> This makes the helpers much more useful and generic. All this is
> better explained in our upcoming patch-set.
I get the usefulness of BTF for future helper evolution and for moving
kernel API, but again, I don't see advantages over static typing check
of (well abstract/generic) kernel object handles in the context.
For instance, how BTF would replace the current BPF_LANDLOCK_PTRACE
context and the associated helper?
Does this mean that the KRSI v1 design is outdated and superseded by the
(future) v2 because of the more important use of BTF?
>> How does KRSI plan to deal with security blobs?
>
> The new prototype uses security blobs but does not expose them to
> user-space. These blobs are then used in various helpers like
> “is_running_executable” which uses blobs on the inode and the
> task_struct. This should become clearer when the next patchset is
> posted.
>
> I don’t think it’s currently possible to allow the blobs to be set
> using eBPF programs with the main reason being that the blob will only
> be set after the program is loaded. The answer to
> “is_running_executable” becomes dependent on whether the file was
> executed before the blob setting eBPF program was loaded.
>
> Blob management with eBPF is not possible unless we can load eBPF
> programs that can set blobs at boot-time.
> In short, the next KRSI version will not give eBPF
> programs access to arbitrarily write security blobs.
A previous version of Landlock enabled programs to tag inodes (cf.
FS_GET): https://lore.kernel.org/lkml/20180227004121.3633-8-mic@digikod.net/
>>>
>>> * These new BPF features also alleviate the original concerns that we
>>> raised when initially proposing KRSI and designing for precise BPF
>>> helpers. We have some patches coming up which incorporate these new
>>> changes and will be sharing something on the mailing list after some
>>> cleanup.
>>>
>>> We can use the common "eBPF+LSM" for both privileged MAC and Audit and
>>> unprivileged sandboxing i.e. Discretionary Access Control.
>>> Here's what it could look like:
>>>
>>> * Common infrastructure allows attachment to all hooks which works well
>>> for privileged MAC and Audit. This could be extended to provide
>>> another attachment type for unprivileged DAC, which can restrict the
>>> hooks that can be attached to, and also the information that is
>>> exposed to the eBPF programs which is something that Landlock could
>>> build.
>>
>> I agree that the "privileged-only" hooks should be a superset of the
>> "security-safe-and-potentially-unprivileged" hooks. :)
>> However, as said previously, I'm convinced it is a requirement to have
>> abstract hooks (and associated program attach types) as defined by Landlock.
>
> I would like to hear the BPF maintainers’ perspective on this. I am
> not sure they agree with you here.
>
> - KP Singh
>
>>
>> I'm not sure what you mean by "the information that is exposed to the
>> eBPF program". Is it the current Landlock implementation of specific
>> contexts and attach types?
…or does BTF could magically solve this?
>>
>>>
>>> * This attachment could use the proposed landlock domains and attach to
>>> the task_struct providing the discretionary access control semantics.
>>
>> Not task_struct but creds, yes. This is a characteristic of sandboxing,
>> which may not be useful for the KRSI use case. It makes sense for KRSI
>> to attach program sets (or Landlock domains) to the whole system, then
>> using the creds does not make sense here. This difference is small and a
>> previous version of Landlock already validated this use case with
>> cgroups [3] (which is postponed to simplify the patch series).
>>
>> [3] https://lore.kernel.org/lkml/20160914072415.26021-17-mic@digikod.net/
>>
>>
>>>
>>> [1] https://static.sched.com/hosted_files/lsseu2019/a2/Kernel%20Runtime%20Security%20Instrumentation.pdf
>>> [2] http://vger.kernel.org/bpfconf2019_talks/bpf-core.pdf
>>>
>>> - KP Singh
>>
>> I think it should be OK to first land something close to this Landlock
>> patch series and then we could extend the domain management features and
>> add the securityfs support that KRSI needs. The main concern seems to be
>> about hook definitions.
>>
>> Another approach would be to land Landlock and KRSI as distinct LSM
>> while trying as much as possible to mutualize code/helpers.
>
^ permalink raw reply
* Re: [PATCH v23 12/24] x86/sgx: Linux Enclave Driver
From: Jarkko Sakkinen @ 2019-11-08 8:20 UTC (permalink / raw)
To: Sean Christopherson
Cc: linux-kernel, x86, linux-sgx, akpm, dave.hansen, nhorman,
npmccallum, serge.ayoun, shay.katz-zamir, haitao.huang,
andriy.shevchenko, tglx, kai.svahn, bp, josh, luto, kai.huang,
rientjes, cedric.xing, puiterwijk, linux-security-module,
Suresh Siddha
In-Reply-To: <20191105111057.GA20879@linux.intel.com>
On Tue, Nov 05, 2019 at 01:11:22PM +0200, Jarkko Sakkinen wrote:
> I'll add @count to address this. This output field will contain the
> number of bytes actually written instead of overwriting input
> parameters, which is a bad practice in anyway.
>
> We don't need to actually cap to anything but API must be able to
> support such scenario. Caller must be prepared to deal with the
> situation where the return value is zero but @count < @length.
I summarized here my reasoning on @count:
https://lore.kernel.org/linux-sgx/20191108081331.GB3370@linux.intel.com/
/Jarkko
^ permalink raw reply
* Re: [PATCH v23 12/24] x86/sgx: Linux Enclave Driver
From: Jarkko Sakkinen @ 2019-11-08 8:05 UTC (permalink / raw)
To: Stephen Smalley
Cc: Sean Christopherson, linux-kernel, x86, linux-sgx, akpm,
dave.hansen, nhorman, npmccallum, serge.ayoun, shay.katz-zamir,
haitao.huang, andriy.shevchenko, tglx, kai.svahn, bp, josh, luto,
kai.huang, rientjes, cedric.xing, puiterwijk,
linux-security-module, Suresh Siddha
In-Reply-To: <f3a3d137-a187-9090-f5af-da306ced5371@tycho.nsa.gov>
On Fri, Nov 01, 2019 at 01:16:59PM -0400, Stephen Smalley wrote:
> On 11/1/19 11:32 AM, Sean Christopherson wrote:
> > On Fri, Nov 01, 2019 at 09:28:17AM -0400, Stephen Smalley wrote:
> > > On 11/1/19 9:16 AM, Stephen Smalley wrote:
> > > > So, IIUC, that means that merging the driver will create a regression with
> > > > respect to LSM control over executable mappings that will only be
> > > > rectified at some future point in time if/when someone submits LSM hooks
> > > > or calls to existing hooks to restore such control. That doesn't seem
> > > > like a good idea. Why can't you include at least that basic level of
> > > > control now? It is one thing to defer finer grained control or
> > > > SGX-specific access controls to the future - that I can understand. But
> > > > introducing a regression in the existing controls is not really ok.
> > >
> > > Unless you are arguing that the existing checks on mmap/mprotect of
> > > /dev/sgx/enclave are a coarse-grained approximation (effectively requiring
> > > WX to the file or execmem for any user of SGX).
> >
> > Yes, that's the argument as running any enclave will require RWX access to
> > /dev/sgx/enclave. EXECMEM won't trigger for SGX users as /dev/sgx/enclave
> > must be MAP_SHARED and it's a non-private file (not backed by anonymous
> > inode, in case I got the file terminology wrong).
>
> Ok, so for SELinux's purposes, one will need to allow :file { open ioctl map
> read write execute } to whatever type is ultimately assigned to
> /dev/sgx/enclave in order to use SGX.
AFAIK yes.
/Jarkko
^ permalink raw reply
* Re: [RFC PATCH 04/14] pipe: Add O_NOTIFICATION_PIPE [ver #2]
From: David Howells @ 2019-11-08 6:42 UTC (permalink / raw)
To: Andy Lutomirski
Cc: dhowells, Linus Torvalds, Greg Kroah-Hartman, Casey Schaufler,
Stephen Smalley, Nicolas Dichtel, raven, Christian Brauner,
keyrings, USB list, linux-block, LSM List, Linux FS Devel,
Linux API, LKML
In-Reply-To: <CALCETrWeN9CGJHz0dzG1uH5Qjbr+xG3OKZuEd33eBY_rAzVkqQ@mail.gmail.com>
Andy Lutomirski <luto@kernel.org> wrote:
> I can open a normal pipe from userspace (with pipe() or pipe2()), and
> I can have two threads. One thread writes to the pipe with write().
> The other thread writes with splice(). Everything works fine.
Yes. Every operation you do on a pipe from userspace is serialised with the
pipe mutex - and both ends share the same pipe.
> What's special about notifications?
The post_notification() cannot take the pipe mutex. It has to be callable
from softirq context. Linus's idea is that when you're actually altering the
ring pointers you should hold the wake-queue spinlock, and post_notification()
holds the wake queue spinlock for the duration of the operation.
This means that post_notification() can be writing to the pipe whilst a
userspace-invoked operation is holding the pipe mutex and is also doing
something to the ring.
David
^ permalink raw reply
* Re: [RFC PATCH 04/14] pipe: Add O_NOTIFICATION_PIPE [ver #2]
From: Andy Lutomirski @ 2019-11-08 5:06 UTC (permalink / raw)
To: David Howells
Cc: Andy Lutomirski, Linus Torvalds, Greg Kroah-Hartman,
Casey Schaufler, Stephen Smalley, Nicolas Dichtel, raven,
Christian Brauner, keyrings, USB list, linux-block, LSM List,
Linux FS Devel, Linux API, LKML
In-Reply-To: <6964.1573152517@warthog.procyon.org.uk>
On Thu, Nov 7, 2019 at 10:48 AM David Howells <dhowells@redhat.com> wrote:
>
> Andy Lutomirski <luto@kernel.org> wrote:
>
> > > Add an O_NOTIFICATION_PIPE flag that can be passed to pipe2() to indicate
> > > that the pipe being created is going to be used for notifications. This
> > > suppresses the use of splice(), vmsplice(), tee() and sendfile() on the
> > > pipe as calling iov_iter_revert() on a pipe when a kernel notification
> > > message has been inserted into the middle of a multi-buffer splice will be
> > > messy.
> >
> > How messy?
>
> Well, iov_iter_revert() on a pipe iterator simply walks backwards along the
> ring discarding the last N contiguous slots (where N is normally the number of
> slots that were filled by whatever operation is being reverted).
>
> However, unless the code that transfers stuff into the pipe takes the spinlock
> spinlock and disables softirqs for the duration of its ring filling, what were
> N contiguous slots may now have kernel notifications interspersed - even if it
> has been holding the pipe mutex.
>
> So, now what do you do? You have to free up just the buffers relevant to the
> iterator and then you can either compact down the ring to free up the space or
> you can leave null slots and let the read side clean them up, thereby
> reducing the capacity of the pipe temporarily.
>
> Either way, iov_iter_revert() gets more complex and has to hold the spinlock.
I feel like I'm missing something fundamental here.
I can open a normal pipe from userspace (with pipe() or pipe2()), and
I can have two threads. One thread writes to the pipe with write().
The other thread writes with splice(). Everything works fine. What's
special about notifications?
^ permalink raw reply
* Re: [PATCH linux-kselftest/test v2] apparmor: add AppArmor KUnit tests for policy unpack
From: Brendan Higgins @ 2019-11-07 23:33 UTC (permalink / raw)
To: Kees Cook
Cc: shuah, john.johansen, jmorris, serge, alan.maguire, yzaikin,
davidgow, mcgrof, tytso, linux-kernel, linux-security-module,
kunit-dev, linux-kselftest, Mike Salvatore
In-Reply-To: <201911060916.AC9E14B@keescook>
On Wed, Nov 06, 2019 at 09:18:27AM -0800, Kees Cook wrote:
> On Tue, Nov 05, 2019 at 04:43:29PM -0800, Brendan Higgins wrote:
> > From: Mike Salvatore <mike.salvatore@canonical.com>
> >
> > Add KUnit tests to test AppArmor unpacking of userspace policies.
> > AppArmor uses a serialized binary format for loading policies. To find
> > policy format documentation see
> > Documentation/admin-guide/LSM/apparmor.rst.
> >
> > In order to write the tests against the policy unpacking code, some
> > static functions needed to be exposed for testing purposes. One of the
> > goals of this patch is to establish a pattern for which testing these
> > kinds of functions should be done in the future.
> >
> > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > Signed-off-by: Mike Salvatore <mike.salvatore@canonical.com>
> > ---
> > security/apparmor/Kconfig | 16 +
> > security/apparmor/policy_unpack.c | 4 +
> > security/apparmor/policy_unpack_test.c | 607 +++++++++++++++++++++++++
> > 3 files changed, 627 insertions(+)
> > create mode 100644 security/apparmor/policy_unpack_test.c
> >
> > diff --git a/security/apparmor/Kconfig b/security/apparmor/Kconfig
> > index d8b1a360a6368..78a33ccac2574 100644
> > --- a/security/apparmor/Kconfig
> > +++ b/security/apparmor/Kconfig
> > @@ -66,3 +66,19 @@ config SECURITY_APPARMOR_DEBUG_MESSAGES
> > Set the default value of the apparmor.debug kernel parameter.
> > When enabled, various debug messages will be logged to
> > the kernel message buffer.
> > +
> > +config SECURITY_APPARMOR_KUNIT_TEST
> > + bool "Build KUnit tests for policy_unpack.c"
> > + depends on KUNIT && SECURITY_APPARMOR
> > + help
> > + This builds the AppArmor KUnit tests.
> > +
> > + KUnit tests run during boot and output the results to the debug log
> > + in TAP format (http://testanything.org/). Only useful for kernel devs
> > + running KUnit test harness and are not for inclusion into a
> > + production build.
> > +
> > + For more information on KUnit and unit tests in general please refer
> > + to the KUnit documentation in Documentation/dev-tools/kunit/.
> > +
> > + If unsure, say N.
> > diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
> > index 8cfc9493eefc7..37c1dd3178fc0 100644
> > --- a/security/apparmor/policy_unpack.c
> > +++ b/security/apparmor/policy_unpack.c
> > @@ -1120,3 +1120,7 @@ int aa_unpack(struct aa_loaddata *udata, struct list_head *lh,
> >
> > return error;
> > }
> > +
> > +#ifdef CONFIG_SECURITY_APPARMOR_KUNIT_TEST
> > +#include "policy_unpack_test.c"
> > +#endif /* CONFIG_SECURITY_APPARMOR_KUNIT_TEST */
>
> To make this even LESS intrusive, the ifdefs could live in ..._test.c.
Less intrusive, yes, but I think I actually like the ifdef here; it
makes it clear from the source that the test is only a part of the build
when configured to do so. Nevertheless, I will change it if anyone feels
strongly about it.
> Also, while I *think* the kernel build system will correctly track this
> dependency, can you double-check that changes to ..._test.c correctly
> trigger a recompile of policy_unpack.c?
Yep, just verified, first I ran the tests and everything passed. Then I
applied the following diff:
diff --git a/security/apparmor/policy_unpack_test.c b/security/apparmor/policy_unpack_test.c
index 533137f45361c..e1b0670dbdc27 100644
--- a/security/apparmor/policy_unpack_test.c
+++ b/security/apparmor/policy_unpack_test.c
@@ -161,7 +161,7 @@ static void policy_unpack_test_unpack_array_with_name(struct kunit *test)
array_size = unpack_array(puf->e, name);
- KUNIT_EXPECT_EQ(test, array_size, (u16)TEST_ARRAY_SIZE);
+ KUNIT_EXPECT_EQ(test, array_size + 1, (u16)TEST_ARRAY_SIZE);
KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1);
}
and reran the tests (to trigger an incremental build) and the test
failed as expected indicating that the dependency is properly tracked.
Cheers!
^ permalink raw reply related
* Re: [PATCH v4 01/10] IMA: Defined an IMA hook to measure keys on key create or update
From: Lakshmi Ramasubramanian @ 2019-11-07 21:12 UTC (permalink / raw)
To: Mimi Zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <1573159988.5028.400.camel@linux.ibm.com>
On 11/7/19 12:53 PM, Mimi Zohar wrote:
>>
>> The measurement decision is not based on whether the keyring is a
>> trusted one or an untrusted one. As long as the IMA policy allows
>> (through the "keyrings=" option) the key will be measured.
>
> We should be able to measure all keys being loaded onto any keyring or
> onto a specific "keyring=". This shouldn't be any different than any
> other policy rule. Once you have this basic feature working, you
> would address loading keys during early boot.
Perfect - that's exactly how I have implemented it right now. Will
continue to test it.
>> Do you want only trusted keyrings to be allowed in the measurement?
>> In my opinion, that decision should be deferred to whoever is setting up
>> the IMA policy.
>
> Right, but it shouldn't be limited to just "trusted" keyrings. This
> way you can first test loading keys onto any keyring.
Thank you.
> Queuing the keys should be independent of measuring the keys.
> Initially you would start with just measuring the key. From a high
> level it would look like:
>
> ima_post_key_create_or_update(...)
> {
> "measure key based on
> policy(key, keyring, ...)"
> }
>
> This requires the IMA "keyring=" policy option support be defined
> first.
>
> Subsequently you would add key queuing support, and then update
> ima_post_key_create_or_update(). It would look like:
>
> ima_post_key_create_or_update(...)
> {
> if (custom policy is loaded)
> "measure key based on policy(key, keyring, ...)"
> else
> "queue key(key, keyring)"
> }
>
> Mimi
Yes - I have the above change working. Will continue testing.
thanks,
-lakshmi
^ permalink raw reply
* Re: [PATCH v4 01/10] IMA: Defined an IMA hook to measure keys on key create or update
From: Mimi Zohar @ 2019-11-07 20:53 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, dhowells, matthewgarrett, sashal,
jamorris, linux-integrity, linux-security-module, keyrings,
linux-kernel
In-Reply-To: <7ce84aa0-729e-c58e-f16a-25490b4e336d@linux.microsoft.com>
On Thu, 2019-11-07 at 10:42 -0800, Lakshmi Ramasubramanian wrote:
> On 11/6/2019 7:40 PM, Mimi Zohar wrote:
>
> >>> I would move the patch that defines the "keyring=" policy option prior
> >>> to this one. Include the call to process_buffer_measurement() in this
> >>> patch. A subsequent patch would add support to defer measuring the
> >>> key, by calling a function named something like
> >>> ima_queue_key_measurement().
> >>>
> >>
> >> As I'd stated in the other response, I wanted to isolate all key related
> >> code in a separate C file and build it if and only if all CONFIG
> >> dependencies are met.
> >
> > The basic measuring of keys shouldn't be any different than any other
> > policy rule, other than it is a key and not a file. This is the
> > reason that I keep saying start out with the basics and then add
> > support to defer measuring keys on the trusted keyrings.
>
> I'll make the changes, rearrange the patches and send an updated set.
>
> I do have a few questions since I am still not fully understanding the
> requirements you are targeting. Appreciate if you could please clarify.
>
> As you already know, I am using the "public key" of the given asymmetric
> key as the "buffer" to measure in process_buffer_measurement().
>
> The measurement decision is not based on whether the keyring is a
> trusted one or an untrusted one. As long as the IMA policy allows
> (through the "keyrings=" option) the key will be measured.
We should be able to measure all keys being loaded onto any keyring or
onto a specific "keyring=". This shouldn't be any different than any
other policy rule. Once you have this basic feature working, you
would address loading keys during early boot.
>
> Do you want only trusted keyrings to be allowed in the measurement?
> In my opinion, that decision should be deferred to whoever is setting up
> the IMA policy.
Right, but it shouldn't be limited to just "trusted" keyrings. This
way you can first test loading keys onto any keyring.
>
> > Only the queueing code needed for measuring keys on the trusted
> > keyrings would be in a separate file.
> >
>
> The decision to process key immediately or defer (queue) is based on
> whether IMA has been initialized or not. Keyring is not used for this
> decision.
>
> Could you please clarify how queuing is related to keyring's
> trustworthiness?
>
> The check for whether the key is an asymmetric one or not, and
> extracting the "public key" if it is an asymmetric key needs to be in a
> separate file to handle the CONFIG dependencies in IMA.
Queuing the keys should be independent of measuring the keys.
Initially you would start with just measuring the key. From a high
level it would look like:
ima_post_key_create_or_update(...)
{
"measure key based on
policy(key, keyring, ...)"
}
This requires the IMA "keyring=" policy option support be defined
first.
Subsequently you would add key queuing support, and then update
ima_post_key_create_or_update(). It would look like:
ima_post_key_create_or_update(...)
{
if (custom policy is loaded)
"measure key based on policy(key, keyring, ...)"
else
"queue key(key, keyring)"
}
Mimi
^ permalink raw reply
* Re: [RFC PATCH 04/14] pipe: Add O_NOTIFICATION_PIPE [ver #2]
From: David Howells @ 2019-11-07 18:48 UTC (permalink / raw)
To: Andy Lutomirski
Cc: dhowells, Linus Torvalds, Greg Kroah-Hartman, Casey Schaufler,
Stephen Smalley, Nicolas Dichtel, raven, Christian Brauner,
keyrings, USB list, linux-block, LSM List, Linux FS Devel,
Linux API, LKML
In-Reply-To: <CALCETrUka9KaOKFbNKUXcA6XvoFxiXPftctSHtN4DL35Cay61w@mail.gmail.com>
Andy Lutomirski <luto@kernel.org> wrote:
> > Add an O_NOTIFICATION_PIPE flag that can be passed to pipe2() to indicate
> > that the pipe being created is going to be used for notifications. This
> > suppresses the use of splice(), vmsplice(), tee() and sendfile() on the
> > pipe as calling iov_iter_revert() on a pipe when a kernel notification
> > message has been inserted into the middle of a multi-buffer splice will be
> > messy.
>
> How messy?
Well, iov_iter_revert() on a pipe iterator simply walks backwards along the
ring discarding the last N contiguous slots (where N is normally the number of
slots that were filled by whatever operation is being reverted).
However, unless the code that transfers stuff into the pipe takes the spinlock
spinlock and disables softirqs for the duration of its ring filling, what were
N contiguous slots may now have kernel notifications interspersed - even if it
has been holding the pipe mutex.
So, now what do you do? You have to free up just the buffers relevant to the
iterator and then you can either compact down the ring to free up the space or
you can leave null slots and let the read side clean them up, thereby
reducing the capacity of the pipe temporarily.
Either way, iov_iter_revert() gets more complex and has to hold the spinlock.
And if you don't take the spinlock whilst you're reverting, more notifications
can come in to make your life more interesting.
There's also a problem with splicing out from a notification pipe that the
messages are scribed onto preallocated buffers, but now the buffers need
refcounts and, in any case, are of limited quantity.
> And is there some way to make it impossible for this to happen?
Yes. That's what I'm doing by declaring the pipe to be unspliceable up front.
> Adding a new flag to pipe2() to avoid messy kernel code seems
> like a poor tradeoff.
By far the easiest place to check whether a pipe can be spliced to is in
get_pipe_info(). That's checking the file anyway. After that, you can't make
the check until the pipe is locked.
Furthermore, if it's not done upfront, the change to the pipe might happen
during a splicing operation that's residing in pipe_wait()... which drops the
pipe mutex.
David
^ permalink raw reply
* Re: [PATCH v4 01/10] IMA: Defined an IMA hook to measure keys on key create or update
From: Lakshmi Ramasubramanian @ 2019-11-07 18:42 UTC (permalink / raw)
To: Mimi Zohar, dhowells, matthewgarrett, sashal, jamorris,
linux-integrity, linux-security-module, keyrings, linux-kernel
In-Reply-To: <1573098037.5028.325.camel@linux.ibm.com>
On 11/6/2019 7:40 PM, Mimi Zohar wrote:
>>> I would move the patch that defines the "keyring=" policy option prior
>>> to this one. Include the call to process_buffer_measurement() in this
>>> patch. A subsequent patch would add support to defer measuring the
>>> key, by calling a function named something like
>>> ima_queue_key_measurement().
>>>
>>> Mimi
>>
>> As I'd stated in the other response, I wanted to isolate all key related
>> code in a separate C file and build it if and only if all CONFIG
>> dependencies are met.
>
> The basic measuring of keys shouldn't be any different than any other
> policy rule, other than it is a key and not a file. This is the
> reason that I keep saying start out with the basics and then add
> support to defer measuring keys on the trusted keyrings.
I'll make the changes, rearrange the patches and send an updated set.
I do have a few questions since I am still not fully understanding the
requirements you are targeting. Appreciate if you could please clarify.
As you already know, I am using the "public key" of the given asymmetric
key as the "buffer" to measure in process_buffer_measurement().
The measurement decision is not based on whether the keyring is a
trusted one or an untrusted one. As long as the IMA policy allows
(through the "keyrings=" option) the key will be measured.
Do you want only trusted keyrings to be allowed in the measurement?
In my opinion, that decision should be deferred to whoever is setting up
the IMA policy.
> Only the queueing code needed for measuring keys on the trusted
> keyrings would be in a separate file.
>
> Mimi
The decision to process key immediately or defer (queue) is based on
whether IMA has been initialized or not. Keyring is not used for this
decision.
Could you please clarify how queuing is related to keyring's
trustworthiness?
The check for whether the key is an asymmetric one or not, and
extracting the "public key" if it is an asymmetric key needs to be in a
separate file to handle the CONFIG dependencies in IMA.
thanks,
-lakshmi
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox