tcg: skip SignatureHeader vendor bytes in parseEfiSignatureList (hash injection)#34
Open
evilgensec wants to merge 1 commit into
Open
Conversation
… injection) parseEfiSignatureList iterated EFI_SIGNATURE_DATA entries starting at offset 0 instead of SignatureHeaderSize, so the vendor-specific SignatureHeader bytes (which per UEFI spec section 31.4.1 sit between the fixed header and the signature entries) were misread as signature entries. A crafted TPM/CCEL event log with zero real entries but attacker-chosen vendor bytes therefore injected arbitrary hashes/certs into the secure-boot db/dbx lists returned by UEFIVariableData.SignatureData(), which extract/secureboot.go uses to build the trusted SecureBootState - i.e. an attacker-controlled event log makes the verifier accept a forged secure-boot signature database. Same defect and threat model as GHSA-9r4w-jg96-92mv, fixed in google/go-attestation's copy of this function (PRs #502/#486/#500) but never propagated to this fork. Port the guards: reject SignatureListSize < 28, SignatureSize < 16, and SignatureSize/SignatureHeaderSize exceeding the remaining list space (uint32 underflow / ~4 GiB make), seek past the vendor header, and start both entry loops at SignatureHeaderSize. Adds a regression test.
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.
What
parseEfiSignatureList(tcg/events.go) iteratedEFI_SIGNATURE_DATAentries starting atsigOffset := 0instead ofSignatureHeaderSize, and never skipped the vendorSignatureHeaderbytes. Per the UEFI spec (§31.4.1),SignatureHeaderSizebytes of vendor data sit between the fixed list header and the actual signature entries and must be skipped.As a result, a crafted
EFI_SIGNATURE_LISTwith zero real entries but attacker-chosen vendor-header bytes has those bytes misread asEFI_SIGNATURE_DATAentries. Through the exportedUEFIVariableData.SignatureData()— whichextract/secureboot.gocalls forPK/KEK/db/dbxwhile buildingSecureBootStatefrom an attestation event log — this injects attacker-chosen hashes/certs into the trusted secure-boot signature database. The event log is attacker-controlled attestation evidence (e.g. a confidential-VM guest's own CCEL + RTMR), so a malicious attester can make the verifier accept a forged secure-boot db/dbx and still return success.Minimal reproduction (a list whose only content is a 16-byte owner GUID + a 32-byte attacker hash, declared entirely as the vendor header):
The missing
SignatureSize - 16/SignatureListSize - 28lower-bound guards additionally allowuint32underflow (infinite loop, or a ~4 GiBmake([]byte, ...)).Why
This is the same defect — and the same fix — as GHSA-9r4w-jg96-92mv in
google/go-attestation, whose copy ofparseEfiSignatureListreceived these guards in PRs #502 / #486 / #500. This repository forked the pre-patch version of the function and never picked them up. (go-attestation's patched loop isfor sigOffset := int(SignatureHeaderSize); ...with aSignatureHeaderSize >= remainingListSizeguard and abuf.Seek(SignatureHeaderSize); this fork still hadfor sigOffset := 0; ...with none. The sibling parserparseDevicePathElementin this same file does have a lower-bound guard, so the omission here is a genuine gap.)Fix
Port the guards: reject
SignatureListSize < 28,SignatureSize < 16, andSignatureSize/SignatureHeaderSizeexceeding the remaining list space;Seekpast the vendor header; and start both entry loops atSignatureHeaderSize. Adds a regression test asserting the vendor bytes are no longer returned as a trusted hash.Testing
go test ./tcg/... ./extract/...passes (existing real-event-log tests unaffected), plus the new regression test.