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 0C1E1C4332F for ; Wed, 19 Jan 2022 12:36:23 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 4F3B8832E3; Wed, 19 Jan 2022 13:36:21 +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="bpgcHyFY"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 9C1C183388; Wed, 19 Jan 2022 13:36:19 +0100 (CET) Received: from mail-pl1-x634.google.com (mail-pl1-x634.google.com [IPv6:2607:f8b0:4864:20::634]) (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 3A28C831F0 for ; Wed, 19 Jan 2022 13:36:16 +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=takahiro.akashi@linaro.org Received: by mail-pl1-x634.google.com with SMTP id t18so2020736plg.9 for ; Wed, 19 Jan 2022 04:36:16 -0800 (PST) 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=Z+fvlAE1wESeITYz1UskWOMgBmjGTu24Y8S2Y3apSfQ=; b=bpgcHyFYcgkHvdtSQWWgUcZdKzTVnUlW5UelC+bfsdUVicDi2IWEnBZK4f3ObAp0GD g/oxLQiB4a1C4baITPwPWv1LuK6lJ3j0f3QSP87UEBt0zRRVG3FSKJTie8k+HVW1sKca rKd6aToE+/qLw9kTNT0w4qsc/CisUFZvjzZbOtjMMPQ1b8xTz+6WY7DeiUFaN85p6izN GhR8RNo8aMrjNqDOr1vHaw2MOOXGGGsglyhTWH0m14jlbUP/H8YM5w/FOADDuYjbQryE QIrK5MA/XhGz/XfiED2iTRh99a9XhRBVOTHHW7HvOv3UyNWlsvM+V0hBNFsSaqoYfbwg wBtg== 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=Z+fvlAE1wESeITYz1UskWOMgBmjGTu24Y8S2Y3apSfQ=; b=WQ1BMyRiLwhXFYZR62rG1x6Te5SyLcffKzU26odQVdyTIqUAW5DECmPg16AjNJapvG wi+/eZn5XIKYTEuCQfsGAYolfzf/mSBXHlh3JPyXvrA/1GajJWBwZi5S9ejIHZWwgj8i A01W7GMM7v5M0ESXmpuYT49d2xNAIXg/ADxNVAjHOgc6uYZmFBdlD2eNZdDu6st0af4c NBCpb9BgIYFloF4kk/839o1L9NFGGkv14Ofi8oBgR2obTpyasAg6Jvs54yfGyYQSfm8J 7tz5f1GKfdy1Pm5mA9Yck27sJVdV6QRAFiw6KGMhUUWyyq4vrZNBjjkr6REbaz4ZCb15 +NmQ== X-Gm-Message-State: AOAM5319U5AnOY0l6HaWQAydOH4nxmYmwcQMiizgkDjNrHKGGzCRWprx YZqbIx+iHfcoY1tXqi80V5zcXQ== X-Google-Smtp-Source: ABdhPJw41LAJ9PLhRnfCXpu2gFoZ2oyaEUkL1Baha8sc9xDtgnGyLr4quZkIgDdfk05P0gxOmPmhoQ== X-Received: by 2002:a17:90b:1949:: with SMTP id nk9mr4033048pjb.219.1642595774412; Wed, 19 Jan 2022 04:36:14 -0800 (PST) Received: from laputa ([2400:4050:c3e1:100:9c4f:3230:d555:4921]) by smtp.gmail.com with ESMTPSA id nn7sm5572971pjb.10.2022.01.19.04.36.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 19 Jan 2022 04:36:13 -0800 (PST) Date: Wed, 19 Jan 2022 21:36:10 +0900 From: AKASHI Takahiro To: Ilias Apalodimas Cc: Heinrich Schuchardt , u-boot@lists.denx.de Subject: Re: [PATCH] lib/crypto: Enable more algorithms in cert verification Message-ID: <20220119123610.GC61490@laputa> Mail-Followup-To: AKASHI Takahiro , Ilias Apalodimas , Heinrich Schuchardt , u-boot@lists.denx.de References: <20220118111238.321742-1-ilias.apalodimas@linaro.org> <20220118123822.GC30001@laputa> <1ddb38a8-b998-4917-a645-bf7356b32e9d@gmx.de> <20220119044710.GB61490@laputa> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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.2 at phobos.denx.de X-Virus-Status: Clean On Wed, Jan 19, 2022 at 09:07:04AM +0200, Ilias Apalodimas wrote: > Hi Akashi-san, > > > On Wed, 19 Jan 2022 at 06:47, AKASHI Takahiro > wrote: > > > > On Tue, Jan 18, 2022 at 08:12:22PM +0200, Ilias Apalodimas wrote: > > > Hi Heinrich, > > > > > > On Tue, 18 Jan 2022 at 18:22, Heinrich Schuchardt wrote: > > > > > > > > On 1/18/22 15:03, Ilias Apalodimas wrote: > > > > > Hi Heinrich, > > > > > > > > > >>>>> - info.checksum = image_get_checksum_algo("sha256,rsa2048"); > > > > > > > > > > [...] > > > > > > > > > >>>>> - info.name = "sha256,rsa2048"; > > > > >>>>> - } else { > > > > >>>>> - pr_warn("unknown msg digest algo: %s\n", sig->hash_algo); > > > > >>>>> + if (strcmp(sig->pkey_algo, "rsa")) { > > > > >>>>> + pr_err("Encryption is not RSA: %s\n", sig->pkey_algo); > > > > >>>>> return -ENOPKG; > > > > >>>>> } > > > > >>>>> + ret = snprintf(algo, sizeof(algo), "%s,%s%d", sig->hash_algo, > > > > >>>>> + sig->pkey_algo, sig->s_size * 8); > > > > >> > > > > >> How do we ensure that the unsafe SHA1 algorithm is not used? > > > > > > > > > > We don't, but the current code allows it as well. Should we enforce this > > > > > from U-Boot though? The spec doesn't forbid it as far as I remember > > > > > > > > Collisions for SHA1 have been first created successfully in 2017. > > > > > > > > It is feasible to create two different EFI binaries with the same SHA1. > > > > One will be reviewed and signed. After copying the signature to the > > > > other one it will happily boot on U-Boot. Ouch. This is exactly what > > > > signatures are meant to avoid. > > > > > > > > We must not accept SHA1 for signatures. > > > > > > Right, but is this the right place to do it? This is function to > > > verify signatures. Isn't it better to keep this as is and then > > > explicitly deny adding sha1 hashed keys into db? > > > > If you don't want to trust SHA1, just disable it with !CONFIG_SHA1. > > No that's not doable. Things like EFI_TCG2 protocol needs that since > we use a sha1 in the tcg eventlog. I simply wonder why you can trust SHA1 in PCR/event log while you don't trust it in secure boot. -Takahiro Akashi > I've looked at the code a bit more > and not adding in db looks either bad or hard to reason about, since > we do have different storage backends(i.e efi variables in RPMB via > standaloneMM). So one way to do this without affecting the generic > crypto code is > > bool efi_signature_verify(struct efi_image_regions *regs, > if (ret < 0 || !signer) > goto out; > > + if (!strcmp(signer->sig->hash_algo, "sha1")) { > + pr_err("SHA1 support is disabled for EFI\n"); > + goto out; > + } > + > if (sinfo->blacklisted) > goto out; > > Cheers > /Ilias > > > -Takahiro Akashi > > > > > Cheers > > > /Ilias > > > > > > > > Best regards > > > > > > > > Heinrich > > > > > > > > > > > > > > Regards > > > > > /Ilias > > > > >> > > > > >> Best regards > > > > >> > > > > >> Heinrich > > > > >> > > > > >>>> > > > > >>>> I'm not sure that this naming rule, in particular the latter part, will > > > > >>>> always hold in the future while all the existing algo's observe it. > > > > >>>> (Maybe we need some note somewhere?) > > > > >>> > > > > >>> The if a few lines below will shield us and return -EINVAL. How about > > > > >>> adding an error message there? > > > > >>> > > > > >>> Cheers > > > > >>> /Ilias > > > > >>>> > > > > >>>> -Takahiro Akashi > > > > >>>> > > > > >>>>> + > > > > >>>>> + if (ret >= sizeof(algo)) > > > > >>>>> + return -EINVAL; > > > > >>>>> + > > > > >>>>> + info.checksum = image_get_checksum_algo((const char *)algo); > > > > >>>>> + info.name = (const char *)algo; > > > > >>>>> info.crypto = image_get_crypto_algo(info.name); > > > > >>>>> - if (IS_ERR(info.checksum) || IS_ERR(info.crypto)) > > > > >>>>> + if (!info.checksum || !info.crypto) > > > > >>>>> return -ENOPKG; > > > > >>>>> > > > > >>>>> info.key = pkey->key; > > > > >>>>> -- > > > > >>>>> 2.30.2 > > > > >>>>> > > > > >> > > > >