SEC: fix usage of insecure RNG

Signed-off-by: Knut Ahlers <knut@ahlers.me>

lib/generator.go

@@ -1,15 +1,15 @@
 // Package securepassword implements a password generator and check.
-package securepassword // import "github.com/Luzifer/password/lib"
+package securepassword
 
 import (
 	"errors"
 	"fmt"
-	"math/rand"
 	"regexp"
 	"strings"
-	"time"
 )
 
+const minPasswordLength = 4
+
 // SecurePassword provides methods for generating secure passwords and
 // checking the security requirements of passwords
 type SecurePassword struct {
@@ -18,11 +18,9 @@ type SecurePassword struct {
 	badCharacters   []string
 }
 
-var (
+// ErrLengthTooLow represents an error thrown if the password will
-	// ErrLengthTooLow represents an error thrown if the password will
+// never be able match the security considerations in this package
-	// never be able match the security considerations in this package
+var ErrLengthTooLow = errors.New("passwords with a length lower than 4 will never meet the security requirements")
-	ErrLengthTooLow = errors.New("Passwords with a length lower than 4 will never meet the security requirements")
-)
 
 // NewSecurePassword initializes a new SecurePassword generator
 func NewSecurePassword() *SecurePassword {
@@ -53,7 +51,7 @@ func NewSecurePassword() *SecurePassword {
 // passwords.
 func (s *SecurePassword) GeneratePassword(length int, special bool) (string, error) {
 	// Sanity check
-	if length < 4 {
+	if length < minPasswordLength {
 		return "", ErrLengthTooLow
 	}
 
@@ -67,9 +65,13 @@ func (s *SecurePassword) GeneratePassword(length int, special bool) (string, err
 	}
 
 	password := ""
-	rand.Seed(time.Now().UnixNano())
 	for {
-		char := string(characterTable[rand.Intn(len(characterTable))])
+		cidx, err := randIntn(len(characterTable))
+		if err != nil {
+			return "", fmt.Errorf("generating random number: %w", err)
+		}
+
+		char := string(characterTable[cidx])
 		if strings.Contains(strings.Join(s.badCharacters, ""), char) {
 			continue
 		}
@@ -134,7 +136,7 @@ func (s *SecurePassword) matchesBasicSecurity(password string, needsSpecialChara
 		return false
 	}
 
-	// If request was to require special characters check for their existance
+	// If request was to require special characters check for their existence
 	if needsSpecialCharacters && !regexp.MustCompile(`[^a-zA-Z0-9]`).Match(bytePassword) {
 		return false
 	}

lib/generator_test.go

@@ -128,7 +128,7 @@ func TestBadCharacters(t *testing.T) {
 	for i := 0; i < 500; i++ {
 		pwd, err := NewSecurePassword().GeneratePassword(20, false)
 		if err != nil {
-			t.Errorf("An error occured: %s", err)
+			t.Errorf("An error occurred: %s", err)
 		}
 		for _, char := range badCharacters {
 			if strings.Contains(pwd, char) {

lib/helpers.go

@@ -0,0 +1,16 @@
+package securepassword
+
+import (
+	"crypto/rand"
+	"fmt"
+	"math/big"
+)
+
+func randIntn(max int) (int, error) {
+	cidx, err := rand.Int(rand.Reader, big.NewInt(int64(max)))
+	if err != nil {
+		return 0, fmt.Errorf("generating random number: %w", err)
+	}
+
+	return int(cidx.Int64()), nil
+}

lib/helpers_test.go

@@ -0,0 +1,21 @@
+package securepassword
+
+import "testing"
+
+func TestRandIntn(t *testing.T) {
+	var (
+		bound      = 16
+		sampleSize = 200000
+	)
+
+	for i := 0; i < sampleSize; i++ {
+		v, err := randIntn(bound)
+		if err != nil {
+			t.Fatalf("error in rng: %s", err)
+		}
+
+		if v < 0 || v >= bound {
+			t.Errorf("rng yielded number out-of-range 0-%d: %d", bound, v)
+		}
+	}
+}

lib/hibp.go

@@ -2,7 +2,7 @@ package securepassword
 
 import (
 	"bufio"
-	"crypto/sha1"
+	"crypto/sha1" //#nosec: G505 // HIBP uses shortened SHA1 to query hashes of vulnerable passwordss
 	"fmt"
 	"net/http"
 	"strings"
@@ -13,7 +13,7 @@ import (
 // ErrPasswordInBreach signals the password passed was found in any
 // breach at least once. The password should not be used if this
 // error is returned.
-var ErrPasswordInBreach = errors.New("Given password is known to HaveIBeenPwned")
+var ErrPasswordInBreach = errors.New("given password is known to HaveIBeenPwned")
 
 // CheckHIBPPasswordHash accesses the HaveIBeenPwned API with the
 // first 5 characters of the SHA1 hash of the password and scans the
@@ -24,7 +24,7 @@ var ErrPasswordInBreach = errors.New("Given password is known to HaveIBeenPwned"
 //
 // See more details at https://haveibeenpwned.com/API/v2#PwnedPasswords
 func CheckHIBPPasswordHash(password string) error {
-	fullHash := fmt.Sprintf("%x", sha1.Sum([]byte(password)))
+	fullHash := fmt.Sprintf("%x", sha1.Sum([]byte(password))) //#nosec: G401 // See crypto/sha1 import
 	checkHash := fullHash[0:5]
 
 	resp, err := http.Get("https://api.pwnedpasswords.com/range/" + checkHash)

lib/xkcd.go

@@ -2,7 +2,7 @@ package securepassword
 
 import (
 	"errors"
-	"math/rand"
+	"fmt"
 	"strings"
 	"time"
 
@@ -30,7 +30,7 @@ func NewXKCDGenerator() *XKCD { return &XKCD{} }
 // GeneratePassword generates a password with the number of words
 // given and optionally the current date prepended
 func (x XKCD) GeneratePassword(length int, addDate bool) (string, error) {
-	if length < 4 {
+	if length < minPasswordLength {
 		return "", ErrTooFewWords
 	}
 
@@ -43,9 +43,13 @@ func (x XKCD) GeneratePassword(length int, addDate bool) (string, error) {
 		password = time.Now().Format("20060102.")
 	}
 
-	rand.Seed(time.Now().UnixNano())
 	for len(usedWords) < length {
-		word := strings.Title(xkcdWordList[rand.Intn(len(xkcdWordList))])
+		widx, err := randIntn(len(xkcdWordList))
+		if err != nil {
+			return "", fmt.Errorf("generating random number: %w", err)
+		}
+
+		word := strings.Title(xkcdWordList[widx])
 		if str.StringInSlice(word, usedWords) {
 			// Don't use a word twice
 			continue

lib/xkcd_test.go

@@ -15,7 +15,6 @@ func TestXKCDWordList(t *testing.T) {
 func TestXKCDGeneratePassword(t *testing.T) {
 	for i := 4; i < 20; i++ {
 		pwd, err := DefaultXKCD.GeneratePassword(i, false)
-
 		if err != nil {
 			t.Fatalf("Generated had an error: %s", err)
 		}