Insecure Default RandomSecret() Generator about gotp HOT 17 CLOSED

time.Now().UnixNano() returns the number of nanoseconds elapsed since January 1, 1970 UTC. It's very hard to attack as a random seed although it can be attacked theoretically. I'll update implement via UUID later~

from gotp.

UnixNano() has very few bits of entropy and would be easy to attack.

This project should use crypto/rand in the Go standard library, which is Go's implementation of a cryptographically secure random number generator for use by cryptographic primitives, OTP generators, and other situations where high-quality, high-entropy randomness is essential. It's also cross-platform and works securely on all platforms Go is available on.

It's very easy to use. For example, https://pkg.go.dev/crypto/rand#example-Read :

package main

import (
	"bytes"
	"crypto/rand"
	"fmt"
)

func main() {
	c := 10
	b := make([]byte, c)
	_, err := rand.Read(b)
	if err != nil {
		fmt.Println("error:", err)
		return
	}
	// The slice should now contain random bytes instead of only zeroes.
	fmt.Println(bytes.Equal(b, make([]byte, c)))

}

from gotp.

No problem!

from gotp.

By the way, thanks @mergenchik for having another look at this issue from @johncave.

from gotp.

I had issue with the seed in random string generator. I ended up with this:

var rnd *rand.Rand

func init() {
	rnd = rand.New(rand.NewSource(time.Now().UnixNano()))
}

from gotp.

I think as @xlzd already noted, time.Now().UnixNano() is difficult to be used in timing attacks. In Java there is a SecureRandom which provides cryptographically strong random number generator. But there is no such in go standard library (edit: there is one, crypto/rand, mentioned in comments below by @codewinch).

So, maybe in your project you can use your own implementation of RandomSecret function which uses more secure random number generator.

Maybe we need to add in documentation, to not use RandomSecret, but implement their own with seeding of rand during program startup.

from gotp.

I had issue with the seed in random string generator. I ended up with this:

var rnd *rand.Rand

func init() {
	rnd = rand.New(rand.NewSource(time.Now().UnixNano()))
}

@ghost this is not much better. Better to use crypto/rand rather than math/rand, which is an unsafe RNG (better for gaming, compression, etc -- other non-cryptographic purposes.)

from gotp.

Unsafe RNG is a very serious vulnerability in this OTP library and should be rectified asap.

from gotp.

My apologies, skipped crypto/rand. Will review you pull request shortly.

from gotp.

Commented here: #12 (comment)

The constrained search space (base32) and very short (16 byte) sample size should be considered another vulnerability.

from gotp.

Commented here: #12 (comment)

The constrained search space (base32) and very short (16 byte) sample size should be considered another vulnerability.

we need to check with RFC. HOTP RFC 4226 and TOTP RFC 6238. I will try to check if got some time.

from gotp.

I checked documentation, there is no constraints on secret size. Future comments on secret size will be in #13.
Need to correctly switch from math/rand to crypto/rand

from gotp.

swtiched from math/rand to crypto/rand in #14.

from gotp.

The shared secret must be at least 128 bits but RFC 4226 recommends a shared secret length of at least 160 bits.

from gotp.

Most other OTP packages (and other crypto packages like golang.org/x/crypto/nacl/box) utilize [32]byte for secrets.

from gotp.

Using 128 bits in 2022 is a tiny bit anachronistic. ;) Is there a practical reason why you can't spare 16 additional bytes for the secret?

from gotp.

Hi @codewinch, let's move this discussion to #13 if you do not mind.

from gotp.