Add functions to reduce integers mod the order to EC_Group

This allows calculating the Barett reduction params just once,
when the group is initialized, then sharing them across all
operations which use that group.

8 files changed, 205 insertions, 165 deletions

diff --git a/src/lib/pubkey/ec_group/ec_group.cpp b/src/lib/pubkey/ec_group/ec_group.cpp
index 0c2cd566c..beff90eec 100644
--- a/src/lib/pubkey/ec_group/ec_group.cpp
+++ b/src/lib/pubkey/ec_group/ec_group.cpp
@@ -35,9 +35,10 @@ class EC_Group_Data final
          m_base_point(m_curve, g_x, g_y),
          m_order(order),
          m_cofactor(cofactor),
+         m_mod_order(order),
          m_oid(oid),
          m_p_bits(p.bits()),
-         m_p_bytes(p.bytes())
+         m_order_bits(order.bits())
          {
          }
 
@@ -60,18 +61,30 @@ class EC_Group_Data final
       BigInt g_y() const { return m_base_point.get_affine_y(); }
 
       size_t p_bits() const { return m_p_bits; }
-      size_t p_bytes() const { return m_p_bytes; }
+      size_t p_bytes() const { return (m_p_bits + 7) / 8; }
+
+      size_t order_bits() const { return m_order_bits; }
+      size_t order_bytes() const { return (m_order_bits + 7) / 8; }
 
       const CurveGFp& curve() const { return m_curve; }
       const PointGFp& base_point() const { return m_base_point; }
 
+      BigInt mod_order(const BigInt& x) const { return m_mod_order.reduce(x); }
+
+      BigInt multiply_mod_order(const BigInt& x, const BigInt& y) const
+         {
+         return m_mod_order.multiply(x, y);
+         }
+
    private:
       CurveGFp m_curve;
       PointGFp m_base_point;
       BigInt m_order;
       BigInt m_cofactor;
+      Modular_Reducer m_mod_order;
       OID m_oid;
-      size_t m_p_bits, m_p_bytes;
+      size_t m_p_bits;
+      size_t m_order_bits;
    };
 
 class EC_Group_Data_Map final
