Merge pull request #4211 from randombit/jack/project-r

Avoid an inversion during ECDSA/SM2/etc signature verification

src/lib/pubkey/ec_group/curve_gfp.h

@@ -114,6 +114,7 @@ class BOTAN_UNSTABLE_API CurveGFp final {
       friend class EC_Group_Data;
       friend class EC_Point_Base_Point_Precompute;
       friend class EC_Point_Var_Point_Precompute;
+      friend class EC_Mul2Table_Data_BN;
 
       /**
       * Create an uninitialized CurveGFp

src/lib/pubkey/ec_group/ec_group.cpp

@@ -761,19 +761,17 @@ std::optional<EC_AffinePoint> EC_Group::Mul2Table::mul2_vartime(const EC_Scalar&
    }
 }
 
-std::optional<EC_Scalar> EC_Group::Mul2Table::mul2_vartime_x_mod_order(const EC_Scalar& x, const EC_Scalar& y) const {
-   auto s = m_tbl->mul2_vartime_x_mod_order(x._inner(), y._inner());
-   if(s) {
-      return EC_Scalar::_from_inner(std::move(s));
-   } else {
-      return {};
-   }
+bool EC_Group::Mul2Table::mul2_vartime_x_mod_order_eq(const EC_Scalar& v,
+                                                      const EC_Scalar& x,
+                                                      const EC_Scalar& y) const {
+   return m_tbl->mul2_vartime_x_mod_order_eq(v._inner(), x._inner(), y._inner());
 }
 
-std::optional<EC_Scalar> EC_Group::Mul2Table::mul2_vartime_x_mod_order(const EC_Scalar& c,
-                                                                       const EC_Scalar& x,
-                                                                       const EC_Scalar& y) const {
-   return this->mul2_vartime_x_mod_order(c * x, c * y);
+bool EC_Group::Mul2Table::mul2_vartime_x_mod_order_eq(const EC_Scalar& v,
+                                                      const EC_Scalar& c,
+                                                      const EC_Scalar& x,
+                                                      const EC_Scalar& y) const {
+   return this->mul2_vartime_x_mod_order_eq(v, c * x, c * y);
 }
 
 }  // namespace Botan

src/lib/pubkey/ec_group/ec_group.h

@@ -249,28 +249,29 @@ class BOTAN_PUBLIC_API(2, 0) EC_Group final {
             std::optional<EC_AffinePoint> mul2_vartime(const EC_Scalar& x, const EC_Scalar& y) const;
 
             /**
-            * Return the x coordinate of g*x + h*y, reduced modulo the order
+            * Check if v equals the x coordinate of g*x + h*y reduced modulo the order
             *
             * Where g is the group generator and h is the value passed to the constructor
             *
-            * Returns nullopt if g*x + h*y was the point at infinity
+            * Returns false if unequal, including if g*x + h*y was the point at infinity
             *
             * @warning this function is variable time with respect to x and y
             */
-            std::optional<EC_Scalar> mul2_vartime_x_mod_order(const EC_Scalar& x, const EC_Scalar& y) const;
+            bool mul2_vartime_x_mod_order_eq(const EC_Scalar& v, const EC_Scalar& x, const EC_Scalar& y) const;
 
             /**
-            * Return the x coordinate of g*x*c + h*y*c, reduced modulo the order
+            * Check if v equals the x coordinate of g*x*c + h*y*c reduced modulo the order
             *
             * Where g is the group generator and h is the value passed to the constructor
             *
-            * Returns nullopt if g*x*c + h*y*c was the point at infinity
+            * Returns false if unequal, including if g*x*c + h*y*c was the point at infinity
             *
-            * @warning this function is variable time with respect to c, x and y
+            * @warning this function is variable time with respect to x and y
             */
-            std::optional<EC_Scalar> mul2_vartime_x_mod_order(const EC_Scalar& c,
-                                                              const EC_Scalar& x,
-                                                              const EC_Scalar& y) const;
+            bool mul2_vartime_x_mod_order_eq(const EC_Scalar& v,
+                                             const EC_Scalar& c,
+                                             const EC_Scalar& x,
+                                             const EC_Scalar& y) const;
 
             ~Mul2Table();
 

src/lib/pubkey/ec_group/ec_inner_bn.cpp

@@ -163,18 +163,71 @@ std::unique_ptr<EC_AffinePoint_Data> EC_Mul2Table_Data_BN::mul2_vartime(const EC
    return std::make_unique<EC_AffinePoint_Data_BN>(m_group, std::move(pt));
 }
 
-std::unique_ptr<EC_Scalar_Data> EC_Mul2Table_Data_BN::mul2_vartime_x_mod_order(const EC_Scalar_Data& x,
-                                                                               const EC_Scalar_Data& y) const {
-   BOTAN_ARG_CHECK(x.group() == m_group && y.group() == m_group, "Curve mismatch");
+bool EC_Mul2Table_Data_BN::mul2_vartime_x_mod_order_eq(const EC_Scalar_Data& v,
+                                                       const EC_Scalar_Data& x,
+                                                       const EC_Scalar_Data& y) const {
+   BOTAN_ARG_CHECK(x.group() == m_group && y.group() == m_group && v.group() == m_group, "Curve mismatch");
 
+   const auto& bn_v = EC_Scalar_Data_BN::checked_ref(v);
    const auto& bn_x = EC_Scalar_Data_BN::checked_ref(x);
    const auto& bn_y = EC_Scalar_Data_BN::checked_ref(y);
-   auto pt = m_tbl.multi_exp(bn_x.value(), bn_y.value());
+   const auto pt = m_tbl.multi_exp(bn_x.value(), bn_y.value());
 
    if(pt.is_zero()) {
-      return nullptr;
+      return false;
+   }
+
+   /*
+   * Note we're working with the projective coordinate directly here!
+   * Nominally we're doing this:
+   *
+   * return m_group->mod_order(pt.get_affine_x()) == bn_v.value();
+   *
+   * However by instead projecting r to an identical z as the x
+   * coordinate, we can compare without having to perform an
+   * expensive inversion in the field.
+   *
+   * That is, given (x*z2) and r, instead of checking if
+   *    (x*z2)*z2^-1 == r,
+   * we check if
+   *    (x*z2) == (r*z2)
+   */
+   auto& curve = m_group->curve();
+
+   secure_vector<word> ws;
+   BigInt vr = bn_v.value();
+   curve.to_rep(vr, ws);
+   BigInt z2, v_z2;
+   curve.sqr(z2, pt.get_z(), ws);
+   curve.mul(v_z2, vr, z2, ws);
+
+   /*
+   * Since (typically) the group order is slightly less than the size
+   * of the field elements, its possible the signer had to reduce the
+   * r component. If they did not reduce r, then this value is correct.
+   *
+   * Due to the Hasse bound, this case occurs almost always; the
+   * probability that a reduction was actually required is
+   * approximately 1 in 2^(n/2) where n is the bit length of the curve.
+   */
+   if(pt.get_x() == v_z2) {
+      return true;
    }
-   return std::make_unique<EC_Scalar_Data_BN>(m_group, m_group->mod_order(pt.get_affine_x()));
+
+   if(m_group->order_is_less_than_p()) {
+      vr = bn_v.value() + m_group->order();
+      if(vr < m_group->p()) {
+         curve.to_rep(vr, ws);
+         curve.mul(v_z2, vr, z2, ws);
+
+         if(pt.get_x() == v_z2) {
+            return true;
+         }
+      }
+   }
+
+   // Reject:
+   return false;
 }
 
 }  // namespace Botan

src/lib/pubkey/ec_group/ec_inner_bn.h

@@ -92,8 +92,9 @@ class EC_Mul2Table_Data_BN final : public EC_Mul2Table_Data {
       std::unique_ptr<EC_AffinePoint_Data> mul2_vartime(const EC_Scalar_Data& x,
                                                         const EC_Scalar_Data& y) const override;
 
-      std::unique_ptr<EC_Scalar_Data> mul2_vartime_x_mod_order(const EC_Scalar_Data& x,
-                                                               const EC_Scalar_Data& y) const override;
+      bool mul2_vartime_x_mod_order_eq(const EC_Scalar_Data& v,
+                                       const EC_Scalar_Data& x,
+                                       const EC_Scalar_Data& y) const override;
 
    private:
       std::shared_ptr<const EC_Group_Data> m_group;

src/lib/pubkey/ec_group/ec_inner_data.cpp

@@ -39,6 +39,7 @@ EC_Group_Data::EC_Group_Data(const BigInt& p,
       m_a_is_minus_3(a == p - 3),
       m_a_is_zero(a.is_zero()),
       m_has_cofactor(m_cofactor != 1),
+      m_order_is_less_than_p(m_order < p),
       m_source(source) {
    if(!m_oid.empty()) {
       DER_Encoder der(m_der_named_curve);

src/lib/pubkey/ec_group/ec_inner_data.h

@@ -93,9 +93,12 @@ class EC_Mul2Table_Data {
       virtual std::unique_ptr<EC_AffinePoint_Data> mul2_vartime(const EC_Scalar_Data& x,
                                                                 const EC_Scalar_Data& y) const = 0;
 
-      // Returns nullptr if g*x + h*y was point at infinity
-      virtual std::unique_ptr<EC_Scalar_Data> mul2_vartime_x_mod_order(const EC_Scalar_Data& x,
-                                                                       const EC_Scalar_Data& y) const = 0;
+      // Check if v == (g*x + h*y).x % n
+      //
+      // Returns false if g*x + h*y was point at infinity
+      virtual bool mul2_vartime_x_mod_order_eq(const EC_Scalar_Data& v,
+                                               const EC_Scalar_Data& x,
+                                               const EC_Scalar_Data& y) const = 0;
 };
 
 class EC_Group_Data final : public std::enable_shared_from_this<EC_Group_Data> {
@@ -136,6 +139,8 @@ class EC_Group_Data final : public std::enable_shared_from_this<EC_Group_Data> {
 
       const BigInt& cofactor() const { return m_cofactor; }
 
+      bool order_is_less_than_p() const { return m_order_is_less_than_p; }
+
       bool has_cofactor() const { return m_has_cofactor; }
 
       const BigInt& g_x() const { return m_g_x; }
@@ -228,6 +233,7 @@ class EC_Group_Data final : public std::enable_shared_from_this<EC_Group_Data> {
       bool m_a_is_minus_3;
       bool m_a_is_zero;
       bool m_has_cofactor;
+      bool m_order_is_less_than_p;
       EC_Group_Source m_source;
 };
 

src/lib/pubkey/ecdsa/ecdsa.cpp

@@ -219,9 +219,8 @@ bool ECDSA_Verification_Operation::verify(const uint8_t msg[], size_t msg_len, c
 
          const auto w = s.invert();
 
-         if(const auto v = m_gy_mul.mul2_vartime_x_mod_order(w, m, r)) {
-            return (v == r);
-         }
+         // Check if r == x_coord(g*w*m + y*w*r) % n
+         return m_gy_mul.mul2_vartime_x_mod_order_eq(r, w, m, r);
       }
    }
 

src/lib/pubkey/ecgdsa/ecgdsa.cpp

@@ -109,9 +109,8 @@ bool ECGDSA_Verification_Operation::verify(const uint8_t msg[], size_t msg_len,
 
          const auto w = r.invert();
 
-         if(const auto v = m_gy_mul.mul2_vartime_x_mod_order(w, m, s)) {
-            return (v == r);
-         }
+         // Check if r == x_coord(g*w*m + y*w*s) % n
+         return m_gy_mul.mul2_vartime_x_mod_order_eq(r, w, m, s);
       }
    }
 

src/lib/pubkey/gost_3410/gost_3410.cpp

@@ -229,9 +229,8 @@ bool GOST_3410_Verification_Operation::verify(const uint8_t msg[],
 
          const auto v = e.invert();
 
-         if(const auto w = m_gy_mul.mul2_vartime_x_mod_order(v, s, r.negate())) {
-            return (w == r);
-         }
+         // Check if r == x_coord(g*v*s - y*v*r) % n
+         return m_gy_mul.mul2_vartime_x_mod_order_eq(r, v, s, r.negate());
       }
    }
 

src/lib/pubkey/sm2/sm2.cpp

@@ -204,9 +204,8 @@ bool SM2_Verification_Operation::is_valid_signature(const uint8_t sig[], size_t
       if(r.is_nonzero() && s.is_nonzero()) {
          const auto t = r + s;
          if(t.is_nonzero()) {
-            if(const auto v = m_gy_mul.mul2_vartime_x_mod_order(s, t)) {
-               return (v.value() + e) == r;
-            }
+            // Check if r - e = x_coord(g*s + y*t) % n
+            return m_gy_mul.mul2_vartime_x_mod_order_eq(r - e, s, t);
          }
       }
    }