fix(attestation)!: label PCR values by the registers a quote attests - #90
Open
samlaf wants to merge 1 commit into
Open
fix(attestation)!: label PCR values by the registers a quote attests#90samlaf wants to merge 1 commit into
samlaf wants to merge 1 commit into
Conversation
A vTPM quote names the registers it covers in the pcrSelect bitmap inside the signed TPMS_ATTEST. The parser skipped that bitmap, so verification labelled the PCR values by their position in the accompanying list - the first value is PCR0, the second PCR1, and so on. That is only correct for a quote selecting a contiguous run from zero. The sending party chooses the selection, so it can choose one that shifts a favourable value into a pinned register's position. Take a machine whose PCR13 and PCR15 hold something the policy forbids. Azure leaves PCR16 and PCR23 at zero, so the machine quotes 0..=12, 16, 17, 23 instead: sixteen of its own unaltered values, a pcrDigest its vTPM computes over exactly those, and a valid AK signature. Counted off by position, PCR16's and PCR23's zeros land where the policy checks 13 and 15, and the machine is accepted. Nothing is forged and every cryptographic check passes. Read off the bitmap, 13 and 15 are not in the quote at all, and check_measurement refuses a pinned register it cannot find. Registers now come from the quote. tpms_attest decodes pcrSelect into ascending register numbers, and accepts exactly one PCR bank, which must be SHA-256 since the values travel as fixed-width [u8; 32]. verify returns the values already paired with their registers, so the pairing cannot be obtained without the checks that give it meaning - the digest comparison is also what fixes the order the values are read in. indexed_pcrs_unverified serves get_measurements, which verifies nothing and now says so in its name. A value list that does not fill its selection is refused rather than paired against a prefix, and a pcrSelect bitmap longer than four octets is refused rather than expanded - the spec bounds it by the TPM's own PCR count. Evidence produced by this repo was never affected: az-cvm-vtpm's get_quote always selects PCRs 0-23, where position and selection agree. Regression tests cover the crafted subset end to end, the fixture's pairing, a non-contiguous selection, a short value list, an oversized bitmap, a second bank and a non-SHA-256 bank. The differential test against tss-esapi now compares the selection alongside extraData and pcrDigest, so the field the labelling depends on is checked against an independent implementation. BREAKING CHANGE: MultiMeasurements::from_pcrs is replaced by from_indexed_pcrs; AttestError loses PcrSelectionCount and gains PcrSelectionBanks, PcrSelectionAlgorithm and PcrSelectionSize; TpmQuoteError gains PcrCountMismatch; TpmQuote::verify returns the PCR pairing instead of (). Closes flashbots#89
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #89
Note that this bug existed before my PR that vendored the parsing code: 7a3c5db
Having the parsing code actually made me discover the bug. the upstream azure library we were previously using to parse also doesnt return the pcrSelect registers, which is probably why we had that bug in the first place.
LLM SUMMARY
A vTPM quote names the registers it covers in the pcrSelect bitmap inside the signed TPMS_ATTEST. The parser skipped that bitmap, so verification labelled the PCR values by their position in the accompanying list - the first value is PCR0, the second PCR1, and so on. That is only correct for a quote selecting a contiguous run from zero.
The sending party chooses the selection, so it can choose one that shifts a favourable value into a pinned register's position. Take a machine whose PCR13 and PCR15 hold something the policy forbids. Azure leaves PCR16 and PCR23 at zero, so the machine quotes 0..=12, 16, 17, 23 instead: sixteen of its own unaltered values, a pcrDigest its vTPM computes over exactly those, and a valid AK signature. Counted off by position, PCR16's and PCR23's zeros land where the policy checks 13 and 15, and the machine is accepted. Nothing is forged and every cryptographic check passes. Read off the bitmap, 13 and 15 are not in the quote at all, and check_measurement refuses a pinned register it cannot find.
Registers now come from the quote. tpms_attest decodes pcrSelect into ascending register numbers, and accepts exactly one PCR bank, which must be SHA-256 since the values travel as fixed-width [u8; 32]. verify returns the values already paired with their registers, so the pairing cannot be obtained without the checks that give it meaning - the digest comparison is also what fixes the order the values are read in. indexed_pcrs_unverified serves get_measurements, which verifies nothing and now says so in its name.
A value list that does not fill its selection is refused rather than paired against a prefix, and a pcrSelect bitmap longer than four octets is refused rather than expanded - the spec bounds it by the TPM's own PCR count.
Evidence produced by this repo was never affected: az-cvm-vtpm's get_quote always selects PCRs 0-23, where position and selection agree.
Regression tests cover the crafted subset end to end, the fixture's pairing, a non-contiguous selection, a short value list, an oversized bitmap, a second bank and a non-SHA-256 bank. The differential test against tss-esapi now compares the selection alongside extraData and pcrDigest, so the field the labelling depends on is checked against an independent implementation.
BREAKING CHANGE: MultiMeasurements::from_pcrs is replaced by from_indexed_pcrs; AttestError loses PcrSelectionCount and gains PcrSelectionBanks, PcrSelectionAlgorithm and PcrSelectionSize; TpmQuoteError gains PcrCountMismatch; TpmQuote::verify returns the PCR pairing instead of ().