Re: [Linux-ima-devel] [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend()

2017-05-30 Thread Roberto Sassu

On 5/30/2017 1:25 PM, Mimi Zohar wrote:

On Tue, 2017-05-30 at 09:28 +0200, Roberto Sassu wrote:

On 5/30/2017 5:29 AM, Mimi Zohar wrote:

On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:




@@ -876,29 +925,46 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int 
pcr_idx, const u8 *hash,
  * isn't, protect against the chip disappearing, by incrementing
  * the module usage count.
  */
-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
+int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
+  struct tpm2_digest *digests)
 {
int rc;
struct tpm_chip *chip;
struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
-   u32 count = 0;
+   struct tpm2_digest *digests_ptr = digests;
+   u32 filled_count = 0;
+   u8 *hash;
int i;

chip = tpm_chip_find_get(chip_num);
if (chip == NULL)
return -ENODEV;

-   if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+   rc = tpm_pcr_check_input(chip, count, digests);
+   if (rc < 0) {
+   dev_dbg(>dev, "%s: invalid arguments\n", __func__);
+   tpm_put_ops(chip);


This rejects the TPM extend, if ANY of the algorithms are unknown.
Suppose that the standards were updated, TPM vendors add support for
the new algorithm, but the kernel has not been updated to reflect the
new algorithms supported.  As the measurement hash already been added
to the IMA measurement list, verifying the measurement list against a
TPM quote will fail, not just for the unknown algorithm, but for all
algorithms.  Something is very broken with this approach.


The alternative is to extend remaining banks with a digest,
for example the first passed by the caller. I will modify
the patch, if everyone agrees on that.


The solution you're proposing is similar to the original solution of
extending the TPM with a padded/truncated SHA1 hash, but this time it
might not be a padded/truncated SHA1 hash, but a different algorithm.
So the attestation server will then need to know which hash algorithm
was used to extend each of the TPM banks - a padded/truncated digest
value or the real digest value.


The convention would be that the first digest in the event log
is truncated/padded, to extend banks for which no digest is
provided. This convention would apply to all callers of
tpm_pcr_extend().

Roberto



The only issue, from the kernel's perspective, will be determining the
algorithm's digest size as the kernel has no knowledge of it.  On TPM
registration/initialization, if you're not already querying the TPM
for the algorithm digest sizes, you will need to do so.

Mimi



--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG


Re: [Linux-ima-devel] [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend()

2017-05-30 Thread Roberto Sassu

On 5/30/2017 1:25 PM, Mimi Zohar wrote:

On Tue, 2017-05-30 at 09:28 +0200, Roberto Sassu wrote:

On 5/30/2017 5:29 AM, Mimi Zohar wrote:

On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:




@@ -876,29 +925,46 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int 
pcr_idx, const u8 *hash,
  * isn't, protect against the chip disappearing, by incrementing
  * the module usage count.
  */
-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
+int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
+  struct tpm2_digest *digests)
 {
int rc;
struct tpm_chip *chip;
struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
-   u32 count = 0;
+   struct tpm2_digest *digests_ptr = digests;
+   u32 filled_count = 0;
+   u8 *hash;
int i;

chip = tpm_chip_find_get(chip_num);
if (chip == NULL)
return -ENODEV;

-   if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+   rc = tpm_pcr_check_input(chip, count, digests);
+   if (rc < 0) {
+   dev_dbg(>dev, "%s: invalid arguments\n", __func__);
+   tpm_put_ops(chip);


This rejects the TPM extend, if ANY of the algorithms are unknown.
Suppose that the standards were updated, TPM vendors add support for
the new algorithm, but the kernel has not been updated to reflect the
new algorithms supported.  As the measurement hash already been added
to the IMA measurement list, verifying the measurement list against a
TPM quote will fail, not just for the unknown algorithm, but for all
algorithms.  Something is very broken with this approach.


The alternative is to extend remaining banks with a digest,
for example the first passed by the caller. I will modify
the patch, if everyone agrees on that.


The solution you're proposing is similar to the original solution of
extending the TPM with a padded/truncated SHA1 hash, but this time it
might not be a padded/truncated SHA1 hash, but a different algorithm.
So the attestation server will then need to know which hash algorithm
was used to extend each of the TPM banks - a padded/truncated digest
value or the real digest value.


The convention would be that the first digest in the event log
is truncated/padded, to extend banks for which no digest is
provided. This convention would apply to all callers of
tpm_pcr_extend().

Roberto



The only issue, from the kernel's perspective, will be determining the
algorithm's digest size as the kernel has no knowledge of it.  On TPM
registration/initialization, if you're not already querying the TPM
for the algorithm digest sizes, you will need to do so.

Mimi



--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG


Re: [Linux-ima-devel] [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend()

2017-05-30 Thread Mimi Zohar
On Tue, 2017-05-30 at 09:28 +0200, Roberto Sassu wrote:
> On 5/30/2017 5:29 AM, Mimi Zohar wrote:
> > On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:


> >> @@ -876,29 +925,46 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, 
> >> int pcr_idx, const u8 *hash,
> >>   * isn't, protect against the chip disappearing, by incrementing
> >>   * the module usage count.
> >>   */
> >> -int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> >> +int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
> >> + struct tpm2_digest *digests)
> >>  {
> >>int rc;
> >>struct tpm_chip *chip;
> >>struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
> >> -  u32 count = 0;
> >> +  struct tpm2_digest *digests_ptr = digests;
> >> +  u32 filled_count = 0;
> >> +  u8 *hash;
> >>int i;
> >>
> >>chip = tpm_chip_find_get(chip_num);
> >>if (chip == NULL)
> >>return -ENODEV;
> >>
> >> -  if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> >> +  rc = tpm_pcr_check_input(chip, count, digests);
> >> +  if (rc < 0) {
> >> +  dev_dbg(>dev, "%s: invalid arguments\n", __func__);
> >> +  tpm_put_ops(chip);
> >
> > This rejects the TPM extend, if ANY of the algorithms are unknown.
> > Suppose that the standards were updated, TPM vendors add support for
> > the new algorithm, but the kernel has not been updated to reflect the
> > new algorithms supported.  As the measurement hash already been added
> > to the IMA measurement list, verifying the measurement list against a
> > TPM quote will fail, not just for the unknown algorithm, but for all
> > algorithms.  Something is very broken with this approach.
> 
> The alternative is to extend remaining banks with a digest,
> for example the first passed by the caller. I will modify
> the patch, if everyone agrees on that.

The solution you're proposing is similar to the original solution of
extending the TPM with a padded/truncated SHA1 hash, but this time it
might not be a padded/truncated SHA1 hash, but a different algorithm.
So the attestation server will then need to know which hash algorithm
was used to extend each of the TPM banks - a padded/truncated digest
value or the real digest value.

The only issue, from the kernel's perspective, will be determining the
algorithm's digest size as the kernel has no knowledge of it.  On TPM
registration/initialization, if you're not already querying the TPM
for the algorithm digest sizes, you will need to do so.

Mimi



Re: [Linux-ima-devel] [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend()

2017-05-30 Thread Mimi Zohar
On Tue, 2017-05-30 at 09:28 +0200, Roberto Sassu wrote:
> On 5/30/2017 5:29 AM, Mimi Zohar wrote:
> > On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:


> >> @@ -876,29 +925,46 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, 
> >> int pcr_idx, const u8 *hash,
> >>   * isn't, protect against the chip disappearing, by incrementing
> >>   * the module usage count.
> >>   */
> >> -int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> >> +int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
> >> + struct tpm2_digest *digests)
> >>  {
> >>int rc;
> >>struct tpm_chip *chip;
> >>struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
> >> -  u32 count = 0;
> >> +  struct tpm2_digest *digests_ptr = digests;
> >> +  u32 filled_count = 0;
> >> +  u8 *hash;
> >>int i;
> >>
> >>chip = tpm_chip_find_get(chip_num);
> >>if (chip == NULL)
> >>return -ENODEV;
> >>
> >> -  if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> >> +  rc = tpm_pcr_check_input(chip, count, digests);
> >> +  if (rc < 0) {
> >> +  dev_dbg(>dev, "%s: invalid arguments\n", __func__);
> >> +  tpm_put_ops(chip);
> >
> > This rejects the TPM extend, if ANY of the algorithms are unknown.
> > Suppose that the standards were updated, TPM vendors add support for
> > the new algorithm, but the kernel has not been updated to reflect the
> > new algorithms supported.  As the measurement hash already been added
> > to the IMA measurement list, verifying the measurement list against a
> > TPM quote will fail, not just for the unknown algorithm, but for all
> > algorithms.  Something is very broken with this approach.
> 
> The alternative is to extend remaining banks with a digest,
> for example the first passed by the caller. I will modify
> the patch, if everyone agrees on that.

The solution you're proposing is similar to the original solution of
extending the TPM with a padded/truncated SHA1 hash, but this time it
might not be a padded/truncated SHA1 hash, but a different algorithm.
So the attestation server will then need to know which hash algorithm
was used to extend each of the TPM banks - a padded/truncated digest
value or the real digest value.

The only issue, from the kernel's perspective, will be determining the
algorithm's digest size as the kernel has no knowledge of it.  On TPM
registration/initialization, if you're not already querying the TPM
for the algorithm digest sizes, you will need to do so.

Mimi



Re: [Linux-ima-devel] [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend()

2017-05-30 Thread Roberto Sassu

On 5/30/2017 5:29 AM, Mimi Zohar wrote:

On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:

The tpm_pcr_extend() definition has been modified to take an array of
tpm2_digest structures, and the size of the array as arguments.

The function now checks if callers provided a digests for each active
PCR bank (or a SHA1 digest for TPM 1.2), to follow the recomendation from
the TCG specifications. See commit c1f92b4b04ad ("tpm: enhance TPM 2.0
PCR extend to support multiple banks"). All banks should be extended
because unused banks could be used by an attacker to hide the true
integrity status of the platform.

The only allowed exception to the rule above is to pass a SHA1 digest.
It has been introduced to maintain compatibility with applications that
expect to interact with a TPM 1.2, and provide only a SHA1 digest.
In this case, the behavior of tpm_pcr_extend() is unchanged and
remaining PCR banks are extended with that digest, padded with zeros.

Signed-off-by: Roberto Sassu 
---
v2

- tpm_pcr_extend() arguments checked by tpm_pcr_check_input()
- modified parameters of tpm_pcr_extend()

 drivers/char/tpm/tpm-interface.c | 76 +---
 drivers/char/tpm/tpm.h   |  6 
 include/linux/tpm.h  | 11 --
 3 files changed, 80 insertions(+), 13 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index aac703e..4b08b02 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -867,6 +867,55 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int 
pcr_idx, const u8 *hash,
 }

 /**
+ * tpm_pcr_check_input - check digests argument
+ *
+ * Return values:
+ *   1: input correct
+ *   0: fill digests with SHA1 digest padded with zeros
+ * -EINVAL: input incorrect
+ */
+static int tpm_pcr_check_input(struct tpm_chip *chip, int count,
+  struct tpm2_digest *digests)
+{
+   bool sha1_only;
+   int found = 0, not_found = 0;
+   int i, j;
+
+   if (count <= 0 || digests == NULL)
+   return -EINVAL;
+
+   sha1_only = (count == 1 && digests[0].alg_id == TPM2_ALG_SHA1);
+
+   if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
+   return sha1_only ? 1 : -EINVAL;
+
+   if (sha1_only)
+   return 0;
+
+   for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
+chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
+   for (j = 0; j < count; j++) {
+   if (digests[j].alg_id == chip->active_banks[i]) {
+   found++;
+   break;
+   }
+   }
+
+   if (j == count) {
+   dev_dbg(>dev, "%s: missing algorithm 0x%X\n",
+   __func__, chip->active_banks[i]);
+   not_found++;
+   }
+   }
+
+   if (not_found == 0 && found != count)
+   dev_dbg(>dev,
+   "%s: duplicate or unsupported algorithm\n", __func__);
+
+   return (not_found == 0 && found == count) ? 1 : -EINVAL;
+}
+
+/**
  * tpm_pcr_extend - extend pcr value with hash
  * @chip_num:  tpm idx # or AN&
  * @pcr_idx:   pcr idx to extend
@@ -876,29 +925,46 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int 
pcr_idx, const u8 *hash,
  * isn't, protect against the chip disappearing, by incrementing
  * the module usage count.
  */
-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
+int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
+  struct tpm2_digest *digests)
 {
int rc;
struct tpm_chip *chip;
struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
-   u32 count = 0;
+   struct tpm2_digest *digests_ptr = digests;
+   u32 filled_count = 0;
+   u8 *hash;
int i;

chip = tpm_chip_find_get(chip_num);
if (chip == NULL)
return -ENODEV;

-   if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+   rc = tpm_pcr_check_input(chip, count, digests);
+   if (rc < 0) {
+   dev_dbg(>dev, "%s: invalid arguments\n", __func__);
+   tpm_put_ops(chip);


This rejects the TPM extend, if ANY of the algorithms are unknown.
Suppose that the standards were updated, TPM vendors add support for
the new algorithm, but the kernel has not been updated to reflect the
new algorithms supported.  As the measurement hash already been added
to the IMA measurement list, verifying the measurement list against a
TPM quote will fail, not just for the unknown algorithm, but for all
algorithms.  Something is very broken with this approach.


The alternative is to extend remaining banks with a digest,
for example the first passed by the caller. I will modify
the patch, if everyone agrees on that.

Roberto




Mimi


+   return rc;
+   }
+
+   hash = 

Re: [Linux-ima-devel] [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend()

2017-05-30 Thread Roberto Sassu

On 5/30/2017 5:29 AM, Mimi Zohar wrote:

On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:

The tpm_pcr_extend() definition has been modified to take an array of
tpm2_digest structures, and the size of the array as arguments.

The function now checks if callers provided a digests for each active
PCR bank (or a SHA1 digest for TPM 1.2), to follow the recomendation from
the TCG specifications. See commit c1f92b4b04ad ("tpm: enhance TPM 2.0
PCR extend to support multiple banks"). All banks should be extended
because unused banks could be used by an attacker to hide the true
integrity status of the platform.

The only allowed exception to the rule above is to pass a SHA1 digest.
It has been introduced to maintain compatibility with applications that
expect to interact with a TPM 1.2, and provide only a SHA1 digest.
In this case, the behavior of tpm_pcr_extend() is unchanged and
remaining PCR banks are extended with that digest, padded with zeros.

Signed-off-by: Roberto Sassu 
---
v2

- tpm_pcr_extend() arguments checked by tpm_pcr_check_input()
- modified parameters of tpm_pcr_extend()

 drivers/char/tpm/tpm-interface.c | 76 +---
 drivers/char/tpm/tpm.h   |  6 
 include/linux/tpm.h  | 11 --
 3 files changed, 80 insertions(+), 13 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index aac703e..4b08b02 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -867,6 +867,55 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int 
pcr_idx, const u8 *hash,
 }

 /**
+ * tpm_pcr_check_input - check digests argument
+ *
+ * Return values:
+ *   1: input correct
+ *   0: fill digests with SHA1 digest padded with zeros
+ * -EINVAL: input incorrect
+ */
+static int tpm_pcr_check_input(struct tpm_chip *chip, int count,
+  struct tpm2_digest *digests)
+{
+   bool sha1_only;
+   int found = 0, not_found = 0;
+   int i, j;
+
+   if (count <= 0 || digests == NULL)
+   return -EINVAL;
+
+   sha1_only = (count == 1 && digests[0].alg_id == TPM2_ALG_SHA1);
+
+   if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
+   return sha1_only ? 1 : -EINVAL;
+
+   if (sha1_only)
+   return 0;
+
+   for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
+chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
+   for (j = 0; j < count; j++) {
+   if (digests[j].alg_id == chip->active_banks[i]) {
+   found++;
+   break;
+   }
+   }
+
+   if (j == count) {
+   dev_dbg(>dev, "%s: missing algorithm 0x%X\n",
+   __func__, chip->active_banks[i]);
+   not_found++;
+   }
+   }
+
+   if (not_found == 0 && found != count)
+   dev_dbg(>dev,
+   "%s: duplicate or unsupported algorithm\n", __func__);
+
+   return (not_found == 0 && found == count) ? 1 : -EINVAL;
+}
+
+/**
  * tpm_pcr_extend - extend pcr value with hash
  * @chip_num:  tpm idx # or AN&
  * @pcr_idx:   pcr idx to extend
@@ -876,29 +925,46 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int 
pcr_idx, const u8 *hash,
  * isn't, protect against the chip disappearing, by incrementing
  * the module usage count.
  */
-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
+int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
+  struct tpm2_digest *digests)
 {
int rc;
struct tpm_chip *chip;
struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
-   u32 count = 0;
+   struct tpm2_digest *digests_ptr = digests;
+   u32 filled_count = 0;
+   u8 *hash;
int i;

chip = tpm_chip_find_get(chip_num);
if (chip == NULL)
return -ENODEV;

-   if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+   rc = tpm_pcr_check_input(chip, count, digests);
+   if (rc < 0) {
+   dev_dbg(>dev, "%s: invalid arguments\n", __func__);
+   tpm_put_ops(chip);


This rejects the TPM extend, if ANY of the algorithms are unknown.
Suppose that the standards were updated, TPM vendors add support for
the new algorithm, but the kernel has not been updated to reflect the
new algorithms supported.  As the measurement hash already been added
to the IMA measurement list, verifying the measurement list against a
TPM quote will fail, not just for the unknown algorithm, but for all
algorithms.  Something is very broken with this approach.


The alternative is to extend remaining banks with a digest,
for example the first passed by the caller. I will modify
the patch, if everyone agrees on that.

Roberto




Mimi


+   return rc;
+   }
+
+   hash = digests[0].digest;
+
+   if (!rc) {

Re: [Linux-ima-devel] [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend()

2017-05-29 Thread Mimi Zohar
On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:
> The tpm_pcr_extend() definition has been modified to take an array of
> tpm2_digest structures, and the size of the array as arguments.
> 
> The function now checks if callers provided a digests for each active
> PCR bank (or a SHA1 digest for TPM 1.2), to follow the recomendation from
> the TCG specifications. See commit c1f92b4b04ad ("tpm: enhance TPM 2.0
> PCR extend to support multiple banks"). All banks should be extended
> because unused banks could be used by an attacker to hide the true
> integrity status of the platform.
> 
> The only allowed exception to the rule above is to pass a SHA1 digest.
> It has been introduced to maintain compatibility with applications that
> expect to interact with a TPM 1.2, and provide only a SHA1 digest.
> In this case, the behavior of tpm_pcr_extend() is unchanged and
> remaining PCR banks are extended with that digest, padded with zeros.
> 
> Signed-off-by: Roberto Sassu 
> ---
> v2
> 
> - tpm_pcr_extend() arguments checked by tpm_pcr_check_input()
> - modified parameters of tpm_pcr_extend()
> 
>  drivers/char/tpm/tpm-interface.c | 76 
> +---
>  drivers/char/tpm/tpm.h   |  6 
>  include/linux/tpm.h  | 11 --
>  3 files changed, 80 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c 
> b/drivers/char/tpm/tpm-interface.c
> index aac703e..4b08b02 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -867,6 +867,55 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int 
> pcr_idx, const u8 *hash,
>  }
> 
>  /**
> + * tpm_pcr_check_input - check digests argument
> + *
> + * Return values:
> + *   1: input correct
> + *   0: fill digests with SHA1 digest padded with zeros
> + * -EINVAL: input incorrect
> + */
> +static int tpm_pcr_check_input(struct tpm_chip *chip, int count,
> +struct tpm2_digest *digests)
> +{
> + bool sha1_only;
> + int found = 0, not_found = 0;
> + int i, j;
> +
> + if (count <= 0 || digests == NULL)
> + return -EINVAL;
> +
> + sha1_only = (count == 1 && digests[0].alg_id == TPM2_ALG_SHA1);
> +
> + if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
> + return sha1_only ? 1 : -EINVAL;
> +
> + if (sha1_only)
> + return 0;
> +
> + for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
> +  chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
> + for (j = 0; j < count; j++) {
> + if (digests[j].alg_id == chip->active_banks[i]) {
> + found++;
> + break;
> + }
> + }
> +
> + if (j == count) {
> + dev_dbg(>dev, "%s: missing algorithm 0x%X\n",
> + __func__, chip->active_banks[i]);
> + not_found++;
> + }
> + }
> +
> + if (not_found == 0 && found != count)
> + dev_dbg(>dev,
> + "%s: duplicate or unsupported algorithm\n", __func__);
> +
> + return (not_found == 0 && found == count) ? 1 : -EINVAL;
> +}
> +
> +/**
>   * tpm_pcr_extend - extend pcr value with hash
>   * @chip_num:tpm idx # or AN&
>   * @pcr_idx: pcr idx to extend
> @@ -876,29 +925,46 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int 
> pcr_idx, const u8 *hash,
>   * isn't, protect against the chip disappearing, by incrementing
>   * the module usage count.
>   */
> -int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> +int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
> +struct tpm2_digest *digests)
>  {
>   int rc;
>   struct tpm_chip *chip;
>   struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
> - u32 count = 0;
> + struct tpm2_digest *digests_ptr = digests;
> + u32 filled_count = 0;
> + u8 *hash;
>   int i;
> 
>   chip = tpm_chip_find_get(chip_num);
>   if (chip == NULL)
>   return -ENODEV;
> 
> - if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> + rc = tpm_pcr_check_input(chip, count, digests);
> + if (rc < 0) {
> + dev_dbg(>dev, "%s: invalid arguments\n", __func__);
> + tpm_put_ops(chip);

This rejects the TPM extend, if ANY of the algorithms are unknown.
Suppose that the standards were updated, TPM vendors add support for
the new algorithm, but the kernel has not been updated to reflect the
new algorithms supported.  As the measurement hash already been added
to the IMA measurement list, verifying the measurement list against a
TPM quote will fail, not just for the unknown algorithm, but for all
algorithms.  Something is very broken with this approach.

Mimi

> + return rc;
> + }
> +
> + hash = digests[0].digest;
> +
> + if (!rc) {
>   memset(digest_list, 0, 

Re: [Linux-ima-devel] [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend()

2017-05-29 Thread Mimi Zohar
On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:
> The tpm_pcr_extend() definition has been modified to take an array of
> tpm2_digest structures, and the size of the array as arguments.
> 
> The function now checks if callers provided a digests for each active
> PCR bank (or a SHA1 digest for TPM 1.2), to follow the recomendation from
> the TCG specifications. See commit c1f92b4b04ad ("tpm: enhance TPM 2.0
> PCR extend to support multiple banks"). All banks should be extended
> because unused banks could be used by an attacker to hide the true
> integrity status of the platform.
> 
> The only allowed exception to the rule above is to pass a SHA1 digest.
> It has been introduced to maintain compatibility with applications that
> expect to interact with a TPM 1.2, and provide only a SHA1 digest.
> In this case, the behavior of tpm_pcr_extend() is unchanged and
> remaining PCR banks are extended with that digest, padded with zeros.
> 
> Signed-off-by: Roberto Sassu 
> ---
> v2
> 
> - tpm_pcr_extend() arguments checked by tpm_pcr_check_input()
> - modified parameters of tpm_pcr_extend()
> 
>  drivers/char/tpm/tpm-interface.c | 76 
> +---
>  drivers/char/tpm/tpm.h   |  6 
>  include/linux/tpm.h  | 11 --
>  3 files changed, 80 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c 
> b/drivers/char/tpm/tpm-interface.c
> index aac703e..4b08b02 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -867,6 +867,55 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int 
> pcr_idx, const u8 *hash,
>  }
> 
>  /**
> + * tpm_pcr_check_input - check digests argument
> + *
> + * Return values:
> + *   1: input correct
> + *   0: fill digests with SHA1 digest padded with zeros
> + * -EINVAL: input incorrect
> + */
> +static int tpm_pcr_check_input(struct tpm_chip *chip, int count,
> +struct tpm2_digest *digests)
> +{
> + bool sha1_only;
> + int found = 0, not_found = 0;
> + int i, j;
> +
> + if (count <= 0 || digests == NULL)
> + return -EINVAL;
> +
> + sha1_only = (count == 1 && digests[0].alg_id == TPM2_ALG_SHA1);
> +
> + if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
> + return sha1_only ? 1 : -EINVAL;
> +
> + if (sha1_only)
> + return 0;
> +
> + for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
> +  chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
> + for (j = 0; j < count; j++) {
> + if (digests[j].alg_id == chip->active_banks[i]) {
> + found++;
> + break;
> + }
> + }
> +
> + if (j == count) {
> + dev_dbg(>dev, "%s: missing algorithm 0x%X\n",
> + __func__, chip->active_banks[i]);
> + not_found++;
> + }
> + }
> +
> + if (not_found == 0 && found != count)
> + dev_dbg(>dev,
> + "%s: duplicate or unsupported algorithm\n", __func__);
> +
> + return (not_found == 0 && found == count) ? 1 : -EINVAL;
> +}
> +
> +/**
>   * tpm_pcr_extend - extend pcr value with hash
>   * @chip_num:tpm idx # or AN&
>   * @pcr_idx: pcr idx to extend
> @@ -876,29 +925,46 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int 
> pcr_idx, const u8 *hash,
>   * isn't, protect against the chip disappearing, by incrementing
>   * the module usage count.
>   */
> -int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> +int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
> +struct tpm2_digest *digests)
>  {
>   int rc;
>   struct tpm_chip *chip;
>   struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
> - u32 count = 0;
> + struct tpm2_digest *digests_ptr = digests;
> + u32 filled_count = 0;
> + u8 *hash;
>   int i;
> 
>   chip = tpm_chip_find_get(chip_num);
>   if (chip == NULL)
>   return -ENODEV;
> 
> - if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> + rc = tpm_pcr_check_input(chip, count, digests);
> + if (rc < 0) {
> + dev_dbg(>dev, "%s: invalid arguments\n", __func__);
> + tpm_put_ops(chip);

This rejects the TPM extend, if ANY of the algorithms are unknown.
Suppose that the standards were updated, TPM vendors add support for
the new algorithm, but the kernel has not been updated to reflect the
new algorithms supported.  As the measurement hash already been added
to the IMA measurement list, verifying the measurement list against a
TPM quote will fail, not just for the unknown algorithm, but for all
algorithms.  Something is very broken with this approach.

Mimi

> + return rc;
> + }
> +
> + hash = digests[0].digest;
> +
> + if (!rc) {
>   memset(digest_list, 0, sizeof(digest_list));
> 
>