linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH v5 0/1] Add dm verity root hash pkcs7 sig validation.
@ 2019-06-19 19:10 Jaskaran Khurana
  2019-06-19 19:10 ` [RFC PATCH v5 1/1] " Jaskaran Khurana
  2019-06-28  4:00 ` [RFC PATCH v5 0/1] " Eric Biggers
  0 siblings, 2 replies; 19+ messages in thread
From: Jaskaran Khurana @ 2019-06-19 19:10 UTC (permalink / raw)
  To: linux-security-module, linux-kernel, linux-integrity, linux-fsdevel
  Cc: agk, snitzer, dm-devel, jmorris, scottsh, ebiggers, mpatocka, gmazyland

This patch set adds in-kernel pkcs7 signature checking for the roothash of
the dm-verity hash tree.
The verification is to support cases where the roothash is not secured by
Trusted Boot, UEFI Secureboot or similar technologies.
One of the use cases for this is for dm-verity volumes mounted after boot,
the root hash provided during the creation of the dm-verity volume has to
be secure and thus in-kernel validation implemented here will be used
before we trust the root hash and allow the block device to be created.

Why we are doing validation in the Kernel?

The reason is to still be secure in cases where the attacker is able to
compromise the user mode application in which case the user mode validation
could not have been trusted.
The root hash signature validation in the kernel along with existing
dm-verity implementation gives a higher level of confidence in the
executable code or the protected data. Before allowing the creation of
the device mapper block device the kernel code will check that the detached
pkcs7 signature passed to it validates the roothash and the signature is
trusted by builtin keys set at kernel creation. The kernel should be
secured using Verified boot, UEFI Secure Boot or similar technologies so we
can trust it.

What about attacker mounting non dm-verity volumes to run executable
code?

This verification can be used to have a security architecture where a LSM
can enforce this verification for all the volumes and by doing this it can
ensure that all executable code runs from signed and trusted dm-verity
volumes.

Further patches will be posted that build on this and enforce this
verification based on policy for all the volumes on the system.

How are these changes tested?

To generate and sign the roothash, dump the roothash returned by 
veritysetup format in a text file, say roothash.txt and then sign using
the openssl command:

openssl smime -sign -nocerts -noattr -binary -in <roothash.txt> 
-inkey <keyfile> -signer <certfile> -outform der -out <out_sigfile>

To pass the roothash signature to dm-verity, veritysetup part of cryptsetup
library was modified to take a optional root-hash-sig parameter.

Commandline used to test the changes:

Use the signature file from above step as a parameter to veritysetup.

veritysetup open  <data_device> <name> <hash_device> <root_hash>
 --root-hash-sig=<root_hash_pkcs7_detached_sig>

The changes for veritysetup are in a topic branch for now at:
https://github.com/jaskarankhurana/veritysetup/tree/veritysetup_add_sig

Set kernel commandline dm_verity.verify_sig=1 or 2 for check/force
dm-verity to do root hash signature validation.

Changelog:

v5 (since previous):
  - Code review feedback given by Milan Broz.
  - Remove the Kconfig for root hash verification and instead add a
    commandline parameter(dm_verity.verify_sig) that determines whether to
    check or enforce root hash signature validation.
  - Fixed a small issue when dm-verity was built sepaerately as a module.
  - Added the openssl commandline that can be used to sign the roothash
    in the cover letter.

v4:
  - Code review feedback given by Milan Broz.
  - Add documentation about the root hash signature parameter.
  - Bump up the dm-verity target version.
  - Provided way to sign and test with veritysetup in cover letter.

v3:
  - Code review feedback given by Sasha Levin.
  - Removed EXPORT_SYMBOL_GPL since this was not required.
  - Removed "This file is released under the GPLv2" since we have SPDX
    identifier.
  - Inside verity_verify_root_hash changed EINVAL to ENOKEY when the key
    descriptor is not specified but due to force option being set it is
    expected.
  - Moved CONFIG check to inside verity_verify_get_sig_from_key.
     (Did not move the sig_opts_cleanup to inside verity_dtr as the
     sig_opts do not need to be allocated for the entire duration the block
     device is active unlike the verity structure, note verity_dtr is
     called      only if verity_ctr fails or after the lifetime of the
     block device.)

v2:
  - Code review feedback to pass the signature binary blob as a key that
    can be looked up in the kernel and be used to verify the roothash.
    [Suggested by Milan Broz]
  - Made the code related change suggested in review of v1.
    [Suggested by Balbir Singh]

v1:
  - Add kconfigs to control dm-verity root has signature verification and
    use the signature if specified to verify the root hash.


Jaskaran Khurana (1):
  Adds in-kernel pkcs7 sig check dmverity roothash

 Documentation/device-mapper/verity.txt |   7 ++
 drivers/md/Kconfig                     |   1 +
 drivers/md/Makefile                    |   2 +-
 drivers/md/dm-verity-target.c          |  36 ++++++-
 drivers/md/dm-verity-verify-sig.c      | 139 +++++++++++++++++++++++++
 drivers/md/dm-verity-verify-sig.h      |  37 +++++++
 6 files changed, 216 insertions(+), 6 deletions(-)
 create mode 100644 drivers/md/dm-verity-verify-sig.c
 create mode 100644 drivers/md/dm-verity-verify-sig.h

-- 
2.17.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [RFC PATCH v5 1/1] Add dm verity root hash pkcs7 sig validation.
  2019-06-19 19:10 [RFC PATCH v5 0/1] Add dm verity root hash pkcs7 sig validation Jaskaran Khurana
@ 2019-06-19 19:10 ` Jaskaran Khurana
  2019-06-25 18:20   ` Mike Snitzer
                     ` (2 more replies)
  2019-06-28  4:00 ` [RFC PATCH v5 0/1] " Eric Biggers
  1 sibling, 3 replies; 19+ messages in thread
From: Jaskaran Khurana @ 2019-06-19 19:10 UTC (permalink / raw)
  To: linux-security-module, linux-kernel, linux-integrity, linux-fsdevel
  Cc: agk, snitzer, dm-devel, jmorris, scottsh, ebiggers, mpatocka, gmazyland

The verification is to support cases where the roothash is not secured by
Trusted Boot, UEFI Secureboot or similar technologies.
One of the use cases for this is for dm-verity volumes mounted after boot,
the root hash provided during the creation of the dm-verity volume has to
be secure and thus in-kernel validation implemented here will be used
before we trust the root hash and allow the block device to be created.

The signature being provided for verification must verify the root hash and
must be trusted by the builtin keyring for verification to succeed.

The hash is added as a key of type "user" and the description is passed to 
the kernel so it can look it up and use it for verification.

Kernel commandline parameter will indicate whether to check (only if 
specified) or force (for all dm verity volumes) roothash signature 
verification.

Kernel commandline: dm_verity.verify_sig=1 or 2 for check/force root hash
signature validation respectively.

Signed-off-by: Jaskaran Khurana <jaskarankhurana@linux.microsoft.com>
---
 Documentation/device-mapper/verity.txt |   7 ++
 drivers/md/Kconfig                     |   1 +
 drivers/md/Makefile                    |   2 +-
 drivers/md/dm-verity-target.c          |  36 ++++++-
 drivers/md/dm-verity-verify-sig.c      | 139 +++++++++++++++++++++++++
 drivers/md/dm-verity-verify-sig.h      |  37 +++++++
 6 files changed, 216 insertions(+), 6 deletions(-)
 create mode 100644 drivers/md/dm-verity-verify-sig.c
 create mode 100644 drivers/md/dm-verity-verify-sig.h

diff --git a/Documentation/device-mapper/verity.txt b/Documentation/device-mapper/verity.txt
index b3d2e4a42255..df7ef1d553cc 100644
--- a/Documentation/device-mapper/verity.txt
+++ b/Documentation/device-mapper/verity.txt
@@ -121,6 +121,13 @@ check_at_most_once
     blocks, and a hash block will not be verified any more after all the data
     blocks it covers have been verified anyway.
 
