Hello,
IMA measures/appraises modules when modprobe or insmod opens and reads them.
Unfortunately, there are no guarantees between what is read by userspace and
what is passed to the kernel via load_module system call. This patch adds
support for digital signature verification of kernel modules.
It uses the upstreamed digital signature verification support, which is also
used by IMA/EVM. There is no dependency on IMA/EVM, but both use the same
signature and key formats, defined by digital signature verification support.
These patches are intended to support the different use cases, from an
individual developer creating ephemeral keys, to the distro having an
existing signing mechanism in place.
For the distro, a well known public key can simply be embedded in the
kernel during the 'make' process.
For the developer, these patches create an ephemeral key during module
install, in order to limit the duration of the private key's existence.
Unfortunately, this necessitates embedding the public key in the kernel,
after the kernel has already been built. A new make target called
'signed_modules_install', creates the keypair, signs the modules,
removes the private key, and then, for now, recompiles the kernel using
'make bzImage'. For the developer, instead of doing 'make
modules_install', the new build process would be 'make', followed by
'make signed_modules_install' and 'make install'.
Scripts:
- new scripts/ksign.sh and scripts/genkey.sh scripts
- new targets signed_module_install and genkey for the top Makefile
- scripts/Makefile.modinst changes
Changelog v2:
- Replaces passing the signature as a separate argument, with appending
the signature to the kernel module during module install, as suggested
by Rusty Russell. (No module-init-tools changes required.)
- The signature is created during module install, after the module was
possibly stripped.
- Added support for using a builtin public key. (No requirement for an
initramfs to load the public key.)
- Added key creation and signing support to kernel Makefiles.
- Permits developers to conveniently sign their own modules with an
ephemeral key using "make signed_modules_install".
- Dmitry & Mimi
Dmitry Kasatkin (4):
integrity: added digest calculation function
modsig: add integrity_module_check hook
modsig: verify module integrity based on signature
modsig: build rules and scripts to generate keys and sign modules
Mimi Zohar (3):
keys: initialize root uid and session keyrings early
integrity: create and inititialize a keyring with builtin public key
modsig: initialize the _module public key keyring
Makefile | 38 ++++++++++
include/linux/integrity.h | 10 +++
kernel/module.c | 9 +++
scripts/Makefile.modinst | 1 +
scripts/genkey.sh | 135 ++++++++++++++++++++++++++++++++++++
scripts/ksign.sh | 64 +++++++++++++++++
security/integrity/Kconfig | 21 ++++++
security/integrity/Makefile | 18 +++++
security/integrity/digsig.c | 31 ++++++++-
security/integrity/digsig_pubkey.c | 96 +++++++++++++++++++++++++
security/integrity/integrity.h | 13 ++++
security/integrity/module.c | 91 ++++++++++++++++++++++++
security/keys/Makefile | 1 +
security/keys/root_keyring.c | 18 +++++
14 files changed, 544 insertions(+), 2 deletions(-)
create mode 100755 scripts/genkey.sh
create mode 100755 scripts/ksign.sh
create mode 100644 security/integrity/digsig_pubkey.c
create mode 100644 security/integrity/module.c
create mode 100644 security/keys/root_keyring.c
--
1.7.9.5
IMA measures/appraises modules when modprobe or insmod opens and read them.
Unfortunately, there are no guarantees between what is read by userspace and
what is passed to the kernel via load_module system call. This patch adds a
hook called integrity_module_check() to verify the integrity of the module
being loaded.
Signed-off-by: Dmitry Kasatkin <[email protected]>
---
include/linux/integrity.h | 10 ++++++++++
kernel/module.c | 9 +++++++++
2 files changed, 19 insertions(+)
diff --git a/include/linux/integrity.h b/include/linux/integrity.h
index 66c5fe9..a80ec06 100644
--- a/include/linux/integrity.h
+++ b/include/linux/integrity.h
@@ -37,4 +37,14 @@ static inline void integrity_inode_free(struct inode *inode)
return;
}
#endif /* CONFIG_INTEGRITY_H */
+
+#ifdef CONFIG_INTEGRITY_MODULES
+int integrity_module_check(const void *hdr, const unsigned long len);
+#else
+static inline int integrity_module_check(const void *buf, unsigned long len)
+{
+ return 0;
+}
+#endif
+
#endif /* _LINUX_INTEGRITY_H */
diff --git a/kernel/module.c b/kernel/module.c
index 4edbd9c..791da47 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -58,6 +58,7 @@
#include <linux/jump_label.h>
#include <linux/pfn.h>
#include <linux/bsearch.h>
+#include <linux/integrity.h>
#define CREATE_TRACE_POINTS
#include <trace/events/module.h>
@@ -2437,6 +2438,14 @@ static int copy_and_check(struct load_info *info,
info->hdr = hdr;
info->len = len;
+
+ err = integrity_module_check(hdr, len);
+ if (err < 0)
+ goto free_hdr;
+
+ /* cut signature tail */
+ info->len = err;
+
return 0;
free_hdr:
--
1.7.9.5
From: Mimi Zohar <[email protected]>
In order to create the integrity keyrings (eg. _evm, _ima, _modules),
root's uid and session keyrings need to be initialized early.
Signed-off-by: Mimi Zohar <[email protected]>
---
security/keys/Makefile | 1 +
security/keys/root_keyring.c | 18 ++++++++++++++++++
2 files changed, 19 insertions(+)
create mode 100644 security/keys/root_keyring.c
diff --git a/security/keys/Makefile b/security/keys/Makefile
index 504aaa0..c93ce8d 100644
--- a/security/keys/Makefile
+++ b/security/keys/Makefile
@@ -18,6 +18,7 @@ obj-y := \
obj-$(CONFIG_KEYS_COMPAT) += compat.o
obj-$(CONFIG_PROC_FS) += proc.o
obj-$(CONFIG_SYSCTL) += sysctl.o
+obj-$(CONFIG_INTEGRITY_SIGNATURE) += root_keyring.o
#
# Key types
diff --git a/security/keys/root_keyring.c b/security/keys/root_keyring.c
new file mode 100644
index 0000000..f6662eb
--- /dev/null
+++ b/security/keys/root_keyring.c
@@ -0,0 +1,18 @@
+/*
+ * Copyright (C) 2012 IBM Corporation
+ *
+ * Author:
+ * Mimi Zohar <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2 of the License.
+ */
+
+#include "internal.h"
+static int __init init_root_keyring(void)
+{
+ return install_user_keyrings();
+}
+
+late_initcall(init_root_keyring);
--
1.7.9.5
This patch adds support for verifying module integrity using the upstreamed
digital signature support. Signatures are appended to the kernel module
binary, as suggested by Rusty Russell. Using of this functionality does not
require changes to insmod and modprobe. The module hash is calculated and
verified against the signature. Public key and signature format can found
in Documentation/digsig.txt
Signed-off-by: Dmitry Kasatkin <[email protected]>
---
security/integrity/Kconfig | 11 ++++++
security/integrity/Makefile | 1 +
security/integrity/module.c | 91 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 103 insertions(+)
create mode 100644 security/integrity/module.c
diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index f789018..fbc7c78 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -27,5 +27,16 @@ config INTEGRITY_PUBKEY
custom built kernels, or a key used to label the entire filesystem for
EVM/IMA-appraisal.
+config INTEGRITY_MODULES
+ bool "Module integrity checking"
+ depends on SECURITY
+ select INTEGRITY_SIGNATURE
+ default n
+ help
+ This option enables module integrity checking,
+ using digital signatures
+
+ If unsure, say N.
+
source security/integrity/ima/Kconfig
source security/integrity/evm/Kconfig
diff --git a/security/integrity/Makefile b/security/integrity/Makefile
index cf234f5..226634c 100644
--- a/security/integrity/Makefile
+++ b/security/integrity/Makefile
@@ -4,6 +4,7 @@
obj-$(CONFIG_INTEGRITY) += integrity.o
obj-$(CONFIG_INTEGRITY_SIGNATURE) += digsig.o
+obj-$(CONFIG_INTEGRITY_MODULES) += module.o
integrity-y := iint.o
diff --git a/security/integrity/module.c b/security/integrity/module.c
new file mode 100644
index 0000000..b0e9ba2
--- /dev/null
+++ b/security/integrity/module.c
@@ -0,0 +1,91 @@
+/*
+ * Copyright (C) 2011,2012 Intel Corporation
+ *
+ * Authors:
+ * Dmitry Kasatkin <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2 of the
+ * License.
+ *
+ * File: module.c
+ * implements the module hooks: integrity_module_check
+ */
+
+#include <linux/module.h>
+#include <linux/security.h>
+
+#include "integrity.h"
+
+static const char sig_magic[] = "This Is A Crypto Signed Module";
+static const char sig_magic_len = sizeof(sig_magic) - 1;
+
+/**
+ * integrity_module_check - check module integrity
+ * @hdr: module data
+ * @len: length of the module
+ * @return: length without signature, error code on failure
+ *
+ * Hook to verify module integrity.
+ *
+ */
+int integrity_module_check(const void *hdr, unsigned long len)
+{
+ u8 digest[SHA1_DIGEST_SIZE];
+ int rc = -EINVAL, siglen;
+ char *tmp;
+
+ if (len < sig_magic_len + 2) {
+ pr_err("MODSIG: signature magic is missing\n");
+ goto err1;
+ }
+
+ tmp = (char *)hdr + len;
+
+ len -= sig_magic_len;
+ tmp -= sig_magic_len;
+ if (memcmp(tmp, sig_magic, sig_magic_len)) {
+ pr_err("MODSIG: wrong signature magic\n");
+ goto err1;
+ }
+
+ /* set to lenth */
+ len -= 2;
+ tmp -= 2;
+ siglen = __be16_to_cpup((u16 *)tmp);
+
+ if (len < siglen) {
+ pr_err("MODSIG: wrong signature\n");
+ goto err1;
+ }
+
+ len -= siglen;
+ tmp -= siglen;
+
+ rc = integrity_calc_digest("sha1", hdr, len, digest);
+ if (rc < 0)
+ goto err1;
+
+ rc = integrity_digsig_verify(INTEGRITY_KEYRING_MODULE, tmp, siglen,
+ digest, sizeof(digest));
+ if (rc) {
+ pr_err("MODSIG: module verification failed: %d", rc);
+ print_hex_dump(KERN_ERR, "hash: ", DUMP_PREFIX_NONE, 32, 1,
+ digest, sizeof(digest), 0);
+ goto err1;
+ }
+ rc = len;
+err1:
+ return rc;
+}
+
+static int __init module_check_init(void)
+{
+ return 0;
+}
+
+late_initcall(module_check_init);
+
+MODULE_DESCRIPTION("Module integrity verification");
+MODULE_LICENSE("GPL");
--
1.7.9.5
There are several functions, that need to calculate digest.
This patch adds common function for use by integrity subsystem.
Signed-off-by: Dmitry Kasatkin <[email protected]>
---
security/integrity/digsig.c | 31 +++++++++++++++++++++++++++++--
security/integrity/integrity.h | 3 +++
2 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 2dc167d..61a0c92 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -13,9 +13,9 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <linux/err.h>
-#include <linux/rbtree.h>
#include <linux/key-type.h>
#include <linux/digsig.h>
+#include <crypto/hash.h>
#include "integrity.h"
@@ -28,7 +28,7 @@ static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
};
int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
- const char *digest, int digestlen)
+ const char *digest, int digestlen)
{
if (id >= INTEGRITY_KEYRING_MAX)
return -EINVAL;
@@ -46,3 +46,30 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
return digsig_verify(keyring[id], sig, siglen, digest, digestlen);
}
+
+int integrity_calc_digest(const char *algo, const void *data, const int len,
+ char *digest)
+{
+ int rc = -ENOMEM;
+ struct crypto_shash *tfm;
+
+ tfm = crypto_alloc_shash(algo, 0, 0);
+ if (IS_ERR(tfm)) {
+ rc = PTR_ERR(tfm);
+ pr_err("Can not allocate %s (reason: %d)\n", algo, rc);
+ return rc;
+ } else {
+ struct {
+ struct shash_desc shash;
+ char ctx[crypto_shash_descsize(tfm)];
+ } desc;
+
+ desc.shash.tfm = tfm;
+ desc.shash.flags = 0;
+
+ rc = crypto_shash_digest(&desc.shash, data, len, digest);
+ }
+
+ crypto_free_shash(tfm);
+ return rc;
+}
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index e21362a..48ee2d4 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -59,6 +59,9 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
#ifdef CONFIG_INTEGRITY_SIGNATURE
+int integrity_calc_digest(const char *algo, const void *data, const int len,
+ char *digest);
+
int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
const char *digest, int digestlen);
--
1.7.9.5
From: Mimi Zohar <[email protected]>
Create and initialize a keyring with the builtin public key. This could
be an ephemeral key, created and destroyed during module install for
custom built kernels, or a key used to label the entire filesystem for
EVM/IMA-appraisal. Uses .incbin based on David Howell's post.
Load the builtin public key on the specified keyring, creating the
keyring if it doesn't already exist.
Signed-off-by: Mimi Zohar <[email protected]>
Signed-off-by: Dmitry Kasatkin <[email protected]>
---
security/integrity/Kconfig | 10 ++++
security/integrity/Makefile | 17 +++++++
security/integrity/digsig_pubkey.c | 96 ++++++++++++++++++++++++++++++++++++
security/integrity/integrity.h | 10 ++++
4 files changed, 133 insertions(+)
create mode 100644 security/integrity/digsig_pubkey.c
diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index 5bd1cc1..f789018 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -17,5 +17,15 @@ config INTEGRITY_SIGNATURE
This is useful for evm and module keyrings, when keys are
usually only added from initramfs.
+config INTEGRITY_PUBKEY
+ boolean "Create a keyring and initialize with builtin public key"
+ depends on INTEGRITY_SIGNATURE
+ default n
+ help
+ Create and initialize a keyring with the builtin public key. This could
+ be an ephemeral key, created and destroyed during module install for
+ custom built kernels, or a key used to label the entire filesystem for
+ EVM/IMA-appraisal.
+
source security/integrity/ima/Kconfig
source security/integrity/evm/Kconfig
diff --git a/security/integrity/Makefile b/security/integrity/Makefile
index d43799c..cf234f5 100644
--- a/security/integrity/Makefile
+++ b/security/integrity/Makefile
@@ -11,3 +11,20 @@ subdir-$(CONFIG_IMA) += ima
obj-$(CONFIG_IMA) += ima/built-in.o
subdir-$(CONFIG_EVM) += evm
obj-$(CONFIG_EVM) += evm/built-in.o
+
+ifeq ($(CONFIG_INTEGRITY_PUBKEY),y)
+
+obj-y += digsig_pubkey.o
+
+pubkeybin = pubkey.bin
+
+$(obj)/digsig_pubkey.o: $(pubkeybin)
+
+$(pubkeybin): FORCE
+ @if [ ! -s $(pubkeybin) ]; then \
+ echo "$(pubkeybin) is missing or empty..."; \
+ if [ ! -f $(pubkeybin) ]; then touch $(pubkeybin); fi; \
+ fi
+
+endif
+
diff --git a/security/integrity/digsig_pubkey.c b/security/integrity/digsig_pubkey.c
new file mode 100644
index 0000000..c714a60
--- /dev/null
+++ b/security/integrity/digsig_pubkey.c
@@ -0,0 +1,96 @@
+/*
+ * Copyright (C) 2012 IBM Corporation
+ * Copyright (C) 2012 Intel Corporation
+ *
+ * Author:
+ * Mimi Zohar <[email protected]>
+ * Dmitry Kasatkin <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2 of the License.
+ *
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/sched.h>
+#include <linux/cred.h>
+#include <linux/err.h>
+#include <linux/key-type.h>
+#include <keys/user-type.h>
+
+#include "integrity.h"
+
+static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
+ "_evm",
+ "_module",
+ "_ima",
+};
+
+asm(".section .init.data,\"aw\"\n"
+ "pubkey:\n"
+ ".incbin \"pubkey.bin\"\n"
+ "pubkey_end:"
+);
+
+extern __initdata const u8 pubkey[];
+extern __initdata const u8 pubkey_end[];
+
+/*
+ * integrity_init_keyring - create and initialize the specified keryring
+ *
+ * Create and initialize a keyring with the builtin public key. This could
+ * be an ephemeral key, created and destroyed during module install for
+ * custom built kernels, or a key used to label the entire filesystem for
+ * EVM/IMA-appraisal.
+ *
+ * Load the builtin public key on the specified keyring, creating the
+ * keyring if it doesn't already exist.
+ *
+ * Return 0 on success.
+ */
+int integrity_init_keyring(const unsigned int id)
+{
+ const struct cred *cred = current_cred();
+ struct user_struct *user = cred->user;
+ struct key *new_keyring, *key;
+ u8 digest[SHA1_DIGEST_SIZE];
+ char keyid[20];
+ int ret, pubkey_size = pubkey_end - pubkey;
+
+ if (pubkey_size == 0) {
+ pr_info("pubkey is missing, skipping...\n");
+ return 0;
+ }
+
+ new_keyring = keyring_alloc(keyring_name[id], user->uid, (gid_t) -1,
+ cred, KEY_ALLOC_NOT_IN_QUOTA,
+ user->uid_keyring);
+ if (IS_ERR(new_keyring)) {
+ ret = PTR_ERR(new_keyring);
+ goto out;
+ }
+
+ ret = integrity_calc_digest("sha1", pubkey, pubkey_size, digest);
+ if (ret < 0)
+ goto out;
+
+ sprintf(keyid, "%llX", __be64_to_cpup((uint64_t *)(digest+12)));
+
+ key = key_alloc(&key_type_user, keyid, 0, 0, current_cred(),
+ (KEY_POS_ALL & ~KEY_POS_SETATTR) |
+ KEY_USR_VIEW | KEY_USR_SEARCH | KEY_USR_READ,
+ KEY_ALLOC_NOT_IN_QUOTA);
+ if (IS_ERR(key)) {
+ ret = PTR_ERR(key);
+ goto out;
+ }
+
+ ret = key_instantiate_and_link(key, pubkey, pubkey_end - pubkey,
+ new_keyring, NULL);
+out:
+ pr_info("integrity: loaded public key %s on %s %s\n", keyid,
+ keyring_name[id], !ret ? "succeeded" : "failed");
+ return ret;
+}
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 48ee2d4..1070db8 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -13,6 +13,7 @@
#include <linux/types.h>
#include <linux/integrity.h>
+#include <linux/rbtree.h>
#include <crypto/sha.h>
/* iint cache flags */
@@ -76,5 +77,14 @@ static inline int integrity_digsig_verify(const unsigned int id,
#endif /* CONFIG_INTEGRITY_SIGNATURE */
+#ifdef CONFIG_INTEGRITY_PUBKEY
+int integrity_init_keyring(const unsigned int id);
+#else
+static inline int integrity_init_keyring(const unsigned int id)
+{
+ return 0;
+}
+#endif /* CONFIG_INTEGRITY_PUBKEY */
+
/* set during initialization */
extern int iint_initialized;
--
1.7.9.5
From: Mimi Zohar <[email protected]>
Create and initialize the _module public key keyring with the builtin
public key.
Signed-off-by: Mimi Zohar <[email protected]>
---
security/integrity/module.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/integrity/module.c b/security/integrity/module.c
index b0e9ba2..f696a9d 100644
--- a/security/integrity/module.c
+++ b/security/integrity/module.c
@@ -82,7 +82,7 @@ err1:
static int __init module_check_init(void)
{
- return 0;
+ return integrity_init_keyring(INTEGRITY_KEYRING_MODULE);
}
late_initcall(module_check_init);
--
1.7.9.5
This patch adds build rules and scripts to generate keys and sign modules.
Two scripts has been added. genkey.sh is used to generate private and
public keys. ksign.sh is used to sign kernel modules. Both scripts
use only standard utilites from coreutils and additionally requires
openssl tool for RSA keys creation and signing.
The patch modifies 'modules_install' target and adds two new targets to
the main kernel Makefile.
1. signed_modules_install
This target creates an ephemeral key pair, signs the kernel modules with
the private key, destroys the private key, and embeds the public key in
the kernel. (Thanks to Dave Hansen for the target name.)
2. modules_install
When CONFIG_INTEGRITY_MODULES is anabled, this target uses an existing
private key to sign kernel modules.
3. genkey
This target is automatically called for signed_modules_install to generate
ephemeral key pair, or can be called manually to generate new private and
public keypair for using with modules_install.
Signed-off-by: Dmitry Kasatkin <[email protected]>
---
Makefile | 38 +++++++++++++
scripts/Makefile.modinst | 1 +
scripts/genkey.sh | 135 ++++++++++++++++++++++++++++++++++++++++++++++
scripts/ksign.sh | 64 ++++++++++++++++++++++
4 files changed, 238 insertions(+)
create mode 100755 scripts/genkey.sh
create mode 100755 scripts/ksign.sh
diff --git a/Makefile b/Makefile
index d845c2a..fe693b3 100644
--- a/Makefile
+++ b/Makefile
@@ -939,10 +939,31 @@ modules.builtin: $(vmlinux-dirs:%=%/modules.builtin)
PHONY += modules_prepare
modules_prepare: prepare scripts
+privkey = privkey.pem
+pubkeybin = pubkey.bin
+
+ifeq ($(CONFIG_INTEGRITY_MODULES),y)
+# command to sign modules
+export quiet_cmd_sign_module = SIGN $$(@)
+export cmd_sign_module = $(srctree)/scripts/ksign.sh $$(2)
+endif
+
# Target to install modules
PHONY += modules_install
+ifeq ($(CONFIG_INTEGRITY_MODULES),y)
+modules_install: _checkkey_
+endif
modules_install: _modinst_ _modinst_post
+PHONY += _checkkey_
+_checkkey_:
+ @if [ ! -f $(privkey) -o ! -f $(pubkeybin) ]; then \
+ echo "Keys are missing. Run 'make genkey' first."; exit 1; \
+ fi; \
+ if [ $(pubkeybin) -nt vmlinux ]; then \
+ echo "$(pubkeybin) is newer than vmlinux. Run 'make bzImage' again."; exit 1; \
+ fi
+
PHONY += _modinst_
_modinst_:
@rm -rf $(MODLIB)/kernel
@@ -957,6 +978,23 @@ _modinst_:
@cp -f $(objtree)/modules.builtin $(MODLIB)/
$(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modinst
+# Target to install signed modules using new ephemeral key
+# It will force to generate new key pair and re-generate bzImage
+# Private key is removed after installing modules
+PHONY += signed_modules_install
+signed_modules_install: genkey _modinst_ _modinst_post _rmprivkey_ bzImage
+
+PHONY += genkey
+genkey: $(privkey) $(pubkeybin)
+
+$(privkey): FORCE
+ @if [ -f $(privkey) ]; then echo "$(privkey) already exists. Remove it first."; exit 1; fi
+ @$(srctree)/scripts/genkey.sh
+
+PHONY += _rmprivkey_
+_rmprivkey_:
+ @rm $(privkey)
+
# This depmod is only for convenience to give the initial
# boot a modules.dep even before / is mounted read-write. However the
# boot script depmod is the master version.
diff --git a/scripts/Makefile.modinst b/scripts/Makefile.modinst
index efa5d94..b0a2e5e 100644
--- a/scripts/Makefile.modinst
+++ b/scripts/Makefile.modinst
@@ -27,6 +27,7 @@ modinst_dir = $(if $(KBUILD_EXTMOD),$(ext-mod-dir),kernel/$(@D))
$(modules):
$(call cmd,modules_install,$(MODLIB)/$(modinst_dir))
+ $(call cmd,sign_module,$(MODLIB)/$(modinst_dir)/$(notdir $@))
# Declare the contents of the .PHONY variable as phony. We keep that
diff --git a/scripts/genkey.sh b/scripts/genkey.sh
new file mode 100755
index 0000000..9a4be9c
--- /dev/null
+++ b/scripts/genkey.sh
@@ -0,0 +1,135 @@
+#!/bin/bash
+
+set -e
+
+function error_exit() {
+ PROGNAME=$(basename $0)
+ echo "${PROGNAME}: ${1:-"Unknown Error"}" 1>&2
+ exit 1
+}
+
+trap "error_exit" ERR
+
+privkey=privkey.pem
+pubkey=pubkey.der
+pubkeybin=pubkey.bin
+
+if [ -f $privkey ]; then echo "$privkey already exists. Remove it first."; exit 1; fi
+
+rm -f $pubkeybin $pubkey
+
+hex2bin() {
+ src=$1
+ file=$2
+ while test -n "$src"; do
+ byte=${src:0:2}
+ src=${src:2}
+ printf "%b" \\x$byte >>$file;
+ done
+}
+
+hex2dec()
+{
+ printf "%d" 0x$1
+}
+
+dec2hex()
+{
+ printf "%0$2x" $1
+}
+
+der_get_length()
+{
+ local hex="$1"
+ local n=0
+ local l
+ #echo $hex
+ l=`hex2dec ${hex:2:2}`
+ if [ "$l" -ge 128 ]; then
+ n=$(( ($l - 128) * 2 ))
+ l=`hex2dec ${hex:4:$n}`
+ fi
+ eval $2=$(( 4 + $n ))
+ eval $3=$(( $l * 2 ))
+ eval $4=$l
+}
+
+count_bits()
+{
+ local bytes=$1
+ local msb=$2
+ bits=$(( ($bytes - 1) * 8 ))
+ local dec=`hex2dec $msb`
+ #echo "MSB: $msb $dec"
+ for (( ; $dec != 0 ; dec=($dec >> 1) )) ; do
+ bits=$(( $bits + 1 ))
+ done
+ eval $3=`dec2hex $bits 4`
+}
+# function to parse openssl public key in DER format
+der_parse()
+{
+ local hex="$1"
+ local tag
+ local pos=0
+ local len=0
+ local next=0
+ while test -n "$hex"; do
+ tag=${hex:0:2}
+ der_get_length "$hex" pos len bytelen
+ #echo "$tag: $pos $len $bytelen"
+ case $tag in
+ 30|03)
+ # sequence of elements
+ der_parse ${hex:$pos:$len}
+ ;;
+ 02)
+ # integer elements: modulus and exponent
+ if [ ${hex:$pos:2} = "00" ]; then
+ pos=$(( $pos + 2 ))
+ len=$(( $len - 2 ))
+ bytelen=$(( $bytelen - 1 ))
+ fi
+ #echo "value: ${hex:$pos:$len}"
+ count_bits $bytelen ${hex:$pos:2} bitlen
+ hex2bin $bitlen $pubkeybin
+ hex2bin ${hex:$pos:$len} $pubkeybin
+ ;;
+ 06|05)
+ # skip OID and NULL
+ #echo "skip, tag: $tag:$pos:$len"
+ ;;
+ 00)
+ # skip octet
+ pos=2
+ len=0
+ ;;
+ *)
+ echo "Must not happen... May be DER output format has changed"
+ echo "tag: $tag:$pos:$len"
+ #echo ${hex:$pos:$len}
+ exit 1
+ ;;
+ esac
+ next=$(( $pos + $len ))
+ hex=${hex:$next}
+ done
+}
+
+# pkh->version = 1;
+# pkh->timestamp = 0; /* PEM has no timestamp */
+# pkh->algo = PUBKEY_ALGO_RSA;
+# pkh->nmpi = 2;
+pkh="01000000000002"
+hex2bin $pkh $pubkeybin
+
+openssl genrsa -out $privkey 2048
+openssl rsa -in $privkey -pubout -outform der -out $pubkey
+
+size=`stat --printf %s $pubkey`
+key=`od -v -t x1 --width=$size pubkey.der | cut -c 9- | tr -d " "`
+
+der_parse $key
+
+rm -f $pubkey
+
diff --git a/scripts/ksign.sh b/scripts/ksign.sh
new file mode 100755
index 0000000..c3b4d4b
--- /dev/null
+++ b/scripts/ksign.sh
@@ -0,0 +1,64 @@
+#!/bin/bash
+
+module=$1
+sigfile=$module.sig
+sigout=$module.sigout
+sigdata=$module.sigdata
+sighash=$module.sighash
+
+pubkey=pubkey.bin
+
+rm -f $sigfile $sighash $sigdata $sigout
+
+hex2bin() {
+ read src
+ while test -n "$src"; do
+ byte=${src:0:2}
+ src=${src:2}
+ printf "%b" \\x$byte >>$1;
+ done
+}
+
+dec2hex()
+{
+ printf "%0$2x" $1
+}
+
+# version 1
+printf "%b" \\x1 >>$sigfile
+# timestamp - big endian
+stamp=`date +%s`
+dec2hex $stamp 8 | hex2bin $sigfile
+# rsa, sha1
+printf "%b%b" \\x0 \\x0 >>$sigfile
+#key id
+sha1sum pubkey.bin | cut -b 25-40 | hex2bin $sigfile
+# number of MPI
+printf "%b" \\x1 >>$sigfile
+
+# construct data for signature hash
+sha1sum $module | cut -b 1-40 | hex2bin $sigdata
+# add header
+cat $sigfile >> $sigdata
+
+# calculate hash to sign
+sha1sum $sigdata | cut -b 1-40 | hex2bin $sighash
+
+# sign final hash
+openssl rsautl -in $sighash -out $sigout -inkey privkey.pem -pkcs -sign
+
+# add MPI length - in big endian
+dec2hex $(( $(stat --printf %s $sigout) * 8 )) 4 | hex2bin $sigfile
+# add signature
+cat $sigout >> $sigfile
+
+# add kernel parameters
+# add signature length - big endian
+dec2hex $(stat --printf %s $sigfile) 4 | hex2bin $sigfile
+echo -n "This Is A Crypto Signed Module" >>$sigfile
+
+# add signature to a module
+cat $sigfile >> $module
+
+rm -f $sighash $sigdata $sigout $sigfile
+
--
1.7.9.5
Quoting Dmitry Kasatkin ([email protected]):
> There are several functions, that need to calculate digest.
> This patch adds common function for use by integrity subsystem.
>
> Signed-off-by: Dmitry Kasatkin <[email protected]>
> ---
> security/integrity/digsig.c | 31 +++++++++++++++++++++++++++++--
> security/integrity/integrity.h | 3 +++
> 2 files changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 2dc167d..61a0c92 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -13,9 +13,9 @@
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> #include <linux/err.h>
> -#include <linux/rbtree.h>
> #include <linux/key-type.h>
> #include <linux/digsig.h>
> +#include <crypto/hash.h>
>
> #include "integrity.h"
>
> @@ -28,7 +28,7 @@ static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
> };
>
> int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
> - const char *digest, int digestlen)
> + const char *digest, int digestlen)
> {
> if (id >= INTEGRITY_KEYRING_MAX)
> return -EINVAL;
> @@ -46,3 +46,30 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
>
> return digsig_verify(keyring[id], sig, siglen, digest, digestlen);
> }
> +
> +int integrity_calc_digest(const char *algo, const void *data, const int len,
> + char *digest)
> +{
> + int rc = -ENOMEM;
> + struct crypto_shash *tfm;
> +
> + tfm = crypto_alloc_shash(algo, 0, 0);
> + if (IS_ERR(tfm)) {
> + rc = PTR_ERR(tfm);
> + pr_err("Can not allocate %s (reason: %d)\n", algo, rc);
> + return rc;
> + } else {
> + struct {
> + struct shash_desc shash;
> + char ctx[crypto_shash_descsize(tfm)];
> + } desc;
Needless confusing indentation here. Just move the struct {} desc; to the
top and drop the else. That will make it much more readable.
> + desc.shash.tfm = tfm;
> + desc.shash.flags = 0;
> +
> + rc = crypto_shash_digest(&desc.shash, data, len, digest);
> + }
> +
> + crypto_free_shash(tfm);
> + return rc;
> +}
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index e21362a..48ee2d4 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -59,6 +59,9 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
>
> #ifdef CONFIG_INTEGRITY_SIGNATURE
>
> +int integrity_calc_digest(const char *algo, const void *data, const int len,
> + char *digest);
> +
> int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
> const char *digest, int digestlen);
>
> --
> 1.7.9.5
>
> --
> 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
Quoting Dmitry Kasatkin ([email protected]):
> IMA measures/appraises modules when modprobe or insmod opens and read them.
> Unfortunately, there are no guarantees between what is read by userspace and
> what is passed to the kernel via load_module system call. This patch adds a
> hook called integrity_module_check() to verify the integrity of the module
> being loaded.
>
> Signed-off-by: Dmitry Kasatkin <[email protected]>
> ---
> include/linux/integrity.h | 10 ++++++++++
> kernel/module.c | 9 +++++++++
> 2 files changed, 19 insertions(+)
>
> diff --git a/include/linux/integrity.h b/include/linux/integrity.h
> index 66c5fe9..a80ec06 100644
> --- a/include/linux/integrity.h
> +++ b/include/linux/integrity.h
> @@ -37,4 +37,14 @@ static inline void integrity_inode_free(struct inode *inode)
> return;
> }
> #endif /* CONFIG_INTEGRITY_H */
> +
> +#ifdef CONFIG_INTEGRITY_MODULES
> +int integrity_module_check(const void *hdr, const unsigned long len);
sadly not bisect-safe, since integrity_module_check() is defined in
the next patch.
> +#else
> +static inline int integrity_module_check(const void *buf, unsigned long len)
> +{
> + return 0;
> +}
> +#endif
> +
> #endif /* _LINUX_INTEGRITY_H */
> diff --git a/kernel/module.c b/kernel/module.c
> index 4edbd9c..791da47 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -58,6 +58,7 @@
> #include <linux/jump_label.h>
> #include <linux/pfn.h>
> #include <linux/bsearch.h>
> +#include <linux/integrity.h>
>
> #define CREATE_TRACE_POINTS
> #include <trace/events/module.h>
> @@ -2437,6 +2438,14 @@ static int copy_and_check(struct load_info *info,
>
> info->hdr = hdr;
> info->len = len;
> +
> + err = integrity_module_check(hdr, len);
> + if (err < 0)
> + goto free_hdr;
> +
> + /* cut signature tail */
> + info->len = err;
> +
> return 0;
>
> free_hdr:
> --
> 1.7.9.5
>
> --
> 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
On Wed, Aug 15, 2012 at 11:11 PM, Serge Hallyn
<[email protected]> wrote:
> Quoting Dmitry Kasatkin ([email protected]):
>> There are several functions, that need to calculate digest.
>> This patch adds common function for use by integrity subsystem.
>>
>> Signed-off-by: Dmitry Kasatkin <[email protected]>
>> ---
>> security/integrity/digsig.c | 31 +++++++++++++++++++++++++++++--
>> security/integrity/integrity.h | 3 +++
>> 2 files changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
>> index 2dc167d..61a0c92 100644
>> --- a/security/integrity/digsig.c
>> +++ b/security/integrity/digsig.c
>> @@ -13,9 +13,9 @@
>> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>
>> #include <linux/err.h>
>> -#include <linux/rbtree.h>
>> #include <linux/key-type.h>
>> #include <linux/digsig.h>
>> +#include <crypto/hash.h>
>>
>> #include "integrity.h"
>>
>> @@ -28,7 +28,7 @@ static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
>> };
>>
>> int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
>> - const char *digest, int digestlen)
>> + const char *digest, int digestlen)
>> {
>> if (id >= INTEGRITY_KEYRING_MAX)
>> return -EINVAL;
>> @@ -46,3 +46,30 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
>>
>> return digsig_verify(keyring[id], sig, siglen, digest, digestlen);
>> }
>> +
>> +int integrity_calc_digest(const char *algo, const void *data, const int len,
>> + char *digest)
>> +{
>> + int rc = -ENOMEM;
>> + struct crypto_shash *tfm;
>> +
>> + tfm = crypto_alloc_shash(algo, 0, 0);
>> + if (IS_ERR(tfm)) {
>> + rc = PTR_ERR(tfm);
>> + pr_err("Can not allocate %s (reason: %d)\n", algo, rc);
>> + return rc;
>> + } else {
>> + struct {
>> + struct shash_desc shash;
>> + char ctx[crypto_shash_descsize(tfm)];
>> + } desc;
>
> Needless confusing indentation here. Just move the struct {} desc; to the
> top and drop the else. That will make it much more readable.
>
Intention was to allocate it only if tfm allocation succeeded..
But indeed failure very unlikely..
thanks.
>> + desc.shash.tfm = tfm;
>> + desc.shash.flags = 0;
>> +
>> + rc = crypto_shash_digest(&desc.shash, data, len, digest);
>> + }
>> +
>> + crypto_free_shash(tfm);
>> + return rc;
>> +}
>> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
>> index e21362a..48ee2d4 100644
>> --- a/security/integrity/integrity.h
>> +++ b/security/integrity/integrity.h
>> @@ -59,6 +59,9 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
>>
>> #ifdef CONFIG_INTEGRITY_SIGNATURE
>>
>> +int integrity_calc_digest(const char *algo, const void *data, const int len,
>> + char *digest);
>> +
>> int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
>> const char *digest, int digestlen);
>>
>> --
>> 1.7.9.5
>>
>> --
>> 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
On Wed, Aug 15, 2012 at 11:16 PM, Serge Hallyn
<[email protected]> wrote:
> Quoting Dmitry Kasatkin ([email protected]):
>> IMA measures/appraises modules when modprobe or insmod opens and read them.
>> Unfortunately, there are no guarantees between what is read by userspace and
>> what is passed to the kernel via load_module system call. This patch adds a
>> hook called integrity_module_check() to verify the integrity of the module
>> being loaded.
>>
>> Signed-off-by: Dmitry Kasatkin <[email protected]>
>> ---
>> include/linux/integrity.h | 10 ++++++++++
>> kernel/module.c | 9 +++++++++
>> 2 files changed, 19 insertions(+)
>>
>> diff --git a/include/linux/integrity.h b/include/linux/integrity.h
>> index 66c5fe9..a80ec06 100644
>> --- a/include/linux/integrity.h
>> +++ b/include/linux/integrity.h
>> @@ -37,4 +37,14 @@ static inline void integrity_inode_free(struct inode *inode)
>> return;
>> }
>> #endif /* CONFIG_INTEGRITY_H */
>> +
>> +#ifdef CONFIG_INTEGRITY_MODULES
>> +int integrity_module_check(const void *hdr, const unsigned long len);
>
> sadly not bisect-safe, since integrity_module_check() is defined in
> the next patch.
>
Ok.. Hooks come after implementation...
Will swap.
Thanks.
>> +#else
>> +static inline int integrity_module_check(const void *buf, unsigned long len)
>> +{
>> + return 0;
>> +}
>> +#endif
>> +
>> #endif /* _LINUX_INTEGRITY_H */
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 4edbd9c..791da47 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -58,6 +58,7 @@
>> #include <linux/jump_label.h>
>> #include <linux/pfn.h>
>> #include <linux/bsearch.h>
>> +#include <linux/integrity.h>
>>
>> #define CREATE_TRACE_POINTS
>> #include <trace/events/module.h>
>> @@ -2437,6 +2438,14 @@ static int copy_and_check(struct load_info *info,
>>
>> info->hdr = hdr;
>> info->len = len;
>> +
>> + err = integrity_module_check(hdr, len);
>> + if (err < 0)
>> + goto free_hdr;
>> +
>> + /* cut signature tail */
>> + info->len = err;
>> +
>> return 0;
>>
>> free_hdr:
>> --
>> 1.7.9.5
>>
>> --
>> 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
On Wed, Aug 15, 2012 at 2:43 PM, Dmitry Kasatkin
<[email protected]> wrote:
> From: Mimi Zohar <[email protected]>
>
> In order to create the integrity keyrings (eg. _evm, _ima, _modules),
> root's uid and session keyrings need to be initialized early.
>
> Signed-off-by: Mimi Zohar <[email protected]>
> ---
> security/keys/Makefile | 1 +
> security/keys/root_keyring.c | 18 ++++++++++++++++++
> 2 files changed, 19 insertions(+)
> create mode 100644 security/keys/root_keyring.c
>
> diff --git a/security/keys/Makefile b/security/keys/Makefile
> index 504aaa0..c93ce8d 100644
> --- a/security/keys/Makefile
> +++ b/security/keys/Makefile
> @@ -18,6 +18,7 @@ obj-y := \
> obj-$(CONFIG_KEYS_COMPAT) += compat.o
> obj-$(CONFIG_PROC_FS) += proc.o
> obj-$(CONFIG_SYSCTL) += sysctl.o
> +obj-$(CONFIG_INTEGRITY_SIGNATURE) += root_keyring.o
>
> #
> # Key types
> diff --git a/security/keys/root_keyring.c b/security/keys/root_keyring.c
> new file mode 100644
> index 0000000..f6662eb
> --- /dev/null
> +++ b/security/keys/root_keyring.c
> @@ -0,0 +1,18 @@
> +/*
> + * Copyright (C) 2012 IBM Corporation
> + *
> + * Author:
> + * Mimi Zohar <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, version 2 of the License.
> + */
> +
> +#include "internal.h"
> +static int __init init_root_keyring(void)
> +{
> + return install_user_keyrings();
> +}
> +
> +late_initcall(init_root_keyring);
> --
Why is this in an entirely new file instead of just being added to
process_keys.c ?
josh
On Wed, Aug 15, 2012 at 2:43 PM, Dmitry Kasatkin
<[email protected]> wrote:
> From: Mimi Zohar <[email protected]>
>
> Create and initialize a keyring with the builtin public key. This could
> be an ephemeral key, created and destroyed during module install for
> custom built kernels, or a key used to label the entire filesystem for
> EVM/IMA-appraisal. Uses .incbin based on David Howell's post.
>
> Load the builtin public key on the specified keyring, creating the
> keyring if it doesn't already exist.
>
> Signed-off-by: Mimi Zohar <[email protected]>
> Signed-off-by: Dmitry Kasatkin <[email protected]>
> ---
> security/integrity/Kconfig | 10 ++++
> security/integrity/Makefile | 17 +++++++
> security/integrity/digsig_pubkey.c | 96 ++++++++++++++++++++++++++++++++++++
> security/integrity/integrity.h | 10 ++++
> 4 files changed, 133 insertions(+)
> create mode 100644 security/integrity/digsig_pubkey.c
>
> diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
> index 5bd1cc1..f789018 100644
> --- a/security/integrity/Kconfig
> +++ b/security/integrity/Kconfig
> @@ -17,5 +17,15 @@ config INTEGRITY_SIGNATURE
> This is useful for evm and module keyrings, when keys are
> usually only added from initramfs.
>
> +config INTEGRITY_PUBKEY
> + boolean "Create a keyring and initialize with builtin public key"
> + depends on INTEGRITY_SIGNATURE
> + default n
> + help
> + Create and initialize a keyring with the builtin public key. This could
> + be an ephemeral key, created and destroyed during module install for
> + custom built kernels, or a key used to label the entire filesystem for
> + EVM/IMA-appraisal.
> +
It could also be a key that is reused explicitly for signing kernels
and kernel modules but has nothing to do with EVM/IMA filesystem
labels, right? E.g. a distro key. I think the commit log and help
text is a bit too restrictive here.
> +int integrity_init_keyring(const unsigned int id)
> +{
> + const struct cred *cred = current_cred();
> + struct user_struct *user = cred->user;
> + struct key *new_keyring, *key;
> + u8 digest[SHA1_DIGEST_SIZE];
> + char keyid[20];
> + int ret, pubkey_size = pubkey_end - pubkey;
> +
> + if (pubkey_size == 0) {
> + pr_info("pubkey is missing, skipping...\n");
> + return 0;
> + }
> +
> + new_keyring = keyring_alloc(keyring_name[id], user->uid, (gid_t) -1,
> + cred, KEY_ALLOC_NOT_IN_QUOTA,
> + user->uid_keyring);
> + if (IS_ERR(new_keyring)) {
> + ret = PTR_ERR(new_keyring);
> + goto out;
> + }
> +
> + ret = integrity_calc_digest("sha1", pubkey, pubkey_size, digest);
> + if (ret < 0)
> + goto out;
> +
> + sprintf(keyid, "%llX", __be64_to_cpup((uint64_t *)(digest+12)));
keyid is only 20 bytes. Is there a guarantee somewhere that restricts
the digest+12 value to be 20 bytes or less and NUL termintated? If
not, should you use snprintf?
> +
> + key = key_alloc(&key_type_user, keyid, 0, 0, current_cred(),
> + (KEY_POS_ALL & ~KEY_POS_SETATTR) |
> + KEY_USR_VIEW | KEY_USR_SEARCH | KEY_USR_READ,
> + KEY_ALLOC_NOT_IN_QUOTA);
> + if (IS_ERR(key)) {
> + ret = PTR_ERR(key);
> + goto out;
You leak new_keyring if here, right?
> + }
> +
> + ret = key_instantiate_and_link(key, pubkey, pubkey_end - pubkey,
> + new_keyring, NULL);
> +out:
> + pr_info("integrity: loaded public key %s on %s %s\n", keyid,
> + keyring_name[id], !ret ? "succeeded" : "failed");
> + return ret;
> +}
josh
On Wed, Aug 15, 2012 at 2:43 PM, Dmitry Kasatkin
<[email protected]> wrote:
> @@ -2437,6 +2438,14 @@ static int copy_and_check(struct load_info *info,
>
> info->hdr = hdr;
> info->len = len;
> +
> + err = integrity_module_check(hdr, len);
> + if (err < 0)
> + goto free_hdr;
> +
> + /* cut signature tail */
> + info->len = err;
> +
> return 0;
>
> free_hdr:
So if I'm reading this correctly, any module that fails signature
verification will fail to load. That makes sense, but I wonder if you
intend to support a non-enforcing mode for module signatures at all?
Actually, a brief document in Documentation describing how this whole
mechanism works and what the fail states are would be good. David's
patches have it nicely spelled out and I don't see anything similar in
your patch set.
josh
On Wed, Aug 15, 2012 at 2:43 PM, Dmitry Kasatkin
<[email protected]> wrote:
> From: Mimi Zohar <[email protected]>
>
> Create and initialize the _module public key keyring with the builtin
> public key.
>
> Signed-off-by: Mimi Zohar <[email protected]>
This seems like it could be folded into "modsig: add
integrity_module_check hook" or "modsig: verify module integrity based
on signature". Not sure why it needs to be a separate patch at first
glance.
josh
On Wed, Aug 15, 2012 at 2:43 PM, Dmitry Kasatkin
<[email protected]> wrote:
> This patch adds build rules and scripts to generate keys and sign modules.
>
> Two scripts has been added. genkey.sh is used to generate private and
> public keys. ksign.sh is used to sign kernel modules. Both scripts
> use only standard utilites from coreutils and additionally requires
> openssl tool for RSA keys creation and signing.
>
> The patch modifies 'modules_install' target and adds two new targets to
> the main kernel Makefile.
>
> 1. signed_modules_install
> This target creates an ephemeral key pair, signs the kernel modules with
> the private key, destroys the private key, and embeds the public key in
> the kernel. (Thanks to Dave Hansen for the target name.)
This requires CONFIG_INTEGRITY_MODULES to be enabled to actually do
anything useful with the signed modules, correct?
>
> 2. modules_install
> When CONFIG_INTEGRITY_MODULES is enabled, this target uses an existing
> private key to sign kernel modules.
If the answer to the above question is yes, then why can't we stick
with a single modules_install command for signing? It would seem to me
that calling signed_modules_install could use an existing key or
generate an ephemeral key in the absence of one and install the signed
modules, and modules_install would simply install unsigned modules.
Or, alternatively, just make modules_install sign or not sign depending
on whether CONFIG_INTEGRITY_MODULES is enabled. I don't see why you
would overload a target or create two different ones when both depend
on that option.
Could you explain the reasoning behind that a bit more?
josh
On Thu, 2012-08-16 at 14:26 -0400, Josh Boyer wrote:
> On Wed, Aug 15, 2012 at 2:43 PM, Dmitry Kasatkin
> <[email protected]> wrote:
> > From: Mimi Zohar <[email protected]>
> >
> > In order to create the integrity keyrings (eg. _evm, _ima, _modules),
> > root's uid and session keyrings need to be initialized early.
> >
> > Signed-off-by: Mimi Zohar <[email protected]>
> > ---
> > security/keys/Makefile | 1 +
> > security/keys/root_keyring.c | 18 ++++++++++++++++++
> > 2 files changed, 19 insertions(+)
> > create mode 100644 security/keys/root_keyring.c
> >
> > diff --git a/security/keys/Makefile b/security/keys/Makefile
> > index 504aaa0..c93ce8d 100644
> > --- a/security/keys/Makefile
> > +++ b/security/keys/Makefile
> > @@ -18,6 +18,7 @@ obj-y := \
> > obj-$(CONFIG_KEYS_COMPAT) += compat.o
> > obj-$(CONFIG_PROC_FS) += proc.o
> > obj-$(CONFIG_SYSCTL) += sysctl.o
> > +obj-$(CONFIG_INTEGRITY_SIGNATURE) += root_keyring.o
> >
> > #
> > # Key types
> > diff --git a/security/keys/root_keyring.c b/security/keys/root_keyring.c
> > new file mode 100644
> > index 0000000..f6662eb
> > --- /dev/null
> > +++ b/security/keys/root_keyring.c
> > @@ -0,0 +1,18 @@
> > +/*
> > + * Copyright (C) 2012 IBM Corporation
> > + *
> > + * Author:
> > + * Mimi Zohar <[email protected]>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation, version 2 of the License.
> > + */
> > +
> > +#include "internal.h"
> > +static int __init init_root_keyring(void)
> > +{
> > + return install_user_keyrings();
> > +}
> > +
> > +late_initcall(init_root_keyring);
> > --
>
> Why is this in an entirely new file instead of just being added to
> process_keys.c ?
>
> josh
Only when "CONFIG_INTEGRITY_SIGNATURE" is selected, does this get built.
Mimi
On Thu, Aug 16, 2012 at 3:08 PM, Mimi Zohar <[email protected]> wrote:
>> > +#include "internal.h"
>> > +static int __init init_root_keyring(void)
>> > +{
>> > + return install_user_keyrings();
>> > +}
>> > +
>> > +late_initcall(init_root_keyring);
>> > --
>>
>> Why is this in an entirely new file instead of just being added to
>> process_keys.c ?
>>
>> josh
>
> Only when "CONFIG_INTEGRITY_SIGNATURE" is selected, does this get built.
Yes, I noticed that. It doesn't explain why it's in its own file. You
could accomplish the same thing by wrapping the function and initcall
in #ifdef CONFIG_INTEGRITY_SIGNATURE in process_keys.c.
josh
On Thu, 2012-08-16 at 14:37 -0400, Josh Boyer wrote:
> On Wed, Aug 15, 2012 at 2:43 PM, Dmitry Kasatkin
> <[email protected]> wrote:
> > From: Mimi Zohar <[email protected]>
> >
> > Create and initialize a keyring with the builtin public key. This could
> > be an ephemeral key, created and destroyed during module install for
> > custom built kernels, or a key used to label the entire filesystem for
> > EVM/IMA-appraisal. Uses .incbin based on David Howell's post.
> >
> > Load the builtin public key on the specified keyring, creating the
> > keyring if it doesn't already exist.
> >
> > Signed-off-by: Mimi Zohar <[email protected]>
> > Signed-off-by: Dmitry Kasatkin <[email protected]>
> > ---
> > security/integrity/Kconfig | 10 ++++
> > security/integrity/Makefile | 17 +++++++
> > security/integrity/digsig_pubkey.c | 96 ++++++++++++++++++++++++++++++++++++
> > security/integrity/integrity.h | 10 ++++
> > 4 files changed, 133 insertions(+)
> > create mode 100644 security/integrity/digsig_pubkey.c
> >
> > diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
> > index 5bd1cc1..f789018 100644
> > --- a/security/integrity/Kconfig
> > +++ b/security/integrity/Kconfig
> > @@ -17,5 +17,15 @@ config INTEGRITY_SIGNATURE
> > This is useful for evm and module keyrings, when keys are
> > usually only added from initramfs.
> >
> > +config INTEGRITY_PUBKEY
> > + boolean "Create a keyring and initialize with builtin public key"
> > + depends on INTEGRITY_SIGNATURE
> > + default n
> > + help
> > + Create and initialize a keyring with the builtin public key. This could
> > + be an ephemeral key, created and destroyed during module install for
> > + custom built kernels, or a key used to label the entire filesystem for
> > + EVM/IMA-appraisal.
> > +
>
> It could also be a key that is reused explicitly for signing kernels
> and kernel modules but has nothing to do with EVM/IMA filesystem
> labels, right? E.g. a distro key. I think the commit log and help
> text is a bit too restrictive here.
Definitely! The cover letter included the distro scenario, but I missed
updating it here.
> > +int integrity_init_keyring(const unsigned int id)
> > +{
> > + const struct cred *cred = current_cred();
> > + struct user_struct *user = cred->user;
> > + struct key *new_keyring, *key;
> > + u8 digest[SHA1_DIGEST_SIZE];
> > + char keyid[20];
> > + int ret, pubkey_size = pubkey_end - pubkey;
> > +
> > + if (pubkey_size == 0) {
> > + pr_info("pubkey is missing, skipping...\n");
> > + return 0;
> > + }
> > +
> > + new_keyring = keyring_alloc(keyring_name[id], user->uid, (gid_t) -1,
> > + cred, KEY_ALLOC_NOT_IN_QUOTA,
> > + user->uid_keyring);
> > + if (IS_ERR(new_keyring)) {
> > + ret = PTR_ERR(new_keyring);
> > + goto out;
> > + }
> > +
> > + ret = integrity_calc_digest("sha1", pubkey, pubkey_size, digest);
> > + if (ret < 0)
> > + goto out;
> > +
> > + sprintf(keyid, "%llX", __be64_to_cpup((uint64_t *)(digest+12)));
>
> keyid is only 20 bytes. Is there a guarantee somewhere that restricts
> the digest+12 value to be 20 bytes or less and NUL termintated? If
> not, should you use snprintf?
Correct, and SHA1 shouldn't be hardcoded either, but configurable.
>
> > +
> > + key = key_alloc(&key_type_user, keyid, 0, 0, current_cred(),
> > + (KEY_POS_ALL & ~KEY_POS_SETATTR) |
> > + KEY_USR_VIEW | KEY_USR_SEARCH | KEY_USR_READ,
> > + KEY_ALLOC_NOT_IN_QUOTA);
> > + if (IS_ERR(key)) {
> > + ret = PTR_ERR(key);
> > + goto out;
>
> You leak new_keyring if here, right?
thanks
> > + }
> > +
> > + ret = key_instantiate_and_link(key, pubkey, pubkey_end - pubkey,
> > + new_keyring, NULL);
> > +out:
> > + pr_info("integrity: loaded public key %s on %s %s\n", keyid,
> > + keyring_name[id], !ret ? "succeeded" : "failed");
> > + return ret;
> > +}
>
> josh
Mimi
On Thu, 2012-08-16 at 15:13 -0400, Josh Boyer wrote:
> On Thu, Aug 16, 2012 at 3:08 PM, Mimi Zohar <[email protected]> wrote:
> >> > +#include "internal.h"
> >> > +static int __init init_root_keyring(void)
> >> > +{
> >> > + return install_user_keyrings();
> >> > +}
> >> > +
> >> > +late_initcall(init_root_keyring);
> >> > --
> >>
> >> Why is this in an entirely new file instead of just being added to
> >> process_keys.c ?
> >>
> >> josh
> >
> > Only when "CONFIG_INTEGRITY_SIGNATURE" is selected, does this get built.
>
> Yes, I noticed that. It doesn't explain why it's in its own file. You
> could accomplish the same thing by wrapping the function and initcall
> in #ifdef CONFIG_INTEGRITY_SIGNATURE in process_keys.c.
I was under the impression using 'ifdefs' in 'C' code was frowned upon
(Documentation/SubmittingPatches section 2.2). This would be an
exception?
Mimi
On Thu, Aug 16, 2012 at 9:49 PM, Josh Boyer <[email protected]> wrote:
> On Wed, Aug 15, 2012 at 2:43 PM, Dmitry Kasatkin
> <[email protected]> wrote:
>> @@ -2437,6 +2438,14 @@ static int copy_and_check(struct load_info *info,
>>
>> info->hdr = hdr;
>> info->len = len;
>> +
>> + err = integrity_module_check(hdr, len);
>> + if (err < 0)
>> + goto free_hdr;
>> +
>> + /* cut signature tail */
>> + info->len = err;
>> +
>> return 0;
>>
>> free_hdr:
>
> So if I'm reading this correctly, any module that fails signature
> verification will fail to load. That makes sense, but I wonder if you
> intend to support a non-enforcing mode for module signatures at all?
> Actually, a brief document in Documentation describing how this whole
> mechanism works and what the fail states are would be good. David's
> patches have it nicely spelled out and I don't see anything similar in
> your patch set.
>
> josh
Hi,
I had enable and enforce mode in my previous patches.
I have removed them just before posting.
I added now enforcing back..
- Dmitry
On Thu, Aug 16, 2012 at 3:45 PM, Mimi Zohar <[email protected]> wrote:
> On Thu, 2012-08-16 at 15:13 -0400, Josh Boyer wrote:
>> On Thu, Aug 16, 2012 at 3:08 PM, Mimi Zohar <[email protected]> wrote:
>> >> > +#include "internal.h"
>> >> > +static int __init init_root_keyring(void)
>> >> > +{
>> >> > + return install_user_keyrings();
>> >> > +}
>> >> > +
>> >> > +late_initcall(init_root_keyring);
>> >> > --
>> >>
>> >> Why is this in an entirely new file instead of just being added to
>> >> process_keys.c ?
>> >>
>> >> josh
>> >
>> > Only when "CONFIG_INTEGRITY_SIGNATURE" is selected, does this get built.
>>
>> Yes, I noticed that. It doesn't explain why it's in its own file. You
>> could accomplish the same thing by wrapping the function and initcall
>> in #ifdef CONFIG_INTEGRITY_SIGNATURE in process_keys.c.
>
> I was under the impression using 'ifdefs' in 'C' code was frowned upon
> (Documentation/SubmittingPatches section 2.2). This would be an
> exception?
If it makes a big ugly mess it's frowned upon. But if you're adding 7
lines of code in a new file that will almost certainly never get more
code added to it, I'm not sure. IMHO, it can go into an existing file.
Others might disagree. Isn't Linux development fun?!
josh
On Thu, 2012-08-16 at 14:54 -0400, Josh Boyer wrote:
> On Wed, Aug 15, 2012 at 2:43 PM, Dmitry Kasatkin
> <[email protected]> wrote:
> > From: Mimi Zohar <[email protected]>
> >
> > Create and initialize the _module public key keyring with the builtin
> > public key.
> >
> > Signed-off-by: Mimi Zohar <[email protected]>
>
> This seems like it could be folded into "modsig: add
> integrity_module_check hook" or "modsig: verify module integrity based
> on signature". Not sure why it needs to be a separate patch at first
> glance.
No reason other than my sending it to Dmitry. I'm sure he can squash it
with his own patches.
thanks,
Mimi
On Thu, 2012-08-16 at 15:59 -0400, Josh Boyer wrote:
> On Thu, Aug 16, 2012 at 3:45 PM, Mimi Zohar <[email protected]> wrote:
> > On Thu, 2012-08-16 at 15:13 -0400, Josh Boyer wrote:
> >> On Thu, Aug 16, 2012 at 3:08 PM, Mimi Zohar <[email protected]> wrote:
> >> >> > +#include "internal.h"
> >> >> > +static int __init init_root_keyring(void)
> >> >> > +{
> >> >> > + return install_user_keyrings();
> >> >> > +}
> >> >> > +
> >> >> > +late_initcall(init_root_keyring);
> >> >> > --
> >> >>
> >> >> Why is this in an entirely new file instead of just being added to
> >> >> process_keys.c ?
> >> >>
> >> >> josh
> >> >
> >> > Only when "CONFIG_INTEGRITY_SIGNATURE" is selected, does this get built.
> >>
> >> Yes, I noticed that. It doesn't explain why it's in its own file. You
> >> could accomplish the same thing by wrapping the function and initcall
> >> in #ifdef CONFIG_INTEGRITY_SIGNATURE in process_keys.c.
> >
> > I was under the impression using 'ifdefs' in 'C' code was frowned upon
> > (Documentation/SubmittingPatches section 2.2). This would be an
> > exception?
>
> If it makes a big ugly mess it's frowned upon. But if you're adding 7
> lines of code in a new file that will almost certainly never get more
> code added to it, I'm not sure. IMHO, it can go into an existing file.
> Others might disagree. Isn't Linux development fun?!
This is just a case where if I had 'ifdef's in 'C' code, I'm sure
someone would have complained. :)
Mimi
On Thu, Aug 16, 2012 at 10:10 PM, Josh Boyer <[email protected]> wrote:
> On Wed, Aug 15, 2012 at 2:43 PM, Dmitry Kasatkin
> <[email protected]> wrote:
>> This patch adds build rules and scripts to generate keys and sign modules.
>>
>> Two scripts has been added. genkey.sh is used to generate private and
>> public keys. ksign.sh is used to sign kernel modules. Both scripts
>> use only standard utilites from coreutils and additionally requires
>> openssl tool for RSA keys creation and signing.
>>
>> The patch modifies 'modules_install' target and adds two new targets to
>> the main kernel Makefile.
>>
>> 1. signed_modules_install
>> This target creates an ephemeral key pair, signs the kernel modules with
>> the private key, destroys the private key, and embeds the public key in
>> the kernel. (Thanks to Dave Hansen for the target name.)
>
> This requires CONFIG_INTEGRITY_MODULES to be enabled to actually do
> anything useful with the signed modules, correct?
>
Correct. It does not make sense to sign module if module support is disabled.
But there scripts/genkey.sh and ksign.sh which works without Makefiles.
So possible to generate keys and sign a module...
>>
>> 2. modules_install
>> When CONFIG_INTEGRITY_MODULES is enabled, this target uses an existing
>> private key to sign kernel modules.
>
> If the answer to the above question is yes, then why can't we stick
> with a single modules_install command for signing? It would seem to me
> that calling signed_modules_install could use an existing key or
> generate an ephemeral key in the absence of one and install the signed
> modules, and modules_install would simply install unsigned modules.
>
> Or, alternatively, just make modules_install sign or not sign depending
> on whether CONFIG_INTEGRITY_MODULES is enabled.
This is what "make modules_install" does. It uses existing private key
and does not remove it after install.
> I don't see why you
> would overload a target or create two different ones when both depend
> on that option.
>
> Could you explain the reasoning behind that a bit more?
The reason for "signed_modules_install" is to limit existence of private key.
Private key is generate just before install, modules installed and
signed, then key is destroyed.
So existence of private key is limited to "time make
signed_modules_install" execution time.
We had a debate about it, and strong message was that we might want to
do it like that...
With the "modules_install", it will not generate new private key
before install, but uses which is already in the directory..
And it will not remove the private key after install.
- Dmitry
>
> josh
On Thu, 2012-08-16 at 15:10 -0400, Josh Boyer wrote:
> On Wed, Aug 15, 2012 at 2:43 PM, Dmitry Kasatkin
> <[email protected]> wrote:
> > This patch adds build rules and scripts to generate keys and sign modules.
> >
> > Two scripts has been added. genkey.sh is used to generate private and
> > public keys. ksign.sh is used to sign kernel modules. Both scripts
> > use only standard utilites from coreutils and additionally requires
> > openssl tool for RSA keys creation and signing.
> >
> > The patch modifies 'modules_install' target and adds two new targets to
> > the main kernel Makefile.
> >
> > 1. signed_modules_install
> > This target creates an ephemeral key pair, signs the kernel modules with
> > the private key, destroys the private key, and embeds the public key in
> > the kernel. (Thanks to Dave Hansen for the target name.)
>
> This requires CONFIG_INTEGRITY_MODULES to be enabled to actually do
> anything useful with the signed modules, correct?
>
> >
> > 2. modules_install
> > When CONFIG_INTEGRITY_MODULES is enabled, this target uses an existing
> > private key to sign kernel modules.
>
> If the answer to the above question is yes, then why can't we stick
> with a single modules_install command for signing? It would seem to me
> that calling signed_modules_install could use an existing key or
> generate an ephemeral key in the absence of one and install the signed
> modules, and modules_install would simply install unsigned modules.
>
> Or, alternatively, just make modules_install sign or not sign depending
> on whether CONFIG_INTEGRITY_MODULES is enabled. I don't see why you
> would overload a target or create two different ones when both depend
> on that option.
>
> Could you explain the reasoning behind that a bit more?
If the key exists during the build, there is no need for the additional
'make bzImage'. I'll update the patch description based on the
following explaination taken from the cover letter:
For the developer, these patches create an ephemeral key during module
install, in order to limit the duration of the private key's existence.
Unfortunately, this necessitates embedding the public key in the kernel,
after the kernel has already been built. A new make target called
'signed_modules_install', creates the keypair, signs the modules,
removes the private key, and then, for now, recompiles the kernel using
'make bzImage'. For the developer, instead of doing 'make
modules_install', the new build process would be 'make', followed by
'make signed_modules_install' and 'make install'.
thanks,
Mimi
On Thu, Aug 16, 2012 at 4:12 PM, Kasatkin, Dmitry
<[email protected]> wrote:
>>> 1. signed_modules_install
>>> This target creates an ephemeral key pair, signs the kernel modules with
>>> the private key, destroys the private key, and embeds the public key in
>>> the kernel. (Thanks to Dave Hansen for the target name.)
>>
>> This requires CONFIG_INTEGRITY_MODULES to be enabled to actually do
>> anything useful with the signed modules, correct?
>>
>
> Correct. It does not make sense to sign module if module support is disabled.
> But there scripts/genkey.sh and ksign.sh which works without Makefiles.
> So possible to generate keys and sign a module...
Right, but it won't actually do anything if the config option isn't set.
Which means someone calling 'make signed_modules_install' won't actually
get signed modules. That's confusing.
>>> 2. modules_install
>>> When CONFIG_INTEGRITY_MODULES is enabled, this target uses an existing
>>> private key to sign kernel modules.
>>
>> If the answer to the above question is yes, then why can't we stick
>> with a single modules_install command for signing? It would seem to me
>> that calling signed_modules_install could use an existing key or
>> generate an ephemeral key in the absence of one and install the signed
>> modules, and modules_install would simply install unsigned modules.
>>
>> Or, alternatively, just make modules_install sign or not sign depending
>> on whether CONFIG_INTEGRITY_MODULES is enabled.
>
> This is what "make modules_install" does. It uses existing private key
> and does not remove it after install.
Right. I should have been more clear. I was suggesting that "make
modules_install" generate a key as well if one does not already exist.
Essentially removing the necessity for sign_modules_install.
>> I don't see why you
>> would overload a target or create two different ones when both depend
>> on that option.
>>
>> Could you explain the reasoning behind that a bit more?
>
> The reason for "signed_modules_install" is to limit existence of private key.
> Private key is generate just before install, modules installed and
> signed, then key is destroyed.
> So existence of private key is limited to "time make
> signed_modules_install" execution time.
>
> We had a debate about it, and strong message was that we might want to
> do it like that...
I guess I personally don't see the need to destroy they key so quickly.
Is the concern that an intruder might grab the key and use it to sign a
module that the developer would then later on somehow load? Or
similarly someone would grab a temporary key from a distro build
machine? That limits the attack surface, sure, but I'm not sure it's
really reasonable.
For a developer that isn't distributing kernels to others, it's just
adding more time to the compile (which I know can be disabled, but
still). For a distribution, most of them are either using a private
key already or they have a buildsystem that destroys a buildroot after
a build completes. The key is already going to be destroyed in that
scenario.
josh
On Thu, Aug 16, 2012 at 12:11 AM, Kasatkin, Dmitry
<[email protected]> wrote:
> On Wed, Aug 15, 2012 at 11:11 PM, Serge Hallyn
> <[email protected]> wrote:
>> Quoting Dmitry Kasatkin ([email protected]):
>>> There are several functions, that need to calculate digest.
>>> This patch adds common function for use by integrity subsystem.
>>>
>>> Signed-off-by: Dmitry Kasatkin <[email protected]>
>>> ---
>>> security/integrity/digsig.c | 31 +++++++++++++++++++++++++++++--
>>> security/integrity/integrity.h | 3 +++
>>> 2 files changed, 32 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
>>> index 2dc167d..61a0c92 100644
>>> --- a/security/integrity/digsig.c
>>> +++ b/security/integrity/digsig.c
>>> @@ -13,9 +13,9 @@
>>> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>
>>> #include <linux/err.h>
>>> -#include <linux/rbtree.h>
>>> #include <linux/key-type.h>
>>> #include <linux/digsig.h>
>>> +#include <crypto/hash.h>
>>>
>>> #include "integrity.h"
>>>
>>> @@ -28,7 +28,7 @@ static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
>>> };
>>>
>>> int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
>>> - const char *digest, int digestlen)
>>> + const char *digest, int digestlen)
>>> {
>>> if (id >= INTEGRITY_KEYRING_MAX)
>>> return -EINVAL;
>>> @@ -46,3 +46,30 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
>>>
>>> return digsig_verify(keyring[id], sig, siglen, digest, digestlen);
>>> }
>>> +
>>> +int integrity_calc_digest(const char *algo, const void *data, const int len,
>>> + char *digest)
>>> +{
>>> + int rc = -ENOMEM;
>>> + struct crypto_shash *tfm;
>>> +
>>> + tfm = crypto_alloc_shash(algo, 0, 0);
>>> + if (IS_ERR(tfm)) {
>>> + rc = PTR_ERR(tfm);
>>> + pr_err("Can not allocate %s (reason: %d)\n", algo, rc);
>>> + return rc;
>>> + } else {
>>> + struct {
>>> + struct shash_desc shash;
>>> + char ctx[crypto_shash_descsize(tfm)];
>>> + } desc;
>>
>> Needless confusing indentation here. Just move the struct {} desc; to the
>> top and drop the else. That will make it much more readable.
>>
>
> Intention was to allocate it only if tfm allocation succeeded..
> But indeed failure very unlikely..
>
BTW.. The reason for such code is that ctx member uses function in the
parameter:
char ctx[crypto_shash_descsize(tfm)];
It is not possible to do it before tfm allocation...
So I cannot move it up..
I can only kmalloc it then.
There are many places of such allocation on stack in the kernel code.
sparse also complains. I know..
But it looks for me reasonable, as descriptor size is not big...
- Dmitry
> thanks.
>
>>> + desc.shash.tfm = tfm;
>>> + desc.shash.flags = 0;
>>> +
>>> + rc = crypto_shash_digest(&desc.shash, data, len, digest);
>>> + }
>>> +
>>> + crypto_free_shash(tfm);
>>> + return rc;
>>> +}
>>> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
>>> index e21362a..48ee2d4 100644
>>> --- a/security/integrity/integrity.h
>>> +++ b/security/integrity/integrity.h
>>> @@ -59,6 +59,9 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
>>>
>>> #ifdef CONFIG_INTEGRITY_SIGNATURE
>>>
>>> +int integrity_calc_digest(const char *algo, const void *data, const int len,
>>> + char *digest);
>>> +
>>> int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
>>> const char *digest, int digestlen);
>>>
>>> --
>>> 1.7.9.5
>>>
>>> --
>>> 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
On Thu, Aug 16, 2012 at 11:31 PM, Josh Boyer <[email protected]> wrote:
> On Thu, Aug 16, 2012 at 4:12 PM, Kasatkin, Dmitry
> <[email protected]> wrote:
>>>> 1. signed_modules_install
>>>> This target creates an ephemeral key pair, signs the kernel modules with
>>>> the private key, destroys the private key, and embeds the public key in
>>>> the kernel. (Thanks to Dave Hansen for the target name.)
>>>
>>> This requires CONFIG_INTEGRITY_MODULES to be enabled to actually do
>>> anything useful with the signed modules, correct?
>>>
>>
>> Correct. It does not make sense to sign module if module support is disabled.
>> But there scripts/genkey.sh and ksign.sh which works without Makefiles.
>> So possible to generate keys and sign a module...
>
> Right, but it won't actually do anything if the config option isn't set.
> Which means someone calling 'make signed_modules_install' won't actually
> get signed modules. That's confusing.
>
Yes.. It had to be behind "ifeq CONFIG_INTEGRITY_MODULES"
>>>> 2. modules_install
>>>> When CONFIG_INTEGRITY_MODULES is enabled, this target uses an existing
>>>> private key to sign kernel modules.
>>>
>>> If the answer to the above question is yes, then why can't we stick
>>> with a single modules_install command for signing? It would seem to me
>>> that calling signed_modules_install could use an existing key or
>>> generate an ephemeral key in the absence of one and install the signed
>>> modules, and modules_install would simply install unsigned modules.
>>>
>>> Or, alternatively, just make modules_install sign or not sign depending
>>> on whether CONFIG_INTEGRITY_MODULES is enabled.
>>
>> This is what "make modules_install" does. It uses existing private key
>> and does not remove it after install.
>
> Right. I should have been more clear. I was suggesting that "make
> modules_install" generate a key as well if one does not already exist.
> Essentially removing the necessity for sign_modules_install.
>
>>> I don't see why you
>>> would overload a target or create two different ones when both depend
>>> on that option.
>>>
>>> Could you explain the reasoning behind that a bit more?
>>
>> The reason for "signed_modules_install" is to limit existence of private key.
>> Private key is generate just before install, modules installed and
>> signed, then key is destroyed.
>> So existence of private key is limited to "time make
>> signed_modules_install" execution time.
>>
>> We had a debate about it, and strong message was that we might want to
>> do it like that...
>
> I guess I personally don't see the need to destroy they key so quickly.
> Is the concern that an intruder might grab the key and use it to sign a
> module that the developer would then later on somehow load? Or
> similarly someone would grab a temporary key from a distro build
> machine? That limits the attack surface, sure, but I'm not sure it's
> really reasonable.
>
> For a developer that isn't distributing kernels to others, it's just
> adding more time to the compile (which I know can be disabled, but
> still). For a distribution, most of them are either using a private
> key already or they have a buildsystem that destroys a buildroot after
> a build completes. The key is already going to be destroyed in that
> scenario.
>
> josh
Well... Will not argue here. I had similar opinion as well.
Mimi strongly wanted really to "reduce" the existence time of the key...
- Dmitry
On Thu, Aug 16, 2012 at 9:37 PM, Josh Boyer <[email protected]> wrote:
> On Wed, Aug 15, 2012 at 2:43 PM, Dmitry Kasatkin
> <[email protected]> wrote:
>> From: Mimi Zohar <[email protected]>
>>
>> Create and initialize a keyring with the builtin public key. This could
>> be an ephemeral key, created and destroyed during module install for
>> custom built kernels, or a key used to label the entire filesystem for
>> EVM/IMA-appraisal. Uses .incbin based on David Howell's post.
>>
>> Load the builtin public key on the specified keyring, creating the
>> keyring if it doesn't already exist.
>>
>> Signed-off-by: Mimi Zohar <[email protected]>
>> Signed-off-by: Dmitry Kasatkin <[email protected]>
>> ---
>> security/integrity/Kconfig | 10 ++++
>> security/integrity/Makefile | 17 +++++++
>> security/integrity/digsig_pubkey.c | 96 ++++++++++++++++++++++++++++++++++++
>> security/integrity/integrity.h | 10 ++++
>> 4 files changed, 133 insertions(+)
>> create mode 100644 security/integrity/digsig_pubkey.c
>>
>> diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
>> index 5bd1cc1..f789018 100644
>> --- a/security/integrity/Kconfig
>> +++ b/security/integrity/Kconfig
>> @@ -17,5 +17,15 @@ config INTEGRITY_SIGNATURE
>> This is useful for evm and module keyrings, when keys are
>> usually only added from initramfs.
>>
>> +config INTEGRITY_PUBKEY
>> + boolean "Create a keyring and initialize with builtin public key"
>> + depends on INTEGRITY_SIGNATURE
>> + default n
>> + help
>> + Create and initialize a keyring with the builtin public key. This could
>> + be an ephemeral key, created and destroyed during module install for
>> + custom built kernels, or a key used to label the entire filesystem for
>> + EVM/IMA-appraisal.
>> +
>
> It could also be a key that is reused explicitly for signing kernels
> and kernel modules but has nothing to do with EVM/IMA filesystem
> labels, right? E.g. a distro key. I think the commit log and help
> text is a bit too restrictive here.
>
>> +int integrity_init_keyring(const unsigned int id)
>> +{
>> + const struct cred *cred = current_cred();
>> + struct user_struct *user = cred->user;
>> + struct key *new_keyring, *key;
>> + u8 digest[SHA1_DIGEST_SIZE];
>> + char keyid[20];
>> + int ret, pubkey_size = pubkey_end - pubkey;
>> +
>> + if (pubkey_size == 0) {
>> + pr_info("pubkey is missing, skipping...\n");
>> + return 0;
>> + }
>> +
>> + new_keyring = keyring_alloc(keyring_name[id], user->uid, (gid_t) -1,
>> + cred, KEY_ALLOC_NOT_IN_QUOTA,
>> + user->uid_keyring);
>> + if (IS_ERR(new_keyring)) {
>> + ret = PTR_ERR(new_keyring);
>> + goto out;
>> + }
>> +
>> + ret = integrity_calc_digest("sha1", pubkey, pubkey_size, digest);
>> + if (ret < 0)
>> + goto out;
>> +
>> + sprintf(keyid, "%llX", __be64_to_cpup((uint64_t *)(digest+12)));
>
> keyid is only 20 bytes. Is there a guarantee somewhere that restricts
> the digest+12 value to be 20 bytes or less and NUL termintated? If
> not, should you use snprintf?
>
No.. Digest here is 20 bytes, but keyid is 8 bytes - it is u64...
It cannot go beyond here...
>> +
>> + key = key_alloc(&key_type_user, keyid, 0, 0, current_cred(),
>> + (KEY_POS_ALL & ~KEY_POS_SETATTR) |
>> + KEY_USR_VIEW | KEY_USR_SEARCH | KEY_USR_READ,
>> + KEY_ALLOC_NOT_IN_QUOTA);
>> + if (IS_ERR(key)) {
>> + ret = PTR_ERR(key);
>> + goto out;
>
> You leak new_keyring if here, right?
>
>> + }
>> +
>> + ret = key_instantiate_and_link(key, pubkey, pubkey_end - pubkey,
>> + new_keyring, NULL);
>> +out:
>> + pr_info("integrity: loaded public key %s on %s %s\n", keyid,
>> + keyring_name[id], !ret ? "succeeded" : "failed");
>> + return ret;
>> +}
>
> josh
Quoting Kasatkin, Dmitry ([email protected]):
> On Thu, Aug 16, 2012 at 12:11 AM, Kasatkin, Dmitry
> <[email protected]> wrote:
> > On Wed, Aug 15, 2012 at 11:11 PM, Serge Hallyn
> > <[email protected]> wrote:
> >> Quoting Dmitry Kasatkin ([email protected]):
> >>> There are several functions, that need to calculate digest.
> >>> This patch adds common function for use by integrity subsystem.
> >>>
> >>> Signed-off-by: Dmitry Kasatkin <[email protected]>
> >>> ---
> >>> security/integrity/digsig.c | 31 +++++++++++++++++++++++++++++--
> >>> security/integrity/integrity.h | 3 +++
> >>> 2 files changed, 32 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> >>> index 2dc167d..61a0c92 100644
> >>> --- a/security/integrity/digsig.c
> >>> +++ b/security/integrity/digsig.c
> >>> @@ -13,9 +13,9 @@
> >>> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >>>
> >>> #include <linux/err.h>
> >>> -#include <linux/rbtree.h>
> >>> #include <linux/key-type.h>
> >>> #include <linux/digsig.h>
> >>> +#include <crypto/hash.h>
> >>>
> >>> #include "integrity.h"
> >>>
> >>> @@ -28,7 +28,7 @@ static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
> >>> };
> >>>
> >>> int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
> >>> - const char *digest, int digestlen)
> >>> + const char *digest, int digestlen)
> >>> {
> >>> if (id >= INTEGRITY_KEYRING_MAX)
> >>> return -EINVAL;
> >>> @@ -46,3 +46,30 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
> >>>
> >>> return digsig_verify(keyring[id], sig, siglen, digest, digestlen);
> >>> }
> >>> +
> >>> +int integrity_calc_digest(const char *algo, const void *data, const int len,
> >>> + char *digest)
> >>> +{
> >>> + int rc = -ENOMEM;
> >>> + struct crypto_shash *tfm;
> >>> +
> >>> + tfm = crypto_alloc_shash(algo, 0, 0);
> >>> + if (IS_ERR(tfm)) {
> >>> + rc = PTR_ERR(tfm);
> >>> + pr_err("Can not allocate %s (reason: %d)\n", algo, rc);
> >>> + return rc;
> >>> + } else {
> >>> + struct {
> >>> + struct shash_desc shash;
> >>> + char ctx[crypto_shash_descsize(tfm)];
> >>> + } desc;
> >>
> >> Needless confusing indentation here. Just move the struct {} desc; to the
> >> top and drop the else. That will make it much more readable.
> >>
> >
> > Intention was to allocate it only if tfm allocation succeeded..
> > But indeed failure very unlikely..
> >
>
> BTW.. The reason for such code is that ctx member uses function in the
> parameter:
>
> char ctx[crypto_shash_descsize(tfm)];
>
> It is not possible to do it before tfm allocation...
> So I cannot move it up..
Ah, I see. Cool :)
> I can only kmalloc it then.
Well no, you could use another function I suppose.
But if you're going to leave it as is, please at least move the whole
rest of the function into the else{} :) Yes, no functional change,
but a change in how it looks to the eye of someone trying to review
and look for actual free-unallocated-memory errors or leaks.
-serge
On Fri, 2012-08-17 at 00:04 +0300, Kasatkin, Dmitry wrote:
> On Thu, Aug 16, 2012 at 11:31 PM, Josh Boyer <[email protected]> wrote:
> > On Thu, Aug 16, 2012 at 4:12 PM, Kasatkin, Dmitry
> > <[email protected]> wrote:
> >>>> 1. signed_modules_install
> >>>> This target creates an ephemeral key pair, signs the kernel modules with
> >>>> the private key, destroys the private key, and embeds the public key in
> >>>> the kernel. (Thanks to Dave Hansen for the target name.)
> >>>
> >>> This requires CONFIG_INTEGRITY_MODULES to be enabled to actually do
> >>> anything useful with the signed modules, correct?
> >>>
> >>
> >> Correct. It does not make sense to sign module if module support is disabled.
> >> But there scripts/genkey.sh and ksign.sh which works without Makefiles.
> >> So possible to generate keys and sign a module...
> >
> > Right, but it won't actually do anything if the config option isn't set.
> > Which means someone calling 'make signed_modules_install' won't actually
> > get signed modules. That's confusing.
> >
>
> Yes.. It had to be behind "ifeq CONFIG_INTEGRITY_MODULES"
>
> >>>> 2. modules_install
> >>>> When CONFIG_INTEGRITY_MODULES is enabled, this target uses an existing
> >>>> private key to sign kernel modules.
> >>>
> >>> If the answer to the above question is yes, then why can't we stick
> >>> with a single modules_install command for signing? It would seem to me
> >>> that calling signed_modules_install could use an existing key or
> >>> generate an ephemeral key in the absence of one and install the signed
> >>> modules, and modules_install would simply install unsigned modules.
> >>>
> >>> Or, alternatively, just make modules_install sign or not sign depending
> >>> on whether CONFIG_INTEGRITY_MODULES is enabled.
> >>
> >> This is what "make modules_install" does. It uses existing private key
> >> and does not remove it after install.
> >
> > Right. I should have been more clear. I was suggesting that "make
> > modules_install" generate a key as well if one does not already exist.
> > Essentially removing the necessity for sign_modules_install.
> >
> >>> I don't see why you
> >>> would overload a target or create two different ones when both depend
> >>> on that option.
> >>>
> >>> Could you explain the reasoning behind that a bit more?
> >>
> >> The reason for "signed_modules_install" is to limit existence of private key.
> >> Private key is generate just before install, modules installed and
> >> signed, then key is destroyed.
> >> So existence of private key is limited to "time make
> >> signed_modules_install" execution time.
> >>
> >> We had a debate about it, and strong message was that we might want to
> >> do it like that...
> >
> > I guess I personally don't see the need to destroy they key so quickly.
> > Is the concern that an intruder might grab the key and use it to sign a
> > module that the developer would then later on somehow load? Or
> > similarly someone would grab a temporary key from a distro build
> > machine? That limits the attack surface, sure, but I'm not sure it's
> > really reasonable.
> >
> > For a developer that isn't distributing kernels to others, it's just
> > adding more time to the compile (which I know can be disabled, but
> > still). For a distribution, most of them are either using a private
> > key already or they have a buildsystem that destroys a buildroot after
> > a build completes. The key is already going to be destroyed in that
> > scenario.
> >
> > josh
>
> Well... Will not argue here. I had similar opinion as well.
>
> Mimi strongly wanted really to "reduce" the existence time of the key...
The options are creating the key during 'make' or 'make
modules_install'. If you create the key during 'make', then you have no
way of knowing whether or not it is a persistent or ephemeral key, and
whether it should be deleted after signing the modules.
You could create a persistent key using 'make genkey', before 'make',
and never delete the private key. Then there wouldn't be any
overhead. :) If 'CONFIG_INTEGRITY_MODULE' is configured, 'make
modules_install' would use the existing key.
'make signed_modules_install' would be for creating and using ephemeral
keys.
What do you think?
Mimi
On Thu, Aug 16, 2012 at 12:13 AM, Kasatkin, Dmitry
<[email protected]> wrote:
> On Wed, Aug 15, 2012 at 11:16 PM, Serge Hallyn
> <[email protected]> wrote:
>> Quoting Dmitry Kasatkin ([email protected]):
>>> IMA measures/appraises modules when modprobe or insmod opens and read them.
>>> Unfortunately, there are no guarantees between what is read by userspace and
>>> what is passed to the kernel via load_module system call. This patch adds a
>>> hook called integrity_module_check() to verify the integrity of the module
>>> being loaded.
>>>
>>> Signed-off-by: Dmitry Kasatkin <[email protected]>
>>> ---
>>> include/linux/integrity.h | 10 ++++++++++
>>> kernel/module.c | 9 +++++++++
>>> 2 files changed, 19 insertions(+)
>>>
>>> diff --git a/include/linux/integrity.h b/include/linux/integrity.h
>>> index 66c5fe9..a80ec06 100644
>>> --- a/include/linux/integrity.h
>>> +++ b/include/linux/integrity.h
>>> @@ -37,4 +37,14 @@ static inline void integrity_inode_free(struct inode *inode)
>>> return;
>>> }
>>> #endif /* CONFIG_INTEGRITY_H */
>>> +
>>> +#ifdef CONFIG_INTEGRITY_MODULES
>>> +int integrity_module_check(const void *hdr, const unsigned long len);
>>
>> sadly not bisect-safe, since integrity_module_check() is defined in
>> the next patch.
>>
>
> Ok.. Hooks come after implementation...
> Will swap.
>
Hi Serge.
Actually Kconfig option is defined in following implementation patch,
so CONFIG_INTEGRITY_MODULES is not defined and it will compile inline
empty function.
Why is it not bisect-safe?
- Dmitry
> Thanks.
>
>>> +#else
>>> +static inline int integrity_module_check(const void *buf, unsigned long len)
>>> +{
>>> + return 0;
>>> +}
>>> +#endif
>>> +
>>> #endif /* _LINUX_INTEGRITY_H */
>>> diff --git a/kernel/module.c b/kernel/module.c
>>> index 4edbd9c..791da47 100644
>>> --- a/kernel/module.c
>>> +++ b/kernel/module.c
>>> @@ -58,6 +58,7 @@
>>> #include <linux/jump_label.h>
>>> #include <linux/pfn.h>
>>> #include <linux/bsearch.h>
>>> +#include <linux/integrity.h>
>>>
>>> #define CREATE_TRACE_POINTS
>>> #include <trace/events/module.h>
>>> @@ -2437,6 +2438,14 @@ static int copy_and_check(struct load_info *info,
>>>
>>> info->hdr = hdr;
>>> info->len = len;
>>> +
>>> + err = integrity_module_check(hdr, len);
>>> + if (err < 0)
>>> + goto free_hdr;
>>> +
>>> + /* cut signature tail */
>>> + info->len = err;
>>> +
>>> return 0;
>>>
>>> free_hdr:
>>> --
>>> 1.7.9.5
>>>
>>> --
>>> 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
On Thu, Aug 16, 2012 at 10:28 PM, Mimi Zohar <[email protected]> wrote:
> On Thu, 2012-08-16 at 14:37 -0400, Josh Boyer wrote:
>> On Wed, Aug 15, 2012 at 2:43 PM, Dmitry Kasatkin
>> <[email protected]> wrote:
>> > From: Mimi Zohar <[email protected]>
>> >
>> > Create and initialize a keyring with the builtin public key. This could
>> > be an ephemeral key, created and destroyed during module install for
>> > custom built kernels, or a key used to label the entire filesystem for
>> > EVM/IMA-appraisal. Uses .incbin based on David Howell's post.
>> >
>> > Load the builtin public key on the specified keyring, creating the
>> > keyring if it doesn't already exist.
>> >
>> > Signed-off-by: Mimi Zohar <[email protected]>
>> > Signed-off-by: Dmitry Kasatkin <[email protected]>
>> > ---
>> > security/integrity/Kconfig | 10 ++++
>> > security/integrity/Makefile | 17 +++++++
>> > security/integrity/digsig_pubkey.c | 96 ++++++++++++++++++++++++++++++++++++
>> > security/integrity/integrity.h | 10 ++++
>> > 4 files changed, 133 insertions(+)
>> > create mode 100644 security/integrity/digsig_pubkey.c
>> >
>> > diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
>> > index 5bd1cc1..f789018 100644
>> > --- a/security/integrity/Kconfig
>> > +++ b/security/integrity/Kconfig
>> > @@ -17,5 +17,15 @@ config INTEGRITY_SIGNATURE
>> > This is useful for evm and module keyrings, when keys are
>> > usually only added from initramfs.
>> >
>> > +config INTEGRITY_PUBKEY
>> > + boolean "Create a keyring and initialize with builtin public key"
>> > + depends on INTEGRITY_SIGNATURE
>> > + default n
>> > + help
>> > + Create and initialize a keyring with the builtin public key. This could
>> > + be an ephemeral key, created and destroyed during module install for
>> > + custom built kernels, or a key used to label the entire filesystem for
>> > + EVM/IMA-appraisal.
>> > +
>>
>> It could also be a key that is reused explicitly for signing kernels
>> and kernel modules but has nothing to do with EVM/IMA filesystem
>> labels, right? E.g. a distro key. I think the commit log and help
>> text is a bit too restrictive here.
>
> Definitely! The cover letter included the distro scenario, but I missed
> updating it here.
>
>> > +int integrity_init_keyring(const unsigned int id)
>> > +{
>> > + const struct cred *cred = current_cred();
>> > + struct user_struct *user = cred->user;
>> > + struct key *new_keyring, *key;
>> > + u8 digest[SHA1_DIGEST_SIZE];
>> > + char keyid[20];
>> > + int ret, pubkey_size = pubkey_end - pubkey;
>> > +
>> > + if (pubkey_size == 0) {
>> > + pr_info("pubkey is missing, skipping...\n");
>> > + return 0;
>> > + }
>> > +
>> > + new_keyring = keyring_alloc(keyring_name[id], user->uid, (gid_t) -1,
>> > + cred, KEY_ALLOC_NOT_IN_QUOTA,
>> > + user->uid_keyring);
>> > + if (IS_ERR(new_keyring)) {
>> > + ret = PTR_ERR(new_keyring);
>> > + goto out;
>> > + }
>> > +
>> > + ret = integrity_calc_digest("sha1", pubkey, pubkey_size, digest);
>> > + if (ret < 0)
>> > + goto out;
>> > +
>> > + sprintf(keyid, "%llX", __be64_to_cpup((uint64_t *)(digest+12)));
>>
>> keyid is only 20 bytes. Is there a guarantee somewhere that restricts
>> the digest+12 value to be 20 bytes or less and NUL termintated? If
>> not, should you use snprintf?
>
> Correct, and SHA1 shouldn't be hardcoded either, but configurable.
>
>>
>> > +
>> > + key = key_alloc(&key_type_user, keyid, 0, 0, current_cred(),
>> > + (KEY_POS_ALL & ~KEY_POS_SETATTR) |
>> > + KEY_USR_VIEW | KEY_USR_SEARCH | KEY_USR_READ,
>> > + KEY_ALLOC_NOT_IN_QUOTA);
>> > + if (IS_ERR(key)) {
>> > + ret = PTR_ERR(key);
>> > + goto out;
>>
>> You leak new_keyring if here, right?
>
> thanks
>
Actually we are not leaking.
new_keyring is instantiated and linked to the uid keyring..
Another question is, do we want to leave keyring there for "may be
later" use, or just destroy...
>> > + }
>> > +
>> > + ret = key_instantiate_and_link(key, pubkey, pubkey_end - pubkey,
>> > + new_keyring, NULL);
>> > +out:
>> > + pr_info("integrity: loaded public key %s on %s %s\n", keyid,
>> > + keyring_name[id], !ret ? "succeeded" : "failed");
>> > + return ret;
>> > +}
>>
>> josh
>
> Mimi
>
On Thu, Aug 16, 2012 at 8:53 PM, Mimi Zohar <[email protected]> wrote:
>> >> The reason for "signed_modules_install" is to limit existence of private key.
>> >> Private key is generate just before install, modules installed and
>> >> signed, then key is destroyed.
>> >> So existence of private key is limited to "time make
>> >> signed_modules_install" execution time.
>> >>
>> >> We had a debate about it, and strong message was that we might want to
>> >> do it like that...
>> >
>> > I guess I personally don't see the need to destroy they key so quickly.
>> > Is the concern that an intruder might grab the key and use it to sign a
>> > module that the developer would then later on somehow load? Or
>> > similarly someone would grab a temporary key from a distro build
>> > machine? That limits the attack surface, sure, but I'm not sure it's
>> > really reasonable.
>> >
>> > For a developer that isn't distributing kernels to others, it's just
>> > adding more time to the compile (which I know can be disabled, but
>> > still). For a distribution, most of them are either using a private
>> > key already or they have a buildsystem that destroys a buildroot after
>> > a build completes. The key is already going to be destroyed in that
>> > scenario.
>> >
>> > josh
>>
>> Well... Will not argue here. I had similar opinion as well.
>>
>> Mimi strongly wanted really to "reduce" the existence time of the key...
>
> The options are creating the key during 'make' or 'make
> modules_install'. If you create the key during 'make', then you have no
> way of knowing whether or not it is a persistent or ephemeral key, and
> whether it should be deleted after signing the modules.
The buildsystem doesn't really _need_ to know that though.
> You could create a persistent key using 'make genkey', before 'make',
> and never delete the private key. Then there wouldn't be any
> overhead. :) If 'CONFIG_INTEGRITY_MODULE' is configured, 'make
> modules_install' would use the existing key.
>
> 'make signed_modules_install' would be for creating and using ephemeral
> keys.
>
> What do you think?
I don't see a need for the kernel make system to ever delete a key.
If one doesn't exist, it should create one if the config options are
set and leave it alone entirely after that. If one exists already,
then it should leave it alone as it already does.
If you really want to enforce ephemeral keys in the make system, then
doing it via 'make clean' or 'make distclean' would make more sense to
me. But I personally think key management is something the developers
or distros should be handling on their own. Creating a key for them is
a convenience so it's worthwhile, but removing it should be done by
them.
josh
On Fri, 2012-08-17 at 07:40 -0400, Josh Boyer wrote:
> On Thu, Aug 16, 2012 at 8:53 PM, Mimi Zohar <[email protected]> wrote:
> >> >> The reason for "signed_modules_install" is to limit existence of private key.
> >> >> Private key is generate just before install, modules installed and
> >> >> signed, then key is destroyed.
> >> >> So existence of private key is limited to "time make
> >> >> signed_modules_install" execution time.
> >> >>
> >> >> We had a debate about it, and strong message was that we might want to
> >> >> do it like that...
> >> >
> >> > I guess I personally don't see the need to destroy they key so quickly.
> >> > Is the concern that an intruder might grab the key and use it to sign a
> >> > module that the developer would then later on somehow load? Or
> >> > similarly someone would grab a temporary key from a distro build
> >> > machine? That limits the attack surface, sure, but I'm not sure it's
> >> > really reasonable.
> >> >
> >> > For a developer that isn't distributing kernels to others, it's just
> >> > adding more time to the compile (which I know can be disabled, but
> >> > still). For a distribution, most of them are either using a private
> >> > key already or they have a buildsystem that destroys a buildroot after
> >> > a build completes. The key is already going to be destroyed in that
> >> > scenario.
> >> >
> >> > josh
> >>
> >> Well... Will not argue here. I had similar opinion as well.
> >>
> >> Mimi strongly wanted really to "reduce" the existence time of the key...
> >
> > The options are creating the key during 'make' or 'make
> > modules_install'. If you create the key during 'make', then you have no
> > way of knowing whether or not it is a persistent or ephemeral key, and
> > whether it should be deleted after signing the modules.
>
> The buildsystem doesn't really _need_ to know that though.
>
> > You could create a persistent key using 'make genkey', before 'make',
> > and never delete the private key. Then there wouldn't be any
> > overhead. :) If 'CONFIG_INTEGRITY_MODULE' is configured, 'make
> > modules_install' would use the existing key.
> >
> > 'make signed_modules_install' would be for creating and using ephemeral
> > keys.
> >
> > What do you think?
>
> I don't see a need for the kernel make system to ever delete a key.
> If one doesn't exist, it should create one if the config options are
> set and leave it alone entirely after that. If one exists already,
> then it should leave it alone as it already does.
Ok. Other than generating a key the first time, the normal development
build process now uses the same key, never requiring the developer to do
anything additional. The developer controls the frequency the keys are
created/deleted. I wonder how often that will be ...
> If you really want to enforce ephemeral keys in the make system, then
> doing it via 'make clean' or 'make distclean' would make more sense to
> me. But I personally think key management is something the developers
> or distros should be handling on their own. Creating a key for them is
> a convenience so it's worthwhile, but removing it should be done by
> them.
Sorry, I disagree. Without the signed_modules_install target, the
developer would need to do each step manually - create new key, sign
modules, remove private key, and embed the new public key in the
bzImage.
I still think the signed_modules_install script, renamed to something
like ephemeral_signed_modules_install, is worthwhile and becomes a
convience tool for the developer, wanting to use ephemeral keys. The
private key, in Dmitry's updated patches soon to be posted, will be
password protected with a random number, that is only accessible to the
current shell.
thanks,
Mimi
On Fri, Aug 17, 2012 at 1:08 PM, Mimi Zohar <[email protected]> wrote:
>> I don't see a need for the kernel make system to ever delete a key.
>> If one doesn't exist, it should create one if the config options are
>> set and leave it alone entirely after that. If one exists already,
>> then it should leave it alone as it already does.
>
> Ok. Other than generating a key the first time, the normal development
> build process now uses the same key, never requiring the developer to do
> anything additional. The developer controls the frequency the keys are
> created/deleted. I wonder how often that will be ...
For a developer, probably not often. Though in reality, the usage of
this by a single developer seems fair small. A much wider, already
existing usage is distribution kernel signing.
For a distribution kernel there are normally two cases:
1) Existing permanent company/distro key. This is already handled.
2) per-kernel-build key (equivalent to your ephemeral key). This would
be handled fine if the key was generated if it didn't already exist and
left alone. The distro build system would remove it when it cleaned up
the buildroot.
>> If you really want to enforce ephemeral keys in the make system, then
>> doing it via 'make clean' or 'make distclean' would make more sense to
>> me. But I personally think key management is something the developers
>> or distros should be handling on their own. Creating a key for them is
>> a convenience so it's worthwhile, but removing it should be done by
>> them.
>
> Sorry, I disagree. Without the signed_modules_install target, the
> developer would need to do each step manually - create new key, sign
> modules, remove private key, and embed the new public key in the
> bzImage.
Not if the buildsystem just made the key for them at 'make' time. I'm
still missing why it isn't done until 'modules_install' time. Seems
pretty sub-optimal.
> I still think the signed_modules_install script, renamed to something
> like ephemeral_signed_modules_install, is worthwhile and becomes a
> convience tool for the developer, wanting to use ephemeral keys. The
> private key, in Dmitry's updated patches soon to be posted, will be
> password protected with a random number, that is only accessible to the
> current shell.
I think the existence of an additional make target for signed modules
is really confusing. Particularly when you consider the target still
exists even if the kernel isn't setup to work with signed modules. If
the config options are set, just have 'make modules_install' do it and
create a key if one doesn't exist (or better yet, have 'make' do it).
Also... password protecting the key that only responds to the current
shell really sounds like a show-stopper for this being used by distros.
There is no way a distro is going to be able to type a password in
during a kernel build. It completely removes the usability of distros
that want to use a per-kernel build ephemeral key. If you're going to
do this, please wrap it in a kconfig option so the second distro case
I mentio above is still possible.
josh
On Fri, Aug 17, 2012 at 1:44 PM, Josh Boyer <[email protected]> wrote:
>> I still think the signed_modules_install script, renamed to something
>> like ephemeral_signed_modules_install, is worthwhile and becomes a
>> convience tool for the developer, wanting to use ephemeral keys. The
>> private key, in Dmitry's updated patches soon to be posted, will be
>> password protected with a random number, that is only accessible to the
>> current shell.
>
> I think the existence of an additional make target for signed modules
> is really confusing. Particularly when you consider the target still
> exists even if the kernel isn't setup to work with signed modules. If
> the config options are set, just have 'make modules_install' do it and
> create a key if one doesn't exist (or better yet, have 'make' do it).
>
> Also... password protecting the key that only responds to the current
> shell really sounds like a show-stopper for this being used by distros.
> There is no way a distro is going to be able to type a password in
> during a kernel build. It completely removes the usability of distros
> that want to use a per-kernel build ephemeral key. If you're going to
> do this, please wrap it in a kconfig option so the second distro case
> I mentio above is still possible.
Here's a suggestion that might help the discussion. In the next
revision of the patch set, include a document in Documentation/ that
covers the module signing design, the purposes it's intended to fit,
and a high level description of the various module loading scenarios
(signed, unsigned, signed with a key not in the keyring, etc). That
way we can at least see at a higher level what the thinking behind the
implemenation is. I think some of our back and forth (while good!) is
because we see signed modules being used for different purposes.
josh
Mimi Zohar <[email protected]> writes:
> On Thu, 2012-08-16 at 15:59 -0400, Josh Boyer wrote:
>> On Thu, Aug 16, 2012 at 3:45 PM, Mimi Zohar <[email protected]> wrote:
>> > On Thu, 2012-08-16 at 15:13 -0400, Josh Boyer wrote:
>> >> On Thu, Aug 16, 2012 at 3:08 PM, Mimi Zohar <[email protected]> wrote:
>> >> >> > +#include "internal.h"
>> >> >> > +static int __init init_root_keyring(void)
>> >> >> > +{
>> >> >> > + return install_user_keyrings();
>> >> >> > +}
>> >> >> > +
>> >> >> > +late_initcall(init_root_keyring);
>> >> >> > --
>> >> >>
>> >> >> Why is this in an entirely new file instead of just being added to
>> >> >> process_keys.c ?
>> >> >>
>> >> >> josh
>> >> >
>> >> > Only when "CONFIG_INTEGRITY_SIGNATURE" is selected, does this get built.
>> >>
>> >> Yes, I noticed that. It doesn't explain why it's in its own file. You
>> >> could accomplish the same thing by wrapping the function and initcall
>> >> in #ifdef CONFIG_INTEGRITY_SIGNATURE in process_keys.c.
>> >
>> > I was under the impression using 'ifdefs' in 'C' code was frowned upon
>> > (Documentation/SubmittingPatches section 2.2). This would be an
>> > exception?
>>
>> If it makes a big ugly mess it's frowned upon. But if you're adding 7
>> lines of code in a new file that will almost certainly never get more
>> code added to it, I'm not sure. IMHO, it can go into an existing file.
>> Others might disagree. Isn't Linux development fun?!
>
> This is just a case where if I had 'ifdef's in 'C' code, I'm sure
> someone would have complained. :)
Why does the code need to be dependent on security modules at all. The
code should work regardless either way.
Eric
On Fri, 2012-08-17 at 13:44 -0400, Josh Boyer wrote:
> On Fri, Aug 17, 2012 at 1:08 PM, Mimi Zohar <[email protected]> wrote:
> >> I don't see a need for the kernel make system to ever delete a key.
> >> If one doesn't exist, it should create one if the config options are
> >> set and leave it alone entirely after that. If one exists already,
> >> then it should leave it alone as it already does.
> >
> > Ok. Other than generating a key the first time, the normal development
> > build process now uses the same key, never requiring the developer to do
> > anything additional. The developer controls the frequency the keys are
> > created/deleted. I wonder how often that will be ...
>
> For a developer, probably not often. Though in reality, the usage of
> this by a single developer seems fair small. A much wider, already
> existing usage is distribution kernel signing.
This seems to be the real issue. Do developers need to sign their own
builds? Nobody is forcing the developer to enable signed modules. If,
however, the developer decides to enforce signed modules, leaving the
private key lying around kind of defeats the purpose.
> For a distribution kernel there are normally two cases:
>
> 1) Existing permanent company/distro key. This is already handled.
> 2) per-kernel-build key (equivalent to your ephemeral key). This would
> be handled fine if the key was generated if it didn't already exist and
> left alone. The distro build system would remove it when it cleaned up
> the buildroot.
Ok, I think we agree here, with the normal 'make', 'make modules_install
install', using an existing key or creating a new key if needed.
> >> If you really want to enforce ephemeral keys in the make system, then
> >> doing it via 'make clean' or 'make distclean' would make more sense to
> >> me. But I personally think key management is something the developers
> >> or distros should be handling on their own. Creating a key for them is
> >> a convenience so it's worthwhile, but removing it should be done by
> >> them.
> >
> > Sorry, I disagree. Without the signed_modules_install target, the
> > developer would need to do each step manually - create new key, sign
> > modules, remove private key, and embed the new public key in the
> > bzImage.
>
> Not if the buildsystem just made the key for them at 'make' time. I'm
> still missing why it isn't done until 'modules_install' time. Seems
> pretty sub-optimal.
Using a private key that has been lying around for an unknown period of
time, kind of defeats the purpose.
> > I still think the signed_modules_install script, renamed to something
> > like ephemeral_signed_modules_install, is worthwhile and becomes a
> > convience tool for the developer, wanting to use ephemeral keys. The
> > private key, in Dmitry's updated patches soon to be posted, will be
> > password protected with a random number, that is only accessible to the
> > current shell.
>
> I think the existence of an additional make target for signed modules
> is really confusing. Particularly when you consider the target still
> exists even if the kernel isn't setup to work with signed modules.
Ok, I see your concern.
> If
> the config options are set, just have 'make modules_install' do it and
> create a key if one doesn't exist (or better yet, have 'make' do it).
If creating the key is deferred to 'make modules_install', for the
reason given above or any other reason, the public key wasn't available
at the time the bzImage was created. 'make modules_install' would now
need to rebuild the bzImage.
thanks,
Mimi
On Wed, 15 Aug 2012 21:43:06 +0300, Dmitry Kasatkin <[email protected]> wrote:
> + } else {
> + struct {
> + struct shash_desc shash;
> + char ctx[crypto_shash_descsize(tfm)];
> + } desc;
Linus had a rant a while ago about using variable-sized stack vars in
the kernel (can't find a reference right now, sorry).
The problem is that either you know there's a limit to
crypto_shash_descsize(), in which case you can just use this here, or
you don't know, in which case, this risks a stack oveflow.
Cheers,
Rusty.
On Sun, Aug 19, 2012 at 9:05 PM, Mimi Zohar <[email protected]> wrote:
> On Fri, 2012-08-17 at 13:44 -0400, Josh Boyer wrote:
>> On Fri, Aug 17, 2012 at 1:08 PM, Mimi Zohar <[email protected]> wrote:
>> >> I don't see a need for the kernel make system to ever delete a key.
>> >> If one doesn't exist, it should create one if the config options are
>> >> set and leave it alone entirely after that. If one exists already,
>> >> then it should leave it alone as it already does.
>> >
>> > Ok. Other than generating a key the first time, the normal development
>> > build process now uses the same key, never requiring the developer to do
>> > anything additional. The developer controls the frequency the keys are
>> > created/deleted. I wonder how often that will be ...
>>
>> For a developer, probably not often. Though in reality, the usage of
>> this by a single developer seems fair small. A much wider, already
>> existing usage is distribution kernel signing.
>
> This seems to be the real issue. Do developers need to sign their own
> builds? Nobody is forcing the developer to enable signed modules. If,
> however, the developer decides to enforce signed modules, leaving the
> private key lying around kind of defeats the purpose.
Right. The issue is that you're making the kernel buildsystem do key
management policy instead of trusting the developers using the keys
mechanism to know what they're doing.
>> For a distribution kernel there are normally two cases:
>>
>> 1) Existing permanent company/distro key. This is already handled.
>> 2) per-kernel-build key (equivalent to your ephemeral key). This would
>> be handled fine if the key was generated if it didn't already exist and
>> left alone. The distro build system would remove it when it cleaned up
>> the buildroot.
>
> Ok, I think we agree here, with the normal 'make', 'make modules_install
> install', using an existing key or creating a new key if needed.
>
>> >> If you really want to enforce ephemeral keys in the make system, then
>> >> doing it via 'make clean' or 'make distclean' would make more sense to
>> >> me. But I personally think key management is something the developers
>> >> or distros should be handling on their own. Creating a key for them is
>> >> a convenience so it's worthwhile, but removing it should be done by
>> >> them.
>> >
>> > Sorry, I disagree. Without the signed_modules_install target, the
>> > developer would need to do each step manually - create new key, sign
>> > modules, remove private key, and embed the new public key in the
>> > bzImage.
>>
>> Not if the buildsystem just made the key for them at 'make' time. I'm
>> still missing why it isn't done until 'modules_install' time. Seems
>> pretty sub-optimal.
>
> Using a private key that has been lying around for an unknown period of
> time, kind of defeats the purpose.
Why is that? Again, you're enforcing policy instead of leaving it up
to the developers. In the existing key case, you have no idea how long
that key has been sitting around either, so I really see a disconnect
between "existing company/private key" and "no key, create one for them
during build time" cases.
>> > I still think the signed_modules_install script, renamed to something
>> > like ephemeral_signed_modules_install, is worthwhile and becomes a
>> > convience tool for the developer, wanting to use ephemeral keys. The
>> > private key, in Dmitry's updated patches soon to be posted, will be
>> > password protected with a random number, that is only accessible to the
>> > current shell.
>>
>> I think the existence of an additional make target for signed modules
>> is really confusing. Particularly when you consider the target still
>> exists even if the kernel isn't setup to work with signed modules.
>
> Ok, I see your concern.
>
>> If
>> the config options are set, just have 'make modules_install' do it and
>> create a key if one doesn't exist (or better yet, have 'make' do it).
>
> If creating the key is deferred to 'make modules_install', for the
> reason given above or any other reason, the public key wasn't available
> at the time the bzImage was created. 'make modules_install' would now
> need to rebuild the bzImage.
Right, which is an utter pain and backwards. That's why I suggested it
be done at 'make' time.
josh
On Mon, 2012-08-20 at 08:32 -0400, Josh Boyer wrote:
> On Sun, Aug 19, 2012 at 9:05 PM, Mimi Zohar <[email protected]> wrote:
> > On Fri, 2012-08-17 at 13:44 -0400, Josh Boyer wrote:
> >> On Fri, Aug 17, 2012 at 1:08 PM, Mimi Zohar <[email protected]> wrote:
> >> >> I don't see a need for the kernel make system to ever delete a key.
> >> >> If one doesn't exist, it should create one if the config options are
> >> >> set and leave it alone entirely after that. If one exists already,
> >> >> then it should leave it alone as it already does.
> >> >
> >> > Ok. Other than generating a key the first time, the normal development
> >> > build process now uses the same key, never requiring the developer to do
> >> > anything additional. The developer controls the frequency the keys are
> >> > created/deleted. I wonder how often that will be ...
> >>
> >> For a developer, probably not often. Though in reality, the usage of
> >> this by a single developer seems fair small. A much wider, already
> >> existing usage is distribution kernel signing.
> >
> > This seems to be the real issue. Do developers need to sign their own
> > builds? Nobody is forcing the developer to enable signed modules. If,
> > however, the developer decides to enforce signed modules, leaving the
> > private key lying around kind of defeats the purpose.
>
> Right. The issue is that you're making the kernel buildsystem do key
> management policy instead of trusting the developers using the keys
> mechanism to know what they're doing.
>
> >> For a distribution kernel there are normally two cases:
> >>
> >> 1) Existing permanent company/distro key. This is already handled.
> >> 2) per-kernel-build key (equivalent to your ephemeral key). This would
> >> be handled fine if the key was generated if it didn't already exist and
> >> left alone. The distro build system would remove it when it cleaned up
> >> the buildroot.
> >
> > Ok, I think we agree here, with the normal 'make', 'make modules_install
> > install', using an existing key or creating a new key if needed.
> >
> >> >> If you really want to enforce ephemeral keys in the make system, then
> >> >> doing it via 'make clean' or 'make distclean' would make more sense to
> >> >> me. But I personally think key management is something the developers
> >> >> or distros should be handling on their own. Creating a key for them is
> >> >> a convenience so it's worthwhile, but removing it should be done by
> >> >> them.
> >> >
> >> > Sorry, I disagree. Without the signed_modules_install target, the
> >> > developer would need to do each step manually - create new key, sign
> >> > modules, remove private key, and embed the new public key in the
> >> > bzImage.
> >>
> >> Not if the buildsystem just made the key for them at 'make' time. I'm
> >> still missing why it isn't done until 'modules_install' time. Seems
> >> pretty sub-optimal.
> >
> > Using a private key that has been lying around for an unknown period of
> > time, kind of defeats the purpose.
>
> Why is that? Again, you're enforcing policy instead of leaving it up
> to the developers. In the existing key case, you have no idea how long
> that key has been sitting around either, so I really see a disconnect
> between "existing company/private key" and "no key, create one for them
> during build time" cases.
Either the company build process, creates and cleans up after the build,
removing the private key, as you described above, or the "private key"
is held securely using an existing signing process (or some combination
of the two).
In the "no key, create one for them during build time", there is no
process in place cleaning up, other than the suggested new make target.
> >> > I still think the signed_modules_install script, renamed to something
> >> > like ephemeral_signed_modules_install, is worthwhile and becomes a
> >> > convience tool for the developer, wanting to use ephemeral keys. The
> >> > private key, in Dmitry's updated patches soon to be posted, will be
> >> > password protected with a random number, that is only accessible to the
> >> > current shell.
> >>
> >> I think the existence of an additional make target for signed modules
> >> is really confusing. Particularly when you consider the target still
> >> exists even if the kernel isn't setup to work with signed modules.
> >
> > Ok, I see your concern.
> >
> >> If
> >> the config options are set, just have 'make modules_install' do it and
> >> create a key if one doesn't exist (or better yet, have 'make' do it).
> >
> > If creating the key is deferred to 'make modules_install', for the
> > reason given above or any other reason, the public key wasn't available
> > at the time the bzImage was created. 'make modules_install' would now
> > need to rebuild the bzImage.
>
> Right, which is an utter pain and backwards. That's why I suggested it
> be done at 'make' time.
Agreed, no doubt about it being a pain or backwards, but the solution
isn't to deny the problem, but to address it. I'm open to
suggestions. :)
Mimi
On Mon, Aug 20, 2012 at 9:13 AM, Mimi Zohar <[email protected]> wrote:
> On Mon, 2012-08-20 at 08:32 -0400, Josh Boyer wrote:
>> On Sun, Aug 19, 2012 at 9:05 PM, Mimi Zohar <[email protected]> wrote:
>> > On Fri, 2012-08-17 at 13:44 -0400, Josh Boyer wrote:
>> >> On Fri, Aug 17, 2012 at 1:08 PM, Mimi Zohar <[email protected]> wrote:
>> >> >> I don't see a need for the kernel make system to ever delete a key.
>> >> >> If one doesn't exist, it should create one if the config options are
>> >> >> set and leave it alone entirely after that. If one exists already,
>> >> >> then it should leave it alone as it already does.
>> >> >
>> >> > Ok. Other than generating a key the first time, the normal development
>> >> > build process now uses the same key, never requiring the developer to do
>> >> > anything additional. The developer controls the frequency the keys are
>> >> > created/deleted. I wonder how often that will be ...
>> >>
>> >> For a developer, probably not often. Though in reality, the usage of
>> >> this by a single developer seems fair small. A much wider, already
>> >> existing usage is distribution kernel signing.
>> >
>> > This seems to be the real issue. Do developers need to sign their own
>> > builds? Nobody is forcing the developer to enable signed modules. If,
>> > however, the developer decides to enforce signed modules, leaving the
>> > private key lying around kind of defeats the purpose.
>>
>> Right. The issue is that you're making the kernel buildsystem do key
>> management policy instead of trusting the developers using the keys
>> mechanism to know what they're doing.
>>
>> >> For a distribution kernel there are normally two cases:
>> >>
>> >> 1) Existing permanent company/distro key. This is already handled.
>> >> 2) per-kernel-build key (equivalent to your ephemeral key). This would
>> >> be handled fine if the key was generated if it didn't already exist and
>> >> left alone. The distro build system would remove it when it cleaned up
>> >> the buildroot.
>> >
>> > Ok, I think we agree here, with the normal 'make', 'make modules_install
>> > install', using an existing key or creating a new key if needed.
>> >
>> >> >> If you really want to enforce ephemeral keys in the make system, then
>> >> >> doing it via 'make clean' or 'make distclean' would make more sense to
>> >> >> me. But I personally think key management is something the developers
>> >> >> or distros should be handling on their own. Creating a key for them is
>> >> >> a convenience so it's worthwhile, but removing it should be done by
>> >> >> them.
>> >> >
>> >> > Sorry, I disagree. Without the signed_modules_install target, the
>> >> > developer would need to do each step manually - create new key, sign
>> >> > modules, remove private key, and embed the new public key in the
>> >> > bzImage.
>> >>
>> >> Not if the buildsystem just made the key for them at 'make' time. I'm
>> >> still missing why it isn't done until 'modules_install' time. Seems
>> >> pretty sub-optimal.
>> >
>> > Using a private key that has been lying around for an unknown period of
>> > time, kind of defeats the purpose.
>>
>> Why is that? Again, you're enforcing policy instead of leaving it up
>> to the developers. In the existing key case, you have no idea how long
>> that key has been sitting around either, so I really see a disconnect
>> between "existing company/private key" and "no key, create one for them
>> during build time" cases.
>
> Either the company build process, creates and cleans up after the build,
> removing the private key, as you described above, or the "private key"
> is held securely using an existing signing process (or some combination
> of the two).
>
> In the "no key, create one for them during build time", there is no
> process in place cleaning up, other than the suggested new make target.
You could have 'make clean/distclean' remove the key if you really
wanted.
>> >> If
>> >> the config options are set, just have 'make modules_install' do it and
>> >> create a key if one doesn't exist (or better yet, have 'make' do it).
>> >
>> > If creating the key is deferred to 'make modules_install', for the
>> > reason given above or any other reason, the public key wasn't available
>> > at the time the bzImage was created. 'make modules_install' would now
>> > need to rebuild the bzImage.
>>
>> Right, which is an utter pain and backwards. That's why I suggested it
>> be done at 'make' time.
>
> Agreed, no doubt about it being a pain or backwards, but the solution
> isn't to deny the problem, but to address it. I'm open to
> suggestions. :)
I've made multiple suggestions. I have no more. Either you agree with
them or your don't. Our fundamental disagreement is the management of
automatically created keys. You want them removed ASAP, I suggest that
people/companies/distributions using that mechanism should be aware of
what they're doing and be responsible for removing keys when they see
fit. Aside from making the behavior dependent on perhaps some kind of
"expert" config setting, I don't see a way past that difference.
josh
On Mon, Aug 20, 2012 at 5:59 AM, Rusty Russell <[email protected]> wrote:
> On Wed, 15 Aug 2012 21:43:06 +0300, Dmitry Kasatkin <[email protected]> wrote:
>> + } else {
>> + struct {
>> + struct shash_desc shash;
>> + char ctx[crypto_shash_descsize(tfm)];
>> + } desc;
>
> Linus had a rant a while ago about using variable-sized stack vars in
> the kernel (can't find a reference right now, sorry).
>
> The problem is that either you know there's a limit to
> crypto_shash_descsize(), in which case you can just use this here, or
> you don't know, in which case, this risks a stack oveflow.
>
> Cheers,
> Rusty.
Well... descriptor size is a algo specific but constant... just
sizeof(some struct).
So there is no possibility to force overflow unless someone implement
some algo driver which
uses too big descriptor size.
- Dmitry