From 0ad72dd0223f5a906cb08c8fef91c0e3e19b3842 Mon Sep 17 00:00:00 2001 From: Jack Lloyd Date: Sat, 13 Jul 2024 20:09:53 -0400 Subject: [PATCH 1/2] Instead of a mul2_vartime_x_mod_order returning a Scalar, perform equality check @gmaxwell pointed out in a really great comment on #1479 that you don't need to actually perform a projective->affine conversion in ECDSA verification, since instead you can project the r value. However in the current setup that's not possible since the function is defining as returning the value and then the comparison happens in the pubkey code. Instead have the expected value be passed down and all that comes back is a boolean accept or reject. This allows the project-r optimization. This also avoids some back and forth with the various type wrappers, which is a small win on its own. --- src/lib/pubkey/ec_group/ec_group.cpp | 20 +++++++++----------- src/lib/pubkey/ec_group/ec_group.h | 19 ++++++++++--------- src/lib/pubkey/ec_group/ec_inner_bn.cpp | 12 +++++++----- src/lib/pubkey/ec_group/ec_inner_bn.h | 5 +++-- src/lib/pubkey/ec_group/ec_inner_data.h | 9 ++++++--- src/lib/pubkey/ecdsa/ecdsa.cpp | 5 ++--- src/lib/pubkey/ecgdsa/ecgdsa.cpp | 5 ++--- src/lib/pubkey/gost_3410/gost_3410.cpp | 5 ++--- src/lib/pubkey/sm2/sm2.cpp | 5 ++--- 9 files changed, 43 insertions(+), 42 deletions(-) diff --git a/src/lib/pubkey/ec_group/ec_group.cpp b/src/lib/pubkey/ec_group/ec_group.cpp index e7a7cbc265e..54ddc9bd4b1 100644 --- a/src/lib/pubkey/ec_group/ec_group.cpp +++ b/src/lib/pubkey/ec_group/ec_group.cpp @@ -761,19 +761,17 @@ std::optional EC_Group::Mul2Table::mul2_vartime(const EC_Scalar& } } -std::optional 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_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 diff --git a/src/lib/pubkey/ec_group/ec_group.h b/src/lib/pubkey/ec_group/ec_group.h index a5dc7b1888f..46ade067628 100644 --- a/src/lib/pubkey/ec_group/ec_group.h +++ b/src/lib/pubkey/ec_group/ec_group.h @@ -249,28 +249,29 @@ class BOTAN_PUBLIC_API(2, 0) EC_Group final { std::optional 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 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 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(); diff --git a/src/lib/pubkey/ec_group/ec_inner_bn.cpp b/src/lib/pubkey/ec_group/ec_inner_bn.cpp index 710fec32063..fd99aa6fdeb 100644 --- a/src/lib/pubkey/ec_group/ec_inner_bn.cpp +++ b/src/lib/pubkey/ec_group/ec_inner_bn.cpp @@ -163,18 +163,20 @@ std::unique_ptr EC_Mul2Table_Data_BN::mul2_vartime(const EC return std::make_unique(m_group, std::move(pt)); } -std::unique_ptr 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()); if(pt.is_zero()) { - return nullptr; + return false; } - return std::make_unique(m_group, m_group->mod_order(pt.get_affine_x())); + return m_group->mod_order(pt.get_affine_x()) == bn_v.value(); } } // namespace Botan diff --git a/src/lib/pubkey/ec_group/ec_inner_bn.h b/src/lib/pubkey/ec_group/ec_inner_bn.h index a130ccf263a..b10227e11e4 100644 --- a/src/lib/pubkey/ec_group/ec_inner_bn.h +++ b/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 mul2_vartime(const EC_Scalar_Data& x, const EC_Scalar_Data& y) const override; - std::unique_ptr 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 m_group; diff --git a/src/lib/pubkey/ec_group/ec_inner_data.h b/src/lib/pubkey/ec_group/ec_inner_data.h index 3d4f1b2c68b..a53cfb9c8a4 100644 --- a/src/lib/pubkey/ec_group/ec_inner_data.h +++ b/src/lib/pubkey/ec_group/ec_inner_data.h @@ -93,9 +93,12 @@ class EC_Mul2Table_Data { virtual std::unique_ptr 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 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 { diff --git a/src/lib/pubkey/ecdsa/ecdsa.cpp b/src/lib/pubkey/ecdsa/ecdsa.cpp index 972a7c98adb..4717703a459 100644 --- a/src/lib/pubkey/ecdsa/ecdsa.cpp +++ b/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); } } diff --git a/src/lib/pubkey/ecgdsa/ecgdsa.cpp b/src/lib/pubkey/ecgdsa/ecgdsa.cpp index 502e4d30b23..a89725290b3 100644 --- a/src/lib/pubkey/ecgdsa/ecgdsa.cpp +++ b/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); } } diff --git a/src/lib/pubkey/gost_3410/gost_3410.cpp b/src/lib/pubkey/gost_3410/gost_3410.cpp index f8740fc4cce..a4bb18ef079 100644 --- a/src/lib/pubkey/gost_3410/gost_3410.cpp +++ b/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()); } } diff --git a/src/lib/pubkey/sm2/sm2.cpp b/src/lib/pubkey/sm2/sm2.cpp index d3f04ca8063..7f06d23aa26 100644 --- a/src/lib/pubkey/sm2/sm2.cpp +++ b/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); } } } From ef6efa3ca26c8d6e8c446061e32ad5852f502f13 Mon Sep 17 00:00:00 2001 From: Jack Lloyd Date: Sat, 13 Jul 2024 23:01:19 -0400 Subject: [PATCH 2/2] Avoid an inversion when checking if x_coord(g*x+h*y) % n == v Given a projective coordinate, we previously performed an inversion to extract the affine x. But instead we can project v using the same value of z as the projective point. This improves ECDSA verification performance by between 4% and 9%, depending on the curve. --- src/lib/pubkey/ec_group/curve_gfp.h | 1 + src/lib/pubkey/ec_group/ec_inner_bn.cpp | 55 ++++++++++++++++++++++- src/lib/pubkey/ec_group/ec_inner_data.cpp | 1 + src/lib/pubkey/ec_group/ec_inner_data.h | 3 ++ 4 files changed, 58 insertions(+), 2 deletions(-) diff --git a/src/lib/pubkey/ec_group/curve_gfp.h b/src/lib/pubkey/ec_group/curve_gfp.h index 3eadee0526e..b47a797a56f 100644 --- a/src/lib/pubkey/ec_group/curve_gfp.h +++ b/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 diff --git a/src/lib/pubkey/ec_group/ec_inner_bn.cpp b/src/lib/pubkey/ec_group/ec_inner_bn.cpp index fd99aa6fdeb..271fe93ae51 100644 --- a/src/lib/pubkey/ec_group/ec_inner_bn.cpp +++ b/src/lib/pubkey/ec_group/ec_inner_bn.cpp @@ -171,12 +171,63 @@ bool EC_Mul2Table_Data_BN::mul2_vartime_x_mod_order_eq(const EC_Scalar_Data& v, 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 false; } - return m_group->mod_order(pt.get_affine_x()) == bn_v.value(); + + /* + * 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 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; + } + + 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 diff --git a/src/lib/pubkey/ec_group/ec_inner_data.cpp b/src/lib/pubkey/ec_group/ec_inner_data.cpp index 8c7c87b3078..e13d15ddde9 100644 --- a/src/lib/pubkey/ec_group/ec_inner_data.cpp +++ b/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); diff --git a/src/lib/pubkey/ec_group/ec_inner_data.h b/src/lib/pubkey/ec_group/ec_inner_data.h index a53cfb9c8a4..c80ebafd5ba 100644 --- a/src/lib/pubkey/ec_group/ec_inner_data.h +++ b/src/lib/pubkey/ec_group/ec_inner_data.h @@ -139,6 +139,8 @@ class EC_Group_Data final : public std::enable_shared_from_this { 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; } @@ -231,6 +233,7 @@ class EC_Group_Data final : public std::enable_shared_from_this { 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; };