Proposed change to tpm driver tpm_pcr_extend

Proposed change to tpm driver tpm_pcr_extend

[Monty Wiseman]

+ Roberto, David
Currently tpm_pcr_extend takes only 20 octets and "expands" if necessary
to fill the size of other pcr banks. We want to make IMA algorithm agile.

Current interface:
int tpm_pcr_extend(struct tpm_chip *chip, int pcr-idx, const u8 *hash)
{
...
}

Proposed:
//Or should we use the defined constants from other libraries, for now
// I'll use the TCG defined constants
#define TPM_DIGEST_SIZE_SHA1   20
#define TPM_DIGEST_SIZE_SHA256 32
// Will need to define TCG algorithm IDs in TPM driver-specific header.
// Alternatively we could use the IDs from IMA / Linux and make the driver
// do the mapping to TCG alg IDs
...

int tpm_pcr_extend(struct tpm_chip *chip, int pcr-idx, struct *pcr_bank_list)
{
// The driver will take the contents of the pcr_bank_list and create a
// TPML_DIGEST_VALUES structure to pass into the TPM2_PCR_Extend command

// See comment below about mismatched pcr banks
}

// The caller will allocate a static set of octet arrays, one for each allocated
// pcr bank
struct pcr_bank_list
{
        uint16 tcg_hash_algid; // this is from the TCG algorthm registry
        uint8 hash_size; // size of octect array
        uint8 *extend_array; // pointer to array of octets to Extend.
        struct list_head pcr_bank;
}

// Caller allocates the following for each PCR bank. Each is pointed to by
// parameter extend_array
unit8[TPM_DIGEST_SIZE_SHA1] hash_sha1;
unit8[TPM_DIGEST_SIZE_SHA256] hash_sha256;
....

Mismatched PCR banks:
There is no programatic method now to enforce a match between
the caller's set of pcr banks and the TPM's allocated banks. For example,
the caller might only be configured to produce SHA1 hashes but the
platform owner (via a firmware setting) may have allocated PCR banks
for SHA1 and SHA256
Therefore, we propose the following as a starting point in the dicussion

1> If a PCR bank is allocated but there is no matching entry in the list of
pcr_bank_list for the pcr allocated bank:
      options:
              A> Append 0's or truncate based on a set of pre-defined rules
                  (See below)
              B> Extend the missing pcr bank with a constant
              C> Don't extend the missing bank.

Option A:
Append 0's or truncate in the following order
TPM_ALG_SHA1 <-> TPM_ALG_SHA256 <-> TPM_ALG_SHA3_256 <-> TPM_ALG_SHA384 <->
TPM_ALG_SHA3_384 <-> TPM_ALG_SHA512 <-> TPM_ALG_SHA3_512

The question is what to do about TPM_ALG_SM3_256?

