Merge pull request #1669 from o1-labs/fix/ecdsa-fix

Revert ECDSA hash/packing logic to previous iteration
o1-labs · Jun 4, 2024 · 5687030 · 5687030
2 parents 8758daa + 39df06e
commit 5687030
Show file tree

Hide file tree

Showing 7 changed files with 213 additions and 67 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -28,12 +28,14 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
   - This can pose an attack surface, since it is easy to maliciously pick either the `+0` or the `-0` representation
   - Use `Int64.isPositiveV2()` and `Int64.modV2()` instead
   - Also deprecated `Int64.neg()` in favor of `Int64.negV2()`, for compatibility with v2 version of `Int64` that will use `Int64.checkV2()`
+- `Ecdsa.verify()` and `Ecdsa.verifySignedHash()` deprecated in favor of `Ecdsa.verifyV2()` and `Ecdsa.verifySignedHashV2()` due to a security vulnerability found in the current implementation https://github.com/o1-labs/o1js/pull/1669
 
 ## [1.3.0](https://github.com/o1-labs/o1js/compare/6a1012162...54d6545bf)
 
 ### Added
 
 - Added `base64Encode()` and `base64Decode(byteLength)` methods to the `Bytes` class. https://github.com/o1-labs/o1js/pull/1659
+- Added `Ecdsa.verifyV2()` and `Ecdsa.verifySignedHashV2` methods to the `Ecdsa` class. https://github.com/o1-labs/o1js/pull/1669
 
 ### Fixes
 

