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 91971C433F5 for ; Thu, 10 Feb 2022 07:13:45 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 8463880FAE; Thu, 10 Feb 2022 08:13:43 +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="j+50fhmA"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 3FB5F8091B; Thu, 10 Feb 2022 08:13:41 +0100 (CET) Received: from mail-wr1-x432.google.com (mail-wr1-x432.google.com [IPv6:2a00:1450:4864:20::432]) (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 0828D8091B for ; Thu, 10 Feb 2022 08:13:38 +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-wr1-x432.google.com with SMTP id d27so4040002wrb.5 for ; Wed, 09 Feb 2022 23:13:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=ug5kajyaoAheq08qxFeVjLnUnC9NhDw+QTq5jnN7Ask=; b=j+50fhmAitbA1U0bARCLEkiSg471V8+83bWDuN/3Qz32NgNneQ5xYPMELtSF8TnC3p yxiIoFbWu8026mDBOnGNlIWHNMUFQ3bNMlTfg4HVn+tTHy84wKy8hMrdkHofHYU73Bzc JtMQLwKvWYsSTlmpREqhZLjC8mHtEzhpgw6cl8GTY2PfsnfZJfRZMQm8meFAZc86NBF4 +97uT8oNCNCbe/AAXYI6QIrniRPc3JiqEGrIlqbGjdlXlrJnM7sqla6gTQVGaRF+3TzN rYYghApShw1EVRn75jfj4C5jXMMvy03idyoitNoVAPwKxDQLrI7cHqWD4F5dyXhX2vgU X+wA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=ug5kajyaoAheq08qxFeVjLnUnC9NhDw+QTq5jnN7Ask=; b=H6n/qr9bpDuiaTSyBgoLD6hkwshXGToUVAC9s/zbBxCt0aemiSUJlCLDUARGdwr/Ls 3uFza7AtWZZqlTJlalVXeWweg7nqrbjQabEIBwJpxeWEMJbXfKK6q/f8NwgNTOnhfdQE Pb+neTHdkWpTpCoZSH0n+X8p61EqXSNs/+Htpp07aIBtVP12y3zBi9Nj3Bsjbo5SS8yo n4dVcGeaRV82p2AMVR6OJr0zz3qQTYroBFJO+ytyvrem3k7Cc0coSMbX55A705wefnjt oBKldixEdD2pF5qFpe/e29kAiY9dJEMV71caEQcOIAU1vsu+bCTUG6PiJr8VL3DpTRoi WJnQ== X-Gm-Message-State: AOAM531JolUM+HiooUojWyGuZcwcPwVlE77b6u1gFfe9XuVlEJPMc7xV fRVXa4wiyMWLBiTknc6BUQFx4A== X-Google-Smtp-Source: ABdhPJyzvMrsqBjHv/qhA4hqeSbmvUH7ybd0fbWEtgTryu+FKtG2NYMmuNnpaGKdbJdrwVsBfHsPmg== X-Received: by 2002:a5d:6da8:: with SMTP id u8mr5350471wrs.362.1644477217408; Wed, 09 Feb 2022 23:13:37 -0800 (PST) Received: from hades (athedsl-4461669.home.otenet.gr. [94.71.4.85]) by smtp.gmail.com with ESMTPSA id g4sm20461319wrd.111.2022.02.09.23.13.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 09 Feb 2022 23:13:36 -0800 (PST) Date: Thu, 10 Feb 2022 09:13:34 +0200 From: Ilias Apalodimas To: AKASHI Takahiro , xypron.glpk@gmx.de, u-boot@lists.denx.de Subject: Re: [RFC PATCH 1/2] efi_loader: fix dual signed image certification Message-ID: References: <20220204073202.4141198-1-ilias.apalodimas@linaro.org> <20220210051348.GD12412@laputa> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220210051348.GD12412@laputa> 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 On Thu, Feb 10, 2022 at 02:13:48PM +0900, AKASHI Takahiro wrote: > Hi Ilias, > > Thank you for reviewing the logic. > > On Fri, Feb 04, 2022 at 09:32:01AM +0200, Ilias Apalodimas wrote: > > The EFI spec allows for images to carry multiple signatures. Currently > > we don't adhere to the verification process for such images. > > In this patch, you're trying to do three things: > * remove efi_image_unsigned_authenticate() > * pull efi_signature_lookup_digest() out of a while loop > * change the logic of authentication > > I'd prefer to see those changes in separate patches for better reviewing. I tried both and the current one seemed easier to review. Heinrich any preference? > > > The spec says: > > "Multiple signatures are allowed to exist in the binary's certificate > > table (as per PE/COFF Section "Attribute Certificate Table"). Only one > > hash or signature is required to be present in db in order to pass > > validation, so long as neither the SHA-256 hash of the binary nor any > > present signature is reflected in dbx." > > I have some concern about what the last phrase, "neither the SHA-256 hash > of the binary nor any present signature is reflected in dbx" means. > See the comment below. > > > With our current implementation signing the image with two certificates > > and inserting both of them in db and one of them dbx doesn't always reject > > the image. The rejection depends on the order that the image was signed > > and the order the certificates are read (and checked) in db. > > > > While at it move the sha256 hash verification outside the signature > > checking loop, since it only needs to run once per image and get simplify > > the logic for authenticating an unsigned imahe using sha256 hashes. > > > > Signed-off-by: Ilias Apalodimas > > --- > > lib/efi_loader/efi_image_loader.c | 88 +++++++------------------------ > > 1 file changed, 18 insertions(+), 70 deletions(-) > > > > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c > > index f41cfa4fccd5..5df35939f702 100644 > > --- a/lib/efi_loader/efi_image_loader.c > > +++ b/lib/efi_loader/efi_image_loader.c > > @@ -516,53 +516,6 @@ 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(u"dbx"); > > - if (!dbx) { > > - EFI_PRINT("Getting signature database(dbx) failed\n"); > > - goto out; > > - } > > - > > - db = efi_sigstore_parse_sigdb(u"db"); > > - if (!db) { > > - EFI_PRINT("Getting signature database(db) failed\n"); > > - goto out; > > - } > > - > > - /* try black-list first */ > > - if (efi_signature_lookup_digest(regs, dbx, true)) { > > - 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, false)) > > - 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 > > @@ -608,14 +561,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 +570,18 @@ static bool efi_image_authenticate(void *efi, size_t efi_size) > > db = efi_sigstore_parse_sigdb(u"db"); > > if (!db) { > > EFI_PRINT("Getting signature database(db) failed\n"); > > - goto err; > > + goto out; > > } > > > > dbx = efi_sigstore_parse_sigdb(u"dbx"); > > if (!dbx) { > > EFI_PRINT("Getting signature database(dbx) failed\n"); > > - goto err; > > + goto out; > > } > > > > if (efi_signature_lookup_digest(regs, dbx, true)) { > > EFI_PRINT("Image's digest was found in \"dbx\"\n"); > > - goto err; > > + goto out; > > } > > > > /* > > @@ -678,7 +624,8 @@ static bool efi_image_authenticate(void *efi, size_t efi_size) > > if (guidcmp(auth, &efi_guid_cert_type_pkcs7)) { > > EFI_PRINT("Certificate type not supported: %pUs\n", > > auth); > > - continue; > > + ret = false; > > + goto out; > > Why should we break the loop here? We were trying to reject signature verification that we don't support, since the equivalent cert might be in dbx. But I am not 100% sure taht's what we want here. > > > } > > > > auth += sizeof(efi_guid_t); > > @@ -686,7 +633,8 @@ static bool efi_image_authenticate(void *efi, size_t efi_size) > > } else if (wincert->wCertificateType > > != WIN_CERT_TYPE_PKCS_SIGNED_DATA) { > > EFI_PRINT("Certificate type not supported\n"); > > - continue; > > + ret = false; > > + goto out; > > } > > > > msg = pkcs7_parse_message(auth, auth_size); > > @@ -717,32 +665,32 @@ static bool efi_image_authenticate(void *efi, size_t efi_size) > > */ > > /* try black-list first */ > > if (efi_signature_verify_one(regs, msg, dbx)) { > > + ret = false; > > EFI_PRINT("Signature was rejected by \"dbx\"\n"); > > - continue; > > + goto out; > > If we go to "out" here, we have no chance to verify some cases: > 1) An image has two signatures, for instance, one signed by SHA1 cert > and the other signed by SHA256 cert. A user wants to reject SHA1 cert > and put the cert in dbx. I am not sure I am following, what does he gain be rejecting the SHA1 portion only? Avoid potential collisions? > But this image can and should yet be verified by SHA256 cert. Why should it be verified? My understanding of the EFI spec is that any match in dbx of any certificate in the signing chain of the signature being verified means reject the image. > 2) A user knows that a given image is safe for some reason even though > he or she doesn't trust the certficate which is used for signing > the image. > > -Takahiro Akashi > > > } > > > > if (!efi_signature_check_signers(msg, dbx)) { > > + ret = false; > > EFI_PRINT("Signer(s) in \"dbx\"\n"); > > - continue; > > + goto out; > > } > > > > /* try white-list */ > > if (efi_signature_verify(regs, msg, db, dbx)) { > > ret = true; > > - break; > > + continue; > > } > > > > EFI_PRINT("Signature was not verified by \"db\"\n"); > > + } > > > > - if (efi_signature_lookup_digest(regs, db, false)) { > > - ret = true; > > - break; > > - } > > > > - EFI_PRINT("Image's digest was not found in \"db\" or \"dbx\"\n"); > > - } > > + /* last resort try the image sha256 hash in db */ > > + if (!ret && efi_signature_lookup_digest(regs, db, false)) > > + ret = true; > > > > -err: > > +out: > > efi_sigstore_free(db); > > efi_sigstore_free(dbx); > > pkcs7_free_message(msg); > > -- > > 2.32.0 > > Thanks /Ilias