Option C:
This is a viable option and may actully be what the caller wants. There actually
is no rule the all banks must be extended. In fact when "sealing", the
caller lists
the pcr banks they want to seal to. (While is it technically possible
to provide the
TPM2_PolicyPCR a mix of banks I don't beieve this practical as only
one expected hash
is provided as input. We should consider this option.

Further, there is no TPM spec requirement to allocate banks for all
PCRs. For example the
following is allowed:

PCR Allocated 1         2         3             4
SHA1 bank     allocated allocated allocated     allocated
SHA256 bank   allocated allocated Not allocated Not allocated

Monty

Re: Proposed change to tpm driver tpm_pcr_extend

[Monty Wiseman]

Adding Jarkko's real address.

On Wed, Oct 24, 2018 at 10:35 AM Monty Wiseman <montywiseman32@gmail.com> wrote:
>
> + Roberto, David
> Currently tpm_pcr_extend takes only 20 octets and "expands" if necessary
> to fill the size of other pcr banks. We want to make IMA algorithm agile.
>
> Current interface:
> int tpm_pcr_extend(struct tpm_chip *chip, int pcr-idx, const u8 *hash)
> {
> ...
> }
>
> Proposed:
> //Or should we use the defined constants from other libraries, for now
> // I'll use the TCG defined constants
> #define TPM_DIGEST_SIZE_SHA1   20
> #define TPM_DIGEST_SIZE_SHA256 32
> // Will need to define TCG algorithm IDs in TPM driver-specific header.
> // Alternatively we could use the IDs from IMA / Linux and make the driver
> // do the mapping to TCG alg IDs
> ...
>
> int tpm_pcr_extend(struct tpm_chip *chip, int pcr-idx, struct *pcr_bank_list)
> {
> // The driver will take the contents of the pcr_bank_list and create a
> // TPML_DIGEST_VALUES structure to pass into the TPM2_PCR_Extend command
>
> // See comment below about mismatched pcr banks
> }
>
> // The caller will allocate a static set of octet arrays, one for each allocated
> // pcr bank
> struct pcr_bank_list
> {
>         uint16 tcg_hash_algid; // this is from the TCG algorthm registry
>         uint8 hash_size; // size of octect array
>         uint8 *extend_array; // pointer to array of octets to Extend.
>         struct list_head pcr_bank;
> }
>
> // Caller allocates the following for each PCR bank. Each is pointed to by
> // parameter extend_array
> unit8[TPM_DIGEST_SIZE_SHA1] hash_sha1;
> unit8[TPM_DIGEST_SIZE_SHA256] hash_sha256;
> ....
>
> Mismatched PCR banks:
> There is no programatic method now to enforce a match between
> the caller's set of pcr banks and the TPM's allocated banks. For example,
> the caller might only be configured to produce SHA1 hashes but the
> platform owner (via a firmware setting) may have allocated PCR banks
> for SHA1 and SHA256
> Therefore, we propose the following as a starting point in the dicussion
>
> 1> If a PCR bank is allocated but there is no matching entry in the list of
> pcr_bank_list for the pcr allocated bank:
>       options:
>               A> Append 0's or truncate based on a set of pre-defined rules
>                   (See below)
>               B> Extend the missing pcr bank with a constant
>               C> Don't extend the missing bank.
>
> Option A:
> Append 0's or truncate in the following order
> TPM_ALG_SHA1 <-> TPM_ALG_SHA256 <-> TPM_ALG_SHA3_256 <-> TPM_ALG_SHA384 <->
> TPM_ALG_SHA3_384 <-> TPM_ALG_SHA512 <-> TPM_ALG_SHA3_512
>
> The question is what to do about TPM_ALG_SM3_256?
>
> Option C:
> This is a viable option and may actully be what the caller wants. There actually
> is no rule the all banks must be extended. In fact when "sealing", the
> caller lists
> the pcr banks they want to seal to. (While is it technically possible
> to provide the
> TPM2_PolicyPCR a mix of banks I don't beieve this practical as only
> one expected hash
> is provided as input. We should consider this option.
>
> Further, there is no TPM spec requirement to allocate banks for all
> PCRs. For example the
> following is allowed:
>
> PCR Allocated 1         2         3             4
> SHA1 bank     allocated allocated allocated     allocated
> SHA256 bank   allocated allocated Not allocated Not allocated
>
> Monty

Re: Proposed change to tpm driver tpm_pcr_extend

[Ken Goldman]

On 10/24/2018 5:35 AM, Monty Wiseman wrote:
> Option C:
> This is a viable option and may actully be what the caller wants. There actually
> is no rule the all banks must be extended. In fact when "sealing", the
> caller lists

Doesn't not extending a bank open the platform to attack?  Even
if one caller is sealing to one bank, other applications may use a 
different bank.  If that bank was no extended, the caller could extend 
counterfeit measurements and subvert an application.

IMHO, for PCRs that are doing software measurements, the rule should be 
that all allocated banks should be extended.

> the pcr banks they want to seal to. (While is it technically possible
> to provide the
> TPM2_PolicyPCR a mix of banks I don't believe this practical as only
> one expected hash
> is provided as input. We should consider this option.

I'm nearly sure that one can run TPM2_PolicyPCR with multiple banks.

1 - The input parameter pcrDigest is optional.  It permits the caller
to check for correct PCRs early in the policy process.  For example,
it could avoid an unnecessary digital signature or password prompt.

That's why policies should be constructed with policypcr before terms 
that require external input.

2 - The spec Part 1 describes the pcrDigest calculation, and I
don't see anything that mandates only one bank.