From 34ef7bd3823bf4401bf8f1f855e1bc77b82b1a43 Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Sat, 23 Nov 2013 16:36:35 -0800 Subject: Revert "ima: define '_ima' as a builtin 'trusted' keyring" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 217091dd7a7a1bdac027ddb7c5a25f6ac0b8e241, which caused the following build error: security/integrity/digsig.c:70:5: error: redefinition of ‘integrity_init_keyring’ security/integrity/integrity.h:149:12: note: previous definition of ‘integrity_init_keyring’ w security/integrity/integrity.h:149:12: warning: ‘integrity_init_keyring’ defined but not used reported by Krzysztof Kolasa. Mimi says: "I made the classic mistake of requesting this patch to be upstreamed at the last second, rather than waiting until the next open window. At this point, the best course would probably be to revert the two commits and fix them for the next open window" Reported-by: Krzysztof Kolasa Acked-by: Mimi Zohar Signed-off-by: Linus Torvalds --- security/integrity/ima/Kconfig | 8 -------- security/integrity/ima/ima_appraise.c | 11 ----------- 2 files changed, 19 deletions(-) (limited to 'security/integrity/ima') diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig index dad8d4c..81a2797 100644 --- a/security/integrity/ima/Kconfig +++ b/security/integrity/ima/Kconfig @@ -123,11 +123,3 @@ config IMA_APPRAISE For more information on integrity appraisal refer to: If unsure, say N. - -config IMA_TRUSTED_KEYRING - bool "Require all keys on the _ima keyring be signed" - depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING - default y - help - This option requires that all keys added to the _ima - keyring be signed by a key on the system trusted keyring. diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 46353ee..734e946 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -381,14 +381,3 @@ int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name) } return result; } - -#ifdef CONFIG_IMA_TRUSTED_KEYRING -static int __init init_ima_keyring(void) -{ - int ret; - - ret = integrity_init_keyring(INTEGRITY_KEYRING_IMA); - return 0; -} -late_initcall(init_ima_keyring); -#endif -- cgit v1.1 From b6f8f16f41d92861621b043389ef49de1c52d613 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 8 Nov 2013 19:21:39 +0100 Subject: ima: do not include field length in template digest calc for ima template To maintain compatibility with userspace tools, the field length must not be included in the template digest calculation for the 'ima' template. Fixes commit: a71dc65 ima: switch to new template management mechanism Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/ima/ima.h | 3 ++- security/integrity/ima/ima_api.c | 1 + security/integrity/ima/ima_crypto.c | 17 ++++++++++++----- 3 files changed, 15 insertions(+), 6 deletions(-) (limited to 'security/integrity/ima') diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index bf03c6a..a21cf70 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -97,7 +97,8 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation, const char *op, struct inode *inode, const unsigned char *filename); int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash); -int ima_calc_field_array_hash(struct ima_field_data *field_data, int num_fields, +int ima_calc_field_array_hash(struct ima_field_data *field_data, + struct ima_template_desc *desc, int num_fields, struct ima_digest_data *hash); int __init ima_calc_boot_aggregate(struct ima_digest_data *hash); void ima_add_violation(struct file *file, const unsigned char *filename, diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index 0e75408..8037484 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -94,6 +94,7 @@ int ima_store_template(struct ima_template_entry *entry, /* this function uses default algo */ hash.hdr.algo = HASH_ALGO_SHA1; result = ima_calc_field_array_hash(&entry->template_data[0], + entry->template_desc, num_fields, &hash.hdr); if (result < 0) { integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode, diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c index 676e029..fdf60de 100644 --- a/security/integrity/ima/ima_crypto.c +++ b/security/integrity/ima/ima_crypto.c @@ -140,6 +140,7 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash) * Calculate the hash of template data */ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data, + struct ima_template_desc *td, int num_fields, struct ima_digest_data *hash, struct crypto_shash *tfm) @@ -160,9 +161,13 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data, return rc; for (i = 0; i < num_fields; i++) { - rc = crypto_shash_update(&desc.shash, - (const u8 *) &field_data[i].len, - sizeof(field_data[i].len)); + if (strcmp(td->name, IMA_TEMPLATE_IMA_NAME) != 0) { + rc = crypto_shash_update(&desc.shash, + (const u8 *) &field_data[i].len, + sizeof(field_data[i].len)); + if (rc) + break; + } rc = crypto_shash_update(&desc.shash, field_data[i].data, field_data[i].len); if (rc) @@ -175,7 +180,8 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data, return rc; } -int ima_calc_field_array_hash(struct ima_field_data *field_data, int num_fields, +int ima_calc_field_array_hash(struct ima_field_data *field_data, + struct ima_template_desc *desc, int num_fields, struct ima_digest_data *hash) { struct crypto_shash *tfm; @@ -185,7 +191,8 @@ int ima_calc_field_array_hash(struct ima_field_data *field_data, int num_fields, if (IS_ERR(tfm)) return PTR_ERR(tfm); - rc = ima_calc_field_array_hash_tfm(field_data, num_fields, hash, tfm); + rc = ima_calc_field_array_hash_tfm(field_data, desc, num_fields, + hash, tfm); ima_free_tfm(tfm); -- cgit v1.1 From 3e8e5503a33577d89bdb7469b851b11f507bbed6 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 8 Nov 2013 19:21:40 +0100 Subject: ima: do not send field length to userspace for digest of ima template This patch defines a new value for the 'ima_show_type' enumerator (IMA_SHOW_BINARY_NO_FIELD_LEN) to prevent that the field length is transmitted through the 'binary_runtime_measurements' interface for the digest field of the 'ima' template. Fixes commit: 3ce1217 ima: define template fields library and new helpers Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/ima/ima.h | 3 ++- security/integrity/ima/ima_fs.c | 14 +++++++++++--- security/integrity/ima/ima_template_lib.c | 6 +++++- 3 files changed, 18 insertions(+), 5 deletions(-) (limited to 'security/integrity/ima') diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index a21cf70..9636e17 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -26,7 +26,8 @@ #include "../integrity.h" -enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_ASCII }; +enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_BINARY_NO_FIELD_LEN, + IMA_SHOW_ASCII }; enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 }; /* digest size for IMA, fits SHA1 or MD5 */ diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c index d47a7c8..db01125 100644 --- a/security/integrity/ima/ima_fs.c +++ b/security/integrity/ima/ima_fs.c @@ -120,6 +120,7 @@ static int ima_measurements_show(struct seq_file *m, void *v) struct ima_template_entry *e; int namelen; u32 pcr = CONFIG_IMA_MEASURE_PCR_IDX; + bool is_ima_template = false; int i; /* get entry */ @@ -145,14 +146,21 @@ static int ima_measurements_show(struct seq_file *m, void *v) ima_putc(m, e->template_desc->name, namelen); /* 5th: template length (except for 'ima' template) */ - if (strcmp(e->template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) + if (strcmp(e->template_desc->name, IMA_TEMPLATE_IMA_NAME) == 0) + is_ima_template = true; + + if (!is_ima_template) ima_putc(m, &e->template_data_len, sizeof(e->template_data_len)); /* 6th: template specific data */ for (i = 0; i < e->template_desc->num_fields; i++) { - e->template_desc->fields[i]->field_show(m, IMA_SHOW_BINARY, - &e->template_data[i]); + enum ima_show_type show = IMA_SHOW_BINARY; + struct ima_template_field *field = e->template_desc->fields[i]; + + if (is_ima_template && strcmp(field->field_id, "d") == 0) + show = IMA_SHOW_BINARY_NO_FIELD_LEN; + field->field_show(m, show, &e->template_data[i]); } return 0; } diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c index 6d66ad6..c38adcc 100644 --- a/security/integrity/ima/ima_template_lib.c +++ b/security/integrity/ima/ima_template_lib.c @@ -109,9 +109,12 @@ static void ima_show_template_data_binary(struct seq_file *m, enum data_formats datafmt, struct ima_field_data *field_data) { - ima_putc(m, &field_data->len, sizeof(u32)); + if (show != IMA_SHOW_BINARY_NO_FIELD_LEN) + ima_putc(m, &field_data->len, sizeof(u32)); + if (!field_data->len) return; + ima_putc(m, field_data->data, field_data->len); } @@ -125,6 +128,7 @@ static void ima_show_template_field_data(struct seq_file *m, ima_show_template_data_ascii(m, show, datafmt, field_data); break; case IMA_SHOW_BINARY: + case IMA_SHOW_BINARY_NO_FIELD_LEN: ima_show_template_data_binary(m, show, datafmt, field_data); break; default: -- cgit v1.1 From dbc335d2dc3c437649eb6b39f4e9aee2a13eb0af Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Mon, 25 Nov 2013 20:18:52 +0100 Subject: ima: make a copy of template_fmt in template_desc_init_fields() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch makes a copy of the 'template_fmt' function argument so that the latter will not be modified by strsep(), which does the splitting by replacing the given separator with '\0'.  IMA: No TPM chip found, activating TPM-bypass!  Unable to handle kernel pointer dereference at virtual kernel address 0000000000842000  Oops: 0004 [#1] SMP  Modules linked in:  CPU: 3 PID: 1 Comm: swapper/0 Not tainted 3.12.0-rc2-00098-g3ce1217d6cd5 #17  task: 000000003ffa0000 ti: 000000003ff84000 task.ti: 000000003ff84000  Krnl PSW : 0704e00180000000 000000000044bf88 (strsep+0x7c/0xa0)             R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:0 EA:3  Krnl GPRS: 000000000000007c 000000000000007c 000000003ff87d90 0000000000821fd8             0000000000000000 000000000000007c 0000000000aa37e0 0000000000aa9008             0000000000000051 0000000000a114d8 0000000100000002 0000000000842bde             0000000000842bdf 00000000006f97f0 000000000040062c 000000003ff87cf0  Krnl Code: 000000000044bf7c: a7f4000a           brc     15,44bf90             000000000044bf80: b90200cc           ltgr    %r12,%r12            #000000000044bf84: a7840006           brc     8,44bf90            >000000000044bf88: 9200c000           mvi     0(%r12),0             000000000044bf8c: 41c0c001           la      %r12,1(%r12)             000000000044bf90: e3c020000024       stg     %r12,0(%r2)             000000000044bf96: b904002b           lgr     %r2,%r11             000000000044bf9a: ebbcf0700004       lmg     %r11,%r12,112(%r15)  Call Trace:  ([<00000000004005fe>] ima_init_template+0xa2/0x1bc)   [<0000000000a7c896>] ima_init+0x7a/0xa8   [<0000000000a7c938>] init_ima+0x24/0x40   [<00000000001000e8>] do_one_initcall+0x68/0x128   [<0000000000a4eb56>] kernel_init_freeable+0x20a/0x2b4   [<00000000006a1ff4>] kernel_init+0x30/0x178   [<00000000006b69fe>] kernel_thread_starter+0x6/0xc   [<00000000006b69f8>] kernel_thread_starter+0x0/0xc  Last Breaking-Event-Address:   [<000000000044bf42>] strsep+0x36/0xa0 Fixes commit: adf53a7 ima: new templates management mechanism Changelog v1: - make template_fmt 'const char *' (reported-by James Morris) - fix kstrdup memory leak (reported-by James Morris) Reported-by: Heiko Carstens Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar Tested-by: Heiko Carstens --- security/integrity/ima/ima_template.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) (limited to 'security/integrity/ima') diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c index 4e5da99..913e192 100644 --- a/security/integrity/ima/ima_template.c +++ b/security/integrity/ima/ima_template.c @@ -90,7 +90,7 @@ static struct ima_template_field *lookup_template_field(const char *field_id) return NULL; } -static int template_fmt_size(char *template_fmt) +static int template_fmt_size(const char *template_fmt) { char c; int template_fmt_len = strlen(template_fmt); @@ -106,23 +106,28 @@ static int template_fmt_size(char *template_fmt) return j + 1; } -static int template_desc_init_fields(char *template_fmt, +static int template_desc_init_fields(const char *template_fmt, struct ima_template_field ***fields, int *num_fields) { - char *c, *template_fmt_ptr = template_fmt; + char *c, *template_fmt_copy; int template_num_fields = template_fmt_size(template_fmt); int i, result = 0; if (template_num_fields > IMA_TEMPLATE_NUM_FIELDS_MAX) return -EINVAL; + /* copying is needed as strsep() modifies the original buffer */ + template_fmt_copy = kstrdup(template_fmt, GFP_KERNEL); + if (template_fmt_copy == NULL) + return -ENOMEM; + *fields = kzalloc(template_num_fields * sizeof(*fields), GFP_KERNEL); if (*fields == NULL) { result = -ENOMEM; goto out; } - for (i = 0; (c = strsep(&template_fmt_ptr, "|")) != NULL && + for (i = 0; (c = strsep(&template_fmt_copy, "|")) != NULL && i < template_num_fields; i++) { struct ima_template_field *f = lookup_template_field(c); @@ -133,10 +138,12 @@ static int template_desc_init_fields(char *template_fmt, (*fields)[i] = f; } *num_fields = i; - return 0; out: - kfree(*fields); - *fields = NULL; + if (result < 0) { + kfree(*fields); + *fields = NULL; + } + kfree(template_fmt_copy); return result; } -- cgit v1.1 From af91706d5ddecb4a9858cca9e90d463037cfd498 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Wed, 27 Nov 2013 14:40:41 +0100 Subject: ima: store address of template_fmt_copy in a pointer before calling strsep This patch stores the address of the 'template_fmt_copy' variable in a new variable, called 'template_fmt_ptr', so that the latter is passed as an argument of strsep() instead of the former. This modification is needed in order to correctly free the memory area referenced by 'template_fmt_copy' (strsep() modifies the pointer of the passed string). Signed-off-by: Roberto Sassu Reported-by: Sebastian Ott Signed-off-by: Mimi Zohar Signed-off-by: James Morris --- security/integrity/ima/ima_template.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'security/integrity/ima') diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c index 913e192..635695f 100644 --- a/security/integrity/ima/ima_template.c +++ b/security/integrity/ima/ima_template.c @@ -110,7 +110,7 @@ static int template_desc_init_fields(const char *template_fmt, struct ima_template_field ***fields, int *num_fields) { - char *c, *template_fmt_copy; + char *c, *template_fmt_copy, *template_fmt_ptr; int template_num_fields = template_fmt_size(template_fmt); int i, result = 0; @@ -127,7 +127,9 @@ static int template_desc_init_fields(const char *template_fmt, result = -ENOMEM; goto out; } - for (i = 0; (c = strsep(&template_fmt_copy, "|")) != NULL && + + template_fmt_ptr = template_fmt_copy; + for (i = 0; (c = strsep(&template_fmt_ptr, "|")) != NULL && i < template_num_fields; i++) { struct ima_template_field *f = lookup_template_field(c); -- cgit v1.1 From 09ae6345721afbb7cf3e0920209b140cbe7bff0d Mon Sep 17 00:00:00 2001 From: Christoph Paasch Date: Mon, 2 Dec 2013 00:05:20 +0100 Subject: ima: Do not free 'entry' before it is initialized 7bc5f447ce9d0 (ima: define new function ima_alloc_init_template() to API) moved the initialization of 'entry' in ima_add_boot_aggregate() a bit more below, after the if (ima_used_chip). So, 'entry' is not initialized while being inside this if-block. So, we should not attempt to free it. Found by Coverity (CID: 1131971) Fixes: 7bc5f447ce9d0 (ima: define new function ima_alloc_init_template() to API) Signed-off-by: Christoph Paasch Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_init.c | 1 - 1 file changed, 1 deletion(-) (limited to 'security/integrity/ima') diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c index 15f34bd..76b8e2c 100644 --- a/security/integrity/ima/ima_init.c +++ b/security/integrity/ima/ima_init.c @@ -63,7 +63,6 @@ static void __init ima_add_boot_aggregate(void) result = ima_calc_boot_aggregate(&hash.hdr); if (result < 0) { audit_cause = "hashing_error"; - kfree(entry); goto err_out; } } -- cgit v1.1 From a7ed7c60e14df5b986f93549717235b882643e7e Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Mon, 2 Dec 2013 19:40:34 +0100 Subject: ima: properly free ima_template_entry structures The new templates management mechanism records information associated to an event into an array of 'ima_field_data' structures and makes it available through the 'template_data' field of the 'ima_template_entry' structure (the element of the measurements list created by IMA). Since 'ima_field_data' contains dynamically allocated data (which length varies depending on the data associated to a selected template field), it is not enough to just free the memory reserved for a 'ima_template_entry' structure if something goes wrong. This patch creates the new function ima_free_template_entry() which walks the array of 'ima_field_data' structures, frees the memory referenced by the 'data' pointer and finally the space reserved for the 'ima_template_entry' structure. Further, it replaces existing kfree() that have a pointer to an 'ima_template_entry' structure as argument with calls to the new function. Fixes: a71dc65: ima: switch to new template management mechanism Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/ima/ima.h | 1 + security/integrity/ima/ima_api.c | 21 +++++++++++++++++---- security/integrity/ima/ima_init.c | 2 +- 3 files changed, 19 insertions(+), 5 deletions(-) (limited to 'security/integrity/ima') diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 9636e17..0356e1d 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -148,6 +148,7 @@ int ima_alloc_init_template(struct integrity_iint_cache *iint, int xattr_len, struct ima_template_entry **entry); int ima_store_template(struct ima_template_entry *entry, int violation, struct inode *inode, const unsigned char *filename); +void ima_free_template_entry(struct ima_template_entry *entry); const char *ima_d_path(struct path *path, char **pathbuf); /* rbtree tree calls to lookup, insert, delete diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index 8037484..c38bbce 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -22,6 +22,19 @@ #include "ima.h" /* + * ima_free_template_entry - free an existing template entry + */ +void ima_free_template_entry(struct ima_template_entry *entry) +{ + int i; + + for (i = 0; i < entry->template_desc->num_fields; i++) + kfree(entry->template_data[i].data); + + kfree(entry); +} + +/* * ima_alloc_init_template - create and initialize a new template entry */ int ima_alloc_init_template(struct integrity_iint_cache *iint, @@ -37,6 +50,7 @@ int ima_alloc_init_template(struct integrity_iint_cache *iint, if (!*entry) return -ENOMEM; + (*entry)->template_desc = template_desc; for (i = 0; i < template_desc->num_fields; i++) { struct ima_template_field *field = template_desc->fields[i]; u32 len; @@ -51,10 +65,9 @@ int ima_alloc_init_template(struct integrity_iint_cache *iint, (*entry)->template_data_len += sizeof(len); (*entry)->template_data_len += len; } - (*entry)->template_desc = template_desc; return 0; out: - kfree(*entry); + ima_free_template_entry(*entry); *entry = NULL; return result; } @@ -134,7 +147,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename, } result = ima_store_template(entry, violation, inode, filename); if (result < 0) - kfree(entry); + ima_free_template_entry(entry); err_out: integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode, filename, op, cause, result, 0); @@ -269,7 +282,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint, if (!result || result == -EEXIST) iint->flags |= IMA_MEASURED; if (result < 0) - kfree(entry); + ima_free_template_entry(entry); } void ima_audit_measurement(struct integrity_iint_cache *iint, diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c index 76b8e2c..3712276 100644 --- a/security/integrity/ima/ima_init.c +++ b/security/integrity/ima/ima_init.c @@ -75,7 +75,7 @@ static void __init ima_add_boot_aggregate(void) result = ima_store_template(entry, violation, NULL, boot_aggregate_name); if (result < 0) - kfree(entry); + ima_free_template_entry(entry); return; err_out: integrity_audit_msg(AUDIT_INTEGRITY_PCR, NULL, boot_aggregate_name, op, -- cgit v1.1