bnb-chain / tss-lib

Threshold Signature Scheme, for ECDSA and EDDSA
MIT License
759 stars 261 forks source link

fix signing issue if the message is leading with 0x00 #284

Closed zargarzadehm closed 6 months ago

zargarzadehm commented 7 months ago

for issue #264

ZhAnGeek commented 6 months ago

Hi @zargarzadehm Thanks for submitting codes! It will work well, however The PR will change m from big.Int to bytes, it will change the interface of NewLocalParty which are highly depended by upstreams. I would recommend use big.Int.FillBytes() when necessary. Could you have an update on it accordingly?

zargarzadehm commented 6 months ago

Hi @zargarzadehm Thanks for submitting codes! It will work well, however The PR will change m from big.Int to bytes, it will change the interface of NewLocalParty which are highly depended by upstreams. I would recommend use big.Int.FillBytes() when necessary. Could you have an update on it accordingly?

yeah, I will update ASAP!

zargarzadehm commented 6 months ago

@ZhAnGeek I tried using big.Int.FillBytes() for testing, but I came across an issue regarding the variable size of the signData. Since signData is non-deterministic, I couldn't create a buffer for it. Moreover, I observed that big.Int.BitLen returns the size of the slice without considering leading zeros.

ZhAnGeek commented 6 months ago

@zargarzadehm Hi, Zarg I kinda get lost, the signData does not need to be changed, I push a simple codes https://github.com/bnb-chain/tss-lib/compare/master...ZhAnGeek:tss-lib:example-filling-bytes, you could check if this could be adapted. You could also check if any potential backoffs and we're welcome for your discussions. (maybe it is now not available to sign bytes pure less than 32)

zargarzadehm commented 6 months ago

@zargarzadehm Hi, Zarg I kinda get lost, the signData does not need to be changed, I push a simple codes master...ZhAnGeek:tss-lib:example-filling-bytes, you could check if this could be adapted. You could also check if any potential backoffs and we're welcome for your discussions. (maybe it is now not available to sign bytes pure less than 32)

By signData I mean msg that we want to sign it. Fixing the size to 32 will cause issues for both data greater and less than 32. The FillBytes method puts 00 for each missing byte, which will affect the signing process. In my scenario, the size is 32, but I believe that fixing the size of msg in the package is not a good solution so unless we want to force users to sign msgs with just 32 bytes.

If you have another option for discussion instead of Github tell me!

ZhAnGeek commented 6 months ago

@zargarzadehm Hi, Zarg I kinda get lost, the signData does not need to be changed, I push a simple codes master...ZhAnGeek:tss-lib:example-filling-bytes, you could check if this could be adapted. You could also check if any potential backoffs and we're welcome for your discussions. (maybe it is now not available to sign bytes pure less than 32)

By signData I mean msg that we want to sign it. Fixing the size to 32 will cause issues for both data greater and less than 32. The FillBytes method puts 00 for each missing byte, which will affect the signing process. In my scenario, the size is 32, but I believe that fixing the size of msg in the package is not a good solution so unless we want to force users to sign msgs with just 32 bytes.

If you have another option for discussion instead of Github tell me!

Yeah I think so, what about adding an optional parameters fullBytes/fullBytesLen, and constraint them in the signing(check if consistent to the big.Int value), which could be compatible to the old function.

zargarzadehm commented 6 months ago

@zargarzadehm Hi, Zarg I kinda get lost, the signData does not need to be changed, I push a simple codes master...ZhAnGeek:tss-lib:example-filling-bytes, you could check if this could be adapted. You could also check if any potential backoffs and we're welcome for your discussions. (maybe it is now not available to sign bytes pure less than 32)

By signData I mean msg that we want to sign it. Fixing the size to 32 will cause issues for both data greater and less than 32. The FillBytes method puts 00 for each missing byte, which will affect the signing process. In my scenario, the size is 32, but I believe that fixing the size of msg in the package is not a good solution so unless we want to force users to sign msgs with just 32 bytes. If you have another option for discussion instead of Github tell me!

Yeah I think so, what about adding an optional parameters fullBytes/fullBytesLen, and constraint them in the signing(check if consistent to the big.Int value), which could be compatible to the old function.

We can have an optional parameter FullBytesLen to NewLocalParty for the size of bigInt and if the size is set we will use FillBytes otherwise use Bytes (like the old function)

ZhAnGeek commented 6 months ago

@zargarzadehm Hi, Zarg I kinda get lost, the signData does not need to be changed, I push a simple codes master...ZhAnGeek:tss-lib:example-filling-bytes, you could check if this could be adapted. You could also check if any potential backoffs and we're welcome for your discussions. (maybe it is now not available to sign bytes pure less than 32)

By signData I mean msg that we want to sign it. Fixing the size to 32 will cause issues for both data greater and less than 32. The FillBytes method puts 00 for each missing byte, which will affect the signing process. In my scenario, the size is 32, but I believe that fixing the size of msg in the package is not a good solution so unless we want to force users to sign msgs with just 32 bytes. If you have another option for discussion instead of Github tell me!

Yeah I think so, what about adding an optional parameters fullBytes/fullBytesLen, and constraint them in the signing(check if consistent to the big.Int value), which could be compatible to the old function.

We can have an optional parameter FullBytesLen to NewLocalParty for the size of bigInt and if the size is set we will use FillBytes otherwise use Bytes (like the old function)

Sounds great! we could do that way.

zargarzadehm commented 6 months ago

@ZhAnGeek

I added an interface NewLocalPartyWithLength like NewLocalParty. Can I change the interface of NewLocalPartyWithKDD like the one below or is it highly dependent by upstreams?

func NewLocalPartyWithKDD(
    msg *big.Int,
    params *tss.Parameters,
    key keygen.LocalPartySaveData,
    keyDerivationDelta *big.Int,
    out chan<- tss.Message,
    end chan<- *common.SignatureData,
    fullBytesLen int,
)
ZhAnGeek commented 6 months ago

@ZhAnGeek

I added an interface NewLocalPartyWithLength like NewLocalParty. Can I change the interface of NewLocalPartyWithKDD like the one below or is it highly dependent by upstreams?

func NewLocalPartyWithKDD(
  msg *big.Int,
  params *tss.Parameters,
  key keygen.LocalPartySaveData,
  keyDerivationDelta *big.Int,
  out chan<- tss.Message,
  end chan<- *common.SignatureData,
  fullBytesLen int,
)

Ahh I suggested you can change it to optional parameters as well, this is an interface use for HD, should be dependent

zargarzadehm commented 6 months ago

@ZhAnGeek I added an interface NewLocalPartyWithLength like NewLocalParty. Can I change the interface of NewLocalPartyWithKDD like the one below or is it highly dependent by upstreams?

func NewLocalPartyWithKDD(
    msg *big.Int,
    params *tss.Parameters,
    key keygen.LocalPartySaveData,
    keyDerivationDelta *big.Int,
    out chan<- tss.Message,
    end chan<- *common.SignatureData,
    fullBytesLen int,
)

Ahh I suggested you can change it to optional parameters as well, this is an interface use for HD, should be dependent

ok, as I know there isn't optional input in Golang functions but as an alternative solution I used Variadic Parameters for FullBytesLength parameter and the merge is ready for review!

zargarzadehm commented 6 months ago

@ZhAnGeek Do you have any plans for new release? As I see there are around 7 PR after latest release!

ZhAnGeek commented 6 months ago

@zargarzadehm New release has been launched, thank you! https://github.com/bnb-chain/tss-lib/releases/tag/v2.0.2