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

feat(crypto): CRP-2579 Add support for derivation to ecdsa_secp256r1 crate #1730

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions rs/crypto/ecdsa_secp256r1/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ package(default_visibility = ["//visibility:public"])

DEPENDENCIES = [
# Keep sorted.
"@crate_index//:hmac",
"@crate_index//:lazy_static",
"@crate_index//:num-bigint",
"@crate_index//:p256",
"@crate_index//:pem",
"@crate_index//:rand",
"@crate_index//:rand_chacha",
"@crate_index//:sha2",
"@crate_index//:simple_asn1",
"@crate_index//:zeroize",
]
Expand All @@ -21,6 +23,7 @@ DEV_DEPENDENCIES = [
"//rs/crypto/sha2",
"//rs/crypto/test_utils/reproducible_rng",
"@crate_index//:hex",
"@crate_index//:hex-literal",
"@crate_index//:wycheproof",
]

Expand Down
3 changes: 3 additions & 0 deletions rs/crypto/ecdsa_secp256r1/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,20 @@ documentation.workspace = true
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
hmac = "0.12"
lazy_static = { workspace = true }
num-bigint = { workspace = true }
p256 = { workspace = true }
pem = "1.1.0"
rand = { workspace = true }
rand_chacha = { workspace = true }
sha2 = { workspace = true }
simple_asn1 = { workspace = true }
zeroize = { workspace = true }

[dev-dependencies]
hex = { workspace = true }
hex-literal = "0.4"
ic-crypto-sha2 = { path = "../sha2" }
ic-crypto-test-utils-reproducible-rng = { path = "../test_utils/reproducible_rng" }
wycheproof = { version = "0.6", default-features = false, features = ["ecdsa"] }
219 changes: 217 additions & 2 deletions rs/crypto/ecdsa_secp256r1/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use p256::{
generic_array::{typenum::Unsigned, GenericArray},
Curve,
},
NistP256,
AffinePoint, NistP256, Scalar,
};
use rand::{CryptoRng, RngCore};
use zeroize::ZeroizeOnDrop;
Expand All @@ -35,6 +35,135 @@ lazy_static::lazy_static! {
static ref SECP256R1_OID: simple_asn1::OID = simple_asn1::oid!(1, 2, 840, 10045, 3, 1, 7);
}

/// A component of a derivation path
#[derive(Clone, Debug)]
pub struct DerivationIndex(pub Vec<u8>);

/// Derivation Path
///
/// A derivation path is simply a sequence of DerivationIndex
#[derive(Clone, Debug)]
pub struct DerivationPath {
path: Vec<DerivationIndex>,
}

