| Message ID | 20220118111238.321742-1-ilias.apalodimas@linaro.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | lib/crypto: Enable more algorithms in cert verification | expand |
Hi Ilias, On Tue, Jan 18, 2022 at 01:12:37PM +0200, Ilias Apalodimas wrote: > Right now the code explicitly limits us to sha1,256 hashes with RSA2048 > encryption. But the limitation is artificial since U-Boot supports > a wider range of algorithms. > > The internal image_get_[checksum|crypto]_algo() functions expect an > argument in the format of <checksum>,<crypto>. So let's remove the size > checking and create the needed string on the fly in order to support > more hash/signing combinations. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > lib/crypto/public_key.c | 27 +++++++++++++-------------- > 1 file changed, 13 insertions(+), 14 deletions(-) > > diff --git a/lib/crypto/public_key.c b/lib/crypto/public_key.c > index df6033cdb499..b783c63f5a51 100644 > --- a/lib/crypto/public_key.c > +++ b/lib/crypto/public_key.c > @@ -97,6 +97,7 @@ int public_key_verify_signature(const struct public_key *pkey, > const struct public_key_signature *sig) > { > struct image_sign_info info; > + char algo[256]; > int ret; > > pr_devel("==>%s()\n", __func__); > @@ -108,29 +109,27 @@ int public_key_verify_signature(const struct public_key *pkey, > return -EINVAL; > > memset(&info, '\0', sizeof(info)); > + memset(algo, 0, sizeof(algo)); > info.padding = image_get_padding_algo("pkcs-1.5"); > /* > * Note: image_get_[checksum|crypto]_algo takes a string > * argument like "<checksum>,<crypto>" > * TODO: support other hash algorithms > */ If this patch is applied, the TODO comment above will make no sense :) > - if (strcmp(sig->pkey_algo, "rsa") || (sig->s_size * 8) != 2048) { > - pr_warn("Encryption is not RSA2048: %s%d\n", > - sig->pkey_algo, sig->s_size * 8); > - return -ENOPKG; > - } > - if (!strcmp(sig->hash_algo, "sha1")) { > - info.checksum = image_get_checksum_algo("sha1,rsa2048"); > - info.name = "sha1,rsa2048"; > - } else if (!strcmp(sig->hash_algo, "sha256")) { > - 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); 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?) -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 >
Akashi-san, On Tue, Jan 18, 2022 at 09:38:22PM +0900, AKASHI Takahiro wrote: > Hi Ilias, > > On Tue, Jan 18, 2022 at 01:12:37PM +0200, Ilias Apalodimas wrote: > > Right now the code explicitly limits us to sha1,256 hashes with RSA2048 > > encryption. But the limitation is artificial since U-Boot supports > > a wider range of algorithms. > > > > The internal image_get_[checksum|crypto]_algo() functions expect an > > argument in the format of <checksum>,<crypto>. So let's remove the size > > checking and create the needed string on the fly in order to support > > more hash/signing combinations. > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > --- > > lib/crypto/public_key.c | 27 +++++++++++++-------------- > > 1 file changed, 13 insertions(+), 14 deletions(-) > > > > diff --git a/lib/crypto/public_key.c b/lib/crypto/public_key.c > > index df6033cdb499..b783c63f5a51 100644 > > --- a/lib/crypto/public_key.c > > +++ b/lib/crypto/public_key.c > > @@ -97,6 +97,7 @@ int public_key_verify_signature(const struct public_key *pkey, > > const struct public_key_signature *sig) > > { > > struct image_sign_info info; > > + char algo[256]; > > int ret; > > > > pr_devel("==>%s()\n", __func__); > > @@ -108,29 +109,27 @@ int public_key_verify_signature(const struct public_key *pkey, > > return -EINVAL; > > > > memset(&info, '\0', sizeof(info)); > > + memset(algo, 0, sizeof(algo)); > > info.padding = image_get_padding_algo("pkcs-1.5"); > > /* > > * Note: image_get_[checksum|crypto]_algo takes a string > > * argument like "<checksum>,<crypto>" > > * TODO: support other hash algorithms > > */ > > If this patch is applied, the TODO comment above will make no sense :) We are still only handle SHA, but there's a printable error now, so i'll get rid of the comment. > > > - if (strcmp(sig->pkey_algo, "rsa") || (sig->s_size * 8) != 2048) { > > - pr_warn("Encryption is not RSA2048: %s%d\n", > > - sig->pkey_algo, sig->s_size * 8); > > - return -ENOPKG; > > - } > > - if (!strcmp(sig->hash_algo, "sha1")) { > > - info.checksum = image_get_checksum_algo("sha1,rsa2048"); > > - info.name = "sha1,rsa2048"; > > - } else if (!strcmp(sig->hash_algo, "sha256")) { > > - 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); > > 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 > >
On 1/18/22 13:50, Ilias Apalodimas wrote: > Akashi-san, > > On Tue, Jan 18, 2022 at 09:38:22PM +0900, AKASHI Takahiro wrote: >> Hi Ilias, >> >> On Tue, Jan 18, 2022 at 01:12:37PM +0200, Ilias Apalodimas wrote: >>> Right now the code explicitly limits us to sha1,256 hashes with RSA2048 >>> encryption. But the limitation is artificial since U-Boot supports >>> a wider range of algorithms. >>> >>> The internal image_get_[checksum|crypto]_algo() functions expect an >>> argument in the format of <checksum>,<crypto>. So let's remove the size >>> checking and create the needed string on the fly in order to support >>> more hash/signing combinations. >>> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> >>> --- >>> lib/crypto/public_key.c | 27 +++++++++++++-------------- >>> 1 file changed, 13 insertions(+), 14 deletions(-) >>> >>> diff --git a/lib/crypto/public_key.c b/lib/crypto/public_key.c >>> index df6033cdb499..b783c63f5a51 100644 >>> --- a/lib/crypto/public_key.c >>> +++ b/lib/crypto/public_key.c >>> @@ -97,6 +97,7 @@ int public_key_verify_signature(const struct public_key *pkey, >>> const struct public_key_signature *sig) >>> { >>> struct image_sign_info info; >>> + char algo[256]; >>> int ret; >>> >>> pr_devel("==>%s()\n", __func__); >>> @@ -108,29 +109,27 @@ int public_key_verify_signature(const struct public_key *pkey, >>> return -EINVAL; >>> >>> memset(&info, '\0', sizeof(info)); >>> + memset(algo, 0, sizeof(algo)); >>> info.padding = image_get_padding_algo("pkcs-1.5"); >>> /* >>> * Note: image_get_[checksum|crypto]_algo takes a string >>> * argument like "<checksum>,<crypto>" >>> * TODO: support other hash algorithms >>> */ >> >> If this patch is applied, the TODO comment above will make no sense :) > > We are still only handle SHA, but there's a printable error now, so i'll > get rid of the comment. > >> >>> - if (strcmp(sig->pkey_algo, "rsa") || (sig->s_size * 8) != 2048) { >>> - pr_warn("Encryption is not RSA2048: %s%d\n", >>> - sig->pkey_algo, sig->s_size * 8); >>> - return -ENOPKG; >>> - } >>> - if (!strcmp(sig->hash_algo, "sha1")) { >>> - info.checksum = image_get_checksum_algo("sha1,rsa2048"); >>> - info.name = "sha1,rsa2048"; >>> - } else if (!strcmp(sig->hash_algo, "sha256")) { >>> - 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? 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 >>>
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 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 > > > > >
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. 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 >>>>> >>
Hi Heinrich, On Tue, 18 Jan 2022 at 18:22, Heinrich Schuchardt <xypron.glpk@gmx.de> 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? 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 > >>>>> > >> >
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 <xypron.glpk@gmx.de> 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. -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 > > >>>>> > > >> > >
Hi Akashi-san, On Wed, 19 Jan 2022 at 06:47, AKASHI Takahiro <takahiro.akashi@linaro.org> 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 <xypron.glpk@gmx.de> 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'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 > > > >>>>> > > > >> > > >
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 > <takahiro.akashi@linaro.org> 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 <xypron.glpk@gmx.de> 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 > > > > >>>>> > > > > >> > > > >
\> > > > 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. You don't trust the PCRs in the eventlog. The eventlog is a human readable form of the events which extended the PCRs to that state. In order to verify the eventlog you need to replay it and compare it against the hardware PCRs. The number and nature of hashing algorithms is configurable but PCR bank configuration is not (yet) supported in U-Boot. However the TPMv2 (the only ones we support on TCG2) never use sha1 exclusively. The usual config for hardware is sha1,256 (or for swtpm sha1,256,384,512). I don't remember if switching a TPMv2 into 'sha1 only' is permitted by the spec, but even if it is, that's a very very bad idea since it defeats the purpose of a v2 hardware in the first place. In any case we can apply similar restrictions on the TCG code when we add the PCR bank config options. Hope that clears it up /Ilias > -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 > > > > > >>>>> > > > > > >> > > > > >
On 1/18/22 19:12, Ilias Apalodimas wrote: > Hi Heinrich, > > On Tue, 18 Jan 2022 at 18:22, Heinrich Schuchardt <xypron.glpk@gmx.de> 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. 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. 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 >>>>>>> >>>> >>
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 <xypron.glpk@gmx.de> 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 > > > > > > > > > > > > > > > > >
diff --git a/lib/crypto/public_key.c b/lib/crypto/public_key.c index df6033cdb499..b783c63f5a51 100644 --- a/lib/crypto/public_key.c +++ b/lib/crypto/public_key.c @@ -97,6 +97,7 @@ int public_key_verify_signature(const struct public_key *pkey, const struct public_key_signature *sig) { struct image_sign_info info; + char algo[256]; int ret; pr_devel("==>%s()\n", __func__); @@ -108,29 +109,27 @@ int public_key_verify_signature(const struct public_key *pkey, return -EINVAL; memset(&info, '\0', sizeof(info)); + memset(algo, 0, sizeof(algo)); info.padding = image_get_padding_algo("pkcs-1.5"); /* * Note: image_get_[checksum|crypto]_algo takes a string * argument like "<checksum>,<crypto>" * TODO: support other hash algorithms */ - if (strcmp(sig->pkey_algo, "rsa") || (sig->s_size * 8) != 2048) { - pr_warn("Encryption is not RSA2048: %s%d\n", - sig->pkey_algo, sig->s_size * 8); - return -ENOPKG; - } - if (!strcmp(sig->hash_algo, "sha1")) { - info.checksum = image_get_checksum_algo("sha1,rsa2048"); - info.name = "sha1,rsa2048"; - } else if (!strcmp(sig->hash_algo, "sha256")) { - 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); + + 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;
Right now the code explicitly limits us to sha1,256 hashes with RSA2048 encryption. But the limitation is artificial since U-Boot supports a wider range of algorithms. The internal image_get_[checksum|crypto]_algo() functions expect an argument in the format of <checksum>,<crypto>. So let's remove the size checking and create the needed string on the fly in order to support more hash/signing combinations. Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- lib/crypto/public_key.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-)