[PATCH] crypto: ecdsa: Fix the public key format description

[PATCH] crypto: ecdsa: Fix the public key format description

[Jarkko Sakkinen]

Public key blob is not just x and y concatenated. It follows RFC5480
section 2.2. Address this by re-documenting the function with the
correct description of the format.

Link: https://datatracker.ietf.org/doc/html/rfc5480
Fixes: 4e6602916bc6 ("crypto: ecdsa - Add support for ECDSA signature verification")
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
It is a bug fix that does not really need a stable backport. Still
categorizes as a bug because by following the existing documentation
you end up with an error code.
 crypto/ecdsa.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c
index 258fffbf623d..55114146ff84 100644
--- a/crypto/ecdsa.c
+++ b/crypto/ecdsa.c
@@ -215,9 +215,8 @@ static int ecdsa_ecc_ctx_reset(struct ecc_ctx *ctx)
 }
 
 /*
- * Set the public key given the raw uncompressed key data from an X509
- * certificate. The key data contain the concatenated X and Y coordinates of
- * the public key.
+ * Set the public ECC key as defined by RFC5480 section 2.2 "Subject Public
+ * Key". Only the uncompressed format is supported.
  */
 static int ecdsa_set_pub_key(struct crypto_akcipher *tfm, const void *key, unsigned int keylen)
 {
-- 
2.45.1

Re: [PATCH] crypto: ecdsa: Fix the public key format description

[Jarkko Sakkinen]

On Mon May 27, 2024 at 11:28 PM EEST, Jarkko Sakkinen wrote:
> Public key blob is not just x and y concatenated. It follows RFC5480
> section 2.2. Address this by re-documenting the function with the
> correct description of the format.
>
> Link: https://datatracker.ietf.org/doc/html/rfc5480
> Fixes: 4e6602916bc6 ("crypto: ecdsa - Add support for ECDSA signature verification")
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> It is a bug fix that does not really need a stable backport. Still
> categorizes as a bug because by following the existing documentation
> you end up with an error code.
>  crypto/ecdsa.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c
> index 258fffbf623d..55114146ff84 100644
> --- a/crypto/ecdsa.c
> +++ b/crypto/ecdsa.c
> @@ -215,9 +215,8 @@ static int ecdsa_ecc_ctx_reset(struct ecc_ctx *ctx)
>  }
>  
>  /*
> - * Set the public key given the raw uncompressed key data from an X509
> - * certificate. The key data contain the concatenated X and Y coordinates of
> - * the public key.
> + * Set the public ECC key as defined by RFC5480 section 2.2 "Subject Public
> + * Key". Only the uncompressed format is supported.
>   */
>  static int ecdsa_set_pub_key(struct crypto_akcipher *tfm, const void *key, unsigned int keylen)
>  {

Based on this, is this now along the lines of correct format":

       *ptr++ = 0x04; /* uncompressed */
       ptr = asn1_encode_octet_string(&ptr[0], &in[sizeof(in)], &x[0], x_size);
       ptr = asn1_encode_octet_string(&ptr[0], &in[sizeof(in)], &x[x_size + 2], x_size);
       in_len = ptr - in;
       ret = crypto_akcipher_set_pub_key(tfm, in, in_len);

Notes:

- ptr points to the beginning of in (initially)
- x: "u16, u8[x_size], u16, u8[x_size]"
- x_size is length of ecc i.e. 32 in the case of p256.

I actually did the snippet by reading the RFC so this is pretty good
test for the patch.

BR, Jarkko

Re: [PATCH] crypto: ecdsa: Fix the public key format description

[Jarkko Sakkinen]

On Tue May 28, 2024 at 12:05 AM EEST, Jarkko Sakkinen wrote:
> On Mon May 27, 2024 at 11:28 PM EEST, Jarkko Sakkinen wrote:
> > Public key blob is not just x and y concatenated. It follows RFC5480
> > section 2.2. Address this by re-documenting the function with the
> > correct description of the format.
> >
> > Link: https://datatracker.ietf.org/doc/html/rfc5480
> > Fixes: 4e6602916bc6 ("crypto: ecdsa - Add support for ECDSA signature verification")
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > It is a bug fix that does not really need a stable backport. Still
> > categorizes as a bug because by following the existing documentation
> > you end up with an error code.
> >  crypto/ecdsa.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c
> > index 258fffbf623d..55114146ff84 100644
> > --- a/crypto/ecdsa.c
> > +++ b/crypto/ecdsa.c
> > @@ -215,9 +215,8 @@ static int ecdsa_ecc_ctx_reset(struct ecc_ctx *ctx)
> >  }
> >  
> >  /*
> > - * Set the public key given the raw uncompressed key data from an X509
> > - * certificate. The key data contain the concatenated X and Y coordinates of
> > - * the public key.
> > + * Set the public ECC key as defined by RFC5480 section 2.2 "Subject Public
> > + * Key". Only the uncompressed format is supported.
> >   */
> >  static int ecdsa_set_pub_key(struct crypto_akcipher *tfm, const void *key, unsigned int keylen)
> >  {
>
> Based on this, is this now along the lines of correct format":
>
>        *ptr++ = 0x04; /* uncompressed */
>        ptr = asn1_encode_octet_string(&ptr[0], &in[sizeof(in)], &x[0], x_size);
>        ptr = asn1_encode_octet_string(&ptr[0], &in[sizeof(in)], &x[x_size + 2], x_size);
>        in_len = ptr - in;
>        ret = crypto_akcipher_set_pub_key(tfm, in, in_len);


I fixed up the above as it should be only single octect string to this:

	ptr = &in[0];
        *ptr++ = 0x04; /* uncompressed */
        ptr = asn1_encode_octet_string(&ptr[0], &in[sizeof(in)],
                                       &data[0], 2 * x_size);
        in_len = ptr - in;
        pr_info("in_len=%u\n", in_len);
        ret = crypto_akcipher_set_pub_key(tfm, in, in_len);
        crypto_free_akcipher(tfm);

It fails in:

	ndigits = DIV_ROUND_UP(digitlen, sizeof(u64));
	if (ndigits != ctx->curve->g.ndigits)
		return -EINVAL;

I checked that in_len=67.

The tfm is deleted at instant because the above code is part of *_query.
TPM2 ECDSA asymmetric key that way that signature verification will work
when it is needed. The key type signs with TPM and verifies with the
software cipher.

BR, Jarkko

Re: [PATCH] crypto: ecdsa: Fix the public key format description

[Jarkko Sakkinen]

On Tue May 28, 2024 at 12:58 AM EEST, Jarkko Sakkinen wrote:
> On Tue May 28, 2024 at 12:05 AM EEST, Jarkko Sakkinen wrote:
> > On Mon May 27, 2024 at 11:28 PM EEST, Jarkko Sakkinen wrote:
> > > Public key blob is not just x and y concatenated. It follows RFC5480
> > > section 2.2. Address this by re-documenting the function with the
> > > correct description of the format.
> > >
> > > Link: https://datatracker.ietf.org/doc/html/rfc5480
> > > Fixes: 4e6602916bc6 ("crypto: ecdsa - Add support for ECDSA signature verification")
> > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > ---
> > > It is a bug fix that does not really need a stable backport. Still
> > > categorizes as a bug because by following the existing documentation
> > > you end up with an error code.
> > >  crypto/ecdsa.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c
> > > index 258fffbf623d..55114146ff84 100644
> > > --- a/crypto/ecdsa.c
> > > +++ b/crypto/ecdsa.c
> > > @@ -215,9 +215,8 @@ static int ecdsa_ecc_ctx_reset(struct ecc_ctx *ctx)
> > >  }
> > >  
> > >  /*
> > > - * Set the public key given the raw uncompressed key data from an X509
> > > - * certificate. The key data contain the concatenated X and Y coordinates of
> > > - * the public key.
> > > + * Set the public ECC key as defined by RFC5480 section 2.2 "Subject Public
> > > + * Key". Only the uncompressed format is supported.
> > >   */
> > >  static int ecdsa_set_pub_key(struct crypto_akcipher *tfm, const void *key, unsigned int keylen)
> > >  {
> >
> > Based on this, is this now along the lines of correct format":
> >
> >        *ptr++ = 0x04; /* uncompressed */
> >        ptr = asn1_encode_octet_string(&ptr[0], &in[sizeof(in)], &x[0], x_size);
> >        ptr = asn1_encode_octet_string(&ptr[0], &in[sizeof(in)], &x[x_size + 2], x_size);
> >        in_len = ptr - in;
> >        ret = crypto_akcipher_set_pub_key(tfm, in, in_len);
>
>
> I fixed up the above as it should be only single octect string to this:
>
> 	ptr = &in[0];
>         *ptr++ = 0x04; /* uncompressed */
>         ptr = asn1_encode_octet_string(&ptr[0], &in[sizeof(in)],
>                                        &data[0], 2 * x_size);
>         in_len = ptr - in;
>         pr_info("in_len=%u\n", in_len);
>         ret = crypto_akcipher_set_pub_key(tfm, in, in_len);
>         crypto_free_akcipher(tfm);
>
> It fails in:
>
> 	ndigits = DIV_ROUND_UP(digitlen, sizeof(u64));
> 	if (ndigits != ctx->curve->g.ndigits)
> 		return -EINVAL;
>
> I checked that in_len=67.
>
> The tfm is deleted at instant because the above code is part of *_query.
> TPM2 ECDSA asymmetric key that way that signature verification will work
> when it is needed. The key type signs with TPM and verifies with the
> software cipher.

3rd trial, i.e. no octect encoding at all but with the undocumented
(in the current mainline) prefix byte:

        ptr = &data[0];
        *ptr++ = 0x04; /* uncompressed */
        memcpy(&ptr[0], &x[0], x_size);
        memcpy(&ptr[x_size], &x[x_size + 2], x_size);
        ret = crypto_akcipher_set_pub_key(tfm, data, 3 * x_size + 1);
        crypto_free_akcipher(tfm);

I added also kprobe:

echo 'r:ecdsa ecdsa_set_pub_key $retval' >> /sys/kernel/tracing/kprobe_events
echo 1 > /sys/kernel/tracing/events/kprobes/enable
cat /sys/kernel/tracing/dynamic_events

Results:

ecdsa: (ecdsa_set_pub_key+0xc1/0xe0 <- ecc_is_pubkey_valid_full) arg1=0xffffffea

So I guess this is on a right track reverse engineering the format used
in this API at least :-)

