diff --git a/efi/preinstall/check_tcglog.go b/efi/preinstall/check_tcglog.go index a4106e10..1fab8404 100644 --- a/efi/preinstall/check_tcglog.go +++ b/efi/preinstall/check_tcglog.go @@ -34,13 +34,16 @@ import ( var ( // supportedAlgs specifies the supported PCR banks, in order of preference. - // XXX: We disallow SHA-1 here - perhaps this should be optionally permitted? supportedAlgs = []tpm2.HashAlgorithmId{ tpm2.HashAlgorithmSHA512, tpm2.HashAlgorithmSHA384, tpm2.HashAlgorithmSHA256, } + weakSupportedAlgs = []tpm2.HashAlgorithmId{ + tpm2.HashAlgorithmSHA1, + } + // supportedPcrs specifies all of the TCG defined PCRs, although we don't actually // support generating profiles for all of them at the moment, nor are we likely to // for some of them in the future. @@ -566,6 +569,46 @@ func (t *tcglogPhaseTracker) reachedOSPresent() bool { return t.phase == tcglogPhaseOSPresent } +func checkPCRBankNotEnabledAndEmpty(tpm *tpm2.TPMContext, alg tpm2.HashAlgorithmId) (emptyPcrs tpm2.HandleList, err error) { + pcrs, err := tpm.GetCapabilityPCRs() + if err != nil { + return nil, fmt.Errorf("cannot obtain active PCRs: %w", err) + } + for _, selection := range pcrs { + if selection.Hash != alg || len(selection.Select) == 0 { + continue + } + + // This bank is missing from the log but enabled on the TPM. Make sure that + // none of the TPM PCRs are not at their reset value - this is very bad for + // remote attestation (not so bad for FDE), but treat this as a serious error + // nonetheless. + var pcrs []int + for _, pcr := range supportedPcrs { + pcrs = append(pcrs, int(pcr)) + } + _, pcrValues, err := tpm.PCRRead(tpm2.PCRSelectionList{{Hash: alg, Select: pcrs}}) + if err != nil { + return nil, err + } + emptyDigest := make(tpm2.Digest, alg.Size()) + for pcr, digest := range pcrValues[alg] { + if bytes.Equal(digest, emptyDigest) { + emptyPcrs = append(emptyPcrs, tpm2.Handle(pcr)) + } + } + } + + return emptyPcrs, nil +} + +type checkFirmwareLogFlags int + +const ( + checkFirmwareLogPermitEmptyPCRBanks checkFirmwareLogFlags = 1 << iota + checkFirmwareLogPermitWeakPCRBanks +) + // checkFirmwareLogAndChoosePCRBank verifies that the firmware TCG log is in crypto-agile form and // consistent with at least one supported PCR bank for the specified mandatory PCRs when reconstructed // for the TCG defined PCRs (0-7). It also ensures that: @@ -578,7 +621,7 @@ func (t *tcglogPhaseTracker) reachedOSPresent() bool { // PCRs set, but the errors will be accessible on the returned results struct. // // The returned results struct indicates the best PCR bank to use and specifies the TPM startup locality as well. -func checkFirmwareLogAndChoosePCRBank(tpm *tpm2.TPMContext, log *tcglog.Log, mandatoryPcrs tpm2.HandleList, permitEmptyPCRBanks bool) (results *pcrBankResults, err error) { +func checkFirmwareLogAndChoosePCRBank(tpm *tpm2.TPMContext, log *tcglog.Log, mandatoryPcrs tpm2.HandleList, flags checkFirmwareLogFlags) (results *pcrBankResults, err error) { // Make sure it's a crypto-agile log if !log.Spec.IsEFI_2() { return nil, errors.New("invalid log spec") @@ -588,42 +631,51 @@ func checkFirmwareLogAndChoosePCRBank(tpm *tpm2.TPMContext, log *tcglog.Log, man // likely to get SHA-256 here - it's only in very recent devices that we have TPMs with // SHA-384 support and corresponding firmware integration. // We try to keep all errors enountered during selection here. + + // Instantiate and maintain an instance of NoSuitablePCRAlgorithmError + // to return later if appropriate. mainErr := &NoSuitablePCRAlgorithmError{ BankErrs: make(map[tpm2.HashAlgorithmId]error), PCRErrs: make(map[tpm2.HashAlgorithmId]map[tpm2.Handle]error), } - var chosenResults *pcrBankResults - for _, alg := range supportedAlgs { - if chosenResults != nil { - // We've already got a good PCR bank, so no need to carry on. - break - } + // Instantiate and maintain an instance of EmptyPCRBanks to return later + // if approriate. + emptyBanksErr := new(EmptyPCRBanksError) + + testAlgs := make([]tpm2.HashAlgorithmId, len(supportedAlgs)) + copy(testAlgs, supportedAlgs) + + if flags&checkFirmwareLogPermitWeakPCRBanks > 0 { + testAlgs = append(testAlgs, weakSupportedAlgs...) + } + + var chosenResults *pcrBankResults + for _, alg := range testAlgs { results, err := checkFirmwareLogAgainstTPMForAlg(tpm, log, alg, mandatoryPcrs) switch { case errors.Is(err, ErrPCRBankMissingFromLog): - if !permitEmptyPCRBanks { - // Make sure that the TPM PCR bank is not enabled - pcrs, err := tpm.GetCapabilityPCRs() - if err != nil { - return nil, fmt.Errorf("cannot obtain active PCRs: %w", err) - } - for _, selection := range pcrs { - if selection.Hash == alg && len(selection.Select) > 0 { - // This bank is missing from the log but enabled on the TPM. - // This is very bad for remote attestation (not so bad for FDE), but treat - // this as a serious error nonetheless. - return nil, &EmptyPCRBankError{alg} - } + if flags&checkFirmwareLogPermitEmptyPCRBanks == 0 { + // Make sure that the TPM PCR bank is not enabled, and if it is, + // that it doesn't contain any empty PCRs. + emptyPcrs, err := checkPCRBankNotEnabledAndEmpty(tpm, alg) + switch { + case err != nil: + emptyBanksErr.Errs = append(emptyBanksErr.Errs, fmt.Errorf("cannot determine whether PCR bank %v is active but empty on the TPM: %w", alg, err)) + case len(emptyPcrs) > 0: + emptyBanksErr.Algs = append(emptyBanksErr.Algs, alg) } } fallthrough case err != nil: // This entire bank is bad mainErr.setBankErr(alg, err) - case results.Ok(): - // This is a good PCR bank + case results.Ok() && chosenResults == nil: + // This will be the best PCR bank chosenResults = results + case results.Ok(): + // We've already chosen a PCR bank, so do nothing in this case. + // All we're doing now is looking for empty banks. default: // This isn't a good PCR bank because some mandatory PCRs // failed. Record the individual PCR errors. @@ -631,6 +683,14 @@ func checkFirmwareLogAndChoosePCRBank(tpm *tpm2.TPMContext, log *tcglog.Log, man } } + // Bail early if there are any active but empty TPM PCR banks that are missing + // from the log, or if we encountered any errors whilst checking for this, in + // order to prioritise this error. We return this even if we found a good PCR + // bank. + if len(emptyBanksErr.Algs) > 0 || len(emptyBanksErr.Errs) > 0 { + return nil, emptyBanksErr + } + if chosenResults == nil { // No suitable PCR bank was found, so return an error that's hopefully useful :( return nil, mainErr diff --git a/efi/preinstall/check_tcglog_test.go b/efi/preinstall/check_tcglog_test.go index 591c396c..b36eb1ed 100644 --- a/efi/preinstall/check_tcglog_test.go +++ b/efi/preinstall/check_tcglog_test.go @@ -20,11 +20,13 @@ package preinstall_test import ( + "bytes" "crypto" "errors" "github.com/canonical/go-tpm2" "github.com/canonical/go-tpm2/mssim" + "github.com/canonical/go-tpm2/mu" tpm2_testutil "github.com/canonical/go-tpm2/testutil" "github.com/canonical/tcglog-parser" . "github.com/snapcore/secboot/efi/preinstall" @@ -222,7 +224,7 @@ type testCheckFirmwareLogAndChoosePCRBankParams struct { startupLocality uint8 disallowPreOSVerification bool mandatoryPcrs tpm2.HandleList - permitEmptyPCRBanks bool + flags CheckFirmwareLogFlags expectedAlg tpm2.HashAlgorithmId } @@ -236,7 +238,7 @@ func (s *tcglogSuite) testCheckFirmwareLogAndChoosePCRBank(c *C, params *testChe DisallowPreOSVerification: params.disallowPreOSVerification, }) s.resetTPMAndReplayLog(c, log, params.logAlgs...) - result, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, params.mandatoryPcrs, params.permitEmptyPCRBanks) + result, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, params.mandatoryPcrs, params.flags) c.Assert(err, IsNil) c.Check(result.Alg, Equals, params.expectedAlg) c.Check(result.StartupLocality, Equals, params.startupLocality) @@ -298,6 +300,25 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankSHA512(c *C) { }) } +func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankSHA1(c *C) { + s.testCheckFirmwareLogAndChoosePCRBank(c, &testCheckFirmwareLogAndChoosePCRBankParams{ + enabledBanks: []tpm2.HashAlgorithmId{tpm2.HashAlgorithmSHA1}, + logAlgs: []tpm2.HashAlgorithmId{tpm2.HashAlgorithmSHA1}, + mandatoryPcrs: tpm2.HandleList{ + internal_efi.PlatformFirmwarePCR, + internal_efi.PlatformConfigPCR, + internal_efi.DriversAndAppsPCR, + internal_efi.DriversAndAppsConfigPCR, + internal_efi.BootManagerCodePCR, + internal_efi.BootManagerConfigPCR, + internal_efi.PlatformManufacturerPCR, + internal_efi.SecureBootPolicyPCR, + }, + flags: CheckFirmwareLogPermitWeakPCRBanks, + expectedAlg: tpm2.HashAlgorithmSHA1, + }) +} + func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankMultipleSHA384(c *C) { s.RequireAlgorithm(c, tpm2.AlgorithmSHA384) s.testCheckFirmwareLogAndChoosePCRBank(c, &testCheckFirmwareLogAndChoosePCRBankParams{ @@ -332,8 +353,8 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankSHA256WithEmptySHA384B internal_efi.PlatformManufacturerPCR, internal_efi.SecureBootPolicyPCR, }, - permitEmptyPCRBanks: true, - expectedAlg: tpm2.HashAlgorithmSHA256, + flags: CheckFirmwareLogPermitEmptyPCRBanks, + expectedAlg: tpm2.HashAlgorithmSHA256, }) } @@ -454,7 +475,7 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankUnexpectedStartupLocal _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, tpm2.HandleList{ internal_efi.PlatformFirmwarePCR, - }, false) + }, 0) c.Check(err, ErrorMatches, `no suitable PCR algorithm available: - TPM_ALG_SHA512: the PCR bank is missing from the TCG log. - TPM_ALG_SHA384: the PCR bank is missing from the TCG log. @@ -533,7 +554,7 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankOutOfPlaceStartupLocal _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, tpm2.HandleList{ internal_efi.PlatformFirmwarePCR, - }, false) + }, 0) c.Check(err, ErrorMatches, `no suitable PCR algorithm available: - TPM_ALG_SHA512: the PCR bank is missing from the TCG log. - TPM_ALG_SHA384: the PCR bank is missing from the TCG log. @@ -576,7 +597,7 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankInvalidStartupLocality _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, tpm2.HandleList{ internal_efi.PlatformFirmwarePCR, - }, false) + }, 0) c.Check(err, ErrorMatches, `no suitable PCR algorithm available: - TPM_ALG_SHA512: the PCR bank is missing from the TCG log. - TPM_ALG_SHA384: the PCR bank is missing from the TCG log. @@ -602,7 +623,7 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankPCRMismatchMandatory(c }) _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, tpm2.HandleList{ internal_efi.PlatformFirmwarePCR, - }, false) + }, 0) c.Check(err, ErrorMatches, `no suitable PCR algorithm available: - TPM_ALG_SHA512: the PCR bank is missing from the TCG log. - TPM_ALG_SHA384: the PCR bank is missing from the TCG log. @@ -636,7 +657,7 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankPCRMismatchNonMandator internal_efi.PlatformManufacturerPCR, internal_efi.SecureBootPolicyPCR, }, - false, + 0, ) c.Assert(err, IsNil) c.Check(results.Alg, Equals, tpm2.HashAlgorithmSHA256) @@ -676,7 +697,7 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankPCRMismatchMandatoryIn internal_efi.PlatformManufacturerPCR, internal_efi.SecureBootPolicyPCR, }, - false, + 0, ) c.Assert(err, IsNil) c.Check(results.Alg, Equals, tpm2.HashAlgorithmSHA256) @@ -712,7 +733,7 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankPCRMismatchNonMandator internal_efi.PlatformManufacturerPCR, internal_efi.SecureBootPolicyPCR, }, - false, + 0, ) c.Assert(err, IsNil) c.Check(results.Alg, Equals, tpm2.HashAlgorithmSHA384) @@ -771,7 +792,7 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankSecureBootConfigJumpsT internal_efi.PlatformManufacturerPCR, internal_efi.SecureBootPolicyPCR, }, - false, + 0, ) c.Assert(err, IsNil) c.Check(results.Alg, Equals, tpm2.HashAlgorithmSHA256) @@ -788,7 +809,7 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankBadSpec(c *C) { Minor: 2, Errata: 0, } - _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, nil, false) + _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, nil, 0) c.Check(err, ErrorMatches, `invalid log spec`) } @@ -821,7 +842,7 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankPreOSMeasurementToNonT log.Events = eventsCopy s.resetTPMAndReplayLog(c, log, tpm2.HashAlgorithmSHA256) - _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, nil, false) + _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, nil, 0) c.Check(err, ErrorMatches, `measurements were made by firmware from pre-OS environment to non-TCG defined PCR 8`) } @@ -845,7 +866,7 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankSeparatorDecodeError(c } s.resetTPMAndReplayLog(c, log, tpm2.HashAlgorithmSHA256) - _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, nil, false) + _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, nil, 0) c.Check(err, ErrorMatches, `invalid event data for EV_SEPARATOR event in PCR 7: some error`) } @@ -869,7 +890,7 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankSeparatorError(c *C) { } s.resetTPMAndReplayLog(c, log, tpm2.HashAlgorithmSHA256) - _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, nil, false) + _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, nil, 0) c.Check(err, ErrorMatches, `EV_SEPARATOR event for PCR 7 indicates an error occurred \(error code in log: 67305985\)`) } @@ -905,7 +926,7 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankUnexpectedSuccessfulSe s.resetTPMAndReplayLog(c, log, tpm2.HashAlgorithmSHA256) - _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, nil, false) + _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, nil, 0) c.Check(err, ErrorMatches, `unexpected normal EV_SEPARATOR event in PCR 0`) } @@ -934,7 +955,7 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankMissingSeparators(c *C log.Events = eventsCopy s.resetTPMAndReplayLog(c, log, tpm2.HashAlgorithmSHA256) - _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, nil, false) + _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, nil, 0) c.Check(err, ErrorMatches, `unexpected EV_EFI_VARIABLE_AUTHORITY event in PCR 7 whilst transitioning to OS-present \(expected EV_SEPARATOR\)`) } @@ -960,7 +981,7 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankMultipleSeparatorsForS log.Events = eventsCopy s.resetTPMAndReplayLog(c, log, tpm2.HashAlgorithmSHA256) - _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, nil, false) + _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, nil, 0) c.Check(err, ErrorMatches, `more than one EV_SEPARATOR event exists for PCR 7`) } @@ -984,11 +1005,13 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankTruncatedLog(c *C) { log.Events = eventsCopy s.resetTPMAndReplayLog(c, log, tpm2.HashAlgorithmSHA256) - _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, nil, false) + _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, nil, 0) c.Check(err, ErrorMatches, `reached the end of the log without seeing EV_SEPARATOR events in all TCG defined PCRs`) } -func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankEmptyPCRBankNotAllowed(c *C) { +func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankEmptyPCRBanksNotAllowed(c *C) { + // Test that EmptyPCRBanksError works and that it takes precedence over + // NoSuitablePCRAlgoithmError by making PCR0 mandatory and invalid for one bank. s.RequireAlgorithm(c, tpm2.AlgorithmSHA384) s.allocatePCRBanks(c, tpm2.HashAlgorithmSHA256, tpm2.HashAlgorithmSHA384) @@ -1012,17 +1035,19 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankEmptyPCRBankNotAllowed internal_efi.PlatformManufacturerPCR, internal_efi.SecureBootPolicyPCR, }, - false, + 0, ) - c.Check(err, ErrorMatches, `the PCR bank for TPM_ALG_SHA384 is missing from the TCG log but is active on the TPM`) + c.Check(err, ErrorMatches, `the PCR bank for TPM_ALG_SHA384 is missing from the TCG log but active and with one or more empty PCRs on the TPM`) - var emptyPCRErr *EmptyPCRBankError + var emptyPCRErr *EmptyPCRBanksError c.Check(errors.As(err, &emptyPCRErr), testutil.IsTrue) } -func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankEmptyPCRBankAllowed(c *C) { +func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankMultipleEmptyPCRBanksNotAllowed(c *C) { + // Test that EmptyPCRBanksError for multiple PCRs works. In this case, there + // is a good bank, but this error takes precedence. s.RequireAlgorithm(c, tpm2.AlgorithmSHA384) - s.allocatePCRBanks(c, tpm2.HashAlgorithmSHA256, tpm2.HashAlgorithmSHA384) + s.allocatePCRBanks(c, tpm2.HashAlgorithmSHA1, tpm2.HashAlgorithmSHA256, tpm2.HashAlgorithmSHA384) log := efitest.NewLog(c, &efitest.LogOptions{ Algorithms: []tpm2.HashAlgorithmId{tpm2.HashAlgorithmSHA256}, @@ -1034,9 +1059,9 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankEmptyPCRBankAllowed(c log = efitest.NewLog(c, &efitest.LogOptions{ Algorithms: []tpm2.HashAlgorithmId{tpm2.HashAlgorithmSHA256}, }) - const permitEmptyPCRBanks = true _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, tpm2.HandleList{ + internal_efi.PlatformFirmwarePCR, internal_efi.PlatformConfigPCR, internal_efi.DriversAndAppsPCR, internal_efi.DriversAndAppsConfigPCR, @@ -1045,12 +1070,18 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankEmptyPCRBankAllowed(c internal_efi.PlatformManufacturerPCR, internal_efi.SecureBootPolicyPCR, }, - permitEmptyPCRBanks, + CheckFirmwareLogPermitWeakPCRBanks, ) - c.Assert(err, IsNil) + c.Check(err, ErrorMatches, `the PCR banks for TPM_ALG_SHA384, TPM_ALG_SHA1 are missing from the TCG log but active and with one or more empty PCRs on the TPM`) + + var emptyPCRErr *EmptyPCRBanksError + c.Check(errors.As(err, &emptyPCRErr), testutil.IsTrue) } -func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankSHA256WithEmptySHA384BankBad(c *C) { +func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankEmptyPCRBanksError(c *C) { + // Test the case where we get a TPM response error when testing if a PCR + // bank has empty PCRs, and make sure that the error takes precedence over + // the one good bank. s.RequireAlgorithm(c, tpm2.AlgorithmSHA384) s.allocatePCRBanks(c, tpm2.HashAlgorithmSHA256, tpm2.HashAlgorithmSHA384) @@ -1060,6 +1091,24 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankSHA256WithEmptySHA384B }) s.resetTPMAndReplayLog(c, log, tpm2.HashAlgorithmSHA256) + s.TPMSimulatorTest.Transport.ResponseIntercept = func(cmdCode tpm2.CommandCode, cmdHandles tpm2.HandleList, cmdAuthArea []tpm2.AuthCommand, cpBytes []byte, rsp *bytes.Buffer) { + if cmdCode != tpm2.CommandGetCapability { + // The only TPM2_GetCapability call is to obtain informatio + // about active PCR banks. + return + } + + hdr := tpm2.ResponseHeader{ + Tag: tpm2.TagNoSessions, + ResponseSize: 10, + ResponseCode: tpm2.ResponseBadTag, + } + rsp.Reset() + _, err := mu.MarshalToWriter(rsp, &hdr) + c.Check(err, IsNil) + } + defer func() { s.TPMSimulatorTest.Transport.ResponseIntercept = nil }() + _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, tpm2.HandleList{ internal_efi.PlatformConfigPCR, @@ -1070,10 +1119,50 @@ func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankSHA256WithEmptySHA384B internal_efi.PlatformManufacturerPCR, internal_efi.SecureBootPolicyPCR, }, - false, + 0, ) - c.Check(err, ErrorMatches, `the PCR bank for TPM_ALG_SHA384 is missing from the TCG log but is active on the TPM`) + c.Check(err, ErrorMatches, `one or more errors detected when trying to determine whether PCR banks missing from the TCG log are enabled with empty PCRs: +- cannot determine whether PCR bank TPM_ALG_SHA512 is active but empty on the TPM: cannot obtain active PCRs: TPM returned a TPM_RC_BAD_TAG error whilst executing command TPM_CC_GetCapability +- cannot determine whether PCR bank TPM_ALG_SHA384 is active but empty on the TPM: cannot obtain active PCRs: TPM returned a TPM_RC_BAD_TAG error whilst executing command TPM_CC_GetCapability +`) + + var emptyPCRErr *EmptyPCRBanksError + c.Check(errors.As(err, &emptyPCRErr), testutil.IsTrue) +} - var bankErr *EmptyPCRBankError - c.Check(errors.As(err, &bankErr), testutil.IsTrue) +func (s *tcglogSuite) TestCheckFirmwareLogAndChoosePCRBankBadSHA1(c *C) { + s.allocatePCRBanks(c, tpm2.HashAlgorithmSHA1) + + log := efitest.NewLog(c, &efitest.LogOptions{ + Algorithms: []tpm2.HashAlgorithmId{tpm2.HashAlgorithmSHA1}, + StartupLocality: 3, + }) + s.resetTPMAndReplayLog(c, log, tpm2.HashAlgorithmSHA1) + + _, err := CheckFirmwareLogAndChoosePCRBank(s.TPM, log, + tpm2.HandleList{ + internal_efi.PlatformFirmwarePCR, + internal_efi.PlatformConfigPCR, + internal_efi.DriversAndAppsPCR, + internal_efi.DriversAndAppsConfigPCR, + internal_efi.BootManagerCodePCR, + internal_efi.BootManagerConfigPCR, + internal_efi.PlatformManufacturerPCR, + internal_efi.SecureBootPolicyPCR, + }, + 0, + ) + c.Check(err, ErrorMatches, `no suitable PCR algorithm available: +- TPM_ALG_SHA512: the PCR bank is missing from the TCG log. +- TPM_ALG_SHA384: the PCR bank is missing from the TCG log. +- TPM_ALG_SHA256: the PCR bank is missing from the TCG log. +`) + + var e *NoSuitablePCRAlgorithmError + c.Assert(errors.As(err, &e), testutil.IsTrue) + + // Test that we can access individual errors. + c.Check(e.BankErrs[tpm2.HashAlgorithmSHA512], Equals, ErrPCRBankMissingFromLog) + c.Check(e.BankErrs[tpm2.HashAlgorithmSHA384], Equals, ErrPCRBankMissingFromLog) + c.Check(e.BankErrs[tpm2.HashAlgorithmSHA256], Equals, ErrPCRBankMissingFromLog) } diff --git a/efi/preinstall/checks.go b/efi/preinstall/checks.go index 3e049cc2..6b836b84 100644 --- a/efi/preinstall/checks.go +++ b/efi/preinstall/checks.go @@ -74,6 +74,12 @@ const ( // PCR 7 is not optional. SecureBootPolicyProfileSupportRequired + // PermitWeakPCRBanks permits selecting a weak PCR algorithm if + // no other valid ones are available. This currently only includes + // SHA1. Without this, RunChecks will only test for SHA2-512, SHA2-384, + // and SHA2-256. + PermitWeakPCRBanks + // PostInstallChecks indicates that the tests are being executed // post-install as opposed to pre-install. PostInstallChecks @@ -242,14 +248,21 @@ func RunChecks(ctx context.Context, flags CheckFlags, loadedImages []secboot_efi mandatoryPcrs = append(mandatoryPcrs, internal_efi.SecureBootPolicyPCR) } - permitEmptyPCRBanks := flags&PermitEmptyPCRBanks > 0 - logResults, err := checkFirmwareLogAndChoosePCRBank(tpm, log, mandatoryPcrs, permitEmptyPCRBanks) + var checkLogFlags checkFirmwareLogFlags + if flags&PermitEmptyPCRBanks > 0 { + checkLogFlags |= checkFirmwareLogPermitEmptyPCRBanks + } + if flags&PermitWeakPCRBanks > 0 { + checkLogFlags |= checkFirmwareLogPermitWeakPCRBanks + } + + logResults, err := checkFirmwareLogAndChoosePCRBank(tpm, log, mandatoryPcrs, checkLogFlags) switch { case tpm2.IsTPMError(err, tpm2.AnyErrorCode, tpm2.AnyCommandCode): return nil, &TPM2DeviceError{err} case err != nil: - var pcrBankErr *EmptyPCRBankError - if errors.As(err, &pcrBankErr) { + var pcrBanksErr *EmptyPCRBanksError + if errors.As(err, &pcrBanksErr) { // return this one unwrapped return nil, err } diff --git a/efi/preinstall/checks_test.go b/efi/preinstall/checks_test.go index 167d606c..49ff55dd 100644 --- a/efi/preinstall/checks_test.go +++ b/efi/preinstall/checks_test.go @@ -287,6 +287,87 @@ C7E003CB c.Check(errors.As(warning, &bmce), testutil.IsTrue) } +func (s *runChecksSuite) TestRunChecksGoodSHA1(c *C) { + meiAttrs := map[string][]byte{ + "fw_ver": []byte(`0:16.1.27.2176 +0:16.1.27.2176 +0:16.0.15.1624 +`), + "fw_status": []byte(`94000245 +09F10506 +00000020 +00004000 +00041F03 +C7E003CB +`), + } + devices := map[string][]internal_efi.SysfsDevice{ + "iommu": []internal_efi.SysfsDevice{ + efitest.NewMockSysfsDevice("dmar0", "/sys/devices/virtual/iommu/dmar0", "iommu", nil), + efitest.NewMockSysfsDevice("dmar1", "/sys/devices/virtual/iommu/dmar1", "iommu", nil), + }, + "mei": []internal_efi.SysfsDevice{ + efitest.NewMockSysfsDevice("mei0", "/sys/devices/pci0000:00/0000:00:16.0/mei/mei0", "mei", meiAttrs), + }, + } + + warnings, err := s.testRunChecks(c, &testRunChecksParams{ + env: efitest.NewMockHostEnvironmentWithOpts( + efitest.WithVirtMode(internal_efi.VirtModeNone, internal_efi.DetectVirtModeAll), + efitest.WithTPMDevice(tpm2_testutil.NewTransportBackedDevice(s.Transport, false, 1)), + efitest.WithLog(efitest.NewLog(c, &efitest.LogOptions{Algorithms: []tpm2.HashAlgorithmId{tpm2.HashAlgorithmSHA256}})), + efitest.WithAMD64Environment("GenuineIntel", []uint64{cpuid.SDBG, cpuid.SMX}, 4, map[uint32]uint64{0xc80: 0x40000000}), + efitest.WithSysfsDevices(devices), + efitest.WithMockVars(efitest.MockVars{ + {Name: "AuditMode", GUID: efi.GlobalVariable}: &efitest.VarEntry{Attrs: efi.AttributeNonVolatile | efi.AttributeBootserviceAccess | efi.AttributeRuntimeAccess, Payload: []byte{0x0}}, + {Name: "BootCurrent", GUID: efi.GlobalVariable}: &efitest.VarEntry{Attrs: efi.AttributeBootserviceAccess | efi.AttributeRuntimeAccess, Payload: []byte{0x3, 0x0}}, + {Name: "BootOptionSupport", GUID: efi.GlobalVariable}: &efitest.VarEntry{Attrs: efi.AttributeBootserviceAccess | efi.AttributeRuntimeAccess, Payload: []byte{0x13, 0x03, 0x00, 0x00}}, + {Name: "DeployedMode", GUID: efi.GlobalVariable}: &efitest.VarEntry{Attrs: efi.AttributeNonVolatile | efi.AttributeBootserviceAccess | efi.AttributeRuntimeAccess, Payload: []byte{0x1}}, + {Name: "SetupMode", GUID: efi.GlobalVariable}: &efitest.VarEntry{Attrs: efi.AttributeBootserviceAccess | efi.AttributeRuntimeAccess, Payload: []byte{0x0}}, + {Name: "OsIndicationsSupported", GUID: efi.GlobalVariable}: &efitest.VarEntry{Attrs: efi.AttributeBootserviceAccess | efi.AttributeRuntimeAccess, Payload: []byte{0x41, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}}, + }.SetSecureBoot(true).SetPK(c, efitest.NewSignatureListX509(c, snakeoilCert, efi.MakeGUID(0x03f66fa4, 0x5eee, 0x479c, 0xa408, [...]uint8{0xc4, 0xdc, 0x0a, 0x33, 0xfc, 0xde})))), + ), + tpmPropertyModifiers: map[tpm2.Property]uint32{ + tpm2.PropertyNVCountersMax: 0, + tpm2.PropertyPSFamilyIndicator: 1, + tpm2.PropertyManufacturer: uint32(tpm2.TPMManufacturerINTC), + }, + enabledBanks: []tpm2.HashAlgorithmId{tpm2.HashAlgorithmSHA256}, + loadedImages: []secboot_efi.Image{ + &mockImage{ + contents: []byte("mock shim executable"), + digest: testutil.DecodeHexString(c, "25e1b08db2f31ff5f5d2ea53e1a1e8fda6e1d81af4f26a7908071f1dec8611b7"), + signatures: []*efi.WinCertificateAuthenticode{ + efitest.ReadWinCertificateAuthenticodeDetached(c, shimUbuntuSig4), + }, + }, + &mockImage{contents: []byte("mock grub executable"), digest: testutil.DecodeHexString(c, "d5a9780e9f6a43c2e53fe9fda547be77f7783f31aea8013783242b040ff21dc0")}, + &mockImage{contents: []byte("mock kernel executable"), digest: testutil.DecodeHexString(c, "2ddfbd91fa1698b0d133c38ba90dbba76c9e08371ff83d03b5fb4c2e56d7e81f")}, + }, + flags: PermitWeakPCRBanks, + expectedPcrAlg: tpm2.HashAlgorithmSHA256, + expectedUsedSecureBootCAs: []*X509CertificateID{NewX509CertificateID(testutil.ParseCertificate(c, msUefiCACert))}, + expectedFlags: NoPlatformConfigProfileSupport | NoDriversAndAppsConfigProfileSupport | NoBootManagerConfigProfileSupport, + }) + c.Assert(err, IsNil) + c.Assert(warnings.Errs, HasLen, 3) + + warning := warnings.Errs[0] + c.Check(warning, ErrorMatches, `error with platform config \(PCR1\) measurements: generating profiles for PCR 1 is not supported yet`) + var pce *PlatformConfigPCRError + c.Check(errors.As(warning, &pce), testutil.IsTrue) + + warning = warnings.Errs[1] + c.Check(warning, ErrorMatches, `error with drivers and apps config \(PCR3\) measurements: generating profiles for PCR 3 is not supported yet`) + var dce *DriversAndAppsConfigPCRError + c.Check(errors.As(warning, &dce), testutil.IsTrue) + + warning = warnings.Errs[2] + c.Check(warning, ErrorMatches, `error with boot manager config \(PCR5\) measurements: generating profiles for PCR 5 is not supported yet`) + var bmce *BootManagerConfigPCRError + c.Check(errors.As(warning, &bmce), testutil.IsTrue) +} + func (s *runChecksSuite) TestRunChecksGoodEmptySHA384(c *C) { meiAttrs := map[string][]byte{ "fw_ver": []byte(`0:16.1.27.2176 @@ -979,6 +1060,7 @@ C7E003CB var bmce *BootManagerConfigPCRError c.Check(errors.As(warning, &bmce), testutil.IsTrue) } + func (s *runChecksSuite) TestRunChecksGoodInvalidPCR0Value(c *C) { meiAttrs := map[string][]byte{ "fw_ver": []byte(`0:16.1.27.2176 @@ -2227,6 +2309,79 @@ C7E003CB c.Check(errors.As(err, &pfe), testutil.IsTrue) } +func (s *runChecksSuite) TestRunChecksBadSHA1(c *C) { + meiAttrs := map[string][]byte{ + "fw_ver": []byte(`0:16.1.27.2176 +0:16.1.27.2176 +0:16.0.15.1624 +`), + "fw_status": []byte(`94000245 +09F10506 +00000020 +00004000 +00041F03 +C7E003CB +`), + } + devices := map[string][]internal_efi.SysfsDevice{ + "iommu": []internal_efi.SysfsDevice{ + efitest.NewMockSysfsDevice("dmar0", "/sys/devices/virtual/iommu/dmar0", "iommu", nil), + efitest.NewMockSysfsDevice("dmar1", "/sys/devices/virtual/iommu/dmar1", "iommu", nil), + }, + "mei": []internal_efi.SysfsDevice{ + efitest.NewMockSysfsDevice("mei0", "/sys/devices/pci0000:00/0000:00:16.0/mei/mei0", "mei", meiAttrs), + }, + } + + _, err := s.testRunChecks(c, &testRunChecksParams{ + env: efitest.NewMockHostEnvironmentWithOpts( + efitest.WithVirtMode(internal_efi.VirtModeNone, internal_efi.DetectVirtModeAll), + efitest.WithTPMDevice(tpm2_testutil.NewTransportBackedDevice(s.Transport, false, 1)), + efitest.WithLog(efitest.NewLog(c, &efitest.LogOptions{Algorithms: []tpm2.HashAlgorithmId{tpm2.HashAlgorithmSHA1}})), + efitest.WithAMD64Environment("GenuineIntel", []uint64{cpuid.SDBG, cpuid.SMX}, 4, map[uint32]uint64{0xc80: 0x40000000}), + efitest.WithSysfsDevices(devices), + efitest.WithMockVars(efitest.MockVars{ + {Name: "AuditMode", GUID: efi.GlobalVariable}: &efitest.VarEntry{Attrs: efi.AttributeNonVolatile | efi.AttributeBootserviceAccess | efi.AttributeRuntimeAccess, Payload: []byte{0x0}}, + {Name: "BootCurrent", GUID: efi.GlobalVariable}: &efitest.VarEntry{Attrs: efi.AttributeBootserviceAccess | efi.AttributeRuntimeAccess, Payload: []byte{0x3, 0x0}}, + {Name: "BootOptionSupport", GUID: efi.GlobalVariable}: &efitest.VarEntry{Attrs: efi.AttributeBootserviceAccess | efi.AttributeRuntimeAccess, Payload: []byte{0x13, 0x03, 0x00, 0x00}}, + {Name: "DeployedMode", GUID: efi.GlobalVariable}: &efitest.VarEntry{Attrs: efi.AttributeNonVolatile | efi.AttributeBootserviceAccess | efi.AttributeRuntimeAccess, Payload: []byte{0x1}}, + {Name: "SetupMode", GUID: efi.GlobalVariable}: &efitest.VarEntry{Attrs: efi.AttributeBootserviceAccess | efi.AttributeRuntimeAccess, Payload: []byte{0x0}}, + {Name: "OsIndicationsSupported", GUID: efi.GlobalVariable}: &efitest.VarEntry{Attrs: efi.AttributeBootserviceAccess | efi.AttributeRuntimeAccess, Payload: []byte{0x41, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}}, + }.SetSecureBoot(true).SetPK(c, efitest.NewSignatureListX509(c, snakeoilCert, efi.MakeGUID(0x03f66fa4, 0x5eee, 0x479c, 0xa408, [...]uint8{0xc4, 0xdc, 0x0a, 0x33, 0xfc, 0xde})))), + ), + tpmPropertyModifiers: map[tpm2.Property]uint32{ + tpm2.PropertyNVCountersMax: 0, + tpm2.PropertyPSFamilyIndicator: 1, + tpm2.PropertyManufacturer: uint32(tpm2.TPMManufacturerINTC), + }, + enabledBanks: []tpm2.HashAlgorithmId{tpm2.HashAlgorithmSHA1}, + loadedImages: []secboot_efi.Image{ + &mockImage{ + contents: []byte("mock shim executable"), + digest: testutil.DecodeHexString(c, "25e1b08db2f31ff5f5d2ea53e1a1e8fda6e1d81af4f26a7908071f1dec8611b7"), + signatures: []*efi.WinCertificateAuthenticode{ + efitest.ReadWinCertificateAuthenticodeDetached(c, shimUbuntuSig4), + }, + }, + &mockImage{contents: []byte("mock grub executable"), digest: testutil.DecodeHexString(c, "d5a9780e9f6a43c2e53fe9fda547be77f7783f31aea8013783242b040ff21dc0")}, + &mockImage{contents: []byte("mock kernel executable"), digest: testutil.DecodeHexString(c, "2ddfbd91fa1698b0d133c38ba90dbba76c9e08371ff83d03b5fb4c2e56d7e81f")}, + }, + }) + c.Check(err, ErrorMatches, `error with TCG log: no suitable PCR algorithm available: +- TPM_ALG_SHA512: the PCR bank is missing from the TCG log. +- TPM_ALG_SHA384: the PCR bank is missing from the TCG log. +- TPM_ALG_SHA256: the PCR bank is missing from the TCG log. +`) + + var e *NoSuitablePCRAlgorithmError + c.Assert(errors.As(err, &e), testutil.IsTrue) + + // Test that we can access individual errors. + c.Check(e.BankErrs[tpm2.HashAlgorithmSHA512], Equals, ErrPCRBankMissingFromLog) + c.Check(e.BankErrs[tpm2.HashAlgorithmSHA384], Equals, ErrPCRBankMissingFromLog) + c.Check(e.BankErrs[tpm2.HashAlgorithmSHA256], Equals, ErrPCRBankMissingFromLog) +} + func (s *runChecksSuite) TestRunChecksBadMandatoryPCR1(c *C) { meiAttrs := map[string][]byte{ "fw_ver": []byte(`0:16.1.27.2176 @@ -3253,8 +3408,8 @@ C7E003CB expectedUsedSecureBootCAs: []*X509CertificateID{NewX509CertificateID(testutil.ParseCertificate(c, msUefiCACert))}, expectedFlags: NoPlatformConfigProfileSupport | NoDriversAndAppsConfigProfileSupport | NoBootManagerConfigProfileSupport, }) - c.Assert(err, ErrorMatches, `the PCR bank for TPM_ALG_SHA384 is missing from the TCG log but is active on the TPM`) + c.Assert(err, ErrorMatches, `the PCR bank for TPM_ALG_SHA384 is missing from the TCG log but active and with one or more empty PCRs on the TPM`) - var be *EmptyPCRBankError + var be *EmptyPCRBanksError c.Check(errors.As(err, &be), testutil.IsTrue) } diff --git a/efi/preinstall/errors.go b/efi/preinstall/errors.go index 064e7a49..b4debb3c 100644 --- a/efi/preinstall/errors.go +++ b/efi/preinstall/errors.go @@ -25,6 +25,7 @@ import ( "errors" "fmt" "io" + "strings" "github.com/canonical/go-tpm2" internal_efi "github.com/snapcore/secboot/internal/efi" @@ -293,20 +294,50 @@ func (e *PCRValueMismatchError) Error() string { return fmt.Sprintf("PCR value mismatch (actual from TPM %#x, reconstructed from log %#x)", e.PCRValue, e.LogValue) } -// EmptyPCRBankError may be returned unwrapped in the event where a PCR bank seems -// to be active but not extended by firmware and not present in the log. This doesn't matter so -// much for FDE because we can select a good bank, but is a serious firmware bug for any scenario -// that requires remote attestation, because it permits an entire trusted computing environment to -// be spoofed by an adversary in software. +// EmptyPCRBanksError may be returned unwrapped in the event where one or more TCG defined PCR +// banks seem to be active but not extended by firmware and not present in the log. This doesn't +// matter so much for FDE because we can select a good bank, but is a serious firmware bug for +// any scenario that requires remote attestation, because it permits an entire trusted computing +// environment to be spoofed by an adversary in software. +// +// If a PCR bank is missing from the TCG log but is enabled on the TPM with empty PCRs, the bank +// will be recorded to the Algs field. +// +// This might also indicate one or more errors that occur whilst checking for this condition. +// These will be stored in the Errs field. // // This error can be ignored by passing the PermitEmptyPCRBanks flag to [RunChecks]. This is // generally ok, as long as the device is not going to be used for any kind of remote attestation. -type EmptyPCRBankError struct { - Alg tpm2.HashAlgorithmId +type EmptyPCRBanksError struct { + Algs []tpm2.HashAlgorithmId // The PCR banks that have empty PCRs in the TCG defined range. + Errs []error // Any errors that occurred when trying to determine whether a bank missing from the log has any empty PCRs. } -func (e *EmptyPCRBankError) Error() string { - return fmt.Sprintf("the PCR bank for %v is missing from the TCG log but is active on the TPM", e.Alg) +func (e *EmptyPCRBanksError) Error() string { + if len(e.Errs) > 0 { + w := new(bytes.Buffer) + fmt.Fprintf(w, "one or more errors detected when trying to determine whether PCR banks missing from the TCG log are enabled with empty PCRs:\n") + for _, err := range e.Errs { + io.WriteString(w, makeIndentedListItem(0, "-", err.Error())) + } + return w.String() + } + + var algs []string + for _, alg := range e.Algs { + algs = append(algs, fmt.Sprintf("%v", alg)) + } + + var s string + switch len(e.Algs) { + case 0: + return "internal error: invalid EmptyPCRBanksError" + case 1: + s = fmt.Sprintf("bank for %s is", algs[0]) + default: + s = fmt.Sprintf("banks for %s are", strings.Join(algs, ", ")) + } + return fmt.Sprintf("the PCR %s missing from the TCG log but active and with one or more empty PCRs on the TPM", s) } // NoSuitablePCRAlgorithmError is returned wrapped in [TCGLogError] if there is no suitable PCR @@ -314,8 +345,12 @@ func (e *EmptyPCRBankError) Error() string { // during testing (multiple banks and multiple PCRs), this error wraps each individual error // that occurred and provides access to them. type NoSuitablePCRAlgorithmError struct { - BankErrs map[tpm2.HashAlgorithmId]error // BankErrs apply to an entire PCR bank - PCRErrs map[tpm2.HashAlgorithmId]map[tpm2.Handle]error // PCRErrs apply to a single PCR in a single bank + // BankErrs apply to an entire PCR bank. This field is only populated + // if it is not possible to check any PCR in this bank. + BankErrs map[tpm2.HashAlgorithmId]error + + // PCRErrs contain errors associated with specific PCRs in a specific bank. + PCRErrs map[tpm2.HashAlgorithmId]map[tpm2.Handle]error } func (e *NoSuitablePCRAlgorithmError) Error() string { diff --git a/efi/preinstall/export_test.go b/efi/preinstall/export_test.go index 85025e4e..58fad974 100644 --- a/efi/preinstall/export_test.go +++ b/efi/preinstall/export_test.go @@ -34,6 +34,7 @@ type ( AuthorityTrustDataSet = authorityTrustDataSet BootManagerCodeResultFlags = bootManagerCodeResultFlags CheckDriversAndAppsMeasurementsResult = checkDriversAndAppsMeasurementsResult + CheckFirmwareLogFlags = checkFirmwareLogFlags CheckTPM2DeviceFlags = checkTPM2DeviceFlags CpuVendor = cpuVendor DetectVirtResult = detectVirtResult @@ -48,6 +49,8 @@ const ( BootManagerCodeSysprepAppsPresent = bootManagerCodeSysprepAppsPresent BootManagerCodeAbsoluteComputraceRunning = bootManagerCodeAbsoluteComputraceRunning BootManagerCodeNotAllLaunchDigestsVerified = bootManagerCodeNotAllLaunchDigestsVerified + CheckFirmwareLogPermitEmptyPCRBanks = checkFirmwareLogPermitEmptyPCRBanks + CheckFirmwareLogPermitWeakPCRBanks = checkFirmwareLogPermitWeakPCRBanks CheckTPM2DeviceInVM = checkTPM2DeviceInVM CheckTPM2DevicePostInstall = checkTPM2DevicePostInstall CpuVendorIntel = cpuVendorIntel