Add ability to seal against user-specified PCR values

diabonas commented 4 years ago

Quoting README.md:

It is not yet possible to specify specific PCR values independent of the currently set PCR values. This would allow disabling the password-less calculate operation after booting the device. This makes most sense, once a TSS2 FAPI is available that will enable an interface to a canonical PCR event log.

Probably the most convenient way to handle this is to add a new option that accepts a file generated by tpm2_pcrlist --out-file.

Since this requires breaking changes to the library ABI, it would be good to implement this before releasing version 1.0.

AndreasFuchsTPM commented 4 years ago

I had though about just adding another symbol, i.e. tpm2totp_reseal_pcrvalue or similar ? For the library, I guess we need to just come up with a datatype for this. Input in tpm2-tools yaml but maybe also json would be good, I agree.

diabonas commented 4 years ago

I had though about just adding another symbol, i.e. tpm2totp_reseal_pcrvalue or similar ?

Oh yeah, that works as well, I like that this is backwards-compatible :)

For the library, I guess we need to just come up with a datatype for this.

Hm, something like

typedef struct {
    BYTE sha1[TPM2_MAX_PCRS][TPM2_SHA1_DIGEST_SIZE];
    BYTE sha256[TPM2_MAX_PCRS][TPM2_SHA256_DIGEST_SIZE];
    BYTE sha384[TPM2_MAX_PCRS][TPM2_SHA384_DIGEST_SIZE];
} PCR_HASHES;

in combination with the already existing uint32_t pcrs and uint32_t banks as masks to know which values should be used?

Input in tpm2-tools yaml but maybe also json would be good, I agree.

I guess we could also use our own file format and provide a script to convert the output from tpm2_pcrlist to the required format for convenience.

AndreasFuchsTPM commented 4 years ago

Hmmm... So far we've stayed out of the tpm2-types.h file for user convenience.

In a different context, I'm using

typedef struct {
    UINT32                                          pcr;
    TPM2_ALG_ID                                 hashAlg;
    TPMU_HA                                      digest;
} TPMS_PCRVALUE;

struct TPML_PCRVALUES {
    UINT32                                        count;
    TPMS_PCRVALUE                        authorizations[];
};

Rational is that I prefer the pcr selector and the pcr value to be close to each other, so code is less complex.

So without tpm-types.h this might become:

typedef struct {
    uint8_t                                          pcr;
    uint8_t                                          bank;
    uint8_t                                      digest[64];
} PCRVALUE;

typedef struct {
    size_t                                        count;
    PCRVALUE                                    pcrvalue[0];
} PCRVALUES;

The 0-length array allows for fitting allocations instead of allocating the max matrix size... Also easier to extend for upcoming sha3 banks or similar...

diabonas commented 4 years ago

Yeah, that works for me as well. Note that this is slightly more general than what we currently do for tpm2totp_generateKey/tpm2totp_reseal: this would allow a PCR specification like sha1:0+sha256:1 (so different sets of PCRs for every bank), while with tpm2totp_reseal, the same PCRs will always be used on all banks (e.g. sha1:0,1+sha256:0,1).

AndreasFuchsTPM commented 4 years ago

Good point... My original idea was to stick to KISS. So maybe you're right and we don't want to give that level of freedome.

I guess mixing the ideas, we could have some

typedef struct {
    uint32_t pcrs;   //bitmask
    uint32_t banks; //bitmask
    uint8_t   digests[0][64];  // or is is the other way around ?
} PCRVALUES;

We'd only need to specify if we cycle pcrs or banks first. Or would this here be too error prone ?

diabonas commented 4 years ago

We'd only need to specify if we cycle pcrs or banks first. Or would this here be too error prone ?

Hm, what I don't like about this solution is that you can't really telegraph or enforce the order convention in code, you really have to read the documentation in order to use it correctly. If we are fine with a fixed maximum number of 32 PCRs (which is the current maximum anyway, since the PCR bitmask is uint32_t) and don't mind a little memory overhead, I could see something like