BR, Jarkko

Re: [PATCH] crypto: ecdsa: Fix the public key format description

[Jarkko Sakkinen]

On Tue May 28, 2024 at 1:18 AM EEST, Jarkko Sakkinen wrote:
> On Tue May 28, 2024 at 12:58 AM EEST, Jarkko Sakkinen wrote:
> > On Tue May 28, 2024 at 12:05 AM EEST, Jarkko Sakkinen wrote:
> > > On Mon May 27, 2024 at 11:28 PM EEST, Jarkko Sakkinen wrote:
> > > > Public key blob is not just x and y concatenated. It follows RFC5480
> > > > section 2.2. Address this by re-documenting the function with the
> > > > correct description of the format.
> > > >
> > > > Link: https://datatracker.ietf.org/doc/html/rfc5480
> > > > Fixes: 4e6602916bc6 ("crypto: ecdsa - Add support for ECDSA signature verification")
> > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > ---
> > > > It is a bug fix that does not really need a stable backport. Still
> > > > categorizes as a bug because by following the existing documentation
> > > > you end up with an error code.
> > > >  crypto/ecdsa.c | 5 ++---
> > > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c
> > > > index 258fffbf623d..55114146ff84 100644
> > > > --- a/crypto/ecdsa.c
> > > > +++ b/crypto/ecdsa.c
> > > > @@ -215,9 +215,8 @@ static int ecdsa_ecc_ctx_reset(struct ecc_ctx *ctx)
> > > >  }
> > > >  
> > > >  /*
> > > > - * Set the public key given the raw uncompressed key data from an X509
> > > > - * certificate. The key data contain the concatenated X and Y coordinates of
> > > > - * the public key.
> > > > + * Set the public ECC key as defined by RFC5480 section 2.2 "Subject Public
> > > > + * Key". Only the uncompressed format is supported.
> > > >   */
> > > >  static int ecdsa_set_pub_key(struct crypto_akcipher *tfm, const void *key, unsigned int keylen)
> > > >  {
> > >
> > > Based on this, is this now along the lines of correct format":
> > >
> > >        *ptr++ = 0x04; /* uncompressed */
> > >        ptr = asn1_encode_octet_string(&ptr[0], &in[sizeof(in)], &x[0], x_size);
> > >        ptr = asn1_encode_octet_string(&ptr[0], &in[sizeof(in)], &x[x_size + 2], x_size);
> > >        in_len = ptr - in;
> > >        ret = crypto_akcipher_set_pub_key(tfm, in, in_len);
> >
> >
> > I fixed up the above as it should be only single octect string to this:
> >
> > 	ptr = &in[0];
> >         *ptr++ = 0x04; /* uncompressed */
> >         ptr = asn1_encode_octet_string(&ptr[0], &in[sizeof(in)],
> >                                        &data[0], 2 * x_size);
> >         in_len = ptr - in;
> >         pr_info("in_len=%u\n", in_len);
> >         ret = crypto_akcipher_set_pub_key(tfm, in, in_len);
> >         crypto_free_akcipher(tfm);
> >
> > It fails in:
> >
> > 	ndigits = DIV_ROUND_UP(digitlen, sizeof(u64));
> > 	if (ndigits != ctx->curve->g.ndigits)
> > 		return -EINVAL;
> >
> > I checked that in_len=67.
> >
> > The tfm is deleted at instant because the above code is part of *_query.
> > TPM2 ECDSA asymmetric key that way that signature verification will work
> > when it is needed. The key type signs with TPM and verifies with the
> > software cipher.
>
> 3rd trial, i.e. no octect encoding at all but with the undocumented
> (in the current mainline) prefix byte:
>
>         ptr = &data[0];
>         *ptr++ = 0x04; /* uncompressed */
>         memcpy(&ptr[0], &x[0], x_size);
>         memcpy(&ptr[x_size], &x[x_size + 2], x_size);
>         ret = crypto_akcipher_set_pub_key(tfm, data, 3 * x_size + 1);
>         crypto_free_akcipher(tfm);
>
> I added also kprobe:
>
> echo 'r:ecdsa ecdsa_set_pub_key $retval' >> /sys/kernel/tracing/kprobe_events
> echo 1 > /sys/kernel/tracing/events/kprobes/enable
> cat /sys/kernel/tracing/dynamic_events
>
> Results:
>
> ecdsa: (ecdsa_set_pub_key+0xc1/0xe0 <- ecc_is_pubkey_valid_full) arg1=0xffffffea
>
> So I guess this is on a right track reverse engineering the format used
> in this API at least :-)