diff --git a/src/examples/crypto/ecdsa/ecdsa.ts b/src/examples/crypto/ecdsa/ecdsa.ts
@@ -23,7 +23,7 @@ const keccakAndEcdsa = ZkProgram({
     verifyEcdsa: {
       privateInputs: [Ecdsa.provable, Secp256k1.provable],
       async method(message: Bytes32, signature: Ecdsa, publicKey: Secp256k1) {
-        return signature.verify(message, publicKey);
+        return signature.verifyV2(message, publicKey);
       },
     },
   },
@@ -38,7 +38,7 @@ const ecdsa = ZkProgram({
     verifySignedHash: {
       privateInputs: [Ecdsa.provable, Secp256k1.provable],
       async method(message: Scalar, signature: Ecdsa, publicKey: Secp256k1) {
-        return signature.verifySignedHash(message, publicKey);
+        return signature.verifySignedHashV2(message, publicKey);
       },
     },
   },

diff --git a/src/lib/provable/crypto/foreign-ecdsa.ts b/src/lib/provable/crypto/foreign-ecdsa.ts
@@ -66,6 +66,15 @@ class EcdsaSignature {
     return { r: this.r.toBigInt(), s: this.s.toBigInt() };
   }
 
+  /**
+   * @deprecated There is a security vulnerability in this method. Use {@link verifyV2} instead.
+   */
+  verify(message: Bytes, publicKey: FlexiblePoint) {
+    let msgHashBytes = Keccak.ethereum(message);
+    let msgHash = keccakOutputToScalar(msgHashBytes, this.Constructor.Curve);
+    return this.verifySignedHash(msgHash, publicKey);
+  }
+
   /**
    * Verify the ECDSA signature given the message (an array of bytes) and public key (a {@link Curve} point).
    *
@@ -100,10 +109,27 @@ class EcdsaSignature {
    * isValid.assertTrue('signature verifies');
    * ```
    */
-  verify(message: Bytes, publicKey: FlexiblePoint) {
+  verifyV2(message: Bytes, publicKey: FlexiblePoint) {
     let msgHashBytes = Keccak.ethereum(message);
     let msgHash = keccakOutputToScalar(msgHashBytes, this.Constructor.Curve);
-    return this.verifySignedHash(msgHash, publicKey);
+    return this.verifySignedHashV2(msgHash, publicKey);
+  }
+
+  /**
+   * @deprecated There is a security vulnerability in this method. Use {@link verifySignedHashV2} instead.
+   */
+  verifySignedHash(
+    msgHash: AlmostForeignField | bigint,
+    publicKey: FlexiblePoint
+  ) {
+    let msgHash_ = this.Constructor.Curve.Scalar.from(msgHash);
+    let publicKey_ = this.Constructor.Curve.from(publicKey);
+    return Ecdsa.verify(
+      this.Constructor.Curve.Bigint,
+      toObject(this),
+      msgHash_.value,
+      toPoint(publicKey_)
+    );
   }
 
   /**
@@ -113,13 +139,13 @@ class EcdsaSignature {
    * In contrast, this method just takes the message hash (a curve scalar) as input, giving you flexibility in
    * choosing the hashing algorithm.
    */
-  verifySignedHash(
+  verifySignedHashV2(
     msgHash: AlmostForeignField | bigint,
     publicKey: FlexiblePoint
   ) {
     let msgHash_ = this.Constructor.Curve.Scalar.from(msgHash);
     let publicKey_ = this.Constructor.Curve.from(publicKey);
-    return Ecdsa.verify(
+    return Ecdsa.verifyV2(
       this.Constructor.Curve.Bigint,
       toObject(this),
       msgHash_.value,

diff --git a/src/lib/provable/gadgets/elliptic-curve.ts b/src/lib/provable/gadgets/elliptic-curve.ts
@@ -212,33 +212,32 @@ function equals(p1: Point, p2: point, Curve: { modulus: bigint }) {
   return xEquals.and(yEquals);
 }
 
-/**
- * Verify an ECDSA signature.
- *
- * Details about the `config` parameter:
- * - For both the generator point `G` and public key `P`, `config` allows you to specify:
- *   - the `windowSize` which is used in scalar multiplication for this point.
- *     this flexibility is good because the optimal window size is different for constant and non-constant points.
- *     empirically, `windowSize=4` for constants and 3 for variables leads to the fewest constraints.
- *     our defaults reflect that the generator is always constant and the public key is variable in typical applications.
- *   - a table of multiples of those points, of length `2^windowSize`, which is used in the scalar multiplication gadget to speed up the computation.
- *     if these are not provided, they are computed on the fly.
- *     for the constant G, computing multiples costs no constraints, so passing them in makes no real difference.
- *     for variable public key, there is a possible use case: if the public key is a public input, then its multiples could also be.
- *     in that case, passing them in would avoid computing them in-circuit and save a few constraints.
- * - The initial aggregator `ia`, see {@link initialAggregator}. By default, `ia` is computed deterministically on the fly.
- */
-function verifyEcdsa(
+function verifyEcdsaGeneric(
   Curve: CurveAffine,
   signature: Ecdsa.Signature,
   msgHash: Field3,
   publicKey: Point,
+  multiScalarMul: (
+    scalars: Field3[],
+    points: Point[],
+    Curve: CurveAffine,
+    tableConfigs?: (
+      | {
+          windowSize?: number;
+          multiples?: Point[];
+        }
+      | undefined
+    )[],
+    mode?: 'assert-nonzero' | 'assert-zero',
+    ia?: point,
+    hashed?: boolean
+  ) => Point,
   config: {
     G?: { windowSize: number; multiples?: Point[] };
     P?: { windowSize: number; multiples?: Point[] };
     ia?: point;
   } = { G: { windowSize: 4 }, P: { windowSize: 4 } }
-) {
+): Bool {
   // constant case
   if (
     EcdsaSignature.isConstant(signature) &&
@@ -285,6 +284,69 @@ function verifyEcdsa(
   return Provable.equal(Field3.provable, Rx, r);
 }
 
+/**
+ * @deprecated There is a security vulnerability with this function, allowing a prover to modify witness values than what is expected. Use {@link verifyEcdsaV2} instead.
+ */
+function verifyEcdsa(
+  Curve: CurveAffine,
+  signature: Ecdsa.Signature,
+  msgHash: Field3,
+  publicKey: Point,
+  config: {
+    G?: { windowSize: number; multiples?: Point[] };
+    P?: { windowSize: number; multiples?: Point[] };
+    ia?: point;
+  } = { G: { windowSize: 4 }, P: { windowSize: 4 } }
+) {
+  return verifyEcdsaGeneric(
+    Curve,
+    signature,
+    msgHash,
+    publicKey,
+    (scalars, points, Curve, configs, mode, ia) =>
+      multiScalarMul(scalars, points, Curve, configs, mode, ia, true),
+    config
+  );
+}
+
+/**
+ * Verify an ECDSA signature.
+ *
+ * Details about the `config` parameter:
+ * - For both the generator point `G` and public key `P`, `config` allows you to specify:
+ *   - the `windowSize` which is used in scalar multiplication for this point.
+ *     this flexibility is good because the optimal window size is different for constant and non-constant points.
+ *     empirically, `windowSize=4` for constants and 3 for variables leads to the fewest constraints.
+ *     our defaults reflect that the generator is always constant and the public key is variable in typical applications.
+ *   - a table of multiples of those points, of length `2^windowSize`, which is used in the scalar multiplication gadget to speed up the computation.
+ *     if these are not provided, they are computed on the fly.
+ *     for the constant G, computing multiples costs no constraints, so passing them in makes no real difference.
+ *     for variable public key, there is a possible use case: if the public key is a public input, then its multiples could also be.
+ *     in that case, passing them in would avoid computing them in-circuit and save a few constraints.
+ * - The initial aggregator `ia`, see {@link initialAggregator}. By default, `ia` is computed deterministically on the fly.
+ */
+function verifyEcdsaV2(
+  Curve: CurveAffine,
+  signature: Ecdsa.Signature,
+  msgHash: Field3,
+  publicKey: Point,
+  config: {
+    G?: { windowSize: number; multiples?: Point[] };
+    P?: { windowSize: number; multiples?: Point[] };
+    ia?: point;
+  } = { G: { windowSize: 4 }, P: { windowSize: 3 } }
+) {
+  return verifyEcdsaGeneric(
+    Curve,
+    signature,
+    msgHash,
+    publicKey,
+    (scalars, points, Curve, configs, mode, ia) =>
+      multiScalarMul(scalars, points, Curve, configs, mode, ia, false),
+    config
+  );
+}
+
 /**
  * Bigint implementation of ECDSA verify
  */
@@ -311,6 +373,36 @@ function verifyEcdsaConstant(
   return Curve.Scalar.equal(R.x, r);
 }
 
+function multiScalarMulConstant(
+  scalars: Field3[],
+  points: Point[],
+  Curve: CurveAffine,
+  mode: 'assert-nonzero' | 'assert-zero' = 'assert-nonzero'
+): Point {
+  let n = points.length;
+  assert(scalars.length === n, 'Points and scalars lengths must match');
+  assertPositiveInteger(n, 'Expected at least 1 point and scalar');
+  let useGlv = Curve.hasEndomorphism;
+
+  // TODO dedicated MSM
+  let s = scalars.map(Field3.toBigint);
+  let P = points.map(Point.toBigint);
+  let sum = Curve.zero;
+  for (let i = 0; i < n; i++) {
+    if (useGlv) {
+      sum = Curve.add(sum, Curve.Endo.scale(P[i], s[i]));
+    } else {
+      sum = Curve.add(sum, Curve.scale(P[i], s[i]));
+    }
+  }
+  if (mode === 'assert-zero') {
+    assert(sum.infinity, 'scalar multiplication: expected zero result');
+    return Point.from(Curve.zero);
+  }
+  assert(!sum.infinity, 'scalar multiplication: expected non-zero result');
+  return Point.from(sum);
+}
+
 /**
  * Multi-scalar multiplication:
  *
@@ -339,7 +431,8 @@ function multiScalarMul(
     | undefined
   )[] = [],
   mode: 'assert-nonzero' | 'assert-zero' = 'assert-nonzero',
-  ia?: point
+  ia?: point,
+  hashed: boolean = true
 ): Point {
   let n = points.length;
   assert(scalars.length === n, 'Points and scalars lengths must match');
@@ -348,23 +441,7 @@ function multiScalarMul(
 
   // constant case
   if (scalars.every(Field3.isConstant) && points.every(Point.isConstant)) {
-    // TODO dedicated MSM
-    let s = scalars.map(Field3.toBigint);
-    let P = points.map(Point.toBigint);
-    let sum = Curve.zero;
-    for (let i = 0; i < n; i++) {
-      if (useGlv) {
-        sum = Curve.add(sum, Curve.Endo.scale(P[i], s[i]));
-      } else {
-        sum = Curve.add(sum, Curve.scale(P[i], s[i]));
-      }
-    }
-    if (mode === 'assert-zero') {
-      assert(sum.infinity, 'scalar multiplication: expected zero result');
-      return Point.from(Curve.zero);
-    }
-    assert(!sum.infinity, 'scalar multiplication: expected non-zero result');
-    return Point.from(sum);
+    return multiScalarMulConstant(scalars, points, Curve, mode);
   }
 
   // parse or build point tables
@@ -427,31 +504,42 @@ function multiScalarMul(
   // a Point is 6 field elements, the hash is just 1 field element
   const HashedPoint = Hashed.create(Point.provable);
 
-  let hashedTables = tables.map((table) =>
-    table.map((point) => HashedPoint.hash(point))
-  );
-
   // initialize sum to the initial aggregator, which is expected to be unrelated to any point that this gadget is used with
   // note: this is a trick to ensure _completeness_ of the gadget
   // soundness follows because add() and double() are sound, on all inputs that are valid non-zero curve points
   ia ??= initialAggregator(Curve);
   let sum = Point.from(ia);
 
+  let hashedTables: Hashed<Point>[][] = [];
+  if (hashed) {
+    hashedTables = tables.map((table) =>
+      table.map((point) => HashedPoint.hash(point))
+    );
+  }
+
   for (let i = maxBits - 1; i >= 0; i--) {
     // add in multiple of each point
     for (let j = 0; j < n; j++) {
       let windowSize = windowSizes[j];
       if (i % windowSize === 0) {
         // pick point to add based on the scalar chunk
         let sj = scalarChunks[j][i / windowSize];
-        let sjP =
-          windowSize === 1
-            ? points[j]
-            : arrayGetGeneric(
-                HashedPoint.provable,
-                hashedTables[j],
-                sj
-              ).unhash();
+        let sjP;
+        if (hashed) {
+          sjP =
+            windowSize === 1
+              ? points[j]
+              : arrayGetGeneric(
+                  HashedPoint.provable,
+                  hashedTables[j],
+                  sj
+                ).unhash();
+        } else {
+          sjP =
+            windowSize === 1
+              ? points[j]
+              : arrayGetGeneric(Point.provable, tables[j], sj);
+        }
 
         // ec addition
         let added = add(sum, sjP, Curve);
@@ -725,6 +813,7 @@ const EcdsaSignature = {
 const Ecdsa = {
   sign: signEcdsa,
   verify: verifyEcdsa,
+  verifyV2: verifyEcdsaV2,
   Signature: EcdsaSignature,
 };
 

diff --git a/src/lib/provable/test/custom-gates-recursion.unit-test.ts b/src/lib/provable/test/custom-gates-recursion.unit-test.ts
@@ -46,7 +46,7 @@ let program = ZkProgram({
         let msgHash_ = Provable.witness(Field3.provable, () => msgHash);
         let publicKey_ = Provable.witness(Point.provable, () => publicKey);
 
-        return Ecdsa.verify(Secp256k1, signature_, msgHash_, publicKey_);
+        return Ecdsa.verifyV2(Secp256k1, signature_, msgHash_, publicKey_);
       },
     },
   },