bnb-chain / tss-lib

Threshold Signature Scheme, for ECDSA and EDDSA
MIT License
790 stars 271 forks source link

bad result from eddsa signing (signature length is 63 bytes instead of 64) #202

Open levischechter opened 2 years ago

levischechter commented 2 years ago

well, this is my implementation for the eddsa signing lib (i took it straight from the test file)

for {
        select {
            case err := <-errCh:
                fmt.Printf("Error: %s", err)
                break signing
            case msg := <-outCh:
                dest := msg.GetTo()
                from := msg.GetFrom()
                if dest == nil { 
                    for i := 0; i < len(pIDs); i++ {
                        if i == from.Index {
                            continue
                        }
                        if ok := updater(msg, errCh, p2ps[from.Index], i); !ok{
                            return
                        }
                    }
                } else { 
                    for _, destination := range dest {
                        // fmt.Printf("dest: %s\n", destination)
                        if destination.Index == from.Index {
                            return
                        }
                        if ok := updater(msg, errCh, p2ps[from.Index], destination.Index); !ok {
                            return
                        }
                    }
                }
            case save := <-endCh:
                R := new(big.Int).SetBytes(save.R[:]) 
                S := new(big.Int).SetBytes(save.S[:])
                V := new(big.Int).SetBytes(save.SignatureRecovery[:]) 
                M := new(big.Int).SetBytes(save.M[:])  
                sig := new(big.Int).SetBytes(save.Signature[:])
                fmt.Printf("{sig=%s, r=%s, s=%s, v=%s, m=%s}", sig.String(), R.String(), S.String(), V.String(), M.String())
                break signing
        }
}

and I'm getting this results:

sig=420103990468094270452503032366447392207478837919537176652153928397188071222906781927852793259516476775925732882093705982917658354422734550707258699001614 r=74351319199570382178743445073466484013692963667981877516020532843769404065032 s=6642376151640936682087191879529680821793081600695722025258330097752334617937 v=0 m=67260724992502234755737739768655988254295965243685701744353092692572152002707134201481919157416715395395920045373771964593332106659303697396315618080968421380000565753354594396895542442837315488109783563881525681399025826912743833132178807855007542815784925967474517295939336119244292341928311732662384109954691246947926564020247960034325191731029857915633664

when i'm converting r,s to bytes i'm getting this 63-length-bytes-as-hex string: a4615af8de21bc3538ce1a2c06e9b283d75dd61cadae5c8afe010a9bef6c0508eaf73a0c4b9cc79161ac1cb3ceea0d3e62900d414b0fc33d7dbbf537f774d5

when i'm converting sig to bytes i'm getting this 63-length-bytes-as-hex string: 8056cef9b0a01fe8a5caead1cd65dd783b2e9062c1ace3835bc21def85a61a4514d777f53bfdbd733fcb014d40029e6d3a0ee3ccbc11a1679ccb9c4a073af0

am i doing something wrong? p.s. sometimes it's working

yshurik commented 2 years ago

Are you sure save.Signature is correct format to apply as big.Int? I believe GetR and GetS are enough: This works fine (though via btcec):

                sig_obj := btcec.Signature{
                    R: new(big.Int).SetBytes(save.GetR()),
                    S: new(big.Int).SetBytes(save.GetS()),
                }
                ok := sig_obj.Verify(hash_data, joint_pubkey)
levischechter commented 2 years ago

Are you sure save.Signature is correct format to apply as big.Int? I believe GetR and GetS are enough: This works fine (though via btcec):

              sig_obj := btcec.Signature{
                  R: new(big.Int).SetBytes(save.GetR()),
                  S: new(big.Int).SetBytes(save.GetS()),
              }
              ok := sig_obj.Verify(hash_data, joint_pubkey)

save.Signature is of type: []byte which is compatible with big.Int.setBytes

but the problem is that I'm getting S value with 31 bytes and it's supposed to be 32

yshurik commented 2 years ago

Have a look at btcec: It is a case when S is less than half the order N

func (sig *Signature) Serialize() []byte {
    // low 'S' malleability breaker
    sigS := sig.S
    if sigS.Cmp(S256().halfOrder) == 1 {
        sigS = new(big.Int).Sub(S256().N, sigS)
    }
levischechter commented 2 years ago

Have a look at btcec: It is a case when S is less than half the order N

func (sig *Signature) Serialize() []byte {
  // low 'S' malleability breaker
  sigS := sig.S
  if sigS.Cmp(S256().halfOrder) == 1 {
      sigS = new(big.Int).Sub(S256().N, sigS)
  }

I don't think it's related as the issue i came up with is in eddsa and not ecdsa

yycen commented 2 years ago

In the protocol, the signature bytes is EncodedBytes(r) + EncodedBytes(s) where EncodedBytes is little endian (reversed) order of bytes. So you may need to reverse r and s to see if it matches sig.

round.data.Signature = append(bigIntToEncodedBytes(round.temp.r)[:], sumS[:]...) round.data.R = round.temp.r.Bytes() round.data.S = s.Bytes() round.data.M = round.temp.m.Bytes()

levischechter commented 2 years ago

the problem is not that r+s doesn't match sig. the problem is that (R+S).length is 63 instead of 64 and so Signature.length is 63 instead of 64

yycen commented 2 years ago

sig, _ := new(big.Int).SetString("420103990468094270452503032366447392207478837919537176652153928397188071222906781927852793259516476775925732882093705982917658354422734550707258699001614", 10) fmt.Println(sig.Bytes())

r, _ := new(big.Int).SetString("74351319199570382178743445073466484013692963667981877516020532843769404065032", 10) fmt.Println(r.Bytes())

s, _ := new(big.Int).SetString("6642376151640936682087191879529680821793081600695722025258330097752334617937", 10) fmt.Println(s.Bytes())

fmt.Println(len(sig.Bytes()), len(r.Bytes()), len(s.Bytes()))

Not sure the problem you encounted. As the vaulue you provided, r and s are 32bytes, and sig is 64bytes, which sig[:32] are reversed padded of r, and sig[32:] are reversed padded of s

21fahm commented 1 year ago

Can i see your:

updater(msg, errCh, p2ps[from.Index], destination.Index);

@levischechter