Commit a068d937 authored by Mimi Zohar's avatar Mimi Zohar

Merge branch 'validate-policy-rules' into next-integrity

From "ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support"
coverletter.

This series ultimately extends the supported IMA rule conditionals for
the KEXEC_CMDLINE hook function. As of today, there's an imbalance in
IMA language conditional support for KEXEC_CMDLINE rules in comparison
to KEXEC_KERNEL_CHECK and KEXEC_INITRAMFS_CHECK rules. The KEXEC_CMDLINE
rules do not support *any* conditionals so you cannot have a sequence of
rules like this:

 dont_measure func=KEXEC_KERNEL_CHECK obj_type=foo_t
 dont_measure func=KEXEC_INITRAMFS_CHECK obj_type=foo_t
 dont_measure func=KEXEC_CMDLINE obj_type=foo_t
 measure func=KEXEC_KERNEL_CHECK
 measure func=KEXEC_INITRAMFS_CHECK
 measure func=KEXEC_CMDLINE

Instead, KEXEC_CMDLINE rules can only be measured or not measured and
there's no additional flexibility in today's implementation of the
KEXEC_CMDLINE hook function.

With this series, the above sequence of rules becomes valid and any
calls to kexec_file_load() with a kernel and initramfs inode type of
foo_t will not be measured (that includes the kernel cmdline buffer)
while all other objects given to a kexec_file_load() syscall will be
measured. There's obviously not an inode directly associated with the
kernel cmdline buffer but this patch series ties the inode based
decision making for KEXEC_CMDLINE to the kernel's inode. I think this
will be intuitive to policy authors.

While reading IMA code and preparing to make this change, I realized
that the buffer based hook functions (KEXEC_CMDLINE and KEY_CHECK) are
quite special in comparison to longer standing hook functions. These
buffer based hook functions can only support measure actions and there
are some restrictions on the conditionals that they support. However,
the rule parser isn't enforcing any of those restrictions and IMA policy
authors wouldn't have any immediate way of knowing that the policy that
they wrote is invalid. For example, the sequence of rules above parses
successfully in today's kernel but the
"dont_measure func=KEXEC_CMDLINE ..." rule is incorrectly handled in
ima_match_rules(). The dont_measure rule is *always* considered to be a
match so, surprisingly, no KEXEC_CMDLINE measurements are made.