impl DerivationPath {
/// Create a BIP32-style derivation path
pub fn new_bip32(bip32: &[u32]) -> Self {
Comment on lines +51 to +52
Copy link
Member

Choose a reason for hiding this comment

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

It this intentionally called new_bip32, given that BIP32 isn't defined for secp256r1? IIUC, the derivation is done as per SLIP10, but given that SLIP10 is a generalization of BIP32, it might make sense to call it new_bip32 nevertheless.

let mut path = Vec::with_capacity(bip32.len());
for n in bip32 {
path.push(DerivationIndex(n.to_be_bytes().to_vec()));
}
Self::new(path)
}

/// Create a free-form derivation path
pub fn new(path: Vec<DerivationIndex>) -> Self {
Self { path }
}

/// Create a path from a canister ID and a user provided path
pub fn from_canister_id_and_path(canister_id: &[u8], path: &[Vec<u8>]) -> Self {
let mut vpath = Vec::with_capacity(1 + path.len());
vpath.push(DerivationIndex(canister_id.to_vec()));

for n in path {
vpath.push(DerivationIndex(n.to_vec()));
}
Self::new(vpath)
}

/// Return the length of this path
pub fn len(&self) -> usize {
self.path.len()
}

/// Return if this path is empty
pub fn is_empty(&self) -> bool {
self.len() == 0
}

/// Return the components of the derivation path
pub fn path(&self) -> &[DerivationIndex] {
&self.path
}

fn ckd(idx: &[u8], input: &[u8], chain_code: &[u8; 32]) -> ([u8; 32], Scalar) {
use hmac::{Hmac, Mac};
use p256::elliptic_curve::ops::Reduce;
use sha2::Sha512;

let mut hmac = Hmac::<Sha512>::new_from_slice(chain_code)
.expect("HMAC-SHA-512 should accept 256 bit key");

hmac.update(input);
hmac.update(idx);

let hmac_output: [u8; 64] = hmac.finalize().into_bytes().into();

let fb = p256::FieldBytes::from_slice(&hmac_output[..32]);
let next_offset = <p256::Scalar as Reduce<p256::U256>>::reduce_bytes(fb);
let next_chain_key: [u8; 32] = hmac_output[32..].to_vec().try_into().expect("Correct size");

// If iL >= order, try again with the "next" index as described in SLIP-10
if next_offset.to_bytes().to_vec() != hmac_output[..32] {
let mut next_input = [0u8; 33];
next_input[0] = 0x01;
next_input[1..].copy_from_slice(&next_chain_key);
Self::ckd(idx, &next_input, chain_code)
} else {
(next_chain_key, next_offset)
}
}

fn ckd_pub(
idx: &[u8],
pt: AffinePoint,
chain_code: &[u8; 32],
) -> ([u8; 32], Scalar, AffinePoint) {
use p256::elliptic_curve::{group::GroupEncoding, ops::MulByGenerator};
use p256::ProjectivePoint;

let mut ckd_input = pt.to_bytes();

let pt: ProjectivePoint = pt.into();

loop {
let (next_chain_code, next_offset) = Self::ckd(idx, &ckd_input, chain_code);

let next_pt = (pt + ProjectivePoint::mul_by_generator(&next_offset)).to_affine();

// If the new key is not infinity, we're done: return the new key
if !bool::from(next_pt.is_identity()) {
return (next_chain_code, next_offset, next_pt);
}

// Otherwise set up the next input as defined by SLIP-0010
ckd_input[0] = 0x01;
ckd_input[1..].copy_from_slice(&next_chain_code);
}
}

fn derive_offset(
&self,
pt: AffinePoint,
chain_code: &[u8; 32],
) -> (AffinePoint, Scalar, [u8; 32]) {
let mut offset = Scalar::ZERO;
let mut pt = pt;
let mut chain_code = *chain_code;

for idx in self.path() {
let (next_chain_code, next_offset, next_pt) = Self::ckd_pub(&idx.0, pt, &chain_code);
chain_code = next_chain_code;
pt = next_pt;
offset = offset.add(&next_offset);
}

(pt, offset, chain_code)
}
}

const PEM_HEADER_PKCS8: &str = "PRIVATE KEY";
const PEM_HEADER_RFC5915: &str = "EC PRIVATE KEY";

Expand Down Expand Up @@ -305,6 +434,57 @@ impl PrivateKey {
let key = self.key.verifying_key();
PublicKey { key: *key }
}

/// Derive a private key from this private key using a derivation path
///
/// This is the same derivation system used by the Internet Computer when
/// deriving subkeys for threshold ECDSA with secp256r1
///
/// As long as each index of the derivation path is a 4-byte input with the highest
/// bit cleared, this derivation scheme matches SLIP-10
///
/// See <https://internetcomputer.org/docs/current/references/ic-interface-spec#ic-ecdsa_public_key>
/// for details on the derivation scheme.
Comment on lines +443 to +447
Copy link
Member

Choose a reason for hiding this comment

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

The given URL actually doesn't give any details on the derivation scheme for curves different from secp256k1. How about creating a ticket to improve this (e.g., to mention SLIP-10)?

///
pub fn derive_subkey(&self, derivation_path: &DerivationPath) -> (Self, [u8; 32]) {
let chain_code = [0u8; 32];
self.derive_subkey_with_chain_code(derivation_path, &chain_code)
}

