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

Failing test for verify_batch #117

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Feb 22, 2020

sometimes verify_batch return Ok(_) on this input, sometimes Err(_)

Not sure if it is as designed

@burdges
Copy link

burdges commented Feb 22, 2020

Is this maybe verifying because [0u8, 32] turns out to be the identity for the curve and the zero for the scalar field? We'd have one scalar zero and two curve identities in the verification equation. Any message works because you're multiplying the hash by the curve identity.

I pushed w3f/schnorrkel@b89b0d2 to schnorrkel that achieves the same goal with randomized signatures

@burdges
Copy link

burdges commented Feb 22, 2020

I see, your test's failure is intermittent, oops. If replace it with this code then I always get the batch verify_batch error, never the single verified error, but with numbers ranging from 0-12 or possibly higher.

#[test]
fn verify_batch_not_flaky_on_empty_input() {
    let sig = Signature::from_bytes(&[0u8; 64]).unwrap();
    let pk = PublicKey::from_bytes(&[0u8; 32]).unwrap();
    // use curve25519_dalek::traits::IsIdentity;
    // panic!("{:?}", (&pk.1 + &pk.1).is_identity());
    for i in 0..100 {
        if pk.verify(&[], &sig).is_ok() {
            panic!("Successfully single verified empty message with empty signature and empty public ({})", i);
        }
    }
    for i in 0..100 {
        if verify_batch( &[&[]], &[sig], &[pk]).is_ok() {
            panic!("Successfully batch verified empty message with empty signature and empty public ({})", i);
        }
    }
}

Anyways [0u8; 32] is not the identity because Ed25519 compresses as the y coordinate, but instead it's actually one of the two points of order 4. Your test passes whenever zs[0] winds up divisible by four, which happens 25% of the time.

In other words, you've independently found #115 for which one good explanation is https://moderncrypto.org/mail-archive/curves/2016/000836.html

We're ostensibly not worried about signatures that fail single verification, but pass batch verification. We really do not want valid signatures that fail batch verification intermittently though.

We run into trouble because of reduction in the multiplication zhrams[i] := hrams[i] * zs[i] in https://github.com/dalek-cryptography/ed25519-dalek/blob/master/src/batch.rs#L208

It's possible hrams[i] is divisible by 2, 4, or 8 and clears the torsion from the single verification, but a significant fraction of zs[i] undoes this which results in diverging batch verification results, and validators disagreeing about block validity.

Isis' recent commits indicate her them wanting to make batch verification deterministic with delinearization instead of system randomness, perhaps to fix this, not sure.

@isislovecruft
Copy link
Member

First, thanks for taking the time to file this issue, @NikVolf, and also thanks @burdges for the comments.

