From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 86917C433F5 for ; Thu, 27 Jan 2022 21:48:03 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id B38C2830AB; Thu, 27 Jan 2022 22:48:00 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="kWnoSpz7"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id B10C0832DD; Thu, 27 Jan 2022 22:47:58 +0100 (CET) Received: from mail-yb1-xb2b.google.com (mail-yb1-xb2b.google.com [IPv6:2607:f8b0:4864:20::b2b]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 0E3E4830AB for ; Thu, 27 Jan 2022 22:47:55 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ilias.apalodimas@linaro.org Received: by mail-yb1-xb2b.google.com with SMTP id c6so12820783ybk.3 for ; Thu, 27 Jan 2022 13:47:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=NlESUO7m98ksJ9VSbS9SIvJPqB/c2sExHYgy73gdSMs=; b=kWnoSpz7dj58R3JXNAcpWVnparCFDfEt6ysPQLVK2EZR3rpyx+wt5uvaaEdd/4tDDs 9YtM6Deuvu3RnUEtvrMaId39IFDkfOAsasvlp8hDwQhBUhYd/diXleBXxohug8ihEAFc /wWlN4LLSti2BrYlstO43g6AvQDhTLHF6d85bBbdTQeDpJdKpuIZJK7hcBwbQQeE8Udh k/mZ0dQYPbnLPHDpJ9lrXTqhVrpMC8nt4SDoALtu91xW8iWLgYbcUdyfyR8uAKtrK/NK f7BOGiTQZkHUOa7wn4yxBm0AhjhLmKG9X+R/zEamBBY4gf4GUBt2n5tnh1y1yS19ZaFT ybCA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=NlESUO7m98ksJ9VSbS9SIvJPqB/c2sExHYgy73gdSMs=; b=uyHXU1KUfs7ae6oKlkGqZtWWod3239yr1VQM+dIz2fq7ymTOuSmPl/sjzExseLSj2j LaKBBAp2sIayYMS9mgK8a33E0PNvU4CKO2DVYQ61wsg27P3jHkYv6lej6AhDSCdltV3J oTGEckq1kp/H6x5zr8fY/W+zpHw2XqnYguzwTbKqv3A7r6VYqkTqRk4V5DqeKBelY2rk Ptuv3/Wp7b06czwOArzv7dURCE8EDzecIvjE3koJfCc6fWdUW8tUySEtRjeduK7JfpqD 0Dw04U65LkFmuixSNHuyZFN14Z93nhVH0kvuWIcInpbS4+DCqbQsflJOsaD/B6NlnAUr Khbw== X-Gm-Message-State: AOAM531uZz28M0ynB+OXClCQZ8fYJyC3nfyBWE+B1fhxEC1Cp8f+NvIL ZdCxbNVEVNUT1k0xDGN47p8JwGwisOPNMe4F9J6NBQ== X-Google-Smtp-Source: ABdhPJyDkq/CK9pR9shUIKBHC7g6lkVqiddHNYtNAMiyk0cP4ISZL+xexmPhTbut4qmxMCp+FRRROPyAxa29Nv4KTRI= X-Received: by 2002:a25:da97:: with SMTP id n145mr8132300ybf.687.1643320073085; Thu, 27 Jan 2022 13:47:53 -0800 (PST) MIME-Version: 1.0 References: <20220125144629.712580-1-ilias.apalodimas@linaro.org> In-Reply-To: <20220125144629.712580-1-ilias.apalodimas@linaro.org> From: Ilias Apalodimas Date: Thu, 27 Jan 2022 23:47:17 +0200 Message-ID: Subject: Re: [PATCH 1/1] efi_loader: clean up uefi secure boot image verification logic To: xypron.glpk@gmx.de, takahiro.akashi@linaro.org Cc: u-boot@lists.denx.de Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean Heinrich, On Tue, 25 Jan 2022 at 16:46, Ilias Apalodimas wrote: > > We currently distinguish between signed and non signed PE/COFF executables > while trying to authenticate signatures and/or sha256 hashes in db and dbx. > That code duplication can be avoided. When checking for sha256 hashes, > in db, we don't really care if the image is signed or not. The sequence > for verifying the binary can be > 1. Is the sha256 of the image in dbx, reject the image > 2. Certification checking > 2a. Is the image signed with a certificate that's found in db and > not in dbx > 2b. The image carries a cert which is signed by a cert in db (and > not in dbx) and the image can be verified against the former > 3. Is the sha256 of the image in db > > So weed out the 'special' case, which is handling unsigned images. > > While at it move the checking of a hash outside the certificate > verification loop which isn't really needed and check for the hash > only once. Also allow a mix of signatures and hashes in db instead > of explicitly breaking out the SHA256 verification loop if a signature > happens to be the present before the hash. > > It's worth noting that (2a) only works if the certificate is self > signed, but that can be fixed in a following patch. I've found more unsupported cases in our existing code while testing. Please ignore this patchset I'll send updates which supersede it Thanks /Ilias > > Signed-off-by: Ilias Apalodimas > --- > changes since RFC: > - added a comment indication sha256 approval is the last resort case > - reworded the commit message, based on Takahiro's proposal > > lib/efi_loader/efi_image_loader.c | 89 +++++++------------------------ > lib/efi_loader/efi_signature.c | 2 +- > 2 files changed, 20 insertions(+), 71 deletions(-) > > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c > index 255613eb72ba..90594acf9623 100644 > --- a/lib/efi_loader/efi_image_loader.c > +++ b/lib/efi_loader/efi_image_loader.c > @@ -516,63 +516,17 @@ err: > } > > #ifdef CONFIG_EFI_SECURE_BOOT > -/** > - * efi_image_unsigned_authenticate() - authenticate unsigned image with > - * SHA256 hash > - * @regs: List of regions to be verified > - * > - * If an image is not signed, it doesn't have a signature. In this case, > - * its message digest is calculated and it will be compared with one of > - * hash values stored in signature databases. > - * > - * Return: true if authenticated, false if not > - */ > -static bool efi_image_unsigned_authenticate(struct efi_image_regions *regs) > -{ > - struct efi_signature_store *db = NULL, *dbx = NULL; > - bool ret = false; > - > - dbx = efi_sigstore_parse_sigdb(L"dbx"); > - if (!dbx) { > - EFI_PRINT("Getting signature database(dbx) failed\n"); > - goto out; > - } > - > - db = efi_sigstore_parse_sigdb(L"db"); > - if (!db) { > - EFI_PRINT("Getting signature database(db) failed\n"); > - goto out; > - } > - > - /* try black-list first */ > - if (efi_signature_lookup_digest(regs, dbx)) { > - EFI_PRINT("Image is not signed and its digest found in \"dbx\"\n"); > - goto out; > - } > - > - /* try white-list */ > - if (efi_signature_lookup_digest(regs, db)) > - ret = true; > - else > - EFI_PRINT("Image is not signed and its digest not found in \"db\" or \"dbx\"\n"); > - > -out: > - efi_sigstore_free(db); > - efi_sigstore_free(dbx); > - > - return ret; > -} > - > /** > * efi_image_authenticate() - verify a signature of signed image > * @efi: Pointer to image > * @efi_size: Size of @efi > * > - * A signed image should have its signature stored in a table of its PE header. > + * An image should have its signature stored in the image certificate table > + * > * So if an image is signed and only if if its signature is verified using > - * signature databases, an image is authenticated. > - * If an image is not signed, its validity is checked by using > - * efi_image_unsigned_authenticated(). > + * signature databases or if it's sha256 is found in db an image is > + * authenticated. > + * > * TODO: > * When AuditMode==0, if the image's signature is not found in > * the authorized database, or is found in the forbidden database, > @@ -608,14 +562,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size) > if (!efi_image_parse(new_efi, efi_size, ®s, &wincerts, > &wincerts_len)) { > EFI_PRINT("Parsing PE executable image failed\n"); > - goto err; > - } > - > - if (!wincerts) { > - /* The image is not signed */ > - ret = efi_image_unsigned_authenticate(regs); > - > - goto err; > + goto out; > } > > /* > @@ -624,18 +571,18 @@ static bool efi_image_authenticate(void *efi, size_t efi_size) > db = efi_sigstore_parse_sigdb(L"db"); > if (!db) { > EFI_PRINT("Getting signature database(db) failed\n"); > - goto err; > + goto out; > } > > dbx = efi_sigstore_parse_sigdb(L"dbx"); > if (!dbx) { > EFI_PRINT("Getting signature database(dbx) failed\n"); > - goto err; > + goto out; > } > > if (efi_signature_lookup_digest(regs, dbx)) { > EFI_PRINT("Image's digest was found in \"dbx\"\n"); > - goto err; > + goto out; > } > > /* > @@ -729,20 +676,22 @@ static bool efi_image_authenticate(void *efi, size_t efi_size) > /* try white-list */ > if (efi_signature_verify(regs, msg, db, dbx)) { > ret = true; > - break; > + goto out; > } > > EFI_PRINT("Signature was not verified by \"db\"\n"); > > - if (efi_signature_lookup_digest(regs, db)) { > - ret = true; > - break; > - } > - > - EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n"); > } > > -err: > + /* > + * This our last resort, the image is not found in dbx and is not > + * authenticated by any certs in db. Try the image hash in db > + */ > + if (efi_signature_lookup_digest(regs, db)) > + ret = true; > + else > + EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n"); > +out: > efi_sigstore_free(db); > efi_sigstore_free(dbx); > pkcs7_free_message(msg); > diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c > index 3243e2c60de0..8fa82f8cea4c 100644 > --- a/lib/efi_loader/efi_signature.c > +++ b/lib/efi_loader/efi_signature.c > @@ -176,7 +176,7 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs, > if (guidcmp(&siglist->sig_type, &efi_guid_sha256)) { > EFI_PRINT("Digest algorithm is not supported: %pUs\n", > &siglist->sig_type); > - break; > + continue; > } > > if (!efi_hash_regions(regs->reg, regs->num, &hash, &size)) { > -- > 2.30.2 >