Skip to content
Merged
22 changes: 22 additions & 0 deletions crypto/bls/src/generic_secret_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ where
GenericPublicKey::from_point(self.point.public_key())
}

/// Returns a reference to the underlying BLS point.
pub fn point(&self) -> &Sec {
&self.point
}

/// Serialize `self` as compressed bytes.
///
/// ## Note
Expand Down Expand Up @@ -89,3 +94,20 @@ where
}
}
}

impl<Sig, Pub, Sec> GenericSecretKey<Sig, Pub, Sec>
where
Sig: TSignature<Pub>,
Pub: TPublicKey,
Sec: TSecretKey<Sig, Pub> + Clone,
{
/// Instantiates `Self` from a `point`.
/// Takes a reference, as moves might accidentally leave behind key material
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on this? Is it because zeroize only happens on drop, and a move is implemented as a memcpy without a drop?

Copy link
Member Author

@dknopik dknopik Feb 18, 2025

Choose a reason for hiding this comment

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

Exactly.

Cloning here ensures that we have two owned blst::min_pk::SecretKey, which will be zeroed each on drop. Moving may or may not do a memcpy (up to rustc to inline or do other optimisations), and may cause data to be left behind.

See also: https://docs.rs/zeroize/latest/zeroize/index.html#stackheap-zeroing-notes

Copy link
Member

Choose a reason for hiding this comment

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

I suspect there are other places we've run afoul of this. Good to know.

I wonder how invasive it would be to use Pin on secret keys. Might be an exploration for another day

pub fn from_point(point: &Sec) -> Self {
Self {
point: point.clone(),
_phantom_signature: PhantomData,
_phantom_public_key: PhantomData,
}
}
}
41 changes: 39 additions & 2 deletions crypto/bls/src/generic_signature.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ use tree_hash::TreeHash;
/// The byte-length of a BLS signature when serialized in compressed form.
pub const SIGNATURE_BYTES_LEN: usize = 96;

/// The byte-length of a BLS signature when serialized in uncompressed form.
pub const SIGNATURE_UNCOMPRESSED_BYTES_LEN: usize = 192;

/// Represents the signature at infinity.
pub const INFINITY_SIGNATURE: [u8; SIGNATURE_BYTES_LEN] = [
0xc0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
Expand All @@ -22,6 +25,16 @@ pub const INFINITY_SIGNATURE: [u8; SIGNATURE_BYTES_LEN] = [
0,
];

pub const INFINITY_SIGNATURE_UNCOMPRESSED: [u8; SIGNATURE_UNCOMPRESSED_BYTES_LEN] = [
0x40, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0,
];

/// The compressed bytes used to represent `GenericSignature::empty()`.
pub const NONE_SIGNATURE: [u8; SIGNATURE_BYTES_LEN] = [0; SIGNATURE_BYTES_LEN];

Expand All @@ -31,9 +44,15 @@ pub trait TSignature<GenericPublicKey>: Sized + Clone {
/// Serialize `self` as compressed bytes.
fn serialize(&self) -> [u8; SIGNATURE_BYTES_LEN];

/// Serialize `self` as uncompressed bytes.
fn serialize_uncompressed(&self) -> [u8; SIGNATURE_UNCOMPRESSED_BYTES_LEN];

/// Deserialize `self` from compressed bytes.
fn deserialize(bytes: &[u8]) -> Result<Self, Error>;

/// Serialize `self` from uncompressed bytes.
fn deserialize_uncompressed(bytes: &[u8]) -> Result<Self, Error>;

/// Returns `true` if `self` is a signature across `msg` by `pubkey`.
fn verify(&self, pubkey: &GenericPublicKey, msg: Hash256) -> bool;
}
Expand Down Expand Up @@ -93,12 +112,12 @@ where
}

/// Returns a reference to the underlying BLS point.
pub(crate) fn point(&self) -> Option<&Sig> {
pub fn point(&self) -> Option<&Sig> {
self.point.as_ref()
}

/// Instantiates `Self` from a `point`.
pub(crate) fn from_point(point: Sig, is_infinity: bool) -> Self {
pub fn from_point(point: Sig, is_infinity: bool) -> Self {
Self {
point: Some(point),
is_infinity,
Expand All @@ -115,6 +134,13 @@ where
}
}

/// Serialize `self` as compressed bytes.
pub fn serialize_uncompressed(&self) -> Option<[u8; SIGNATURE_UNCOMPRESSED_BYTES_LEN]> {
self.point
.as_ref()
.map(|point| point.serialize_uncompressed())
}

/// Deserialize `self` from compressed bytes.
pub fn deserialize(bytes: &[u8]) -> Result<Self, Error> {
let point = if bytes == &NONE_SIGNATURE[..] {
Expand All @@ -129,6 +155,17 @@ where
_phantom: PhantomData,
})
}

/// Deserialize `self` from uncompressed bytes.
pub fn deserialize_uncompressed(bytes: &[u8]) -> Result<Self, Error> {
// The "none signature" is a beacon chain concept. As we never directly deal with
// uncompressed signatures on the beacon chain, it does not apply here.
Ok(Self {
point: Some(Sig::deserialize_uncompressed(bytes)?),
is_infinity: bytes == &INFINITY_SIGNATURE_UNCOMPRESSED[..],
_phantom: PhantomData,
})
}
}