+root_hash_sig_key_desc <key_description>
+    This is the description of the USER_KEY that the kernel will lookup to get
+    the pkcs7 signature of the roothash. The pkcs7 signature is used to validate
+    the root hash during the creation of the device mapper block device.
+    Verification of roothash depends on the config DM_VERITY_VERIFY_ROOTHASH_SIG
+    being set in the kernel.
+
 Theory of operation
 ===================
 
diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index db269a348b20..2d658a3512cb 100644
--- a/drivers/md/Kconfig
+++ b/drivers/md/Kconfig
@@ -475,6 +475,7 @@ config DM_VERITY
 	select CRYPTO
 	select CRYPTO_HASH
 	select DM_BUFIO
+	select SYSTEM_DATA_VERIFICATION
 	---help---
 	  This device-mapper target creates a read-only device that
 	  transparently validates the data on one underlying device against
diff --git a/drivers/md/Makefile b/drivers/md/Makefile
index be7a6eb92abc..3b47b256b15e 100644
--- a/drivers/md/Makefile
+++ b/drivers/md/Makefile
@@ -18,7 +18,7 @@ dm-cache-y	+= dm-cache-target.o dm-cache-metadata.o dm-cache-policy.o \
 		    dm-cache-background-tracker.o
 dm-cache-smq-y   += dm-cache-policy-smq.o
 dm-era-y	+= dm-era-target.o
-dm-verity-y	+= dm-verity-target.o
+dm-verity-y	+= dm-verity-target.o dm-verity-verify-sig.o
 md-mod-y	+= md.o md-bitmap.o
 raid456-y	+= raid5.o raid5-cache.o raid5-ppl.o
 dm-zoned-y	+= dm-zoned-target.o dm-zoned-metadata.o dm-zoned-reclaim.o
diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index f4c31ffaa88e..adf7f376be7d 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -16,7 +16,7 @@
 
 #include "dm-verity.h"
 #include "dm-verity-fec.h"
-
+#include "dm-verity-verify-sig.h"
 #include <linux/module.h>
 #include <linux/reboot.h>
 
@@ -34,7 +34,8 @@
 #define DM_VERITY_OPT_IGN_ZEROES	"ignore_zero_blocks"
 #define DM_VERITY_OPT_AT_MOST_ONCE	"check_at_most_once"
 
-#define DM_VERITY_OPTS_MAX		(2 + DM_VERITY_OPTS_FEC)
+#define DM_VERITY_OPTS_MAX		(2 + DM_VERITY_OPTS_FEC + \
+					 DM_VERITY_ROOT_HASH_VERIFICATION_OPTS)
 
 static unsigned dm_verity_prefetch_cluster = DM_VERITY_DEFAULT_PREFETCH_SIZE;
 
@@ -855,7 +856,8 @@ static int verity_alloc_zero_digest(struct dm_verity *v)
 	return r;
 }
 
-static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
+static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
+				 struct dm_verity_sig_opts *verify_args)
 {
 	int r;
 	unsigned argc;
@@ -904,6 +906,14 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
 			if (r)
 				return r;
 			continue;
+		} else if (verity_verify_is_sig_opt_arg(arg_name)) {
+			r = verity_verify_sig_parse_opt_args(as, v,
+							     verify_args,
+							     &argc, arg_name);
+			if (r)
+				return r;
+			continue;
+
 		}
 
 		ti->error = "Unrecognized verity feature request";
@@ -930,6 +940,7 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
 static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 {
 	struct dm_verity *v;
+	struct dm_verity_sig_opts verify_args = {0};
 	struct dm_arg_set as;
 	unsigned int num;
 	unsigned long long num_ll;
@@ -937,6 +948,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	int i;
 	sector_t hash_position;
 	char dummy;
+	char *root_hash_digest_to_validate;
 
 	v = kzalloc(sizeof(struct dm_verity), GFP_KERNEL);
 	if (!v) {
@@ -1070,6 +1082,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 		r = -EINVAL;
 		goto bad;
 	}
+	root_hash_digest_to_validate = argv[8];
 
 	if (strcmp(argv[9], "-")) {
 		v->salt_size = strlen(argv[9]) / 2;
@@ -1095,11 +1108,20 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 		as.argc = argc;
 		as.argv = argv;
 
-		r = verity_parse_opt_args(&as, v);
+		r = verity_parse_opt_args(&as, v, &verify_args);
 		if (r < 0)
 			goto bad;
 	}
 
+	/* Root hash signature is  a optional parameter*/
+	r = verity_verify_root_hash(root_hash_digest_to_validate,
+				    strlen(root_hash_digest_to_validate),
+				    verify_args.sig,
+				    verify_args.sig_size);
+	if (r < 0) {
+		ti->error = "Root hash verification failed";
+		goto bad;
+	}
 	v->hash_per_block_bits =
 		__fls((1 << v->hash_dev_block_bits) / v->digest_size);
 
@@ -1165,9 +1187,13 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	ti->per_io_data_size = roundup(ti->per_io_data_size,
 				       __alignof__(struct dm_verity_io));
 
+	verity_verify_sig_opts_cleanup(&verify_args);
+
 	return 0;
 
 bad:
+
+	verity_verify_sig_opts_cleanup(&verify_args);
 	verity_dtr(ti);
 
 	return r;
@@ -1175,7 +1201,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 
 static struct target_type verity_target = {
 	.name		= "verity",
-	.version	= {1, 4, 0},
+	.version	= {1, 5, 0},
 	.module		= THIS_MODULE,
 	.ctr		= verity_ctr,
 	.dtr		= verity_dtr,
diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
new file mode 100644
index 000000000000..189b321d4ee6
--- /dev/null
+++ b/drivers/md/dm-verity-verify-sig.c
@@ -0,0 +1,139 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Microsoft Corporation.
+ *
+ * Author:  Jaskaran Singh Khurana <jaskarankhurana@linux.microsoft.com>
+ *
+ */
+#include <linux/device-mapper.h>
+#include <linux/verification.h>
+#include <keys/user-type.h>
+#include <linux/module.h>
+#include "dm-verity.h"
+#include "dm-verity-verify-sig.h"
+
+#define DM_VERITY_VERIFY_ERR(s) DM_VERITY_ROOT_HASH_VERIFICATION " " s
+
+static int verify_sig;
+module_param(verify_sig, int, 0);
+MODULE_PARM_DESC(verify_sig,
+		"Verify the roothash of dm-verity hash tree");
+
+#define DM_VERITY_IS_SIG_CHECK_ENABLED() \
+	(verify_sig == DM_VERITY_VERIFY_SIG_CHECK || \
+	 verify_sig == DM_VERITY_VERIFY_SIG_FORCE)
+
+#define DM_VERITY_IS_SIG_FORCE_ENABLED() \
+	(verify_sig == DM_VERITY_VERIFY_SIG_FORCE)
+
+bool verity_verify_is_sig_opt_arg(const char *arg_name)
+{
+	return (!strcasecmp(arg_name,
+			    DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY));
+}
+
+static int verity_verify_get_sig_from_key(const char *key_desc,
+					struct dm_verity_sig_opts *sig_opts)
+{
+	struct key *key;
+	const struct user_key_payload *ukp;
+	int ret = 0;
+
+	if (!DM_VERITY_IS_SIG_CHECK_ENABLED())
+		return 0;
+
+	key = request_key(&key_type_user,
+			key_desc, NULL);
+	if (IS_ERR(key))
+		return PTR_ERR(key);
+
+	down_read(&key->sem);
+
+	ukp = user_key_payload_locked(key);
+	if (!ukp) {
+		ret = -EKEYREVOKED;
+		goto end;
+	}
+
+	sig_opts->sig = kmalloc(ukp->datalen, GFP_KERNEL);
+	if (!sig_opts->sig) {
+		ret = -ENOMEM;
+		goto end;
+	}
+	sig_opts->sig_size = ukp->datalen;
+
+	memcpy(sig_opts->sig, ukp->data, sig_opts->sig_size);
+
+end:
+	up_read(&key->sem);
+	key_put(key);
+
+	return ret;
+}
+
+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as,
+				     struct dm_verity *v,
+				     struct dm_verity_sig_opts *sig_opts,
+				     unsigned int *argc,
+				     const char *arg_name)
+{
+	struct dm_target *ti = v->ti;
+	int ret = 0;
+	const char *sig_key = NULL;
+
+	if (!*argc) {
+		ti->error = DM_VERITY_VERIFY_ERR("Signature key not specified");
+		return -EINVAL;
+	}
+
+	sig_key = dm_shift_arg(as);
+	(*argc)--;
+
+	ret = verity_verify_get_sig_from_key(sig_key, sig_opts);
+	if (ret < 0)
+		ti->error = DM_VERITY_VERIFY_ERR("Invalid key specified");
+
+	return ret;
+}
+
+/*
+ * verify_verify_roothash - Verify the root hash of the verity hash device
+ *			     using builtin trusted keys.
+ *
+ * @root_hash: For verity, the roothash/data to be verified.
+ * @root_hash_len: Size of the roothash/data to be verified.
+ * @sig_data: The trusted signature that verifies the roothash/data.
+ * @sig_len: Size of the signature.
+ *
+ */
+int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
+			    const void *sig_data, size_t sig_len)
+{
+	int ret;
+
+	if (!DM_VERITY_IS_SIG_CHECK_ENABLED())
+		return 0;
+
+	if (!root_hash || root_hash_len == 0)
+		return -EINVAL;
+
+	if (!sig_data  || sig_len == 0) {
+		if (DM_VERITY_IS_SIG_FORCE_ENABLED())
+			return -ENOKEY;
+		else
+			return 0;
+	}
+
+	ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
+				sig_len, NULL, VERIFYING_UNSPECIFIED_SIGNATURE,
+				NULL, NULL);
+
+	return ret;
+}
+
+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts)
+{
+	kfree(sig_opts->sig);
+	sig_opts->sig = NULL;
+	sig_opts->sig_size = 0;
+}
diff --git a/drivers/md/dm-verity-verify-sig.h b/drivers/md/dm-verity-verify-sig.h
new file mode 100644
index 000000000000..3ac750996efb
--- /dev/null
+++ b/drivers/md/dm-verity-verify-sig.h
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Microsoft Corporation.
+ *
+ * Author:  Jaskaran Singh Khurana <jaskarankhurana@linux.microsoft.com>
+ *
+ */
+#ifndef DM_VERITY_SIG_VERIFICATION_H
+#define DM_VERITY_SIG_VERIFICATION_H
+
+#define DM_VERITY_ROOT_HASH_VERIFICATION "DM Verity Sig Verification"
+#define DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY "root_hash_sig_key_desc"
+#define DM_VERITY_ROOT_HASH_VERIFICATION_OPTS 2
+
+enum dm_verity_verif_opt {
+	DM_VERITY_VERIFY_SIG_NONE = 0,
+	DM_VERITY_VERIFY_SIG_CHECK,
+	DM_VERITY_VERIFY_SIG_FORCE,
+	DM_VERITY_VERIFY_SIG_MAX,
+};
+
+struct dm_verity_sig_opts {
+	unsigned int sig_size;
+	u8 *sig;
+};
+int verity_verify_root_hash(const void *data, size_t data_len,
+			    const void *sig_data, size_t sig_len);
+
+bool verity_verify_is_sig_opt_arg(const char *arg_name);
+
+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
+				    struct dm_verity_sig_opts *sig_opts,
+				    unsigned int *argc, const char *arg_name);
+
+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts);
+
+#endif /* DM_VERITY_SIG_VERIFICATION_H */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [RFC PATCH v5 1/1] Add dm verity root hash pkcs7 sig validation.
  2019-06-19 19:10 ` [RFC PATCH v5 1/1] " Jaskaran Khurana
@ 2019-06-25 18:20   ` Mike Snitzer
  2019-06-26  5:48     ` Milan Broz
  2019-08-13 18:49     ` Jaskaran Singh Khurana
  2019-06-27 12:17   ` Milan Broz
  2019-06-27 23:41   ` Eric Biggers
  2 siblings, 2 replies; 19+ messages in thread
