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 EECF4C433EF for ; Tue, 19 Apr 2022 01:46:57 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id BBCF783868; Tue, 19 Apr 2022 03:46:54 +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="cx4oBcQB"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 7CB97839A7; Tue, 19 Apr 2022 03:46:53 +0200 (CEST) Received: from mail-pg1-x52d.google.com (mail-pg1-x52d.google.com [IPv6:2607:f8b0:4864:20::52d]) (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 6BC4880F91 for ; Tue, 19 Apr 2022 03:46:49 +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=takahiro.akashi@linaro.org Received: by mail-pg1-x52d.google.com with SMTP id u2so22122452pgq.10 for ; Mon, 18 Apr 2022 18:46:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:in-reply-to; bh=HYSD4JMJjFFxjYFFMShjlJheK7TJMJ4RiO2KCB8LXmc=; b=cx4oBcQBN7XGsIrkjYNxEs5vpNGw77ldGnejicXT4TO0g4jU1wTIrQCJOaXlXVGkCs 0tEOKkfBX0lbu8unefnDs9IeCezbDdB+Nps2wYF0HlOG9J5Rj8GhKz/7MBBN4gf89oTn VB2hED4VWCaPpti7WwXcGecK+ApQTo66xxdZoatZ+FvQwD8Y/XY0Ajk4ptCIFYjBX0f3 N52PXFlFzbRTL9o9l7cCxM/uFTAjsygViKDIdhGimVBBOjKLdZNIcK+Wwa8fuCRmxYTw v5zK/FVEl85nFi2AWrApAo6TBd+U6EcPlqJuc2A27Od3nzbl4jXUCBuTgJLl5XRuaM1c EENw== 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:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :in-reply-to; bh=HYSD4JMJjFFxjYFFMShjlJheK7TJMJ4RiO2KCB8LXmc=; b=dMZo9+G5r7fb5xbT2t3vWFE85869QOr7sHIVfAMW687U1MhXpfOO+R1ynBTRONd/kx XEx8FVcy0YENxyK32qRpecjBa6XRaluBapUJdScfXHXJ6eMYW16BRJPh9CHMhX67jVWj sOXqNkXuwchokLh+QmQNPSHMkFalLkl0XsMDwdGuUItFosm+qriZp6xtcMf1JRT42RUj aoao91p6DwD6GHQPXRCC0fpNn07/AB/KrsD1velSJzjM042OVnsSBvprmLS9gGP2BkZp i/qFA22Ro9d6rtvg2LVJ3iIxAGsMtplvhSw+YbitCH6X82xQbK9cR6GHk+MQAeUXInt1 tEBw== X-Gm-Message-State: AOAM531erbvEu11QP8Pm47MwYO89sWsgGlch/LgndPb4RGY3laWtzmf4 8HuKPysOye/A58I1CavlrTtnPQ== X-Google-Smtp-Source: ABdhPJy57XHsVqtQz8HhxGaBDuK4q5Tdy4Vb71OWp33yP7dkYli1v4tk0pLh11FgyimKekqHT7xjbQ== X-Received: by 2002:a63:f40f:0:b0:3aa:2a1c:138c with SMTP id g15-20020a63f40f000000b003aa2a1c138cmr2200291pgi.83.1650332807574; Mon, 18 Apr 2022 18:46:47 -0700 (PDT) Received: from laputa ([2400:4050:c3e1:100:5858:d3a6:5cad:ceeb]) by smtp.gmail.com with ESMTPSA id u20-20020a056a00099400b0050a7685b792sm5397808pfg.213.2022.04.18.18.46.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 18 Apr 2022 18:46:47 -0700 (PDT) Date: Tue, 19 Apr 2022 10:46:43 +0900 From: AKASHI Takahiro To: Ilias Apalodimas Cc: xypron.glpk@gmx.de, Stuart.Yoder@arm.com, paul.liu@linaro.org, u-boot@lists.denx.de Subject: Re: [PATCH 1/2] efi_loader: add sha384/512 on certificate revocation Message-ID: <20220419014643.GA47455@laputa> Mail-Followup-To: AKASHI Takahiro , Ilias Apalodimas , xypron.glpk@gmx.de, Stuart.Yoder@arm.com, paul.liu@linaro.org, u-boot@lists.denx.de References: <20220418180724.1855888-1-ilias.apalodimas@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220418180724.1855888-1-ilias.apalodimas@linaro.org> 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 Ilias, On Mon, Apr 18, 2022 at 09:07:22PM +0300, Ilias Apalodimas wrote: > Currently we don't support sha384/512 for the X.509 certificate > in dbx. Moreover if we come across such a hash we skip the check > and approve the image, although the image might needs to be rejected. > > Rework the code a bit and fix it by adding an array of structs with the > supported GUIDs, len and literal used in the U-Boot crypto APIs instead > of hardcoding the GUID types. > > 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 > --- > changes since v2: > - updated changelog (there was no v1) > changes since RFC: > - add an array of structs with the algo info info of a function > - checking hash_calculate result in efi_hash_regions() > include/efi_api.h | 6 +++ > include/efi_loader.h | 7 +++ > lib/efi_loader/efi_helper.c | 85 ++++++++++++++++++++++++++++++++++ > lib/efi_loader/efi_signature.c | 76 +++++++++++++++++++++--------- > 4 files changed, 151 insertions(+), 23 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/include/efi_loader.h b/include/efi_loader.h > index af36639ec6a7..ce221ee9317b 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -300,6 +300,8 @@ extern const efi_guid_t efi_guid_image_security_database; > extern const efi_guid_t efi_guid_sha256; > extern const efi_guid_t efi_guid_cert_x509; > extern const efi_guid_t efi_guid_cert_x509_sha256; > +extern const efi_guid_t efi_guid_cert_x509_sha384; > +extern const efi_guid_t efi_guid_cert_x509_sha512; > extern const efi_guid_t efi_guid_cert_type_pkcs7; > > /* GUID of RNG protocol */ > @@ -671,6 +673,11 @@ efi_status_t efi_file_size(struct efi_file_handle *fh, efi_uintn_t *size); > /* get a device path from a Boot#### option */ > struct efi_device_path *efi_get_dp_from_boot(const efi_guid_t guid); > > +/* get len, string (used in u-boot crypto from a guid */ > +const char *guid_to_sha_str(const efi_guid_t *guid); > +int guid_to_sha_len(const efi_guid_t *guid); > +int algo_to_len(const char *algo); > + > /** > * efi_size_in_pages() - convert size in bytes to size in pages > * > diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c > index 802d39ed97b6..c186ba4a3c01 100644 > --- a/lib/efi_loader/efi_helper.c > +++ b/lib/efi_loader/efi_helper.c > @@ -92,3 +92,88 @@ err: > free(var_value); > return NULL; > } > + > +const struct guid_to_hash_map { > + efi_guid_t guid; > + const char algo[32]; > + u32 bits; > +} guid_to_hash[] = { > + { > + EFI_CERT_X509_SHA256_GUID, > + "sha256", > + 256, => SHA256_SUM_LEN * 8 > + }, > + { > + EFI_CERT_SHA256_GUID, > + "sha256", > + 256, > + }, > + { > + EFI_CERT_X509_SHA384_GUID, > + "sha384", > + 384, > + }, > + { > + EFI_CERT_X509_SHA512_GUID, > + "sha512", > + 512, > + }, > +}; > + > +#define MAX_GUID_TO_HASH_COUNT ARRAY_SIZE(guid_to_hash) > + > +/** guid_to_sha_str - return the sha string e.g "sha256" for a given guid > + * used on EFI security databases > + * > + * @guid: guid to check > + * > + * Return: len or 0 if no match is found > + */ > +const char *guid_to_sha_str(const efi_guid_t *guid) > +{ > + size_t i; > + > + for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) { > + if (!guidcmp(guid, &guid_to_hash[i].guid)) > + return guid_to_hash[i].algo; > + } > + > + return NULL; > +} > + > +/** guid_to_sha_len - return the sha size in bytes for a given guid > + * used on EFI security databases > + * > + * @guid: guid to check > + * > + * Return: len or 0 if no match is found > + */ > +int guid_to_sha_len(const efi_guid_t *guid) > +{ > + size_t i; > + > + for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) { > + if (!guidcmp(guid, &guid_to_hash[i].guid)) > + return guid_to_hash[i].bits / 8; > + } > + > + return 0; > +} If we have algo_to_len(), we don't need guid_to_sha_len(). > + > +/** algo_to_len - return the sha size in bytes for a given string > + * > + * @guid: string to check > + * > + * Return: len or 0 if no match is found > + */ > +int algo_to_len(const char *algo) This function seems to be quite generic and useful for others. Why not implement it along with hash_calculate() in hash-checksum.c (without using guid_to_hash[]). > +{ > + size_t i; > + > + for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) { > + if (!strcmp(algo, guid_to_hash[i].algo)) > + return guid_to_hash[i].bits / 8; > + } > + > + return 0; > +} > diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c > index 79ed077ae7dd..cf01f21b4d04 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; > 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, const char *hash_algo, int *len) > { > + int ret; > + > + if (!hash_algo || !len) > + return false; > + > + *len = algo_to_len(hash_algo); Please modify it this way; int hash_len; hash_len = algo_to_len(hash_algo); if (len) *len = hash_len; Then use hash_len instead of *len hereafterr. This way, we don't have to define a unused local variable below code. > + if (!*len) > + return false; > + > if (!*hash) { > - *hash = calloc(1, SHA256_SUM_LEN); > + *hash = calloc(1, *len); > if (!*hash) { > EFI_PRINT("Out of memory\n"); > return false; > } > } > - if (size) > - *size = SHA256_SUM_LEN; > > - hash_calculate("sha256", regs, count, *hash); > + ret = hash_calculate(hash_algo, regs, count, *hash); > + if (ret) > + return false; > #ifdef DEBUG > EFI_PRINT("hash calculated:\n"); > print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, > - *hash, SHA256_SUM_LEN, false); > + *hash, *len, false); > #endif > > return true; > @@ -190,7 +201,6 @@ 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; > bool found = false; > bool hash_done = false; > > @@ -200,6 +210,8 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs, > goto out; > > for (siglist = db; siglist; siglist = siglist->next) { > + int len = 0; > + const char *hash_algo = NULL; > /* > * if the hash algorithm is unsupported and we get an entry in > * dbx reject the image > @@ -215,8 +227,14 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs, > if (guidcmp(&siglist->sig_type, &efi_guid_sha256)) > continue; > > + hash_algo = guid_to_sha_str(&efi_guid_sha256); > + /* > + * We could check size and hash_algo but efi_hash_regions() > + * will do that for us > + */ > if (!hash_done && > - !efi_hash_regions(regs->reg, regs->num, &hash, &size)) { > + !efi_hash_regions(regs->reg, regs->num, &hash, hash_algo, > + &len)) { > EFI_PRINT("Digesting an image failed\n"); > break; > } > @@ -229,8 +247,8 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs, > print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, > sig_data->data, sig_data->size, false); > #endif > - if (sig_data->size == size && > - !memcmp(sig_data->data, hash, size)) { > + if (sig_data->size == len && > + !memcmp(sig_data->data, hash, len)) { > found = true; > free(hash); > goto out; > @@ -263,8 +281,9 @@ 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; > + int len = 0; > bool found = false; > + const char *hash_algo = NULL; > > EFI_PRINT("%s: Enter, %p, %p\n", __func__, cert, db); > > @@ -278,7 +297,11 @@ 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)) > + > + /* We just need any sha256 algo to start the matching */ > + hash_algo = guid_to_sha_str(&efi_guid_sha256); Why not check if hash_algo is NULL? > + len = guid_to_sha_len(&efi_guid_sha256); => len = algo_to_len(hash_algo); Or even we don't need this line as efi_has_regions() will set it for us. > + if (!efi_hash_regions(reg, 1, &hash, hash_algo, &len)) > goto out; > > EFI_PRINT("%s: searching for %s\n", __func__, cert->subject); > @@ -290,6 +313,7 @@ static bool efi_lookup_certificate(struct x509_certificate *cert, > for (sig_data = siglist->sig_data_list; sig_data; > sig_data = sig_data->next) { > struct x509_certificate *cert_tmp; > + int len_tmp; We don't need len_tmp if the change I mentioned above is made. # we don't want to define a unused variable. > cert_tmp = x509_cert_parse(sig_data->data, > sig_data->size); > @@ -300,12 +324,13 @@ 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, hash_algo, > + &len_tmp)) => NULL > goto out; > > x509_free_certificate(cert_tmp); > > - if (!memcmp(hash, hash_tmp, size)) { > + if (!memcmp(hash, hash_tmp, len)) { > found = true; > goto out; > } > @@ -400,9 +425,10 @@ 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; > + int len = 0; > time64_t revoc_time; > bool revoked = false; > + const char *hash_algo = NULL; > > EFI_PRINT("%s: Enter, %p, %p, %p\n", __func__, sinfo, cert, dbx); > > @@ -411,13 +437,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)) > + hash_algo = guid_to_sha_str(&siglist->sig_type); > + if (!hash_algo) > 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, hash_algo, &len)) > goto out; > > for (sig_data = siglist->sig_data_list; sig_data; > @@ -429,18 +456,18 @@ static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo, > * }; > */ > #ifdef DEBUG > - if (sig_data->size >= size) { > + if (sig_data->size >= len) { > EFI_PRINT("hash in db:\n"); > print_hex_dump(" ", DUMP_PREFIX_OFFSET, > 16, 1, > - sig_data->data, size, false); > + sig_data->data, len, false); > } > #endif > - if ((sig_data->size < size + sizeof(time64_t)) || > - memcmp(sig_data->data, hash, size)) > + if ((sig_data->size < len + sizeof(time64_t)) || > + memcmp(sig_data->data, hash, len)) > continue; > > - memcpy(&revoc_time, sig_data->data + size, > + memcpy(&revoc_time, sig_data->data + len, > sizeof(revoc_time)); > EFI_PRINT("revocation time: 0x%llx\n", revoc_time); > /* > @@ -488,6 +515,7 @@ bool efi_signature_verify(struct efi_image_regions *regs, > goto out; > > for (sinfo = msg->signed_infos; sinfo; sinfo = sinfo->next) { > + int len; ditto > EFI_PRINT("Signed Info: digest algo: %s, pkey algo: %s\n", > sinfo->sig->hash_algo, sinfo->sig->pkey_algo); > > @@ -500,7 +528,9 @@ 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, > + guid_to_sha_str(&efi_guid_sha256), > + &len)) { => NULL -Takahiro Akashi > EFI_PRINT("Digesting an image failed\n"); > goto out; > } > -- > 2.32.0 >