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

Fix BLST bindings: Error handling for infinite values of sigs and vks #2322

Open
wants to merge 9 commits into
base: main
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
2 changes: 1 addition & 1 deletion Cargo.lock

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

2 changes: 1 addition & 1 deletion mithril-stm/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "mithril-stm"
version = "0.3.38"
version = "0.3.39"
edition = { workspace = true }
authors = { workspace = true }
homepage = { workspace = true }
Expand Down
33 changes: 33 additions & 0 deletions mithril-stm/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,14 @@ pub enum MultiSignatureError {
/// At least one signature in the batch is invalid
#[error("One signature in the batch is invalid")]
BatchInvalid,

/// Single signature is the infinity
#[error("Single signature is the infinity")]
SignatureInfinity(Signature),

/// Verification key is the infinity
#[error("Verification key is the infinity")]
VerificationKeyInfinity(Box<VerificationKey>),
}

/// Errors which can be output by Mithril single signature verification.
Expand Down Expand Up @@ -138,6 +146,10 @@ pub enum RegisterError {
#[error("This key has already been registered.")]
KeyRegistered(Box<VerificationKey>),

/// Verification key is the infinity
#[error("Verification key is the infinity")]
VerificationKeyInfinity(Box<VerificationKey>),

/// The supplied key is not valid
#[error("The verification of correctness of the supplied key is invalid.")]
KeyInvalid(Box<VerificationKeyPoP>),
Expand All @@ -159,6 +171,8 @@ impl From<MultiSignatureError> for StmSignatureError {
MultiSignatureError::BatchInvalid => unreachable!(),
MultiSignatureError::KeyInvalid(_) => unreachable!(),
MultiSignatureError::AggregateSignatureInvalid => unreachable!(),
MultiSignatureError::SignatureInfinity(_) => unreachable!(),
MultiSignatureError::VerificationKeyInfinity(_) => unreachable!(),
}
}
}
Expand Down Expand Up @@ -194,6 +208,12 @@ impl<D: Digest + FixedOutput> From<MultiSignatureError> for StmAggregateSignatur
MultiSignatureError::SignatureInvalid(_) => {
Self::CoreVerificationError(CoreVerifierError::from(e))
}
MultiSignatureError::SignatureInfinity(_) => {
Self::CoreVerificationError(CoreVerifierError::from(e))
}
MultiSignatureError::VerificationKeyInfinity(_) => {
Self::CoreVerificationError(CoreVerifierError::from(e))
}
}
}
}
Expand Down Expand Up @@ -230,6 +250,8 @@ impl From<MultiSignatureError> for CoreVerifierError {
MultiSignatureError::SerializationError => unreachable!(),
MultiSignatureError::KeyInvalid(_) => unreachable!(),
MultiSignatureError::SignatureInvalid(_e) => unreachable!(),
MultiSignatureError::SignatureInfinity(_) => unreachable!(),
MultiSignatureError::VerificationKeyInfinity(_) => unreachable!(),
}
}
}
Expand All @@ -245,6 +267,7 @@ impl From<MultiSignatureError> for RegisterError {
match e {
MultiSignatureError::SerializationError => Self::SerializationError,
MultiSignatureError::KeyInvalid(e) => Self::KeyInvalid(e),
MultiSignatureError::VerificationKeyInfinity(e) => Self::VerificationKeyInfinity(e),
_ => unreachable!(),
}
}
Expand All @@ -255,9 +278,19 @@ impl From<MultiSignatureError> for RegisterError {
pub(crate) fn blst_err_to_mithril(
e: BLST_ERROR,
sig: Option<Signature>,
key: Option<VerificationKey>,
) -> Result<(), MultiSignatureError> {
match e {
BLST_ERROR::BLST_SUCCESS => Ok(()),
BLST_ERROR::BLST_PK_IS_INFINITY => {
if let Some(s) = sig {
return Err(MultiSignatureError::SignatureInfinity(s));
}
if let Some(vk) = key {
return Err(MultiSignatureError::VerificationKeyInfinity(Box::new(vk)));
}
Err(MultiSignatureError::SerializationError)
}
BLST_ERROR::BLST_VERIFY_FAIL => {
if let Some(s) = sig {
Err(MultiSignatureError::SignatureInvalid(s))
Expand Down
9 changes: 3 additions & 6 deletions mithril-stm/src/key_reg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,9 @@ impl KeyReg {
/// The function fails when the proof of possession is invalid or when the key is already registered.
pub fn register(&mut self, stake: Stake, pk: VerificationKeyPoP) -> Result<(), RegisterError> {
if let Entry::Vacant(e) = self.keys.entry(pk.vk) {
if pk.check().is_ok() {
e.insert(stake);
return Ok(());
} else {
return Err(RegisterError::KeyInvalid(Box::new(pk)));
}
pk.check()?;
e.insert(stake);
return Ok(());
}
Err(RegisterError::KeyRegistered(Box::new(pk.vk)))
}
Expand Down
135 changes: 104 additions & 31 deletions mithril-stm/src/multi_sig.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ impl SigningKey {
pub fn from_bytes(bytes: &[u8]) -> Result<Self, MultiSignatureError> {
match BlstSk::from_bytes(&bytes[..32]) {
Ok(sk) => Ok(Self(sk)),
Err(e) => Err(blst_err_to_mithril(e, None)
Err(e) => Err(blst_err_to_mithril(e, None, None)
.expect_err("If deserialization is not successful, blst returns and error different to SUCCESS."))
}
}
Expand All @@ -108,7 +108,7 @@ impl VerificationKey {
pub fn from_bytes(bytes: &[u8]) -> Result<Self, MultiSignatureError> {
match BlstVk::key_validate(&bytes[..96]) {
Ok(vk) => Ok(Self(vk)),
Err(e) => Err(blst_err_to_mithril(e, None)
Err(e) => Err(blst_err_to_mithril(e, None, None)
.expect_err("If deserialization is not successful, blst returns and error different to SUCCESS."))
}
}
Expand Down Expand Up @@ -196,15 +196,19 @@ impl VerificationKeyPoP {
// If we are really looking for performance improvements, we can combine the
// two final exponentiations (for verifying k1 and k2) into a single one.
pub fn check(&self) -> Result<(), MultiSignatureError> {
let result = verify_pairing(&self.vk, &self.pop);

if !(self.pop.k1.verify(false, POP, &[], &[], &self.vk.0, false)
== BLST_ERROR::BLST_SUCCESS
&& result)
{
return Err(MultiSignatureError::KeyInvalid(Box::new(*self)));
match self.vk.0.validate() {
Ok(_) => {
let result = verify_pairing(&self.vk, &self.pop);
if !(self.pop.k1.verify(false, POP, &[], &[], &self.vk.0, false)
== BLST_ERROR::BLST_SUCCESS
&& result)
{
return Err(MultiSignatureError::KeyInvalid(Box::new(*self)));
}
Ok(())
}
Err(e) => blst_err_to_mithril(e, None, Some(self.vk)),
}
Ok(())
}

/// Convert to a 144 byte string.
Expand Down Expand Up @@ -261,7 +265,7 @@ impl ProofOfPossession {
let k1 = match BlstSig::from_bytes(&bytes[..48]) {
Ok(key) => key,
Err(e) => {
return Err(blst_err_to_mithril(e, None)
return Err(blst_err_to_mithril(e, None, None)
.expect_err("If it passed, blst returns and error different to SUCCESS."))
}
};
Expand All @@ -287,10 +291,14 @@ impl From<&SigningKey> for ProofOfPossession {
impl Signature {
/// Verify a signature against a verification key.
pub fn verify(&self, msg: &[u8], mvk: &VerificationKey) -> Result<(), MultiSignatureError> {
blst_err_to_mithril(
self.0.verify(false, msg, &[], &[], &mvk.0, false),
Some(*self),
)
match self.0.validate(true) {
Ok(_) => blst_err_to_mithril(
self.0.verify(false, msg, &[], &[], &mvk.0, false),
Some(*self),
None,
),
Err(e) => blst_err_to_mithril(e, Some(*self), None),
}
Comment on lines +294 to +301
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this would be more readable:

Suggested change
match self.0.validate(true) {
Ok(_) => blst_err_to_mithril(
self.0.verify(false, msg, &[], &[], &mvk.0, false),
Some(*self),
None,
),
Err(e) => blst_err_to_mithril(e, Some(*self), None),
}
blst_err_to_mithril(
self.0.validate(true).map_or_else(
|e| e,
|_| self.0.verify(false, msg, &[], &[], &mvk.0, false),
),
Some(*self),
None,
)

}

/// Dense mapping function indexed by the index to be evaluated.
Expand Down Expand Up @@ -323,7 +331,7 @@ impl Signature {
pub fn from_bytes(bytes: &[u8]) -> Result<Self, MultiSignatureError> {
match BlstSig::sig_validate(&bytes[..48], true) {
Ok(sig) => Ok(Self(sig)),
Err(e) => Err(blst_err_to_mithril(e, None)
Err(e) => Err(blst_err_to_mithril(e, None, None)
.expect_err("If deserialization is not successful, blst returns and error different to SUCCESS."))
}
}
Expand Down Expand Up @@ -376,7 +384,6 @@ impl Signature {
}

let transmuted_vks: Vec<blst_p2> = vks.iter().map(vk_from_p2_affine).collect();

let transmuted_sigs: Vec<blst_p1> = signatures.iter().map(sig_to_p1).collect();

let grouped_vks = p2_affines::from(transmuted_vks.as_slice());
Expand All @@ -400,6 +407,7 @@ impl Signature {
blst_err_to_mithril(
aggr_sig.0.verify(false, msg, &[], &[], &aggr_vk.0, false),
Some(aggr_sig),
None,
)
}

Expand All @@ -415,7 +423,7 @@ impl Signature {
false,
) {
Ok(sig) => BlstSig::from_aggregate(&sig),
Err(e) => return blst_err_to_mithril(e, None),
Err(e) => return blst_err_to_mithril(e, None, None),
};

let p2_vks: Vec<&BlstVk> = vks.iter().map(|vk| &vk.0).collect();
Expand All @@ -427,6 +435,7 @@ impl Signature {
blst_err_to_mithril(
batched_sig.aggregate_verify(false, &slice_msgs, &[], &p2_vks, false),
None,
None,
)
.map_err(|_| MultiSignatureError::BatchInvalid)
}
Expand Down Expand Up @@ -628,6 +637,8 @@ mod unsafe_helpers {
#[cfg(test)]
mod tests {
use super::*;
use crate::error::RegisterError;
use crate::key_reg::KeyReg;
use proptest::prelude::*;
use rand_chacha::ChaCha20Rng;
use rand_core::{OsRng, SeedableRng};
Expand All @@ -643,19 +654,69 @@ mod tests {
let sk = SigningKey::gen(&mut ChaCha20Rng::from_seed(seed));
let vk = VerificationKey::from(&sk);
let sig = sk.sign(&msg);
assert!(sig.verify(&msg, &vk).is_ok());

let result = sig.verify(&msg, &vk);
assert!(result.is_ok(), "verify {result:?}");
}

#[test]
fn test_invalid_sig(msg in prop::collection::vec(any::<u8>(), 1..128),
seed in any::<[u8;32]>(),
) {
fn test_invalid_sig(msg in prop::collection::vec(any::<u8>(), 1..128), seed in any::<[u8;32]>()) {
let mut rng = ChaCha20Rng::from_seed(seed);
let sk1 = SigningKey::gen(&mut rng);
let vk1 = VerificationKey::from(&sk1);
let sk2 = SigningKey::gen(&mut rng);
let fake_sig = sk2.sign(&msg);
assert!(fake_sig.verify(&msg, &vk1).is_err());

let result = fake_sig.verify(&msg, &vk1);
assert_eq!(result, Err(MultiSignatureError::SignatureInvalid(fake_sig)));
}

#[test]
fn test_infinity_sig(msg in prop::collection::vec(any::<u8>(), 1..128), seed in any::<[u8;32]>()) {
let mut rng = ChaCha20Rng::from_seed(seed);
let sk = SigningKey::gen(&mut rng);
let vk = VerificationKey::from(&sk);

let p1 = blst_p1::default();
let sig_infinity = Signature(p1_affine_to_sig(&p1));

let result = sig_infinity.verify(&msg, &vk);
assert_eq!(result, Err(MultiSignatureError::SignatureInfinity(sig_infinity)));
}

#[test]
fn test_infinity_vk(seed in any::<[u8;32]>()) {
let mut rng = ChaCha20Rng::from_seed(seed);
let sk = SigningKey::gen(&mut rng);
let pop = ProofOfPossession::from(&sk);

let p2 = blst_p2::default();
let vk_infinity = VerificationKey(p2_affine_to_vk(&p2));
let vkpop_infinity = VerificationKeyPoP { vk: vk_infinity, pop };

let result = vkpop_infinity.check();
assert_eq!(result, Err(MultiSignatureError::VerificationKeyInfinity(Box::new(vkpop_infinity.vk))));
}

#[test]
fn test_keyreg_with_infinity_vk(num_sigs in 2..16usize, seed in any::<[u8;32]>()) {
let mut rng = ChaCha20Rng::from_seed(seed);
let mut kr = KeyReg::init();

let sk = SigningKey::gen(&mut rng);
let pop = ProofOfPossession::from(&sk);
let p2 = blst_p2::default();
let vk_infinity = VerificationKey(p2_affine_to_vk(&p2));
let vkpop_infinity = VerificationKeyPoP { vk: vk_infinity, pop };

for _ in 0..num_sigs {
let sk = SigningKey::gen(&mut rng);
let vkpop = VerificationKeyPoP::from(&sk);
let _ = kr.register(1, vkpop);
}

let result = kr.register(1, vkpop_infinity);
assert_eq!(result, Err(RegisterError::VerificationKeyInfinity(Box::new(vkpop_infinity.vk))));
}

#[test]
Expand All @@ -675,7 +736,8 @@ mod tests {
mvks.push(vk);
}

assert!(Signature::verify_aggregate(&msg, &mvks, &sigs).is_ok());
let result = Signature::verify_aggregate(&msg, &mvks, &sigs);
assert!(result.is_ok(), "Aggregate verification failed {result:?}");
}

#[test]
Expand Down Expand Up @@ -749,13 +811,23 @@ mod tests {
sigs.push(sig);
mvks.push(vk);
}
assert!(Signature::verify_aggregate(&msg, &mvks, &sigs).is_ok());
let (agg_vk, agg_sig) = Signature::aggregate(&mvks, &sigs).unwrap();
batch_msgs.push(msg.to_vec());
batch_vk.push(agg_vk);
batch_sig.push(agg_sig);
let verify_aggregate = Signature::verify_aggregate(&msg, &mvks, &sigs);
assert!(verify_aggregate.is_ok(), "Aggregate verification {verify_aggregate:?}");

match Signature::aggregate(&mvks, &sigs) {
Ok((agg_vk, agg_sig)) => {
batch_msgs.push(msg.to_vec());
batch_vk.push(agg_vk);
batch_sig.push(agg_sig);
}
Err(MultiSignatureError::AggregateSignatureInvalid) => {
println!("Aggregation failed.");
}
_ => unreachable!(),
}
}
assert!(Signature::batch_verify_aggregates(&batch_msgs, &batch_vk, &batch_sig).is_ok());
let batch_verify_aggregates = Signature::batch_verify_aggregates(&batch_msgs, &batch_vk, &batch_sig);
assert!(batch_verify_aggregates.is_ok(), "{batch_verify_aggregates:?}");
Comment on lines +814 to +830
Copy link
Member

Choose a reason for hiding this comment

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

This should be also rolled back as we have discussed.


// If we have an invalid signature, the batch verification will fail
let mut msg = [0u8; 32];
Expand All @@ -764,7 +836,8 @@ mod tests {
let fake_sig = sk.sign(&msg);
batch_sig[0] = fake_sig;

assert!(Signature::batch_verify_aggregates(&batch_msgs, &batch_vk, &batch_sig).is_err());
let batch_result = Signature::batch_verify_aggregates(&batch_msgs, &batch_vk, &batch_sig);
assert_eq!(batch_result, Err(MultiSignatureError::BatchInvalid));
}
}

Expand Down
Loading