From: Mike Snitzer @ 2019-06-25 18:20 UTC (permalink / raw)
  To: Jaskaran Khurana, gmazyland
  Cc: linux-security-module, linux-kernel, linux-integrity,
	linux-fsdevel, scottsh, ebiggers, jmorris, dm-devel, mpatocka,
	agk

On Wed, Jun 19 2019 at  3:10pm -0400,
Jaskaran Khurana <jaskarankhurana@linux.microsoft.com> wrote:

> The verification is to support cases where the roothash is not secured by
> Trusted Boot, UEFI Secureboot or similar technologies.
> One of the use cases for this is for dm-verity volumes mounted after boot,
> the root hash provided during the creation of the dm-verity volume has to
> be secure and thus in-kernel validation implemented here will be used
> before we trust the root hash and allow the block device to be created.
> 
> The signature being provided for verification must verify the root hash and
> must be trusted by the builtin keyring for verification to succeed.
> 
> The hash is added as a key of type "user" and the description is passed to 
> the kernel so it can look it up and use it for verification.
> 
> Kernel commandline parameter will indicate whether to check (only if 
> specified) or force (for all dm verity volumes) roothash signature 
> verification.
> 
> Kernel commandline: dm_verity.verify_sig=1 or 2 for check/force root hash
> signature validation respectively.
> 
> Signed-off-by: Jaskaran Khurana <jaskarankhurana@linux.microsoft.com>

Milan and/or others: could you please provide review and if you're OK
with this patch respond accordingly?

Thanks,
Mike

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC PATCH v5 1/1] Add dm verity root hash pkcs7 sig validation.
  2019-06-25 18:20   ` Mike Snitzer
@ 2019-06-26  5:48     ` Milan Broz
  2019-08-13 18:49     ` Jaskaran Singh Khurana
  1 sibling, 0 replies; 19+ messages in thread
From: Milan Broz @ 2019-06-26  5:48 UTC (permalink / raw)
  To: Mike Snitzer, Jaskaran Khurana
  Cc: linux-security-module, linux-kernel, linux-integrity,
	linux-fsdevel, scottsh, ebiggers, jmorris, dm-devel, mpatocka,
	agk

On 25/06/2019 20:20, Mike Snitzer wrote:
> On Wed, Jun 19 2019 at  3:10pm -0400,
> Jaskaran Khurana <jaskarankhurana@linux.microsoft.com> wrote:
> 
>> The verification is to support cases where the roothash is not secured by
>> Trusted Boot, UEFI Secureboot or similar technologies.
>> One of the use cases for this is for dm-verity volumes mounted after boot,
>> the root hash provided during the creation of the dm-verity volume has to
>> be secure and thus in-kernel validation implemented here will be used
>> before we trust the root hash and allow the block device to be created.
>>
>> The signature being provided for verification must verify the root hash and
>> must be trusted by the builtin keyring for verification to succeed.
>>
>> The hash is added as a key of type "user" and the description is passed to 
>> the kernel so it can look it up and use it for verification.
>>
>> Kernel commandline parameter will indicate whether to check (only if 
>> specified) or force (for all dm verity volumes) roothash signature 
>> verification.
>>
>> Kernel commandline: dm_verity.verify_sig=1 or 2 for check/force root hash
>> signature validation respectively.
>>
>> Signed-off-by: Jaskaran Khurana <jaskarankhurana@linux.microsoft.com>
> 
> Milan and/or others: could you please provide review and if you're OK
> with this patch respond accordingly?

Stand by please :)

I like the patch, I think all major problems were solved, but I still need to test it somehow.

Anyway, for the time being, I keep all ongoing patches that need some later
userspace support in my branch
https://git.kernel.org/pub/scm/linux/kernel/git/mbroz/linux.git/log/?h=dm-cryptsetup
so at least it get some automated testing.

Milan


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC PATCH v5 1/1] Add dm verity root hash pkcs7 sig validation.
  2019-06-19 19:10 ` [RFC PATCH v5 1/1] " Jaskaran Khurana
  2019-06-25 18:20   ` Mike Snitzer
@ 2019-06-27 12:17   ` Milan Broz
  2019-06-28  1:52     ` Jaskaran Singh Khurana
  2019-06-27 23:41   ` Eric Biggers
  2 siblings, 1 reply; 19+ messages in thread