AFAIK TPM2 x and y are big-endian but I have no idea what
ecc_digits_from_bytes() expects because it has no documentation.

I guess it expect little-endian format, is this correct?

BR, Jarkko

Re: [PATCH] crypto: ecdsa: Fix the public key format description

[Jarkko Sakkinen]

On Tue May 28, 2024 at 1:31 AM EEST, Jarkko Sakkinen wrote:
> >         ret = crypto_akcipher_set_pub_key(tfm, data, 3 * x_size + 1);

Noticed this mistake i.e. fixed it with "2 * x_size + 1"

This is results earlier failure:

ecdsa: (tpm2_key_ecdsa_query+0x10d/0x170 <- ecdsa_set_pub_key) arg1=0xffffffea

Totally lost with the expected input format after trying out various 
options.

BR, Jarkko

Re: [PATCH] crypto: ecdsa: Fix the public key format description

[Jarkko Sakkinen]

On Tue May 28, 2024 at 1:49 AM EEST, Jarkko Sakkinen wrote:
> On Tue May 28, 2024 at 1:31 AM EEST, Jarkko Sakkinen wrote:
> > >         ret = crypto_akcipher_set_pub_key(tfm, data, 3 * x_size + 1);
>
> Noticed this mistake i.e. fixed it with "2 * x_size + 1"
>
> This is results earlier failure:
>
> ecdsa: (tpm2_key_ecdsa_query+0x10d/0x170 <- ecdsa_set_pub_key) arg1=0xffffffea
>
> Totally lost with the expected input format after trying out various 
> options.

