public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Dov Murik" <dovmurik@linux.ibm.com>
To: Dov Murik <dovmurik@linux.ibm.com>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	Brijesh Singh <brijesh.singh@amd.com>,
	Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: devel@edk2.groups.io,
	Tobin Feldman-Fitzthum <tobin@linux.ibm.com>,
	Tobin Feldman-Fitzthum <tobin@ibm.com>,
	Jim Cadden <jcadden@ibm.com>,
	James Bottomley <jejb@linux.ibm.com>,
	Hubertus Franke <frankeh@us.ibm.com>,
	Ard Biesheuvel <ardb+tianocore@kernel.org>,
	Jordan Justen <jordan.l.justen@intel.com>,
	Ashish Kalra <ashish.kalra@amd.com>,
	Brijesh Singh <brijesh.singh@amd.com>,
	Erdem Aktas <erdemaktas@google.com>,
	Jiewen Yao <jiewen.yao@intel.com>, Min Xu <min.m.xu@intel.com>,
	Tom Lendacky <thomas.lendacky@amd.com>
Subject: Re: [PATCH v3 10/11] OvmfPkg: add BlobVerifierLibSevHashes
Date: Wed, 21 Jul 2021 09:29:15 +0000	[thread overview]
Message-ID: <20210721092915.GA1219115@amdrome3> (raw)
In-Reply-To: <20210720080401.3662854-11-dovmurik@linux.ibm.com>

Tom, Brijesh, Ard,

I think I found a bug in this patch.  I used libfuzzer to test the
VerifyBlob implementation here, and it immediately found a few "read
memory out of range" issues.  See details below in VerifyBlob.

If the Guest Owner properly validates the measurement (which includes
the expected well-formatted hashes table), then QEMU cannot modify it by
including problematic length fields, etc.  So I this bug is not a
security issue - it doesn't allow QEMU to circumvent the
kernel/initrd/cmdline measurement check.

That said, I think that OVMF should not access memory ranges it's not
supposed to, and that's why I think hardening this parsing function is a
good idea.

I'll submit another version with added validity checks of the hashes
table structure. I'll also add the INT32 explanation comment per Tom's
suggestion.



