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 A77FFC433EF for ; Mon, 11 Apr 2022 08:40:44 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 3737583B32; Mon, 11 Apr 2022 10:40:42 +0200 (CEST) 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="Sl16Muq2"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 7873C83B34; Mon, 11 Apr 2022 10:40:40 +0200 (CEST) Received: from mail-wr1-x433.google.com (mail-wr1-x433.google.com [IPv6:2a00:1450:4864:20::433]) (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 3A45783AEE for ; Mon, 11 Apr 2022 10:40:37 +0200 (CEST) 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-x433.google.com with SMTP id e8so5484980wra.7 for ; Mon, 11 Apr 2022 01:40:37 -0700 (PDT) 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=ubqFrwEYXCapYJ+ygLWJXpIl9aU8FdiMxT38IVrdVhM=; b=Sl16Muq2RoDnCPEwEXE9P18ZfkXj9BStVwlLR087uw7bq51O+LODAQrP1JFAih30MP j3oMO6xI5hxyS7D5QfakwXdyymCUvcOP1sL/20G2H3TMR5WBpiTeZF8xy0bFvGB26seY TgTQuj8ZQqX543rtoGZuz9U4dl46kxjl+ZAVob6/EBUGo+tJc4PgaU799QjbNgrwAT45 DMS1FRN7tdKakEVuNdJCaItbs1bcFMvpL9qhNodrWy7npb7u2JPMQoGvQOG2G3pGxLpa a1A2arYbLCdiGde2+i8nVYcFSeoHwApj8HEttME3hglAuYxU6gPayakcYHQye8CJR01/ 8Omw== 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=ubqFrwEYXCapYJ+ygLWJXpIl9aU8FdiMxT38IVrdVhM=; b=jNQxr/92/suufPMvUgApN+3IhJDNSe6GNWa8P1YnBKq8Ga+76xqN0YU6u0Iw67zC6S uAdXd7pzdHgxmX9LZxT5nna6gQLTyNwEKgyg+eSpD/hJoolz9XuVcQon/rF83OjXlNK9 ska+y+ufTkc7flPcw9EOVj2Nabi221BmY1bSUGoKJLhAtu2oNBQ3n9/YQl06nQYv4cAf 3KFkIpBothdoIpf8/jCQ4kOc5LTDzaYkTaUlQYOAfrgCW1eWSuavHikWuE/rV9+A9/8F iV5wDaSFPAoHodwQrJJCYlXqd5yBTqp28mfMM9qp+1dHAvpOi0kVkUTr8jxrfdwxNleW n1JA== X-Gm-Message-State: AOAM531mVoRwgovYX6Rx2Kl/O9MY4xrUs7NPSMpmW5BC7532hGuGeyvS AsEgPBAGGcdr7a2LV4ubbVw3iA== X-Google-Smtp-Source: ABdhPJzkL59e0EzTca+DGK9OW/EMJFlXzbHVQsbs0SVRZh6o/xnG4a75VPujzbtKMWXEAcJIodRXWg== X-Received: by 2002:adf:ec41:0:b0:1ed:beee:6f8f with SMTP id w1-20020adfec41000000b001edbeee6f8fmr23560199wrn.110.1649666436690; Mon, 11 Apr 2022 01:40:36 -0700 (PDT) Received: from hera (athedsl-4461682.home.otenet.gr. [94.71.4.98]) by smtp.gmail.com with ESMTPSA id o4-20020a5d6484000000b002057ad822d4sm27597334wri.48.2022.04.11.01.40.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 11 Apr 2022 01:40:36 -0700 (PDT) Date: Mon, 11 Apr 2022 11:40:34 +0300 From: Ilias Apalodimas To: AKASHI Takahiro , xypron.glpk@gmx.de, Stuart.Yoder@arm.com, paul.liu@linaro.org, u-boot@lists.denx.de Subject: Re: [RFC PATCH] efi_loader: add sha384/512 on certificate revocation Message-ID: References: <20220411075622.2069454-1-ilias.apalodimas@linaro.org> <20220411083108.GA47465@laputa> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220411083108.GA47465@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 Hi Akashi-san, > On Mon, Apr 11, 2022 at 05:31:08PM +0900, AKASHI Takahiro wrote: > On Mon, Apr 11, 2022 at 10:56:22AM +0300, Ilias Apalodimas wrote: > > Currently we don't support sha384/512 for the X.509 > > certificate To-Be-Signed contents. Moreover if we come across such a > > hash we skip the check and approve the image, although the image > > might needs to be rejected. > > Are you sure? You seem to be talking about efi_signature_check_revocation() here. > Please be more specific. Arm has a security ACS testsuite [1]. The whole checking fails exactly on this bug. > > > It's worth noting here that efi_hash_regions() can now be reused from > > efi_signature_lookup_digest() and add sha348/512 support there as well > > > > Signed-off-by: Ilias Apalodimas > > --- > > include/efi_api.h | 6 ++++ > > lib/efi_loader/efi_signature.c | 62 ++++++++++++++++++++++++++-------- > > 2 files changed, 53 insertions(+), 15 deletions(-) > > > > diff --git a/include/efi_api.h b/include/efi_api.h > > index 982c2001728d..b9a04958f9ba 100644 > > --- a/include/efi_api.h > > +++ b/include/efi_api.h > > @@ -1873,6 +1873,12 @@ struct efi_system_resource_table { > > #define EFI_CERT_X509_SHA256_GUID \ > > EFI_GUID(0x3bd2a492, 0x96c0, 0x4079, 0xb4, 0x20, \ > > 0xfc, 0xf9, 0x8e, 0xf1, 0x03, 0xed) > > +#define EFI_CERT_X509_SHA384_GUID \ > > + EFI_GUID(0x7076876e, 0x80c2, 0x4ee6, \ > > + 0xaa, 0xd2, 0x28, 0xb3, 0x49, 0xa6, 0x86, 0x5b) > > +#define EFI_CERT_X509_SHA512_GUID \ > > + EFI_GUID(0x446dbf63, 0x2502, 0x4cda, \ > > + 0xbc, 0xfa, 0x24, 0x65, 0xd2, 0xb0, 0xfe, 0x9d) > > #define EFI_CERT_TYPE_PKCS7_GUID \ > > EFI_GUID(0x4aafd29d, 0x68df, 0x49ee, 0x8a, 0xa9, \ > > 0x34, 0x7d, 0x37, 0x56, 0x65, 0xa7) > > diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c > > index 79ed077ae7dd..392eae6c0d64 100644 > > --- a/lib/efi_loader/efi_signature.c > > +++ b/lib/efi_loader/efi_signature.c > > @@ -24,6 +24,8 @@ const efi_guid_t efi_guid_sha256 = EFI_CERT_SHA256_GUID; > > const efi_guid_t efi_guid_cert_rsa2048 = EFI_CERT_RSA2048_GUID; > > const efi_guid_t efi_guid_cert_x509 = EFI_CERT_X509_GUID; > > const efi_guid_t efi_guid_cert_x509_sha256 = EFI_CERT_X509_SHA256_GUID; > > +const efi_guid_t efi_guid_cert_x509_sha384 = EFI_CERT_X509_SHA384_GUID; > > +const efi_guid_t efi_guid_cert_x509_sha512 = EFI_CERT_X509_SHA512_GUID; > > Please add, at least, one test case along with this patch > if you want to expand the coverage of UEFI specification. Sure, once the RFC is discussed > > > const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; > > > > static u8 pkcs7_hdr[] = { > > @@ -124,23 +126,32 @@ struct pkcs7_message *efi_parse_pkcs7_header(const void *buf, > > * Return: true on success, false on error > > */ > > static bool efi_hash_regions(struct image_region *regs, int count, > > - void **hash, size_t *size) > > + void **hash, size_t size) > > It would be better to hand off either an algorithm name or a corresponding guid > rather than "size" as an identification of hash algo for extending this function > in the future. > That's exactly what happens here. There's a guid_to_sha_len() that derives the len for us. I'd rather keep this as is, since the impact on the rest of the code is minimal. > > { > > + char hash_algo[16]; > > + int ret; > > + > > + /* basic sanity checking */ > > + if (!size) > > + return false; > > + > > + ret = snprintf(hash_algo, sizeof(hash_algo), "sha%ld", size * 8); > > + if (ret >= sizeof(hash_algo)) > > + return false; > > + > > if (!*hash) { > > - *hash = calloc(1, SHA256_SUM_LEN); > > + *hash = calloc(1, size); > > if (!*hash) { > > EFI_PRINT("Out of memory\n"); > > return false; > > } > > } > > - if (size) > > - *size = SHA256_SUM_LEN; > > > > - hash_calculate("sha256", regs, count, *hash); > > + hash_calculate(hash_algo, regs, count, *hash); > > #ifdef DEBUG > > EFI_PRINT("hash calculated:\n"); > > print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, > > - *hash, SHA256_SUM_LEN, false); > > + *hash, size, false); > > #endif > > > > return true; > > @@ -190,7 +201,7 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs, > > struct efi_signature_store *siglist; > > struct efi_sig_data *sig_data; > > void *hash = NULL; > > - size_t size = 0; > > + size_t size = SHA256_SUM_LEN; > > bool found = false; > > bool hash_done = false; > > > > @@ -216,7 +227,7 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs, > > continue; > > > > if (!hash_done && > > - !efi_hash_regions(regs->reg, regs->num, &hash, &size)) { > > + !efi_hash_regions(regs->reg, regs->num, &hash, size)) { > > EFI_PRINT("Digesting an image failed\n"); > > break; > > } > > @@ -263,7 +274,7 @@ static bool efi_lookup_certificate(struct x509_certificate *cert, > > struct efi_sig_data *sig_data; > > struct image_region reg[1]; > > void *hash = NULL, *hash_tmp = NULL; > > - size_t size = 0; > > + size_t size = SHA256_SUM_LEN; > > bool found = false; > > > > EFI_PRINT("%s: Enter, %p, %p\n", __func__, cert, db); > > @@ -278,7 +289,7 @@ static bool efi_lookup_certificate(struct x509_certificate *cert, > > /* calculate hash of TBSCertificate */ > > reg[0].data = cert->tbs; > > reg[0].size = cert->tbs_size; > > - if (!efi_hash_regions(reg, 1, &hash, &size)) > > + if (!efi_hash_regions(reg, 1, &hash, size)) > > goto out; > > > > EFI_PRINT("%s: searching for %s\n", __func__, cert->subject); > > @@ -300,7 +311,7 @@ static bool efi_lookup_certificate(struct x509_certificate *cert, > > cert_tmp->subject); > > reg[0].data = cert_tmp->tbs; > > reg[0].size = cert_tmp->tbs_size; > > - if (!efi_hash_regions(reg, 1, &hash_tmp, NULL)) > > + if (!efi_hash_regions(reg, 1, &hash_tmp, size)) > > goto out; > > > > x509_free_certificate(cert_tmp); > > @@ -377,6 +388,26 @@ out: > > return verified; > > } > > > > +/** guid_to_sha_len - return the sha size in bytes for a given guid > > + * used of EFI security databases > > + * > > + * @guid: guid to check > > + * > > + * Return: len or 0 if no match is found > > + */ > > +static int guid_to_sha_len(efi_guid_t *guid) > > +{ > > + int size = 0; > > + > > + if (!guidcmp(guid, &efi_guid_cert_x509_sha256)) > > + size = SHA256_SUM_LEN; > > + else if (!guidcmp(guid, &efi_guid_cert_x509_sha384)) > > + size = SHA384_SUM_LEN; > > + else if (!guidcmp(guid, &efi_guid_cert_x509_sha512)) > > + size = SHA512_SUM_LEN; > > + > > + return size; > > +} > > /** > > * efi_signature_check_revocation - check revocation with dbx > > * @sinfo: Signer's info > > @@ -400,7 +431,7 @@ static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo, > > struct efi_sig_data *sig_data; > > struct image_region reg[1]; > > void *hash = NULL; > > - size_t size = 0; > > + size_t size = SHA256_SUM_LEN; > > time64_t revoc_time; > > bool revoked = false; > > > > @@ -411,13 +442,14 @@ static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo, > > > > EFI_PRINT("Checking revocation against %s\n", cert->subject); > > for (siglist = dbx; siglist; siglist = siglist->next) { > > - if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509_sha256)) > > + size = guid_to_sha_len(&siglist->sig_type); > > + if (!size) > > Even with this change, > > Moreover if we come across such a > > hash we skip the check and approve the image, although the image > > might needs to be rejected. > > the behavior won't be fixed. > Are there other x509_shaxxx for dbx that the spec describes? > -Takahiro Akashi > > > continue; > > > > /* calculate hash of TBSCertificate */ > > reg[0].data = cert->tbs; > > reg[0].size = cert->tbs_size; > > - if (!efi_hash_regions(reg, 1, &hash, &size)) > > + if (!efi_hash_regions(reg, 1, &hash, size)) > > goto out; > > > > for (sig_data = siglist->sig_data_list; sig_data; > > @@ -500,7 +532,7 @@ bool efi_signature_verify(struct efi_image_regions *regs, > > */ > > if (!msg->data && > > !efi_hash_regions(regs->reg, regs->num, > > - (void **)&sinfo->sig->digest, NULL)) { > > + (void **)&sinfo->sig->digest, SHA256_SUM_LEN)) { > > EFI_PRINT("Digesting an image failed\n"); > > goto out; > > } > > -- > > 2.32.0 > > [1] https://github.com/ARM-software/arm-systemready/tree/security-extension-acs Thanks /Ilias