Insecure Default RandomSecret() Generator

[johncave]

Using the current time as a random seed opens the library up to timing attacks if the time that the user enabled OTP can be guessed. Worse, some sites may record this as a matter of courtesy for example to display "You have had OTP enabled since October 2018".

[xlzd]

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~

[ghost]

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()))
}

[mergenchik]

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.

[codewinch]

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)))

}

[codewinch]

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.)

[codewinch]

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

[mergenchik]

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

[codewinch]

No problem!

[codewinch]

Commented here: #12 (comment)

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

[codewinch]

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

[mergenchik]

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.

[mergenchik]

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

[mergenchik]

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

[codewinch]

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

[codewinch]

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

[codewinch]

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?

[mergenchik]

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