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 707CBC433F5 for ; Wed, 19 Jan 2022 14:54:20 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 8FB0880FE7; Wed, 19 Jan 2022 15:54:18 +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="raS156+D"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 9ADBE83388; Wed, 19 Jan 2022 15:54:17 +0100 (CET) Received: from mail-wm1-x336.google.com (mail-wm1-x336.google.com [IPv6:2a00:1450:4864:20::336]) (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 E487580384 for ; Wed, 19 Jan 2022 15:54:13 +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-wm1-x336.google.com with SMTP id w26so5550860wmi.0 for ; Wed, 19 Jan 2022 06:54:13 -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:references:mime-version :content-disposition:in-reply-to; bh=mDRKWsaECSe4f2FS0+9wm+A4DaTXLP9wZByQm65xxqw=; b=raS156+Dm06PQMJt45Xe0ukHsZUFZJ4genZVs42e5/KdlYm6QaAkW275cCjI3eeReJ Hoqth7XmzoAdjApu5FqVjlHTAebhAmsm7KDMMwrF8yDYfAgefAhpOrq9ScUNmI7ixBKH AMABtwl2j6icvL8PgDQ/tym2yv8+Z4G4/NkwELzWM9iuJcZpqchce996Y05l2CErsaXl x/sMnQ5pxl1e+aNEEwNPyjTzjhhWGNFoWFuOD7AHDgcAg2C37pyEWc0gv3zrwLFgpdCb esuJzkKrShGXdQ0mjqIKBV5LF2kPZri/6WR9OA7tsAYuek4e8a32noqvE6sogxPqc1rB H7gA== 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:references :mime-version:content-disposition:in-reply-to; bh=mDRKWsaECSe4f2FS0+9wm+A4DaTXLP9wZByQm65xxqw=; b=zbj2RQ1e0WfVd4eT+zrAA9HGbzd97qhcmLy9YL/MeW+9nz61Lhk81yCs3m9SrVo64H ZT2rajQCzBICvxQOeiMMkAYxh1YCn1yjwl9GSTaCGq96PxZ/kibaiv0LD+OMdzxoBTqg 2VKawRFkCkuqRrRV3rzAW6pgJk2w3MmHT3mJ0eXjCaK9d2nBqRQXKzX1oG4fyItXjrP/ j/NF07QHaueBU9YNlnOPee5iPSwetbpw2tnQSBFl6bznKW8hWElaXrx94Ve0jf/QRwjp O39jFy4Pzpv3T1qcbsRRHs5rAlMftTnngzdXu3rBxxPzJNtb/n5ADXAhisYQxATqxfxe J/rg== X-Gm-Message-State: AOAM533RZALoatFncs8nw4eJ6biIrIrGivgX61FmHcDDLPnxP42+6o4Y 7ppiTaV1RbL4sTH1oS7CqvtKIQ== X-Google-Smtp-Source: ABdhPJykhRWSwTXlM4mtC7I2rQVg03yjTIDR/zvigJmyp1N/gCVG4bmtKC6JgUAKwoX9SeUMyuW8lA== X-Received: by 2002:a05:600c:b50:: with SMTP id k16mr3934799wmr.85.1642604053455; Wed, 19 Jan 2022 06:54:13 -0800 (PST) Received: from hades (athedsl-4461669.home.otenet.gr. [94.71.4.85]) by smtp.gmail.com with ESMTPSA id a15sm93924wrp.41.2022.01.19.06.54.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 19 Jan 2022 06:54:12 -0800 (PST) Date: Wed, 19 Jan 2022 16:54:10 +0200 From: Ilias Apalodimas To: Heinrich Schuchardt Cc: AKASHI Takahiro , u-boot@lists.denx.de Subject: Re: [PATCH] lib/crypto: Enable more algorithms in cert verification Message-ID: References: <20220118111238.321742-1-ilias.apalodimas@linaro.org> <20220118123822.GC30001@laputa> <1ddb38a8-b998-4917-a645-bf7356b32e9d@gmx.de> <2d0694f3-5c21-62be-b9da-701126f9ad96@gmx.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2d0694f3-5c21-62be-b9da-701126f9ad96@gmx.de> 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 Hi Heinrich, On Wed, Jan 19, 2022 at 03:22:53PM +0100, Heinrich Schuchardt wrote: > On 1/18/22 19:12, 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? > > You must assume that PK, KEK, db are preexisting or are seeded from a > file. So the check should be done when loading an image. I've already replied on this and sent a v2. Can you have a look at that ? It is happening during loading. The call chain here is efi_load_pe() -> efi_image_authenticate() -> efi_signature_verify() > > To be more precise: > > An image should not be validated based on an SHA1 signature or SHA1 hash > but it must be possible to reject an image based on an SHA1 hash. That's two different things and imho should go into completely different patchsets. The v2 I've sent only deals with certs. If you want to kick out sha1 in general there's efi_signature_lookup_digest() we should go and change. But the purpose of this patchset is fix rsa4096 encrypted signatures, not remove the sha1 overall. So can we review the v2 and I'll send a different patchset for sha1 hashes. Thanks /Ilias > > Best regards > > Heinrich > > > > > 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 > > > > > > > > > > > > > > > > >