On Sun, May 10, 2020 at 04:56:21PM +0530, Sughosh Ganu wrote: > On Fri, 8 May 2020 at 06:12, Akashi Takahiro <takahiro.aka...@linaro.org> > wrote: > > > On Thu, May 07, 2020 at 05:20:35PM +0530, Sughosh Ganu wrote: > > > On Thu, 7 May 2020 at 13:49, Akashi Takahiro <takahiro.aka...@linaro.org > > > > > > wrote: > > > > > > > Sughosh, > > > > > > > > On Thu, Apr 30, 2020 at 11:06:28PM +0530, Sughosh Ganu wrote: > > > > > Add support for authenticating uefi capsules. Most of the signature > > > > > verification functionality is shared with the uefi secure boot > > > > > feature. > > > > > > > > > > The root certificate containing the public key used for the signature > > > > > verification is stored as an efi variable, similar to the variables > > > > > used for uefi secure boot. The root certificate is stored as an efi > > > > > signature list(esl) file -- this file contains the x509 certificate > > > > > which is the root certificate. > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.g...@linaro.org> > > > > > --- > > > > > include/efi_api.h | 17 +++++ > > > > > include/efi_loader.h | 8 ++- > > > > > lib/efi_loader/Kconfig | 16 +++++ > > > > > lib/efi_loader/efi_capsule.c | 126 > > +++++++++++++++++++++++++++++++++ > > > > > lib/efi_loader/efi_signature.c | 4 +- > > > > > 5 files changed, 167 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > > <snip> > > > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > > > > > index ec2976ceba..245deabbed 100644 > > > > > --- a/lib/efi_loader/Kconfig > > > > > +++ b/lib/efi_loader/Kconfig > > > > > @@ -110,6 +110,22 @@ config EFI_CAPSULE_FIT_DEVICE > > > > > help > > > > > Define storage device, say 1:1, for storing FIT image > > > > > > > > > > +config EFI_CAPSULE_AUTHENTICATE > > > > > + bool "Update Capsule authentication" > > > > > + depends on EFI_HAVE_CAPSULE_SUPPORT > > > > > + depends on EFI_CAPSULE_ON_DISK > > > > > + depends on EFI_FIRMWARE_MANAGEMENT_PROTOCOL > > > > > > > > Do we need those two configurations? > > > > > > > > > > Right, I think we can just do with the EFI_CAPSULE_ON_DISK. Will change. > > > > Actually I don't think we need EFI_CAPSULE_ON_DISK neither. > > > > We at least need EFI_HAVE_CAPSULE_SUPPORT, isn't it.
Ah, yes of course. -Takahiro Akashi > > > > > > > > > > > > > > > + select SHA256 > > > > > + select RSA > > > > > + select RSA_VERIFY > > > > > + select RSA_VERIFY_WITH_PKEY > > > > > + select X509_CERTIFICATE_PARSER > > > > > + select PKCS7_MESSAGE_PARSER > > > > > + default n > > > > > + help > > > > > + Select this option if you want to enable capsule > > > > > + authentication > > > > > + > > > > > config EFI_DEVICE_PATH_TO_TEXT > > > > > bool "Device path to text protocol" > > > > > default y > > > > > diff --git a/lib/efi_loader/efi_capsule.c > > b/lib/efi_loader/efi_capsule.c > > > > > index 931d363edc..a265d36ff0 100644 > > > > > --- a/lib/efi_loader/efi_capsule.c > > > > > +++ b/lib/efi_loader/efi_capsule.c > > > > > @@ -12,6 +12,10 @@ > > > > > #include <malloc.h> > > > > > #include <mapmem.h> > > > > > #include <sort.h> > > > > > +#include "../lib/crypto/pkcs7_parser.h" > > > > > + > > > > > > > > As you might notice, the location was changed by > > > > my recent patch. > > > > > > > > > > Will check and update. > > > > > > > > > > > > > > > +#include <crypto/pkcs7.h> > > > > > +#include <linux/err.h> > > > > > > > > > > const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID; > > > > > static const efi_guid_t efi_guid_firmware_management_capsule_id = > > > > > @@ -245,6 +249,128 @@ out: > > > > > > > > > > return ret; > > > > > } > > > > > + > > > > > +#if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE) > > > > > + > > > > > +const efi_guid_t efi_guid_capsule_root_cert_guid = > > > > > + EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > > > > > + > > > > > +__weak u16 *efi_get_truststore_name(void) > > > > > +{ > > > > > + return L"CRT"; > > > > > > > > This variable is not defined by UEFI specification, isn't it? > > > > How can this variable be protected? > > > > > > > > > > This is not part of the uefi specification. The uefi specification does > > not > > > mandate any particular method for storing the root certificate to be used > > > for capsule authentication. The edk2 code gets the root certificate > > through > > > a pcd. I had tried using KEK variable for storing the root certificate, > > and > > > had also come up with an implementation. However, the addition/deletion > > and > > > update of the secure variables is very closely tied with the secure boot > > > feature and the secure boot state of the system, which is the reason why > > i > > > dropped that solution and used a separate variable, which can be defined > > by > > > the vendor to store the root certificate. > > > > My concern is that, without any protection, the value of this variable > > can be modified by a mal-user. > > (One simple solution would be to set this variable read-only.) > > > > Yes, that is one solution. This will also be taken care of in a scenario > where the platform is booted in a trusted chain, and the env is also part > of the image which gets verified by the previous stage(e.g BL2) before > jumping to the u-boot image. If there is any change in the u-boot image, > that would result in a boot failure -- but that would need the env to be > part of the u-boot image which gets verified. I will explore making this > variable read-only. > > > > > > > > > > > > > > +} > > > > > + > > > > > +__weak const efi_guid_t *efi_get_truststore_vendor(void) > > > > > +{ > > > > > + > > > > > + return &efi_guid_capsule_root_cert_guid; > > > > > +} > > > > > + > > > > > +/** > > > > > + * efi_capsule_authenticate() - Authenticate a uefi capsule > > > > > + * > > > > > + * @capsule: Capsule file with the authentication > > > > > + * header > > > > > + * @capsule_size: Size of the entire capsule > > > > > + * @image: pointer to the image payload minus the > > > > > + * authentication header > > > > > + * @image_size: size of the image payload > > > > > + * > > > > > + * Authenticate the capsule signature with the public key contained > > > > > + * in the root certificate stored as an efi environment variable > > > > > + * > > > > > + * Return: EFI_SUCCESS on successfull authentication or > > > > > + * EFI_SECURITY_VIOLATION on authentication failure > > > > > + */ > > > > > +efi_status_t efi_capsule_authenticate(const void *capsule, > > > > > + efi_uintn_t capsule_size, > > > > > + void **image, efi_uintn_t > > > > *image_size) > > > > > +{ > > > > > + uint64_t monotonic_count; > > > > > + struct efi_signature_store *truststore; > > > > > + struct pkcs7_message *capsule_sig; > > > > > + struct efi_image_regions *regs; > > > > > + struct efi_firmware_image_authentication *auth_hdr; > > > > > + efi_status_t status; > > > > > + > > > > > + status = EFI_SECURITY_VIOLATION; > > > > > + capsule_sig = NULL; > > > > > + truststore = NULL; > > > > > + regs = NULL; > > > > > + > > > > > + /* Sanity checks */ > > > > > + if (capsule == NULL || capsule_size == 0) > > > > > + goto out; > > > > > + > > > > > + auth_hdr = (struct efi_firmware_image_authentication *)capsule; > > > > > + if (capsule_size < sizeof(*auth_hdr)) > > > > > + goto out; > > > > > + > > > > > + if (auth_hdr->auth_info.hdr.dwLength <= > > > > > + offsetof(struct win_certificate_uefi_guid, cert_data)) > > > > > + goto out; > > > > > + > > > > > + if (guidcmp(&auth_hdr->auth_info.cert_type, > > > > &efi_guid_cert_type_pkcs7)) > > > > > + goto out; > > > > > + > > > > > + *image = (uint8_t *)capsule + > > sizeof(auth_hdr->monotonic_count) + > > > > > + auth_hdr->auth_info.hdr.dwLength; > > > > > + *image_size = capsule_size - auth_hdr->auth_info.hdr.dwLength - > > > > > + > > sizeof(auth_hdr->monotonic_count); > > > > > + memcpy(&monotonic_count, &auth_hdr->monotonic_count, > > > > > + sizeof(monotonic_count)); > > > > > + > > > > > + /* data to be digested */ > > > > > + regs = calloc(sizeof(*regs) + sizeof(struct image_region) * 2, > > 1); > > > > > + if (!regs) > > > > > + goto out; > > > > > + > > > > > + regs->max = 2; > > > > > + efi_image_region_add(regs, (uint8_t *)*image, > > > > > + (uint8_t *)*image + *image_size, 1); > > > > > + > > > > > + efi_image_region_add(regs, (uint8_t *)&monotonic_count, > > > > > + (uint8_t *)&monotonic_count + > > > > sizeof(monotonic_count), > > > > > + 1); > > > > > > > > Is the order of regions to be calculated correct? > > > > It seems that 'monotonic_count' precedes 'image' itself > > > > in a capsule header. > > > > > > > > > > Does that have any impact on the hash value computed. > > > > Not 100% sure, but if it doesn't, it cannot guarantee uniqueness > > of digest values. > > > > Will check this. > > -sughosh > > > > > > Thanks, > > -Takahiro Akashi > > > > > I did not see any > > > difference in the hash value computed due to the order in which the > > regions > > > are added. But that could be because of the way the hash value gets > > > computed at the time of building the capsule. I will check this out. > > > > > > > > > > > > > > > + > > > > > + capsule_sig = > > efi_parse_pkcs7_header(auth_hdr->auth_info.cert_data, > > > > > + > > > > auth_hdr->auth_info.hdr.dwLength > > > > > + - > > > > sizeof(auth_hdr->auth_info)); > > > > > + if (IS_ERR(capsule_sig)) { > > > > > > > > As Patrick reported, ex-efi_variable_parse_signature() > > > > returns NULL on error cases, this check should be "!capsule_sig." > > > > > > > > > > Will change. > > > > > > -sughosh > >