Repository: randombit/botan
Pull request #2325 (2020-04-08 02:23:22)

Fix flaky GOST 34.10 signature test
The problem was a race between the GOST 34.10 signature and verification
tests. Each registered an EC group, but signature registered it with an
OID and verification without. If during threaded test runs, verificatino
ran first, then signature would fail because there was no OID.

Fix the tests by registering using an OID in both cases. Also fix
EC_Group registration so if the same group is registered with and
without an OID, we update the internal state to use the OID.

Fixes GH #2197


Patch:
diff --git a/src/lib/pubkey/ec_group/ec_group.cpp b/src/lib/pubkey/ec_group/ec_group.cpp
index 41b48790f..801f24bf3 100644
--- a/src/lib/pubkey/ec_group/ec_group.cpp
+++ b/src/lib/pubkey/ec_group/ec_group.cpp
@@ -62,6 +62,12 @@ class EC_Group_Data final
                  this->g_y() == g_y);
          }
 
+      void set_oid(const OID& oid)
+         {
+         BOTAN_STATE_CHECK(m_oid.empty());
+         m_oid = oid;
+         }
+
       const OID& oid() const { return m_oid; }
       const BigInt& p() const { return m_curve.get_p(); }
       const BigInt& a() const { return m_curve.get_a(); }
@@ -178,16 +184,39 @@ class EC_Group_Data_Map final
 
          for(auto i : m_registered_curves)
             {
-            if(oid.has_value())
+            if(!oid.empty())
                {
                if(i->oid() == oid)
+                  {
+                  if(!i->match(p, a, b, g_x, g_y, order, cofactor))
+                     throw Invalid_Argument("Attempting to register a curve using OID " + oid.to_string() +
+                                            " but another curve is already registered using that OID");
                   return i;
+                  }
                else if(i->oid().has_value())
-                  continue;
+                  continue; // distinct OIDs so not a match
                }
 
             if(i->match(p, a, b, g_x, g_y, order, cofactor))
+               {
+               /*
+               * If the same curve was previously created without an OID
+               * but has been registered again using an OID, save that OID.
+               */
+               if(oid.empty() == false)
+                  {
+                  if(i->oid().empty() == true)
+                     {
+                     i->set_oid(oid);
+                     }
+                  else
+                     {
+                     throw Invalid_Argument("Cannot register ECC group with OID " + oid.to_string() +
+                                            " already registered using " + i->oid().to_string());
+                     }
+                  }
                return i;
+               }
             }
 
          // Not found - if OID is set try looking up that way
@@ -205,28 +234,14 @@ class EC_Group_Data_Map final
             }
 
          // Not found or no OID, add data and return
-         return add_curve(p, a, b, g_x, g_y, order, cofactor, oid);
-         }
-
-   private:
-
-      std::shared_ptr<EC_Group_Data> add_curve(const BigInt& p,
-                                               const BigInt& a,
-                                               const BigInt& b,
-                                               const BigInt& g_x,
-                                               const BigInt& g_y,
-                                               const BigInt& order,
-                                               const BigInt& cofactor,
-                                               const OID& oid)
-         {
          std::shared_ptr<EC_Group_Data> d =
             std::make_shared<EC_Group_Data>(p, a, b, g_x, g_y, order, cofactor, oid);
 
-         // This function is always called with the lock held
          m_registered_curves.push_back(d);
          return d;
          }
 
+   private:
       mutex_type m_mutex;
       std::vector<std::shared_ptr<EC_Group_Data>> m_registered_curves;
    };