From: Milan Broz @ 2019-06-27 12:17 UTC (permalink / raw)
  To: Jaskaran Khurana, linux-security-module, linux-kernel,
	linux-integrity, linux-fsdevel
  Cc: agk, snitzer, dm-devel, jmorris, scottsh, ebiggers, mpatocka

Hi,

I tried to test test the patch, two comments below.

On 19/06/2019 21:10, Jaskaran Khurana wrote:
> The verification is to support cases where the roothash is not secured by
> Trusted Boot, UEFI Secureboot or similar technologies.
> One of the use cases for this is for dm-verity volumes mounted after boot,
> the root hash provided during the creation of the dm-verity volume has to
> be secure and thus in-kernel validation implemented here will be used
> before we trust the root hash and allow the block device to be created.
> 
> The signature being provided for verification must verify the root hash and
> must be trusted by the builtin keyring for verification to succeed.
> 
> The hash is added as a key of type "user" and the description is passed to 
> the kernel so it can look it up and use it for verification.
> 
> Kernel commandline parameter will indicate whether to check (only if 
> specified) or force (for all dm verity volumes) roothash signature 
> verification.
> 
> Kernel commandline: dm_verity.verify_sig=1 or 2 for check/force root hash
> signature validation respectively.

1) I think the switch should be just boolean - enforce signatures for all dm-verity targets
(with default to false/off).

The rest should be handled by simple logic - if the root_hash_sig_key_desc option
is specified, the signature MUST be validated in the constructor, all errors should cause
failure (bad reference in keyring, bad signature, etc).

(Now it ignores for example bad reference to the keyring, this is quite misleading.)

If a user wants to activate a dm-verity device without a signature, just remove
optional argument referencing the signature.
(This is not possible with dm_verity.verify_sig set to true/on.)


2) All DM targets must provide the same mapping table status ("dmsetup table"
command) as initially configured.
The output of the command should be directly usable as mapping table constructor.

Your patch is missing that part, I tried to fix it, add-on patch is here
https://git.kernel.org/pub/scm/linux/kernel/git/mbroz/linux.git/commit/?h=dm-cryptsetup&id=a26c10806f5257e255b6a436713127e762935ad3
(feel free to fold it in your patch)


Thanks,
Milan

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC PATCH v5 1/1] Add dm verity root hash pkcs7 sig validation.
  2019-06-19 19:10 ` [RFC PATCH v5 1/1] " Jaskaran Khurana
  2019-06-25 18:20   ` Mike Snitzer
  2019-06-27 12:17   ` Milan Broz
@ 2019-06-27 23:41   ` Eric Biggers
  2019-06-28  1:49     ` Jaskaran Singh Khurana
  2 siblings, 1 reply; 19+ messages in thread
From: Eric Biggers @ 2019-06-27 23:41 UTC (permalink / raw)
  To: Jaskaran Khurana
  Cc: linux-security-module, linux-kernel, linux-integrity,
	linux-fsdevel, agk, snitzer, dm-devel, jmorris, scottsh,
	mpatocka, gmazyland

Hi Jaskaran, one comment (I haven't reviewed this in detail):

On Wed, Jun 19, 2019 at 12:10:48PM -0700, Jaskaran Khurana wrote:
> diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
> index db269a348b20..2d658a3512cb 100644
> --- a/drivers/md/Kconfig
> +++ b/drivers/md/Kconfig
> @@ -475,6 +475,7 @@ config DM_VERITY
>  	select CRYPTO
>  	select CRYPTO_HASH
>  	select DM_BUFIO
> +	select SYSTEM_DATA_VERIFICATION
>  	---help---
>  	  This device-mapper target creates a read-only device that
>  	  transparently validates the data on one underlying device against
> diff --git a/drivers/md/Makefile b/drivers/md/Makefile
> index be7a6eb92abc..3b47b256b15e 100644
> --- a/drivers/md/Makefile
> +++ b/drivers/md/Makefile
> @@ -18,7 +18,7 @@ dm-cache-y	+= dm-cache-target.o dm-cache-metadata.o dm-cache-policy.o \
>  		    dm-cache-background-tracker.o
>  dm-cache-smq-y   += dm-cache-policy-smq.o
>  dm-era-y	+= dm-era-target.o
> -dm-verity-y	+= dm-verity-target.o
> +dm-verity-y	+= dm-verity-target.o dm-verity-verify-sig.o
>  md-mod-y	+= md.o md-bitmap.o
>  raid456-y	+= raid5.o raid5-cache.o raid5-ppl.o
>  dm-zoned-y	+= dm-zoned-target.o dm-zoned-metadata.o dm-zoned-reclaim.o

Perhaps this should be made optional and controlled by a kconfig option
CONFIG_DM_VERITY_SIGNATURE_VERIFICATION, similar to CONFIG_DM_VERITY_FEC?

CONFIG_SYSTEM_DATA_VERIFICATION brings in a lot of stuff, which might be
unnecessary for some dm-verity users.  Also, you've already separated most of
the code out into a separate .c file anyway.

- Eric

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC PATCH v5 1/1] Add dm verity root hash pkcs7 sig validation.
  2019-06-27 23:41   ` Eric Biggers
@ 2019-06-28  1:49     ` Jaskaran Singh Khurana
  2019-06-28  3:00       ` Eric Biggers
  0 siblings, 1 reply; 19+ messages in thread
From: Jaskaran Singh Khurana @ 2019-06-28  1:49 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-security-module, linux-kernel, linux-integrity,
	linux-fsdevel, agk, snitzer, dm-devel, jmorris, scottsh,
	mpatocka, gmazyland



On Thu, 27 Jun 2019, Eric Biggers wrote:

> Hi Jaskaran, one comment (I haven't reviewed this in detail):
>
> On Wed, Jun 19, 2019 at 12:10:48PM -0700, Jaskaran Khurana wrote:
>> diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
>> index db269a348b20..2d658a3512cb 100644
>> --- a/drivers/md/Kconfig
>> +++ b/drivers/md/Kconfig
>> @@ -475,6 +475,7 @@ config DM_VERITY
>>  	select CRYPTO
>>  	select CRYPTO_HASH
>>  	select DM_BUFIO
>> +	select SYSTEM_DATA_VERIFICATION
>>  	---help---
>>  	  This device-mapper target creates a read-only device that
>>  	  transparently validates the data on one underlying device against
>> diff --git a/drivers/md/Makefile b/drivers/md/Makefile
>> index be7a6eb92abc..3b47b256b15e 100644
>> --- a/drivers/md/Makefile
>> +++ b/drivers/md/Makefile
>> @@ -18,7 +18,7 @@ dm-cache-y	+= dm-cache-target.o dm-cache-metadata.o dm-cache-policy.o \
>>  		    dm-cache-background-tracker.o
>>  dm-cache-smq-y   += dm-cache-policy-smq.o
>>  dm-era-y	+= dm-era-target.o
>> -dm-verity-y	+= dm-verity-target.o
>> +dm-verity-y	+= dm-verity-target.o dm-verity-verify-sig.o
>>  md-mod-y	+= md.o md-bitmap.o
>>  raid456-y	+= raid5.o raid5-cache.o raid5-ppl.o
>>  dm-zoned-y	+= dm-zoned-target.o dm-zoned-metadata.o dm-zoned-reclaim.o
>
> Perhaps this should be made optional and controlled by a kconfig option
> CONFIG_DM_VERITY_SIGNATURE_VERIFICATION, similar to CONFIG_DM_VERITY_FEC?
>
> CONFIG_SYSTEM_DATA_VERIFICATION brings in a lot of stuff, which might be
> unnecessary for some dm-verity users.  Also, you've already separated most of
> the code out into a separate .c file anyway.
>
> - Eric
>
Hello Eric,

This started with a config (see V4). We didnot want scripts that pass this 
parameter to suddenly stop working if for some reason the 
verification is turned off so the optional parameter was just 
parsed and no validation happened if the CONFIG was turned off. This was 
changed to a commandline parameter after feedback from the community, so I would prefer 
to keep it *now* as commandline parameter. Let me know if you are OK with 
this.

Regards,
JK

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC PATCH v5 1/1] Add dm verity root hash pkcs7 sig validation.
  2019-06-27 12:17   ` Milan Broz
@ 2019-06-28  1:52     ` Jaskaran Singh Khurana
  0 siblings, 0 replies; 19+ messages in thread
