| Message ID | 20240527202840.4818-1-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: ecdsa: Fix the public key format description | expand |
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
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
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
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
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
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
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) > {
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
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
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
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
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
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.
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) {
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(-)