2014-06-12 20:17:41

by Dmitry Kasatkin

[permalink] [raw]
Subject: [PATCH v1a 0/2] KEYS: validate key trust with owner and builtin keys only

This is a repost of the patchset cleanly on the top of linux-integrity
next-trusted-keys branch.

Instead of allowing public keys, with certificates signed by any key on
the system trusted keyring, to be added to a trusted keyring, this patch
set further restricts the certificates to those signed by a particular key
or builtin keys on the system keyring.

This patch defines a new kernel parameter 'keys_ownerid={id:xxx | builtin}'
to use specific key or any builtin key.

Thanks,
Dmitry

Dmitry Kasatkin (2):
KEYS: validate certificate trust only with selected owner key
KEYS: validate certificate trust only with builtin keys

Documentation/kernel-parameters.txt | 5 +++++
crypto/asymmetric_keys/x509_public_key.c | 32 ++++++++++++++++++++++++++++++--
include/linux/key.h | 1 +
kernel/system_keyring.c | 1 +
4 files changed, 37 insertions(+), 2 deletions(-)

--
1.9.1


2014-06-12 20:17:43

by Dmitry Kasatkin

[permalink] [raw]
Subject: [PATCH v1a 1/2] KEYS: validate certificate trust only with selected owner key

Instead of allowing public keys, with certificates signed by any
key on the system trusted keyring, to be added to a trusted keyring,
this patch further restricts the certificates to those signed by a
particular key on the system keyring.

This patch defines a new kernel parameter 'keys_ownerid' to specify
owner's key id which must be used for trust validation of certificates.

Idea belongs to Mimi Zohar.

Signed-off-by: Dmitry Kasatkin <[email protected]>
---
Documentation/kernel-parameters.txt | 5 +++++
crypto/asymmetric_keys/x509_public_key.c | 23 +++++++++++++++++++++++
2 files changed, 28 insertions(+)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 7116fda..7a810d3 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1434,6 +1434,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
use the HighMem zone if it exists, and the Normal
zone if it does not.

+ keys_ownerid=[KEYS] This parameter identifies a specific key(s) on
+ the system trusted keyring to be used for certificate
+ trust validation.
+ format: id:<keyid>
+
kgdbdbgp= [KGDB,HW] kgdb over EHCI usb debug port.
Format: <Controller#>[,poll interval]
The controller # is the number of the ehci usb debug
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 7a9b386..d46b790 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -24,6 +24,19 @@
#include "public_key.h"
#include "x509_parser.h"