/// Derive a private key from this private key using a derivation path
/// and chain code
///
/// This is the same derivation system used by the Internet Computer when
/// deriving subkeys for threshold ECDSA with secp256r1
///
/// As long as each index of the derivation path is a 4-byte input with the highest
/// bit cleared, this derivation scheme matches SLIP-10
///
/// See <https://internetcomputer.org/docs/current/references/ic-interface-spec#ic-ecdsa_public_key>
/// for details on the derivation scheme.
///
pub fn derive_subkey_with_chain_code(
&self,
derivation_path: &DerivationPath,
chain_code: &[u8; 32],
) -> (Self, [u8; 32]) {
use p256::NonZeroScalar;

let public_key: AffinePoint = self.key.verifying_key().as_affine().clone();
Copy link
Member

Choose a reason for hiding this comment

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

Clippy has a comment here:

error: using `clone` on type `AffinePoint<NistP256>` which implements the `Copy` trait
   --> rs/crypto/ecdsa_secp256r1/src/lib.rs:473:39
    |
473 |         let public_key: AffinePoint = self.key.verifying_key().as_affine().clone();
    |                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try dereferencing it: `*self.key.verifying_key().as_affine()`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
    = note: `-D clippy::clone-on-copy` implied by `-D clippy::all`
    = help: to override `-D clippy::all` add `#[allow(clippy::clone_on_copy)]`

let (_pt, offset, derived_chain_code) =
derivation_path.derive_offset(public_key, chain_code);

let derived_scalar = self.key.as_nonzero_scalar().as_ref().add(&offset);

let nz_ds =
NonZeroScalar::new(derived_scalar).expect("Derivation always produces non-zero sum");

let derived_key = Self {
key: p256::SecretKey::from(nz_ds).into(),
Copy link
Member

Choose a reason for hiding this comment

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

It seems we can also go directly: p256::ecdsa::SigningKey::from(nz_ds)

};

(derived_key, derived_chain_code)
}
}

/// An ECDSA public key
Expand Down Expand Up @@ -374,7 +554,7 @@ impl PublicKey {
/// valid signature, then (r,-s) is also valid. To avoid this malleability,
/// some systems require that s be "normalized" to the smallest value.
///
/// This normalization is quite common on secp256k1, but is virtually
/// This normalization is quite common on secp256r1, but is virtually
Copy link
Member

Choose a reason for hiding this comment

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

Was this change intentional? The text now says: "normalization is common on r1, but is unknown for r1".

/// unknown and unimplemented for secp256r1. The vast majority of secp256r1
/// signatures will not be normalized. Thus this verification *does not*
/// ensure any non-malleability properties.
Expand All @@ -399,4 +579,39 @@ impl PublicKey {

self.key.verify_prehash(digest, &signature).is_ok()
}

/// Derive a public key from this public key using a derivation path
///
/// This is the same derivation system used by the Internet Computer when
/// deriving subkeys for threshold ECDSA with secp256r1
///
pub fn derive_subkey(&self, derivation_path: &DerivationPath) -> (Self, [u8; 32]) {
let chain_code = [0u8; 32];
self.derive_subkey_with_chain_code(derivation_path, &chain_code)
}

/// Derive a public key from this public key using a derivation path
/// and chain code
///
/// This is the same derivation system used by the Internet Computer when
/// deriving subkeys for threshold ECDSA with secp256r1
///
/// This derivation matches SLIP-10
pub fn derive_subkey_with_chain_code(
&self,
derivation_path: &DerivationPath,
chain_code: &[u8; 32],
) -> (Self, [u8; 32]) {
let public_key: AffinePoint = *self.key.as_affine();
let (pt, _offset, chain_code) = derivation_path.derive_offset(public_key, chain_code);

let derived_key = Self {
key: p256::PublicKey::from(
p256::PublicKey::from_affine(pt).expect("Derived point is valid"),
)
.into(),
Comment on lines +609 to +612
Copy link
Member

Choose a reason for hiding this comment

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

It seems one conversion layer is to itself, so it should be obsolete. Also, how about being explicit (instead of doing into). As a result, I suggest we do

            key: p256::ecdsa::VerifyingKey::from(
                p256::PublicKey::from_affine(pt).expect("Derived point is valid"),
            ),

};

(derived_key, chain_code)
}
}
Loading
Loading