From: Jaskaran Singh Khurana @ 2019-06-28  1:52 UTC (permalink / raw)
  To: Milan Broz
  Cc: linux-security-module, linux-kernel, linux-integrity,
	linux-fsdevel, agk, snitzer, dm-devel, jmorris, scottsh,
	ebiggers, mpatocka



On Thu, 27 Jun 2019, Milan Broz wrote:

> Hi,
>
> I tried to test test the patch, two comments below.
>
> On 19/06/2019 21:10, Jaskaran Khurana wrote:
>> The verification is to support cases where the roothash is not secured by
>> Trusted Boot, UEFI Secureboot or similar technologies.
>> One of the use cases for this is for dm-verity volumes mounted after boot,
>> the root hash provided during the creation of the dm-verity volume has to
>> be secure and thus in-kernel validation implemented here will be used
>> before we trust the root hash and allow the block device to be created.
>>
>> The signature being provided for verification must verify the root hash and
>> must be trusted by the builtin keyring for verification to succeed.
>>
>> The hash is added as a key of type "user" and the description is passed to
>> the kernel so it can look it up and use it for verification.
>>
>> Kernel commandline parameter will indicate whether to check (only if
>> specified) or force (for all dm verity volumes) roothash signature
>> verification.
>>
>> Kernel commandline: dm_verity.verify_sig=1 or 2 for check/force root hash
>> signature validation respectively.
>
> 1) I think the switch should be just boolean - enforce signatures for all dm-verity targets
> (with default to false/off).
>
> The rest should be handled by simple logic - if the root_hash_sig_key_desc option
> is specified, the signature MUST be validated in the constructor, all errors should cause
> failure (bad reference in keyring, bad signature, etc).
>
> (Now it ignores for example bad reference to the keyring, this is quite misleading.)
>
> If a user wants to activate a dm-verity device without a signature, just remove
> optional argument referencing the signature.
> (This is not possible with dm_verity.verify_sig set to true/on.)
>
>
> 2) All DM targets must provide the same mapping table status ("dmsetup table"
> command) as initially configured.
> The output of the command should be directly usable as mapping table constructor.
>
> Your patch is missing that part, I tried to fix it, add-on patch is here
> https://git.kernel.org/pub/scm/linux/kernel/git/mbroz/linux.git/commit/?h=dm-cryptsetup&id=a26c10806f5257e255b6a436713127e762935ad3
> (feel free to fold it in your patch)
>
>
> Thanks,
> Milan
>
Hello Milan,

Thanks for testing and reviewing this. I will take care of the above 
comments change it to a boolean and for the second point merge the add-on 
patch that you shared.

Regards,
Jaskaran

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC PATCH v5 1/1] Add dm verity root hash pkcs7 sig validation.
  2019-06-28  1:49     ` Jaskaran Singh Khurana
@ 2019-06-28  3:00       ` Eric Biggers
  2019-06-28  5:12         ` Milan Broz
  0 siblings, 1 reply; 19+ messages in thread
From: Eric Biggers @ 2019-06-28  3:00 UTC (permalink / raw)
  To: Jaskaran Singh Khurana
  Cc: linux-security-module, linux-kernel, linux-integrity,
	linux-fsdevel, agk, snitzer, dm-devel, jmorris, scottsh,
	mpatocka, gmazyland

Hi Jaskaran,

On Thu, Jun 27, 2019 at 06:49:58PM -0700, Jaskaran Singh Khurana wrote:
> 
> 
> On Thu, 27 Jun 2019, Eric Biggers wrote:
> 
> > Hi Jaskaran, one comment (I haven't reviewed this in detail):
> > 
> > On Wed, Jun 19, 2019 at 12:10:48PM -0700, Jaskaran Khurana wrote:
> > > diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
> > > index db269a348b20..2d658a3512cb 100644
> > > --- a/drivers/md/Kconfig
> > > +++ b/drivers/md/Kconfig
> > > @@ -475,6 +475,7 @@ config DM_VERITY
> > >  	select CRYPTO
> > >  	select CRYPTO_HASH
> > >  	select DM_BUFIO
> > > +	select SYSTEM_DATA_VERIFICATION
> > >  	---help---
> > >  	  This device-mapper target creates a read-only device that
> > >  	  transparently validates the data on one underlying device against
> > > diff --git a/drivers/md/Makefile b/drivers/md/Makefile
> > > index be7a6eb92abc..3b47b256b15e 100644
> > > --- a/drivers/md/Makefile
> > > +++ b/drivers/md/Makefile
> > > @@ -18,7 +18,7 @@ dm-cache-y	+= dm-cache-target.o dm-cache-metadata.o dm-cache-policy.o \
> > >  		    dm-cache-background-tracker.o
> > >  dm-cache-smq-y   += dm-cache-policy-smq.o
> > >  dm-era-y	+= dm-era-target.o
> > > -dm-verity-y	+= dm-verity-target.o
> > > +dm-verity-y	+= dm-verity-target.o dm-verity-verify-sig.o
> > >  md-mod-y	+= md.o md-bitmap.o
> > >  raid456-y	+= raid5.o raid5-cache.o raid5-ppl.o
> > >  dm-zoned-y	+= dm-zoned-target.o dm-zoned-metadata.o dm-zoned-reclaim.o
> > 
> > Perhaps this should be made optional and controlled by a kconfig option
> > CONFIG_DM_VERITY_SIGNATURE_VERIFICATION, similar to CONFIG_DM_VERITY_FEC?
> > 
> > CONFIG_SYSTEM_DATA_VERIFICATION brings in a lot of stuff, which might be
> > unnecessary for some dm-verity users.  Also, you've already separated most of
> > the code out into a separate .c file anyway.
> > 
> > - Eric
> > 
> Hello Eric,
> 
> This started with a config (see V4). We didnot want scripts that pass this
> parameter to suddenly stop working if for some reason the verification is
> turned off so the optional parameter was just parsed and no validation
> happened if the CONFIG was turned off. This was changed to a commandline
> parameter after feedback from the community, so I would prefer to keep it
> *now* as commandline parameter. Let me know if you are OK with this.
> 
> Regards,
> JK

Sorry, I haven't been following the whole discussion.  (BTW, you sent out
multiple versions both called "v4", and using a cover letter for a single patch
makes it unnecessarily difficult to review.)  However, it appears Milan were
complaining about the DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE option which set the
policy for signature verification, *not* the DM_VERITY_VERIFY_ROOTHASH_SIG
option which enabled support for signature verification.  Am I missing
something?  You can have a module parameter which controls the "signatures
required" setting, while also allowing people to compile out kernel support for
the signature verification feature.

Sure, it means that the signature verification support won't be guaranteed to be
present when dm-verity is.  But the same is true of the hash algorithm (e.g.
sha512), and of the forward error correction feature.  Since the signature
verification is nontrivial and pulls in a lot of other kernel code which might
not be otherwise needed (via SYSTEM_DATA_VERIFICATION), it seems a natural
candidate for putting the support behind a Kconfig option.

BTW, I'm doing something similar for fs-verity; see
https://patchwork.kernel.org/patch/11008077/.  The signature verification
support is behind CONFIG_FS_VERITY_BUILTIN_SIGNATURES, but the policy of
requiring signatures is a sysctl fs.verity.require_signatures.

- Eric

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC PATCH v5 0/1] Add dm verity root hash pkcs7 sig validation.
  2019-06-19 19:10 [RFC PATCH v5 0/1] Add dm verity root hash pkcs7 sig validation Jaskaran Khurana
  2019-06-19 19:10 ` [RFC PATCH v5 1/1] " Jaskaran Khurana
