[PATCH] lib/crypto: Enable more algorithms in cert verification

[PATCH] lib/crypto: Enable more algorithms in cert verification

[Ilias Apalodimas]

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 (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;
-- 
2.30.2

Re: [PATCH] lib/crypto: Enable more algorithms in cert verification

[AKASHI Takahiro]

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
> 

Re: [PATCH] lib/crypto: Enable more algorithms in cert verification

[Ilias Apalodimas]

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
> > 

Re: [PATCH] lib/crypto: Enable more algorithms in cert verification

[Heinrich Schuchardt]

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
>>>

Re: [PATCH] lib/crypto: Enable more algorithms in cert verification

[Ilias Apalodimas]

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
> > > > 
> 

Re: [PATCH] lib/crypto: Enable more algorithms in cert verification

[Heinrich Schuchardt]

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
>>>>>
>>

Re: [PATCH] lib/crypto: Enable more algorithms in cert verification

[Ilias Apalodimas]

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
> >>>>>
> >>
>

Re: [PATCH] lib/crypto: Enable more algorithms in cert verification

[AKASHI Takahiro]

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
> > >>>>>
> > >>
> >

Re: [PATCH] lib/crypto: Enable more algorithms in cert verification

[Ilias Apalodimas]

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
> > > >>>>>
> > > >>
> > >

Re: [PATCH] lib/crypto: Enable more algorithms in cert verification

[AKASHI Takahiro]

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
> > > > >>>>>
> > > > >>
> > > >

Re: [PATCH] lib/crypto: Enable more algorithms in cert verification

[Ilias Apalodimas]

\> >
> > 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
> > > > > >>>>>
> > > > > >>
> > > > >

Re: [PATCH] lib/crypto: Enable more algorithms in cert verification

[Heinrich Schuchardt]

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
>>>>>>>
>>>>
>>

Re: [PATCH] lib/crypto: Enable more algorithms in cert verification

[Ilias Apalodimas]

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
> > > > > > > > 
> > > > > 
> > > 
>