OK got it working with:

        ptr = &data[0];
        *ptr++ = 0x04; /* uncompressed */
        memcpy(&ptr[0], &x[2], x_size);
        memcpy(&ptr[x_size], &x[2 + x_size + 2], x_size);
        ret = crypto_akcipher_set_pub_key(tfm, data, 2 * x_size + 1);
        crypto_free_akcipher(tfm);

Had still a few "off-bys".

Makes me wonder why this is not in ASN.1.
E.g. TPM2 stuff and for instance RSA code takes ASN.1.

This all and the required prefix byte really should be explained in
the documentation of this function. I.e. follows the RFC in the sense
that number is big-endian and has the prefix byte, but it does not
follow it in the sense that x and y are not in input octect strings.

Why is that? Does not feel right intuitively.

BR, Jarkko

Re: [PATCH] crypto: ecdsa: Fix the public key format description

[Stefan Berger]

On 5/27/24 18:59, Jarkko Sakkinen wrote:
> On Tue May 28, 2024 at 1:49 AM EEST, Jarkko Sakkinen wrote:
>> On Tue May 28, 2024 at 1:31 AM EEST, Jarkko Sakkinen wrote:
>>>>          ret = crypto_akcipher_set_pub_key(tfm, data, 3 * x_size + 1);
>>
>> Noticed this mistake i.e. fixed it with "2 * x_size + 1"
>>
>> This is results earlier failure:
>>
>> ecdsa: (tpm2_key_ecdsa_query+0x10d/0x170 <- ecdsa_set_pub_key) arg1=0xffffffea
>>
>> Totally lost with the expected input format after trying out various
>> options.
> 
> OK got it working with:
> 
>          ptr = &data[0];
>          *ptr++ = 0x04; /* uncompressed */
>          memcpy(&ptr[0], &x[2], x_size);
>          memcpy(&ptr[x_size], &x[2 + x_size + 2], x_size);
>          ret = crypto_akcipher_set_pub_key(tfm, data, 2 * x_size + 1);
>          crypto_free_akcipher(tfm);
> 
> Had still a few "off-bys".
> 
> Makes me wonder why this is not in ASN.1.
> E.g. TPM2 stuff and for instance RSA code takes ASN.1.
> 
> This all and the required prefix byte really should be explained in
> the documentation of this function. I.e. follows the RFC in the sense
> that number is big-endian and has the prefix byte, but it does not
> follow it in the sense that x and y are not in input octect strings.
> 
> Why is that? Does not feel right intuitively.

