[PATCH 1/3] ndctl: fix load-keys for user master-key

[PATCH 1/3] ndctl: fix load-keys for user master-key

[Dave Jiang]

load-keys incorrectly assumes that all keys have TPM handles. TPM handle is
only for trusted-keys. Fix in order to allow user master-key work.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 ndctl/load-keys.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/ndctl/load-keys.c b/ndctl/load-keys.c
index 8e4998f2..19380152 100644
--- a/ndctl/load-keys.c
+++ b/ndctl/load-keys.c
@@ -213,10 +213,8 @@ static int load_keys(struct loadkeys *lk_ctx, const char *keypath,
 
 	if (!tpmhandle) {
 		rc = check_tpm_handle(lk_ctx);
-		if (rc < 0) {
-			rc = -errno;
-			goto erropen;
-		}
+		if (rc < 0)
+			printf("No TPM handle discovered.\n");
 	}
 
 	rc = load_master_key(lk_ctx, param.key_path);

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 2/3] ndctl: fix key blob loading for user keys

[Dave Jiang]

The syntax for loading user master key is different than loading a trusted
key. Fix so we can load user key properly.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 ndctl/load-keys.c |   13 +++++--------
 ndctl/util/keys.c |   20 +++++++++++++++-----
 ndctl/util/keys.h |   10 ++++++++--
 3 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/ndctl/load-keys.c b/ndctl/load-keys.c
index 19380152..c0d0d743 100644
--- a/ndctl/load-keys.c
+++ b/ndctl/load-keys.c
@@ -25,12 +25,7 @@ static struct parameters {
 	const char *tpm_handle;
 } param;
 
-enum key_type {
-	KEY_USER = 0,
-	KEY_TRUSTED,
-};
-
-static const char *key_names[] = {"user", "trusted"};
+static const char *key_names[] = {"user", "trusted", "encrypted"};
 
 static struct loadkeys {
 	enum key_type key_type;
@@ -44,6 +39,7 @@ static int load_master_key(struct loadkeys *lk_ctx, const char *keypath)
 	char *blob;
 	int size, rc;
 	char path[PATH_MAX];
+	enum key_type;
 
 	rc = sprintf(path, "%s/nvdimm-master.blob", keypath);
 	if (rc < 0)
@@ -65,7 +61,8 @@ static int load_master_key(struct loadkeys *lk_ctx, const char *keypath)
 		return -errno;
 	}
 
-	blob = ndctl_load_key_blob(path, &size, param.tpm_handle, -1);
+	blob = ndctl_load_key_blob(path, &size, param.tpm_handle, -1,
+			lk_ctx->key_type);
 	if (!blob)
 		return -ENOMEM;
 
