Make tpm2 quote functions accept PCR selections for multiple banks

[hansinator]

The TPM quote commands accept an array of PCR selections but the go-tpm library did not reflect this in its public API, so I added some brackets and dots here and there.

[jkl73]

Hi, PCRSelection struct already contains an array of PCR banks, you can use it with multiple PCRs like
tpm2.PCRSelection{Hash: tpm2HashAlgo, PCRs: []int{1,2,3}}

[hansinator]

@jkl73 Are you sure? Your example does contain an array of PCR indices, but no array for PCR banks. It requires an array of arrays, like this []tpm2.PCRSelection{{Hash: tpm2HashAlgo1, PCRs: []int{1,2,3}, {Hash: tpm2HashAlgo2, PCRs: []int{3,4,5}}.

The way the library represents the data structure in quote info right now is not matching the spec, but my PR does.

[jkl73]

Ah I see, I misunderstood. I was thinking multiple PCR indices... (instead of banks/hashes).
The Change LGTM, but it may break some downstream packages which use Quote or QuoteRaw

[hansinator]

Quote and QuoteRaw already have an unused parameter and accompanying comments that say to remove them on the next breaking API change. Release history shows that this library doesn't forever keep its API stable and the last point release had a breaking change. So I wanted to propose a more tidy but breaking solution first.

I could also change the PR and introduce something like QuoteEx and QuoteRawEx having the new signatures and let the other two functions forward to these.

[josephlr]

I agree that matching the spec here would be reasonable when we next make a breaking change to Quote/QuoteRaw/etc... I'm just not sure when that will be. We would probably want to clean up some other minor things at the same time (like the unused parameter @hansinator mentioned. @chrisfenner do we think we'll have another breaking change before the TPM-direct stuff is ready?

[chrisfenner]

Is there a use-case for quoting multiple banks of PCRs in one invocation? I agree with the doubt that we should introduce a breaking change here.

I've written about a related topic but sadly quoting multiple banks is only part of the solution to problems involving multiple PCR banks. I strongly think that TPM users should only have one bank of PCRs allocated at a time.

The Direct API could do this, @hansinator would you be interested in trying out that development branch? I'd be more than happy to add Quote and anything else you might need real quick (as adding commands there is, by design, fairly cheap).

[hansinator]

We are using the library in a production setting with customers. Unfortunately, we can't use a dev branch.

In practice our customers are having systems where
a) multiple PCR banks are active and hashed into
b) multiple banks are active but not with the same set of registers, some 2021 produced Dell servers f.e. have SHA2 active with no registers (thus no hashes, not an empty unextented register, but literally the registers are completely missing) and SHA1 active with a full set of registers

We can advise people to configure their UEFI settings correctly, but they typically won't want to when they deploy our software to their servers. So we have to work with what we get and that is the use-case for quoting multiple PCR banks. We have >10MB of extra data in a quote and we don't want round-trips with our remote attestation servers, so we need to quote everything there is in one go.

Personally, I feel a library should act as a tool and not impose policies, so I'd reason that way when thinking about making it possible to quote multiple PCR banks.

[chrisfenner]

@hansinator that seems reasonable, and we get you need to work with what you have. We would be in favor of making this change the next time we release a breaking change for go-tpm. I think that it will at least be a little bit before the Direct API is ready, so I don't want you to have to wait on that.

So I can get a sense of urgency here, what is the cost to calling tpm2.Quote twice (first with all the SHA1 PCRs, second with all the SHA256 PCRs)? It seemed like you had some additional costs on the verification side?

Re: this:

b) multiple banks are active but not with the same set of registers, some 2021 produced Dell servers f.e. have SHA2 active with no registers (thus no hashes, not an empty unextented register, but literally the registers are completely missing) and SHA1 active with a full set of registers

That's very weird to me, and it implies a firmware bug if not exactly the same issue I was worried about (CVE-2021-42299). I'd like to follow up about it, would you be willing to shoot me a mail?

[hansinator]

@chrisfenner We are prepared to use a soft-fork in the meantime, even if it takes a while.