Heinrich,
On Fri, Jul 03, 2020 at 12:56:55PM +0200, Heinrich Schuchardt wrote:
> On 09.06.20 07:09, AKASHI Takahiro wrote:
> > UEFI specification requires that we shall support three type of
> > certificates of authenticode in PE image:
> > WIN_CERT_TYPE_EFI_GUID with the guid, EFI_CERT_TYPE_PCKS7_GUID
> > WIN_CERT_TYPE_PKCS_SIGNED_DATA
> > WIN_CERT_TYPE_EFI_PKCS1_15
> >
> > As EDK2 does, we will support the first two that are pkcs7 SignedData.
> >
> > Signed-off-by: AKASHI Takahiro <[email protected]>
> > ---
> > lib/efi_loader/efi_image_loader.c | 56 ++++++++++++++++++++++++-------
> > 1 file changed, 44 insertions(+), 12 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_image_loader.c
> > b/lib/efi_loader/efi_image_loader.c
> > index 5b00fea2f113..38b2c24ab1d6 100644
> > --- a/lib/efi_loader/efi_image_loader.c
> > +++ b/lib/efi_loader/efi_image_loader.c
> > @@ -484,7 +484,8 @@ static bool efi_image_authenticate(void *efi, size_t
> > efi_size)
> > struct efi_signature_store *db = NULL, *dbx = NULL;
> > struct x509_certificate *cert = NULL;
> > void *new_efi = NULL;
> > - size_t new_efi_size;
> > + u8 *auth, *wincerts_end;
> > + size_t new_efi_size, auth_size;
> > bool ret = false;
> >
> > if (!efi_secure_boot_enabled())
> > @@ -533,21 +534,52 @@ static bool efi_image_authenticate(void *efi, size_t
> > efi_size)
> > }
> >
> > /* go through WIN_CERTIFICATE list */
> > - for (wincert = wincerts;
> > - (void *)wincert < (void *)wincerts + wincerts_len;
> > - wincert = (void *)wincert + ALIGN(wincert->dwLength, 8)) {
> > - if (wincert->dwLength < sizeof(*wincert)) {
> > - EFI_PRINT("%s: dwLength too small: %u < %zu\n",
> > - __func__, wincert->dwLength,
> > - sizeof(*wincert));
> > - goto err;
> > + for (wincert = wincerts, wincerts_end = (u8 *)wincerts + wincerts_len;
> > + (u8 *)wincert < wincerts_end;
>
> Shouldn't you compare the end of the current certificate to wincerts_end?
No.
> ((u8 *)wincert + ALIGN(wincert->dwLength, 8))) <= wincerts_end
>
> But you doing such a comparison anyway two lines below. So there is no
> need to duplicate the test here.
>
> > + wincert = (WIN_CERTIFICATE *)
> > + ((u8 *)wincert + ALIGN(wincert->dwLength, 8))) {
> > + if ((u8 *)wincert + sizeof(*wincert) >= wincerts_end)
> > + break;
Is this the line that you mentioned as "two lines below"?
If so, the check is not the exact same.
As an image is provided by a *user*, we have to be as careful
as possible in handling data in an image.
In this case, the value of size in a header may not always be
correct (or at least, can be corrupted). So we will make sure
that we have enough data for accessing a header at the beginning.
> Why is this >= and not >?
Because "<start> + <size>" would be exclusive.
So I will keep my code unchanged.
-Takahiro Akashi
> Best regards
>
> Heinrich
>
> > +
> > + if (wincert->dwLength <= sizeof(*wincert)) {
> > + EFI_PRINT("dwLength too small: %u < %zu\n",
> > + wincert->dwLength, sizeof(*wincert));
> > + continue;
> > + }
> > +
> > + EFI_PRINT("WIN_CERTIFICATE_TYPE: 0x%x\n",
> > + wincert->wCertificateType);
> > +
> > + auth = (u8 *)wincert + sizeof(*wincert);
> > + auth_size = wincert->dwLength - sizeof(*wincert);
> > + if (wincert->wCertificateType == WIN_CERT_TYPE_EFI_GUID) {
> > + if (auth + sizeof(efi_guid_t) >= wincerts_end)
> > + break;
> > +
> > + if (auth_size <= sizeof(efi_guid_t)) {
> > + EFI_PRINT("dwLength too small: %u < %zu\n",
> > + wincert->dwLength, sizeof(*wincert));
> > + continue;
> > + }
> > + if (guidcmp(auth, &efi_guid_cert_type_pkcs7)) {
> > + EFI_PRINT("Certificate type not supported:
> > %pUl\n",
> > + auth);
> > + continue;
> > + }
> > +
> > + auth += sizeof(efi_guid_t);
> > + auth_size -= sizeof(efi_guid_t);
> > + } else if (wincert->wCertificateType
> > + != WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
> > + EFI_PRINT("Certificate type not supported\n");
> > + continue;
> > }
> > - msg = pkcs7_parse_message((void *)wincert + sizeof(*wincert),
> > - wincert->dwLength - sizeof(*wincert));
> > +
> > + msg = pkcs7_parse_message(auth, auth_size);
> > if (IS_ERR(msg)) {
> > EFI_PRINT("Parsing image's signature failed\n");
> > msg = NULL;
> > - goto err;
> > + continue;
> > }
> >
> > /* try black-list first */
> >
>
