* [PATCH 1/4] KEYS: handle error code encoded in pointer
2014-10-03 9:09 [PATCH 0/4] KEYS fixes Dmitry Kasatkin
@ 2014-10-03 9:09 ` Dmitry Kasatkin
2014-10-03 9:09 ` [PATCH 2/4] KEYS: provide pure subject key identifier (fingerprint) as key id Dmitry Kasatkin
` (8 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Dmitry Kasatkin @ 2014-10-03 9:09 UTC (permalink / raw)
To: zohar, linux-ima-devel, linux-security-module, dhowells, jmorris,
rusty, keyrings
Cc: linux-kernel, dmitry.kasatkin, Dmitry Kasatkin
If hexlen is odd then function returns an error.
Use IS_ERR to check for error, otherwise invalid pointer
is used and kernel gives oops:
[ 132.816522] BUG: unable to handle kernel paging request at
ffffffffffffffea
[ 132.819902] IP: [<ffffffff812bfc20>] asymmetric_key_id_same+0x14/0x36
[ 132.820302] PGD 1a12067 PUD 1a14067 PMD 0
[ 132.820302] Oops: 0000 [#1] SMP
[ 132.820302] Modules linked in: bridge(E) stp(E) llc(E) evdev(E)
serio_raw(E) i2c_piix4(E) button(E) fuse(E)
[ 132.820302] CPU: 0 PID: 2993 Comm: cat Tainted: G E
3.16.0-kds+ #2847
[ 132.820302] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[ 132.820302] task: ffff88004249a430 ti: ffff880056640000 task.ti:
ffff880056640000
[ 132.820302] RIP: 0010:[<ffffffff812bfc20>] [<ffffffff812bfc20>]
asymmetric_key_id_same+0x14/0x36
[ 132.820302] RSP: 0018:ffff880056643930 EFLAGS: 00010246
[ 132.820302] RAX: 0000000000000000 RBX: ffffffffffffffea RCX:
ffff880056643ae0
[ 132.820302] RDX: 000000000000005e RSI: ffffffffffffffea RDI:
ffff88005bac9300
[ 132.820302] RBP: ffff880056643948 R08: 0000000000000003 R09:
00000007504aa01a
[ 132.820302] R10: 0000000000000000 R11: 0000000000000000 R12:
ffff88005d68ca40
[ 132.820302] R13: 0000000000000101 R14: 0000000000000000 R15:
ffff88005bac5280
[ 132.820302] FS: 00007f67a153c740(0000) GS:ffff88005da00000(0000)
knlGS:0000000000000000
[ 132.820302] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 132.820302] CR2: ffffffffffffffea CR3: 000000002e663000 CR4:
00000000000006f0
[ 132.820302] Stack:
[ 132.820302] ffffffff812bfc66 ffff880056643ae0 ffff88005bac5280
ffff880056643958
[ 132.820302] ffffffff812bfc9d ffff880056643980 ffffffff812971d9
ffff88005ce930c1
[ 132.820302] ffff88005ce930c0 0000000000000000 ffff8800566439c8
ffffffff812fb753
[ 132.820302] Call Trace:
[ 132.820302] [<ffffffff812bfc66>] ? asymmetric_match_key_ids+0x24/0x42
[ 132.820302] [<ffffffff812bfc9d>] asymmetric_key_cmp+0x19/0x1b
[ 132.820302] [<ffffffff812971d9>] keyring_search_iterator+0x74/0xd7
[ 132.820302] [<ffffffff812fb753>] assoc_array_subtree_iterate+0x67/0xd2
[ 132.820302] [<ffffffff81297165>] ? key_default_cmp+0x20/0x20
[ 132.820302] [<ffffffff812fbaa1>] assoc_array_iterate+0x19/0x1e
[ 132.820302] [<ffffffff81297332>] search_nested_keyrings+0xf6/0x2b6
[ 132.820302] [<ffffffff810728da>] ? sched_clock_cpu+0x91/0xa2
[ 132.820302] [<ffffffff810860d2>] ? mark_held_locks+0x58/0x6e
[ 132.820302] [<ffffffff810a137d>] ? current_kernel_time+0x77/0xb8
[ 132.820302] [<ffffffff81297871>] keyring_search_aux+0xe1/0x14c
[ 132.820302] [<ffffffff812977fc>] ? keyring_search_aux+0x6c/0x14c
[ 132.820302] [<ffffffff8129796b>] keyring_search+0x8f/0xb6
[ 132.820302] [<ffffffff812bfc84>] ? asymmetric_match_key_ids+0x42/0x42
[ 132.820302] [<ffffffff81297165>] ? key_default_cmp+0x20/0x20
[ 132.820302] [<ffffffff812ab9e3>] asymmetric_verify+0xa4/0x214
[ 132.820302] [<ffffffff812ab90e>] integrity_digsig_verify+0xb1/0xe2
[ 132.820302] [<ffffffff812abe41>] ? evm_verifyxattr+0x6a/0x7a
[ 132.820302] [<ffffffff812b0390>] ima_appraise_measurement+0x160/0x370
[ 132.820302] [<ffffffff81161db2>] ? d_absolute_path+0x5b/0x7a
[ 132.820302] [<ffffffff812ada30>] process_measurement+0x322/0x404
Reported-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
crypto/asymmetric_keys/asymmetric_type.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
index f0f2111..29983cb 100644
--- a/crypto/asymmetric_keys/asymmetric_type.c
+++ b/crypto/asymmetric_keys/asymmetric_type.c
@@ -153,8 +153,8 @@ static int asymmetric_key_match_preparse(struct key_match_data *match_data)
}
match_id = asymmetric_key_hex_to_key_id(id);
- if (!match_id)
- return -ENOMEM;
+ if (IS_ERR(match_id))
+ return PTR_ERR(match_id);
match_data->preparsed = match_id;
match_data->cmp = asymmetric_key_cmp;
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 2/4] KEYS: provide pure subject key identifier (fingerprint) as key id
2014-10-03 9:09 [PATCH 0/4] KEYS fixes Dmitry Kasatkin
2014-10-03 9:09 ` [PATCH 1/4] KEYS: handle error code encoded in pointer Dmitry Kasatkin
@ 2014-10-03 9:09 ` Dmitry Kasatkin
2014-10-03 9:09 ` [PATCH 3/4] module: search the key only by keyid Dmitry Kasatkin
` (7 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Dmitry Kasatkin @ 2014-10-03 9:09 UTC (permalink / raw)
To: zohar, linux-ima-devel, linux-security-module, dhowells, jmorris,
rusty, keyrings
Cc: linux-kernel, dmitry.kasatkin, Dmitry Kasatkin
Earlier KEYs code used pure subject key identifies for
searching keys. Latest merged code removed that and broke
compatibility with integrity subsytem signatures and original
format of module signatures.
This patch returns back fingerprint and partial matching.
Reported-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
crypto/asymmetric_keys/asymmetric_type.c | 23 ++++++++++++++++++++++-
crypto/asymmetric_keys/x509_cert_parser.c | 6 ++++++
crypto/asymmetric_keys/x509_parser.h | 1 +
crypto/asymmetric_keys/x509_public_key.c | 2 ++
include/keys/asymmetric-type.h | 2 +-
5 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
index 29983cb..9c83129 100644
--- a/crypto/asymmetric_keys/asymmetric_type.c
+++ b/crypto/asymmetric_keys/asymmetric_type.c
@@ -45,7 +45,8 @@ struct asymmetric_key_id *asymmetric_key_generate_id(const void *val_1,
return ERR_PTR(-ENOMEM);
kid->len = len_1 + len_2;
memcpy(kid->data, val_1, len_1);
- memcpy(kid->data + len_1, val_2, len_2);
+ if (val_2)
+ memcpy(kid->data + len_1, val_2, len_2);
return kid;
}
EXPORT_SYMBOL_GPL(asymmetric_key_generate_id);
@@ -66,6 +67,23 @@ bool asymmetric_key_id_same(const struct asymmetric_key_id *kid1,
EXPORT_SYMBOL_GPL(asymmetric_key_id_same);
/**
+ * asymmetric_key_id_partial - Return true if two asymmetric keys IDs
+ * partially match
+ * @kid_1, @kid_2: The key IDs to compare
+ */
+bool asymmetric_key_id_partial(const struct asymmetric_key_id *kid1,
+ const struct asymmetric_key_id *kid2)
+{
+ if (!kid1 || !kid2)
+ return false;
+ if (kid1->len < kid2->len)
+ return false;
+ return memcmp(kid1->data + (kid1->len - kid2->len),
+ kid2->data, kid2->len) == 0;
+}
+EXPORT_SYMBOL_GPL(asymmetric_key_id_partial);
+
+/**
* asymmetric_match_key_ids - Search asymmetric key IDs
* @kids: The list of key IDs to check
* @match_id: The key ID we're looking for
@@ -79,6 +97,8 @@ bool asymmetric_match_key_ids(const struct asymmetric_key_ids *kids,
return true;
if (asymmetric_key_id_same(kids->id[1], match_id))
return true;
+ if (asymmetric_key_id_partial(kids->id[2], match_id))
+ return true;
return false;
}
EXPORT_SYMBOL_GPL(asymmetric_match_key_ids);
@@ -261,6 +281,7 @@ static void asymmetric_key_free_preparse(struct key_preparsed_payload *prep)
if (kids) {
kfree(kids->id[0]);
kfree(kids->id[1]);
+ kfree(kids->id[2]);
kfree(kids);
}
kfree(prep->description);
diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
index 96151b2..7cf36fe 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -48,6 +48,7 @@ void x509_free_certificate(struct x509_certificate *cert)
kfree(cert->subject);
kfree(cert->id);
kfree(cert->skid);
+ kfree(cert->skid_only);
kfree(cert->authority);
kfree(cert->sig.digest);
mpi_free(cert->sig.rsa.s);
@@ -442,6 +443,11 @@ int x509_process_extension(void *context, size_t hdrlen,
return PTR_ERR(kid);
ctx->cert->skid = kid;
pr_debug("subjkeyid %*phN\n", kid->len, kid->data);
+ kid = asymmetric_key_generate_id(v, vlen, NULL, 0);
+ if (IS_ERR(kid))
+ return PTR_ERR(kid);
+ ctx->cert->skid_only = kid;
+ pr_debug("subjkeyid %*phN\n", kid->len, kid->data);
return 0;
}
diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
index 4e1a384..a0b3052 100644
--- a/crypto/asymmetric_keys/x509_parser.h
+++ b/crypto/asymmetric_keys/x509_parser.h
@@ -21,6 +21,7 @@ struct x509_certificate {
char *subject; /* Name of certificate subject */
struct asymmetric_key_id *id; /* Issuer + serial number */
struct asymmetric_key_id *skid; /* Subject key identifier */
+ struct asymmetric_key_id *skid_only; /* Subject key identifier only*/
struct asymmetric_key_id *authority; /* Authority key identifier */
struct tm valid_from;
struct tm valid_to;
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 1d9a4c5..a0673c9 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -302,6 +302,7 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
goto error_free_desc;
kids->id[0] = cert->id;
kids->id[1] = cert->skid;
+ kids->id[2] = cert->skid_only;
/* We're pinning the module by being linked against it */
__module_get(public_key_subtype.owner);
@@ -315,6 +316,7 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
cert->pub = NULL;
cert->id = NULL;
cert->skid = NULL;
+ cert->skid_only = NULL;
desc = NULL;
ret = 0;
diff --git a/include/keys/asymmetric-type.h b/include/keys/asymmetric-type.h
index 044ab0d..54fb119 100644
--- a/include/keys/asymmetric-type.h
+++ b/include/keys/asymmetric-type.h
@@ -45,7 +45,7 @@ struct asymmetric_key_id {
};
struct asymmetric_key_ids {
- void *id[2];
+ void *id[3];
};
extern bool asymmetric_key_id_same(const struct asymmetric_key_id *kid1,
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 3/4] module: search the key only by keyid
2014-10-03 9:09 [PATCH 0/4] KEYS fixes Dmitry Kasatkin
2014-10-03 9:09 ` [PATCH 1/4] KEYS: handle error code encoded in pointer Dmitry Kasatkin
2014-10-03 9:09 ` [PATCH 2/4] KEYS: provide pure subject key identifier (fingerprint) as key id Dmitry Kasatkin
@ 2014-10-03 9:09 ` Dmitry Kasatkin
2014-10-03 9:09 ` [PATCH 4/4] integrity: do zero padding of the key id Dmitry Kasatkin
` (6 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Dmitry Kasatkin @ 2014-10-03 9:09 UTC (permalink / raw)
To: zohar, linux-ima-devel, linux-security-module, dhowells, jmorris,
rusty, keyrings
Cc: linux-kernel, dmitry.kasatkin, Dmitry Kasatkin
Latest KEYS code change the way keys identified and module
signing keys are not searchable anymore with original id.
This patch fixes this problem without change module signature
data.
Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
kernel/module_signing.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index be5b8fa..f329de8 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -134,26 +134,20 @@ static struct key *request_asymmetric_key(const char *signer, size_t signer_len,
const u8 *key_id, size_t key_id_len)
{
key_ref_t key;
- size_t i;
char *id, *q;
pr_devel("==>%s(,%zu,,%zu)\n", __func__, signer_len, key_id_len);
/* Construct an identifier. */
- id = kmalloc(signer_len + 2 + key_id_len * 2 + 1, GFP_KERNEL);
+ id = kmalloc(2 + 1 + key_id_len * 2 + 1, GFP_KERNEL);
if (!id)
return ERR_PTR(-ENOKEY);
- memcpy(id, signer, signer_len);
-
- q = id + signer_len;
+ q = id;
+ *q++ = 'i';
+ *q++ = 'd';
*q++ = ':';
- *q++ = ' ';
- for (i = 0; i < key_id_len; i++) {
- *q++ = hex_asc[*key_id >> 4];
- *q++ = hex_asc[*key_id++ & 0x0f];
- }
-
+ q = bin2hex(q, key_id, key_id_len);
*q = 0;
pr_debug("Look up: \"%s\"\n", id);
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 4/4] integrity: do zero padding of the key id
2014-10-03 9:09 [PATCH 0/4] KEYS fixes Dmitry Kasatkin
` (2 preceding siblings ...)
2014-10-03 9:09 ` [PATCH 3/4] module: search the key only by keyid Dmitry Kasatkin
@ 2014-10-03 9:09 ` Dmitry Kasatkin
2014-10-03 10:43 ` [PATCH 0/4] KEYS fixes Dmitry Kasatkin
` (5 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Dmitry Kasatkin @ 2014-10-03 9:09 UTC (permalink / raw)
To: zohar, linux-ima-devel, linux-security-module, dhowells, jmorris,
rusty, keyrings
Cc: linux-kernel, dmitry.kasatkin, Dmitry Kasatkin
Latest KEYS code return error if hexadecimal string length id odd.
Fix it.
Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
security/integrity/digsig_asymmetric.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c
index 37e0d98..4fec181 100644
--- a/security/integrity/digsig_asymmetric.c
+++ b/security/integrity/digsig_asymmetric.c
@@ -28,7 +28,7 @@ static struct key *request_asymmetric_key(struct key *keyring, uint32_t keyid)
struct key *key;
char name[12];
- sprintf(name, "id:%x", keyid);
+ sprintf(name, "id:%08x", keyid);
pr_debug("key search: \"%s\"\n", name);
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 0/4] KEYS fixes
2014-10-03 9:09 [PATCH 0/4] KEYS fixes Dmitry Kasatkin
` (3 preceding siblings ...)
2014-10-03 9:09 ` [PATCH 4/4] integrity: do zero padding of the key id Dmitry Kasatkin
@ 2014-10-03 10:43 ` Dmitry Kasatkin
2014-10-03 12:46 ` [PATCH 3/4] module: search the key only by keyid David Howells
` (4 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Dmitry Kasatkin @ 2014-10-03 10:43 UTC (permalink / raw)
To: dhowells, keyrings
Cc: zohar, linux-ima-devel, linux-security-module, jmorris, rusty,
linux-kernel, dmitry.kasatkin
Hi David,
I slightly update what I posted and added one fix and one suggestion on
the top.
Here is those patches on the top of Jame's tree...
http://git.kernel.org/cgit/linux/kernel/git/kasatkin/linux-digsig.git/log/?h=keys-fixes
- Dmitry
On 03/10/14 12:09, Dmitry Kasatkin wrote:
> I reported yesterday problems with new KEYS.
> Module signature verification is broken, integrity subsystem verification is
> broken, kernel oopses.
>
> Here is few fixes.
>
> - Dmitry
>
> Dmitry Kasatkin (4):
> KEYS: handle error code encoded in pointer
> KEYS: provide pure subject key identifier (fingerprint) as key id
> module: search the key only by keyid
> integrity: do zero padding of the key id
>
> crypto/asymmetric_keys/asymmetric_type.c | 27 ++++++++++++++++++++++++---
> crypto/asymmetric_keys/x509_cert_parser.c | 6 ++++++
> crypto/asymmetric_keys/x509_parser.h | 1 +
> crypto/asymmetric_keys/x509_public_key.c | 2 ++
> include/keys/asymmetric-type.h | 2 +-
> kernel/module_signing.c | 16 +++++-----------
> security/integrity/digsig_asymmetric.c | 2 +-
> 7 files changed, 40 insertions(+), 16 deletions(-)
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 3/4] module: search the key only by keyid
2014-10-03 9:09 [PATCH 0/4] KEYS fixes Dmitry Kasatkin
` (4 preceding siblings ...)
2014-10-03 10:43 ` [PATCH 0/4] KEYS fixes Dmitry Kasatkin
@ 2014-10-03 12:46 ` David Howells
2014-10-03 12:49 ` Dmitry Kasatkin
2014-10-03 12:46 ` [PATCH 1/4] KEYS: handle error code encoded in pointer David Howells
` (3 subsequent siblings)
9 siblings, 1 reply; 19+ messages in thread
From: David Howells @ 2014-10-03 12:46 UTC (permalink / raw)
To: Dmitry Kasatkin
Cc: dhowells, zohar, linux-ima-devel, linux-security-module, jmorris,
rusty, keyrings, linux-kernel, dmitry.kasatkin
Dmitry Kasatkin <d.kasatkin@samsung.com> wrote:
> Latest KEYS code change the way keys identified and module
> signing keys are not searchable anymore with original id.
>
> This patch fixes this problem without change module signature
> data.
This isn't sufficient. The key search must also include the signer.
David
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 3/4] module: search the key only by keyid
2014-10-03 12:46 ` [PATCH 3/4] module: search the key only by keyid David Howells
@ 2014-10-03 12:49 ` Dmitry Kasatkin
2014-10-03 12:53 ` Dmitry Kasatkin
0 siblings, 1 reply; 19+ messages in thread
From: Dmitry Kasatkin @ 2014-10-03 12:49 UTC (permalink / raw)
To: David Howells
Cc: zohar, linux-ima-devel, linux-security-module, jmorris, rusty,
keyrings, linux-kernel, dmitry.kasatkin
On 03/10/14 15:46, David Howells wrote:
> Dmitry Kasatkin <d.kasatkin@samsung.com> wrote:
>
>> Latest KEYS code change the way keys identified and module
>> signing keys are not searchable anymore with original id.
>>
>> This patch fixes this problem without change module signature
>> data.
> This isn't sufficient. The key search must also include the signer.
>
IMA uses "id:<id>" partial matching.. There is no signer in the signature.
It is added as "last resort"
It is here... the same but I renamed with finger print..
http://git.kernel.org/cgit/linux/kernel/git/kasatkin/linux-digsig.git/commit/?h=keys-fixes&id=f036bb9a4c1b3c548f315226d3284e6a91d284e7
- Dmitry
> David
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] module: search the key only by keyid
2014-10-03 12:49 ` Dmitry Kasatkin
@ 2014-10-03 12:53 ` Dmitry Kasatkin
2014-10-03 13:08 ` Dmitry Kasatkin
2014-10-03 13:40 ` David Howells
0 siblings, 2 replies; 19+ messages in thread
From: Dmitry Kasatkin @ 2014-10-03 12:53 UTC (permalink / raw)
To: David Howells
Cc: zohar, linux-ima-devel, linux-security-module, jmorris, rusty,
keyrings, linux-kernel, dmitry.kasatkin
On 03/10/14 15:49, Dmitry Kasatkin wrote:
> On 03/10/14 15:46, David Howells wrote:
>> Dmitry Kasatkin <d.kasatkin@samsung.com> wrote:
>>
>>> Latest KEYS code change the way keys identified and module
>>> signing keys are not searchable anymore with original id.
>>>
>>> This patch fixes this problem without change module signature
>>> data.
>> This isn't sufficient. The key search must also include the signer.
>>
> IMA uses "id:<id>" partial matching.. There is no signer in the signature.
> It is added as "last resort"
>
> It is here... the same but I renamed with finger print..
>
> http://git.kernel.org/cgit/linux/kernel/git/kasatkin/linux-digsig.git/commit/?h=keys-fixes&id=f036bb9a4c1b3c548f315226d3284e6a91d284e7
>
> - Dmitry
>
>
For module actually I made it as a fix because it was broken.
Other requires changes in module signature format...
- Dmitry
>> David
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] module: search the key only by keyid
2014-10-03 12:53 ` Dmitry Kasatkin
@ 2014-10-03 13:08 ` Dmitry Kasatkin
2014-10-03 13:40 ` David Howells
1 sibling, 0 replies; 19+ messages in thread
From: Dmitry Kasatkin @ 2014-10-03 13:08 UTC (permalink / raw)
To: David Howells
Cc: zohar, linux-ima-devel, linux-security-module, jmorris, rusty,
keyrings, linux-kernel, dmitry.kasatkin
On 03/10/14 15:53, Dmitry Kasatkin wrote:
> On 03/10/14 15:49, Dmitry Kasatkin wrote:
>> On 03/10/14 15:46, David Howells wrote:
>>> Dmitry Kasatkin <d.kasatkin@samsung.com> wrote:
>>>
>>>> Latest KEYS code change the way keys identified and module
>>>> signing keys are not searchable anymore with original id.
>>>>
>>>> This patch fixes this problem without change module signature
>>>> data.
>>> This isn't sufficient. The key search must also include the signer.
BTW. But actually why signer is needed to find the key?
Every key has unique fingerprint.
Or you say that different certificates might have the same PK?
What I would consider strange. But anyway, if PK is the same, then
verification succeed.
- Dmitry
>> IMA uses "id:<id>" partial matching.. There is no signer in the signature.
>> It is added as "last resort"
>>
>> It is here... the same but I renamed with finger print..
>>
>> http://git.kernel.org/cgit/linux/kernel/git/kasatkin/linux-digsig.git/commit/?h=keys-fixes&id=f036bb9a4c1b3c548f315226d3284e6a91d284e7
>>
>> - Dmitry
>>
>>
> For module actually I made it as a fix because it was broken.
> Other requires changes in module signature format...
>
> - Dmitry
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] module: search the key only by keyid
2014-10-03 12:53 ` Dmitry Kasatkin
2014-10-03 13:08 ` Dmitry Kasatkin
@ 2014-10-03 13:40 ` David Howells
2014-10-03 14:00 ` Dmitry Kasatkin
2014-10-06 12:44 ` James Morris
1 sibling, 2 replies; 19+ messages in thread
From: David Howells @ 2014-10-03 13:40 UTC (permalink / raw)
To: Dmitry Kasatkin
Cc: dhowells, zohar, linux-ima-devel, linux-security-module, jmorris,
rusty, keyrings, linux-kernel, dmitry.kasatkin
Dmitry Kasatkin <d.kasatkin@samsung.com> wrote:
> BTW. But actually why signer is needed to find the key?
> Every key has unique fingerprint.
The SKID is by no means guaranteed unique, is not mandatory and has no defined
algorithm for generating it.
> Or you say that different certificates might have the same PK?
> What I would consider strange. But anyway, if PK is the same, then
> verification succeed.
Do note: We *do* need to get away from using SKIDs. We have situations where
we have to use a key that doesn't have one.
David
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] module: search the key only by keyid
2014-10-03 13:40 ` David Howells
@ 2014-10-03 14:00 ` Dmitry Kasatkin
2014-10-06 12:44 ` James Morris
1 sibling, 0 replies; 19+ messages in thread
From: Dmitry Kasatkin @ 2014-10-03 14:00 UTC (permalink / raw)
To: David Howells
Cc: zohar, linux-ima-devel, linux-security-module, jmorris, rusty,
keyrings, linux-kernel, dmitry.kasatkin
On 03/10/14 16:40, David Howells wrote:
> Dmitry Kasatkin <d.kasatkin@samsung.com> wrote:
>
>> BTW. But actually why signer is needed to find the key?
>> Every key has unique fingerprint.
> The SKID is by no means guaranteed unique, is not mandatory and has no defined
> algorithm for generating it.
SKID is unique. SKID == SHA1(PK)
I understand that it may be missing for someone. But if it presents in
the certificate it should not be a problem...
>> Or you say that different certificates might have the same PK?
>> What I would consider strange. But anyway, if PK is the same, then
>> verification succeed.
> Do note: We *do* need to get away from using SKIDs. We have situations where
> we have to use a key that doesn't have one.
>
> David
I understand that... What I claim is that if there is a SKID, it is
unique and enough to identify certificate in the keyring...
Integrity subsystem uses partial SKID and it MUST NOT be broken for
compatibility.
Dmitry
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] module: search the key only by keyid
2014-10-03 13:40 ` David Howells
2014-10-03 14:00 ` Dmitry Kasatkin
@ 2014-10-06 12:44 ` James Morris
2014-10-06 17:14 ` Dmitry Kasatkin
2014-10-06 19:39 ` Mimi Zohar
1 sibling, 2 replies; 19+ messages in thread
From: James Morris @ 2014-10-06 12:44 UTC (permalink / raw)
To: David Howells
Cc: Dmitry Kasatkin, zohar, linux-ima-devel, linux-security-module,
rusty, keyrings, linux-kernel, dmitry.kasatkin
On Fri, 3 Oct 2014, David Howells wrote:
> Dmitry Kasatkin <d.kasatkin@samsung.com> wrote:
>
> > BTW. But actually why signer is needed to find the key?
> > Every key has unique fingerprint.
>
> The SKID is by no means guaranteed unique, is not mandatory and has no defined
> algorithm for generating it.
>
> > Or you say that different certificates might have the same PK?
> > What I would consider strange. But anyway, if PK is the same, then
> > verification succeed.
>
> Do note: We *do* need to get away from using SKIDs. We have situations where
> we have to use a key that doesn't have one.
>
David, I need to push to Linus for 3.17 -- please finalize the fix for
this and send me a pull request.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] module: search the key only by keyid
2014-10-06 12:44 ` James Morris
@ 2014-10-06 17:14 ` Dmitry Kasatkin
2014-10-06 19:39 ` Mimi Zohar
1 sibling, 0 replies; 19+ messages in thread
From: Dmitry Kasatkin @ 2014-10-06 17:14 UTC (permalink / raw)
To: James Morris, David Howells
Cc: zohar, linux-ima-devel, linux-security-module, rusty, keyrings,
linux-kernel, dmitry.kasatkin
On 06/10/14 15:44, James Morris wrote:
> On Fri, 3 Oct 2014, David Howells wrote:
>
>> Dmitry Kasatkin <d.kasatkin@samsung.com> wrote:
>>
>>> BTW. But actually why signer is needed to find the key?
>>> Every key has unique fingerprint.
>> The SKID is by no means guaranteed unique, is not mandatory and has no defined
>> algorithm for generating it.
>>
>>> Or you say that different certificates might have the same PK?
>>> What I would consider strange. But anyway, if PK is the same, then
>>> verification succeed.
>> Do note: We *do* need to get away from using SKIDs. We have situations where
>> we have to use a key that doesn't have one.
>>
> David, I need to push to Linus for 3.17 -- please finalize the fix for
> this and send me a pull request.
>
>
>
Hi David,
I tested KEYS fixes and it works well for modules and integrity.
http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-next-fixes
Thanks!
- Dmitry
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] module: search the key only by keyid
2014-10-06 12:44 ` James Morris
2014-10-06 17:14 ` Dmitry Kasatkin
@ 2014-10-06 19:39 ` Mimi Zohar
1 sibling, 0 replies; 19+ messages in thread
From: Mimi Zohar @ 2014-10-06 19:39 UTC (permalink / raw)
To: James Morris
Cc: David Howells, Dmitry Kasatkin, linux-ima-devel,
linux-security-module, rusty, keyrings, linux-kernel,
dmitry.kasatkin
On Mon, 2014-10-06 at 23:44 +1100, James Morris wrote:
> On Fri, 3 Oct 2014, David Howells wrote:
>
> > Dmitry Kasatkin <d.kasatkin@samsung.com> wrote:
> >
> > > BTW. But actually why signer is needed to find the key?
> > > Every key has unique fingerprint.
> >
> > The SKID is by no means guaranteed unique, is not mandatory and has no defined
> > algorithm for generating it.
> >
> > > Or you say that different certificates might have the same PK?
> > > What I would consider strange. But anyway, if PK is the same, then
> > > verification succeed.
> >
> > Do note: We *do* need to get away from using SKIDs. We have situations where
> > we have to use a key that doesn't have one.
> >
>
> David, I need to push to Linus for 3.17 -- please finalize the fix for
> this and send me a pull request.
Thanks Dmitry, David. Everything now seems to be working properly with
the patchset David posted today.
Mimi
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] KEYS: handle error code encoded in pointer
2014-10-03 9:09 [PATCH 0/4] KEYS fixes Dmitry Kasatkin
` (5 preceding siblings ...)
2014-10-03 12:46 ` [PATCH 3/4] module: search the key only by keyid David Howells
@ 2014-10-03 12:46 ` David Howells
2014-10-03 14:19 ` [PATCH 0/4] KEYS fixes Mimi Zohar
` (2 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: David Howells @ 2014-10-03 12:46 UTC (permalink / raw)
To: Dmitry Kasatkin
Cc: dhowells, zohar, linux-ima-devel, linux-security-module, jmorris,
rusty, keyrings, linux-kernel, dmitry.kasatkin
Dmitry Kasatkin <d.kasatkin@samsung.com> wrote:
> If hexlen is odd then function returns an error.
> Use IS_ERR to check for error, otherwise invalid pointer
> is used and kernel gives oops:
>
> [ 132.816522] BUG: unable to handle kernel paging request at
> ffffffffffffffea
> [ 132.819902] IP: [<ffffffff812bfc20>] asymmetric_key_id_same+0x14/0x36
> [ 132.820302] PGD 1a12067 PUD 1a14067 PMD 0
> [ 132.820302] Oops: 0000 [#1] SMP
> [ 132.820302] Modules linked in: bridge(E) stp(E) llc(E) evdev(E)
> serio_raw(E) i2c_piix4(E) button(E) fuse(E)
> [ 132.820302] CPU: 0 PID: 2993 Comm: cat Tainted: G E
> 3.16.0-kds+ #2847
> [ 132.820302] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
> [ 132.820302] task: ffff88004249a430 ti: ffff880056640000 task.ti:
> ffff880056640000
> [ 132.820302] RIP: 0010:[<ffffffff812bfc20>] [<ffffffff812bfc20>]
> asymmetric_key_id_same+0x14/0x36
> [ 132.820302] RSP: 0018:ffff880056643930 EFLAGS: 00010246
> [ 132.820302] RAX: 0000000000000000 RBX: ffffffffffffffea RCX:
> ffff880056643ae0
> [ 132.820302] RDX: 000000000000005e RSI: ffffffffffffffea RDI:
> ffff88005bac9300
> [ 132.820302] RBP: ffff880056643948 R08: 0000000000000003 R09:
> 00000007504aa01a
> [ 132.820302] R10: 0000000000000000 R11: 0000000000000000 R12:
> ffff88005d68ca40
> [ 132.820302] R13: 0000000000000101 R14: 0000000000000000 R15:
> ffff88005bac5280
> [ 132.820302] FS: 00007f67a153c740(0000) GS:ffff88005da00000(0000)
> knlGS:0000000000000000
> [ 132.820302] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> [ 132.820302] CR2: ffffffffffffffea CR3: 000000002e663000 CR4:
> 00000000000006f0
> [ 132.820302] Stack:
> [ 132.820302] ffffffff812bfc66 ffff880056643ae0 ffff88005bac5280
> ffff880056643958
> [ 132.820302] ffffffff812bfc9d ffff880056643980 ffffffff812971d9
> ffff88005ce930c1
> [ 132.820302] ffff88005ce930c0 0000000000000000 ffff8800566439c8
> ffffffff812fb753
> [ 132.820302] Call Trace:
> [ 132.820302] [<ffffffff812bfc66>] ? asymmetric_match_key_ids+0x24/0x42
> [ 132.820302] [<ffffffff812bfc9d>] asymmetric_key_cmp+0x19/0x1b
> [ 132.820302] [<ffffffff812971d9>] keyring_search_iterator+0x74/0xd7
> [ 132.820302] [<ffffffff812fb753>] assoc_array_subtree_iterate+0x67/0xd2
> [ 132.820302] [<ffffffff81297165>] ? key_default_cmp+0x20/0x20
> [ 132.820302] [<ffffffff812fbaa1>] assoc_array_iterate+0x19/0x1e
> [ 132.820302] [<ffffffff81297332>] search_nested_keyrings+0xf6/0x2b6
> [ 132.820302] [<ffffffff810728da>] ? sched_clock_cpu+0x91/0xa2
> [ 132.820302] [<ffffffff810860d2>] ? mark_held_locks+0x58/0x6e
> [ 132.820302] [<ffffffff810a137d>] ? current_kernel_time+0x77/0xb8
> [ 132.820302] [<ffffffff81297871>] keyring_search_aux+0xe1/0x14c
> [ 132.820302] [<ffffffff812977fc>] ? keyring_search_aux+0x6c/0x14c
> [ 132.820302] [<ffffffff8129796b>] keyring_search+0x8f/0xb6
> [ 132.820302] [<ffffffff812bfc84>] ? asymmetric_match_key_ids+0x42/0x42
> [ 132.820302] [<ffffffff81297165>] ? key_default_cmp+0x20/0x20
> [ 132.820302] [<ffffffff812ab9e3>] asymmetric_verify+0xa4/0x214
> [ 132.820302] [<ffffffff812ab90e>] integrity_digsig_verify+0xb1/0xe2
> [ 132.820302] [<ffffffff812abe41>] ? evm_verifyxattr+0x6a/0x7a
> [ 132.820302] [<ffffffff812b0390>] ima_appraise_measurement+0x160/0x370
> [ 132.820302] [<ffffffff81161db2>] ? d_absolute_path+0x5b/0x7a
> [ 132.820302] [<ffffffff812ada30>] process_measurement+0x322/0x404
>
> Reported-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
Applied.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 0/4] KEYS fixes
2014-10-03 9:09 [PATCH 0/4] KEYS fixes Dmitry Kasatkin
` (6 preceding siblings ...)
2014-10-03 12:46 ` [PATCH 1/4] KEYS: handle error code encoded in pointer David Howells
@ 2014-10-03 14:19 ` Mimi Zohar
2014-10-03 15:54 ` [PATCH] X.509: If available, use the raw subjKeyId to form the key description David Howells
2014-10-06 13:51 ` [PATCH 2/4] KEYS: provide pure subject key identifier (fingerprint) as key id David Howells
9 siblings, 0 replies; 19+ messages in thread
From: Mimi Zohar @ 2014-10-03 14:19 UTC (permalink / raw)
To: Dmitry Kasatkin
Cc: linux-ima-devel, linux-security-module, dhowells, jmorris, rusty,
keyrings, linux-kernel, dmitry.kasatkin
On Fri, 2014-10-03 at 12:09 +0300, Dmitry Kasatkin wrote:
> I reported yesterday problems with new KEYS.
> Module signature verification is broken, integrity subsystem verification is
> broken, kernel oopses.
>
> Here is few fixes.
Thanks, Dmitry! with these patch fixes, I'm now able to boot normally
with IMA-appraisal enabled in enforcing mode.
David, these two commits broke IMA-appraisal. Either these patches need
to be reverted or the partial match needs to be re-introduced. Dmitry's
patches do the latter.
757932e PKCS#7: Handle PKCS#7 messages that contain no X.509 certs
46963b7 KEYS: Overhaul key identification when searching for asymmetric
keys
Mimi
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH] X.509: If available, use the raw subjKeyId to form the key description
2014-10-03 9:09 [PATCH 0/4] KEYS fixes Dmitry Kasatkin
` (7 preceding siblings ...)
2014-10-03 14:19 ` [PATCH 0/4] KEYS fixes Mimi Zohar
@ 2014-10-03 15:54 ` David Howells
2014-10-06 13:51 ` [PATCH 2/4] KEYS: provide pure subject key identifier (fingerprint) as key id David Howells
9 siblings, 0 replies; 19+ messages in thread
From: David Howells @ 2014-10-03 15:54 UTC (permalink / raw)
To: Dmitry Kasatkin
Cc: dhowells, zohar, linux-ima-devel, linux-security-module, jmorris,
rusty, keyrings, linux-kernel, dmitry.kasatkin
Module signing matches keys by comparing against the key description exactly.
However, the way the key description gets constructed got changed to be
composed of the subject name plus the certificate serial number instead of the
subject name and the subjectKeyId. I changed this to avoid problems with
certificates that don't *have* a subjectKeyId.
Instead, if available, use the raw subjectKeyId to form the key description
and only use the serial number if the subjectKeyId doesn't exist.
Reported-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---
x509_cert_parser.c | 2 ++
x509_parser.h | 2 ++
x509_public_key.c | 9 +++++++--
3 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
index 96151b2b91a2..393706f33fa5 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -435,6 +435,8 @@ int x509_process_extension(void *context, size_t hdrlen,
v += 2;
vlen -= 2;
+ ctx->cert->raw_skid_size = vlen;
+ ctx->cert->raw_skid = v;
kid = asymmetric_key_generate_id(v, vlen,
ctx->cert->raw_subject,
ctx->cert->raw_subject_size);
diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
index 4e1a384901ed..3f0f0f081621 100644
--- a/crypto/asymmetric_keys/x509_parser.h
+++ b/crypto/asymmetric_keys/x509_parser.h
@@ -34,6 +34,8 @@ struct x509_certificate {
const void *raw_issuer; /* Raw issuer name in ASN.1 */
const void *raw_subject; /* Raw subject name in ASN.1 */
unsigned raw_subject_size;
+ unsigned raw_skid_size;
+ const void *raw_skid; /* Raw subjectKeyId in ASN.1 */
unsigned index;
bool seen; /* Infinite recursion prevention */
bool verified;
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 1d9a4c555376..8bffb06b2683 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -279,8 +279,13 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
/* Propose a description */
sulen = strlen(cert->subject);
- srlen = cert->raw_serial_size;
- q = cert->raw_serial;
+ if (cert->raw_skid) {
+ srlen = cert->raw_skid_size;
+ q = cert->raw_skid;
+ } else {
+ srlen = cert->raw_serial_size;
+ q = cert->raw_serial;
+ }
if (srlen > 1 && *q == 0) {
srlen--;
q++;
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 2/4] KEYS: provide pure subject key identifier (fingerprint) as key id
2014-10-03 9:09 [PATCH 0/4] KEYS fixes Dmitry Kasatkin
` (8 preceding siblings ...)
2014-10-03 15:54 ` [PATCH] X.509: If available, use the raw subjKeyId to form the key description David Howells
@ 2014-10-06 13:51 ` David Howells
9 siblings, 0 replies; 19+ messages in thread
From: David Howells @ 2014-10-06 13:51 UTC (permalink / raw)
To: Dmitry Kasatkin
Cc: dhowells, zohar, linux-ima-devel, linux-security-module, jmorris,
rusty, keyrings, linux-kernel, dmitry.kasatkin
Dmitry Kasatkin <d.kasatkin@samsung.com> wrote:
> - memcpy(kid->data + len_1, val_2, len_2);
> + if (val_2)
> + memcpy(kid->data + len_1, val_2, len_2);
Btw, I think this should probably be "if (len_2)" rather than "if (val_2)" so
that we don't bother with the memcpy() if there's no second component.
David
^ permalink raw reply [flat|nested] 19+ messages in thread