typedef struct {
    uint32_t hash_algorithm;
    uint8_t pcrs[32 /*TPM2_MAX_PCRS*/][64 /* sizeof(TPMU_HA) */];
} PCRBANK;

typedef struct {
    uint32_t pcr_selection; // bitmask
    uint32_t bank_count;
    PCRBANK banks[];
} PCRVALUES;

Looks a bit clumsy, but it should hopefully be quite unambiguous how to use it. What do you think?

AndreasFuchsTPM commented 4 years ago

Banks / Bankalgs are a bitmask right now, corect ? So would hash_algorithm just be the same value as is usually used for the bitmask ? I'd hate to introduce a new enum for this stuff...

diabonas commented 4 years ago

Banks / Bankalgs are a bitmask right now, corect ? So would hash_algorithm just be the same value as is usually used for the bitmask ?

Indeed, I would just reuse TPM2TOTP_BANK_SHA* for the bitmask as the values for hash_algorithm.

I'd hate to introduce a new enum for this stuff...

I also thought about redefining these as an enum so that we could do

typedef struct {
    uint32_t pcrs;  // bitmask
    uint32_t banks; // bitmask
    uint8_t   digests[][32][64];
} PCRVALUES;

to be used like pcrvalues[TPM2TOTP_BANK_SHA256][0][0], but I think this is more cumbersome because you would then have to use pcrvalues.banks |= (1 << TPM2TOTP_BANK_SHA256) for setting the bitmask, which I think is by far the more common use case.

AndreasFuchsTPM commented 4 years ago

I don't think I understand what the third dimension in the array was for ? Look pretty much like https://github.com/tpm2-software/tpm2-totp/issues/51#issuecomment-513805197 otherwise...

diabonas commented 4 years ago

I don't think I understand what the third dimension in the array was for ?

The dimensions would be pcrvalues[<hash algorithm>][<pcr index>][<byte in the hash digest>] - the third dimension probably wouldn't be used directly, instead you would do something like memcpy(pcrvalues[TPM2TOTP_BANK_SHA256][0], digest, sizeof(digest)), but you need a byte field to store the hash value.

Look pretty much like #51 (comment) otherwise...

Indeed, the only differences are

"digests[][32][64]" automatically takes care of addressing the correct position to store the hash value, you don't have to know/look up in which order to loop through the banks and PCRs like you do for "digests[0][64]".
"digests[0][64]" uses a more compact memory layout where you only loop over the PCRs you actually use, whereas in "digests[][32][64]", you always allocate space for 32 PCRs in each bank regardless of how many are set in the PCR bitmask.
"digests[][32][64]" with the current TPM2TOTP_BANK_SHA* definition as powers of two would waste a lot of space since only digests[1], digests[2] and digest[4] are used at all. Therefore an enum would have to be used instead so that TPM2TOTP_BANK_SHA1=0, TPM2TOTP_BANK_SHA256=1, TPM2TOTP_BANK_SHA384=2.

AndreasFuchsTPM commented 4 years ago

pcrvalues[TPM2TOTP_BANK_SHA256] would be pcrvalues[2] so pcrvalue[1] would be empty ? I don't think I like skipping over entries in a var-length array, aka []... I think I like https://github.com/tpm2-software/tpm2-totp/issues/51#issuecomment-513866697 better...

So it's https://github.com/tpm2-software/tpm2-totp/issues/51#issuecomment-513866697 or https://github.com/tpm2-software/tpm2-totp/issues/51#issuecomment-513805197 I guess ?

diabonas commented 4 years ago

So it's #51 (comment) or #51 (comment) I guess ?

:+1: If I find the time, I'll try implementing at least the library function and see what feels better :)

tpm2-software / tpm2-totp

Add ability to seal against user-specified PCR values #51