@@ -246,6 +259,9 @@ std::shared_ptr<EC_Group_Data> EC_Group::BER_decode_EC_group(const uint8_t bits[
          .end_cons()
          .verify_end();
 
+      if(p.bits() < 64 || p.is_negative() || a.is_negative() || b.is_negative() || order <= 0 || cofactor <= 0)
+         throw Decoding_Error("Invalid ECC parameters");
+
       std::pair<BigInt, BigInt> base_xy = Botan::OS2ECP(base_pt.data(), base_pt.size(), p, a, b);
 
       return ec_group_data().lookup_or_create(p, a, b, base_xy.first, base_xy.second, order, cofactor, OID());
@@ -348,6 +364,16 @@ size_t EC_Group::get_p_bytes() const
    return data().p_bytes();
    }
 
+size_t EC_Group::get_order_bits() const
+   {
+   return data().order_bits();
+   }
+
+size_t EC_Group::get_order_bytes() const
+   {
+   return data().order_bytes();
+   }
+
 const BigInt& EC_Group::get_p() const
    {
    return data().p();
@@ -378,6 +404,16 @@ const BigInt& EC_Group::get_cofactor() const
    return data().cofactor();
    }
 
+BigInt EC_Group::mod_order(const BigInt& k) const
+   {
+   return data().mod_order(k);
+   }
+
+BigInt EC_Group::multiply_mod_order(const BigInt& x, const BigInt& y) const
+   {
+   return data().multiply_mod_order(x, y);
+   }
+
 const OID& EC_Group::get_curve_oid() const
    {
    return data().oid();
diff --git a/src/lib/pubkey/ec_group/ec_group.h b/src/lib/pubkey/ec_group/ec_group.h
index 1dc839540..b4b0ec9b3 100644
--- a/src/lib/pubkey/ec_group/ec_group.h
+++ b/src/lib/pubkey/ec_group/ec_group.h
@@ -133,6 +133,16 @@ class BOTAN_PUBLIC_API(2,0) EC_Group final
       size_t get_p_bytes() const;
 
       /**
+      * Return the size of group order in bits (same as get_order().bits())
+      */
+      size_t get_order_bits() const;
+
+      /**
+      * Return the size of p in bytes (same as get_order().bytes())
+      */
+      size_t get_order_bytes() const;
+
+      /**
       * Return the prime modulus of the field
       */
       const BigInt& get_p() const;
@@ -159,6 +169,22 @@ class BOTAN_PUBLIC_API(2,0) EC_Group final
       */
       const BigInt& get_order() const;
 
+      /*
+      * Reduce x modulo the order
+      */
+      BigInt mod_order(const BigInt& x) const;
+
+      /*
+      * Reduce (x*y) modulo the order
+      */
+      BigInt multiply_mod_order(const BigInt& x, const BigInt& y) const;
+
+      /**
+      * Return the cofactor
+      * @result the cofactor
+      */
+      const BigInt& get_cofactor() const;
+
       /**
       * Return the OID of these domain parameters
       * @result the OID as a string
@@ -172,12 +198,6 @@ class BOTAN_PUBLIC_API(2,0) EC_Group final
       const OID& get_curve_oid() const;
 
       /**
-      * Return the cofactor
-      * @result the cofactor
-      */
-      const BigInt& get_cofactor() const;
-
-      /**
       * Return a point on this curve with the affine values x, y
       */
       PointGFp point(const BigInt& x, const BigInt& y) const;
diff --git a/src/lib/pubkey/ecdsa/ecdsa.cpp b/src/lib/pubkey/ecdsa/ecdsa.cpp
index 96bc50ec0..163936c08 100644
--- a/src/lib/pubkey/ecdsa/ecdsa.cpp
+++ b/src/lib/pubkey/ecdsa/ecdsa.cpp
@@ -2,7 +2,7 @@
 * ECDSA implemenation
 * (C) 2007 Manuel Hartl, FlexSecure GmbH
 *     2007 Falko Strenzke, FlexSecure GmbH
-*     2008-2010,2015,2016 Jack Lloyd
+*     2008-2010,2015,2016,2018 Jack Lloyd
 *     2016 René Korthaus
 *
 * Botan is released under the Simplified BSD License (see license.txt)
@@ -52,26 +52,25 @@ class ECDSA_Signature_Operation final : public PK_Ops::Signature_with_EMSA
       ECDSA_Signature_Operation(const ECDSA_PrivateKey& ecdsa,
                                 const std::string& emsa) :
          PK_Ops::Signature_with_EMSA(emsa),
-         m_order(ecdsa.domain().get_order()),
-         m_base_point(ecdsa.domain().get_base_point(), m_order),
-         m_x(ecdsa.private_value()),
-         m_mod_order(m_order)
+         m_group(ecdsa.domain()),
+         m_base_point(m_group.get_base_point(), m_group.get_order()),
+         m_x(ecdsa.private_value())
          {
 #if defined(BOTAN_HAS_RFC6979_GENERATOR)
          m_rfc6979_hash = hash_for_emsa(emsa);
 #endif
          }
 
-      size_t max_input_bits() const override { return m_order.bits(); }
+      size_t max_input_bits() const override { return m_group.get_order_bits(); }
 
       secure_vector<uint8_t> raw_sign(const uint8_t msg[], size_t msg_len,
-                                   RandomNumberGenerator& rng) override;
+                                      RandomNumberGenerator& rng) override;
 
    private:
-      const BigInt& m_order;
+      const EC_Group m_group;
       Blinded_Point_Multiply m_base_point;
       const BigInt& m_x;
-      Modular_Reducer m_mod_order;
+
 #if defined(BOTAN_HAS_RFC6979_GENERATOR)
       std::string m_rfc6979_hash;
 #endif
@@ -84,20 +83,21 @@ ECDSA_Signature_Operation::raw_sign(const uint8_t msg[], size_t msg_len,
    const BigInt m(msg, msg_len);
 
 #if defined(BOTAN_HAS_RFC6979_GENERATOR)
-   const BigInt k = generate_rfc6979_nonce(m_x, m_order, m, m_rfc6979_hash);
+   const BigInt k = generate_rfc6979_nonce(m_x, m_group.get_order(), m, m_rfc6979_hash);
 #else
-   const BigInt k = BigInt::random_integer(rng, 1, m_order);
+   const BigInt k = BigInt::random_integer(rng, 1, m_group.get_order());
 #endif
 
+   const BigInt k_inv = inverse_mod(k, m_group.get_order());
    const PointGFp k_times_P = m_base_point.blinded_multiply(k, rng);
-   const BigInt r = m_mod_order.reduce(k_times_P.get_affine_x());
-   const BigInt s = m_mod_order.multiply(inverse_mod(k, m_order), mul_add(m_x, r, m));
+   const BigInt r = m_group.mod_order(k_times_P.get_affine_x());
+   const BigInt s = m_group.multiply_mod_order(k_inv, mul_add(m_x, r, m));
 
    // With overwhelming probability, a bug rather than actual zero r/s
-   BOTAN_ASSERT(s != 0, "invalid s");
-   BOTAN_ASSERT(r != 0, "invalid r");
+   if(r.is_zero() || s.is_zero())
+      throw Internal_Error("During ECDSA signature generated zero r/s");
 
-   return BigInt::encode_fixed_length_int_pair(r, s, m_order.bytes());
+   return BigInt::encode_fixed_length_int_pair(r, s, m_group.get_order_bytes());
    }
 
 /**
@@ -109,52 +109,46 @@ class ECDSA_Verification_Operation final : public PK_Ops::Verification_with_EMSA
       ECDSA_Verification_Operation(const ECDSA_PublicKey& ecdsa,
                                    const std::string& emsa) :
          PK_Ops::Verification_with_EMSA(emsa),
-         m_base_point(ecdsa.domain().get_base_point()),
-         m_public_point(ecdsa.public_point()),
-         m_order(ecdsa.domain().get_order()),
-         m_mod_order(m_order)
+         m_group(ecdsa.domain()),
+         m_public_point(ecdsa.public_point())
          {
-         //m_public_point.precompute_multiples();
          }
 
-      size_t max_input_bits() const override { return m_order.bits(); }
+      size_t max_input_bits() const override { return m_group.get_order_bits(); }
 
       bool with_recovery() const override { return false; }
 
       bool verify(const uint8_t msg[], size_t msg_len,
                   const uint8_t sig[], size_t sig_len) override;
    private:
-      const PointGFp& m_base_point;
+      const EC_Group m_group;
       const PointGFp& m_public_point;
-      const BigInt& m_order;
-      // FIXME: should be offered by curve
-      Modular_Reducer m_mod_order;
    };
 
 bool ECDSA_Verification_Operation::verify(const uint8_t msg[], size_t msg_len,
                                           const uint8_t sig[], size_t sig_len)
    {
-   if(sig_len != m_order.bytes()*2)
+   if(sig_len != m_group.get_order_bytes() * 2)
       return false;
 
-   BigInt e(msg, msg_len);
+   const BigInt e(msg, msg_len);
 
-   BigInt r(sig, sig_len / 2);
-   BigInt s(sig + sig_len / 2, sig_len / 2);
+   const BigInt r(sig, sig_len / 2);
+   const BigInt s(sig + sig_len / 2, sig_len / 2);
 
-   if(r <= 0 || r >= m_order || s <= 0 || s >= m_order)
+   if(r <= 0 || r >= m_group.get_order() || s <= 0 || s >= m_group.get_order())
       return false;
 
-   BigInt w = inverse_mod(s, m_order);
+   const BigInt w = inverse_mod(s, m_group.get_order());
 
-   const BigInt u1 = m_mod_order.reduce(e * w);
-   const BigInt u2 = m_mod_order.reduce(r * w);
-   const PointGFp R = multi_exponentiate(m_base_point, u1, m_public_point, u2);
+   const BigInt u1 = m_group.multiply_mod_order(e, w);
+   const BigInt u2 = m_group.multiply_mod_order(r, w);
+   const PointGFp R = multi_exponentiate(m_group.get_base_point(), u1, m_public_point, u2);
 
    if(R.is_zero())
       return false;
 
-   const BigInt v = m_mod_order.reduce(R.get_affine_x());
+   const BigInt v = m_group.mod_order(R.get_affine_x());
    return (v == r);
    }
 
diff --git a/src/lib/pubkey/ecgdsa/ecgdsa.cpp b/src/lib/pubkey/ecgdsa/ecgdsa.cpp
index e5e17623f..3685306c6 100644
--- a/src/lib/pubkey/ecgdsa/ecgdsa.cpp
+++ b/src/lib/pubkey/ecgdsa/ecgdsa.cpp
@@ -1,6 +1,7 @@
 /*
 * ECGDSA (BSI-TR-03111, version 2.0)
 * (C) 2016 René Korthaus
+* (C) 2018 Jack Lloyd
 *
 * Botan is released under the Simplified BSD License (see license.txt)
 */
@@ -36,23 +37,21 @@ class ECGDSA_Signature_Operation final : public PK_Ops::Signature_with_EMSA
       ECGDSA_Signature_Operation(const ECGDSA_PrivateKey& ecgdsa,
                                 const std::string& emsa) :
          PK_Ops::Signature_with_EMSA(emsa),
-         m_order(ecgdsa.domain().get_order()),
-         m_base_point(ecgdsa.domain().get_base_point(), m_order),
-         m_x(ecgdsa.private_value()),
-         m_mod_order(m_order)
+         m_group(ecgdsa.domain()),
+         m_base_point(m_group.get_base_point(), m_group.get_order()),
+         m_x(ecgdsa.private_value())
          {
          }
 
       secure_vector<uint8_t> raw_sign(const uint8_t msg[], size_t msg_len,
-                                   RandomNumberGenerator& rng) override;
+                                      RandomNumberGenerator& rng) override;
 