You found the appropriate documentation -- thanks.
The old function documentation stated that it takes 'raw uncompressed 
key data from an x509 certificate'. So, one should take the data from 
the x509 certificate starting with 0x04 as shown here.

         Subject Public Key Info:
             Public Key Algorithm: id-ecPublicKey
                 Public-Key: (256 bit)
                 pub:
                     04:c0:55:b4:68:7a:80:bc:0e:ba:b3:66:40:5f:07:
                     aa:27:d4:da:b4:79:2e:4d:a4:f4:f4:33:b1:22:6a:
                     6c:e9:8c:30:8d:6c:df:ac:65:f0:93:d9:7a:70:7a:
                     05:dc:7a:7d:b3:91:18:22:9a:5c:86:9a:87:72:3b:
                     32:1a:92:81:1d
                 ASN1 OID: prime256v1
                 NIST CURVE: P-256


These are two concatenated x & y coordinates with a leading 0x4. The 
numbers are not ints in ASN.1 format but 'plain' integers.

A *signature*, however, is in ASN.1:

     Signature Value:
         30:45:02:21:00:d9:d7:64:ba:5d:03:07:ee:20:a0:12:16:46:
         31:e6:8e:66:0c:17:0d:74:07:87:58:5a:13:fc:14:62:98:9a:
         99:02:20:59:ff:29:9c:52:b9:0a:35:3c:4b:03:bb:47:0e:c8:
         3e:2d:cb:3e:1c:d3:51:88:91:b1:40:e3:03:86:1b:2a:e8

