Revert ECDSA hash/packing logic to previous iteration #1669
Conversation
MartinMinkov
force-pushed
the
fix/ecdsa-fix
branch
from
1934f02 to
e6b6019
Compare
| * In contrast, this method just takes the message hash (a curve scalar) as input, giving you flexibility in | ||
| * choosing the hashing algorithm. | ||
| */ | ||
| verifySignedHashV2( |
There was a problem hiding this comment.
Was it necessary to add a new verifySignedHashV2 function here? It seems so since the deprecated verify was being used here, but not sure if it's necessary to reflect everywhere.
There was a problem hiding this comment.
yeah thanks for adding this as well!
| * - The initial aggregator `ia`, see {@link initialAggregator}. By default, `ia` is computed deterministically on the fly. | ||
| */ | ||
| function verifyEcdsa( | ||
| function verifyEcdsaGeneric( |
There was a problem hiding this comment.
Refactored some of the duplicated logic between both verify functions and specified behaviour with a hashed parameter, which guards the code that is responsible for the vulnerability
MartinMinkov
force-pushed
the
fix/ecdsa-fix
branch
from
e6b6019 to
c209daa
Compare
| Curve: CurveAffine, | ||
| signature: Ecdsa.Signature, | ||
| msgHash: Field3, | ||
| publicKey: Point, | ||
| multiScalarMul: ( |
There was a problem hiding this comment.
Adds a parameter to specify the version of multiScalarMul we want (passing in hashed as true or false)
MartinMinkov
force-pushed
the
fix/ecdsa-fix
branch
from
c209daa to
844c00a
Compare
… ecdsa.unit-test.ts): replace deprecated verify and verifySignedHash methods with verifyV2 and verifySignedHashV2 to address security vulnerability
…in Ecdsa class due to security vulnerability fix and enhancement
MartinMinkov
force-pushed
the
fix/ecdsa-fix
branch
from
844c00a to
21f6ebd
Compare
| * let isValid = sig.verify(msg, pk); | ||
| * isValid.assertTrue('signature verifies'); | ||
| * ``` | ||
| * @deprecated There is a security vulnerability in this method. Use {@link verifyV2} instead. |
There was a problem hiding this comment.
could you completely remove any other doccomments here except the deprecation notice? we don't want to encourage people to use it
| /** | ||
| * Verify the ECDSA signature given the message hash (a {@link Scalar}) and public key (a {@link Curve} point). | ||
| * | ||
| * This is a building block of {@link EcdsaSignature.verify}, where the input message is also hashed. | ||
| * In contrast, this method just takes the message hash (a curve scalar) as input, giving you flexibility in | ||
| * choosing the hashing algorithm. | ||
| * @deprecated There is a security vulnerability in this method. Use {@link verifySignedHashV2} instead. |
There was a problem hiding this comment.
could you completely remove any other doccomments here except the deprecation notice? we don't want to encourage people to use it
| * 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. | ||
| * @deprecated There is a security vulnerability with this function, allowing a prover to modify witness values than what is expected. Use {@link verifyEcdsaV2} instead. |
There was a problem hiding this comment.
could you completely remove any other doccomments here except the deprecation notice? we don't want to encourage people to use it
| G?: { windowSize: number; multiples?: Point[] }; | ||
| P?: { windowSize: number; multiples?: Point[] }; | ||
| ia?: point; | ||
| } = { G: { windowSize: 4 }, P: { windowSize: 4 } } |
There was a problem hiding this comment.
could you try changing the default back to this?
| } = { G: { windowSize: 4 }, P: { windowSize: 4 } } | |
| } = { G: { windowSize: 4 }, P: { windowSize: 3 } } |
it should use fewer constraints
There was a problem hiding this comment.
Fixed here!
Here is the lower constraints
| * - The initial aggregator `ia`, see {@link initialAggregator}. By default, `ia` is computed deterministically on the fly. | ||
| */ | ||
| function verifyEcdsa( | ||
| function verifyEcdsaGeneric( |
| ? points[j] | ||
| : arrayGetGeneric( | ||
| HashedPoint.provable, | ||
| hashedTables[j], |
There was a problem hiding this comment.
it costs constraints to create the hashedTables (on line 521), which we only need in the deprecated case.
could you also add a check there and make it an empty array when hashed = false?
…ods and their documentation
…object to optimize elliptic curve calculations
…hashed condition to optimize memory usage and improve performance
This PR reverts the changes made in #1376 and #1377 due to a vulnerability found in the ECSDA logic and how a vulnerability can occur when unpacking data in a specific way.