-      size_t max_input_bits() const override { return m_order.bits(); }
+      size_t max_input_bits() const override { return m_group.get_order_bits(); }
 
    private:
-      const BigInt& m_order;
+      const EC_Group m_group;
       Blinded_Point_Multiply m_base_point;
       const BigInt& m_x;
-      Modular_Reducer m_mod_order;
    };
 
 secure_vector<uint8_t>
@@ -61,17 +60,17 @@ ECGDSA_Signature_Operation::raw_sign(const uint8_t msg[], size_t msg_len,
    {
    const BigInt m(msg, msg_len);
 
-   BigInt k = BigInt::random_integer(rng, 1, m_order);
+   BigInt k = BigInt::random_integer(rng, 1, m_group.get_order());
 
    const PointGFp k_times_P = m_base_point.blinded_multiply(k, rng);
-   const BigInt r = m_mod_order.reduce(k_times_P.get_affine_x());
-   const BigInt s = m_mod_order.multiply(m_x, mul_sub(k, r, m));
+   const BigInt r = m_group.mod_order(k_times_P.get_affine_x());
+   const BigInt s = m_group.multiply_mod_order(m_x, mul_sub(k, r, m));
 
    // With overwhelming probability, a bug rather than actual zero r/s
-   BOTAN_ASSERT(s != 0, "invalid s");
-   BOTAN_ASSERT(r != 0, "invalid r");
+   if(r.is_zero() || s.is_zero())
+      throw Internal_Error("During ECGDSA signature generated zero r/s");
 
-   return BigInt::encode_fixed_length_int_pair(r, s, m_order.bytes());
+   return BigInt::encode_fixed_length_int_pair(r, s, m_group.get_order_bytes());
    }
 
 /**
@@ -84,51 +83,46 @@ class ECGDSA_Verification_Operation final : public PK_Ops::Verification_with_EMS
       ECGDSA_Verification_Operation(const ECGDSA_PublicKey& ecgdsa,
                                    const std::string& emsa) :
          PK_Ops::Verification_with_EMSA(emsa),
-         m_base_point(ecgdsa.domain().get_base_point()),
-         m_public_point(ecgdsa.public_point()),
-         m_order(ecgdsa.domain().get_order()),
-         m_mod_order(m_order)
+         m_group(ecgdsa.domain()),
+         m_public_point(ecgdsa.public_point())
          {
          }
 
-      size_t max_input_bits() const override { return m_order.bits(); }
+      size_t max_input_bits() const override { return m_group.get_order_bits(); }
 
       bool with_recovery() const override { return false; }
 
       bool verify(const uint8_t msg[], size_t msg_len,
                   const uint8_t sig[], size_t sig_len) override;
    private:
-      const PointGFp& m_base_point;
+      const EC_Group m_group;
       const PointGFp& m_public_point;
-      const BigInt& m_order;
-      // FIXME: should be offered by curve
-      Modular_Reducer m_mod_order;
    };
 
 bool ECGDSA_Verification_Operation::verify(const uint8_t msg[], size_t msg_len,
                                            const uint8_t sig[], size_t sig_len)
    {
-   if(sig_len != m_order.bytes()*2)
+   if(sig_len != m_group.get_order_bytes() * 2)
       return false;
 
-   BigInt e(msg, msg_len);
+   const BigInt e(msg, msg_len);
 
-   BigInt r(sig, sig_len / 2);
-   BigInt s(sig + sig_len / 2, sig_len / 2);
+   const BigInt r(sig, sig_len / 2);
+   const BigInt s(sig + sig_len / 2, sig_len / 2);
 
-   if(r <= 0 || r >= m_order || s <= 0 || s >= m_order)
+   if(r <= 0 || r >= m_group.get_order() || s <= 0 || s >= m_group.get_order())
       return false;
 
-   BigInt w = inverse_mod(r, m_order);
+   const BigInt w = inverse_mod(r, m_group.get_order());
 
-   const BigInt u1 = m_mod_order.reduce(e * w);
-   const BigInt u2 = m_mod_order.reduce(s * w);
-   const PointGFp R = multi_exponentiate(m_base_point, u1, m_public_point, u2);
+   const BigInt u1 = m_group.multiply_mod_order(e, w);
+   const BigInt u2 = m_group.multiply_mod_order(s, w);
+   const PointGFp R = multi_exponentiate(m_group.get_base_point(), u1, m_public_point, u2);
 
    if(R.is_zero())
       return false;
 
-   const BigInt v = m_mod_order.reduce(R.get_affine_x());
+   const BigInt v = m_group.mod_order(R.get_affine_x());
    return (v == r);
    }
 
diff --git a/src/lib/pubkey/eckcdsa/eckcdsa.cpp b/src/lib/pubkey/eckcdsa/eckcdsa.cpp
index 4d88f4cf8..d6e0957f6 100644
--- a/src/lib/pubkey/eckcdsa/eckcdsa.cpp
+++ b/src/lib/pubkey/eckcdsa/eckcdsa.cpp
@@ -1,6 +1,7 @@
 /*
 * ECKCDSA (ISO/IEC 14888-3:2006/Cor.2:2009)
 * (C) 2016 René Korthaus, Sirrix AG
+* (C) 2018 Jack Lloyd
 *
 * Botan is released under the Simplified BSD License (see license.txt)
 */
@@ -43,10 +44,9 @@ class ECKCDSA_Signature_Operation final : public PK_Ops::Signature_with_EMSA
       ECKCDSA_Signature_Operation(const ECKCDSA_PrivateKey& eckcdsa,
                                 const std::string& emsa) :
          PK_Ops::Signature_with_EMSA(emsa),
-         m_order(eckcdsa.domain().get_order()),
-         m_base_point(eckcdsa.domain().get_base_point(), m_order),
+         m_group(eckcdsa.domain()),
+         m_base_point(m_group.get_base_point(), m_group.get_order()),
          m_x(eckcdsa.private_value()),
-         m_mod_order(m_order),
          m_prefix()
          {
          const BigInt public_point_x = eckcdsa.public_point().get_affine_x();
@@ -59,18 +59,17 @@ class ECKCDSA_Signature_Operation final : public PK_Ops::Signature_with_EMSA
          }
 
       secure_vector<uint8_t> raw_sign(const uint8_t msg[], size_t msg_len,
-                                   RandomNumberGenerator& rng) override;
+                                      RandomNumberGenerator& rng) override;
 
