Re: [edk2-devel] [PATCH v3 14/14] OvmfPkg: use generic QEMU image loader for secure boot enabled builds

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

On 6/10/20 11:22 AM, Laszlo Ersek wrote:
> On 06/09/20 12:45, Ard Biesheuvel wrote:
>> On 6/9/20 11:51 AM, Laszlo Ersek via groups.io wrote:
>>> Hi Ard,
>>>
>>> On 03/05/20 14:46, Ard Biesheuvel wrote:
>>>> The QemuLoadImageLib implementation we currently use for all OVMF
>>>> builds copies the behavior of the QEMU loader code that precedes it,
>>>> which is to disregard UEFI secure boot policies entirely when it comes
>>>> to loading kernel images that have been specified on the QEMU command
>>>> line. This behavior deviates from ArmVirtQemu based builds, which do
>>>> take UEFI secure boot policies into account, and refuse to load images
>>>> from the command line that cannot be authenticated.
>>>>
>>>> The disparity was originally due to the fact that the QEMU command line
>>>> kernel loader did not use LoadImage and StartImage at all, but this
>>>> changed recently, and now, there are only a couple of reasons left to
>>>> stick with the legacy loader:
>>>> - it permits loading images that lack a valid PE/COFF header,
>>>> - it permits loading X64 kernels on IA32 firmware running on a X64
>>>>    capable system.
>>>>
>>>> Since every non-authentic PE/COFF image can trivially be converted into
>>>> an image that lacks a valid PE/COFF header, the former case can simply
>>>> not be supported in a UEFI secure boot context. The latter case is
>>>> highly
>>>> theoretical, given that one could easily switch to native X64
>>>> firmware in
>>>> a VM scenario.
>>>>
>>>> That leaves us with little justification to use the legacy loader at all
>>>> when UEFI secure boot policies are in effect, so let's switch to the
>>>> generic loader for UEFI secure boot enabled builds.
>>>>
>>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2566
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>>> ---
>>>>   OvmfPkg/OvmfPkgIa32.dsc    | 4 ++++
>>>>   OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++++
>>>>   OvmfPkg/OvmfPkgX64.dsc     | 4 ++++
>>>>   3 files changed, 12 insertions(+)
>>>>
>>>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>>>> index ec21d2f3f6cb..8916255df4df 100644
>>>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>>>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>>>> @@ -363,7 +363,11 @@ [LibraryClasses.common.DXE_DRIVER]
>>>>     PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>>>>     MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>>>>   Â
>>>> QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
>>>> +!if $(SECURE_BOOT_ENABLE) == TRUE
>>>> +Â
>>>> QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
>>>>
>>>> +!else
>>>>   Â
>>>> QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
>>>>
>>>> +!endif
>>>>   !if $(TPM_ENABLE) == TRUE
>>>>   Â
>>>> Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf
>>>>
>>>>   Â
>>>> Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
>>>>
>>>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>>>> index 3c80a18c6086..342ff96cc279 100644
>>>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>>>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>>>> @@ -367,7 +367,11 @@ [LibraryClasses.common.DXE_DRIVER]
>>>>     PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>>>>     MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>>>>   Â
>>>> QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
>>>> +!if $(SECURE_BOOT_ENABLE) == TRUE
>>>> +Â
>>>> QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
>>>>
>>>> +!else
>>>>   Â
>>>> QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
>>>>
>>>> +!endif
>>>>   !if $(TPM_ENABLE) == TRUE
>>>>   Â
>>>> Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf
>>>>
>>>>   Â
>>>> Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
>>>>
>>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>>>> index 63f1f935f4f3..1fb2de5e0121 100644
>>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>>> @@ -367,7 +367,11 @@ [LibraryClasses.common.DXE_DRIVER]
>>>>     PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
>>>>     MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>>>>   Â
>>>> QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
>>>> +!if $(SECURE_BOOT_ENABLE) == TRUE
>>>> +Â
>>>> QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
>>>>
>>>> +!else
>>>>   Â
>>>> QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
>>>>
>>>> +!endif
>>>>   !if $(TPM_ENABLE) == TRUE
>>>>   Â
>>>> Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf
>>>>
>>>>   Â
>>>> Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
>>>>
>>>>
>>>
>>> this patch (commit ced77332cab6) breaks the following use case (on X64):
>>>
>>> (1) we're defining (installing) a new libvirt domain with
>>>
>>>    virt-install --location NETWORK-URL
>>>
>>> where NETWORK-URL identifies a remote (e.g. https) "exploded" Linux
>>> distro tree,
>>>
>>> (2) in the variable store template file, from which the new domain will
>>> have its variable store instantiated, the Microsoft certs had been
>>> enrolled (by running EnrollDefaultKeys),
>>>
>>> (3) the guest kernel ("vmlinuz") downloaded in step (1) is only signed
>>> by the distro vendor, not by Microsoft.
>>>
>>> When installing a distro with "virt-install" from a UEFI-bootable ISO
>>> image (exposed to the guest e.g. via virtio-scsi), that is, not from a
>>> remote "exploded" OS tree like above, then "shim" is booted first. Shim
>>> is accepted by an MS certificate. Then shim/grub boot the guest kernel,
>>> and thanks to shim, the vendor signature on that kernel is accepted.
>>> This is why "shim" exists.
>>>
>>> However, when installing the guest with "virt-install --location
>>> NETWORK-URL", then virt-install first downloads the broken-out vmlinuz
>>> and initrd files from the remote (e.g. https) OS tree, to local
>>> temporary files, and launches the guest (for installation) with a direct
>>> (fw_cfg) kernel boot. Because "vmlinuz" is only signed by the OS vendor,
>>> and "shim" is not used at all, and the varstore only has the MS certs,
>>> the guest (installer) now fails to boot.
>>>
>>> This used to work before, and there's an argument to be made that it
>>> wasn't insecure. Because -- as I understand it anyway --, the usage is
>>> not unlike "audit mode / deployed mode":
>>>
>>> - When you boot the guest for the very first time, you make the guest
>>> firmware blindly trust the guest kernel, in effect -- because, you are
>>> providing the kernel from a source whose trustworthiness is guaranteed
>>> by other means (for example, you pass such an https NETWORK-URL to
>>> virt-install on the host side that you trust).
>>>
>>> - Once the guest is installed, and either rebooted from within, or shut
>>> down altogether and relaunched, then the fw-cfg "bypass" won't happen
>>> again. It's also not needed any more, because with the installation
>>> completed, "shim" has also become part of the boot process (from the
>>> virtual disk).
>>>
>>> Do you have any ideas to make this scenario work again? Or else, can we
>>> predicate this patch for the DSC files on a build flag that's different
>>> from SECURE_BOOT_ENABLE?
>>>
>>> (Put more formally: right now, the EFI_SECURITY_VIOLATION branch, and
>>> the EFI_ACCESS_DENIED branch that I posted for TianoCore#2785 earlier,
>>> in X86QemuLoadImageLib, are dead code. Because, those conditions are
>>> only possible when SECURE_BOOT_ENABLE is defined -- but in that case, we
>>> (currently) don't use X86QemuLoadImageLib at all.)
>>>
>>
>> Hmm, I did wonder about that and then forgot again ...
>>
>>> I do see that the *exact* virt-install scenario above has never worked
>>> in ArmVirtQemu / AARCH64. (Given that ArmVirtQemu has always used
>>> gBS->LoadImage() for fw_cfg kernel boot.) However, to us (RH) anyway,
>>> that difference between AARCH64 and X64 isn't a practical one. That's
>>> because we have never set SECURE_BOOT_ENABLE for ArmVirtQemu in the
>>> first place, due to not having an SMM-like pflash protection on AARCH64.
>>>
>>> ... Unfortunately, when I reviewed this patch, I failed to recall that
>>> "virt-install --location NETWORK-URL" relied on fw_cfg kernel boot. And
>>> I failed to realize that, by extension, virt-install relied on fw_cfg
>>> kernel boot bypassing Secure Boot. :(
>>>
>>
>> I would argue that the cleanest way to deal with this is to introduce
>> something like
>>
>> gEfiSecurityPkgTokenSpaceGuid.PcdHttpsLoadedImageVerificationPolicy
>>
>> so that you can mark HTTPS loaded images as trusted. In my opinion, a
>> HTTPS loaded image is sufficiently different from the other classes of
>> images that it warrants a separate policy.
>>
>> But I suppose this could be a difficult sell, and it will take more time
>> than you are willing to spend on it?
> 
> Primarily, I wouldn't suggest including "https" in the name of the PCD,
> as "https" is just an example above. If NETWORK-URL is in the "http"
> (not "https"), or even "ftp" scheme, but is served from localhost (for
> example), it could be considered secure.
> 
> More generally, the guest firmware need not know where the kernel/initrd
> over fw_cfg originate from, only that they should be trusted.
> 
> The PCD idea looks nice to me; I'd just make it an OvmfPkg PCD. Options:
> 
> (1) make it a FeaturePCD, and introduce a new build flag (-D) for
> controlling it
> 
> (2) make it a FeaturePCD, and do not introduce a new flag (-D), instead,
> let users rely on the already available "--pcd" option of "build" to
> control it
> 
> (3) make it a dynamic boolean PCD, and use the recently introduced
> fw_cfg control interface
> 
> 
> In all three options, the OVMF DSC files would unconditionally use the
> X86QemuLoadImageLib instance, we'd just predicate the fallback to the
> legacy Linux/x86 boot protocol on:
> 
>    SecureBoot disabled || FallbackPcd
> 
> (where "SecureBoot disabled" means that the SecureBoot standard UEFI
> variable doesn't exist, or has zero value).
> 
> I think option (2) means the least churn, and it would be "good enough"
> functionally.
> 
> Does this look palatable to you?
> 
> 
>> So what is the point of DEFER_EXECUTE_ON_SECURITY_VIOLATION anyway, if
>> you cannot start the image anyway?
> 
> Honestly: I'm not sure!
> 
>> Because I would prefer it if we could
>> start the image anyway, Unfortunately, the only way seems to be
> 
> Right; it seems like the EFI_SECURITY_VIOLATION status would get saved
> in the private LoadImageStatus field, and that would cause StartImage()
> to fail early as well. I couldn't find a spot to "re-set" LoadImageStatus.
> 
>> to use
>> the loaded image protocol data (base and size in memory) and do another
>> LoadImage() after adding the hash to "db", and this is also more
>> elaborate than I would like.
> 
> Agreed. We shouldn't mess with "db".
> 
>> Of course, the *correct* fix is to add the vendor cert to db - you are
>> loading a signed image from a known source, and this whole dance with
>> shim and the MS certs is ridiculous, and I am still hoping we can avoid
>> this mess on ARM
> 
> I considered this. It didn't look feasible to me at first sight (how
> many vendors?), but now that you mention it and I've looked at it more
> closely, it does look feasible. Selfishly (!), if I look at the guest OS
> types officially supported on RHEL8 hosts:
> 
> - https://access.redhat.com/articles/973133
> - https://access.redhat.com/articles/973163
> 
> then all we (RH) would need are two patches for (upstream)
> OvmfPkg/EnrollDefaultKeys. Namely, we should enroll two more DB entries:
> 
> - SUSE's CA certificate to which the key(s) chain that they use for
> signing their kernels
> 
> - Red Hat's CA certificate to which the key(s) chain that we use for
> signing our kernels.
> 
> That would strictly be an improvement even for upstream OVMF (more
> vendors accepted).
> 
> Ultimately I think the right way is to pick option (3) -- dynamic PCD --
> *and* add the SUSE and RH CA certs to OvmfPkg/EnrollDefaultKeys. That
> means SUSE and RH signed kernels can be booted at once, with secure
> verification, and users would still have a QEMU command line option for
> opting *into* the legacy boot protocol fallback.
> 
> Do you agree? Would you be OK with patches to this effect?
> 

Yes, option #3 looks reasonable to me. And adding the certs is the right 
thing to do regardless - going around installing the MS certs everywhere 
is a recipe for disaster, and AIUI, MS now requires you to remove them 
in some cases?? (for DeviceGuard support or sth like that)