From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web12.6985.1591781539917524137 for ; Wed, 10 Jun 2020 02:32:20 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 9AEAA1F1; Wed, 10 Jun 2020 02:32:18 -0700 (PDT) Received: from [192.168.1.69] (unknown [10.37.8.184]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A44813F73D; Wed, 10 Jun 2020 02:32:17 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH v3 14/14] OvmfPkg: use generic QEMU image loader for secure boot enabled builds To: Laszlo Ersek , devel@edk2.groups.io Cc: =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= References: <20200305134607.20125-1-ard.biesheuvel@linaro.org> <20200305134607.20125-15-ard.biesheuvel@linaro.org> <5243a882-187f-6ca4-2450-a6c958a934e3@redhat.com> <7d7d1cbc-d1ca-c3dd-c2ac-00084d79decf@redhat.com> From: "Ard Biesheuvel" Message-ID: <20eb271e-ac27-c097-e933-2d2273dfb4d5@arm.com> Date: Wed, 10 Jun 2020 11:32:15 +0200 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 MIME-Version: 1.0 In-Reply-To: <7d7d1cbc-d1ca-c3dd-c2ac-00084d79decf@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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 com= es >>>> to loading kernel images that have been specified on the QEMU comman= d >>>> line. This behavior deviates from ArmVirtQemu based builds, which do >>>> take UEFI secure boot policies into account, and refuse to load imag= es >>>> from the command line that cannot be authenticated. >>>> >>>> The disparity was originally due to the fact that the QEMU command l= ine >>>> kernel loader did not use LoadImage and StartImage at all, but this >>>> changed recently, and now, there are only a couple of reasons left t= o >>>> 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 >>>> =C2=A0=C2=A0 capable system. >>>> >>>> Since every non-authentic PE/COFF image can trivially be converted i= nto >>>> an image that lacks a valid PE/COFF header, the former case can simp= ly >>>> 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=3D2566 >>>> Signed-off-by: Ard Biesheuvel >>>> Reviewed-by: Laszlo Ersek >>>> --- >>>> =C2=A0 OvmfPkg/OvmfPkgIa32.dsc=C2=A0=C2=A0=C2=A0 | 4 ++++ >>>> =C2=A0 OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++++ >>>> =C2=A0 OvmfPkg/OvmfPkgX64.dsc=C2=A0=C2=A0=C2=A0=C2=A0 | 4 ++++ >>>> =C2=A0 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] >>>> =C2=A0=C2=A0=C2=A0 PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciL= ibI440FxQ35.inf >>>> =C2=A0=C2=A0=C2=A0 MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitL= ib.inf >>>> =C2=A0=C2=A0=C2 >>>> QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg= .inf >>>> +!if $(SECURE_BOOT_ENABLE) =3D=3D TRUE >>>> +=C2 >>>> QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemu= LoadImageLib.inf >>>> >>>> +!else >>>> =C2=A0=C2=A0=C2 >>>> QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImag= eLib.inf >>>> >>>> +!endif >>>> =C2=A0 !if $(TPM_ENABLE) =3D=3D TRUE >>>> =C2=A0=C2=A0=C2 >>>> Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibT= cg.inf >>>> >>>> =C2=A0=C2=A0=C2 >>>> Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg= 2.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] >>>> =C2=A0=C2=A0=C2=A0 PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciL= ibI440FxQ35.inf >>>> =C2=A0=C2=A0=C2=A0 MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitL= ib.inf >>>> =C2=A0=C2=A0=C2 >>>> QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg= .inf >>>> +!if $(SECURE_BOOT_ENABLE) =3D=3D TRUE >>>> +=C2 >>>> QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemu= LoadImageLib.inf >>>> >>>> +!else >>>> =C2=A0=C2=A0=C2 >>>> QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImag= eLib.inf >>>> >>>> +!endif >>>> =C2=A0 !if $(TPM_ENABLE) =3D=3D TRUE >>>> =C2=A0=C2=A0=C2 >>>> Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibT= cg.inf >>>> >>>> =C2=A0=C2=A0=C2 >>>> Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg= 2.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] >>>> =C2=A0=C2=A0=C2=A0 PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciL= ibI440FxQ35.inf >>>> =C2=A0=C2=A0=C2=A0 MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitL= ib.inf >>>> =C2=A0=C2=A0=C2 >>>> QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg= .inf >>>> +!if $(SECURE_BOOT_ENABLE) =3D=3D TRUE >>>> +=C2 >>>> QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemu= LoadImageLib.inf >>>> >>>> +!else >>>> =C2=A0=C2=A0=C2 >>>> QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImag= eLib.inf >>>> >>>> +!endif >>>> =C2=A0 !if $(TPM_ENABLE) =3D=3D TRUE >>>> =C2=A0=C2=A0=C2 >>>> Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibT= cg.inf >>>> >>>> =C2=A0=C2=A0=C2 >>>> Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg= 2.inf >>>> >>>> >>> >>> this patch (commit ced77332cab6) breaks the following use case (on X6= 4): >>> >>> (1) we're defining (installing) a new libvirt domain with >>> >>> =C2=A0=C2=A0 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 wi= ll >>> 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 signe= d >>> 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. Sh= im >>> is accepted by an MS certificate. Then shim/grub boot the guest kerne= l, >>> 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 vmlinu= z >>> and initrd files from the remote (e.g. https) OS tree, to local >>> temporary files, and launches the guest (for installation) with a dir= ect >>> (fw_cfg) kernel boot. Because "vmlinuz" is only signed by the OS vend= or, >>> 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 i= s >>> 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 ar= e >>> providing the kernel from a source whose trustworthiness is guarantee= d >>> 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 sh= ut >>> 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 differe= nt >>> 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 worke= d >>> 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 AARCH= 64. >>> >>> ... Unfortunately, when I reviewed this patch, I failed to recall tha= t >>> "virt-install --location NETWORK-URL" relied on fw_cfg kernel boot. A= nd >>> 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 ti= me >> than you are willing to spend on it? >=20 > 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. >=20 > More generally, the guest firmware need not know where the kernel/initr= d > over fw_cfg originate from, only that they should be trusted. >=20 > The PCD idea looks nice to me; I'd just make it an OvmfPkg PCD. Options= : >=20 > (1) make it a FeaturePCD, and introduce a new build flag (-D) for > controlling it >=20 > (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 >=20 > (3) make it a dynamic boolean PCD, and use the recently introduced > fw_cfg control interface >=20 >=20 > 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: >=20 > SecureBoot disabled || FallbackPcd >=20 > (where "SecureBoot disabled" means that the SecureBoot standard UEFI > variable doesn't exist, or has zero value). >=20 > I think option (2) means the least churn, and it would be "good enough" > functionally. >=20 > Does this look palatable to you? >=20 >=20 >> So what is the point of DEFER_EXECUTE_ON_SECURITY_VIOLATION anyway, if >> you cannot start the image anyway? >=20 > Honestly: I'm not sure! >=20 >> Because I would prefer it if we could >> start the image anyway, Unfortunately, the only way seems to be >=20 > 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" LoadImageStat= us. >=20 >> to use >> the loaded image protocol data (base and size in memory) and do anothe= r >> LoadImage() after adding the hash to "db", and this is also more >> elaborate than I would like. >=20 > Agreed. We shouldn't mess with "db". >=20 >> 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 avoi= d >> this mess on ARM >=20 > 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 O= S > types officially supported on RHEL8 hosts: >=20 > - https://access.redhat.com/articles/973133 > - https://access.redhat.com/articles/973163 >=20 > then all we (RH) would need are two patches for (upstream) > OvmfPkg/EnrollDefaultKeys. Namely, we should enroll two more DB entries= : >=20 > - SUSE's CA certificate to which the key(s) chain that they use for > signing their kernels >=20 > - Red Hat's CA certificate to which the key(s) chain that we use for > signing our kernels. >=20 > That would strictly be an improvement even for upstream OVMF (more > vendors accepted). >=20 > 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. >=20 > Do you agree? Would you be OK with patches to this effect? >=20 Yes, option #3 looks reasonable to me. And adding the certs is the right=20 thing to do regardless - going around installing the MS certs everywhere=20 is a recipe for disaster, and AIUI, MS now requires you to remove them=20 in some cases?? (for DeviceGuard support or sth like that)