@ 2019-06-28  4:00 ` Eric Biggers
  2019-06-28 19:45   ` Jaskaran Singh Khurana
  2019-06-29  4:01   ` James Morris
  1 sibling, 2 replies; 19+ messages in thread
From: Eric Biggers @ 2019-06-28  4:00 UTC (permalink / raw)
  To: Jaskaran Khurana
  Cc: linux-security-module, linux-kernel, linux-integrity,
	linux-fsdevel, agk, snitzer, dm-devel, jmorris, scottsh,
	mpatocka, gmazyland

On Wed, Jun 19, 2019 at 12:10:47PM -0700, Jaskaran Khurana wrote:
> This patch set adds in-kernel pkcs7 signature checking for the roothash of
> the dm-verity hash tree.
> The verification is to support cases where the roothash is not secured by
> Trusted Boot, UEFI Secureboot or similar technologies.
> One of the use cases for this is for dm-verity volumes mounted after boot,
> the root hash provided during the creation of the dm-verity volume has to
> be secure and thus in-kernel validation implemented here will be used
> before we trust the root hash and allow the block device to be created.
> 
> Why we are doing validation in the Kernel?
> 
> The reason is to still be secure in cases where the attacker is able to
> compromise the user mode application in which case the user mode validation
> could not have been trusted.
> The root hash signature validation in the kernel along with existing
> dm-verity implementation gives a higher level of confidence in the
> executable code or the protected data. Before allowing the creation of
> the device mapper block device the kernel code will check that the detached
> pkcs7 signature passed to it validates the roothash and the signature is
> trusted by builtin keys set at kernel creation. The kernel should be
> secured using Verified boot, UEFI Secure Boot or similar technologies so we
> can trust it.
> 
> What about attacker mounting non dm-verity volumes to run executable
> code?
> 
> This verification can be used to have a security architecture where a LSM
> can enforce this verification for all the volumes and by doing this it can
> ensure that all executable code runs from signed and trusted dm-verity
> volumes.
> 
> Further patches will be posted that build on this and enforce this
> verification based on policy for all the volumes on the system.
> 

I don't understand your justification for this feature.

If userspace has already been pwned severely enough for the attacker to be
executing arbitrary code with CAP_SYS_ADMIN (which is what the device mapper
ioctls need), what good are restrictions on loading more binaries from disk?

Please explain your security model.

- Eric

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC PATCH v5 1/1] Add dm verity root hash pkcs7 sig validation.
  2019-06-28  3:00       ` Eric Biggers
@ 2019-06-28  5:12         ` Milan Broz
  2019-06-28 17:03           ` Jaskaran Singh Khurana
  0 siblings, 1 reply; 19+ messages in thread
From: Milan Broz @ 2019-06-28  5:12 UTC (permalink / raw)
  To: Eric Biggers, Jaskaran Singh Khurana
  Cc: linux-security-module, linux-kernel, linux-integrity,
	linux-fsdevel, agk, snitzer, dm-devel, jmorris, scottsh,
	mpatocka

On 28/06/2019 05:00, Eric Biggers wrote:
>> Hello Eric,
>>
>> This started with a config (see V4). We didnot want scripts that pass this
>> parameter to suddenly stop working if for some reason the verification is
>> turned off so the optional parameter was just parsed and no validation
>> happened if the CONFIG was turned off. This was changed to a commandline
>> parameter after feedback from the community, so I would prefer to keep it
>> *now* as commandline parameter. Let me know if you are OK with this.
>>
>> Regards,
>> JK
> 
> Sorry, I haven't been following the whole discussion.  (BTW, you sent out
> multiple versions both called "v4", and using a cover letter for a single patch
> makes it unnecessarily difficult to review.)  However, it appears Milan were
> complaining about the DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE option which set the
> policy for signature verification, *not* the DM_VERITY_VERIFY_ROOTHASH_SIG
> option which enabled support for signature verification.  Am I missing
> something?  You can have a module parameter which controls the "signatures
> required" setting, while also allowing people to compile out kernel support for
> the signature verification feature.

Yes, this was exactly my point.

I think I even mention in some reply to use exactly the same config Makefile logic
as for FEC - to allow completely compile it out of the source:

ifeq ($(CONFIG_DM_VERITY_FEC),y)
dm-verity-objs                  += dm-verity-fec.o
endif

> Sure, it means that the signature verification support won't be guaranteed to be
> present when dm-verity is.  But the same is true of the hash algorithm (e.g.
> sha512), and of the forward error correction feature.  Since the signature
> verification is nontrivial and pulls in a lot of other kernel code which might
> not be otherwise needed (via SYSTEM_DATA_VERIFICATION), it seems a natural
> candidate for putting the support behind a Kconfig option.

On the other side, dm-verity is meant for a system verification, so if it depends
on SYSTEM_DATA_VERIFICATION is ... not so surprising :)

But the change above is quite easy and while we already have FEC as config option,
perhaps let's do it the same here.

Milan

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC PATCH v5 1/1] Add dm verity root hash pkcs7 sig validation.
  2019-06-28  5:12         ` Milan Broz
@ 2019-06-28 17:03           ` Jaskaran Singh Khurana
  0 siblings, 0 replies; 19+ messages in thread
From: Jaskaran Singh Khurana @ 2019-06-28 17:03 UTC (permalink / raw)
  To: Milan Broz
  Cc: Eric Biggers, linux-security-module, linux-kernel,
	linux-integrity, linux-fsdevel, agk, snitzer, dm-devel, jmorris,
	scottsh, mpatocka


Hello Eric/Milan,

On Fri, 28 Jun 2019, Milan Broz wrote:

> On 28/06/2019 05:00, Eric Biggers wrote:
>>> Hello Eric,
>>>
>>> This started with a config (see V4). We didnot want scripts that pass this
>>> parameter to suddenly stop working if for some reason the verification is
>>> turned off so the optional parameter was just parsed and no validation
>>> happened if the CONFIG was turned off. This was changed to a commandline
>>> parameter after feedback from the community, so I would prefer to keep it
>>> *now* as commandline parameter. Let me know if you are OK with this.
>>>
>>> Regards,
>>> JK
>>
>> Sorry, I haven't been following the whole discussion.  (BTW, you sent out
>> multiple versions both called "v4", and using a cover letter for a single patch
>> makes it unnecessarily difficult to review.)  However, it appears Milan were
>> complaining about the DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE option which set the
>> policy for signature verification, *not* the DM_VERITY_VERIFY_ROOTHASH_SIG
>> option which enabled support for signature verification.  Am I missing
>> something?  You can have a module parameter which controls the "signatures
>> required" setting, while also allowing people to compile out kernel support for
>> the signature verification feature.
>
> Yes, this was exactly my point.
>
> I think I even mention in some reply to use exactly the same config Makefile logic
> as for FEC - to allow completely compile it out of the source:
>
> ifeq ($(CONFIG_DM_VERITY_FEC),y)
> dm-verity-objs                  += dm-verity-fec.o
> endif
>
>> Sure, it means that the signature verification support won't be guaranteed to be
>> present when dm-verity is.  But the same is true of the hash algorithm (e.g.
>> sha512), and of the forward error correction feature.  Since the signature
>> verification is nontrivial and pulls in a lot of other kernel code which might
>> not be otherwise needed (via SYSTEM_DATA_VERIFICATION), it seems a natural
>> candidate for putting the support behind a Kconfig option.
>
> On the other side, dm-verity is meant for a system verification, so if it depends
> on SYSTEM_DATA_VERIFICATION is ... not so surprising :)
>
> But the change above is quite easy and while we already have FEC as config option,
> perhaps let's do it the same here.
>
> Milan
>
Yes, I will make this change. Please consider this discussion as resolved. 
Thanks.

Regards,
Jaskaran.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC PATCH v5 0/1] Add dm verity root hash pkcs7 sig validation.
  2019-06-28  4:00 ` [RFC PATCH v5 0/1] " Eric Biggers
@ 2019-06-28 19:45   ` Jaskaran Singh Khurana
  2019-06-28 20:34     ` Eric Biggers
  2019-06-29  4:01   ` James Morris
  1 sibling, 1 reply; 19+ messages in thread
From: Jaskaran Singh Khurana @ 2019-06-28 19:45 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-security-module, linux-kernel, linux-integrity,
	linux-fsdevel, agk, snitzer, dm-devel, jmorris, scottsh,
	mpatocka, gmazyland


Hello Eric,
On Thu, 27 Jun 2019, Eric Biggers wrote:

> On Wed, Jun 19, 2019 at 12:10:47PM -0700, Jaskaran Khurana wrote:
>> This patch set adds in-kernel pkcs7 signature checking for the roothash of
>> the dm-verity hash tree.
>> The verification is to support cases where the roothash is not secured by
>> Trusted Boot, UEFI Secureboot or similar technologies.
>> One of the use cases for this is for dm-verity volumes mounted after boot,
>> the root hash provided during the creation of the dm-verity volume has to
>> be secure and thus in-kernel validation implemented here will be used
>> before we trust the root hash and allow the block device to be created.
>>
>> Why we are doing validation in the Kernel?
>>
>> The reason is to still be secure in cases where the attacker is able to
>> compromise the user mode application in which case the user mode validation
>> could not have been trusted.
>> The root hash signature validation in the kernel along with existing
>> dm-verity implementation gives a higher level of confidence in the
>> executable code or the protected data. Before allowing the creation of
>> the device mapper block device the kernel code will check that the detached
>> pkcs7 signature passed to it validates the roothash and the signature is
>> trusted by builtin keys set at kernel creation. The kernel should be
>> secured using Verified boot, UEFI Secure Boot or similar technologies so we
>> can trust it.
>>
>> What about attacker mounting non dm-verity volumes to run executable
>> code?
>>
>> This verification can be used to have a security architecture where a LSM
>> can enforce this verification for all the volumes and by doing this it can
>> ensure that all executable code runs from signed and trusted dm-verity
>> volumes.
>>
>> Further patches will be posted that build on this and enforce this
>> verification based on policy for all the volumes on the system.
>>
>
> I don't understand your justification for this feature.
>
> If userspace has already been pwned severely enough for the attacker to be
> executing arbitrary code with CAP_SYS_ADMIN (which is what the device mapper
> ioctls need), what good are restrictions on loading more binaries from disk?
>
> Please explain your security model.
>
> - Eric
>

In a datacenter like environment, this will protect the system from below 
attacks:

1.Prevents attacker from deploying scripts that run arbitrary executables on the system.
2.Prevents physically present malicious admin to run arbitrary code on the
   machine.

Regards,
Jaskaran

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC PATCH v5 0/1] Add dm verity root hash pkcs7 sig validation.
  2019-06-28 19:45   ` Jaskaran Singh Khurana
@ 2019-06-28 20:34     ` Eric Biggers
  2019-06-28 23:27       ` Jaskaran Singh Khurana
  0 siblings, 1 reply; 19+ messages in thread
From: Eric Biggers @ 2019-06-28 20:34 UTC (permalink / raw)
  To: Jaskaran Singh Khurana
  Cc: linux-security-module, linux-kernel, linux-integrity,
	linux-fsdevel, agk, snitzer, dm-devel, jmorris, scottsh,
	mpatocka, gmazyland

On Fri, Jun 28, 2019 at 12:45:11PM -0700, Jaskaran Singh Khurana wrote:
> 
> Hello Eric,
> On Thu, 27 Jun 2019, Eric Biggers wrote:
> 
> > On Wed, Jun 19, 2019 at 12:10:47PM -0700, Jaskaran Khurana wrote:
> > > This patch set adds in-kernel pkcs7 signature checking for the roothash of
> > > the dm-verity hash tree.
> > > The verification is to support cases where the roothash is not secured by
> > > Trusted Boot, UEFI Secureboot or similar technologies.
> > > One of the use cases for this is for dm-verity volumes mounted after boot,
> > > the root hash provided during the creation of the dm-verity volume has to
> > > be secure and thus in-kernel validation implemented here will be used
> > > before we trust the root hash and allow the block device to be created.
> > > 
> > > Why we are doing validation in the Kernel?
> > > 
> > > The reason is to still be secure in cases where the attacker is able to
> > > compromise the user mode application in which case the user mode validation
> > > could not have been trusted.
> > > The root hash signature validation in the kernel along with existing
> > > dm-verity implementation gives a higher level of confidence in the
> > > executable code or the protected data. Before allowing the creation of
> > > the device mapper block device the kernel code will check that the detached
> > > pkcs7 signature passed to it validates the roothash and the signature is
> > > trusted by builtin keys set at kernel creation. The kernel should be
> > > secured using Verified boot, UEFI Secure Boot or similar technologies so we
> > > can trust it.
> > > 
> > > What about attacker mounting non dm-verity volumes to run executable
> > > code?
> > > 
> > > This verification can be used to have a security architecture where a LSM
> > > can enforce this verification for all the volumes and by doing this it can
> > > ensure that all executable code runs from signed and trusted dm-verity
> > > volumes.
> > > 
> > > Further patches will be posted that build on this and enforce this
> > > verification based on policy for all the volumes on the system.
> > > 
> > 
> > I don't understand your justification for this feature.
> > 
> > If userspace has already been pwned severely enough for the attacker to be
> > executing arbitrary code with CAP_SYS_ADMIN (which is what the device mapper
> > ioctls need), what good are restrictions on loading more binaries from disk?
> > 
> > Please explain your security model.
> > 
> > - Eric
> > 
> 
> In a datacenter like environment, this will protect the system from below
> attacks:
> 
> 1.Prevents attacker from deploying scripts that run arbitrary executables on the system.
> 2.Prevents physically present malicious admin to run arbitrary code on the
>   machine.
> 
> Regards,
> Jaskaran

So you are trying to protect against people who already have a root shell?

Can't they just e.g. run /usr/bin/python and type in some Python code?

Or run /usr/bin/curl and upload all your secret data to their server.

- Eric

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC PATCH v5 0/1] Add dm verity root hash pkcs7 sig validation.
  2019-06-28 20:34     ` Eric Biggers
@ 2019-06-28 23:27       ` Jaskaran Singh Khurana
  0 siblings, 0 replies; 19+ messages in thread
From: Jaskaran Singh Khurana @ 2019-06-28 23:27 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-security-module, linux-kernel, linux-integrity,
	linux-fsdevel, agk, snitzer, dm-devel, jmorris, scottsh,
	mpatocka, gmazyland


Hello Eric,

On Fri, 28 Jun 2019, Eric Biggers wrote:

>> In a datacenter like environment, this will protect the system from below
>> attacks:
>>
>> 1.Prevents attacker from deploying scripts that run arbitrary executables on the system.
>> 2.Prevents physically present malicious admin to run arbitrary code on the
>>   machine.
>>
>> Regards,
>> Jaskaran
>
> So you are trying to protect against people who already have a root shell?
>
> Can't they just e.g. run /usr/bin/python and type in some Python code?
>
> Or run /usr/bin/curl and upload all your secret data to their server.
>
> - Eric
>

You are correct, it would not be feasible for a general purpose distro, 
but for embedded systems and other cases where there is a more tightly 
locked-down system.

Regards,
Jaskaran.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC PATCH v5 0/1] Add dm verity root hash pkcs7 sig validation.
  2019-06-28  4:00 ` [RFC PATCH v5 0/1] " Eric Biggers
  2019-06-28 19:45   ` Jaskaran Singh Khurana
@ 2019-06-29  4:01   ` James Morris
  2019-07-01  9:41     ` Milan Broz
  1 sibling, 1 reply; 19+ messages in thread
From: James Morris @ 2019-06-29  4:01 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Jaskaran Khurana, linux-security-module, linux-kernel,
	linux-integrity, linux-fsdevel, agk, snitzer, dm-devel, scottsh,
	mpatocka, gmazyland

On Thu, 27 Jun 2019, Eric Biggers wrote:

> I don't understand your justification for this feature.
> 
> If userspace has already been pwned severely enough for the attacker to be
> executing arbitrary code with CAP_SYS_ADMIN (which is what the device mapper
> ioctls need), what good are restrictions on loading more binaries from disk?
> 
> Please explain your security model.

Let's say the system has a policy where all code must be signed with a 
valid key, and that one mechanism for enforcing this is via signed 
dm-verity volumes. Validating the signature within the kernel provides 
stronger assurance than userspace validation. The kernel validates and 
executes the code, using kernel-resident keys, and does not need to rely 
on validation which has occurred across a trust boundary.

You don't need arbitrary CAP_SYS_ADMIN code execution, you just need a 
flaw in the app (or its dependent libraries, or configuration) which 
allows signature validation to be bypassed.

The attacker now needs a kernel rather than a userspace vulnerability to 
bypass the signed code policy.

-- 
James Morris
<jmorris@namei.org>


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC PATCH v5 0/1] Add dm verity root hash pkcs7 sig validation.
  2019-06-29  4:01   ` James Morris
