* [PATCH v2 0/3] IMA: Infrastructure for measurement of critical kernel data
@ 2020-08-21 18:21 Tushar Sugandhi
2020-08-21 18:21 ` [PATCH v2 1/3] IMA: generalize keyring specific measurement constructs Tushar Sugandhi
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Tushar Sugandhi @ 2020-08-21 18:21 UTC (permalink / raw)
To: zohar, stephen.smalley.work, casey, agk, snitzer, gmazyland
Cc: tyhicks, sashal, jmorris, nramas, linux-integrity, selinux,
linux-security-module, linux-kernel, dm-devel
There are several kernel components that contain critical data which if
accidentally or maliciously altered, can compromise the security of the
kernel. Example of such components would include LSMs like SELinux, or
AppArmor; or device-mapper targets like dm-crypt, dm-verity etc.
Many of these components do not use the capabilities provided by kernel
integrity subsystem (IMA), and thus they don't use the benefits of
extended TPM PCR quotes and ultimately the benefits of remote attestation.
This series bridges this gap, so that potential kernel components that
contain data critical to the security of the kernel could take advantage
of IMA's measuring and quoting abilities - thus ultimately enabling
remote attestation for their specific data.
System administrators may want to pick and choose which kernel
components they would want to enable for measurements, quoting, and
remote attestation. To enable that, a new IMA policy is introduced.
And lastly, the functionality is exposed through a function
ima_measure_critical_data(). The functionality is generic enough to
measure the data of any kernel component at run-time.
This series is based on the following repo/branch:
repo: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git
branch: next-integrity
This series also has a dependency on the following patch series:
https://patchwork.kernel.org/patch/11709527/
Change Log v2:
- Reverted the unnecessary indentations in existing #define.
- Updated the description to replace the word 'enlightened' with
'supported'.
- Reverted the unnecessary rename of attribute size to buf_len.
- Introduced a boolean parameter measure_buf_hash as per community
feedback to support measuring hash of the buffer, instead of the
buffer itself.
Tushar Sugandhi (3):
IMA: generalize keyring specific measurement constructs
IMA: add policy to support measuring critical data from kernel
components
IMA: define IMA hook to measure critical data from kernel components
Documentation/ABI/testing/ima_policy | 6 +-
include/linux/ima.h | 11 ++
security/integrity/ima/ima.h | 12 ++-
security/integrity/ima/ima_api.c | 8 +-
security/integrity/ima/ima_appraise.c | 2 +-
security/integrity/ima/ima_asymmetric_keys.c | 2 +-
security/integrity/ima/ima_main.c | 69 +++++++++++--
security/integrity/ima/ima_policy.c | 100 +++++++++++++++----
security/integrity/ima/ima_queue_keys.c | 3 +-
9 files changed, 167 insertions(+), 46 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/3] IMA: generalize keyring specific measurement constructs
2020-08-21 18:21 [PATCH v2 0/3] IMA: Infrastructure for measurement of critical kernel data Tushar Sugandhi
@ 2020-08-21 18:21 ` Tushar Sugandhi
2020-08-21 18:21 ` [PATCH v2 2/3] IMA: add policy to support measuring critical data from kernel components Tushar Sugandhi
2020-08-21 18:21 ` [PATCH v2 3/3] IMA: define IMA hook to measure " Tushar Sugandhi
2 siblings, 0 replies; 8+ messages in thread
From: Tushar Sugandhi @ 2020-08-21 18:21 UTC (permalink / raw)
To: zohar, stephen.smalley.work, casey, agk, snitzer, gmazyland
Cc: tyhicks, sashal, jmorris, nramas, linux-integrity, selinux,
linux-security-module, linux-kernel, dm-devel
IMA functions such as ima_match_keyring(), process_buffer_measurement(),
ima_match_policy() etc. handle data specific to keyrings. Currently,
these constructs are not generic to handle any func specific data.
This makes it harder to extend without code duplication.
Refactor the keyring specific measurement constructs to be generic and
reusable in other measurement scenarios.
Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
security/integrity/ima/ima.h | 6 ++---
security/integrity/ima/ima_api.c | 6 ++---
security/integrity/ima/ima_main.c | 6 ++---
security/integrity/ima/ima_policy.c | 42 ++++++++++++++++-------------
4 files changed, 33 insertions(+), 27 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 38043074ce5e..8875085db689 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -255,7 +255,7 @@ static inline void ima_process_queued_keys(void) {}
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,
- const char *keyring);
+ const char *func_data);
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,
@@ -267,7 +267,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
struct ima_template_desc *template_desc);
void process_buffer_measurement(struct inode *inode, const void *buf, int size,
const char *eventname, enum ima_hooks func,
- int pcr, const char *keyring);
+ int pcr, const char *func_data);
void ima_audit_measurement(struct integrity_iint_cache *iint,
const unsigned char *filename);
int ima_alloc_init_template(struct ima_event_data *event_data,
@@ -283,7 +283,7 @@ const char *ima_d_path(const struct path *path, char **pathbuf, char *filename);
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,
- const char *keyring);
+ const char *func_data);
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 4f39fb93f278..af218babd198 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -170,7 +170,7 @@ 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=
- * @keyring: keyring name used to determine the action
+ * @func_data: private data specific to @func, can be NULL.
*
* The policy is defined in terms of keypairs:
* subj=, obj=, type=, func=, mask=, fsmagic=
@@ -186,14 +186,14 @@ 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,
- const char *keyring)
+ const char *func_data)
{
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, keyring);
+ template_desc, func_data);
}
/*
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8a91711ca79b..c870fd6d2f83 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -732,13 +732,13 @@ 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 name to determine the action to be performed
+ * @func_data: private data specific to @func, can be NULL.
*
* Based on policy, the buffer is measured into the ima log.
*/
void process_buffer_measurement(struct inode *inode, const void *buf, int size,
const char *eventname, enum ima_hooks func,
- int pcr, const char *keyring)
+ int pcr, const char *func_data)
{
int ret = 0;
const char *audit_cause = "ENOMEM";
@@ -770,7 +770,7 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size,
if (func) {
security_task_getsecid(current, &secid);
action = ima_get_action(inode, current_cred(), secid, 0, func,
- &pcr, &template, keyring);
+ &pcr, &template, func_data);
if (!(action & IMA_MEASURE))
return;
}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index fe1df373c113..8866e84d0062 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -451,15 +451,21 @@ int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
}
/**
- * ima_match_keyring - determine whether the keyring matches the measure rule
- * @rule: a pointer to a rule
- * @keyring: name of the keyring to match against the measure rule
+ * ima_match_rule_data - determine whether the given func_data matches
+ * the measure rule data
+ * @rule: IMA policy rule
+ * @opt_list: rule data to match func_data against
+ * @func_data: data to match against the measure rule data
+ * @allow_empty_opt_list: If true matches all func_data
* @cred: a pointer to a credentials structure for user validation
*
- * Returns true if keyring matches one in the rule, false otherwise.
+ * Returns true if func_data matches one in the rule, false otherwise.
*/
-static bool ima_match_keyring(struct ima_rule_entry *rule,
- const char *keyring, const struct cred *cred)
+static bool ima_match_rule_data(struct ima_rule_entry *rule,
+ const struct ima_rule_opt_list *opt_list,
+ const char *func_data,
+ bool allow_empty_opt_list,
+ const struct cred *cred)
{
bool matched = false;
size_t i;
@@ -467,14 +473,14 @@ static bool ima_match_keyring(struct ima_rule_entry *rule,
if ((rule->flags & IMA_UID) && !rule->uid_op(cred->uid, rule->uid))
return false;
- if (!rule->keyrings)
- return true;
+ if (!opt_list)
+ return allow_empty_opt_list;
- if (!keyring)
+ if (!func_data)
return false;
- for (i = 0; i < rule->keyrings->count; i++) {
- if (!strcmp(rule->keyrings->items[i], keyring)) {
+ for (i = 0; i < opt_list->count; i++) {
+ if (!strcmp(opt_list->items[i], func_data)) {
matched = true;
break;
}
@@ -491,20 +497,21 @@ static bool ima_match_keyring(struct ima_rule_entry *rule,
* @secid: the secid of the task to be validated
* @func: LIM hook identifier
* @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC)
- * @keyring: keyring name to check in policy for KEY_CHECK func
+ * @func_data: private data specific to @func, can be NULL.
*
* Returns true on rule match, false on failure.
*/
static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
const struct cred *cred, u32 secid,
enum ima_hooks func, int mask,
- const char *keyring)
+ const char *func_data)
{
int i;
if (func == KEY_CHECK) {
return (rule->flags & IMA_FUNC) && (rule->func == func) &&
- ima_match_keyring(rule, keyring, cred);
+ ima_match_rule_data(rule, rule->keyrings, func_data,
+ true, cred);
}
if ((rule->flags & IMA_FUNC) &&
(rule->func != func && func != POST_SETATTR))
@@ -608,8 +615,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
- * @keyring: the keyring name, if given, to be used to check in the policy.
- * keyring can be NULL if func is anything other than KEY_CHECK.
+ * @func_data: private data specific to @func, can be NULL.
*
* Measure decision based on func/mask/fsmagic and LSM(subj/obj/type)
* conditions.
@@ -621,7 +627,7 @@ 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,
- const char *keyring)
+ const char *func_data)
{
struct ima_rule_entry *entry;
int action = 0, actmask = flags | (flags << 1);
@@ -636,7 +642,7 @@ int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
continue;
if (!ima_match_rules(entry, inode, cred, secid, func, mask,
- keyring))
+ func_data))
continue;
action |= entry->flags & IMA_ACTION_FLAGS;
--
2.17.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/3] IMA: add policy to support measuring critical data from kernel components
2020-08-21 18:21 [PATCH v2 0/3] IMA: Infrastructure for measurement of critical kernel data Tushar Sugandhi
2020-08-21 18:21 ` [PATCH v2 1/3] IMA: generalize keyring specific measurement constructs Tushar Sugandhi
@ 2020-08-21 18:21 ` Tushar Sugandhi
2020-08-24 22:46 ` Mimi Zohar
2020-08-21 18:21 ` [PATCH v2 3/3] IMA: define IMA hook to measure " Tushar Sugandhi
2 siblings, 1 reply; 8+ messages in thread
From: Tushar Sugandhi @ 2020-08-21 18:21 UTC (permalink / raw)
To: zohar, stephen.smalley.work, casey, agk, snitzer, gmazyland
Cc: tyhicks, sashal, jmorris, nramas, linux-integrity, selinux,
linux-security-module, linux-kernel, dm-devel
There would be several candidate kernel components suitable for IMA
measurement. Not all of them would have support for IMA measurement.
Also, system administrators may not want to measure data for all of
them, even when they support IMA measurement. An IMA policy specific
to various kernel components is needed to measure their respective
critical data.
Add a new IMA policy CRITICAL_DATA+data_sources to support measuring
various critical kernel components. This policy would enable the
system administrators to limit the measurement to the components,
if the components support IMA measurement.
Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
Documentation/ABI/testing/ima_policy | 6 ++-
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_api.c | 2 +-
security/integrity/ima/ima_policy.c | 62 +++++++++++++++++++++++++---
4 files changed, 63 insertions(+), 8 deletions(-)
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index cd572912c593..a0dd0f108555 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -29,7 +29,7 @@ Description:
base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
[FIRMWARE_CHECK]
[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
- [KEXEC_CMDLINE] [KEY_CHECK]
+ [KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA]
mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
[[^]MAY_EXEC]
fsmagic:= hex value
@@ -125,3 +125,7 @@ Description:
keys added to .builtin_trusted_keys or .ima keyring:
measure func=KEY_CHECK keyrings=.builtin_trusted_keys|.ima
+
+ Example of measure rule using CRITICAL_DATA to measure critical data
+
+ measure func=CRITICAL_DATA data_sources=selinux|apparmor|dm-crypt
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 8875085db689..0f4209a92bfb 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -200,6 +200,7 @@ static inline unsigned int ima_hash_key(u8 *digest)
hook(POLICY_CHECK, policy) \
hook(KEXEC_CMDLINE, kexec_cmdline) \
hook(KEY_CHECK, key) \
+ hook(CRITICAL_DATA, critical_data) \
hook(MAX_CHECK, none)
#define __ima_hook_enumify(ENUM, str) ENUM,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index af218babd198..9917e1730cb6 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -176,7 +176,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
* 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 | KEY_CHECK
+ * | KEXEC_CMDLINE | KEY_CHECK | CRITICAL_DATA
* mask: contains the permission mask
* fsmagic: hex value
*
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 8866e84d0062..7b649095ac7a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -33,6 +33,7 @@
#define IMA_PCR 0x0100
#define IMA_FSNAME 0x0200
#define IMA_KEYRINGS 0x0400
+#define IMA_DATA_SOURCES 0x0800
#define UNKNOWN 0
#define MEASURE 0x0001 /* same as IMA_MEASURE */
@@ -84,6 +85,7 @@ struct ima_rule_entry {
} lsm[MAX_LSM_RULES];
char *fsname;
struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyrings */
+ struct ima_rule_opt_list *data_sources; /* Measure data from these sources */
struct ima_template_desc *template;
};
@@ -508,14 +510,23 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
{
int i;
- if (func == KEY_CHECK) {
- return (rule->flags & IMA_FUNC) && (rule->func == func) &&
- ima_match_rule_data(rule, rule->keyrings, func_data,
- true, cred);
- }
if ((rule->flags & IMA_FUNC) &&
(rule->func != func && func != POST_SETATTR))
return false;
+
+ switch (func) {
+ case KEY_CHECK:
+ return ((rule->func == func) &&
+ ima_match_rule_data(rule, rule->keyrings,
+ func_data, true, cred));
+ case CRITICAL_DATA:
+ return ((rule->func == func) &&
+ ima_match_rule_data(rule, rule->data_sources,
+ func_data, false, cred));
+ default:
+ break;
+ }
+
if ((rule->flags & IMA_MASK) &&
(rule->mask != mask && func != POST_SETATTR))
return false;
@@ -911,7 +922,7 @@ enum {
Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
Opt_appraise_type, Opt_appraise_flag,
Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
- Opt_err
+ Opt_data_sources, Opt_err
};
static const match_table_t policy_tokens = {
@@ -948,6 +959,7 @@ static const match_table_t policy_tokens = {
{Opt_pcr, "pcr=%s"},
{Opt_template, "template=%s"},
{Opt_keyrings, "keyrings=%s"},
+ {Opt_data_sources, "data_sources=%s"},
{Opt_err, NULL}
};
@@ -1110,6 +1122,19 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
if (ima_rule_contains_lsm_cond(entry))
return false;
+ break;
+ case CRITICAL_DATA:
+ if (entry->action & ~(MEASURE | DONT_MEASURE))
+ return false;
+
+ if (!(entry->flags & IMA_DATA_SOURCES) ||
+ (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR |
+ IMA_DATA_SOURCES)))
+ return false;
+
+ if (ima_rule_contains_lsm_cond(entry))
+ return false;
+
break;
default:
return false;
@@ -1242,6 +1267,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
else if (IS_ENABLED(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) &&
strcmp(args[0].from, "KEY_CHECK") == 0)
entry->func = KEY_CHECK;
+ else if (strcmp(args[0].from, "CRITICAL_DATA") == 0)
+ entry->func = CRITICAL_DATA;
else
result = -EINVAL;
if (!result)
@@ -1312,6 +1339,23 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
entry->flags |= IMA_KEYRINGS;
break;
+ case Opt_data_sources:
+ ima_log_string(ab, "data_sources", args[0].from);
+
+ if (entry->data_sources) {
+ result = -EINVAL;
+ break;
+ }
+
+ entry->data_sources = ima_alloc_rule_opt_list(args);
+ if (IS_ERR(entry->data_sources)) {
+ result = PTR_ERR(entry->data_sources);
+ entry->data_sources = NULL;
+ break;
+ }
+
+ entry->flags |= IMA_DATA_SOURCES;
+ break;
case Opt_fsuuid:
ima_log_string(ab, "fsuuid", args[0].from);
@@ -1692,6 +1736,12 @@ int ima_policy_show(struct seq_file *m, void *v)
seq_puts(m, " ");
}
+ if (entry->flags & IMA_DATA_SOURCES) {
+ seq_puts(m, "data_sources=");
+ ima_show_rule_opt_list(m, entry->data_sources);
+ 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 [flat|nested] 8+ messages in thread
* [PATCH v2 3/3] IMA: define IMA hook to measure critical data from kernel components
2020-08-21 18:21 [PATCH v2 0/3] IMA: Infrastructure for measurement of critical kernel data Tushar Sugandhi
2020-08-21 18:21 ` [PATCH v2 1/3] IMA: generalize keyring specific measurement constructs Tushar Sugandhi
2020-08-21 18:21 ` [PATCH v2 2/3] IMA: add policy to support measuring critical data from kernel components Tushar Sugandhi
@ 2020-08-21 18:21 ` Tushar Sugandhi
2 siblings, 0 replies; 8+ messages in thread
From: Tushar Sugandhi @ 2020-08-21 18:21 UTC (permalink / raw)
To: zohar, stephen.smalley.work, casey, agk, snitzer, gmazyland
Cc: tyhicks, sashal, jmorris, nramas, linux-integrity, selinux,
linux-security-module, linux-kernel, dm-devel
Currently, IMA does not provide a generic function to kernel components
to measure their data. A generic function provided by IMA would
enable various parts of the kernel with easier and faster on-boarding to
use IMA infrastructure, would avoid code duplication, and consistent
usage of IMA policy CRITICAL_DATA+data_sources across the kernel.
Define a generic IMA function ima_measure_critical_data() to measure
data from various kernel components. Limit the measurement to the
components that are specified in the IMA policy -
CRITICAL_DATA+data_sources.
Update process_buffer_measurement() to return the status code of the
operation.
Introduce a boolean parameter measure_buf_hash to support measuring
hash of a buffer, instead of the buffer itself. This is useful when
the buffer being measured is too large, which may result in bloated
IMA logs.
Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
include/linux/ima.h | 11 ++++
security/integrity/ima/ima.h | 7 ++-
security/integrity/ima/ima_appraise.c | 2 +-
security/integrity/ima/ima_asymmetric_keys.c | 2 +-
security/integrity/ima/ima_main.c | 65 +++++++++++++++++---
security/integrity/ima/ima_queue_keys.c | 3 +-
6 files changed, 75 insertions(+), 15 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index d15100de6cdd..136fc02580db 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -26,6 +26,10 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
extern void ima_post_path_mknod(struct dentry *dentry);
extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
+extern int ima_measure_critical_data(const char *event_name,
+ const char *event_data_source,
+ const void *buf, int buf_len,
+ bool measure_buf_hash);
#ifdef CONFIG_IMA_KEXEC
extern void ima_add_kexec_buffer(struct kimage *image);
@@ -104,6 +108,13 @@ static inline int ima_file_hash(struct file *file, char *buf, size_t buf_size)
}
static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {}
+static inline int ima_measure_critical_data(const char *event_name,
+ const char *event_data_source,
+ const void *buf, int buf_len,
+ bool measure_buf_hash)
+{
+ return -EOPNOTSUPP;
+}
#endif /* CONFIG_IMA */
#ifndef CONFIG_IMA_KEXEC
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 0f4209a92bfb..00b84052c8f1 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -266,9 +266,10 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
struct evm_ima_xattr_data *xattr_value,
int xattr_len, const struct modsig *modsig, int pcr,
struct ima_template_desc *template_desc);
-void process_buffer_measurement(struct inode *inode, const void *buf, int size,
- const char *eventname, enum ima_hooks func,
- int pcr, const char *func_data);
+int process_buffer_measurement(struct inode *inode, const void *buf, int size,
+ const char *eventname, enum ima_hooks func,
+ int pcr, const char *func_data,
+ bool measure_buf_hash);
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 372d16382960..20adffe5bf58 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -336,7 +336,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
process_buffer_measurement(NULL, digest, digestsize,
"blacklisted-hash", NONE,
- pcr, NULL);
+ pcr, NULL, false);
}
return rc;
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index 1c68c500c26f..a74095793936 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -60,5 +60,5 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
*/
process_buffer_measurement(NULL, payload, payload_len,
keyring->description, KEY_CHECK, 0,
- keyring->description);
+ keyring->description, false);
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index c870fd6d2f83..a889bf40cb7e 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -733,17 +733,21 @@ int ima_load_data(enum kernel_load_data_id id)
* @func: IMA hook
* @pcr: pcr to extend the measurement
* @func_data: private data specific to @func, can be NULL.
+ * @measure_buf_hash: if set to true - will measure hash of the buf,
+ * instead of buf
*
* Based on policy, the buffer is measured into the ima log.
*/
-void process_buffer_measurement(struct inode *inode, const void *buf, int size,
- const char *eventname, enum ima_hooks func,
- int pcr, const char *func_data)
+int process_buffer_measurement(struct inode *inode, const void *buf, int size,
+ const char *eventname, enum ima_hooks func,
+ int pcr, const char *func_data,
+ bool measure_buf_hash)
{
int ret = 0;
const char *audit_cause = "ENOMEM";
struct ima_template_entry *entry = NULL;
struct integrity_iint_cache iint = {};
+ struct integrity_iint_cache digest_iint = {};
struct ima_event_data event_data = {.iint = &iint,
.filename = eventname,
.buf = buf,
@@ -752,13 +756,13 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size,
struct {
struct ima_digest_data hdr;
char digest[IMA_MAX_DIGEST_SIZE];
- } hash = {};
+ } hash = {}, digest_hash = {};
int violation = 0;
int action = 0;
u32 secid;
if (!ima_policy_flag)
- return;
+ return 0;
/*
* Both LSM hooks and auxilary based buffer measurements are
@@ -772,7 +776,7 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size,
action = ima_get_action(inode, current_cred(), secid, 0, func,
&pcr, &template, func_data);
if (!(action & IMA_MEASURE))
- return;
+ return 0;
}
if (!pcr)
@@ -787,7 +791,7 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size,
pr_err("template %s init failed, result: %d\n",
(strlen(template->name) ?
template->name : template->fmt), ret);
- return;
+ return ret;
}
}
@@ -801,6 +805,24 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size,
goto out;
}
+ if (measure_buf_hash) {
+ digest_iint.ima_hash = &digest_hash.hdr;
+ digest_iint.ima_hash->algo = ima_hash_algo;
+ digest_iint.ima_hash->length = hash_digest_size[ima_hash_algo];
+
+ ret = ima_calc_buffer_hash(hash.hdr.digest,
+ iint.ima_hash->length,
+ digest_iint.ima_hash);
+ if (ret < 0) {
+ audit_cause = "digest_hashing_error";
+ goto out;
+ }
+
+ event_data.iint = &digest_iint;
+ event_data.buf = hash.hdr.digest;
+ event_data.buf_len = iint.ima_hash->length;
+ }
+
ret = ima_alloc_init_template(&event_data, &entry, template);
if (ret < 0) {
audit_cause = "alloc_entry";
@@ -819,7 +841,7 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size,
func_measure_str(func),
audit_cause, ret, 0, ret);
- return;
+ return ret;
}
/**
@@ -842,10 +864,35 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
return;
process_buffer_measurement(file_inode(f.file), buf, size,
- "kexec-cmdline", KEXEC_CMDLINE, 0, NULL);
+ "kexec-cmdline", KEXEC_CMDLINE, 0, NULL,
+ false);
fdput(f);
}
+/**
+ * ima_measure_critical_data - measure critical data
+ * @event_name: name for the given data
+ * @event_data_source: name of the event data source
+ * @buf: pointer to buffer containing data to measure
+ * @buf_len: length of buffer(in bytes)
+ * @measure_buf_hash: if set to true - will measure hash of the buf,
+ * instead of buf
+ *
+ * Buffers can only be measured, not appraised.
+ */
+int ima_measure_critical_data(const char *event_name,
+ const char *event_data_source,
+ const void *buf, int buf_len,
+ bool measure_buf_hash)
+{
+ if (!event_name || !event_data_source || !buf || !buf_len)
+ return -EINVAL;
+
+ return process_buffer_measurement(NULL, buf, buf_len, event_name,
+ CRITICAL_DATA, 0, event_data_source,
+ measure_buf_hash);
+}
+
static int __init init_ima(void)
{
int error;
diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
index 69a8626a35c0..c2f2ad34f9b7 100644
--- a/security/integrity/ima/ima_queue_keys.c
+++ b/security/integrity/ima/ima_queue_keys.c
@@ -162,7 +162,8 @@ void ima_process_queued_keys(void)
entry->payload_len,
entry->keyring_name,
KEY_CHECK, 0,
- entry->keyring_name);
+ entry->keyring_name,
+ false);
list_del(&entry->list);
ima_free_key_entry(entry);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/3] IMA: add policy to support measuring critical data from kernel components
2020-08-21 18:21 ` [PATCH v2 2/3] IMA: add policy to support measuring critical data from kernel components Tushar Sugandhi
@ 2020-08-24 22:46 ` Mimi Zohar
2020-08-25 17:32 ` Tushar Sugandhi
0 siblings, 1 reply; 8+ messages in thread
From: Mimi Zohar @ 2020-08-24 22:46 UTC (permalink / raw)
To: Tushar Sugandhi, stephen.smalley.work, casey, agk, snitzer, gmazyland
Cc: tyhicks, sashal, jmorris, nramas, linux-integrity, selinux,
linux-security-module, linux-kernel, dm-devel
On Fri, 2020-08-21 at 11:21 -0700, Tushar Sugandhi wrote:
> There would be several candidate kernel components suitable for IMA
> measurement. Not all of them would have support for IMA measurement.
> Also, system administrators may not want to measure data for all of
> them, even when they support IMA measurement. An IMA policy specific
> to various kernel components is needed to measure their respective
> critical data.
>
> Add a new IMA policy CRITICAL_DATA+data_sources to support measuring
> various critical kernel components. This policy would enable the
> system administrators to limit the measurement to the components,
> if the components support IMA measurement.
>
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
> Documentation/ABI/testing/ima_policy | 6 ++-
> security/integrity/ima/ima.h | 1 +
> security/integrity/ima/ima_api.c | 2 +-
> security/integrity/ima/ima_policy.c | 62 +++++++++++++++++++++++++---
> 4 files changed, 63 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index cd572912c593..a0dd0f108555 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -29,7 +29,7 @@ Description:
> base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
> [FIRMWARE_CHECK]
> [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
> - [KEXEC_CMDLINE] [KEY_CHECK]
> + [KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA]
> mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
> [[^]MAY_EXEC]
> fsmagic:= hex value
> @@ -125,3 +125,7 @@ Description:
> keys added to .builtin_trusted_keys or .ima keyring:
>
> measure func=KEY_CHECK keyrings=.builtin_trusted_keys|.ima
> +
> + Example of measure rule using CRITICAL_DATA to measure critical data
> +
> + measure func=CRITICAL_DATA data_sources=selinux|apparmor|dm-crypt
This example uses "data_sources" without first defining it in the
"option:" section. Defining two new options is an indication that this
patch should be split up. One which defines the "CRITICAL_DATA" and
another one which defines the new key value pair. The term
"data_sources" is pretty generic. Perhaps constrain it a bit by re-
naming it "critical_data=". Or was such using a generic name
intentional?
Normally "CRITICAL_DATA" would be defined with the critical data hook,
but that seems to be defined in patch 3/3 "IMA: define IMA hook to
measure critical data from kernel components".
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 8875085db689..0f4209a92bfb 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -200,6 +200,7 @@ static inline unsigned int ima_hash_key(u8 *digest)
> hook(POLICY_CHECK, policy) \
> hook(KEXEC_CMDLINE, kexec_cmdline) \
> hook(KEY_CHECK, key) \
> + hook(CRITICAL_DATA, critical_data) \
> hook(MAX_CHECK, none)
>
> #define __ima_hook_enumify(ENUM, str) ENUM,
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index af218babd198..9917e1730cb6 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -176,7 +176,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
> * 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 | KEY_CHECK
> + * | KEXEC_CMDLINE | KEY_CHECK | CRITICAL_DATA
> * mask: contains the permission mask
> * fsmagic: hex value
> *
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 8866e84d0062..7b649095ac7a 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -33,6 +33,7 @@
> #define IMA_PCR 0x0100
> #define IMA_FSNAME 0x0200
> #define IMA_KEYRINGS 0x0400
> +#define IMA_DATA_SOURCES 0x0800
>
> #define UNKNOWN 0
> #define MEASURE 0x0001 /* same as IMA_MEASURE */
> @@ -84,6 +85,7 @@ struct ima_rule_entry {
> } lsm[MAX_LSM_RULES];
> char *fsname;
> struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyrings */
> + struct ima_rule_opt_list *data_sources; /* Measure data from these sources */
> struct ima_template_desc *template;
> };
>
> @@ -508,14 +510,23 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> {
> int i;
>
> - if (func == KEY_CHECK) {
> - return (rule->flags & IMA_FUNC) && (rule->func == func) &&
> - ima_match_rule_data(rule, rule->keyrings, func_data,
> - true, cred);
> - }
> if ((rule->flags & IMA_FUNC) &&
> (rule->func != func && func != POST_SETATTR))
> return false;
> +
> + switch (func) {
> + case KEY_CHECK:
> + return ((rule->func == func) &&
> + ima_match_rule_data(rule, rule->keyrings,
> + func_data, true, cred));
> + case CRITICAL_DATA:
> + return ((rule->func == func) &&
> + ima_match_rule_data(rule, rule->data_sources,
> + func_data, false, cred));
> + default:
> + break;
> + }
> +
> if ((rule->flags & IMA_MASK) &&
> (rule->mask != mask && func != POST_SETATTR))
> return false;
> @@ -911,7 +922,7 @@ enum {
> Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
> Opt_appraise_type, Opt_appraise_flag,
> Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
> - Opt_err
> + Opt_data_sources, Opt_err
> };
>
> static const match_table_t policy_tokens = {
> @@ -948,6 +959,7 @@ static const match_table_t policy_tokens = {
> {Opt_pcr, "pcr=%s"},
> {Opt_template, "template=%s"},
> {Opt_keyrings, "keyrings=%s"},
> + {Opt_data_sources, "data_sources=%s"},
> {Opt_err, NULL}
> };
>
> @@ -1110,6 +1122,19 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> if (ima_rule_contains_lsm_cond(entry))
> return false;
>
> + break;
> + case CRITICAL_DATA:
> + if (entry->action & ~(MEASURE | DONT_MEASURE))
> + return false;
> +
> + if (!(entry->flags & IMA_DATA_SOURCES) ||
> + (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR |
> + IMA_DATA_SOURCES)))
> + return false;
Requiring IMA_FUNC and IMA_DATA_SOURCES makes sense, but why are
IMA_UID and IMA_PCR required?
> +
> + if (ima_rule_contains_lsm_cond(entry))
> + return false;
> +
> break;
> default:
> return false;
> @@ -1242,6 +1267,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> else if (IS_ENABLED(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) &&
> strcmp(args[0].from, "KEY_CHECK") == 0)
> entry->func = KEY_CHECK;
> + else if (strcmp(args[0].from, "CRITICAL_DATA") == 0)
> + entry->func = CRITICAL_DATA;
> else
> result = -EINVAL;
> if (!result)
> @@ -1312,6 +1339,23 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>
> entry->flags |= IMA_KEYRINGS;
> break;
> + case Opt_data_sources:
> + ima_log_string(ab, "data_sources", args[0].from);
> +
> + if (entry->data_sources) {
> + result = -EINVAL;
> + break;
> + }
> +
> + entry->data_sources = ima_alloc_rule_opt_list(args);
> + if (IS_ERR(entry->data_sources)) {
> + result = PTR_ERR(entry->data_sources);
> + entry->data_sources = NULL;
> + break;
> + }
> +
"keyrings=" isn't bounded because keyrings can be created by userspace.
Perhaps keyring names has a minimum/maximum length. IMA isn't
measuring userspace construsts. Shouldn't the list of critical data
being measured be bounded and verified?
Mimi
> + entry->flags |= IMA_DATA_SOURCES;
> + break;
> case Opt_fsuuid:
> ima_log_string(ab, "fsuuid", args[0].from);
>
> @@ -1692,6 +1736,12 @@ int ima_policy_show(struct seq_file *m, void *v)
> seq_puts(m, " ");
> }
>
> + if (entry->flags & IMA_DATA_SOURCES) {
> + seq_puts(m, "data_sources=");
> + ima_show_rule_opt_list(m, entry->data_sources);
> + seq_puts(m, " ");
> + }
> +
> if (entry->flags & IMA_PCR) {
> snprintf(tbuf, sizeof(tbuf), "%d", entry->pcr);
> seq_printf(m, pt(Opt_pcr), tbuf);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/3] IMA: add policy to support measuring critical data from kernel components
2020-08-24 22:46 ` Mimi Zohar
@ 2020-08-25 17:32 ` Tushar Sugandhi
2020-08-25 20:43 ` Mimi Zohar
0 siblings, 1 reply; 8+ messages in thread
From: Tushar Sugandhi @ 2020-08-25 17:32 UTC (permalink / raw)
To: Mimi Zohar, stephen.smalley.work, casey, agk, snitzer, gmazyland
Cc: tyhicks, sashal, jmorris, nramas, linux-integrity, selinux,
linux-security-module, linux-kernel, dm-devel
On 2020-08-24 3:46 p.m., Mimi Zohar wrote:
> On Fri, 2020-08-21 at 11:21 -0700, Tushar Sugandhi wrote:
>> There would be several candidate kernel components suitable for IMA
>> measurement. Not all of them would have support for IMA measurement.
>> Also, system administrators may not want to measure data for all of
>> them, even when they support IMA measurement. An IMA policy specific
>> to various kernel components is needed to measure their respective
>> critical data.
>>
>> Add a new IMA policy CRITICAL_DATA+data_sources to support measuring
>> various critical kernel components. This policy would enable the
>> system administrators to limit the measurement to the components,
>> if the components support IMA measurement.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
>> ---
>> Documentation/ABI/testing/ima_policy | 6 ++-
>> security/integrity/ima/ima.h | 1 +
>> security/integrity/ima/ima_api.c | 2 +-
>> security/integrity/ima/ima_policy.c | 62 +++++++++++++++++++++++++---
>> 4 files changed, 63 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
>> index cd572912c593..a0dd0f108555 100644
>> --- a/Documentation/ABI/testing/ima_policy
>> +++ b/Documentation/ABI/testing/ima_policy
>> @@ -29,7 +29,7 @@ Description:
>> base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
>> [FIRMWARE_CHECK]
>> [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
>> - [KEXEC_CMDLINE] [KEY_CHECK]
>> + [KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA]
>> mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
>> [[^]MAY_EXEC]
>> fsmagic:= hex value
>> @@ -125,3 +125,7 @@ Description:
>> keys added to .builtin_trusted_keys or .ima keyring:
>>
>> measure func=KEY_CHECK keyrings=.builtin_trusted_keys|.ima
>> +
>> + Example of measure rule using CRITICAL_DATA to measure critical data
>> +
>> + measure func=CRITICAL_DATA data_sources=selinux|apparmor|dm-crypt
>
> This example uses "data_sources" without first defining it in the
> "option:" section. Defining two new options is an indication that this
Thanks. I will define "data_sources" first in "option:" section.
> patch should be split up. One which defines the "CRITICAL_DATA" and
> another one which defines the new key value pair. The term
I intentionally kept the "CRITICAL_DATA" and "data_sources" in the same
patch.
CRITICAL_DATA is different than KEY_CHECK because in case of KEY_CHECK,
"keyrings=" is optional. If "keyrings=" is not specified, then we
measure all keyrings.
Where for CRITICAL_DATA, "data_sources=" is mandatory.
Because the data sources would be diverse and orthogonal to each other,
(unlike "keyrings=") - not specifying "data_sources=" shouldn't result
in IMA blindly measuring all data sources.
Since CRITICAL_DATA, and "data_sources=" go hand in hand, I wanted them
to be part of the same patch.
> "data_sources" is pretty generic. Perhaps constrain it a bit by re-
> naming it "critical_data=". Or was such using a generic name
> intentional?
>
We intentionally kept the name generic because the data to be measured
could be coming from any kernel component with any granularity (from a
single bool to megabytes of data). The kernel component is also loosely
defined here. It could be an LSM (like SELinux), or a broader base layer
(like device-mapper), or a specific module (like dm-crypt), or it could
be different parts of a single module.
Also, we didn't want to name "data_sources" as "critical_data" to avoid
confusion with func "CRITICAL_DATA".
> Normally "CRITICAL_DATA" would be defined with the critical data hook,
> but that seems to be defined in patch 3/3 "IMA: define IMA hook to
> measure critical data from kernel components".
>
I can make the "CRITICAL_DATA" and the hook as part of the same patch.
That would mean combining patch 2 and 3 into a single one.
Does it sound ok?
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index 8875085db689..0f4209a92bfb 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -200,6 +200,7 @@ static inline unsigned int ima_hash_key(u8 *digest)
>> hook(POLICY_CHECK, policy) \
>> hook(KEXEC_CMDLINE, kexec_cmdline) \
>> hook(KEY_CHECK, key) \
>> + hook(CRITICAL_DATA, critical_data) \
>> hook(MAX_CHECK, none)
>>
>> #define __ima_hook_enumify(ENUM, str) ENUM,
>> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
>> index af218babd198..9917e1730cb6 100644
>> --- a/security/integrity/ima/ima_api.c
>> +++ b/security/integrity/ima/ima_api.c
>> @@ -176,7 +176,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
>> * 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 | KEY_CHECK
>> + * | KEXEC_CMDLINE | KEY_CHECK | CRITICAL_DATA
>> * mask: contains the permission mask
>> * fsmagic: hex value
>> *
>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>> index 8866e84d0062..7b649095ac7a 100644
>> --- a/security/integrity/ima/ima_policy.c
>> +++ b/security/integrity/ima/ima_policy.c
>> @@ -33,6 +33,7 @@
>> #define IMA_PCR 0x0100
>> #define IMA_FSNAME 0x0200
>> #define IMA_KEYRINGS 0x0400
>> +#define IMA_DATA_SOURCES 0x0800
>>
>> #define UNKNOWN 0
>> #define MEASURE 0x0001 /* same as IMA_MEASURE */
>> @@ -84,6 +85,7 @@ struct ima_rule_entry {
>> } lsm[MAX_LSM_RULES];
>> char *fsname;
>> struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyrings */
>> + struct ima_rule_opt_list *data_sources; /* Measure data from these sources */
>> struct ima_template_desc *template;
>> };
>>
>> @@ -508,14 +510,23 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>> {
>> int i;
>>
>> - if (func == KEY_CHECK) {
>> - return (rule->flags & IMA_FUNC) && (rule->func == func) &&
>> - ima_match_rule_data(rule, rule->keyrings, func_data,
>> - true, cred);
>> - }
>> if ((rule->flags & IMA_FUNC) &&
>> (rule->func != func && func != POST_SETATTR))
>> return false;
>> +
>> + switch (func) {
>> + case KEY_CHECK:
>> + return ((rule->func == func) &&
>> + ima_match_rule_data(rule, rule->keyrings,
>> + func_data, true, cred));
>> + case CRITICAL_DATA:
>> + return ((rule->func == func) &&
>> + ima_match_rule_data(rule, rule->data_sources,
>> + func_data, false, cred));
>> + default:
>> + break;
>> + }
>> +
>> if ((rule->flags & IMA_MASK) &&
>> (rule->mask != mask && func != POST_SETATTR))
>> return false;
>> @@ -911,7 +922,7 @@ enum {
>> Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
>> Opt_appraise_type, Opt_appraise_flag,
>> Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
>> - Opt_err
>> + Opt_data_sources, Opt_err
>> };
>>
>> static const match_table_t policy_tokens = {
>> @@ -948,6 +959,7 @@ static const match_table_t policy_tokens = {
>> {Opt_pcr, "pcr=%s"},
>> {Opt_template, "template=%s"},
>> {Opt_keyrings, "keyrings=%s"},
>> + {Opt_data_sources, "data_sources=%s"},
>> {Opt_err, NULL}
>> };
>>
>> @@ -1110,6 +1122,19 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
>> if (ima_rule_contains_lsm_cond(entry))
>> return false;
>>
>> + break;
>> + case CRITICAL_DATA:
>> + if (entry->action & ~(MEASURE | DONT_MEASURE))
>> + return false;
>> +
>> + if (!(entry->flags & IMA_DATA_SOURCES) ||
>> + (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR |
>> + IMA_DATA_SOURCES)))
>> + return false;
>
> Requiring IMA_FUNC and IMA_DATA_SOURCES makes sense, but why are
> IMA_UID and IMA_PCR required?
>
Since the data to be measured could be for any scenario, I didn't want
to restrict the kernel components from choosing UID to measure the data
for, or restrict them from choosing PCR to store the measurements in.
But as the consumers are kernel components, perhaps support for IMA_UID
is not required. But we should still support IMA_PCR.
Please let me know what do you think, and I can update the logic
accordingly.
>> +
>> + if (ima_rule_contains_lsm_cond(entry))
>> + return false;
>> +
>> break;
>> default:
>> return false;
>> @@ -1242,6 +1267,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>> else if (IS_ENABLED(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) &&
>> strcmp(args[0].from, "KEY_CHECK") == 0)
>> entry->func = KEY_CHECK;
>> + else if (strcmp(args[0].from, "CRITICAL_DATA") == 0)
>> + entry->func = CRITICAL_DATA;
>> else
>> result = -EINVAL;
>> if (!result)
>> @@ -1312,6 +1339,23 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>>
>> entry->flags |= IMA_KEYRINGS;
>> break;
>> + case Opt_data_sources:
>> + ima_log_string(ab, "data_sources", args[0].from);
>> +
>> + if (entry->data_sources) {
>> + result = -EINVAL;
>> + break;
>> + }
>> +
>> + entry->data_sources = ima_alloc_rule_opt_list(args);
>> + if (IS_ERR(entry->data_sources)) {
>> + result = PTR_ERR(entry->data_sources);
>> + entry->data_sources = NULL;
>> + break;
>> + }
>> +
>
> "keyrings=" isn't bounded because keyrings can be created by userspace.
> Perhaps keyring names has a minimum/maximum length. IMA isn't
> measuring userspace construsts. Shouldn't the list of critical data
> being measured be bounded and verified?
The comment is not entirely clear.
Do you mean there should be some sort of allow_list in IMA, against
which the values in "data_sources=" should be vetted? And if the
value is present in the IMA allow_list, then only the measurements for
that data source are allowed?
Or do you mean something else?
~Tushar
>
> Mimi
>
>> + entry->flags |= IMA_DATA_SOURCES;
>> + break;
>> case Opt_fsuuid:
>> ima_log_string(ab, "fsuuid", args[0].from);
>>
>> @@ -1692,6 +1736,12 @@ int ima_policy_show(struct seq_file *m, void *v)
>> seq_puts(m, " ");
>> }
>>
>> + if (entry->flags & IMA_DATA_SOURCES) {
>> + seq_puts(m, "data_sources=");
>> + ima_show_rule_opt_list(m, entry->data_sources);
>> + seq_puts(m, " ");
>> + }
>> +
>> if (entry->flags & IMA_PCR) {
>> snprintf(tbuf, sizeof(tbuf), "%d", entry->pcr);
>> seq_printf(m, pt(Opt_pcr), tbuf);
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/3] IMA: add policy to support measuring critical data from kernel components
2020-08-25 17:32 ` Tushar Sugandhi
@ 2020-08-25 20:43 ` Mimi Zohar
2020-08-25 23:23 ` Tushar Sugandhi
0 siblings, 1 reply; 8+ messages in thread
From: Mimi Zohar @ 2020-08-25 20:43 UTC (permalink / raw)
To: Tushar Sugandhi, stephen.smalley.work, casey, agk, snitzer, gmazyland
Cc: tyhicks, sashal, jmorris, nramas, linux-integrity, selinux,
linux-security-module, linux-kernel, dm-devel
On Tue, 2020-08-25 at 10:32 -0700, Tushar Sugandhi wrote:
>
> On 2020-08-24 3:46 p.m., Mimi Zohar wrote:
> > On Fri, 2020-08-21 at 11:21 -0700, Tushar Sugandhi wrote:
> > > There would be several candidate kernel components suitable for IMA
> > > measurement. Not all of them would have support for IMA measurement.
> > > Also, system administrators may not want to measure data for all of
> > > them, even when they support IMA measurement. An IMA policy specific
> > > to various kernel components is needed to measure their respective
> > > critical data.
> > >
> > > Add a new IMA policy CRITICAL_DATA+data_sources to support measuring
> > > various critical kernel components. This policy would enable the
> > > system administrators to limit the measurement to the components,
> > > if the components support IMA measurement.
> > >
> > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> > > ---
> > > Documentation/ABI/testing/ima_policy | 6 ++-
> > > security/integrity/ima/ima.h | 1 +
> > > security/integrity/ima/ima_api.c | 2 +-
> > > security/integrity/ima/ima_policy.c | 62 +++++++++++++++++++++++++---
> > > 4 files changed, 63 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> > > index cd572912c593..a0dd0f108555 100644
> > > --- a/Documentation/ABI/testing/ima_policy
> > > +++ b/Documentation/ABI/testing/ima_policy
> > > @@ -29,7 +29,7 @@ Description:
> > > base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
> > > [FIRMWARE_CHECK]
> > > [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
> > > - [KEXEC_CMDLINE] [KEY_CHECK]
> > > + [KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA]
> > > mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
> > > [[^]MAY_EXEC]
> > > fsmagic:= hex value
> > > @@ -125,3 +125,7 @@ Description:
> > > keys added to .builtin_trusted_keys or .ima keyring:
> > >
> > > measure func=KEY_CHECK keyrings=.builtin_trusted_keys|.ima
> > > +
> > > + Example of measure rule using CRITICAL_DATA to measure critical data
> > > +
> > > + measure func=CRITICAL_DATA data_sources=selinux|apparmor|dm-crypt
> >
> > This example uses "data_sources" without first defining it in the
> > "option:" section. Defining two new options is an indication that this
> Thanks. I will define "data_sources" first in "option:" section.
> > patch should be split up. One which defines the "CRITICAL_DATA" and
> > another one which defines the new key value pair. The term
> I intentionally kept the "CRITICAL_DATA" and "data_sources" in the same
> patch.
>
> CRITICAL_DATA is different than KEY_CHECK because in case of KEY_CHECK,
> "keyrings=" is optional. If "keyrings=" is not specified, then we
> measure all keyrings.
>
> Where for CRITICAL_DATA, "data_sources=" is mandatory.
>
> Because the data sources would be diverse and orthogonal to each other,
> (unlike "keyrings=") - not specifying "data_sources=" shouldn't result
> in IMA blindly measuring all data sources.
Good point.
>
> Since CRITICAL_DATA, and "data_sources=" go hand in hand, I wanted them
> to be part of the same patch.
Separating them will help clarify the patch description. There's no
harm in defining the critical data source first.
> > "data_sources" is pretty generic. Perhaps constrain it a bit by re-
> > naming it "critical_data=". Or was such using a generic name
> > intentional?
> >
> We intentionally kept the name generic because the data to be measured
> could be coming from any kernel component with any granularity (from a
> single bool to megabytes of data). The kernel component is also loosely
> defined here. It could be an LSM (like SELinux), or a broader base layer
> (like device-mapper), or a specific module (like dm-crypt), or it could
> be different parts of a single module.
>
> Also, we didn't want to name "data_sources" as "critical_data" to avoid
> confusion with func "CRITICAL_DATA".
The point is that you're measuring critical data, not just any data
from any source. Whatever term is used, it needs to be added to the
Documentation/ABI/testing/ima_policy. I think something that is self
describing will help. See what makes the most sense.
> > Normally "CRITICAL_DATA" would be defined with the critical data hook,
> > but that seems to be defined in patch 3/3 "IMA: define IMA hook to
> > measure critical data from kernel components".
> >
> I can make the "CRITICAL_DATA" and the hook as part of the same patch.
> That would mean combining patch 2 and 3 into a single one.
>
> Does it sound ok?
In the other thread, we discussed separating out "measure_payload_hash"from other changes. The end result you want one logical change per patch. Each patch builds upon the previous one. (Look at how Tyler does it.)
> > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > > index 8875085db689..0f4209a92bfb 100644
> > > --- a/security/integrity/ima/ima.h
> > > +++ b/security/integrity/ima/ima.h
> > > @@ -200,6 +200,7 @@ static inline unsigned int ima_hash_key(u8 *digest)
> > > hook(POLICY_CHECK, policy) \
> > > hook(KEXEC_CMDLINE, kexec_cmdline) \
> > > hook(KEY_CHECK, key) \
> > > + hook(CRITICAL_DATA, critical_data) \
> > > hook(MAX_CHECK, none)
> > >
> > > #define __ima_hook_enumify(ENUM, str) ENUM,
> > > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> > > index af218babd198..9917e1730cb6 100644
> > > --- a/security/integrity/ima/ima_api.c
> > > +++ b/security/integrity/ima/ima_api.c
> > > @@ -176,7 +176,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
> > > * 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 | KEY_CHECK
> > > + * | KEXEC_CMDLINE | KEY_CHECK | CRITICAL_DATA
> > > * mask: contains the permission mask
> > > * fsmagic: hex value
> > > *
> > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > > index 8866e84d0062..7b649095ac7a 100644
> > > --- a/security/integrity/ima/ima_policy.c
> > > +++ b/security/integrity/ima/ima_policy.c
> > > @@ -33,6 +33,7 @@
> > > #define IMA_PCR 0x0100
> > > #define IMA_FSNAME 0x0200
> > > #define IMA_KEYRINGS 0x0400
> > > +#define IMA_DATA_SOURCES 0x0800
> > >
> > > #define UNKNOWN 0
> > > #define MEASURE 0x0001 /* same as IMA_MEASURE */
> > > @@ -84,6 +85,7 @@ struct ima_rule_entry {
> > > } lsm[MAX_LSM_RULES];
> > > char *fsname;
> > > struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyrings */
> > > + struct ima_rule_opt_list *data_sources; /* Measure data from these sources */
> > > struct ima_template_desc *template;
> > > };
> > >
> > > @@ -508,14 +510,23 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> > > {
> > > int i;
> > >
> > > - if (func == KEY_CHECK) {
> > > - return (rule->flags & IMA_FUNC) && (rule->func == func) &&
> > > - ima_match_rule_data(rule, rule->keyrings, func_data,
> > > - true, cred);
> > > - }
> > > if ((rule->flags & IMA_FUNC) &&
> > > (rule->func != func && func != POST_SETATTR))
> > > return false;
> > > +
> > > + switch (func) {
> > > + case KEY_CHECK:
> > > + return ((rule->func == func) &&
> > > + ima_match_rule_data(rule, rule->keyrings,
> > > + func_data, true, cred));
> > > + case CRITICAL_DATA:
> > > + return ((rule->func == func) &&
> > > + ima_match_rule_data(rule, rule->data_sources,
> > > + func_data, false, cred));
> > > + default:
> > > + break;
> > > + }
> > > +
> > > if ((rule->flags & IMA_MASK) &&
> > > (rule->mask != mask && func != POST_SETATTR))
> > > return false;
> > > @@ -911,7 +922,7 @@ enum {
> > > Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
> > > Opt_appraise_type, Opt_appraise_flag,
> > > Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
> > > - Opt_err
> > > + Opt_data_sources, Opt_err
> > > };
> > >
> > > static const match_table_t policy_tokens = {
> > > @@ -948,6 +959,7 @@ static const match_table_t policy_tokens = {
> > > {Opt_pcr, "pcr=%s"},
> > > {Opt_template, "template=%s"},
> > > {Opt_keyrings, "keyrings=%s"},
> > > + {Opt_data_sources, "data_sources=%s"},
> > > {Opt_err, NULL}
> > > };
> > >
> > > @@ -1110,6 +1122,19 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> > > if (ima_rule_contains_lsm_cond(entry))
> > > return false;
> > >
> > > + break;
> > > + case CRITICAL_DATA:
> > > + if (entry->action & ~(MEASURE | DONT_MEASURE))
> > > + return false;
> > > +
> > > + if (!(entry->flags & IMA_DATA_SOURCES) ||
> > > + (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR |
> > > + IMA_DATA_SOURCES)))
> > > + return false;
> >
> > Requiring IMA_FUNC and IMA_DATA_SOURCES makes sense, but why are
> > IMA_UID and IMA_PCR required?
> >
> Since the data to be measured could be for any scenario, I didn't want
> to restrict the kernel components from choosing UID to measure the data
> for, or restrict them from choosing PCR to store the measurements in.
> But as the consumers are kernel components, perhaps support for IMA_UID
> is not required. But we should still support IMA_PCR.
> Please let me know what do you think, and I can update the logic
> accordingly.
I think I misinterpreted this code. As long as IMA_UID and IMA_PCR
aren't required, then it is fine.
> > > +
> > > + if (ima_rule_contains_lsm_cond(entry))
> > > + return false;
> > > +
> > > break;
> > > default:
> > > return false;
> > > @@ -1242,6 +1267,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> > > else if (IS_ENABLED(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) &&
> > > strcmp(args[0].from, "KEY_CHECK") == 0)
> > > entry->func = KEY_CHECK;
> > > + else if (strcmp(args[0].from, "CRITICAL_DATA") == 0)
> > > + entry->func = CRITICAL_DATA;
> > > else
> > > result = -EINVAL;
> > > if (!result)
> > > @@ -1312,6 +1339,23 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> > >
> > > entry->flags |= IMA_KEYRINGS;
> > > break;
> > > + case Opt_data_sources:
> > > + ima_log_string(ab, "data_sources", args[0].from);
> > > +
> > > + if (entry->data_sources) {
> > > + result = -EINVAL;
> > > + break;
> > > + }
> > > +
> > > + entry->data_sources = ima_alloc_rule_opt_list(args);
> > > + if (IS_ERR(entry->data_sources)) {
> > > + result = PTR_ERR(entry->data_sources);
> > > + entry->data_sources = NULL;
> > > + break;
> > > + }
> > > +
> >
> > "keyrings=" isn't bounded because keyrings can be created by userspace.
> > Perhaps keyring names has a minimum/maximum length. IMA isn't
> > measuring userspace construsts. Shouldn't the list of critical data
> > being measured be bounded and verified?
> The comment is not entirely clear.
> Do you mean there should be some sort of allow_list in IMA, against
> which the values in "data_sources=" should be vetted? And if the
> value is present in the IMA allow_list, then only the measurements for
> that data source are allowed?
>
> Or do you mean something else?
Yes, something along those lines. Does the list of critical data need
to be vetted? And if so, against what?
Mimi
> >
> > > + entry->flags |= IMA_DATA_SOURCES;
> > > + break;
> > > case Opt_fsuuid:
> > > ima_log_string(ab, "fsuuid", args[0].from);
> > >
> > > @@ -1692,6 +1736,12 @@ int ima_policy_show(struct seq_file *m, void *v)
> > > seq_puts(m, " ");
> > > }
> > >
> > > + if (entry->flags & IMA_DATA_SOURCES) {
> > > + seq_puts(m, "data_sources=");
> > > + ima_show_rule_opt_list(m, entry->data_sources);
> > > + seq_puts(m, " ");
> > > + }
> > > +
> > > if (entry->flags & IMA_PCR) {
> > > snprintf(tbuf, sizeof(tbuf), "%d", entry->pcr);
> > > seq_printf(m, pt(Opt_pcr), tbuf);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/3] IMA: add policy to support measuring critical data from kernel components
2020-08-25 20:43 ` Mimi Zohar
@ 2020-08-25 23:23 ` Tushar Sugandhi
0 siblings, 0 replies; 8+ messages in thread
From: Tushar Sugandhi @ 2020-08-25 23:23 UTC (permalink / raw)
To: Mimi Zohar, stephen.smalley.work, casey, agk, snitzer, gmazyland
Cc: tyhicks, sashal, jmorris, nramas, linux-integrity, selinux,
linux-security-module, linux-kernel, dm-devel
On 2020-08-25 1:43 p.m., Mimi Zohar wrote:
> On Tue, 2020-08-25 at 10:32 -0700, Tushar Sugandhi wrote:
>>
>> On 2020-08-24 3:46 p.m., Mimi Zohar wrote:
>>> On Fri, 2020-08-21 at 11:21 -0700, Tushar Sugandhi wrote:
>>>> There would be several candidate kernel components suitable for IMA
>>>> measurement. Not all of them would have support for IMA measurement.
>>>> Also, system administrators may not want to measure data for all of
>>>> them, even when they support IMA measurement. An IMA policy specific
>>>> to various kernel components is needed to measure their respective
>>>> critical data.
>>>>
>>>> Add a new IMA policy CRITICAL_DATA+data_sources to support measuring
>>>> various critical kernel components. This policy would enable the
>>>> system administrators to limit the measurement to the components,
>>>> if the components support IMA measurement.
>>>>
>>>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
>>>> ---
>>>> Documentation/ABI/testing/ima_policy | 6 ++-
>>>> security/integrity/ima/ima.h | 1 +
>>>> security/integrity/ima/ima_api.c | 2 +-
>>>> security/integrity/ima/ima_policy.c | 62 +++++++++++++++++++++++++---
>>>> 4 files changed, 63 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
>>>> index cd572912c593..a0dd0f108555 100644
>>>> --- a/Documentation/ABI/testing/ima_policy
>>>> +++ b/Documentation/ABI/testing/ima_policy
>>>> @@ -29,7 +29,7 @@ Description:
>>>> base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
>>>> [FIRMWARE_CHECK]
>>>> [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
>>>> - [KEXEC_CMDLINE] [KEY_CHECK]
>>>> + [KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA]
>>>> mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
>>>> [[^]MAY_EXEC]
>>>> fsmagic:= hex value
>>>> @@ -125,3 +125,7 @@ Description:
>>>> keys added to .builtin_trusted_keys or .ima keyring:
>>>>
>>>> measure func=KEY_CHECK keyrings=.builtin_trusted_keys|.ima
>>>> +
>>>> + Example of measure rule using CRITICAL_DATA to measure critical data
>>>> +
>>>> + measure func=CRITICAL_DATA data_sources=selinux|apparmor|dm-crypt
>>>
>>> This example uses "data_sources" without first defining it in the
>>> "option:" section. Defining two new options is an indication that this
>> Thanks. I will define "data_sources" first in "option:" section.
>>> patch should be split up. One which defines the "CRITICAL_DATA" and
>>> another one which defines the new key value pair. The term
>> I intentionally kept the "CRITICAL_DATA" and "data_sources" in the same
>> patch.
>>
>> CRITICAL_DATA is different than KEY_CHECK because in case of KEY_CHECK,
>> "keyrings=" is optional. If "keyrings=" is not specified, then we
>> measure all keyrings.
>>
>> Where for CRITICAL_DATA, "data_sources=" is mandatory.
>>
>> Because the data sources would be diverse and orthogonal to each other,
>> (unlike "keyrings=") - not specifying "data_sources=" shouldn't result
>> in IMA blindly measuring all data sources.
>
> Good point.
>>
>> Since CRITICAL_DATA, and "data_sources=" go hand in hand, I wanted them
>> to be part of the same patch.
>
> Separating them will help clarify the patch description. There's no
> harm in defining the critical data source first.
>
I will put func=CRITICAL_DATA into one patch, and "data_sources=" into
the next patch. Coding wise, the reverse order of patches (where
"data_sources=" goes in the first patch, before func=CRITICAL_DATA)
doesn't make sense. Because ima_match_rules() etc. have switch cases
built around func=CRITICAL_DATA etc.
>>> "data_sources" is pretty generic. Perhaps constrain it a bit by re-
>>> naming it "critical_data=". Or was such using a generic name
>>> intentional?
>>>
>> We intentionally kept the name generic because the data to be measured
>> could be coming from any kernel component with any granularity (from a
>> single bool to megabytes of data). The kernel component is also loosely
>> defined here. It could be an LSM (like SELinux), or a broader base layer
>> (like device-mapper), or a specific module (like dm-crypt), or it could
>> be different parts of a single module.
>>
>> Also, we didn't want to name "data_sources" as "critical_data" to avoid
>> confusion with func "CRITICAL_DATA".
>
> The point is that you're measuring critical data, not just any data
> from any source. Whatever term is used, it needs to be added to the
> Documentation/ABI/testing/ima_policy. I think something that is self
> describing will help. See what makes the most sense.
Fair enough.
Does "critical_kernel_data_sources=" sound ok?
>
>>> Normally "CRITICAL_DATA" would be defined with the critical data hook,
>>> but that seems to be defined in patch 3/3 "IMA: define IMA hook to
>>> measure critical data from kernel components".
>>>
>> I can make the "CRITICAL_DATA" and the hook as part of the same patch.
>> That would mean combining patch 2 and 3 into a single one.
>>
>> Does it sound ok?
>
> In the other thread, we discussed separating out "measure_payload_hash"from other changes. The end result you want one logical change per patch. Each patch builds upon the previous one. (Look at how Tyler does it.)
Will do.
>
>>>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>>>> index 8875085db689..0f4209a92bfb 100644
>>>> --- a/security/integrity/ima/ima.h
>>>> +++ b/security/integrity/ima/ima.h
>>>> @@ -200,6 +200,7 @@ static inline unsigned int ima_hash_key(u8 *digest)
>>>> hook(POLICY_CHECK, policy) \
>>>> hook(KEXEC_CMDLINE, kexec_cmdline) \
>>>> hook(KEY_CHECK, key) \
>>>> + hook(CRITICAL_DATA, critical_data) \
>>>> hook(MAX_CHECK, none)
>>>>
>>>> #define __ima_hook_enumify(ENUM, str) ENUM,
>>>> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
>>>> index af218babd198..9917e1730cb6 100644
>>>> --- a/security/integrity/ima/ima_api.c
>>>> +++ b/security/integrity/ima/ima_api.c
>>>> @@ -176,7 +176,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
>>>> * 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 | KEY_CHECK
>>>> + * | KEXEC_CMDLINE | KEY_CHECK | CRITICAL_DATA
>>>> * mask: contains the permission mask
>>>> * fsmagic: hex value
>>>> *
>>>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>>>> index 8866e84d0062..7b649095ac7a 100644
>>>> --- a/security/integrity/ima/ima_policy.c
>>>> +++ b/security/integrity/ima/ima_policy.c
>>>> @@ -33,6 +33,7 @@
>>>> #define IMA_PCR 0x0100
>>>> #define IMA_FSNAME 0x0200
>>>> #define IMA_KEYRINGS 0x0400
>>>> +#define IMA_DATA_SOURCES 0x0800
>>>>
>>>> #define UNKNOWN 0
>>>> #define MEASURE 0x0001 /* same as IMA_MEASURE */
>>>> @@ -84,6 +85,7 @@ struct ima_rule_entry {
>>>> } lsm[MAX_LSM_RULES];
>>>> char *fsname;
>>>> struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyrings */
>>>> + struct ima_rule_opt_list *data_sources; /* Measure data from these sources */
>>>> struct ima_template_desc *template;
>>>> };
>>>>
>>>> @@ -508,14 +510,23 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>>>> {
>>>> int i;
>>>>
>>>> - if (func == KEY_CHECK) {
>>>> - return (rule->flags & IMA_FUNC) && (rule->func == func) &&
>>>> - ima_match_rule_data(rule, rule->keyrings, func_data,
>>>> - true, cred);
>>>> - }
>>>> if ((rule->flags & IMA_FUNC) &&
>>>> (rule->func != func && func != POST_SETATTR))
>>>> return false;
>>>> +
>>>> + switch (func) {
>>>> + case KEY_CHECK:
>>>> + return ((rule->func == func) &&
>>>> + ima_match_rule_data(rule, rule->keyrings,
>>>> + func_data, true, cred));
>>>> + case CRITICAL_DATA:
>>>> + return ((rule->func == func) &&
>>>> + ima_match_rule_data(rule, rule->data_sources,
>>>> + func_data, false, cred));
>>>> + default:
>>>> + break;
>>>> + }
>>>> +
>>>> if ((rule->flags & IMA_MASK) &&
>>>> (rule->mask != mask && func != POST_SETATTR))
>>>> return false;
>>>> @@ -911,7 +922,7 @@ enum {
>>>> Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
>>>> Opt_appraise_type, Opt_appraise_flag,
>>>> Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
>>>> - Opt_err
>>>> + Opt_data_sources, Opt_err
>>>> };
>>>>
>>>> static const match_table_t policy_tokens = {
>>>> @@ -948,6 +959,7 @@ static const match_table_t policy_tokens = {
>>>> {Opt_pcr, "pcr=%s"},
>>>> {Opt_template, "template=%s"},
>>>> {Opt_keyrings, "keyrings=%s"},
>>>> + {Opt_data_sources, "data_sources=%s"},
>>>> {Opt_err, NULL}
>>>> };
>>>>
>>>> @@ -1110,6 +1122,19 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
>>>> if (ima_rule_contains_lsm_cond(entry))
>>>> return false;
>>>>
>>>> + break;
>>>> + case CRITICAL_DATA:
>>>> + if (entry->action & ~(MEASURE | DONT_MEASURE))
>>>> + return false;
>>>> +
>>>> + if (!(entry->flags & IMA_DATA_SOURCES) ||
>>>> + (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR |
>>>> + IMA_DATA_SOURCES)))
>>>> + return false;
>>>
>>> Requiring IMA_FUNC and IMA_DATA_SOURCES makes sense, but why are
>>> IMA_UID and IMA_PCR required?
>>>
>> Since the data to be measured could be for any scenario, I didn't want
>> to restrict the kernel components from choosing UID to measure the data
>> for, or restrict them from choosing PCR to store the measurements in.
>> But as the consumers are kernel components, perhaps support for IMA_UID
>> is not required. But we should still support IMA_PCR.
>> Please let me know what do you think, and I can update the logic
>> accordingly.
>
> I think I misinterpreted this code. As long as IMA_UID and IMA_PCR
> aren't required, then it is fine.
Yes, IMA_UID and IMA_PCR are not mandatory. Only IMA_DATA_SOURCES is.
I will keep both of them.
Thanks for confirming.
>
>>>> +
>>>> + if (ima_rule_contains_lsm_cond(entry))
>>>> + return false;
>>>> +
>>>> break;
>>>> default:
>>>> return false;
>>>> @@ -1242,6 +1267,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>>>> else if (IS_ENABLED(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) &&
>>>> strcmp(args[0].from, "KEY_CHECK") == 0)
>>>> entry->func = KEY_CHECK;
>>>> + else if (strcmp(args[0].from, "CRITICAL_DATA") == 0)
>>>> + entry->func = CRITICAL_DATA;
>>>> else
>>>> result = -EINVAL;
>>>> if (!result)
>>>> @@ -1312,6 +1339,23 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>>>>
>>>> entry->flags |= IMA_KEYRINGS;
>>>> break;
>>>> + case Opt_data_sources:
>>>> + ima_log_string(ab, "data_sources", args[0].from);
>>>> +
>>>> + if (entry->data_sources) {
>>>> + result = -EINVAL;
>>>> + break;
>>>> + }
>>>> +
>>>> + entry->data_sources = ima_alloc_rule_opt_list(args);
>>>> + if (IS_ERR(entry->data_sources)) {
>>>> + result = PTR_ERR(entry->data_sources);
>>>> + entry->data_sources = NULL;
>>>> + break;
>>>> + }
>>>> +
>>>
>>> "keyrings=" isn't bounded because keyrings can be created by userspace.
>>> Perhaps keyring names has a minimum/maximum length. IMA isn't
>>> measuring userspace construsts. Shouldn't the list of critical data
>>> being measured be bounded and verified?
>> The comment is not entirely clear.
>> Do you mean there should be some sort of allow_list in IMA, against
>> which the values in "data_sources=" should be vetted? And if the
>> value is present in the IMA allow_list, then only the measurements for
>> that data source are allowed?
>>
>> Or do you mean something else?
>
> Yes, something along those lines. Does the list of critical data need
> to be vetted? And if so, against what?
I am thinking of having an enum and string array - just like ima_hooks
and ima_hooks_measure_str in ima.h.
And any new kernel component that would support generic IMA measurements
in future would have to add itself to the enum/array.
And the param *event_data_source in ima_measure_critical_data() will be
vetted against the above enum/string array.
I will implement it in the next iteration, and hopefully the vetting
workflow will be more clear.
~Tushar
>
> Mimi
>
>>>
>>>> + entry->flags |= IMA_DATA_SOURCES;
>>>> + break;
>>>> case Opt_fsuuid:
>>>> ima_log_string(ab, "fsuuid", args[0].from);
>>>>
>>>> @@ -1692,6 +1736,12 @@ int ima_policy_show(struct seq_file *m, void *v)
>>>> seq_puts(m, " ");
>>>> }
>>>>
>>>> + if (entry->flags & IMA_DATA_SOURCES) {
>>>> + seq_puts(m, "data_sources=");
>>>> + ima_show_rule_opt_list(m, entry->data_sources);
>>>> + seq_puts(m, " ");
>>>> + }
>>>> +
>>>> if (entry->flags & IMA_PCR) {
>>>> snprintf(tbuf, sizeof(tbuf), "%d", entry->pcr);
>>>> seq_printf(m, pt(Opt_pcr), tbuf);
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-08-25 23:23 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-21 18:21 [PATCH v2 0/3] IMA: Infrastructure for measurement of critical kernel data Tushar Sugandhi
2020-08-21 18:21 ` [PATCH v2 1/3] IMA: generalize keyring specific measurement constructs Tushar Sugandhi
2020-08-21 18:21 ` [PATCH v2 2/3] IMA: add policy to support measuring critical data from kernel components Tushar Sugandhi
2020-08-24 22:46 ` Mimi Zohar
2020-08-25 17:32 ` Tushar Sugandhi
2020-08-25 20:43 ` Mimi Zohar
2020-08-25 23:23 ` Tushar Sugandhi
2020-08-21 18:21 ` [PATCH v2 3/3] IMA: define IMA hook to measure " Tushar Sugandhi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).