+static char *owner_keyid;
+static int __init default_owner_keyid_set(char *str)
+{
+ if (!str) /* default system keyring */
+ return 1;
+
+ if (strncmp(str, "id:", 3) == 0)
+ owner_keyid = str; /* owner local key 'id:xxxxxx' */
+
+ return 1;
+}
+__setup("keys_ownerid=", default_owner_keyid_set);
+
/*
* Find a key in the given keyring by issuer and authority.
*/
@@ -169,6 +182,16 @@ static int x509_validate_trust(struct x509_certificate *cert,
if (!trust_keyring)
return -EOPNOTSUPP;

+ if (owner_keyid) {
+ /* validate trust only with the owner_keyid if specified */
+ /* partial match of keyid according to the asymmetric_type.c */
+ int idlen = strlen(owner_keyid) - 3; /* - id: */
+ int authlen = strlen(cert->authority);
+ char *auth = cert->authority + authlen - idlen;
+ if (idlen > authlen || strcasecmp(owner_keyid + 3, auth))
+ return -EPERM;
+ }
+
key = x509_request_asymmetric_key(trust_keyring,
cert->issuer, strlen(cert->issuer),
cert->authority,
--
1.9.1

2014-06-12 20:17:58

by Dmitry Kasatkin

[permalink] [raw]
Subject: [PATCH v1a 2/2] KEYS: validate certificate trust only with builtin keys

Instead of allowing public keys, with certificates signed by any
key on the system trusted keyring, to be added to a trusted keyring,
this patch further restricts the certificates to those signed only by
builtin keys on the system keyring.

This patch defines a new option 'builtin' for the kernel parameter
'keys_ownerid' to allow trust validation using builtin keys.

Idea belongs to Mimi Zohar.

Signed-off-by: Dmitry Kasatkin <[email protected]>
---
Documentation/kernel-parameters.txt | 2 +-
crypto/asymmetric_keys/x509_public_key.c | 9 +++++++--
include/linux/key.h | 1 +
kernel/system_keyring.c | 1 +
4 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 7a810d3..336dabe 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1437,7 +1437,7 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
keys_ownerid=[KEYS] This parameter identifies a specific key(s) on
the system trusted keyring to be used for certificate
trust validation.
- format: id:<keyid>
+ format: { id:<keyid> | builtin }

kgdbdbgp= [KGDB,HW] kgdb over EHCI usb debug port.
Format: <Controller#>[,poll interval]
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index d46b790..c3805a8 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -24,6 +24,7 @@
#include "public_key.h"
#include "x509_parser.h"

+static bool builtin_keys;
static char *owner_keyid;
static int __init default_owner_keyid_set(char *str)
{
@@ -32,6 +33,8 @@ static int __init default_owner_keyid_set(char *str)

if (strncmp(str, "id:", 3) == 0)
owner_keyid = str; /* owner local key 'id:xxxxxx' */
+ else if (strcmp(str, "builtin") == 0)
+ builtin_keys = true;

return 1;
}
@@ -197,8 +200,10 @@ static int x509_validate_trust(struct x509_certificate *cert,
cert->authority,
strlen(cert->authority));
if (!IS_ERR(key)) {
- pk = key->payload.data;
- ret = x509_check_signature(pk, cert);
+ if (!builtin_keys || test_bit(KEY_FLAG_BUILTIN, &key->flags)) {
+ pk = key->payload.data;
+ ret = x509_check_signature(pk, cert);
+ }
key_put(key);
}
return ret;
diff --git a/include/linux/key.h b/include/linux/key.h
index cd0abb8..67c8e7e 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -170,6 +170,7 @@ struct key {
#define KEY_FLAG_INVALIDATED 7 /* set if key has been invalidated */
#define KEY_FLAG_TRUSTED 8 /* set if key is trusted */
#define KEY_FLAG_TRUSTED_ONLY 9 /* set if keyring only accepts links to trusted keys */
+#define KEY_FLAG_BUILTIN 10 /* set if key is builtin */

/* the key type and key description string
* - the desc is used to match a key against search criteria
diff --git a/kernel/system_keyring.c b/kernel/system_keyring.c
index 52ebc70..875f64e 100644
--- a/kernel/system_keyring.c
+++ b/kernel/system_keyring.c
@@ -89,6 +89,7 @@ static __init int load_system_certificate_list(void)
pr_err("Problem loading in-kernel X.509 certificate (%ld)\n",
PTR_ERR(key));
} else {
+ set_bit(KEY_FLAG_BUILTIN, &key_ref_to_ptr(key)->flags);
pr_notice("Loaded X.509 cert '%s'\n",
key_ref_to_ptr(key)->description);
key_ref_put(key);
--
1.9.1

2014-06-16 11:43:29

by Mimi Zohar

[permalink] [raw]
Subject: Re: [PATCH v1a 1/2] KEYS: validate certificate trust only with selected owner key


On Thu, 2014-06-12 at 23:17 +0300, Dmitry Kasatkin wrote:
> Instead of allowing public keys, with certificates signed by any
> key on the system trusted keyring, to be added to a trusted keyring,
> this patch further restricts the certificates to those signed by a
> particular key on the system keyring.
>
> This patch defines a new kernel parameter 'keys_ownerid' to specify
> owner's key id which must be used for trust validation of certificates.
>
> Idea belongs to Mimi Zohar.
>
> Signed-off-by: Dmitry Kasatkin <[email protected]>
> ---
> Documentation/kernel-parameters.txt | 5 +++++
> crypto/asymmetric_keys/x509_public_key.c | 23 +++++++++++++++++++++++
> 2 files changed, 28 insertions(+)
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 7116fda..7a810d3 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1434,6 +1434,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> use the HighMem zone if it exists, and the Normal
> zone if it does not.
>
> + keys_ownerid=[KEYS] This parameter identifies a specific key(s) on
> + the system trusted keyring to be used for certificate
> + trust validation.
> + format: id:<keyid>
> +
> kgdbdbgp= [KGDB,HW] kgdb over EHCI usb debug port.
> Format: <Controller#>[,poll interval]
> The controller # is the number of the ehci usb debug
> diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
> index 7a9b386..d46b790 100644
> --- a/crypto/asymmetric_keys/x509_public_key.c
> +++ b/crypto/asymmetric_keys/x509_public_key.c
> @@ -24,6 +24,19 @@
> #include "public_key.h"
> #include "x509_parser.h"
>
> +static char *owner_keyid;
> +static int __init default_owner_keyid_set(char *str)
> +{
> + if (!str) /* default system keyring */
> + return 1;
> +
> + if (strncmp(str, "id:", 3) == 0)
> + owner_keyid = str; /* owner local key 'id:xxxxxx' */
> +
> + return 1;
> +}
> +__setup("keys_ownerid=", default_owner_keyid_set);
> +
> /*
> * Find a key in the given keyring by issuer and authority.
> */
> @@ -169,6 +182,16 @@ static int x509_validate_trust(struct x509_certificate *cert,
> if (!trust_keyring)
> return -EOPNOTSUPP;
>
> + if (owner_keyid) {
> + /* validate trust only with the owner_keyid if specified */
> + /* partial match of keyid according to the asymmetric_type.c */
> + int idlen = strlen(owner_keyid) - 3; /* - id: */
> + int authlen = strlen(cert->authority);
> + char *auth = cert->authority + authlen - idlen;
> + if (idlen > authlen || strcasecmp(owner_keyid + 3, auth))
> + return -EPERM;
> + }
> +

We shouldn't hard code the test here, but use the key type's match
function. For example, the "KEYS: define an owner trusted keyring" (v4)
patch defined a key_match() function.

thanks,

Mimi

> key = x509_request_asymmetric_key(trust_keyring,
> cert->issuer, strlen(cert->issuer),
> cert->authority,



2014-06-16 11:43:42

by Mimi Zohar

[permalink] [raw]
Subject: Re: [PATCH v1a 2/2] KEYS: validate certificate trust only with builtin keys


On Thu, 2014-06-12 at 23:17 +0300, Dmitry Kasatkin wrote:
> Instead of allowing public keys, with certificates signed by any
> key on the system trusted keyring, to be added to a trusted keyring,
> this patch further restricts the certificates to those signed only by
> builtin keys on the system keyring.
>
> This patch defines a new option 'builtin' for the kernel parameter
> 'keys_ownerid' to allow trust validation using builtin keys.

Thanks, this patch works without a separate 'owned' trusted keyring, but
we need to wait until the UEFI key patches are upstreamed.

thanks,

Mimi

> Idea belongs to Mimi Zohar.
>
> Signed-off-by: Dmitry Kasatkin <[email protected]>
> ---
> Documentation/kernel-parameters.txt | 2 +-
> crypto/asymmetric_keys/x509_public_key.c | 9 +++++++--
> include/linux/key.h | 1 +
> kernel/system_keyring.c | 1 +
> 4 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 7a810d3..336dabe 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1437,7 +1437,7 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> keys_ownerid=[KEYS] This parameter identifies a specific key(s) on
> the system trusted keyring to be used for certificate
> trust validation.
> - format: id:<keyid>
> + format: { id:<keyid> | builtin }
>
> kgdbdbgp= [KGDB,HW] kgdb over EHCI usb debug port.
> Format: <Controller#>[,poll interval]
> diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
> index d46b790..c3805a8 100644
> --- a/crypto/asymmetric_keys/x509_public_key.c
> +++ b/crypto/asymmetric_keys/x509_public_key.c
> @@ -24,6 +24,7 @@
> #include "public_key.h"
> #include "x509_parser.h"
>
> +static bool builtin_keys;
> static char *owner_keyid;
> static int __init default_owner_keyid_set(char *str)
> {
> @@ -32,6 +33,8 @@ static int __init default_owner_keyid_set(char *str)
>
> if (strncmp(str, "id:", 3) == 0)
> owner_keyid = str; /* owner local key 'id:xxxxxx' */
> + else if (strcmp(str, "builtin") == 0)
> + builtin_keys = true;
>
> return 1;
> }
> @@ -197,8 +200,10 @@ static int x509_validate_trust(struct x509_certificate *cert,
> cert->authority,
> strlen(cert->authority));
> if (!IS_ERR(key)) {
> - pk = key->payload.data;
> - ret = x509_check_signature(pk, cert);
> + if (!builtin_keys || test_bit(KEY_FLAG_BUILTIN, &key->flags)) {
> + pk = key->payload.data;
> + ret = x509_check_signature(pk, cert);
> + }
> key_put(key);
> }
> return ret;
> diff --git a/include/linux/key.h b/include/linux/key.h
> index cd0abb8..67c8e7e 100644
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -170,6 +170,7 @@ struct key {
> #define KEY_FLAG_INVALIDATED 7 /* set if key has been invalidated */
> #define KEY_FLAG_TRUSTED 8 /* set if key is trusted */
> #define KEY_FLAG_TRUSTED_ONLY 9 /* set if keyring only accepts links to trusted keys */
> +#define KEY_FLAG_BUILTIN 10 /* set if key is builtin */
>
> /* the key type and key description string
> * - the desc is used to match a key against search criteria
> diff --git a/kernel/system_keyring.c b/kernel/system_keyring.c
> index 52ebc70..875f64e 100644
> --- a/kernel/system_keyring.c
> +++ b/kernel/system_keyring.c
> @@ -89,6 +89,7 @@ static __init int load_system_certificate_list(void)
> pr_err("Problem loading in-kernel X.509 certificate (%ld)\n",
> PTR_ERR(key));
> } else {
> + set_bit(KEY_FLAG_BUILTIN, &key_ref_to_ptr(key)->flags);
> pr_notice("Loaded X.509 cert '%s'\n",
> key_ref_to_ptr(key)->description);
> key_ref_put(key);





2014-06-17 08:58:45

by Dmitry Kasatkin

[permalink] [raw]
Subject: Re: [PATCH v1a 1/2] KEYS: validate certificate trust only with selected owner key

On 16/06/14 14:43, Mimi Zohar wrote:
> On Thu, 2014-06-12 at 23:17 +0300, Dmitry Kasatkin wrote:
>> Instead of allowing public keys, with certificates signed by any
>> key on the system trusted keyring, to be added to a trusted keyring,
>> this patch further restricts the certificates to those signed by a
>> particular key on the system keyring.
>>
>> This patch defines a new kernel parameter 'keys_ownerid' to specify
>> owner's key id which must be used for trust validation of certificates.
>>
>> Idea belongs to Mimi Zohar.
>>
>> Signed-off-by: Dmitry Kasatkin <[email protected]>
>> ---
>> Documentation/kernel-parameters.txt | 5 +++++
>> crypto/asymmetric_keys/x509_public_key.c | 23 +++++++++++++++++++++++
>> 2 files changed, 28 insertions(+)
>>
>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>> index 7116fda..7a810d3 100644
>> --- a/Documentation/kernel-parameters.txt
>> +++ b/Documentation/kernel-parameters.txt
>> @@ -1434,6 +1434,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>> use the HighMem zone if it exists, and the Normal
>> zone if it does not.
>>
>> + keys_ownerid=[KEYS] This parameter identifies a specific key(s) on
>> + the system trusted keyring to be used for certificate
>> + trust validation.
>> + format: id:<keyid>
>> +
>> kgdbdbgp= [KGDB,HW] kgdb over EHCI usb debug port.
>> Format: <Controller#>[,poll interval]
>> The controller # is the number of the ehci usb debug
>> diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
>> index 7a9b386..d46b790 100644
>> --- a/crypto/asymmetric_keys/x509_public_key.c
>> +++ b/crypto/asymmetric_keys/x509_public_key.c
>> @@ -24,6 +24,19 @@
>> #include "public_key.h"
>> #include "x509_parser.h"
>>
>> +static char *owner_keyid;
>> +static int __init default_owner_keyid_set(char *str)
>> +{
>> + if (!str) /* default system keyring */
>> + return 1;
>> +
>> + if (strncmp(str, "id:", 3) == 0)
>> + owner_keyid = str; /* owner local key 'id:xxxxxx' */
>> +
>> + return 1;
>> +}
>> +__setup("keys_ownerid=", default_owner_keyid_set);
>> +
>> /*
>> * Find a key in the given keyring by issuer and authority.
>> */
>> @@ -169,6 +182,16 @@ static int x509_validate_trust(struct x509_certificate *cert,
>> if (!trust_keyring)
>> return -EOPNOTSUPP;
>>
>> + if (owner_keyid) {
>> + /* validate trust only with the owner_keyid if specified */
>> + /* partial match of keyid according to the asymmetric_type.c */
>> + int idlen = strlen(owner_keyid) - 3; /* - id: */
>> + int authlen = strlen(cert->authority);
>> + char *auth = cert->authority + authlen - idlen;
>> + if (idlen > authlen || strcasecmp(owner_keyid + 3, auth))
>> + return -EPERM;
>> + }
>> +
> We shouldn't hard code the test here, but use the key type's match
> function. For example, the "KEYS: define an owner trusted keyring" (v4)
> patch defined a key_match() function.
>
> thanks,

Right.

I addressed this in the following patchset.

Thanks.


>
> Mimi
>
>> key = x509_request_asymmetric_key(trust_keyring,
>> cert->issuer, strlen(cert->issuer),
>> cert->authority,
>
>
>
> --
> 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
>