diff --git a/src/lib/pubkey/gost_3410/gost_3410.cpp b/src/lib/pubkey/gost_3410/gost_3410.cpp
index 61f674370..d1f8fc786 100644
--- a/src/lib/pubkey/gost_3410/gost_3410.cpp
+++ b/src/lib/pubkey/gost_3410/gost_3410.cpp
@@ -54,12 +54,12 @@ AlgorithmIdentifier GOST_3410_PublicKey::algorithm_identifier() const
    {
    std::vector<uint8_t> params;
 
-   DER_Encoder(params)
-      .start_cons(SEQUENCE)
-         .encode(domain().get_curve_oid())
-      .end_cons();
+   const OID gost_oid = get_oid();
+   const OID domain_oid = domain().get_curve_oid();
 
-   return AlgorithmIdentifier(get_oid(), params);
+   DER_Encoder(params).start_cons(SEQUENCE).encode(domain_oid).end_cons();
+
+   return AlgorithmIdentifier(gost_oid, params);
    }
 
 GOST_3410_PublicKey::GOST_3410_PublicKey(const AlgorithmIdentifier& alg_id,


Test patch:
diff --git a/src/scripts/test_all_configs.py b/src/scripts/test_all_configs.py
index d143c171d..227abd952 100755
--- a/src/scripts/test_all_configs.py
+++ b/src/scripts/test_all_configs.py
@@ -49,10 +49,8 @@ def try_to_run(cmdline):
     if failed:
         print("FAILURE")
         print(stdout)
-        print(stdout)
         print(stderr)
         sys.stdout.flush()
-        #sys.exit(1)
 
     return not failed
 
@@ -75,8 +73,8 @@ def run_test_build(configure_py, modules, include, jobs, run_tests):
     if run_tests is False:
         return True
 
-    # Flaky test causing errors when running tests (GH #2197)
-    tests_to_skip = ['gost_3411_sign']
+    # Flaky test causing errors when running tests
+    tests_to_skip = []
 
     cmdline = ['./botan-test', '--test-threads=%d' % (jobs)]
 
diff --git a/src/tests/data/pubkey/gost_3410_sign.vec b/src/tests/data/pubkey/gost_3410_sign.vec
index 67599a7ec..6df3e682c 100644
--- a/src/tests/data/pubkey/gost_3410_sign.vec
+++ b/src/tests/data/pubkey/gost_3410_sign.vec
@@ -8,6 +8,8 @@ Order = 0x8000000000000000000000000000000150FE8A1892976154C59CFC193ACCF5B3
 Cofactor = 1
 Gx = 2
 Gy = 0x8E2A8A0E65147D4BD6316030E16D19C85C97F0A9CA267122B96ABBCEA7E8FC8
+Oid = 1.3.6.1.4.1.25258.2
+
 X = 0x7A929ADE789BB9BE10ED359DD39A72C11B60961F49397EEE1D19CE9891EC3B28
 
 Hash = Raw
diff --git a/src/tests/data/pubkey/gost_3410_verify.vec b/src/tests/data/pubkey/gost_3410_verify.vec
index 65e2dcd2c..5798538d6 100644
--- a/src/tests/data/pubkey/gost_3410_verify.vec
+++ b/src/tests/data/pubkey/gost_3410_verify.vec
@@ -8,6 +8,8 @@ Order = 0x8000000000000000000000000000000150FE8A1892976154C59CFC193ACCF5B3
 Cofactor = 1
 Gx = 2
 Gy = 0x8E2A8A0E65147D4BD6316030E16D19C85C97F0A9CA267122B96ABBCEA7E8FC8
+Oid = 1.3.6.1.4.1.25258.2
+
 Px = 0x7F2B49E270DB6D90D8595BEC458B50C58585BA1D4E9B788F6689DBD8E56FD80B
 Py = 0x26F1B489D6701DD185C8413A977B3CBBAF64D1C593D26627DFFB101A87FF77DA
 
diff --git a/src/tests/test_gost_3410.cpp b/src/tests/test_gost_3410.cpp
index 99ee0fc80..a7f176a57 100644
--- a/src/tests/test_gost_3410.cpp
+++ b/src/tests/test_gost_3410.cpp
@@ -23,7 +23,7 @@ class GOST_3410_2001_Verification_Tests final : public PK_Signature_Verification
       GOST_3410_2001_Verification_Tests() : PK_Signature_Verification_Test(
             "GOST 34.10-2001",
             "pubkey/gost_3410_verify.vec",
-            "P,A,B,Gx,Gy,Order,Cofactor,Px,Py,Hash,Msg,Signature") {}
+            "P,A,B,Gx,Gy,Oid,Order,Cofactor,Px,Py,Hash,Msg,Signature") {}
 
       std::unique_ptr<Botan::Public_Key> load_public_key(const VarMap& vars) override
          {
@@ -34,12 +34,13 @@ class GOST_3410_2001_Verification_Tests final : public PK_Signature_Verification
          const BigInt Gy = vars.get_req_bn("Gy");
          const BigInt order = vars.get_req_bn("Order");
          const BigInt cofactor = vars.get_req_bn("Cofactor");
+         const Botan::OID oid(vars.get_req_str("Oid"));
+
+         Botan::EC_Group group(p, a, b, Gx, Gy, order, cofactor, oid);
 
          const BigInt Px = vars.get_req_bn("Px");
          const BigInt Py = vars.get_req_bn("Py");
 
-         Botan::EC_Group group(p, a, b, Gx, Gy, order, cofactor);
-
          const Botan::PointGFp public_point = group.point(Px, Py);
 
          std::unique_ptr<Botan::Public_Key> key(new Botan::GOST_3410_PublicKey(group, public_point));
@@ -61,7 +62,7 @@ class GOST_3410_2001_Signature_Tests final : public PK_Signature_Generation_Test
       GOST_3410_2001_Signature_Tests() : PK_Signature_Generation_Test(
             "GOST 34.10-2001",
             "pubkey/gost_3410_sign.vec",
-            "P,A,B,Gx,Gy,Order,X,Cofactor,Hash,Nonce,Msg,Signature") {}
+            "P,A,B,Gx,Gy,Oid,Order,X,Cofactor,Hash,Nonce,Msg,Signature") {}
 
       std::unique_ptr<Botan::Private_Key> load_private_key(const VarMap& vars) override
          {
@@ -72,13 +73,12 @@ class GOST_3410_2001_Signature_Tests final : public PK_Signature_Generation_Test
          const BigInt Gy = vars.get_req_bn("Gy");
          const BigInt order = vars.get_req_bn("Order");
          const BigInt cofactor = vars.get_req_bn("Cofactor");
-
-         const BigInt x = vars.get_req_bn("X");
-
-         const Botan::OID oid("1.3.6.1.4.1.25258.2");
+         const Botan::OID oid(vars.get_req_str("Oid"));
 
          Botan::EC_Group group(p, a, b, Gx, Gy, order, cofactor, oid);
 
+         const BigInt x = vars.get_req_bn("X");
+
          std::unique_ptr<Botan::Private_Key> key(new Botan::GOST_3410_PrivateKey(Test::rng(), group, x));
          return key;
          }