ecc: No check for broken public key when verify signature (ECDSA, ECDSA for SM and GOST)
Closed, ResolvedPublic

Description

My fuzzer found this:

ecc curve: secp256r1
public key X:
4534198767316794591643245143622298809742628679895448054572722918996032022405
public key Y:
107839128084157537346759045080774377135290251058561962283882310383644151460337
cleartext: {0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x00, 0xff, 0xff,
0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
0xbc, 0xe6, 0xfa, 0xad, 0xa7, 0x17, 0x9e, 0x84, 0xf3, 0xb9, 0xca, 0xc2,
0xfc, 0x63, 0x25, 0x51} (32 bytes)
signature R:
4534198767316794591643245143622298809742628679895448054572722918996032022405
signature S:
4534198767316794591643245143622298809742628679895448054572722918996032022405

where 'cleartext' is the data passed as-is (unhashed) to the verification
function.

gcry_pk_verify() returns GPG_ERR_NO_ERROR for these parameters but other
libraries return failure.

gniibe created this task.Feb 1 2021, 8:13 AM

Here is a patch adding those checks:

diff --git a/cipher/ecc-ecdsa.c b/cipher/ecc-ecdsa.c
index d540578e..30103f14 100644
--- a/cipher/ecc-ecdsa.c
+++ b/cipher/ecc-ecdsa.c
@@ -172,6 +172,9 @@ _gcry_ecc_ecdsa_verify (gcry_mpi_t input, mpi_ec_t ec,
   mpi_point_struct Q, Q1, Q2;
   unsigned int nbits;
 
+  if (!_gcry_mpi_ec_curve_point (ec->Q, ec))
+    return GPG_ERR_BROKEN_PUBKEY;
+
   if( !(mpi_cmp_ui (r, 0) > 0 && mpi_cmp (r, ec->n) < 0) )
     return GPG_ERR_BAD_SIGNATURE; /* Assertion	0 < r < n  failed.  */
   if( !(mpi_cmp_ui (s, 0) > 0 && mpi_cmp (s, ec->n) < 0) )
diff --git a/cipher/ecc-gost.c b/cipher/ecc-gost.c
index c5ab774d..36230f8a 100644
--- a/cipher/ecc-gost.c
+++ b/cipher/ecc-gost.c
@@ -132,6 +132,9 @@ _gcry_ecc_gost_verify (gcry_mpi_t input, mpi_ec_t ec,
   gcry_mpi_t e, x, z1, z2, v, rv, zero;
   mpi_point_struct Q, Q1, Q2;
 
+  if (!_gcry_mpi_ec_curve_point (ec->Q, ec))
+    return GPG_ERR_BROKEN_PUBKEY;
+
   if( !(mpi_cmp_ui (r, 0) > 0 && mpi_cmp (r, ec->n) < 0) )
     return GPG_ERR_BAD_SIGNATURE; /* Assertion	0 < r < n  failed.  */
   if( !(mpi_cmp_ui (s, 0) > 0 && mpi_cmp (s, ec->n) < 0) )
diff --git a/cipher/ecc-sm2.c b/cipher/ecc-sm2.c
index 135c7697..c52629fd 100644
--- a/cipher/ecc-sm2.c
+++ b/cipher/ecc-sm2.c
@@ -500,6 +500,9 @@ _gcry_ecc_sm2_verify (gcry_mpi_t input, mpi_ec_t ec,
   gcry_mpi_t x1, y1;
   unsigned int nbits;
 
+  if (!_gcry_mpi_ec_curve_point (ec->Q, ec))
+    return GPG_ERR_BROKEN_PUBKEY;
+
   /* r, s within [1, n-1] */
   if (mpi_cmp_ui (r, 1) < 0 || mpi_cmp (r, ec->n) > 0)
     return GPG_ERR_BAD_SIGNATURE;
werner added a subscriber: werner.

I think that a backport to 1.8. also makes sense

werner closed this task as Resolved.

Backport was done with commit rC1d312bc65846 (for unknown reasons it did not show up in the list of bugs related to this bug; I added it by hand). Fix will go into 1.8.8.