@@ -122,7 +119,7 @@ static int load_dimm_keys(struct loadkeys *lk_ctx)
 		}
 
 		blob = ndctl_load_key_blob(dent->d_name, &size, NULL,
-				lk_ctx->dirfd);
+				lk_ctx->dirfd, KEY_ENCRYPTED);
 		if (!blob) {
 			free(fname);
 			continue;
diff --git a/ndctl/util/keys.c b/ndctl/util/keys.c
index 622533d7..a621a5f5 100644
--- a/ndctl/util/keys.c
+++ b/ndctl/util/keys.c
@@ -103,13 +103,17 @@ static int get_key_desc(struct ndctl_dimm *dimm, char *desc,
 }
 
 char *ndctl_load_key_blob(const char *path, int *size, const char *postfix,
-		int dirfd)
+		int dirfd, enum key_type key_type)
 {
 	struct stat st;
 	ssize_t read_bytes = 0;
 	int rc, fd;
 	char *blob, *pl, *rdptr;
 	char prefix[] = "load ";
+	bool need_prefix = false;
+
+	if (key_type == KEY_ENCRYPTED || key_type == KEY_TRUSTED)
+		need_prefix = true;
 
 	fd = openat(dirfd, path, O_RDONLY);
 	if (fd < 0) {
@@ -133,7 +137,10 @@ char *ndctl_load_key_blob(const char *path, int *size, const char *postfix,
 		return NULL;
 	}
 
-	*size = st.st_size + sizeof(prefix) - 1;
+	*size = st.st_size;
+	if (need_prefix)
+		*size += strlen(prefix);
+
 	/*
 	 * We need to increment postfix and space.
 	 * "keyhandle=" is 10 bytes, plus null termination.
@@ -146,8 +153,11 @@ char *ndctl_load_key_blob(const char *path, int *size, const char *postfix,
 		return NULL;
 	}
 
-	memcpy(blob, prefix, sizeof(prefix) - 1);
-	pl = blob + sizeof(prefix) - 1;
+	if (need_prefix) {
+		memcpy(blob, prefix, strlen(prefix));
+		pl = blob + strlen(prefix);
+	} else
+		pl = blob;
 
 	rdptr = pl;
 	do {
@@ -300,7 +310,7 @@ static key_serial_t dimm_load_key(struct ndctl_dimm *dimm,
 	if (rc < 0)
 		return rc;
 
-	blob = ndctl_load_key_blob(path, &size, NULL, -1);
+	blob = ndctl_load_key_blob(path, &size, NULL, -1, KEY_ENCRYPTED);
 	if (!blob)
 		return -ENOMEM;
 
diff --git a/ndctl/util/keys.h b/ndctl/util/keys.h
index eab78d2f..9bc995ac 100644
--- a/ndctl/util/keys.h
+++ b/ndctl/util/keys.h
@@ -12,9 +12,15 @@ enum ndctl_key_type {
 	ND_ZERO_KEY,
 };
 
+enum key_type {
+	KEY_USER = 0,
+	KEY_TRUSTED,
+	KEY_ENCRYPTED,
+};
+
 #ifdef ENABLE_KEYUTILS
 char *ndctl_load_key_blob(const char *path, int *size, const char *postfix,
-		int dirfd);
+		int dirfd, enum key_type key_type);
 int ndctl_dimm_setup_key(struct ndctl_dimm *dimm, const char *kek,
 				enum ndctl_key_type key_type);
 int ndctl_dimm_update_key(struct ndctl_dimm *dimm, const char *kek,
@@ -25,7 +31,7 @@ int ndctl_dimm_secure_erase_key(struct ndctl_dimm *dimm,
 int ndctl_dimm_overwrite_key(struct ndctl_dimm *dimm);
 #else
 char *ndctl_load_key_blob(const char *path, int *size, const char *postfix,
-		int dirfd)
+		int dirfd, enum key_type key_type)
 {
 	return NULL;
 }

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 3/3] ndctl: add unit test for load-keys

[Dave Jiang]

Add to security.sh to test load-keys for user keys.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 test/security.sh |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 4 deletions(-)

diff --git a/test/security.sh b/test/security.sh
index 1b7a9a1a..7bd60293 100755
--- a/test/security.sh
+++ b/test/security.sh
@@ -6,8 +6,10 @@ rc=77
 dev=""
 id=""
 keypath="/etc/ndctl/keys"
-masterkey="nvdimm-master-test"
-masterpath="$keypath/$masterkey"
+masterkey="nvdimm-master"
+masterpath="$keypath/$masterkey.blob"
+backup_key=0
+backup_handle=0
 
 . ./common
 
@@ -32,6 +34,15 @@ setup_keys()
 		mkdir -p "$keypath"
 	fi
 
+	if [ -f "$masterpath" ]; then
+		mv "$masterpath" "$masterpath.bak"
+		$backup_key=1
+	fi
+	if [ -f "$keypath/tpm.handle" ]; then
+		mv "$keypath/tpm.handle" "$keypath/tmp.handle.bak"
+		$backup_handle=1
+	fi
+
 	dd if=/dev/urandom bs=1 count=32 2>/dev/null | keyctl padd user "$masterkey" @u
 	keyctl pipe "$(keyctl search @u user $masterkey)" > "$masterpath"
 }
@@ -53,6 +64,12 @@ test_cleanup()
 	if [ -f $masterpath ]; then
 		rm -f "$masterpath"
 	fi
+	if [ "$backup_key" -eq 1 ]; then
+		mv "$masterpath.bak" "$masterpath"
+	fi
+	if [ "$backup_handle" -eq 1 ]; then
+		mv "$keypath/tpm.handle.bak" "$keypath/tmp.handle"
+	fi
 }
 
 lock_dimm()
@@ -188,6 +205,33 @@ test_5_security_freeze()
 	fi
 }
 
+test_6_load_keys()
+{
+	if keyctl search @u encrypted nvdimm:"$id"; then
+		keyctl unlink "$(keyctl search @u encrypted nvdimm:"$id")"
+	fi
+
+	if keyctl search @u user "$masterkey"; then
+		keyctl unlink "$(keyctl search @u user $masterkey)"
+	fi
+
+	$NDCTL load-keys
+
+	if keyctl search @u user "$masterkey"; then
+		echo "master key loaded"
+	else
+		echo "master key fail to loaded"
+		err "$LINENO"
+	fi
+
+	if keyctl search @u encrypted nvdimm:"$id"; then
+		echo "dimm key loaded"
+	else
+		echo "dimm key failed to load"
+		err "$LINENO"
+	fi
+}
+
 check_min_kver "5.0" || do_skip "may lack security handling"
 uid="$(keyctl show | grep -Eo "_uid.[0-9]+" | head -1 | cut -d. -f2-)"
 if [ "$uid" -ne 0 ]; then
@@ -210,11 +254,15 @@ test_3_security_setup_and_erase
 echo "Test 4, unlock dimm"
 test_4_security_unlock
 
-# Freeze should always be run last because it locks security state and require
-# nfit_test module unload.
+# Freeze should always be run as last DIMM operation because it locks
+# security state and require nfit_test module unload.
 echo "Test 5, freeze security"
 test_5_security_freeze
 
+# this is purely on keyctl management and does not involve nvdimm
+echo "Test 6, test load-keys"
+test_6_load_keys
+
 test_cleanup
 _cleanup
 exit 0

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] ndctl: fix load-keys for user master-key

[Dan Williams]

On Thu, Mar 28, 2019 at 12:07 PM Dave Jiang <dave.jiang@intel.com> wrote:
>
> load-keys incorrectly assumes that all keys have TPM handles. TPM handle is
> only for trusted-keys. Fix in order to allow user master-key work.

s/work/to operate/ ?

>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  ndctl/load-keys.c |    6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/ndctl/load-keys.c b/ndctl/load-keys.c
> index 8e4998f2..19380152 100644
> --- a/ndctl/load-keys.c
> +++ b/ndctl/load-keys.c
> @@ -213,10 +213,8 @@ static int load_keys(struct loadkeys *lk_ctx, const char *keypath,
>
>         if (!tpmhandle) {
>                 rc = check_tpm_handle(lk_ctx);
> -               if (rc < 0) {
> -                       rc = -errno;
> -                       goto erropen;
> -               }
> +               if (rc < 0)
> +                       printf("No TPM handle discovered.\n");

Please keep error messages on stderr.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] ndctl: fix load-keys for user master-key

[Dave Jiang]

On 3/28/19 12:11 PM, Dan Williams wrote:
> On Thu, Mar 28, 2019 at 12:07 PM Dave Jiang <dave.jiang@intel.com> wrote:
>>
>> load-keys incorrectly assumes that all keys have TPM handles. TPM handle is
>> only for trusted-keys. Fix in order to allow user master-key work.
> 
> s/work/to operate/ ?
> 
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  ndctl/load-keys.c |    6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/ndctl/load-keys.c b/ndctl/load-keys.c
>> index 8e4998f2..19380152 100644
>> --- a/ndctl/load-keys.c
>> +++ b/ndctl/load-keys.c
>> @@ -213,10 +213,8 @@ static int load_keys(struct loadkeys *lk_ctx, const char *keypath,
>>
>>         if (!tpmhandle) {
>>                 rc = check_tpm_handle(lk_ctx);
>> -               if (rc < 0) {
>> -                       rc = -errno;
>> -                       goto erropen;
>> -               }
>> +               if (rc < 0)
>> +                       printf("No TPM handle discovered.\n");
> 
> Please keep error messages on stderr.

Actually this is info. At this point we do not have context as to if a
trusted key or user key being loaded, and it could be we are using user
master key or it could be TPM handle can't be found.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/3] ndctl: fix key blob loading for user keys

[Dan Williams]

On Thu, Mar 28, 2019 at 12:07 PM Dave Jiang <dave.jiang@intel.com> wrote:
>
> The syntax for loading user master key is different than loading a trusted
> key. Fix so we can load user key properly.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  ndctl/load-keys.c |   13 +++++--------
>  ndctl/util/keys.c |   20 +++++++++++++++-----
>  ndctl/util/keys.h |   10 ++++++++--
>  3 files changed, 28 insertions(+), 15 deletions(-)
>
> diff --git a/ndctl/load-keys.c b/ndctl/load-keys.c
> index 19380152..c0d0d743 100644
> --- a/ndctl/load-keys.c
> +++ b/ndctl/load-keys.c
> @@ -25,12 +25,7 @@ static struct parameters {
>         const char *tpm_handle;
>  } param;
>
> -enum key_type {
> -       KEY_USER = 0,
> -       KEY_TRUSTED,
> -};
> -
> -static const char *key_names[] = {"user", "trusted"};
> +static const char *key_names[] = {"user", "trusted", "encrypted"};
>
>  static struct loadkeys {
>         enum key_type key_type;
> @@ -44,6 +39,7 @@ static int load_master_key(struct loadkeys *lk_ctx, const char *keypath)
>         char *blob;
>         int size, rc;
>         char path[PATH_MAX];
> +       enum key_type;
>
>         rc = sprintf(path, "%s/nvdimm-master.blob", keypath);
>         if (rc < 0)
> @@ -65,7 +61,8 @@ static int load_master_key(struct loadkeys *lk_ctx, const char *keypath)
>                 return -errno;
>         }
>
> -       blob = ndctl_load_key_blob(path, &size, param.tpm_handle, -1);
> +       blob = ndctl_load_key_blob(path, &size, param.tpm_handle, -1,
> +                       lk_ctx->key_type);

Where is  lk_ctx->key_type set? I don't think I see any assignments in
this patch.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] ndctl: fix load-keys for user master-key

[Dan Williams]

On Thu, Mar 28, 2019 at 12:17 PM Dave Jiang <dave.jiang@intel.com> wrote:
>
>
>
> On 3/28/19 12:11 PM, Dan Williams wrote:
> > On Thu, Mar 28, 2019 at 12:07 PM Dave Jiang <dave.jiang@intel.com> wrote:
> >>
> >> load-keys incorrectly assumes that all keys have TPM handles. TPM handle is
> >> only for trusted-keys. Fix in order to allow user master-key work.
> >
> > s/work/to operate/ ?
> >
> >>
> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> >> ---
> >>  ndctl/load-keys.c |    6 ++----
> >>  1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/ndctl/load-keys.c b/ndctl/load-keys.c
> >> index 8e4998f2..19380152 100644
> >> --- a/ndctl/load-keys.c
> >> +++ b/ndctl/load-keys.c
> >> @@ -213,10 +213,8 @@ static int load_keys(struct loadkeys *lk_ctx, const char *keypath,
> >>
> >>         if (!tpmhandle) {
> >>                 rc = check_tpm_handle(lk_ctx);
> >> -               if (rc < 0) {
> >> -                       rc = -errno;
> >> -                       goto erropen;
> >> -               }
> >> +               if (rc < 0)
> >> +                       printf("No TPM handle discovered.\n");
> >
> > Please keep error messages on stderr.
>
> Actually this is info. At this point we do not have context as to if a
> trusted key or user key being loaded, and it could be we are using user
> master key or it could be TPM handle can't be found.

Ok, but it should still be stderr. ndctl tries to enforce only json on stdout.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/3] ndctl: fix key blob loading for user keys

[Dave Jiang]

On 3/28/19 12:18 PM, Dan Williams wrote:
> On Thu, Mar 28, 2019 at 12:07 PM Dave Jiang <dave.jiang@intel.com> wrote:
>>
>> The syntax for loading user master key is different than loading a trusted
>> key. Fix so we can load user key properly.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  ndctl/load-keys.c |   13 +++++--------
>>  ndctl/util/keys.c |   20 +++++++++++++++-----
>>  ndctl/util/keys.h |   10 ++++++++--
>>  3 files changed, 28 insertions(+), 15 deletions(-)
>>
>> diff --git a/ndctl/load-keys.c b/ndctl/load-keys.c
>> index 19380152..c0d0d743 100644
>> --- a/ndctl/load-keys.c
>> +++ b/ndctl/load-keys.c
>> @@ -25,12 +25,7 @@ static struct parameters {
>>         const char *tpm_handle;
>>  } param;
>>
>> -enum key_type {
>> -       KEY_USER = 0,
>> -       KEY_TRUSTED,
>> -};
>> -
>> -static const char *key_names[] = {"user", "trusted"};
>> +static const char *key_names[] = {"user", "trusted", "encrypted"};
>>
>>  static struct loadkeys {
>>         enum key_type key_type;
>> @@ -44,6 +39,7 @@ static int load_master_key(struct loadkeys *lk_ctx, const char *keypath)
>>         char *blob;
>>         int size, rc;
>>         char path[PATH_MAX];
>> +       enum key_type;
>>
>>         rc = sprintf(path, "%s/nvdimm-master.blob", keypath);
>>         if (rc < 0)
>> @@ -65,7 +61,8 @@ static int load_master_key(struct loadkeys *lk_ctx, const char *keypath)
>>                 return -errno;
>>         }
>>
>> -       blob = ndctl_load_key_blob(path, &size, param.tpm_handle, -1);
>> +       blob = ndctl_load_key_blob(path, &size, param.tpm_handle, -1,
>> +                       lk_ctx->key_type);
> 
> Where is  lk_ctx->key_type set? I don't think I see any assignments in
> this patch.
> 

In ndctl/load-keys.c:load_master_key() previously. It's not new.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 3/3] ndctl: add unit test for load-keys

[Verma, Vishal L]

On Thu, 2019-03-28 at 12:07 -0700, Dave Jiang wrote:
> Add to security.sh to test load-keys for user keys.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  test/security.sh |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/test/security.sh b/test/security.sh
> index 1b7a9a1a..7bd60293 100755
> --- a/test/security.sh
> +++ b/test/security.sh
> @@ -6,8 +6,10 @@ rc=77
>  dev=""
>  id=""
>  keypath="/etc/ndctl/keys"
> -masterkey="nvdimm-master-test"
> -masterpath="$keypath/$masterkey"
> +masterkey="nvdimm-master"
> +masterpath="$keypath/$masterkey.blob"
> +backup_key=0
> +backup_handle=0
>  
>  . ./common
>  
> @@ -32,6 +34,15 @@ setup_keys()
>  		mkdir -p "$keypath"
>  	fi
>  
> +	if [ -f "$masterpath" ]; then
> +		mv "$masterpath" "$masterpath.bak"
> +		$backup_key=1
> +	fi
> +	if [ -f "$keypath/tpm.handle" ]; then
> +		mv "$keypath/tpm.handle" "$keypath/tmp.handle.bak"
> +		$backup_handle=1
> +	fi
> +
>  	dd if=/dev/urandom bs=1 count=32 2>/dev/null | keyctl padd user "$masterkey" @u
>  	keyctl pipe "$(keyctl search @u user $masterkey)" > "$masterpath"
>  }
> @@ -53,6 +64,12 @@ test_cleanup()
>  	if [ -f $masterpath ]; then
>  		rm -f "$masterpath"
>  	fi
> +	if [ "$backup_key" -eq 1 ]; then
> +		mv "$masterpath.bak" "$masterpath"
> +	fi
> +	if [ "$backup_handle" -eq 1 ]; then
> +		mv "$keypath/tpm.handle.bak" "$keypath/tmp.handle"
> +	fi
>  }
>  
>  lock_dimm()
> @@ -188,6 +205,33 @@ test_5_security_freeze()
>  	fi
>  }
>  
> +test_6_load_keys()
> +{
> +	if keyctl search @u encrypted nvdimm:"$id"; then
> +		keyctl unlink "$(keyctl search @u encrypted nvdimm:"$id")"
> +	fi
> +
> +	if keyctl search @u user "$masterkey"; then
> +		keyctl unlink "$(keyctl search @u user $masterkey)"

missing quotes around "$masterkey"

> +	fi
> +
> +	$NDCTL load-keys
> +
> +	if keyctl search @u user "$masterkey"; then
> +		echo "master key loaded"
> +	else
> +		echo "master key fail to loaded"

'failed to load'

> +		err "$LINENO"
> +	fi
> +
> +	if keyctl search @u encrypted nvdimm:"$id"; then
> +		echo "dimm key loaded"
> +	else
> +		echo "dimm key failed to load"
> +		err "$LINENO"
> +	fi
> +}
> +
>  check_min_kver "5.0" || do_skip "may lack security handling"
>  uid="$(keyctl show | grep -Eo "_uid.[0-9]+" | head -1 | cut -d. -f2-)"
>  if [ "$uid" -ne 0 ]; then
> @@ -210,11 +254,15 @@ test_3_security_setup_and_erase
>  echo "Test 4, unlock dimm"
>  test_4_security_unlock
>  
> -# Freeze should always be run last because it locks security state and require
> -# nfit_test module unload.
> +# Freeze should always be run as last DIMM operation because it locks
> +# security state and require nfit_test module unload.

Should we just insert this new test as test_0 at the start?
a. That preserves the last-ness or test_5, and the comment above doesn't
get lost in the middle, and
b. keyctl is kind of core to any security operation, so it makes sense
to test out aspects of it first.

>  echo "Test 5, freeze security"
>  test_5_security_freeze
>  
> +# this is purely on keyctl management and does not involve nvdimm
> +echo "Test 6, test load-keys"
> +test_6_load_keys
> +
>  test_cleanup
>  _cleanup
>  exit 0
> 

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm