Re: [RFC PATCH 11/12] certs: Add a secondary system keyring that can be added to dynamically [ver #2]
On Tue, 2016-03-08 at 15:32 +, David Howells wrote: > Mimi Zohar wrote: > > > > The problem boils down to a difficulty in concocting a name that > > > describes a > > > complex situation that may change depending on the configuration. I can > > > make > > > it "restrict_link_by_any_system_trusted" if you'd prefer. > > > > > > That's why I want "system trusted keyrings" to refer to the builtin and > > > the > > > secondary - *and* an extra UEFI keyring if we grow one of those. It's a > > > collection of related keyrings. > > > > Sigh, this is the same discussion we've had for years. > > No, it isn't. Good! > > The UEFI keys should not be trusted to validate the certificates being added > > to the IMA keyring. > > A machine-security (e.g. UEFI) keyring will conceivably live in > certs/system_keyring.c and only be enabled if CONFIG_SYSTEM_TRUSTED_KEYRINGS=y > and, say, CONFIG_MACHINE_TRUSTED_KEYRING=y. I didn't say that IMA necessarily > has to use it. Ok. > What we need to do is define a set of functions allow IMA to get the > restrictions it wants, depending on configuration. In the code I currently > have, I think we have those: > > restrict_link_reject Option 1 > restrict_link_by_builtin_trusted Option 2 > restrict_link_by_system_trusted By renaming the system keyring to builtin, this is where it becomes unclear what is included by restrict_link_by_system_trusted - builtin and secondary, or builtin, secondary, and UEFI. > If you really want, I can add a restrict_link_for_ima in there, but I'd rather > not if IMA can use whichever of the above three most suits it. How about: > > restrict_link_reject > restrict_link_by_builtin_trusted > restrict_link_by_builtin_or_secondary_trusted Option 3 - "restrict_link_by_builtin_or_secondary_trusted" is a bit wordy, but there wouldn't be any confusion. Mimi > > Neither should the keys on the secondary keyring, unless specifically IMA > > Kconfig enabled, be used to validate the certificates being added to the IMA > > keyring. > > Yes. > > David >
Re: [RFC PATCH 11/12] certs: Add a secondary system keyring that can be added to dynamically [ver #2]
Mimi Zohar wrote: > > The problem boils down to a difficulty in concocting a name that describes a > > complex situation that may change depending on the configuration. I can > > make > > it "restrict_link_by_any_system_trusted" if you'd prefer. > > > > That's why I want "system trusted keyrings" to refer to the builtin and the > > secondary - *and* an extra UEFI keyring if we grow one of those. It's a > > collection of related keyrings. > > Sigh, this is the same discussion we've had for years. No, it isn't. > The UEFI keys should not be trusted to validate the certificates being added > to the IMA keyring. A machine-security (e.g. UEFI) keyring will conceivably live in certs/system_keyring.c and only be enabled if CONFIG_SYSTEM_TRUSTED_KEYRINGS=y and, say, CONFIG_MACHINE_TRUSTED_KEYRING=y. I didn't say that IMA necessarily has to use it. What we need to do is define a set of functions allow IMA to get the restrictions it wants, depending on configuration. In the code I currently have, I think we have those: restrict_link_reject restrict_link_by_builtin_trusted restrict_link_by_system_trusted If you really want, I can add a restrict_link_for_ima in there, but I'd rather not if IMA can use whichever of the above three most suits it. How about: restrict_link_reject restrict_link_by_builtin_trusted restrict_link_by_builtin_or_secondary_trusted > Neither should the keys on the secondary keyring, unless specifically IMA > Kconfig enabled, be used to validate the certificates being added to the IMA > keyring. Yes. David
Re: [RFC PATCH 11/12] certs: Add a secondary system keyring that can be added to dynamically [ver #2]
On Tue, 2016-03-08 at 14:43 +, David Howells wrote: > The problem boils down to a difficulty in concocting a name that describes a > complex situation that may change depending on the configuration. I can make > it "restrict_link_by_any_system_trusted" if you'd prefer. > > That's why I want "system trusted keyrings" to refer to the builtin and the > secondary - *and* an extra UEFI keyring if we grow one of those. It's a > collection of related keyrings. Sigh, this is the same discussion we've had for years. The UEFI keys should not be trusted to validate the certificates being added to the IMA keyring. Neither should the keys on the secondary keyring, unless specifically IMA Kconfig enabled, be used to validate the certificates being added to the IMA keyring. Just because the UEFI keys or the secondary keyring is enabled, doesn't mean that certificates signed by a key on either of those keyrings, should be added to the IMA keyring. The machine/system owner should control which keys can be used to verify certificates being added to the IMA keyring. Mimi
Re: [RFC PATCH 11/12] certs: Add a secondary system keyring that can be added to dynamically [ver #2]
Mimi Zohar wrote: > Would then restrict_link_to_system_trusted imply both the builtin and > secondary keyrings or just the builtin keyrings? Both, if available; just builtin if the secondary is not available. restrict_link_by_builtin_trusted() does only the builtin. > Changing the system keyring name to builtin keys, without changing the > corresponding restrict_link name, obfuscates what is really happening. You can still look at the code, it's not as if it's particularly hidden. The problem boils down to a difficulty in concocting a name that describes a complex situation that may change depending on the configuration. I can make it "restrict_link_by_any_system_trusted" if you'd prefer. That's why I want "system trusted keyrings" to refer to the builtin and the secondary - *and* an extra UEFI keyring if we grow one of those. It's a collection of related keyrings. David
Re: [RFC PATCH 11/12] certs: Add a secondary system keyring that can be added to dynamically [ver #2]
On Tue, 2016-03-08 at 13:13 +, David Howells wrote: > Mimi Zohar wrote: > > > but we're left with a lot of references to "system_trusted" (eg. > > restrict_link_to_system_trusted, depends on SYSTEM_TRUSTED_KEYRING > > How about I pluralise it to SYSTEM_TRUSTED_KEYRINGS? The fact that one is > called builtin and the other secondary doesn't detract from the fact that > they're both system-wide rings of trusted keys. Would then restrict_link_to_system_trusted imply both the builtin and secondary keyrings or just the builtin keyrings? Changing the system keyring name to builtin keys, without changing the corresponding restrict_link name, obfuscates what is really happening. Mimi
Re: [RFC PATCH 11/12] certs: Add a secondary system keyring that can be added to dynamically [ver #2]
On 16-03-08 13:13:59, David Howells wrote: > Mimi Zohar wrote: > > > but we're left with a lot of references to "system_trusted" (eg. > > restrict_link_to_system_trusted, depends on SYSTEM_TRUSTED_KEYRING > > How about I pluralise it to SYSTEM_TRUSTED_KEYRINGS? The fact that one is > called builtin and the other secondary doesn't detract from the fact that > they're both system-wide rings of trusted keys. > > Or would you prefer .system_trusted_keys and .secondary_trusted_keys? Even > though the second is also a "system" trusted keyring. Ah, naming things... This is true science... :-) cheers, Petko
Re: [RFC PATCH 11/12] certs: Add a secondary system keyring that can be added to dynamically [ver #2]
Mimi Zohar wrote: > but we're left with a lot of references to "system_trusted" (eg. > restrict_link_to_system_trusted, depends on SYSTEM_TRUSTED_KEYRING How about I pluralise it to SYSTEM_TRUSTED_KEYRINGS? The fact that one is called builtin and the other secondary doesn't detract from the fact that they're both system-wide rings of trusted keys. Or would you prefer .system_trusted_keys and .secondary_trusted_keys? Even though the second is also a "system" trusted keyring. David
Re: [RFC PATCH 11/12] certs: Add a secondary system keyring that can be added to dynamically [ver #2]
On Fri, 2016-03-04 at 15:01 +, David Howells wrote: > Add a secondary system keyring that can be added to by root whilst the > system is running - provided the key being added is vouched for by a key > built into the kernel or already added to the secondary keyring. > > Rename .system_keyring to .builtin_trusted_keys to distinguish it more > obviously from the new keyring (called .secondary_trusted_keys). Renaming of the system_trusted_keyring to builtin_trusted_keys is fine, but we're left with a lot of references to "system_trusted" (eg. restrict_link_to_system_trusted, depends on SYSTEM_TRUSTED_KEYRING, the subsequent patch description and Kconfig use "system trusted keyrings", etc). Without changing these references, I'm not convinced this is an improvement. Mimi > The new keyring needs to be enabled with CONFIG_SECONDARY_TRUSTED_KEYRING. > > If the secondary keyring is enabled, a link is created from that to > .builtin_trusted_keys so that the the latter will automatically be searched > too if the secondary keyring is searched. > > Signed-off-by: David Howells
[RFC PATCH 11/12] certs: Add a secondary system keyring that can be added to dynamically [ver #2]
Add a secondary system keyring that can be added to by root whilst the
system is running - provided the key being added is vouched for by a key
built into the kernel or already added to the secondary keyring.
Rename .system_keyring to .builtin_trusted_keys to distinguish it more
obviously from the new keyring (called .secondary_trusted_keys).
The new keyring needs to be enabled with CONFIG_SECONDARY_TRUSTED_KEYRING.
If the secondary keyring is enabled, a link is created from that to
.builtin_trusted_keys so that the the latter will automatically be searched
too if the secondary keyring is searched.
Signed-off-by: David Howells
---
certs/Kconfig |8
certs/system_keyring.c| 79 ++---
include/keys/system_keyring.h |4 ++
3 files changed, 78 insertions(+), 13 deletions(-)
diff --git a/certs/Kconfig b/certs/Kconfig
index 743d480f5f6f..fc5955f5fc8a 100644
--- a/certs/Kconfig
+++ b/certs/Kconfig
@@ -56,4 +56,12 @@ config SYSTEM_EXTRA_CERTIFICATE_SIZE
This is the number of bytes reserved in the kernel image for a
certificate to be inserted.
+config SECONDARY_TRUSTED_KEYRING
+ bool "Provide a keyring to which extra trustable keys may be added"
+ depends on SYSTEM_TRUSTED_KEYRING
+ help
+ If set, provide a keyring to which extra keys may be added, provided
+ those keys are not blacklisted and are vouched for by a key built
+ into the kernel or already in the secondary trusted keyring.
+
endmenu
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 3cfc88c76aee..9700f37350ff 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -18,7 +18,10 @@
#include
#include
-static struct key *system_trusted_keyring;
+static struct key *builtin_trusted_keys;
+#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
+static struct key *secondary_trusted_keys;
+#endif
extern __initconst const u8 system_certificate_list[];
extern __initconst const unsigned long system_certificate_list_size;
@@ -33,26 +36,68 @@ int restrict_link_by_system_trusted(struct key *keyring,
const struct key_type *type,
const union key_payload *payload)
{
- return restrict_link_by_signature(system_trusted_keyring,
- type, payload);
+ /* If we have a secondary trusted keyring, then that contains a link
+* through to the builtin keyring and the search will follow that link.
+*/
+#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
+ if (type == &key_type_keyring &&
+ keyring == secondary_trusted_keys &&
+ payload == &builtin_trusted_keys->payload)
+ /* Allow the builtin keyring to be added to the secondary */
+ return 0;
+
+ return restrict_link_by_signature(secondary_trusted_keys, type,
payload);
+#else
+ return restrict_link_by_signature(builtin_trusted_keys, type, payload);
+#endif
+}
+
+/**
+ * restrict_link_to_builtin_trusted - Restrict keyring addition by built in CA
+ *
+ * Restrict the addition of keys into a keyring based on the key-to-be-added
+ * being vouched for by a key in the built in system keyring.
+ */
+int restrict_link_by_builtin_trusted(struct key *keyring,
+const struct key_type *type,
+const union key_payload *payload)
+{
+ return restrict_link_by_signature(builtin_trusted_keys, type, payload);
}
/*
- * Load the compiled-in keys
+ * Create the trusted keyrings
*/
static __init int system_trusted_keyring_init(void)
{
- pr_notice("Initialise system trusted keyring\n");
+ pr_notice("Initialise system trusted keyrings\n");
- system_trusted_keyring =
- keyring_alloc(".system_keyring",
+ builtin_trusted_keys =
+ keyring_alloc(".builtin_trusted_keys",
KUIDT_INIT(0), KGIDT_INIT(0), current_cred(),
((KEY_POS_ALL & ~KEY_POS_SETATTR) |
KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH),
KEY_ALLOC_NOT_IN_QUOTA,
+ NULL, NULL);
+ if (IS_ERR(builtin_trusted_keys))
+ panic("Can't allocate builtin trusted keyring\n");
+
+#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
+ secondary_trusted_keys =
+ keyring_alloc(".secondary_trusted_keys",
+ KUIDT_INIT(0), KGIDT_INIT(0), current_cred(),
+ ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
+ KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH |
+ KEY_USR_WRITE),
+ KEY_ALLOC_NOT_IN_QUOTA,
restrict_link_by_system_trusted, NULL);
- if (IS_ERR(system_trusted_keyring))
- panic("Can