While making the rule parser more strict, I realized that the parser
does not correctly free all of the allocated memory associated with an
ima_rule_entry when going down some error paths. Invalid policy loaded
by the policy administrator could result in small memory leaks.
parents 34e980bb 4834177e
......@@ -25,7 +25,7 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
enum kernel_read_file_id id);
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(const void *buf, int size);
extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
#ifdef CONFIG_IMA_KEXEC
extern void ima_add_kexec_buffer(struct kimage *image);
......@@ -103,7 +103,7 @@ static inline int ima_file_hash(struct file *file, char *buf, size_t buf_size)
return -EOPNOTSUPP;
}
static inline void ima_kexec_cmdline(const void *buf, int size) {}
static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {}
#endif /* CONFIG_IMA */
#ifndef CONFIG_IMA_KEXEC
......
......@@ -287,7 +287,7 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
goto out;
}
ima_kexec_cmdline(image->cmdline_buf,
ima_kexec_cmdline(kernel_fd, image->cmdline_buf,
image->cmdline_buf_len - 1);
}
......
......@@ -265,7 +265,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
struct evm_ima_xattr_data *xattr_value,
int xattr_len, const struct modsig *modsig, int pcr,
struct ima_template_desc *template_desc);
void process_buffer_measurement(const void *buf, int size,
void process_buffer_measurement(struct inode *inode, const void *buf, int size,
const char *eventname, enum ima_hooks func,
int pcr, const char *keyring);
void ima_audit_measurement(struct integrity_iint_cache *iint,
......@@ -372,7 +372,6 @@ static inline int ima_read_xattr(struct dentry *dentry,
#endif /* CONFIG_IMA_APPRAISE */
#ifdef CONFIG_IMA_APPRAISE_MODSIG
bool ima_hook_supports_modsig(enum ima_hooks func);
int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
struct modsig **modsig);
void ima_collect_modsig(struct modsig *modsig, const void *buf, loff_t size);
......@@ -382,11 +381,6 @@ int ima_get_raw_modsig(const struct modsig *modsig, const void **data,
u32 *data_len);
void ima_free_modsig(struct modsig *modsig);
#else
static inline bool ima_hook_supports_modsig(enum ima_hooks func)
{
return false;
}
static inline int ima_read_modsig(enum ima_hooks func, const void *buf,
loff_t buf_len, struct modsig **modsig)
{
......@@ -420,6 +414,7 @@ static inline void ima_free_modsig(struct modsig *modsig)
#ifdef CONFIG_IMA_LSM_RULES
#define security_filter_rule_init security_audit_rule_init
#define security_filter_rule_free security_audit_rule_free
#define security_filter_rule_match security_audit_rule_match
#else
......@@ -430,6 +425,10 @@ static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr,
return -EINVAL;
}
static inline void security_filter_rule_free(void *lsmrule)
{
}
static inline int security_filter_rule_match(u32 secid, u32 field, u32 op,
void *lsmrule)
{
......
......@@ -162,7 +162,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
/**
* ima_get_action - appraise & measure decision based on policy.
* @inode: pointer to inode to measure
* @inode: pointer to the inode associated with the object being validated
* @cred: pointer to credentials structure to validate
* @secid: secid of the task being validated
* @mask: contains the permission mask (MAY_READ, MAY_WRITE, MAY_EXEC,
......
......@@ -328,7 +328,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
rc = is_binary_blacklisted(digest, digestsize);
if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
process_buffer_measurement(digest, digestsize,
process_buffer_measurement(NULL, digest, digestsize,
"blacklisted-hash", NONE,
pcr, NULL);
}
......
......@@ -58,7 +58,7 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
* if the IMA policy is configured to measure a key linked
* to the given keyring.
*/
process_buffer_measurement(payload, payload_len,
process_buffer_measurement(NULL, payload, payload_len,
keyring->description, KEY_CHECK, 0,
keyring->description);
}
......@@ -726,6 +726,7 @@ int ima_load_data(enum kernel_load_data_id id)
/*
* process_buffer_measurement - Measure the buffer to ima log.
* @inode: inode associated with the object being measured (NULL for KEY_CHECK)
* @buf: pointer to the buffer that needs to be added to the log.
* @size: size of buffer(in bytes).
* @eventname: event name to be used for the buffer entry.
......@@ -735,7 +736,7 @@ int ima_load_data(enum kernel_load_data_id id)
*
* Based on policy, the buffer is measured into the ima log.
*/
void process_buffer_measurement(const void *buf, int size,
void process_buffer_measurement(struct inode *inode, const void *buf, int size,
const char *eventname, enum ima_hooks func,
int pcr, const char *keyring)
{
......@@ -768,7 +769,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,
action = ima_get_action(inode, current_cred(), secid, 0, func,
&pcr, &template, keyring);
if (!(action & IMA_MEASURE))
return;
......@@ -823,16 +824,26 @@ void process_buffer_measurement(const void *buf, int size,
/**
* ima_kexec_cmdline - measure kexec cmdline boot args
* @kernel_fd: file descriptor of the kexec kernel being loaded
* @buf: pointer to buffer
* @size: size of buffer
*
* Buffers can only be measured, not appraised.
*/
void ima_kexec_cmdline(const void *buf, int size)
void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
{
if (buf && size != 0)
process_buffer_measurement(buf, size, "kexec-cmdline",
KEXEC_CMDLINE, 0, NULL);
struct fd f;
if (!buf || !size)
return;
f = fdget(kernel_fd);
if (!f.file)
return;
process_buffer_measurement(file_inode(f.file), buf, size,
"kexec-cmdline", KEXEC_CMDLINE, 0, NULL);
fdput(f);
}
static int __init init_ima(void)
......
......@@ -32,26 +32,6 @@ struct modsig {
u8 raw_pkcs7[];
};
/**
* ima_hook_supports_modsig - can the policy allow modsig for this hook?
*
* modsig is only supported by hooks using ima_post_read_file(), because only
* they preload the contents of the file in a buffer. FILE_CHECK does that in
* some cases, but not when reached from vfs_open(). POLICY_CHECK can support
* it, but it's not useful in practice because it's a text file so deny.
*/
bool ima_hook_supports_modsig(enum ima_hooks func)
{
switch (func) {
case KEXEC_KERNEL_CHECK:
case KEXEC_INITRAMFS_CHECK:
case MODULE_CHECK:
return true;
default:
return false;
}
}
/*
* ima_read_modsig - Read modsig from buf.
*
......
......@@ -74,7 +74,7 @@ struct ima_rule_entry {
int pcr;
struct {
void *rule; /* LSM file metadata specific */
void *args_p; /* audit value */
char *args_p; /* audit value */
int type; /* audit type */
} lsm[MAX_LSM_RULES];
char *fsname;
......@@ -258,9 +258,24 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry)
int i;
for (i = 0; i < MAX_LSM_RULES; i++) {
kfree(entry->lsm[i].rule);
security_filter_rule_free(entry->lsm[i].rule);
kfree(entry->lsm[i].args_p);
}
}
static void ima_free_rule(struct ima_rule_entry *entry)
{
if (!entry)
return;
/*
* entry->template->fields may be allocated in ima_parse_rule() but that
* reference is owned by the corresponding ima_template_desc element in
* the defined_templates list and cannot be freed here
*/
kfree(entry->fsname);
kfree(entry->keyrings);
ima_lsm_free_rule(entry);
kfree(entry);
}
......@@ -285,10 +300,13 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
continue;
nentry->lsm[i].type = entry->lsm[i].type;
nentry->lsm[i].args_p = kstrdup(entry->lsm[i].args_p,
GFP_KERNEL);
if (!nentry->lsm[i].args_p)
goto out_err;
nentry->lsm[i].args_p = entry->lsm[i].args_p;
/*
* Remove the reference from entry so that the associated
* memory will not be freed during a later call to
* ima_lsm_free_rule(entry).
*/
entry->lsm[i].args_p = NULL;
security_filter_rule_init(nentry->lsm[i].type,
Audit_equal,
......@@ -296,13 +314,9 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
&nentry->lsm[i].rule);
if (!nentry->lsm[i].rule)
pr_warn("rule for LSM \'%s\' is undefined\n",
(char *)entry->lsm[i].args_p);
nentry->lsm[i].args_p);
}
return nentry;
out_err:
ima_lsm_free_rule(nentry);
return NULL;
}
static int ima_lsm_update_rule(struct ima_rule_entry *entry)
......@@ -315,11 +329,29 @@ static int ima_lsm_update_rule(struct ima_rule_entry *entry)
list_replace_rcu(&entry->list, &nentry->list);
synchronize_rcu();
/*
* ima_lsm_copy_rule() shallow copied all references, except for the
* LSM references, from entry to nentry so we only want to free the LSM
* references and the entry itself. All other memory refrences will now
* be owned by nentry.
*/
ima_lsm_free_rule(entry);
kfree(entry);
return 0;
}
static bool ima_rule_contains_lsm_cond(struct ima_rule_entry *entry)
{
int i;
for (i = 0; i < MAX_LSM_RULES; i++)
if (entry->lsm[i].args_p)
return true;
return false;
}
/*
* The LSM policy can be reloaded, leaving the IMA LSM based rules referring
* to the old, stale LSM policy. Update the IMA LSM based rules to reflect
......@@ -328,17 +360,10 @@ static int ima_lsm_update_rule(struct ima_rule_entry *entry)
static void ima_lsm_update_rules(void)
{
struct ima_rule_entry *entry, *e;
int i, result, needs_update;
int result;
list_for_each_entry_safe(entry, e, &ima_policy_rules, list) {
needs_update = 0;
for (i = 0; i < MAX_LSM_RULES; i++) {
if (entry->lsm[i].args_p) {
needs_update = 1;
break;
}
}
if (!needs_update)
if (!ima_rule_contains_lsm_cond(entry))
continue;
result = ima_lsm_update_rule(entry);
......@@ -418,13 +443,9 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
{
int i;
if ((func == KEXEC_CMDLINE) || (func == KEY_CHECK)) {
if ((rule->flags & IMA_FUNC) && (rule->func == func)) {
if (func == KEY_CHECK)
return ima_match_keyring(rule, keyring, cred);
return true;
}
return false;
if (func == KEY_CHECK) {
return (rule->flags & IMA_FUNC) && (rule->func == func) &&
ima_match_keyring(rule, keyring, cred);
}
if ((rule->flags & IMA_FUNC) &&
(rule->func != func && func != POST_SETATTR))
......@@ -886,10 +907,11 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
&entry->lsm[lsm_rule].rule);
if (!entry->lsm[lsm_rule].rule) {
pr_warn("rule for LSM \'%s\' is undefined\n",
(char *)entry->lsm[lsm_rule].args_p);
entry->lsm[lsm_rule].args_p);
if (ima_rules == &ima_default_rules) {
kfree(entry->lsm[lsm_rule].args_p);
entry->lsm[lsm_rule].args_p = NULL;
result = -EINVAL;
} else
result = 0;
......@@ -949,6 +971,96 @@ static void check_template_modsig(const struct ima_template_desc *template)
#undef MSG
}
static bool ima_validate_rule(struct ima_rule_entry *entry)
{
/* Ensure that the action is set and is compatible with the flags */
if (entry->action == UNKNOWN)
return false;
if (entry->action != MEASURE && entry->flags & IMA_PCR)
return false;
if (entry->action != APPRAISE &&
entry->flags & (IMA_DIGSIG_REQUIRED | IMA_MODSIG_ALLOWED | IMA_CHECK_BLACKLIST))
return false;
/*
* The IMA_FUNC bit must be set if and only if there's a valid hook
* function specified, and vice versa. Enforcing this property allows
* for the NONE case below to validate a rule without an explicit hook
* function.
*/
if (((entry->flags & IMA_FUNC) && entry->func == NONE) ||
(!(entry->flags & IMA_FUNC) && entry->func != NONE))
return false;
/*
* Ensure that the hook function is compatible with the other
* components of the rule
*/
switch (entry->func) {
case NONE:
case FILE_CHECK:
case MMAP_CHECK:
case BPRM_CHECK:
case CREDS_CHECK:
case POST_SETATTR:
case FIRMWARE_CHECK:
case POLICY_CHECK:
if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC |
IMA_UID | IMA_FOWNER | IMA_FSUUID |
IMA_INMASK | IMA_EUID | IMA_PCR |
IMA_FSNAME | IMA_DIGSIG_REQUIRED |
IMA_PERMIT_DIRECTIO))
return false;
break;
case MODULE_CHECK:
case KEXEC_KERNEL_CHECK:
case KEXEC_INITRAMFS_CHECK:
if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC |
IMA_UID | IMA_FOWNER | IMA_FSUUID |
IMA_INMASK | IMA_EUID | IMA_PCR |
IMA_FSNAME | IMA_DIGSIG_REQUIRED |
IMA_PERMIT_DIRECTIO | IMA_MODSIG_ALLOWED |
IMA_CHECK_BLACKLIST))
return false;
break;
case KEXEC_CMDLINE:
if (entry->action & ~(MEASURE | DONT_MEASURE))
return false;
if (entry->flags & ~(IMA_FUNC | IMA_FSMAGIC | IMA_UID |
IMA_FOWNER | IMA_FSUUID | IMA_EUID |
IMA_PCR | IMA_FSNAME))
return false;
break;
case KEY_CHECK:
if (entry->action & ~(MEASURE | DONT_MEASURE))
return false;
if (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR |
IMA_KEYRINGS))
return false;
if (ima_rule_contains_lsm_cond(entry))
return false;
break;
default:
return false;
}
/* Ensure that combinations of flags are compatible with each other */
if (entry->flags & IMA_CHECK_BLACKLIST &&
!(entry->flags & IMA_MODSIG_ALLOWED))
return false;
return true;
}
static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
{
struct audit_buffer *ab;
......@@ -1126,8 +1238,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
keyrings_len = strlen(args[0].from) + 1;
if ((entry->keyrings) ||
(entry->action != MEASURE) ||
(entry->func != KEY_CHECK) ||
(keyrings_len < 2)) {
result = -EINVAL;
break;
......@@ -1267,15 +1377,10 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
AUDIT_SUBJ_TYPE);
break;
case Opt_appraise_type:
if (entry->action != APPRAISE) {
result = -EINVAL;
break;
}
ima_log_string(ab, "appraise_type", args[0].from);
if ((strcmp(args[0].from, "imasig")) == 0)
entry->flags |= IMA_DIGSIG_REQUIRED;
else if (ima_hook_supports_modsig(entry->func) &&
else if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
strcmp(args[0].from, "imasig|modsig") == 0)
entry->flags |= IMA_DIGSIG_REQUIRED |
IMA_MODSIG_ALLOWED;
......@@ -1284,17 +1389,16 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
break;
case Opt_appraise_flag:
ima_log_string(ab, "appraise_flag", args[0].from);
if (strstr(args[0].from, "blacklist"))
if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
strstr(args[0].from, "blacklist"))
entry->flags |= IMA_CHECK_BLACKLIST;
else
result = -EINVAL;
break;
case Opt_permit_directio:
entry->flags |= IMA_PERMIT_DIRECTIO;
break;
case Opt_pcr:
if (entry->action != MEASURE) {
result = -EINVAL;
break;
}
ima_log_string(ab, "pcr", args[0].from);
result = kstrtoint(args[0].from, 10, &entry->pcr);
......@@ -1332,7 +1436,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
break;
}
}
if (!result && (entry->action == UNKNOWN))
if (!result && !ima_validate_rule(entry))
result = -EINVAL;
else if (entry->action == APPRAISE)
temp_ima_appraise |= ima_appraise_flag(entry->func);
......@@ -1381,7 +1485,7 @@ ssize_t ima_parse_add_rule(char *rule)
result = ima_parse_rule(p, entry);
if (result) {
kfree(entry);
ima_free_rule(entry);
integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
NULL, op, "invalid-policy", result,
audit_info);
......@@ -1402,15 +1506,11 @@ ssize_t ima_parse_add_rule(char *rule)
void ima_delete_rules(void)
{
struct ima_rule_entry *entry, *tmp;
int i;
temp_ima_appraise = 0;
list_for_each_entry_safe(entry, tmp, &ima_temp_rules, list) {
for (i = 0; i < MAX_LSM_RULES; i++)
kfree(entry->lsm[i].args_p);
list_del(&entry->list);
kfree(entry);
ima_free_rule(entry);
}
}
......@@ -1589,27 +1689,27 @@ int ima_policy_show(struct seq_file *m, void *v)
switch (i) {
case LSM_OBJ_USER:
seq_printf(m, pt(Opt_obj_user),
(char *)entry->lsm[i].args_p);
entry->lsm[i].args_p);
break;
case LSM_OBJ_ROLE:
seq_printf(m, pt(Opt_obj_role),
(char *)entry->lsm[i].args_p);
entry->lsm[i].args_p);
break;
case LSM_OBJ_TYPE:
seq_printf(m, pt(Opt_obj_type),
(char *)entry->lsm[i].args_p);
entry->lsm[i].args_p);
break;
case LSM_SUBJ_USER:
seq_printf(m, pt(Opt_subj_user),
(char *)entry->lsm[i].args_p);
entry->lsm[i].args_p);
break;
case LSM_SUBJ_ROLE:
seq_printf(m, pt(Opt_subj_role),
(char *)entry->lsm[i].args_p);
entry->lsm[i].args_p);
break;
case LSM_SUBJ_TYPE:
seq_printf(m, pt(Opt_subj_type),
(char *)entry->lsm[i].args_p);
entry->lsm[i].args_p);
break;
}
seq_puts(m, " ");
......
......@@ -158,7 +158,7 @@ void ima_process_queued_keys(void)
list_for_each_entry_safe(entry, tmp, &ima_keys, list) {
if (!timer_expired)
process_buffer_measurement(entry->payload,
process_buffer_measurement(NULL, entry->payload,
entry->payload_len,
entry->keyring_name,
KEY_CHECK, 0,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment