Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

secp256r1 utils and is_valid_p256_signature #988

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Sending transactions section in account docs (#981)
- before_update and after_update hooks to ERC721Component (#978)
- before_update and after_update hooks to ERC1155Component (#982)
- secp256r1 utils and is_valid_p256_signature (#988)

### Changed (Breaking)

Expand Down
2 changes: 2 additions & 0 deletions _typos.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[files]
extend-exclude = ["test_p256_account.cairo"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we excluding this file?

Copy link
Author

@0xknwn 0xknwn May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because your tool that checks for syntax consider I do spell some hexa value wrong and I do not know any other ways to pass the associated test. see https://github.com/OpenZeppelin/cairo-contracts/actions/runs/9023095283/job/24794108410 for details. What is a better way to fix it?

1 change: 1 addition & 0 deletions src/account/interface.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use starknet::ContractAddress;
use starknet::account::Call;

type EthPublicKey = starknet::secp256k1::Secp256k1Point;
type P256PublicKey = starknet::secp256r1::Secp256r1Point;

const ISRC6_ID: felt252 = 0x2ceccef7f994940b3962a6c67e0ba4fcd37df7d131417c604f91e03caecc1cd;

Expand Down
3 changes: 2 additions & 1 deletion src/account/utils.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@
// OpenZeppelin Contracts for Cairo v0.13.0 (account/utils.cairo)

mod secp256k1;
mod secp256r1;
mod signature;

use signature::{is_valid_stark_signature, is_valid_eth_signature};
use signature::{is_valid_stark_signature, is_valid_eth_signature, is_valid_p256_signature};
use starknet::SyscallResultTrait;
use starknet::account::Call;

Expand Down
69 changes: 69 additions & 0 deletions src/account/utils/secp256r1.cairo
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// SPDX-License-Identifier: MIT
// OpenZeppelin Contracts for Cairo v0.13.0 (account/utils/secp256r1.cairo)

use core::fmt::{Debug, Formatter, Error};
use starknet::SyscallResultTrait;
use starknet::secp256_trait::Secp256PointTrait;
use starknet::secp256r1::{
Secp256r1Point, secp256r1_get_point_from_x_syscall, secp256r1_new_syscall
};

/// Packs a Secp256r1Point into a (felt252, felt252).
///
/// The packing is done as follows:
/// - First felt contains x.low (x being the x-coordinate of the point).
/// - Second felt contains x.high and the parity bit, at the least significant bits (2 * x.high + parity).
impl Secp256r1PointStorePacking of starknet::StorePacking<Secp256r1Point, (felt252, felt252)> {
fn pack(value: Secp256r1Point) -> (felt252, felt252) {
let (x, y) = value.get_coordinates().unwrap_syscall();

let parity = y % 2;
let xhigh_and_parity = 2 * x.high.into() + parity.try_into().unwrap();

(x.low.into(), xhigh_and_parity)
}

fn unpack(value: (felt252, felt252)) -> Secp256r1Point {
let (xlow, xhigh_and_parity) = value;
let xhigh_and_parity: u256 = xhigh_and_parity.into();

let x = u256 {
low: xlow.try_into().unwrap(), high: (xhigh_and_parity / 2).try_into().unwrap(),
};
let parity = xhigh_and_parity % 2 == 1;

// Expects parity odd to be true
secp256r1_get_point_from_x_syscall(x, parity)
.unwrap_syscall()
.expect('Secp256r1Point: Invalid point.')
}
}

impl Secp256r1PointSerde of Serde<Secp256r1Point> {
fn serialize(self: @Secp256r1Point, ref output: Array<felt252>) {
let point = (*self).get_coordinates().unwrap_syscall();
point.serialize(ref output)
}
fn deserialize(ref serialized: Span<felt252>) -> Option<Secp256r1Point> {
let (x, y) = Serde::<(u256, u256)>::deserialize(ref serialized)?;
secp256r1_new_syscall(x, y).unwrap_syscall()
}
}

impl Secp256r1PointPartialEq of PartialEq<Secp256r1Point> {
#[inline(always)]
fn eq(lhs: @Secp256r1Point, rhs: @Secp256r1Point) -> bool {
(*lhs).get_coordinates().unwrap_syscall() == (*rhs).get_coordinates().unwrap_syscall()
}
#[inline(always)]
fn ne(lhs: @Secp256r1Point, rhs: @Secp256r1Point) -> bool {
!(lhs == rhs)
}
}

impl DebugSecp256r1Point of core::fmt::Debug<Secp256r1Point> {
fn fmt(self: @Secp256r1Point, ref f: Formatter) -> Result<(), Error> {
let (x, y) = (*self).get_coordinates().unwrap_syscall();
write!(f, "({x:?},{y:?})")
}
}
23 changes: 22 additions & 1 deletion src/account/utils/signature.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// OpenZeppelin Contracts for Cairo v0.13.0 (account/utils/signature.cairo)

use ecdsa::check_ecdsa_signature;
use openzeppelin::account::interface::EthPublicKey;
use openzeppelin::account::interface::{EthPublicKey, P256PublicKey};
use starknet::secp256_trait;

#[derive(Copy, Drop, Serde)]
Expand All @@ -11,6 +11,12 @@ pub struct EthSignature {
pub s: u256,
}

#[derive(Copy, Drop, Serde)]
pub struct P256Signature {
pub r: u256,
pub s: u256,
}

/// This function assumes the `s` component of the signature to be positive
/// for efficiency reasons. It is not recommended to use it other than for
/// validating account signatures over transaction hashes since otherwise
Expand Down Expand Up @@ -42,3 +48,18 @@ fn is_valid_eth_signature(

secp256_trait::is_valid_signature(msg_hash.into(), signature.r, signature.s, public_key)
}

/// This function assumes the `s` component of the signature to be positive
/// for efficiency reasons. It is not recommended to use it other than for
/// validating account signatures over transaction hashes since otherwise
/// it's not protected against signature malleability.
/// See https://github.com/OpenZeppelin/cairo-contracts/issues/889.
fn is_valid_p256_signature(
msg_hash: felt252, public_key: P256PublicKey, signature: Span<felt252>
) -> bool {
let mut signature = signature;
let signature: P256Signature = Serde::deserialize(ref signature)
.expect('Signature: Invalid format.');

secp256_trait::is_valid_signature(msg_hash.into(), signature.r, signature.s, public_key)
}
2 changes: 2 additions & 0 deletions src/tests/account.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,7 @@ mod test_account;
mod test_dual_account;
mod test_dual_eth_account;
mod test_eth_account;
mod test_p256_account;
mod test_secp256k1;
mod test_secp256r1;
mod test_signature;
29 changes: 29 additions & 0 deletions src/tests/account/test_p256_account.cairo
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
use openzeppelin::account::interface::P256PublicKey;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't have this account in the library yet, I would move this into the test_secp256r1 test file.

Copy link
Author

@0xknwn 0xknwn May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do not but we do, see https://github.com/0xknwn/starknet-modular-account/blob/749a0485b0f50b9350f8f41eb1898e88f2f3a195/src/modules/p256validator/p256validator.cairo#L17 . The reason behind this PR is because we use your project as a library to develop a modular account. It makes sense that we use the same tool for secp256k1 and secp256r1 and that you provide the technology. For that purpose, it really makes sense types are kept in files that are not test and Eth and P256 public keys are in a parallel location

Copy link
Member

@ericnordelo ericnordelo May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I'm not recommending to move the type though, but the contents of this file (to move test_p256_account.cairo contents into test_secp256r1.cairo). Sorry if it was a bit confusing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I will do!

use openzeppelin::account::utils::signature::P256Signature;
use starknet::secp256r1::secp256r1_new_syscall;

#[derive(Drop)]
struct SignedTransactionData {
private_key: u256,
public_key: P256PublicKey,
transaction_hash: felt252,
signature: P256Signature
}

/// This signature was computed using @noble/curves.
fn SIGNED_TX_DATA() -> SignedTransactionData {
SignedTransactionData {
private_key: 0x1efecf7ee1e25bb87098baf2aaab0406167aae0d5ea9ba0d31404bf01886bd0e,
public_key: secp256r1_new_syscall(
0x097420e05fbc83afe4d73b31890187d0cacf2c3653e27f434701a91625f916c2,
0x98a304ff544db99c864308a9b3432324adc6c792181bae33fe7a4cbd48cf263a
)
.unwrap()
.unwrap(),
transaction_hash: 0x1e0cb9e0eb2a8b414df99964673bd493b594c4a627ab031c150ffc81b330706,
signature: P256Signature {
r: 0xfe4e53a283f4715bba1969dff40227c2ca24a6321a89a02e37a0b830c1a0918e,
s: 0x52257a68cfe886341cfaf23841f744230f2af8dadf8bee2e6560c6bbfed8f28f,
}
}
}
141 changes: 141 additions & 0 deletions src/tests/account/test_secp256r1.cairo
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
use openzeppelin::account::utils::secp256r1::{
SyscallResultTrait, Secp256r1Point, DebugSecp256r1Point, Secp256r1PointSerde,
Secp256r1PointPartialEq, Secp256r1PointStorePacking as StorePacking, secp256r1_new_syscall
};
use starknet::secp256_trait::Secp256PointTrait;
use starknet::secp256r1::Secp256r1Impl;

#[test]
fn test_pack_big_secp256r1_points() {
let (big_point_1, big_point_2) = get_points();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these points big points? The idea of this test is to check that the packing is right even for big values, maybe it makes sense to use just big values even if they are not valid points.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you define big points or point me in the right direction so that I can build values that would make sense for that test?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A point with x big enough for the high part of the u256 to use as many bits as possible would be the best case, but making sure that at least the high part is not 0 should suffice.

let private_key = P256_PRIVATEKEY_SAMPLE();

// Check point 1

let (xlow, xhigh_and_parity) = StorePacking::pack(big_point_1);
let xhigh_and_parity: u256 = xhigh_and_parity.into();

let x = u256 {
low: xlow.try_into().unwrap(), high: (xhigh_and_parity / 2).try_into().unwrap()
};
let parity = xhigh_and_parity % 2 == 1;

assert_eq!(x, private_key);
assert_eq!(parity, true, "Parity should be odd");

// Check point 2

let (xlow, xhigh_and_parity) = StorePacking::pack(big_point_2);
let xhigh_and_parity: u256 = xhigh_and_parity.into();

let x = u256 {
low: xlow.try_into().unwrap(), high: (xhigh_and_parity / 2).try_into().unwrap()
};
let parity = xhigh_and_parity % 2 == 1;

assert_eq!(x, private_key);
assert_eq!(parity, false, "Parity should be even");
}

#[test]
fn test_unpack_big_secp256r1_points() {
let (big_point_1, big_point_2) = get_points();

// Check point 1

let (expected_x, expected_y) = big_point_1.get_coordinates().unwrap_syscall();

let (xlow, xhigh_and_parity) = StorePacking::pack(big_point_1);
let (x, y) = StorePacking::unpack((xlow, xhigh_and_parity)).get_coordinates().unwrap_syscall();

assert_eq!(x, expected_x);
assert_eq!(y, expected_y);

// Check point 2

let (expected_x, _) = big_point_2.get_coordinates().unwrap_syscall();

let (xlow, xhigh_and_parity) = StorePacking::pack(big_point_2);
let (x, _) = StorePacking::unpack((xlow, xhigh_and_parity)).get_coordinates().unwrap_syscall();

assert_eq!(x, expected_x);
assert_eq!(y, expected_y);
}

#[test]
fn test_secp256r1_serialization() {
let (big_point_1, big_point_2) = get_points();

let mut serialized_point = array![];
let mut expected_serialization = array![];

// Check point 1

big_point_1.serialize(ref serialized_point);
big_point_1.get_coordinates().unwrap_syscall().serialize(ref expected_serialization);

assert!(serialized_point == expected_serialization);

// Check point 2

big_point_2.serialize(ref serialized_point);
big_point_2.get_coordinates().unwrap_syscall().serialize(ref expected_serialization);

assert!(serialized_point == expected_serialization);
}

#[test]
fn test_secp256r1_deserialization() {
let (big_point_1, big_point_2) = get_points();

// Check point 1

let mut expected_serialization = array![];

big_point_1.get_coordinates().unwrap_syscall().serialize(ref expected_serialization);
let mut expected_serialization = expected_serialization.span();
let deserialized_point = Secp256r1PointSerde::deserialize(ref expected_serialization).unwrap();

assert_eq!(big_point_1, deserialized_point);

// Check point 2

let mut expected_serialization = array![];

big_point_2.get_coordinates().unwrap_syscall().serialize(ref expected_serialization);
let mut expected_serialization = expected_serialization.span();
let deserialized_point = Secp256r1PointSerde::deserialize(ref expected_serialization).unwrap();

assert_eq!(big_point_2, deserialized_point);
}

#[test]
fn test_partial_eq() {
let (big_point_1, big_point_2) = get_points();

assert_eq!(big_point_1, big_point_1);
assert_eq!(big_point_2, big_point_2);
assert!(big_point_1 != big_point_2);
assert!(big_point_2 != big_point_1);
}

//
// Helpers
//

/// This signature was computed using @noble/curves.
fn P256_PRIVATEKEY_SAMPLE() -> u256 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are not verifying signatures for this test, Is this function needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment my next comment. I will remove it, once we agree on what to name it.

0x1efecf7ee1e25bb87098baf2aaab0406167aae0d5ea9ba0d31404bf01886bd0e
}

fn get_points() -> (Secp256r1Point, Secp256r1Point) {
let private_key = P256_PRIVATEKEY_SAMPLE();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need x to be a private key? The name seems a bit misleading.

Copy link
Author

@0xknwn 0xknwn May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can replace the x value on the curve by a const and remove the function. Whatever you want but tell me what you prefer otherwise we may spin around:

  • do you want a const or simply the value?
  • if you prefer a const or a function, how would you like I name it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, I think the value is okay.

let point_1 = Secp256r1Impl::secp256_ec_get_point_from_x_syscall(private_key, true)
.unwrap_syscall()
.unwrap();
let point_2 = Secp256r1Impl::secp256_ec_get_point_from_x_syscall(private_key, false)
.unwrap_syscall()
.unwrap();

(point_1, point_2)
}
Loading
Loading