30:45 => sequence containing 69 bytes
   02:21: => first coordinate with 0x21 bytes
     00:d9 => 0x21 bytes of ASN.1 integer with leading 0 to make the 
following 0x20-byte integer a positive number (its most significant bit 
is set).
   02:20: => int with 0x20 bytes
    ...



> 
> BR, Jarkko

Re: [PATCH] crypto: ecdsa: Fix the public key format description

[Jarkko Sakkinen]

On Tue May 28, 2024 at 3:37 PM EEST, Stefan Berger wrote:
>
>
> On 5/27/24 18:59, Jarkko Sakkinen wrote:
> > On Tue May 28, 2024 at 1:49 AM EEST, Jarkko Sakkinen wrote:
> >> On Tue May 28, 2024 at 1:31 AM EEST, Jarkko Sakkinen wrote:
> >>>>          ret = crypto_akcipher_set_pub_key(tfm, data, 3 * x_size + 1);
> >>
> >> Noticed this mistake i.e. fixed it with "2 * x_size + 1"
> >>
> >> This is results earlier failure:
> >>
> >> ecdsa: (tpm2_key_ecdsa_query+0x10d/0x170 <- ecdsa_set_pub_key) arg1=0xffffffea
> >>
> >> Totally lost with the expected input format after trying out various
> >> options.
> > 
> > OK got it working with:
> > 
> >          ptr = &data[0];
> >          *ptr++ = 0x04; /* uncompressed */
> >          memcpy(&ptr[0], &x[2], x_size);
> >          memcpy(&ptr[x_size], &x[2 + x_size + 2], x_size);
> >          ret = crypto_akcipher_set_pub_key(tfm, data, 2 * x_size + 1);
> >          crypto_free_akcipher(tfm);
> > 
> > Had still a few "off-bys".
> > 
> > Makes me wonder why this is not in ASN.1.
> > E.g. TPM2 stuff and for instance RSA code takes ASN.1.
> > 
> > This all and the required prefix byte really should be explained in
> > the documentation of this function. I.e. follows the RFC in the sense
> > that number is big-endian and has the prefix byte, but it does not
> > follow it in the sense that x and y are not in input octect strings.
> > 
> > Why is that? Does not feel right intuitively.
>
> You found the appropriate documentation -- thanks.
> The old function documentation stated that it takes 'raw uncompressed 
> key data from an x509 certificate'. So, one should take the data from 
> the x509 certificate starting with 0x04 as shown here.
>
>          Subject Public Key Info:
>              Public Key Algorithm: id-ecPublicKey
>                  Public-Key: (256 bit)
>                  pub:
>                      04:c0:55:b4:68:7a:80:bc:0e:ba:b3:66:40:5f:07:
>                      aa:27:d4:da:b4:79:2e:4d:a4:f4:f4:33:b1:22:6a:
>                      6c:e9:8c:30:8d:6c:df:ac:65:f0:93:d9:7a:70:7a:
>                      05:dc:7a:7d:b3:91:18:22:9a:5c:86:9a:87:72:3b:
>                      32:1a:92:81:1d
>                  ASN1 OID: prime256v1
>                  NIST CURVE: P-256
>
>
> These are two concatenated x & y coordinates with a leading 0x4. The 
> numbers are not ints in ASN.1 format but 'plain' integers.
>
> A *signature*, however, is in ASN.1:
>
>      Signature Value:
>          30:45:02:21:00:d9:d7:64:ba:5d:03:07:ee:20:a0:12:16:46:
>          31:e6:8e:66:0c:17:0d:74:07:87:58:5a:13:fc:14:62:98:9a:
>          99:02:20:59:ff:29:9c:52:b9:0a:35:3c:4b:03:bb:47:0e:c8:
>          3e:2d:cb:3e:1c:d3:51:88:91:b1:40:e3:03:86:1b:2a:e8
>
> 30:45 => sequence containing 69 bytes
>    02:21: => first coordinate with 0x21 bytes
>      00:d9 => 0x21 bytes of ASN.1 integer with leading 0 to make the 
> following 0x20-byte integer a positive number (its most significant bit 
> is set).
>    02:20: => int with 0x20 bytes
>     ...