impl<Pub, Sig> GenericSignature<Pub, Sig>
Expand Down
10 changes: 9 additions & 1 deletion crypto/bls/src/impls/blst.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::{
GenericPublicKey, TPublicKey, PUBLIC_KEY_BYTES_LEN, PUBLIC_KEY_UNCOMPRESSED_BYTES_LEN,
},
generic_secret_key::TSecretKey,
generic_signature::{TSignature, SIGNATURE_BYTES_LEN},
generic_signature::{TSignature, SIGNATURE_BYTES_LEN, SIGNATURE_UNCOMPRESSED_BYTES_LEN},
BlstError, Error, Hash256, ZeroizeHash, INFINITY_SIGNATURE,
};
pub use blst::min_pk as blst_core;
Expand Down Expand Up @@ -189,10 +189,18 @@ impl TSignature<blst_core::PublicKey> for blst_core::Signature {
self.to_bytes()
}

fn serialize_uncompressed(&self) -> [u8; SIGNATURE_UNCOMPRESSED_BYTES_LEN] {
self.serialize()
}

fn deserialize(bytes: &[u8]) -> Result<Self, Error> {
Self::from_bytes(bytes).map_err(Into::into)
}

fn deserialize_uncompressed(bytes: &[u8]) -> Result<Self, Error> {
Self::deserialize(bytes).map_err(Into::into)
}

fn verify(&self, pubkey: &blst_core::PublicKey, msg: Hash256) -> bool {
// Public keys have already been checked for subgroup and infinity
// Check Signature inside function for subgroup
Expand Down
12 changes: 11 additions & 1 deletion crypto/bls/src/impls/fake_crypto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::{
GenericPublicKey, TPublicKey, PUBLIC_KEY_BYTES_LEN, PUBLIC_KEY_UNCOMPRESSED_BYTES_LEN,
},
generic_secret_key::{TSecretKey, SECRET_KEY_BYTES_LEN},
generic_signature::{TSignature, SIGNATURE_BYTES_LEN},
generic_signature::{TSignature, SIGNATURE_BYTES_LEN, SIGNATURE_UNCOMPRESSED_BYTES_LEN},
Error, Hash256, ZeroizeHash, INFINITY_PUBLIC_KEY, INFINITY_SIGNATURE,
};

Expand Down Expand Up @@ -106,12 +106,22 @@ impl TSignature<PublicKey> for Signature {
self.0
}

fn serialize_uncompressed(&self) -> [u8; SIGNATURE_UNCOMPRESSED_BYTES_LEN] {
let mut ret = [0; SIGNATURE_UNCOMPRESSED_BYTES_LEN];
ret[0..SIGNATURE_BYTES_LEN].copy_from_slice(&self.0);
ret
}

fn deserialize(bytes: &[u8]) -> Result<Self, Error> {
let mut signature = Self::infinity();
signature.0[..].copy_from_slice(&bytes[0..SIGNATURE_BYTES_LEN]);
Ok(signature)
}

fn deserialize_uncompressed(bytes: &[u8]) -> Result<Self, Error> {
Self::deserialize(bytes)
}

fn verify(&self, _pubkey: &PublicKey, _msg: Hash256) -> bool {
true
}
Expand Down
5 changes: 4 additions & 1 deletion crypto/bls/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,10 @@ pub use generic_public_key::{
INFINITY_PUBLIC_KEY, PUBLIC_KEY_BYTES_LEN, PUBLIC_KEY_UNCOMPRESSED_BYTES_LEN,
};
pub use generic_secret_key::SECRET_KEY_BYTES_LEN;
pub use generic_signature::{INFINITY_SIGNATURE, SIGNATURE_BYTES_LEN};
pub use generic_signature::{
INFINITY_SIGNATURE, INFINITY_SIGNATURE_UNCOMPRESSED, SIGNATURE_BYTES_LEN,
SIGNATURE_UNCOMPRESSED_BYTES_LEN,
};
pub use get_withdrawal_credentials::get_withdrawal_credentials;
pub use zeroize_hash::ZeroizeHash;

Expand Down
17 changes: 16 additions & 1 deletion crypto/bls/tests/tests.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
use bls::{FixedBytesExtended, Hash256, INFINITY_SIGNATURE, SECRET_KEY_BYTES_LEN};
use bls::{
FixedBytesExtended, Hash256, INFINITY_SIGNATURE, INFINITY_SIGNATURE_UNCOMPRESSED,
SECRET_KEY_BYTES_LEN,
};
use ssz::{Decode, Encode};
use std::borrow::Cow;
use std::fmt::Debug;
Expand Down Expand Up @@ -37,6 +40,18 @@ macro_rules! test_suite {
assert!(AggregateSignature::infinity().is_infinity());
}

#[test]
fn infinity_sig_serializations_match() {
let sig = Signature::deserialize(&INFINITY_SIGNATURE).unwrap();
assert_eq!(
sig.serialize_uncompressed().unwrap(),
INFINITY_SIGNATURE_UNCOMPRESSED
);
let sig =
Signature::deserialize_uncompressed(&INFINITY_SIGNATURE_UNCOMPRESSED).unwrap();
assert_eq!(sig.serialize(), INFINITY_SIGNATURE);
}

#[test]
fn ssz_round_trip_multiple_types() {
let mut agg_sig = AggregateSignature::infinity();
Expand Down