A key added to the ima keyring must be signed by a key contained within
either the builtin trusted or secondary trusted keyrings. Currently, there are
CA restrictions described in IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY,
but these restrictions are not enforced within code. Therefore, keys within
either the builtin or secondary may not be a CA and could be used to
vouch for an ima key.
The machine keyring can not be used as another trust anchor for adding keys
to the ima keyring, since CA enforcement does not currently exist [1]. This
would expand the current integrity gap.
Introduce a new root of trust key flag to close this integrity gap for
all keyrings. The first key type to use this is X.509. When a X.509
certificate is self signed, contains kernCertSign Key Usage and contains
the CA bit, the new flag is set. Introduce new keyring restrictions
that not only validates a key is signed by a key contained within the
keyring, but also validates the key has the new root of trust key flag
set. Use this new restriction for keys added to the ima keyring. Now
that we have CA enforcement, allow the machine keyring to be used as another
trust anchor for the ima keyring.
To recap, all keys that previously loaded into the builtin, secondary or
machine keyring will still load after applying this series. Keys
contained within these keyrings may carry the root of trust flag. The
ima keyring will use the new root of trust restriction to validate
CA enforcement. Other keyrings that require a root of trust could also
use this in the future.
[1] https://lore.kernel.org/lkml/[email protected]/
Eric Snowberg (7):
KEYS: Create static version of public_key_verify_signature
KEYS: X.509: Parse Basic Constraints for CA
KEYS: X.509: Parse Key Usage
KEYS: Introduce a builtin root of trust key flag
KEYS: Introduce sig restriction that validates root of trust
KEYS: X.509: Flag Intermediate CA certs as built in
integrity: Use root of trust signature restriction
certs/system_keyring.c | 18 ++++++++++
crypto/asymmetric_keys/restrict.c | 42 +++++++++++++++++++++++
crypto/asymmetric_keys/x509_cert_parser.c | 29 ++++++++++++++++
crypto/asymmetric_keys/x509_parser.h | 2 ++
crypto/asymmetric_keys/x509_public_key.c | 12 +++++++
include/crypto/public_key.h | 9 +++++
include/keys/system_keyring.h | 17 ++++++++-
include/linux/ima.h | 16 +++++++++
include/linux/key-type.h | 3 ++
include/linux/key.h | 2 ++
security/integrity/Kconfig | 1 -
security/integrity/digsig.c | 4 +--
security/keys/key.c | 13 +++++++
13 files changed, 164 insertions(+), 4 deletions(-)
base-commit: 3123109284176b1532874591f7c81f3837bbdc17
--
2.27.0
Parse the X.509 Basic Constraints. The basic constraints extension
identifies whether the subject of the certificate is a CA.
BasicConstraints ::= SEQUENCE {
cA BOOLEAN DEFAULT FALSE,
pathLenConstraint INTEGER (0..MAX) OPTIONAL }
If the CA is true, store it in the x509_certificate. This will be used
in a follow on patch that requires knowing if the public key is a CA.
Signed-off-by: Eric Snowberg <[email protected]>
---
crypto/asymmetric_keys/x509_cert_parser.c | 9 +++++++++
crypto/asymmetric_keys/x509_parser.h | 1 +
2 files changed, 10 insertions(+)
diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
index 2899ed80bb18..30f7374ea9c0 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -583,6 +583,15 @@ int x509_process_extension(void *context, size_t hdrlen,
return 0;
}
+ if (ctx->last_oid == OID_basicConstraints) {
+ if (vlen < 2 || v[0] != (ASN1_CONS_BIT | ASN1_SEQ))
+ return -EBADMSG;
+ if (v[1] != vlen - 2)
+ return -EBADMSG;
+ if (vlen >= 4 && v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
+ ctx->cert->is_root_ca = true;
+ }
+
return 0;
}
diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
index 97a886cbe01c..dc45df9f6594 100644
--- a/crypto/asymmetric_keys/x509_parser.h
+++ b/crypto/asymmetric_keys/x509_parser.h
@@ -38,6 +38,7 @@ struct x509_certificate {
bool self_signed; /* T if self-signed (check unsupported_sig too) */
bool unsupported_sig; /* T if signature uses unsupported crypto */
bool blacklisted;
+ bool is_root_ca; /* T if basic constraints CA is set */
};
/*
--
2.27.0
Parse the X.509 Key Usage. The key usage extension defines the purpose of
the key contained in the certificate.
id-ce-keyUsage OBJECT IDENTIFIER ::= { id-ce 15 }
KeyUsage ::= BIT STRING {
digitalSignature (0),
contentCommitment (1),
keyEncipherment (2),
dataEncipherment (3),
keyAgreement (4),
keyCertSign (5),
cRLSign (6),
encipherOnly (7),
decipherOnly (8) }
If the keyCertSign is set, store it in the x509_certificate structure.
This will be used in a follow on patch that requires knowing the
certificate key usage type.
Signed-off-by: Eric Snowberg <[email protected]>
---
crypto/asymmetric_keys/x509_cert_parser.c | 20 ++++++++++++++++++++
crypto/asymmetric_keys/x509_parser.h | 1 +
2 files changed, 21 insertions(+)
diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
index 30f7374ea9c0..a89f1e0c8a0f 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -576,6 +576,26 @@ int x509_process_extension(void *context, size_t hdrlen,
return 0;
}
+ if (ctx->last_oid == OID_keyUsage) {
+ /*
+ * Get hold of the keyUsage bit string to validate keyCertSign
+ * v[1] is the encoding size
+ * (Expect either 0x02 or 0x03, making it 1 or 2 bytes)
+ * v[2] is the number of unused bits in the bit string
+ * (If >= 3 keyCertSign is missing)
+ * v[3] and possibly v[4] contain the bit string
+ * 0x04 is where KeyCertSign lands in this bit string (from
+ * RFC 5280 4.2.1.3)
+ */
+ if (v[0] != ASN1_BTS || vlen < 4)
+ return -EBADMSG;
+ if (v[1] == 0x02 && v[2] <= 2 && (v[3] & 0x04))
+ ctx->cert->is_kcs_set = true;
+ else if (vlen > 4 && v[1] == 0x03 && (v[3] & 0x04))
+ ctx->cert->is_kcs_set = true;
+ return 0;
+ }
+
if (ctx->last_oid == OID_authorityKeyIdentifier) {
/* Get hold of the CA key fingerprint */
ctx->raw_akid = v;
diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
index dc45df9f6594..d6ac0985d8a5 100644
--- a/crypto/asymmetric_keys/x509_parser.h
+++ b/crypto/asymmetric_keys/x509_parser.h
@@ -39,6 +39,7 @@ struct x509_certificate {
bool unsupported_sig; /* T if signature uses unsupported crypto */
bool blacklisted;
bool is_root_ca; /* T if basic constraints CA is set */
+ bool is_kcs_set; /* T if keyCertSign is set */
};
/*
--
2.27.0
The current keyring restrictions validate if a key can be vouched for by
another key already contained in a keyring. Add a new restriction called
restrict_link_by_rot_and_signature that both vouches for the new key and
validates the vouching key contains the builtin root of trust flag.
Two new system keyring restrictions are added to use
restrict_link_by_rot_and_signature. The first restriction called
restrict_link_by_rot_builtin_trusted uses the builtin_trusted_keys as
the restricted keyring. The second system keyring restriction called
restrict_link_by_rot_builtin_and_secondary_trusted uses the
secondary_trusted_keys as the restricted keyring. Should the machine
keyring be defined, it shall be validated too, since it is linked to
the secondary_trusted_keys keyring.
Signed-off-by: Eric Snowberg <[email protected]>
---
certs/system_keyring.c | 18 +++++++++++++
crypto/asymmetric_keys/restrict.c | 42 +++++++++++++++++++++++++++++++
include/keys/system_keyring.h | 17 ++++++++++++-
3 files changed, 76 insertions(+), 1 deletion(-)
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 05b66ce9d1c9..a8b53446ec25 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -48,6 +48,14 @@ int restrict_link_by_builtin_trusted(struct key *dest_keyring,
builtin_trusted_keys);
}
+int restrict_link_by_rot_builtin_trusted(struct key *dest_keyring,
+ const struct key_type *type,
+ const union key_payload *payload,
+ struct key *unused)
+{
+ return restrict_link_by_rot_and_signature(dest_keyring, type, payload,
+ builtin_trusted_keys);
+}
#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
/**
* restrict_link_by_builtin_and_secondary_trusted - Restrict keyring
@@ -76,6 +84,16 @@ int restrict_link_by_builtin_and_secondary_trusted(
secondary_trusted_keys);
}
+int restrict_link_by_rot_builtin_and_secondary_trusted(
+ struct key *dest_keyring,
+ const struct key_type *type,
+ const union key_payload *payload,
+ struct key *unused)
+{
+ return restrict_link_by_rot_and_signature(dest_keyring, type, payload,
+ secondary_trusted_keys);
+}
+
/**
* Allocate a struct key_restriction for the "builtin and secondary trust"
* keyring. Only for use in system_trusted_keyring_init().
diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index 6b1ac5f5896a..840ea302b40a 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -108,6 +108,48 @@ int restrict_link_by_signature(struct key *dest_keyring,
return ret;
}
+int restrict_link_by_rot_and_signature(struct key *dest_keyring,
+ const struct key_type *type,
+ const union key_payload *payload,
+ struct key *trust_keyring)
+{
+ const struct public_key_signature *sig;
+ struct key *key;
+ int ret;
+
+ if (!trust_keyring)
+ return -ENOKEY;
+
+ if (type != &key_type_asymmetric)
+ return -EOPNOTSUPP;
+
+ sig = payload->data[asym_auth];
+ if (!sig)
+ return -ENOPKG;
+ if (!sig->auth_ids[0] && !sig->auth_ids[1] && !sig->auth_ids[2])
+ return -ENOKEY;
+
+ if (ca_keyid && !asymmetric_key_id_partial(sig->auth_ids[1], ca_keyid))
+ return -EPERM;
+
+ /* See if we have a key that signed this one. */
+ key = find_asymmetric_key(trust_keyring,
+ sig->auth_ids[0], sig->auth_ids[1],
+ sig->auth_ids[2], false);
+ if (IS_ERR(key))
+ return -ENOKEY;
+
+ if (!test_bit(KEY_FLAG_BUILTIN_ROT, &key->flags))
+ ret = -ENOKEY;
+ else if (use_builtin_keys && !test_bit(KEY_FLAG_BUILTIN, &key->flags))
+ ret = -ENOKEY;
+ else
+ ret = verify_signature(key, sig);
+ key_put(key);
+ return ret;
+}
+
+
static bool match_either_id(const struct asymmetric_key_id **pair,
const struct asymmetric_key_id *single)
{
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 2419a735420f..2c1241042f1f 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -17,9 +17,18 @@ extern int restrict_link_by_builtin_trusted(struct key *keyring,
const union key_payload *payload,
struct key *restriction_key);
extern __init int load_module_cert(struct key *keyring);
-
+extern int restrict_link_by_rot_builtin_trusted(struct key *keyring,
+ const struct key_type *type,
+ const union key_payload *payload,
+ struct key *unused);
+extern int restrict_link_by_rot_and_signature(struct key *dest_keyring,
+ const struct key_type *type,
+ const union key_payload *payload,
+ struct key *unused);
#else
#define restrict_link_by_builtin_trusted restrict_link_reject
+#define restrict_link_by_rot_and_signature restrict_link_reject
+#define restrict_link_by_rot_builtin_trusted restrict_link_reject
static inline __init int load_module_cert(struct key *keyring)
{
@@ -34,8 +43,14 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
const struct key_type *type,
const union key_payload *payload,
struct key *restriction_key);
+extern int restrict_link_by_rot_builtin_and_secondary_trusted(
+ struct key *dest_keyring,
+ const struct key_type *type,
+ const union key_payload *payload,
+ struct key *restrict_key);
#else
#define restrict_link_by_builtin_and_secondary_trusted restrict_link_by_builtin_trusted
+#define restrict_link_by_rot_builtin_and_secondary_trusted restrict_link_by_builtin_trusted
#endif
#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
--
2.27.0
Hi Eric,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on 3123109284176b1532874591f7c81f3837bbdc17]
url: https://github.com/intel-lab-lkp/linux/commits/Eric-Snowberg/Add-CA-enforcement-keyring-restrictions/20220407-003209
base: 3123109284176b1532874591f7c81f3837bbdc17
config: riscv-randconfig-r042-20220406 (https://download.01.org/0day-ci/archive/20220407/[email protected]/config)
compiler: riscv64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/68d98a175d29032d888f3f5700c43cf771ef17d8
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Eric-Snowberg/Add-CA-enforcement-keyring-restrictions/20220407-003209
git checkout 68d98a175d29032d888f3f5700c43cf771ef17d8
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=riscv SHELL=/bin/bash crypto/asymmetric_keys/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> crypto/asymmetric_keys/restrict.c:111:5: warning: no previous prototype for 'restrict_link_by_rot_and_signature' [-Wmissing-prototypes]
111 | int restrict_link_by_rot_and_signature(struct key *dest_keyring,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/restrict_link_by_rot_and_signature +111 crypto/asymmetric_keys/restrict.c
110
> 111 int restrict_link_by_rot_and_signature(struct key *dest_keyring,
112 const struct key_type *type,
113 const union key_payload *payload,
114 struct key *trust_keyring)
115 {
116 const struct public_key_signature *sig;
117 struct key *key;
118 int ret;
119
120 if (!trust_keyring)
121 return -ENOKEY;
122
123 if (type != &key_type_asymmetric)
124 return -EOPNOTSUPP;
125
126 sig = payload->data[asym_auth];
127 if (!sig)
128 return -ENOPKG;
129 if (!sig->auth_ids[0] && !sig->auth_ids[1] && !sig->auth_ids[2])
130 return -ENOKEY;
131
132 if (ca_keyid && !asymmetric_key_id_partial(sig->auth_ids[1], ca_keyid))
133 return -EPERM;
134
135 /* See if we have a key that signed this one. */
136 key = find_asymmetric_key(trust_keyring,
137 sig->auth_ids[0], sig->auth_ids[1],
138 sig->auth_ids[2], false);
139 if (IS_ERR(key))
140 return -ENOKEY;
141
142 if (!test_bit(KEY_FLAG_BUILTIN_ROT, &key->flags))
143 ret = -ENOKEY;
144 else if (use_builtin_keys && !test_bit(KEY_FLAG_BUILTIN, &key->flags))
145 ret = -ENOKEY;
146 else
147 ret = verify_signature(key, sig);
148 key_put(key);
149 return ret;
150 }
151
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Eric,
On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
> A key added to the ima keyring must be signed by a key contained within
> either the builtin trusted or secondary trusted keyrings. Currently, there are
> CA restrictions described in IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY,
> but these restrictions are not enforced within code. Therefore, keys within
> either the builtin or secondary may not be a CA and could be used to
> vouch for an ima key.
>
> The machine keyring can not be used as another trust anchor for adding keys
> to the ima keyring, since CA enforcement does not currently exist [1]. This
> would expand the current integrity gap.
>
> Introduce a new root of trust key flag to close this integrity gap for
> all keyrings. The first key type to use this is X.509. When a X.509
> certificate is self signed, contains kernCertSign Key Usage and contains
> the CA bit, the new flag is set. Introduce new keyring restrictions
> that not only validates a key is signed by a key contained within the
> keyring, but also validates the key has the new root of trust key flag
> set. Use this new restriction for keys added to the ima keyring. Now
> that we have CA enforcement, allow the machine keyring to be used as another
> trust anchor for the ima keyring.
>
> To recap, all keys that previously loaded into the builtin, secondary or
> machine keyring will still load after applying this series. Keys
> contained within these keyrings may carry the root of trust flag. The
> ima keyring will use the new root of trust restriction to validate
> CA enforcement. Other keyrings that require a root of trust could also
> use this in the future.
Your initial patch set indicated that you were addressing Linus'
request to allow end-users the ability "to add their own keys and sign
modules they trust". However, from the design of the previous patch
set and now this one, everything indicates a lot more is going on than
just allowing end-users to add their own keys. There would be no
reason for loading all the MOK keys, rather than just the CA keys, onto
the "machine" keyring. Please provide the motivation for this design.
Please note that Patch 6/7 permits intermediary CA keys, without any
mention of it in the cover letter. Please include this in the
motivation for this design.
thanks,
Mimi
> On Apr 6, 2022, at 2:45 PM, Mimi Zohar <[email protected]> wrote:
>
> Hi Eric,
>
> On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
>> A key added to the ima keyring must be signed by a key contained within
>> either the builtin trusted or secondary trusted keyrings. Currently, there are
>> CA restrictions described in IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY,
>> but these restrictions are not enforced within code. Therefore, keys within
>> either the builtin or secondary may not be a CA and could be used to
>> vouch for an ima key.
>>
>> The machine keyring can not be used as another trust anchor for adding keys
>> to the ima keyring, since CA enforcement does not currently exist [1]. This
>> would expand the current integrity gap.
>>
>> Introduce a new root of trust key flag to close this integrity gap for
>> all keyrings. The first key type to use this is X.509. When a X.509
>> certificate is self signed, contains kernCertSign Key Usage and contains
>> the CA bit, the new flag is set. Introduce new keyring restrictions
>> that not only validates a key is signed by a key contained within the
>> keyring, but also validates the key has the new root of trust key flag
>> set. Use this new restriction for keys added to the ima keyring. Now
>> that we have CA enforcement, allow the machine keyring to be used as another
>> trust anchor for the ima keyring.
>>
>> To recap, all keys that previously loaded into the builtin, secondary or
>> machine keyring will still load after applying this series. Keys
>> contained within these keyrings may carry the root of trust flag. The
>> ima keyring will use the new root of trust restriction to validate
>> CA enforcement. Other keyrings that require a root of trust could also
>> use this in the future.
>
> Your initial patch set indicated that you were addressing Linus'
> request to allow end-users the ability "to add their own keys and sign
> modules they trust". However, from the design of the previous patch
> set and now this one, everything indicates a lot more is going on than
> just allowing end-users to add their own keys. There would be no
> reason for loading all the MOK keys, rather than just the CA keys, onto
> the "machine" keyring. Please provide the motivation for this design.
The motivation is to satisfy both Linus and your requests. Linus requested
the ability to allow users to add their own keys and sign modules they trust.
A code signing CA certificate does not require kernCertSign in the usage. Adding
this as a requirement for kernel modules would be a regression (or a bug).
This series addresses your request to only trust validly signed CA certs.
As you pointed out in the Kconfig help for
IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY:
help
Keys may be added to the IMA or IMA blacklist keyrings, if the
key is validly signed by a CA cert in the system built-in or
secondary trusted keyrings.
Intermediate keys between those the kernel has compiled in and the
IMA keys to be added may be added to the system secondary keyring,
provided they are validly signed by a key already resident in the
built-in or secondary trusted keyrings.
requires keys to be “validly” signed by a CA cert. Later the definition of a
validly signed CA cert was defined as: self signed, contains kernCertSign
key usage and contains the CA bit. While this help file states the CA restriction,
nothing in code enforces it. One can place any type of self signed cert in either
keyring and ima will use it. The motivation is for all keys added to the ima
keyring to abide by the restriction defined in the Kconfig help. With this series
this can be accomplished without introducing a regression on keys placed in
any of the system keyrings.
> Please note that Patch 6/7 permits intermediary CA keys, without any
> mention of it in the cover letter. Please include this in the
> motivation for this design.
Ok, I’ll add that in the next round.
On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
> Parse the X.509 Basic Constraints. The basic constraints extension
> identifies whether the subject of the certificate is a CA.
>
> BasicConstraints ::= SEQUENCE {
> cA BOOLEAN DEFAULT FALSE,
> pathLenConstraint INTEGER (0..MAX) OPTIONAL }
>
> If the CA is true, store it in the x509_certificate. This will be used
> in a follow on patch that requires knowing if the public key is a CA.
>
> Signed-off-by: Eric Snowberg <[email protected]>
> ---
> crypto/asymmetric_keys/x509_cert_parser.c | 9 +++++++++
> crypto/asymmetric_keys/x509_parser.h | 1 +
> 2 files changed, 10 insertions(+)
>
> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
> index 2899ed80bb18..30f7374ea9c0 100644
> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -583,6 +583,15 @@ int x509_process_extension(void *context, size_t hdrlen,
> return 0;
> }
>
> + if (ctx->last_oid == OID_basicConstraints) {
> + if (vlen < 2 || v[0] != (ASN1_CONS_BIT | ASN1_SEQ))
> + return -EBADMSG;
> + if (v[1] != vlen - 2)
> + return -EBADMSG;
> + if (vlen >= 4 && v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
> + ctx->cert->is_root_ca = true;
> + }
> +
> return 0;
> }
>
> diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
> index 97a886cbe01c..dc45df9f6594 100644
> --- a/crypto/asymmetric_keys/x509_parser.h
> +++ b/crypto/asymmetric_keys/x509_parser.h
> @@ -38,6 +38,7 @@ struct x509_certificate {
> bool self_signed; /* T if self-signed (check unsupported_sig too) */
> bool unsupported_sig; /* T if signature uses unsupported crypto */
> bool blacklisted;
> + bool is_root_ca; /* T if basic constraints CA is set */
There's no need to prefix variables with "is_". Similar to the
variable "self_signed" simply name this variable "root_ca".
thanks,
Mimi
> };
>
> /*
On Wed, 2022-04-06 at 22:53 +0000, Eric Snowberg wrote:
>
> > On Apr 6, 2022, at 2:45 PM, Mimi Zohar <[email protected]> wrote:
> >
> > Hi Eric,
> >
> > On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
> >> A key added to the ima keyring must be signed by a key contained within
> >> either the builtin trusted or secondary trusted keyrings. Currently, there are
> >> CA restrictions described in IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY,
> >> but these restrictions are not enforced within code. Therefore, keys within
> >> either the builtin or secondary may not be a CA and could be used to
> >> vouch for an ima key.
> >>
> >> The machine keyring can not be used as another trust anchor for adding keys
> >> to the ima keyring, since CA enforcement does not currently exist [1]. This
> >> would expand the current integrity gap.
> >>
> >> Introduce a new root of trust key flag to close this integrity gap for
> >> all keyrings. The first key type to use this is X.509. When a X.509
> >> certificate is self signed, contains kernCertSign Key Usage and contains
> >> the CA bit, the new flag is set. Introduce new keyring restrictions
> >> that not only validates a key is signed by a key contained within the
> >> keyring, but also validates the key has the new root of trust key flag
> >> set. Use this new restriction for keys added to the ima keyring. Now
> >> that we have CA enforcement, allow the machine keyring to be used as another
> >> trust anchor for the ima keyring.
> >>
> >> To recap, all keys that previously loaded into the builtin, secondary or
> >> machine keyring will still load after applying this series. Keys
> >> contained within these keyrings may carry the root of trust flag. The
> >> ima keyring will use the new root of trust restriction to validate
> >> CA enforcement. Other keyrings that require a root of trust could also
> >> use this in the future.
> >
> > Your initial patch set indicated that you were addressing Linus'
> > request to allow end-users the ability "to add their own keys and sign
> > modules they trust". However, from the design of the previous patch
> > set and now this one, everything indicates a lot more is going on than
> > just allowing end-users to add their own keys. There would be no
> > reason for loading all the MOK keys, rather than just the CA keys, onto
> > the "machine" keyring. Please provide the motivation for this design.
>
> The motivation is to satisfy both Linus and your requests. Linus requested
> the ability to allow users to add their own keys and sign modules they trust.
> A code signing CA certificate does not require kernCertSign in the usage. Adding
> this as a requirement for kernel modules would be a regression (or a bug).
Of course a code signing CA certificate should not also be a
certificate signing key (keyCertSign). Remember the
"builtin_trusted_keys" and "secondary_trusted_keys" keyrings are
special. Their root of trust is based on a secure boot signature chain
of trust up to and including a signed kernel image. The "machine"
keyring is totally different in this regard. Establishing a new root
of trust is really difficult. Requiring a root-CA to have key
certifcate signing usage is a level of indirection, which I would
consider a small price to pay for being able to establish a, hopefully
safe or at least safer, new root of trust for trusting "end-user" keys.
>
> This series addresses your request to only trust validly signed CA certs.
> As you pointed out in the Kconfig help for
> IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY:
>
> help
> Keys may be added to the IMA or IMA blacklist keyrings, if the
> key is validly signed by a CA cert in the system built-in or
> secondary trusted keyrings.
>
> Intermediate keys between those the kernel has compiled in and the
> IMA keys to be added may be added to the system secondary keyring,
> provided they are validly signed by a key already resident in the
> built-in or secondary trusted keyrings.
>
> requires keys to be “validly” signed by a CA cert. Later the definition of a
> validly signed CA cert was defined as: self signed, contains kernCertSign
> key usage and contains the CA bit. While this help file states the CA restriction,
> nothing in code enforces it. One can place any type of self signed cert in either
> keyring and ima will use it. The motivation is for all keys added to the ima
> keyring to abide by the restriction defined in the Kconfig help. With this series
> this can be accomplished without introducing a regression on keys placed in
> any of the system keyrings.
>
> > Please note that Patch 6/7 permits intermediary CA keys, without any
> > mention of it in the cover letter. Please include this in the
> > motivation for this design.
>
> Ok, I’ll add that in the next round.
Your cover letter should say that this patch series enables
verification of 3rd party modules.
thanks,
Mimi
> On Apr 8, 2022, at 8:39 AM, Mimi Zohar <[email protected]> wrote:
>
> On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
>> Parse the X.509 Basic Constraints. The basic constraints extension
>> identifies whether the subject of the certificate is a CA.
>>
>> BasicConstraints ::= SEQUENCE {
>> cA BOOLEAN DEFAULT FALSE,
>> pathLenConstraint INTEGER (0..MAX) OPTIONAL }
>>
>> If the CA is true, store it in the x509_certificate. This will be used
>> in a follow on patch that requires knowing if the public key is a CA.
>>
>> Signed-off-by: Eric Snowberg <[email protected]>
>> ---
>> crypto/asymmetric_keys/x509_cert_parser.c | 9 +++++++++
>> crypto/asymmetric_keys/x509_parser.h | 1 +
>> 2 files changed, 10 insertions(+)
>>
>> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
>> index 2899ed80bb18..30f7374ea9c0 100644
>> --- a/crypto/asymmetric_keys/x509_cert_parser.c
>> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
>> @@ -583,6 +583,15 @@ int x509_process_extension(void *context, size_t hdrlen,
>> return 0;
>> }
>>
>> + if (ctx->last_oid == OID_basicConstraints) {
>> + if (vlen < 2 || v[0] != (ASN1_CONS_BIT | ASN1_SEQ))
>> + return -EBADMSG;
>> + if (v[1] != vlen - 2)
>> + return -EBADMSG;
>> + if (vlen >= 4 && v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
>> + ctx->cert->is_root_ca = true;
>> + }
>> +
>> return 0;
>> }
>>
>> diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
>> index 97a886cbe01c..dc45df9f6594 100644
>> --- a/crypto/asymmetric_keys/x509_parser.h
>> +++ b/crypto/asymmetric_keys/x509_parser.h
>> @@ -38,6 +38,7 @@ struct x509_certificate {
>> bool self_signed; /* T if self-signed (check unsupported_sig too) */
>> bool unsupported_sig; /* T if signature uses unsupported crypto */
>> bool blacklisted;
>> + bool is_root_ca; /* T if basic constraints CA is set */
>
> There's no need to prefix variables with "is_". Similar to the
> variable "self_signed" simply name this variable "root_ca".
I’ll change this name (and also the one you identified in the 3rd patch) in the next
round, thanks.
On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
> Parse the X.509 Key Usage. The key usage extension defines the purpose of
> the key contained in the certificate.
>
> id-ce-keyUsage OBJECT IDENTIFIER ::= { id-ce 15 }
>
> KeyUsage ::= BIT STRING {
> digitalSignature (0),
> contentCommitment (1),
> keyEncipherment (2),
> dataEncipherment (3),
> keyAgreement (4),
> keyCertSign (5),
> cRLSign (6),
> encipherOnly (7),
> decipherOnly (8) }
>
> If the keyCertSign is set, store it in the x509_certificate structure.
> This will be used in a follow on patch that requires knowing the
> certificate key usage type.
>
> Signed-off-by: Eric Snowberg <[email protected]>
> ---
> crypto/asymmetric_keys/x509_cert_parser.c | 20 ++++++++++++++++++++
> crypto/asymmetric_keys/x509_parser.h | 1 +
> 2 files changed, 21 insertions(+)
>
> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
> index 30f7374ea9c0..a89f1e0c8a0f 100644
> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -576,6 +576,26 @@ int x509_process_extension(void *context, size_t hdrlen,
> return 0;
> }
>
> + if (ctx->last_oid == OID_keyUsage) {
> + /*
> + * Get hold of the keyUsage bit string to validate keyCertSign
> + * v[1] is the encoding size
> + * (Expect either 0x02 or 0x03, making it 1 or 2 bytes)
> + * v[2] is the number of unused bits in the bit string
> + * (If >= 3 keyCertSign is missing)
> + * v[3] and possibly v[4] contain the bit string
> + * 0x04 is where KeyCertSign lands in this bit string (from
> + * RFC 5280 4.2.1.3)
> + */
> + if (v[0] != ASN1_BTS || vlen < 4)
> + return -EBADMSG;
> + if (v[1] == 0x02 && v[2] <= 2 && (v[3] & 0x04))
> + ctx->cert->is_kcs_set = true;
> + else if (vlen > 4 && v[1] == 0x03 && (v[3] & 0x04))
> + ctx->cert->is_kcs_set = true;
> + return 0;
> + }
> +
> if (ctx->last_oid == OID_authorityKeyIdentifier) {
> /* Get hold of the CA key fingerprint */
> ctx->raw_akid = v;
> diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
> index dc45df9f6594..d6ac0985d8a5 100644
> --- a/crypto/asymmetric_keys/x509_parser.h
> +++ b/crypto/asymmetric_keys/x509_parser.h
> @@ -39,6 +39,7 @@ struct x509_certificate {
> bool unsupported_sig; /* T if signature uses unsupported crypto */
> bool blacklisted;
> bool is_root_ca; /* T if basic constraints CA is set */
> + bool is_kcs_set; /* T if keyCertSign is set */
> };
Again there is no need to prefix the variable with "is_" or suffix it
with "set". Simply naming the variable "cert_signing" or
"keycertsign", like "self_signed", will improve code readability.
thanks,
Mimi
Hi Eric,
I wonder if there is any update on this work? I would be glad to do
anything that may be helpful including testing a new version of code.
--
Best regards,
Coiby
> On Nov 4, 2022, at 7:20 AM, Coiby Xu <[email protected]> wrote:
>
> Hi Eric,
>
> I wonder if there is any update on this work? I would be glad to do
> anything that may be helpful including testing a new version of code.
>
> --
> Best regards,
> Coiby
The discussion on this topic briefly moved over to this thread [1]. I took
the lack of response to mean an approach like this would not be considered.
If it would be considered, I am willing to continue working on a solution
to this problem.
1. https://lore.kernel.org/linux-integrity/[email protected]/
On 2022/11/04 9:20 AM, Coiby Xu wrote:
> Hi Eric,
>
> I wonder if there is any update on this work? I would be glad to do
> anything that may be helpful including testing a new version of code.
>
Hi Coiby,
Yes, this discussion got stuck when we couldn't agree on one of the
following options:
(A) Filter which keys from MOK (or a management system) are loaded
onto the .machine keyring. Specifically, load only keys with
CA+keyCertSign attributes.
(B) Load all keys from MOK (or a management system) onto the
.machine keyring. Then, subsequently filter those to restrict
which ones can be loaded onto the .ima keyring specifically.
The objection to (A) was that distros would have to go through
two steps instead of one to load keys. The one-step method of
loading keys was supported by an out-of-tree patch and then by
the addition of the .machine keyring.
The objection to (B) was that, because the .machine keyring is now
linked to the .secondary keyring, it expands the scope of what the
kernel has trusted in the past. The effect is that keys in MOK
have the same broad scope as keys previously restricted to
.builtin and .secondary. It doesn't affect just IMA, but the rest
of the kernel as well.
I would suggest that we can get unstuck by considering:
(C) Defining a systemd (or dracut module) to load keys onto the
.secondary keyring
(D) Using a configuration option to specify what types of
.machine keys should be allowed to pass through to the
.secondary keyring.
The distro could choose (A) by allowing only
CA+keyCertSign keys.
The distro could choose (B) by allowing any kind
of key.
We all seemed to agree that enforcing key usage should be
implemented and that a useful future effort is to add policies
to keys and keyrings, like, "This key can only be used for
verifying kernel modules."
I hope we can come to an agreement so work can proceed and IMA
can be re-enabled.
-Elaine Palmer
> On Nov 8, 2022, at 6:24 PM, Elaine Palmer <[email protected]> wrote:
>
>
>
> On 2022/11/04 9:20 AM, Coiby Xu wrote:
>> Hi Eric,
>>
>> I wonder if there is any update on this work? I would be glad to do
>> anything that may be helpful including testing a new version of code.
>>
> Hi Coiby,
>
> Yes, this discussion got stuck when we couldn't agree on one of the
> following options:
>
> (A) Filter which keys from MOK (or a management system) are loaded
> onto the .machine keyring. Specifically, load only keys with
> CA+keyCertSign attributes.
>
> (B) Load all keys from MOK (or a management system) onto the
> .machine keyring. Then, subsequently filter those to restrict
> which ones can be loaded onto the .ima keyring specifically.
>
> The objection to (A) was that distros would have to go through
> two steps instead of one to load keys. The one-step method of
> loading keys was supported by an out-of-tree patch and then by
> the addition of the .machine keyring.
>
> The objection to (B) was that, because the .machine keyring is now
> linked to the .secondary keyring, it expands the scope of what the
> kernel has trusted in the past. The effect is that keys in MOK
> have the same broad scope as keys previously restricted to
> .builtin and .secondary. It doesn't affect just IMA, but the rest
> of the kernel as well.
>
> I would suggest that we can get unstuck by considering:
>
> (C) Defining a systemd (or dracut module) to load keys onto the
> .secondary keyring
>
> (D) Using a configuration option to specify what types of
> .machine keys should be allowed to pass through to the
> .secondary keyring.
>
> The distro could choose (A) by allowing only
> CA+keyCertSign keys.
>
> The distro could choose (B) by allowing any kind
> of key.
>
> We all seemed to agree that enforcing key usage should be
> implemented and that a useful future effort is to add policies
> to keys and keyrings, like, "This key can only be used for
> verifying kernel modules."
>
> I hope we can come to an agreement so work can proceed and IMA
> can be re-enabled.
I would be open to making the changes necessary to support both (A and B)
options. What type of configuration option would be considered? Would this
be a compile time Kconfig, a Linux boot command line parameter, or another
MOK variable?
On 2022/11/09 9:25 AM, Eric Snowberg wrote:
>
>> On Nov 8, 2022, at 6:24 PM, Elaine Palmer <[email protected]> wrote:
>>
>>
>>
>> On 2022/11/04 9:20 AM, Coiby Xu wrote:
>>> Hi Eric,
>>>
>>> I wonder if there is any update on this work? I would be glad to do
>>> anything that may be helpful including testing a new version of code.
>>>
>> Hi Coiby,
>>
>> Yes, this discussion got stuck when we couldn't agree on one of the
>> following options:
>>
>> (A) Filter which keys from MOK (or a management system) are loaded
>> onto the .machine keyring. Specifically, load only keys with
>> CA+keyCertSign attributes.
>>
>> (B) Load all keys from MOK (or a management system) onto the
>> .machine keyring. Then, subsequently filter those to restrict
>> which ones can be loaded onto the .ima keyring specifically.
>>
>> The objection to (A) was that distros would have to go through
>> two steps instead of one to load keys. The one-step method of
>> loading keys was supported by an out-of-tree patch and then by
>> the addition of the .machine keyring.
>>
>> The objection to (B) was that, because the .machine keyring is now
>> linked to the .secondary keyring, it expands the scope of what the
>> kernel has trusted in the past. The effect is that keys in MOK
>> have the same broad scope as keys previously restricted to
>> .builtin and .secondary. It doesn't affect just IMA, but the rest
>> of the kernel as well.
>>
>> I would suggest that we can get unstuck by considering:
>>
>> (C) Defining a systemd (or dracut module) to load keys onto the
>> .secondary keyring
>>
>> (D) Using a configuration option to specify what types of
>> .machine keys should be allowed to pass through to the
>> .secondary keyring.
>>
>> The distro could choose (A) by allowing only
>> CA+keyCertSign keys.
>>
>> The distro could choose (B) by allowing any kind
>> of key.
>>
>> We all seemed to agree that enforcing key usage should be
>> implemented and that a useful future effort is to add policies
>> to keys and keyrings, like, "This key can only be used for
>> verifying kernel modules."
>>
>> I hope we can come to an agreement so work can proceed and IMA
>> can be re-enabled.
> I would be open to making the changes necessary to support both (A and B)
> options. What type of configuration option would be considered? Would this
> be a compile time Kconfig, a Linux boot command line parameter, or another
> MOK variable?
>
Thank you, Eric. A compile time Kconfig would be the most secure, yet
would still support (B) when allowed.