Ah, thanks for this insight! I found this out by trial and error but
good to see that it matches with the "theory". Thank you.

BR, Jarkko

Re: [PATCH] crypto: ecdsa: Fix the public key format description

[Jarkko Sakkinen]

On Tue May 28, 2024 at 3:37 PM EEST, Stefan Berger wrote:
>      Signature Value:
>          30:45:02:21:00:d9:d7:64:ba:5d:03:07:ee:20:a0:12:16:46:
>          31:e6:8e:66:0c:17:0d:74:07:87:58:5a:13:fc:14:62:98:9a:
>          99:02:20:59:ff:29:9c:52:b9:0a:35:3c:4b:03:bb:47:0e:c8:
>          3e:2d:cb:3e:1c:d3:51:88:91:b1:40:e3:03:86:1b:2a:e8
>
> 30:45 => sequence containing 69 bytes
>    02:21: => first coordinate with 0x21 bytes
>      00:d9 => 0x21 bytes of ASN.1 integer with leading 0 to make the 
> following 0x20-byte integer a positive number (its most significant bit 
> is set).
>    02:20: => int with 0x20 bytes
>     ...

This actually helped me located the bug in my code: I had 32 bytes for
the first one, with no leading zero. I.e. total length was off-by-one.

So I'll just extend either or both based on msb? 

BR, Jarkko

Re: [PATCH] crypto: ecdsa: Fix the public key format description

[Jarkko Sakkinen]

On Tue May 28, 2024 at 4:26 PM EEST, Jarkko Sakkinen wrote:
> On Tue May 28, 2024 at 3:37 PM EEST, Stefan Berger wrote:
> >      Signature Value:
> >          30:45:02:21:00:d9:d7:64:ba:5d:03:07:ee:20:a0:12:16:46:
> >          31:e6:8e:66:0c:17:0d:74:07:87:58:5a:13:fc:14:62:98:9a:
> >          99:02:20:59:ff:29:9c:52:b9:0a:35:3c:4b:03:bb:47:0e:c8:
> >          3e:2d:cb:3e:1c:d3:51:88:91:b1:40:e3:03:86:1b:2a:e8
> >
> > 30:45 => sequence containing 69 bytes
> >    02:21: => first coordinate with 0x21 bytes
> >      00:d9 => 0x21 bytes of ASN.1 integer with leading 0 to make the 
> > following 0x20-byte integer a positive number (its most significant bit 
> > is set).
> >    02:20: => int with 0x20 bytes
> >     ...
>
> This actually helped me located the bug in my code: I had 32 bytes for
> the first one, with no leading zero. I.e. total length was off-by-one.
>
> So I'll just extend either or both based on msb? 