-      size_t max_input_bits() const override { return m_order.bits(); }
+      size_t max_input_bits() const override { return m_group.get_order_bits(); }
 
       bool has_prefix() override { return true; }
       secure_vector<uint8_t> message_prefix() const override { return m_prefix; }
 
    private:
-      const BigInt& m_order;
+      const EC_Group m_group;
       Blinded_Point_Multiply m_base_point;
       const BigInt& m_x;
-      Modular_Reducer m_mod_order;
       secure_vector<uint8_t> m_prefix;
    };
 
@@ -78,7 +77,7 @@ secure_vector<uint8_t>
 ECKCDSA_Signature_Operation::raw_sign(const uint8_t msg[], size_t,
                                      RandomNumberGenerator& rng)
    {
-   const BigInt k = BigInt::random_integer(rng, 1, m_order);
+   const BigInt k = BigInt::random_integer(rng, 1, m_group.get_order());
    const PointGFp k_times_P = m_base_point.blinded_multiply(k, rng);
    const BigInt k_times_P_x = k_times_P.get_affine_x();
 
@@ -94,13 +93,14 @@ ECKCDSA_Signature_Operation::raw_sign(const uint8_t msg[], size_t,
 
    xor_buf(c, msg, c.size());
    BigInt w(c.data(), c.size());
-   w = m_mod_order.reduce(w);
+   w = m_group.mod_order(w);
 
-   const BigInt s = m_mod_order.multiply(m_x, k - w);
-   BOTAN_ASSERT(s != 0, "invalid s");
+   const BigInt s = m_group.multiply_mod_order(m_x, k - w);
+   if(s.is_zero())
+      throw Internal_Error("During ECKCDSA signature generation created zero s");
 
    secure_vector<uint8_t> output = BigInt::encode_1363(r, c.size());
-   output += BigInt::encode_1363(s, m_order.bytes());
+   output += BigInt::encode_1363(s, m_group.get_order_bytes());
    return output;
    }
 
@@ -114,10 +114,8 @@ class ECKCDSA_Verification_Operation final : public PK_Ops::Verification_with_EM
       ECKCDSA_Verification_Operation(const ECKCDSA_PublicKey& eckcdsa,
                                    const std::string& emsa) :
          PK_Ops::Verification_with_EMSA(emsa),
-         m_base_point(eckcdsa.domain().get_base_point()),
+         m_group(eckcdsa.domain()),
          m_public_point(eckcdsa.public_point()),
-         m_order(eckcdsa.domain().get_order()),
-         m_mod_order(m_order),
          m_prefix()
          {
          const BigInt public_point_x = m_public_point.get_affine_x();
@@ -132,18 +130,15 @@ class ECKCDSA_Verification_Operation final : public PK_Ops::Verification_with_EM
       bool has_prefix() override { return true; }
       secure_vector<uint8_t> message_prefix() const override { return m_prefix; }
 
-      size_t max_input_bits() const override { return m_order.bits(); }
+      size_t max_input_bits() const override { return m_group.get_order_bits(); }
 
       bool with_recovery() const override { return false; }
 
       bool verify(const uint8_t msg[], size_t msg_len,
                   const uint8_t sig[], size_t sig_len) override;
    private:
-      const PointGFp& m_base_point;
+      const EC_Group m_group;
       const PointGFp& m_public_point;
-      const BigInt& m_order;
-      // FIXME: should be offered by curve
-      Modular_Reducer m_mod_order;
       secure_vector<uint8_t> m_prefix;
    };
 
@@ -152,8 +147,11 @@ bool ECKCDSA_Verification_Operation::verify(const uint8_t msg[], size_t,
    {
    const std::unique_ptr<HashFunction> hash = HashFunction::create(hash_for_signature());
    //calculate size of r
-   size_t size_r = std::min(hash -> output_length(), m_order.bytes());
-   if(sig_len != size_r+m_order.bytes())
+
+   const size_t order_bytes = m_group.get_order_bytes();
+
+   const size_t size_r = std::min(hash -> output_length(), order_bytes);
+   if(sig_len != size_r + order_bytes)
       {
       return false;
       }
@@ -161,9 +159,9 @@ bool ECKCDSA_Verification_Operation::verify(const uint8_t msg[], size_t,
    secure_vector<uint8_t> r(sig, sig + size_r);
 
    // check that 0 < s < q
-   const BigInt s(sig + size_r, m_order.bytes());
+   const BigInt s(sig + size_r, order_bytes);
 
-   if(s <= 0 || s >= m_order)
+   if(s <= 0 || s >= m_group.get_order())
       {
       return false;
       }
@@ -171,9 +169,9 @@ bool ECKCDSA_Verification_Operation::verify(const uint8_t msg[], size_t,
    secure_vector<uint8_t> r_xor_e(r);
    xor_buf(r_xor_e, msg, r.size());
    BigInt w(r_xor_e.data(), r_xor_e.size());
-   w = m_mod_order.reduce(w);
-   
-   const PointGFp q = multi_exponentiate(m_base_point, w, m_public_point, s);
+   w = m_group.mod_order(w);
+
+   const PointGFp q = multi_exponentiate(m_group.get_base_point(), w, m_public_point, s);
    const BigInt q_x = q.get_affine_x();
    secure_vector<uint8_t> c(q_x.bytes());
    q_x.binary_encode(c.data());
diff --git a/src/lib/pubkey/gost_3410/gost_3410.cpp b/src/lib/pubkey/gost_3410/gost_3410.cpp
index f005a349b..5d7c425d0 100644
--- a/src/lib/pubkey/gost_3410/gost_3410.cpp
+++ b/src/lib/pubkey/gost_3410/gost_3410.cpp
@@ -2,7 +2,7 @@
 * GOST 34.10-2001 implemenation
 * (C) 2007 Falko Strenzke, FlexSecure GmbH
 *          Manuel Hartl, FlexSecure GmbH
-* (C) 2008-2010,2015 Jack Lloyd
+* (C) 2008-2010,2015,2018 Jack Lloyd
 *
 * Botan is released under the Simplified BSD License (see license.txt)
 */
@@ -100,19 +100,18 @@ class GOST_3410_Signature_Operation final : public PK_Ops::Signature_with_EMSA
       GOST_3410_Signature_Operation(const GOST_3410_PrivateKey& gost_3410,
                                     const std::string& emsa) :
          PK_Ops::Signature_with_EMSA(emsa),
-         m_order(gost_3410.domain().get_order()),
-         m_mod_order(m_order),
-         m_base_point(gost_3410.domain().get_base_point(), m_order),
-         m_x(gost_3410.private_value()) {}
+         m_group(gost_3410.domain()),
+         m_base_point(m_group.get_base_point(), m_group.get_order()),
+         m_x(gost_3410.private_value())
+         {}
 
-      size_t max_input_bits() const override { return m_order.bits(); }
+      size_t max_input_bits() const override { return m_group.get_order_bits(); }
 
       secure_vector<uint8_t> raw_sign(const uint8_t msg[], size_t msg_len,
-                                   RandomNumberGenerator& rng) override;
+                                      RandomNumberGenerator& rng) override;
 
    private:
-      const BigInt& m_order;
-      Modular_Reducer m_mod_order;
+      const EC_Group m_group;
       Blinded_Point_Multiply m_base_point;
       const BigInt& m_x;
    };
@@ -122,29 +121,28 @@ GOST_3410_Signature_Operation::raw_sign(const uint8_t msg[], size_t msg_len,
                                         RandomNumberGenerator& rng)
    {
    BigInt k;
+
    do
-      k.randomize(rng, m_order.bits()-1);
-   while(k >= m_order);
+      {
+      k.randomize(rng, m_group.get_order_bits() - 1);
+      } while(k >= m_group.get_order());
 
    BigInt e = decode_le(msg, msg_len);
 
-   e = m_mod_order.reduce(e);
+   e = m_group.mod_order(e);
    if(e == 0)
       e = 1;
 
    const PointGFp k_times_P = m_base_point.blinded_multiply(k, rng);
    BOTAN_ASSERT(k_times_P.on_the_curve(), "GOST 34.10 k*g is on the curve");
 
-   const BigInt r = m_mod_order.reduce(k_times_P.get_affine_x());
-   const BigInt s = m_mod_order.reduce(r*m_x + k*e);
+   const BigInt r = m_group.mod_order(k_times_P.get_affine_x());
+   const BigInt s = m_group.mod_order(r*m_x + k*e);
 
    if(r == 0 || s == 0)
-      throw Invalid_State("GOST 34.10: r == 0 || s == 0");
+      throw Internal_Error("GOST 34.10 signature generation failed, r/s equal to zero");
 
-   secure_vector<uint8_t> output(2*m_order.bytes());
-   s.binary_encode(&output[output.size() / 2 - s.bytes()]);
-   r.binary_encode(&output[output.size() - r.bytes()]);
-   return output;
+   return BigInt::encode_fixed_length_int_pair(s, r, m_group.get_order_bytes());
    }
 
 /**
@@ -157,46 +155,46 @@ class GOST_3410_Verification_Operation final : public PK_Ops::Verification_with_
       GOST_3410_Verification_Operation(const GOST_3410_PublicKey& gost,
                                        const std::string& emsa) :
          PK_Ops::Verification_with_EMSA(emsa),
-         m_base_point(gost.domain().get_base_point()),
-         m_public_point(gost.public_point()),
-         m_order(gost.domain().get_order()) {}
+         m_group(gost.domain()),
+         m_public_point(gost.public_point())
+         {}
 
-      size_t max_input_bits() const override { return m_order.bits(); }
+      size_t max_input_bits() const override { return m_group.get_order_bits(); }
 
       bool with_recovery() const override { return false; }
 
       bool verify(const uint8_t msg[], size_t msg_len,
                   const uint8_t sig[], size_t sig_len) override;
    private:
-      const PointGFp& m_base_point;
+      const EC_Group m_group;
       const PointGFp& m_public_point;
-      const BigInt& m_order;
    };
 
 bool GOST_3410_Verification_Operation::verify(const uint8_t msg[], size_t msg_len,
                                               const uint8_t sig[], size_t sig_len)
    {
-   if(sig_len != m_order.bytes()*2)
+   if(sig_len != m_group.get_order_bytes() * 2)
       return false;
 
-   BigInt e = decode_le(msg, msg_len);
+   const BigInt s(sig, sig_len / 2);
+   const BigInt r(sig + sig_len / 2, sig_len / 2);
 
-   BigInt s(sig, sig_len / 2);
-   BigInt r(sig + sig_len / 2, sig_len / 2);
+   const BigInt& order = m_group.get_order();
 
-   if(r <= 0 || r >= m_order || s <= 0 || s >= m_order)
+   if(r <= 0 || r >= order || s <= 0 || s >= order)
       return false;
 
-   e %= m_order;
+   BigInt e = decode_le(msg, msg_len);
+   e = m_group.mod_order(e);
    if(e == 0)
       e = 1;
 
-   BigInt v = inverse_mod(e, m_order);
+   const BigInt v = inverse_mod(e, order);
 
-   BigInt z1 = (s*v) % m_order;
-   BigInt z2 = (-r*v) % m_order;
+   const BigInt z1 = m_group.multiply_mod_order(s, v);
+   const BigInt z2 = m_group.multiply_mod_order(-r, v);
 
-   PointGFp R = multi_exponentiate(m_base_point, z1,
+   PointGFp R = multi_exponentiate(m_group.get_base_point(), z1,
                                    m_public_point, z2);
 
    if(R.is_zero())
diff --git a/src/lib/pubkey/sm2/sm2.cpp b/src/lib/pubkey/sm2/sm2.cpp
index 652985ec9..05c8417ec 100644
--- a/src/lib/pubkey/sm2/sm2.cpp
+++ b/src/lib/pubkey/sm2/sm2.cpp
@@ -1,14 +1,15 @@
 /*
 * SM2 Signatures
 * (C) 2017 Ribose Inc
+* (C) 2018 Jack Lloyd
 *
 * Botan is released under the Simplified BSD License (see license.txt)
 */
 
 #include <botan/sm2.h>
 #include <botan/internal/pk_ops_impl.h>
+#include <botan/numthry.h>
 #include <botan/keypair.h>
-#include <botan/reducer.h>
 #include <botan/hash.h>
 
 namespace Botan {
@@ -81,11 +82,10 @@ class SM2_Signature_Operation final : public PK_Ops::Signature
       SM2_Signature_Operation(const SM2_Signature_PrivateKey& sm2,
                               const std::string& ident,
                               const std::string& hash) :
-         m_order(sm2.domain().get_order()),
-         m_base_point(sm2.domain().get_base_point(), m_order),
+         m_group(sm2.domain()),
+         m_base_point(sm2.domain().get_base_point(), sm2.domain().get_order()),
          m_x(sm2.private_value()),
          m_da_inv(sm2.get_da_inv()),
-         m_mod_order(m_order),
          m_hash(HashFunction::create_or_throw(hash))
          {
          // ZA=H256(ENTLA || IDA || a || b || xG || yG || xA || yA)
@@ -101,11 +101,10 @@ class SM2_Signature_Operation final : public PK_Ops::Signature
       secure_vector<uint8_t> sign(RandomNumberGenerator& rng) override;
 
    private:
-      const BigInt& m_order;
+      const EC_Group m_group;
       Blinded_Point_Multiply m_base_point;
       const BigInt& m_x;
       const BigInt& m_da_inv;
-      Modular_Reducer m_mod_order;
 
       std::vector<uint8_t> m_za;
       std::unique_ptr<HashFunction> m_hash;
@@ -114,18 +113,18 @@ class SM2_Signature_Operation final : public PK_Ops::Signature
 secure_vector<uint8_t>
 SM2_Signature_Operation::sign(RandomNumberGenerator& rng)
    {
-   const BigInt k = BigInt::random_integer(rng, 1, m_order);
+   const BigInt k = BigInt::random_integer(rng, 1, m_group.get_order());
 
    const PointGFp k_times_P = m_base_point.blinded_multiply(k, rng);
 
    const BigInt e = BigInt::decode(m_hash->final());
-   const BigInt r = m_mod_order.reduce(k_times_P.get_affine_x() + e);
-   const BigInt s = m_mod_order.multiply(m_da_inv, (k - r*m_x));
+   const BigInt r = m_group.mod_order(k_times_P.get_affine_x() + e);
+   const BigInt s = m_group.multiply_mod_order(m_da_inv, (k - r*m_x));
 
    // prepend ZA for next signature if any
    m_hash->update(m_za);
 
-   return BigInt::encode_fixed_length_int_pair(r, s, m_order.bytes());
+   return BigInt::encode_fixed_length_int_pair(r, s, m_group.get_order().bytes());
    }
 
 /**
@@ -137,10 +136,8 @@ class SM2_Verification_Operation final : public PK_Ops::Verification
       SM2_Verification_Operation(const SM2_Signature_PublicKey& sm2,
                                  const std::string& ident,
                                  const std::string& hash) :
-         m_base_point(sm2.domain().get_base_point()),
+         m_group(sm2.domain()),
          m_public_point(sm2.public_point()),
-         m_order(sm2.domain().get_order()),
-         m_mod_order(m_order),
          m_hash(HashFunction::create_or_throw(hash))
          {
          // ZA=H256(ENTLA || IDA || a || b || xG || yG || xA || yA)
@@ -155,11 +152,8 @@ class SM2_Verification_Operation final : public PK_Ops::Verification
 
       bool is_valid_signature(const uint8_t sig[], size_t sig_len) override;
    private:
-      const PointGFp& m_base_point;
+      const EC_Group m_group;
       const PointGFp& m_public_point;
-      const BigInt& m_order;
-      // FIXME: should be offered by curve
-      Modular_Reducer m_mod_order;
       std::vector<uint8_t> m_za;
       std::unique_ptr<HashFunction> m_hash;
    };
@@ -171,27 +165,27 @@ bool SM2_Verification_Operation::is_valid_signature(const uint8_t sig[], size_t
    // Update for next verification
    m_hash->update(m_za);
 
-   if(sig_len != m_order.bytes()*2)
+   if(sig_len != m_group.get_order().bytes()*2)
       return false;
 
    const BigInt r(sig, sig_len / 2);
    const BigInt s(sig + sig_len / 2, sig_len / 2);
 
-   if(r <= 0 || r >= m_order || s <= 0 || s >= m_order)
+   if(r <= 0 || r >= m_group.get_order() || s <= 0 || s >= m_group.get_order())
       return false;
 
-   const BigInt t = m_mod_order.reduce(r + s);
+   const BigInt t = m_group.mod_order(r + s);
 
    if(t == 0)
       return false;
 
-   const PointGFp R = multi_exponentiate(m_base_point, s, m_public_point, t);
+   const PointGFp R = multi_exponentiate(m_group.get_base_point(), s, m_public_point, t);
 
    // ???
    if(R.is_zero())
       return false;
 
-   return (m_mod_order.reduce(R.get_affine_x() + e) == r);
+   return (m_group.mod_order(R.get_affine_x() + e) == r);
    }
 
 }
diff --git a/src/tests/unit_ecc.cpp b/src/tests/unit_ecc.cpp
index 96e2438b3..bd8295033 100644
--- a/src/tests/unit_ecc.cpp
+++ b/src/tests/unit_ecc.cpp
@@ -889,9 +889,15 @@ class ECC_Invalid_Key_Tests final : public Text_Based_Test
          const std::string encoded = get_req_str(vars, "SubjectPublicKey");
          Botan::DataSource_Memory key_data(Botan::hex_decode(encoded));
 
-         std::unique_ptr<Botan::Public_Key> key(Botan::X509::load_key(key_data));
-         result.test_eq("public key fails check", key->check_key(Test::rng(), false), false);
-
+         try
+            {
+            std::unique_ptr<Botan::Public_Key> key(Botan::X509::load_key(key_data));
+            result.test_eq("public key fails check", key->check_key(Test::rng(), false), false);
+            }
+         catch(Botan::Decoding_Error&)
+            {
+            result.test_success("Decoding invalid ECC key results in decoding error exception");
+            }
 
          return result;
          }