secp256k1 implementation fails for certain inputs about gocoin HOT 6 CLOSED

Thanks for reporting. I will look into it when I only find some time.

from gocoin.

I fixed it for now, but I did it by analyzing the problem in my code, not by merging all the recent fixes from the original sipa's lib.

I should review all the changes that have been made in the original lib, ever since the version that my ported lib was based on. But I will need to find much more time for it.

Anyway, the fix for the issue you found is already committed: https://github.com/piotrnar/gocoin/blob/master/lib/secp256k1/xy.go

And thanks so much for reporting this, as it was potentially a very serious one.

from gocoin.

SIPA added very good unit tests. I am porting over unit tests and doing
more tests to compare between the libraries.

On Tue, Dec 15, 2015 at 12:30 PM, Piotr Narewski [email protected]
wrote:

I fixed it for now, but I did it by analyzing the problem in my code, not
by merging all the recent fixes from the original sipa's lib.

I should review all the changes that have been made in the original lib,
ever since the version that my ported lib was based on. But I will need to
find much more time for it.

Anyway, the fix for the issue you found is already committed:
https://github.com/piotrnar/gocoin/blob/master/lib/secp256k1/xy.go

And thanks so much for reporting this, as it was potentially a very
serious one.

—
Reply to this email directly or view it on GitHub
#15 (comment).

from gocoin.

Add

if pk.IsValid() == false {
log.Panic()
}

before return in

func Multiply(xy, k []byte) []byte {

and

func BaseMultiply(k []byte) []byte {

Then try for random input values. I get crash.

from gocoin.

OK - I see what you mean now. :)
Sorry for all the previous, confusing replies.

I think it fails because not any random value can be a valid public key.
You cannot just take 256 bits of random data and feed it to the function with 02 or 03 in front. Or 512 bits with 04 in front...

If you want to create a random public key, generate 256 bits of random data (that will be your secret key) and then use the function BaseMultiply() to get the public key - this will give you a valid public key that you can pass to BaseMultiplyAdd.

I do not see any problems with BaseMultiply() though - it always returns a valid key:

package main

import (
    "github.com/piotrnar/gocoin/lib/secp256k1"
    "encoding/hex"
    "crypto/rand"
)


func main() {
    var sec [32]byte
    var out [33]byte
    var cnt uint64
    for {
        rand.Read(sec[:])

        if !secp256k1.BaseMultiply(sec[:], out[:]) {
            println("BaseMultiplyAdd returned error", cnt)
            return
        }

        var pk secp256k1.XY
        if !pk.ParsePubkey(out[:]) {
            println("returned ParsePubkey failed", cnt)
            return
        }

        if !pk.IsValid() {
            println("IsValid failed", cnt, hex.EncodeToString(out[:]))
            return
        }


        cnt++
        if (cnt&0xfff)==0 {
            println("cnt", cnt)
        }
    }
}

from gocoin.

Yes. That is desired behaviour. It is better that it fails, than people use invalid public key. Feeding in invalid public key can leak bits of private key if used improperly for ECDH, so there is strict verification.

from gocoin.