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 <[email protected]>
Reviewed-by: Laszlo Ersek <[email protected]>
---
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?
So what is the point of DEFER_EXECUTE_ON_SECURITY_VIOLATION anyway, if
you cannot start the image anyway? Because I would prefer it if we could
start the image anyway, Unfortunately, the only way seems to be 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.
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60947): https://edk2.groups.io/g/devel/message/60947
Mute This Topic: https://groups.io/mt/71749529/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