While the deterministic batch verification does fix this specific intermittent batch verification failure (the reasons why it fixes it in this specific case, I'll get to in a moment), this isn't why I worked on that; instead those reasons are explained by @kchalkias here.

Let's look at why it fails intermittently. The batch verification equation is:

-\sum\limits_{i=0}^n z_i s_i (mod \ell) B + \sum\limits_{i=0}^n z_i R_i + \sum\limits_{i=0}^n \bigg( z_i \mathcal{H}(R \| A \| M) (mod \ell) \bigg) A_i = 0
  1. For the first sum, since the signature's s = 0, then regardless of what the nonce z is, we end up with the identity element.
  2. For the second, it doesn't matter what z is, we will always end up with a point of some small order, but not necessarily of order 4. Depending on which small order group it's in, and depending on if z combined with the H(R||A||M) in the third summation produces an element in the same small order group, the two when added together are the identity element. Further, if z is any multiple of 4, the result here is again the identity element.
  3. For the third, it depends on what the result of the H(R||A||M) is, and whether the nonce z multiplied by it produces a point of small order in the same subgroup. For this specific input (i.e. the blank message), only points in \mathcal E[4] satisfy the verification equation.

Point 3 above is why the deterministic batch verification would fix it, for this specific input, however no amount of protection against a bad RNG is going to recover from a malicious input of points of small order and the scalar additive identity. That is, if we instead compile your test with cargo test --features batch_deterministic, it always passes because it's deterministic and the output of the RNG that we're getting isn't one that can be exploited with points of small order, but that does not at all guarantee that the deterministic output will be unexploitable for other inputs, and in fact, it may even facilitate (since the adversary knows the state of the RNG, which is produced by hashing a transcript of the protocol) an adversary brute forcing specially crafted inputs which erroneously pass even deterministic batch verification. (The purpose of the deterministic batch verification is to prevent e.g. an RNG which does nothing but output zeroes from causing similar problems, and, in fact there is no way in ed25519-dalek now for this to happen, as when deterministic verification is disabled—as is by default—the behaviour is to use "synthetic" randomness, i.e. the transcript but also seeded with system-obtained randomness.)

The only way I can see to prevent this issue from happening would be to check that neither the R component of the signature nor the public key are of small order. Also it should be checked that the s component of the signature is not the additive identity.

@isislovecruft
Copy link
Member

Example code to better see what's happening here:

Cargo.toml:

[package]                                                                                                                                                                                                                                      
name = "foobar"                                                                                                                                                                                                                                
version = "0.1.0"                                                                                                                                                                                                                              
authors = ["Isis Lovecruft <[email protected]>"]                                                                                                                                                                                      
edition = "2018"                                                                                                                                                                                                                               
                                                                                                                                                                                                                                               
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html                                                                                                                                               
                                                                                                                                                                                                                                               
[dependencies]                                                                                                                                                                                                                                 
curve25519-dalek = { version = "2"}                                                                                                                                                            
rand = "*"                                                                                                                                                                                                                                     
sha2 = "0.8"

src/main.rs:

use curve25519_dalek::constants::ED25519_BASEPOINT_TABLE;                                                                                                                                                                                      
use curve25519_dalek::constants::EIGHT_TORSION;                                                                                                                                                                                                
use curve25519_dalek::edwards::CompressedEdwardsY;                                                                                                                                                                                             
use curve25519_dalek::scalar::Scalar;                                                                                                                                                                                                          
use curve25519_dalek::traits::IsIdentity;                                                                                                                                                                                                      
                                                                                                                                                                                                                                               
use rand::thread_rng;                                                                                                                                                                                                                          
                                                                                                                                                                                                                                               
use sha2::Digest;                                                                                                                                                                                                                              
use sha2::Sha512;                                                                                                                                                                                                                              
                                                                                                                                                                                                                                               
fn main() {                                                                                                                                                                                                                                    
    let mut rng = thread_rng();                                                                                                                                                                                                                
                                                                                                                                                                                                                                               
    let nonce = Scalar::random(&mut rng);                                                                                                                                                                                                      
    // An example of a `z` which causes the verification to pass.                                                                                                                                                                              
    //let nonce = Scalar::from(127u8);                                                                                                                                                                                                         
                                                                                                                                                                                                                                               
    println!("nonce = {:?}", nonce);                                                                                                                                                                                                           
                                                                                                                                                                                                                                               
    let first = &(&nonce * &Scalar::zero()) * &ED25519_BASEPOINT_TABLE;                                                                                                                                                                        
                                                                                                                                                                                                                                               
    println!("first = {:?}", first.compress());                                                                                                                                                                                                
                                                                                                                                                                                                                                               
    let second = CompressedEdwardsY([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,]).decompress().unwrap() * &nonce;                                                                                                        
                                                                                                                                                                                                                                               
    println!("second = {:?}", second);                                                                                                                                                                                                         
    println!("second = {:?}", second.compress());                                                                                                                                                                                              
    println!("second is identity: {}", second.is_identity());                                                                                                                                                                                  
    println!("second is small order: {}", second.is_small_order());                                                                                                                                                                            
    println!("second is torsion free: {}", second.is_torsion_free());                                                                                                                                                                          
                                                                                                                                                                                                                                               
    let mut h: Sha512 = Sha512::default();                                                                                                                                                                                                     
    h.input([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,]);                                                                                                                                                               
    h.input([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,]);                                                                                                                                                               
    h.input(&[]);                                                                                                                                                                                                                              
                                                                                                                                                                                                                                               
    let hram = Scalar::from_hash(h);                                                                                                                                                                                                           
                                                                                                                                                                                                                                               
    // I was trying to find a specific divisor/bit pattern here.  The Scalar::bits()                                                                                                                                                           
    // function in dalek is private so you'll have to fork it and make it pub if you                                                                                                                                                           
    // want to debug this.                                                                                                                                                                                                                     
    //print!("hram bits = ");                                                                                                                                                                                                                  
    //for i in 0..256 {                                                                                                                                                                                                                        
    //    print!("{}", hram.bits()[i]);                                                                                                                                                                                                        
    //}                                                                                                                                                                                                                                        
    //println!();                                                                                                                                                                                                                              
                                                                                                                                                                                                                                               
    let third = &(&nonce * &hram) * CompressedEdwardsY([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,]).decompress().unwrap();                                                                                              
                                                                                                                                                                                                                                               
    println!("third = {:?}", third);                                                                                                                                                                                                           
    println!("third = {:?}", third.compress());                                                                                                                                                                                                
    println!("third is small order: {}", third.is_small_order());                                                                                                                                                                              
    println!("third is torsion free: {}", third.is_torsion_free());                                                                                                                                                                            
                                                                                                                                                                                                                                               
    let sum = &first + &second + &third;                                                                                                                                                                                                       
    println!("sum equals identity: {}", sum.is_identity());                                                                                                                                                                                    
                                                                                                                                                                                                                                               
    if sum.is_identity() {                                                                                                                                                                                                                     
        for i in 0..8 {                                                                                                                                                                                                                        
            if EIGHT_TORSION[i].compress() == second.compress() {                                                                                                                                                                              
                println!{"second is E[8] index {}", i};                                                                                                                                                                                        
            }                                                                                                                                                                                                                                  
            if EIGHT_TORSION[i].compress() == third.compress() {                                                                                                                                                                               
                println!{"third is E[8] index {}", i};                                                                                                                                                                                         
            }                                                                                                                                                                                                                                  
        }                                                                                                                                                                                                                                      
    }
}

Example output:

∃!isisⒶwintermute:(master #)/tmp/foobar ∴ cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/foobar`
nonce = Scalar{
        bytes: [159, 131, 144, 89, 35, 189, 6, 127, 135, 6, 229, 126, 186, 36, 164, 5, 173, 189, 154, 135, 7, 9, 221, 68, 166, 194, 159, 89, 186, 27, 18, 9],                                                                                 
}
first = CompressedEdwardsY: [1, 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]                                                                                                                  
second = EdwardsPoint{
        X: FieldElement51([2196963485250514, 1074217201222803, 260058224171617, 1199244361387740, 1728788920764473]),
        Y: FieldElement51([2251799813685229, 2251799813685247, 2251799813685247, 2251799813685247, 2251799813685247]),
        Z: FieldElement51([1466901059752530, 619630306190515, 1936246639957922, 919087525353124, 727047657121906]),
        T: FieldElement51([2251799813685229, 2251799813685247, 2251799813685247, 2251799813685247, 2251799813685247])
}
second = CompressedEdwardsY: [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, 128]                                                                                                               
second is identity: false
second is small order: true
second is torsion free: false
third = EdwardsPoint{
        X: FieldElement51([476136717288259, 2203848157667486, 1158744360323770, 1945735288663543, 1254162920112039]),
        Y: FieldElement51([2251799813685229, 2251799813685247, 2251799813685247, 2251799813685247, 2251799813685247]),
        Z: FieldElement51([476029891769546, 321189867506252, 428417399100066, 1736060194215308, 1956192926563457]),
        T: FieldElement51([2251799813685229, 2251799813685247, 2251799813685247, 2251799813685247, 2251799813685247])
}
third = CompressedEdwardsY: [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]                                                                                                                  
third is small order: true
third is torsion free: false
sum equals identity: true
second is E[8] index 2
third is E[8] index 6

@valerini
Copy link

The only way I can see to prevent this issue from happening would be to check that neither the R component of the signature nor the public key are of small order. Also it should be checked that the s component of the signature is not the additive identity.

Will this be enough though?
If I create a malicious signature as (R, s), where R is of order 2 * L (i.e. 2 * R is a point of order L, R has a torsion component of order 2, multiplying by any even number will kill this small torsion component in R), public key A is of order L (with no small-torsion components). Then the individual verification equation will fail (s * G != H(R, A, M) * A + R) as there is a point of order 2 * L on the right and a point of order L on the left. But the batch verification will succeed with probability 50% because whenever R is multiplied by an even scalar, its small order component vanishes and (R,s) passes the verification equation. So such a signature will also produce a flaky test.

I think the only solution that makes batch verification work and makes it compatible with individual signature verification is to multiply by a cofactor in all the equations.

@isislovecruft
Copy link
Member

I think the only solution that makes batch verification work and makes it compatible with individual signature verification is to multiply by a cofactor in all the equations.

I might be misremembering what we concluded when talking with Dan Boneh, but after we verified a sketch of a proof that removing one of the linearisation factors was equally secure as single-signature verification, someone had the idea to simply change the first linearisation factor to the cofactor. That is, rather than

-\sum\limits_{i=0}^n z_i s_i (mod \ell) B + \sum\limits_{i=0}^n z_i R_i + \sum\limits_{i=0}^n \bigg( z_i \mathcal{H}(R \| A \| M) (mod \ell) \bigg) A_i = 0

To instead do

-8 s_0 (mod \ell) B -\sum\limits_{i=1}^n z_i s_i (mod \ell) B + 8 R_0 + \sum\limits_{i=1}^n z_i R_i + 8 \mathcall{H}(R \| A \| M) (mod \ell) A_0 + \sum\limits_{i=1}^n \bigg( z_i \mathcal{H}(R \| A \| M) (mod \ell) \bigg) A_i = 0

The latter idea we then decided was bad because it allowed the first signature to have e.g. s_0=0, R_0 as a point of small order, and then the only thing preventing the signature from erroneously verifying was essentially that the public key goes into the hash and thus would need to be brute forced to find one which produces another point of small order for the last term in the equation.

Instead, just for clarity's sake since it seems like people are paying attention to this issue, I propose we check if any Rs or As are points of small order before the verification equation, e.g.:

for (key, signature) in public_keys.iter().zip(signatures.iter()) {
    // Check if the key or signature.R multiplied by the cofactor is the additive identity and if so return an error.
    if key.point.is_small_order() || signature.R.is_small_order() {
        return Err();
    }
}

@burdges
Copy link

burdges commented Oct 28, 2020

You'll need 256ish bit scalar multiplications everywhere else if the first delinearisation factor is 0 or 3 bits. It's faster to use 128 bit scalar multiplications everywhere.

I doubt .is_small_order() addresses many cases when applied to keys and signatures, but EdwardsPoint::optional_multiscalar_mul(..).ok_or(..)..is_small_order() is equivalent to multiplying by the cofactor everywhere. Ain't clear verify_strict achieves much either.

As @valerini says, adversaries who exploit this cofactor multiplication create signatures that pass derandomized batch verification, but maybe fail single verification, and worse pass-or-fail non-deterministic batch verification intermittently. Afaik you address the single verification best with some verify_relaxed or verify_batchable method that does roughly

let R = EdwardsPoint::vartime_double_scalar_mul_basepoint(&k, &(minus_A), &signature.s);
if (R - signature_R).is_small_order() { .. 

It's interesting you made batch_deterministic a feature flag, so that if anyone activates it anywhere then even without this they're protected from intermittent batch verification failures, and resulting in consensus failures, assuming they fix their batches. It risks batch_deterministic being activated by surprise, but probably harmless in practice. I'd make batch_deterministic a default feature myself.

Could you just have SignatureRelaxed or SignatureBatchable and SignatureStandard types that users choose when deserializing? It fits your traits nicely and provide docs context to explain the situation. You could supply verify_batch only for SignatureRelaxed/SignatureBatchable.

In fact, blockchains could even upgrade from SignatureStandard to SignatureRelaxed / SignatureBatchable without breaking their history. It'll play havoc with alternative non-rust implementation of course. I suppose someone could do an RFC for relaxed ed25519 for consistent batch verification.

@huitseeker
Copy link
Contributor

I think the only solution that makes batch verification work and makes it compatible with individual signature verification is to multiply by a cofactor in all the equations.

I think it's important to insist that in the above, Lera means "the only solution that makes batch verification work and makes it compatible with cofactored individual signature verification". For more on this specific point, see table 3 of https://eprint.iacr.org/2020/1244

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants