2014-10-03 09:10:07

by Dmitry Kasatkin

[permalink] [raw]
Subject: [PATCH 0/4] 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(-)

--
1.9.1


2014-10-03 09:10:13

by Dmitry Kasatkin

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

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

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

2014-10-03 09:10:11

by Dmitry Kasatkin

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

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

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

2014-10-03 09:11:13

by Dmitry Kasatkin

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

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

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

2014-10-03 09:11:33

by Dmitry Kasatkin

[permalink] [raw]
Subject: [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(-)

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

2014-10-03 10:43:04

by Dmitry Kasatkin

[permalink] [raw]
Subject: Re: [PATCH 0/4] KEYS fixes

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

2014-10-03 12:46:47

by David Howells

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

Dmitry Kasatkin <[email protected]> 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

2014-10-03 12:47:09

by David Howells

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

Dmitry Kasatkin <[email protected]> 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 <[email protected]>
> Signed-off-by: Dmitry Kasatkin <[email protected]>

Applied.

2014-10-03 12:49:48

by Dmitry Kasatkin

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

On 03/10/14 15:46, David Howells wrote:
> Dmitry Kasatkin <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2014-10-03 12:54:01

by Dmitry Kasatkin

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

On 03/10/14 15:49, Dmitry Kasatkin wrote:
> On 03/10/14 15:46, David Howells wrote:
>> Dmitry Kasatkin <[email protected]> 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 [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
>

2014-10-03 13:09:01

by Dmitry Kasatkin

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

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 <[email protected]> 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
>
>

2014-10-03 13:40:33

by David Howells

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

Dmitry Kasatkin <[email protected]> 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

2014-10-03 14:00:12

by Dmitry Kasatkin

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

On 03/10/14 16:40, David Howells wrote:
> Dmitry Kasatkin <[email protected]> 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

2014-10-03 14:19:38

by Mimi Zohar

[permalink] [raw]
Subject: Re: [PATCH 0/4] KEYS fixes

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

2014-10-06 12:45:23

by James Morris

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

On Fri, 3 Oct 2014, David Howells wrote:

> Dmitry Kasatkin <[email protected]> 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
<[email protected]>

2014-10-06 13:52:13

by David Howells

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

Dmitry Kasatkin <[email protected]> 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

2014-10-06 17:14:34

by Dmitry Kasatkin

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

On 06/10/14 15:44, James Morris wrote:
> On Fri, 3 Oct 2014, David Howells wrote:
>
>> Dmitry Kasatkin <[email protected]> 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

2014-10-06 19:39:57

by Mimi Zohar

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

On Mon, 2014-10-06 at 23:44 +1100, James Morris wrote:
> On Fri, 3 Oct 2014, David Howells wrote:
>
> > Dmitry Kasatkin <[email protected]> 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