@ 2019-07-01  9:41     ` Milan Broz
  2019-07-01 17:33       ` Jaskaran Singh Khurana
  0 siblings, 1 reply; 19+ messages in thread
From: Milan Broz @ 2019-07-01  9:41 UTC (permalink / raw)
  To: James Morris, Eric Biggers
  Cc: Jaskaran Khurana, linux-security-module, linux-kernel,
	linux-integrity, linux-fsdevel, agk, snitzer, dm-devel, scottsh,
	mpatocka

On 29/06/2019 06:01, James Morris wrote:
> On Thu, 27 Jun 2019, Eric Biggers wrote:
> 
>> I don't understand your justification for this feature.
>>
>> If userspace has already been pwned severely enough for the attacker to be
>> executing arbitrary code with CAP_SYS_ADMIN (which is what the device mapper
>> ioctls need), what good are restrictions on loading more binaries from disk?
>>
>> Please explain your security model.
> 
> Let's say the system has a policy where all code must be signed with a 
> valid key, and that one mechanism for enforcing this is via signed 
> dm-verity volumes. Validating the signature within the kernel provides 
> stronger assurance than userspace validation. The kernel validates and 
> executes the code, using kernel-resident keys, and does not need to rely 
> on validation which has occurred across a trust boundary.

Yes, but as it is implemented in this patch, a certificate is provided as
a binary blob by the (super)user that activates the dm-verity device.

Actually, I can put there anything that looks like a correct signature (self-signed
or so), and dm-verity code is happy because the root hash is now signed.

Maybe could this concept be extended to support in-kernel compiled certificates?

I like the idea of signed root hash, but the truth is that if you have access
to device activation, it brings nothing, you can just put any cert in the keyring
and use it.

Milan

> 
> You don't need arbitrary CAP_SYS_ADMIN code execution, you just need a 
> flaw in the app (or its dependent libraries, or configuration) which 
> allows signature validation to be bypassed.
> 
> The attacker now needs a kernel rather than a userspace vulnerability to 
> bypass the signed code policy.
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC PATCH v5 0/1] Add dm verity root hash pkcs7 sig validation.
  2019-07-01  9:41     ` Milan Broz
@ 2019-07-01 17:33       ` Jaskaran Singh Khurana
  0 siblings, 0 replies; 19+ messages in thread
From: Jaskaran Singh Khurana @ 2019-07-01 17:33 UTC (permalink / raw)
  To: Milan Broz
  Cc: James Morris, Eric Biggers, linux-security-module, linux-kernel,
	linux-integrity, linux-fsdevel, agk, snitzer, dm-devel, scottsh,
	mpatocka


Hello Milan,
On Mon, 1 Jul 2019, Milan Broz wrote:

> On 29/06/2019 06:01, James Morris wrote:
>> On Thu, 27 Jun 2019, Eric Biggers wrote:
>>
>>> I don't understand your justification for this feature.
>>>
>>> If userspace has already been pwned severely enough for the attacker to be
>>> executing arbitrary code with CAP_SYS_ADMIN (which is what the device mapper
>>> ioctls need), what good are restrictions on loading more binaries from disk?
>>>
>>> Please explain your security model.
>>
>> Let's say the system has a policy where all code must be signed with a
>> valid key, and that one mechanism for enforcing this is via signed
>> dm-verity volumes. Validating the signature within the kernel provides
>> stronger assurance than userspace validation. The kernel validates and
>> executes the code, using kernel-resident keys, and does not need to rely
>> on validation which has occurred across a trust boundary.
>
> Yes, but as it is implemented in this patch, a certificate is provided as
> a binary blob by the (super)user that activates the dm-verity device.
>
> Actually, I can put there anything that looks like a correct signature (self-signed
> or so), and dm-verity code is happy because the root hash is now signed.
>
> Maybe could this concept be extended to support in-kernel compiled certificates?
>
> I like the idea of signed root hash, but the truth is that if you have access
> to device activation, it brings nothing, you can just put any cert in the keyring
> and use it.
>
> Milan
>

The signature needs to be trusted by the .builtin_trusted_keys which is
a read-only list of keys that were compiled into the kernel. The 
verify_pkcs7_signature verifies trust against the builtin keyring so I 
think what you are suggesting is covered here.

Regards,
Jaskaran.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC PATCH v5 1/1] Add dm verity root hash pkcs7 sig validation.
  2019-06-25 18:20   ` Mike Snitzer
  2019-06-26  5:48     ` Milan Broz
@ 2019-08-13 18:49     ` Jaskaran Singh Khurana
  1 sibling, 0 replies; 19+ messages in thread
From: Jaskaran Singh Khurana @ 2019-08-13 18:49 UTC (permalink / raw)
  To: Mike Snitzer
  Cc: gmazyland, linux-security-module, linux-kernel, linux-integrity,
	linux-fsdevel, scottsh, ebiggers, jmorris, dm-devel, mpatocka,
	agk


Hello Mike,
On Tue, 25 Jun 2019, Mike Snitzer wrote:

> On Wed, Jun 19 2019 at  3:10pm -0400,
> Jaskaran Khurana <jaskarankhurana@linux.microsoft.com> wrote:
>
>> The verification is to support cases where the roothash is not secured by
>> Trusted Boot, UEFI Secureboot or similar technologies.
>> One of the use cases for this is for dm-verity volumes mounted after boot,
>> the root hash provided during the creation of the dm-verity volume has to
>> be secure and thus in-kernel validation implemented here will be used
>> before we trust the root hash and allow the block device to be created.
>>
>> The signature being provided for verification must verify the root hash and
>> must be trusted by the builtin keyring for verification to succeed.
>>
>> The hash is added as a key of type "user" and the description is passed to
>> the kernel so it can look it up and use it for verification.
>>
>> Kernel commandline parameter will indicate whether to check (only if
>> specified) or force (for all dm verity volumes) roothash signature
>> verification.
>>
>> Kernel commandline: dm_verity.verify_sig=1 or 2 for check/force root hash
>> signature validation respectively.
>>
>> Signed-off-by: Jaskaran Khurana <jaskarankhurana@linux.microsoft.com>
>
> Milan and/or others: could you please provide review and if you're OK
> with this patch respond accordingly?
>

The v7 of this patch was Reviewed and Tested by Milan Broz. Could you tell 
me when this will be merged/next steps, if required I can post the patches 
again.

> Thanks,
> Mike
>
Regards,
Jaskaran

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2019-08-13 22:42 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-19 19:10 [RFC PATCH v5 0/1] Add dm verity root hash pkcs7 sig validation Jaskaran Khurana
2019-06-19 19:10 ` [RFC PATCH v5 1/1] " Jaskaran Khurana
2019-06-25 18:20   ` Mike Snitzer
2019-06-26  5:48     ` Milan Broz
2019-08-13 18:49     ` Jaskaran Singh Khurana
2019-06-27 12:17   ` Milan Broz
2019-06-28  1:52     ` Jaskaran Singh Khurana
2019-06-27 23:41   ` Eric Biggers
2019-06-28  1:49     ` Jaskaran Singh Khurana
2019-06-28  3:00       ` Eric Biggers
2019-06-28  5:12         ` Milan Broz
2019-06-28 17:03           ` Jaskaran Singh Khurana
2019-06-28  4:00 ` [RFC PATCH v5 0/1] " Eric Biggers
2019-06-28 19:45   ` Jaskaran Singh Khurana
2019-06-28 20:34     ` Eric Biggers
2019-06-28 23:27       ` Jaskaran Singh Khurana
2019-06-29  4:01   ` James Morris
2019-07-01  9:41     ` Milan Broz
2019-07-01 17:33       ` Jaskaran Singh Khurana

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).