Actually I use a patch that I made for early version:

https://lore.kernel.org/linux-integrity/20240521152659.26438-3-jarkko@kernel.org/

BR, Jarkko

Re: [PATCH] crypto: ecdsa: Fix the public key format description

[Stefan Berger]

On 5/27/24 16:28, Jarkko Sakkinen wrote:
> Public key blob is not just x and y concatenated. It follows RFC5480
> section 2.2. Address this by re-documenting the function with the
> correct description of the format.
> 
> Link: https://datatracker.ietf.org/doc/html/rfc5480
> Fixes: 4e6602916bc6 ("crypto: ecdsa - Add support for ECDSA signature verification")
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

> ---
> It is a bug fix that does not really need a stable backport. Still
> categorizes as a bug because by following the existing documentation
> you end up with an error code.
>   crypto/ecdsa.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c
> index 258fffbf623d..55114146ff84 100644
> --- a/crypto/ecdsa.c
> +++ b/crypto/ecdsa.c
> @@ -215,9 +215,8 @@ static int ecdsa_ecc_ctx_reset(struct ecc_ctx *ctx)
>   }
>   
>   /*
> - * Set the public key given the raw uncompressed key data from an X509
> - * certificate. The key data contain the concatenated X and Y coordinates of
> - * the public key.
> + * Set the public ECC key as defined by RFC5480 section 2.2 "Subject Public
> + * Key". Only the uncompressed format is supported.
>    */
>   static int ecdsa_set_pub_key(struct crypto_akcipher *tfm, const void *key, unsigned int keylen)
>   {

Re: [PATCH] crypto: ecdsa: Fix the public key format description

[Jarkko Sakkinen]

On Tue May 28, 2024 at 2:18 PM EEST, Stefan Berger wrote:
>
>
> On 5/27/24 16:28, Jarkko Sakkinen wrote:
> > Public key blob is not just x and y concatenated. It follows RFC5480
> > section 2.2. Address this by re-documenting the function with the
> > correct description of the format.
> > 
> > Link: https://datatracker.ietf.org/doc/html/rfc5480
> > Fixes: 4e6602916bc6 ("crypto: ecdsa - Add support for ECDSA signature verification")
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

I think doing TPM2 ECDSA is a good test for this code, which is not
*that* mature in terms of age (from 2021 if I checked correctly). I just
try to complain at instant when I see badly documented code, when using
something new, because after a while you become blind to it...

The code quality itself is IMHO in good level and I could understand
what it is doing.

The EKEYREJECTED that I got is I think my own fault. Have to just test
the fix and send updated version of TPM2 Asymmetric Keys. Getting that
patch set to the mainline will also support quite well crypto/ecdsa.c,
which my code uses for verifying the signature using the public key.

Just adding these bits to underline that I don't think in any level
that any of this code would suck ;-) It is all good and working so
far...

BR, Jarkko

Re: [PATCH] crypto: ecdsa: Fix the public key format description

[Herbert Xu]

On Mon, May 27, 2024 at 11:28:39PM +0300, Jarkko Sakkinen wrote:
> Public key blob is not just x and y concatenated. It follows RFC5480
> section 2.2. Address this by re-documenting the function with the
> correct description of the format.
> 
> Link: https://datatracker.ietf.org/doc/html/rfc5480
> Fixes: 4e6602916bc6 ("crypto: ecdsa - Add support for ECDSA signature verification")
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> It is a bug fix that does not really need a stable backport. Still
> categorizes as a bug because by following the existing documentation
> you end up with an error code.
>  crypto/ecdsa.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt