[PATCH 0/4] KEYS fixes

[PATCH 0/4] KEYS fixes

[Dmitry Kasatkin]

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(-)

-- 
1.9.1

[PATCH 1/4] KEYS: handle error code encoded in pointer

[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

Re: [PATCH 1/4] KEYS: handle error code encoded in pointer

[David Howells]

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.

[PATCH 2/4] KEYS: provide pure subject key identifier (fingerprint) as key id

[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

Re: [PATCH 2/4] KEYS: provide pure subject key identifier (fingerprint) as key id

[David Howells]

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

[PATCH 3/4] module: search the key only by keyid

[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

Re: [PATCH 3/4] module: search the key only by keyid

[David Howells]

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

Re: [PATCH 3/4] module: search the key only by keyid

[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
>

Re: [PATCH 3/4] module: search the key only by keyid

[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
>

Re: [PATCH 3/4] module: search the key only by keyid

[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
>
>

Re: [PATCH 3/4] module: search the key only by keyid

[David Howells]

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

Re: [PATCH 3/4] module: search the key only by keyid

[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

Re: [PATCH 3/4] module: search the key only by keyid

[James Morris]

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>

Re: [PATCH 3/4] module: search the key only by keyid

[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

Re: [PATCH 3/4] module: search the key only by keyid

[Mimi Zohar]

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 

[PATCH 4/4] integrity: do zero padding of the key id

[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

Re: [PATCH 0/4] KEYS fixes

[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(-)
>

Re: [PATCH 0/4] KEYS fixes

[Mimi Zohar]

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

[PATCH] X.509: If available, use the raw subjKeyId to form the key description

[David Howells]

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++;