Skip to contentSkip to conversation

KEYS fixes

PATCH Applied Dmitry Kasatkin 4 participants 19 messages Oct 3, 2014 – Oct 6, 2014

Subsystems (6)
Crypto API Security (LSM) Keys/keyrings Integrity measurement architecture (IMA) +2 more subsystems
lore
Share & tools
lore

Dmitry Kasatkin

author
#0 lore raw

cover KEYS fixes

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(-)
Signature · 1 line
1.9.1
author
#1 lore raw

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

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 <[email protected]>
Signed-off-by Dmitry Kasatkin <[email protected]>
---
 crypto/asymmetric_keys/asymmetric_type.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
crypto/asymmetric_keys/asymmetric_type.c +2 -2
index f0f2111..29983cb 100644
@@ -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;
Signature · 1 line
1.9.1
maintainer Applied
#2 lore raw

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

Quoted 64 lines · 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
Quoted 2 lines
Reported-by Dmitry Kasatkin <[email protected]>
Signed-off-by Dmitry Kasatkin <[email protected]>

Applied.

author
#3 lore raw

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

In reply to 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 <[email protected]>
Signed-off-by Dmitry Kasatkin <[email protected]>
---
 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(-)
crypto/asymmetric_keys/asymmetric_type.c +22 -1
index 29983cb..9c83129 100644
@@ -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);
crypto/asymmetric_keys/x509_cert_parser.c +6 -0
index 96151b2..7cf36fe 100644
@@ -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;
 	}
crypto/asymmetric_keys/x509_parser.h +1 -0
index 4e1a384..a0b3052 100644
@@ -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;
crypto/asymmetric_keys/x509_public_key.c +2 -0
index 1d9a4c5..a0673c9 100644
@@ -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;
include/keys/asymmetric-type.h +1 -1
index 044ab0d..54fb119 100644
@@ -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,
Signature · 1 line
1.9.1
maintainer
#4 lore raw

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

crypto/asymmetric_keys/asymmetric_type.c struct asymmetric_key_id *asymmetric_key_generate_id(const void *val_1, · line 49

Quoted 3 lines · Dmitry Kasatkin
crypto/asymmetric_keys/asymmetric_type.c +2 -1
-	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

author
#5 lore raw

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

In reply to 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 <[email protected]>
---
 kernel/module_signing.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)
kernel/module_signing.c +5 -11
index be5b8fa..f329de8 100644
@@ -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);
Signature · 1 line
1.9.1
maintainer
#6 lore raw

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

Quoted 5 lines · 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.

This isn't sufficient. The key search must also include the signer.

David

author
#7 lore raw

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

Quoted 9 lines · David Howells
Quoted 5 lines · 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.

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

Quoted 6 lines

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#8 lore raw

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

Quoted 10 lines · Dmitry Kasatkin
Quoted 9 lines · David Howells
Quoted 5 lines · 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.

This isn't sufficient. The key search must also include the signer.

Quoted 10 lines

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

Quoted 11 lines
Quoted 6 lines

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#9 lore raw

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

Quoted 10 lines · Dmitry Kasatkin
Quoted 9 lines · Dmitry Kasatkin
Quoted 8 lines · David Howells
Quoted 5 lines · 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.

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

Quoted 16 lines
Quoted 10 lines

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

maintainer
#10 lore raw

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

Quoted 2 lines · Dmitry Kasatkin

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.

Quoted 3 lines

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

author
#11 lore raw

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

Quoted 6 lines · David Howells
Quoted 2 lines · Dmitry Kasatkin

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

Quoted 7 lines
Quoted 3 lines

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

maintainer
#12 lore raw

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

In reply to David Howells
Quoted 8 lines · David Howells
Quoted 2 lines · Dmitry Kasatkin

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.

Quoted 7 lines
Quoted 3 lines

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.

Signature · 2 lines
James Morris
<[email protected]>
author
#13 lore raw

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

Quoted 9 lines · James Morris
Quoted 7 lines · David Howells
Quoted 2 lines · Dmitry Kasatkin

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.

Quoted 11 lines
Quoted 6 lines
Quoted 3 lines

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

#14 lore raw

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

In reply to James Morris
Quoted 14 lines · James Morris
Quoted 12 lines · David Howells
Quoted 2 lines · Dmitry Kasatkin

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.

Quoted 3 lines

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.

Quoted 6 lines
Quoted 3 lines

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

author
#15 lore raw

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

In reply to Dmitry Kasatkin

Latest KEYS code return error if hexadecimal string length id odd. Fix it.

Signed-off-by Dmitry Kasatkin <[email protected]>
---
 security/integrity/digsig_asymmetric.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
security/integrity/digsig_asymmetric.c +1 -1
index 37e0d98..4fec181 100644
@@ -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);
Signature · 1 line
1.9.1
author
#16 lore raw
In reply to 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

Quoted 23 lines · 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(-)
#17 lore raw
In reply to Dmitry Kasatkin
Quoted 5 lines · 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.

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

maintainer
#18 lore raw

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

In reply to 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 <[email protected]>
Signed-off-by David Howells <[email protected]>
---
 x509_cert_parser.c |    2 ++
 x509_parser.h      |    2 ++
 x509_public_key.c  |    9 +++++++--
 3 files changed, 11 insertions(+), 2 deletions(-)
crypto/asymmetric_keys/x509_cert_parser.c +2 -0
index 96151b2b91a2..393706f33fa5 100644
@@ -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);
crypto/asymmetric_keys/x509_parser.h +2 -0
index 4e1a384901ed..3f0f0f081621 100644
@@ -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;
crypto/asymmetric_keys/x509_public_key.c +7 -2
index 1d9a4c555376..8bffb06b2683 100644
@@ -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++;