On Tue, Jul 20, 2021 at 08:04:00AM +0000, Dov Murik wrote:
> Add an implementation for BlobVerifierLib that locates the SEV hashes
> table and verifies that the calculated hashes of the kernel, initrd, and
> cmdline blobs indeed match the expected hashes stated in the hashes
> table.
> 
> If there's a missing hash or a hash mismatch then EFI_ACCESS_DENIED is
> returned which will cause a failure to load a kernel image.
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ashish Kalra <ashish.kalra@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> Co-developed-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf |  37 ++++
>  OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c      | 200 ++++++++++++++++++++
>  2 files changed, 237 insertions(+)
> 
> diff --git a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
> new file mode 100644
> index 000000000000..76ca0b8154ce
> --- /dev/null
> +++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
> @@ -0,0 +1,37 @@
> +## @file
> +#
> +#  Blob verifier library that uses SEV hashes table.  The hashes table holds the
> +#  allowed hashes of the kernel, initrd, and cmdline blobs.
> +#
> +#  Copyright (C) 2021, IBM Corp
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 1.29
> +  BASE_NAME                      = BlobVerifierLibSevHashes
> +  FILE_GUID                      = 59e713b5-eff3-46a7-8d8b-46f4c004ad7b
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = BlobVerifierLib
> +  CONSTRUCTOR                    = BlobVerifierLibSevHashesConstructor
> +
> +[Sources]
> +  BlobVerifierSevHashes.c
> +
> +[Packages]
> +  CryptoPkg/CryptoPkg.dec
> +  MdePkg/MdePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> +  BaseCryptLib
> +  BaseMemoryLib
> +  DebugLib
> +  PcdLib
> +
> +[FixedPcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase
> +  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
> diff --git a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
> new file mode 100644
> index 000000000000..797d63d18067
> --- /dev/null
> +++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
> @@ -0,0 +1,200 @@
> +/** @file
> +
> +  Blob verifier library that uses SEV hashes table.  The hashes table holds the
> +  allowed hashes of the kernel, initrd, and cmdline blobs.
> +
> +  Copyright (C) 2021, IBM Corporation
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#include <Library/BaseCryptLib.h>
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/BlobVerifierLib.h>
> +
> +/**
> +  The SEV Hashes table must be in encrypted memory and has the table
> +  and its entries described by
> +
> +  <GUID>|UINT16 <len>|<data>
> +
> +  With the whole table GUID being 9438d606-4f22-4cc9-b479-a793d411fd21
> +
> +  The current possible table entries are for the kernel, the initrd
> +  and the cmdline:
> +
> +  4de79437-abd2-427f-b835-d5b172d2045b  kernel
> +  44baf731-3a2f-4bd7-9af1-41e29169781d  initrd
> +  97d02dd8-bd20-4c94-aa78-e7714d36ab2a  cmdline
> +
> +  The size of the entry is used to identify the hash, but the
> +  expectation is that it will be 32 bytes of SHA-256.
> +**/
> +
> +#define SEV_HASH_TABLE_GUID \
> +  (GUID) { 0x9438d606, 0x4f22, 0x4cc9, { 0xb4, 0x79, 0xa7, 0x93, 0xd4, 0x11, 0xfd, 0x21 } }
> +#define SEV_KERNEL_HASH_GUID \
> +  (GUID) { 0x4de79437, 0xabd2, 0x427f, { 0xb8, 0x35, 0xd5, 0xb1, 0x72, 0xd2, 0x04, 0x5b } }
> +#define SEV_INITRD_HASH_GUID \
> +  (GUID) { 0x44baf731, 0x3a2f, 0x4bd7, { 0x9a, 0xf1, 0x41, 0xe2, 0x91, 0x69, 0x78, 0x1d } }
> +#define SEV_CMDLINE_HASH_GUID \
> +  (GUID) { 0x97d02dd8, 0xbd20, 0x4c94, { 0xaa, 0x78, 0xe7, 0x71, 0x4d, 0x36, 0xab, 0x2a } }
> +
> +STATIC CONST EFI_GUID mSevKernelHashGuid = SEV_KERNEL_HASH_GUID;
> +STATIC CONST EFI_GUID mSevInitrdHashGuid = SEV_INITRD_HASH_GUID;
> +STATIC CONST EFI_GUID mSevCmdlineHashGuid = SEV_CMDLINE_HASH_GUID;
> +
> +#pragma pack (1)
> +typedef struct {
> +  GUID   Guid;
> +  UINT16 Len;
> +  UINT8  Data[];
> +} HASH_TABLE;
> +#pragma pack ()
> +
> +STATIC HASH_TABLE *mHashesTable;
> +STATIC UINT16 mHashesTableSize;
> +
> +STATIC
> +CONST GUID*
> +FindBlobEntryGuid (
> +  IN  CONST CHAR16    *BlobName
> +  )
> +{
> +  if (StrCmp (BlobName, L"kernel") == 0) {
> +    return &mSevKernelHashGuid;
> +  } else if (StrCmp (BlobName, L"initrd") == 0) {
> +    return &mSevInitrdHashGuid;
> +  } else if (StrCmp (BlobName, L"cmdline") == 0) {
> +    return &mSevCmdlineHashGuid;
> +  } else {
> +    return NULL;
> +  }
> +}
> +
> +/**
> +  Verify blob from an external source.
> +
> +  @param[in] BlobName           The name of the blob
> +  @param[in] Buf                The data of the blob
> +  @param[in] BufSize            The size of the blob in bytes
> +
> +  @retval EFI_SUCCESS           The blob was verified successfully.
> +  @retval EFI_ACCESS_DENIED     The blob could not be verified, and therefore
> +                                should be considered non-secure.
> +**/
> +EFI_STATUS
> +EFIAPI
> +VerifyBlob (
> +  IN  CONST CHAR16    *BlobName,
> +  IN  CONST VOID      *Buf,
> +  IN  UINT32          BufSize
> +  )
> +{
> +  CONST GUID *Guid;
> +  INT32 Len;
> +  HASH_TABLE *Entry;
> +
> +  if (mHashesTable == NULL || mHashesTableSize == 0) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Verifier called but no hashes table discoverd in MEMFD\n",
> +      __FUNCTION__));
> +    return EFI_ACCESS_DENIED;
> +  }
> +
> +  Guid = FindBlobEntryGuid (BlobName);
> +  if (Guid == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: Unknown blob name \"%s\"\n", __FUNCTION__,
> +      BlobName));
> +    return EFI_ACCESS_DENIED;
> +  }
> +
> +  for (Entry = mHashesTable, Len = 0;
> +       Len < (INT32)mHashesTableSize;
> +       Len += Entry->Len,
> +       Entry = (HASH_TABLE *)((UINT8 *)Entry + Entry->Len)) {
> +    UINTN EntrySize;
> +    EFI_STATUS Status;
> +    UINT8 Hash[SHA256_DIGEST_SIZE];
> +
> +    if (!CompareGuid (&Entry->Guid, Guid)) {

Bug: This can access memory above the mHashTableSize limit.  Consider
mHashTableSize == 1.  We enter the for loop but CompareGuid will "read"
16 bytes of Entry->Guid, which is past the limit.


> +      continue;
> +    }
> +
> +    DEBUG ((DEBUG_INFO, "%a: Found GUID %g in table\n", __FUNCTION__, Guid));
> +
> +    EntrySize = Entry->Len - sizeof (Entry->Guid) - sizeof (Entry->Len);

Bug: This can access memory above the mHashTableSize limit.  Consider
mHashTableSize == 16.  CompareGuid is OK but Entry->Len accesses the 2
bytes at offset 16, which is after the limit.


> +    if (EntrySize != SHA256_DIGEST_SIZE) {
> +      DEBUG ((DEBUG_ERROR, "%a: Hash has the wrong size %d != %d\n",
> +        __FUNCTION__, EntrySize, SHA256_DIGEST_SIZE));
> +      return EFI_ACCESS_DENIED;
> +    }
> +
> +    //
> +    // Calculate the buffer's hash and verify that it is identical to the
> +    // expected hash table entry
> +    //
> +    Sha256HashAll (Buf, BufSize, Hash);
> +
> +    if (CompareMem (Entry->Data, Hash, EntrySize) == 0) {

Bug: This can access memory above the mHashTableSize limit.  Consider
mHashTableSize == 21.  CompareGuid is OK and Entry->Len == 50 as
expected (18 bytes header and 32 bytes for SHA256).  But CompareMem will
try to read the 32 bytes when it's only allowed to read 3 bytes
(21 - 18).


-Dov


> +      Status = EFI_SUCCESS;
> +      DEBUG ((DEBUG_INFO, "%a: Hash comparison succeeded for \"%s\"\n",
> +        __FUNCTION__, BlobName));
> +    } else {
> +      Status = EFI_ACCESS_DENIED;
> +      DEBUG ((DEBUG_ERROR, "%a: Hash comparison failed for \"%s\"\n",
> +        __FUNCTION__, BlobName));
> +    }
> +    return Status;
> +  }
> +
> +  DEBUG ((DEBUG_ERROR, "%a: Hash GUID %g not found in table\n", __FUNCTION__,
> +    Guid));
> +  return EFI_ACCESS_DENIED;
> +}
> +
> +/**
> +  Locate the SEV hashes table.
> +
> +  This function always returns success, even if the table can't be found.  The
> +  subsequent VerifyBlob calls will fail if no table was found.
> +
> +  @retval RETURN_SUCCESS   The verifier tables were set up correctly
> +**/
> +RETURN_STATUS
> +EFIAPI
> +BlobVerifierLibSevHashesConstructor (
> +  VOID
> +  )
> +{
> +  HASH_TABLE *Ptr = (void *)(UINTN)FixedPcdGet64 (PcdQemuHashTableBase);
> +  UINT32 Size = FixedPcdGet32 (PcdQemuHashTableSize);
> +
> +  mHashesTable = NULL;
> +  mHashesTableSize = 0;
> +
> +  if (Ptr == NULL || Size == 0) {
> +    return RETURN_SUCCESS;
> +  }
> +
> +  if (!CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID)) {
> +    return RETURN_SUCCESS;
> +  }
> +
> +  if (Ptr->Len < (sizeof Ptr->Guid + sizeof Ptr->Len)) {
> +    return RETURN_SUCCESS;
> +  }
> +
> +  DEBUG ((DEBUG_INFO, "%a: Found injected hashes table in secure location\n",
> +    __FUNCTION__));
> +
> +  mHashesTable = (HASH_TABLE *)Ptr->Data;
> +  mHashesTableSize = Ptr->Len - sizeof Ptr->Guid - sizeof Ptr->Len;
> +
> +  DEBUG ((DEBUG_VERBOSE, "%a: mHashesTable=0x%p, Size=%u\n", __FUNCTION__,
> +    mHashesTable, mHashesTableSize));
> +
> +  return RETURN_SUCCESS;
> +}
> -- 
> 2.25.1
> 

  parent reply	other threads:[~2021-07-21  9:29 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-20  8:03 [PATCH v3 00/11] Measured SEV boot with kernel/initrd/cmdline Dov Murik
2021-07-20  8:03 ` [PATCH v3 01/11] OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming Dov Murik
2021-07-20  8:03 ` [PATCH v3 02/11] OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds Dov Murik
2021-07-20  8:03 ` [PATCH v3 03/11] OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg Dov Murik
2021-07-20  8:03 ` [PATCH v3 04/11] OvmfPkg: add library class BlobVerifierLib with null implementation Dov Murik
2021-07-20 16:18   ` Lendacky, Thomas
2021-07-20  8:03 ` [PATCH v3 05/11] OvmfPkg: add BlobVerifierLibNull to DSC Dov Murik
2021-07-20 16:20   ` Lendacky, Thomas
2021-07-20  8:03 ` [PATCH v3 06/11] ArmVirtPkg: " Dov Murik
2021-07-20  8:03 ` [PATCH v3 07/11] OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg Dov Murik
2021-07-20 16:24   ` Lendacky, Thomas
2021-07-20  8:03 ` [PATCH v3 08/11] OvmfPkg/AmdSev/SecretPei: build hob for full page Dov Murik
2021-07-20 16:24   ` Lendacky, Thomas
2021-07-20 16:44   ` Brijesh Singh
2021-07-20  8:03 ` [PATCH v3 09/11] OvmfPkg/AmdSev: reserve MEMFD space for for firmware config hashes Dov Murik
2021-07-20 16:45   ` Brijesh Singh
2021-07-20  8:04 ` [PATCH v3 10/11] OvmfPkg: add BlobVerifierLibSevHashes Dov Murik
2021-07-20 17:18   ` Lendacky, Thomas
2021-07-20 18:24   ` Brijesh Singh
2021-07-21  9:29   ` Dov Murik [this message]
2021-07-20  8:04 ` [PATCH v3 11/11] OvmfPkg/AmdSev: Enforce hash verification of kernel blobs Dov Murik
2021-07-20 17:19   ` Lendacky, Thomas
2021-07-20 17:22 ` [PATCH v3 00/11] Measured SEV boot with kernel/initrd/cmdline Lendacky, Thomas
2021-07-20 17:27   ` Ard Biesheuvel
2021-07-21  7:55     ` Dov Murik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210721092915.GA1219115@amdrome3 \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox