From 698109fb04f11e223b308493d853de7b8a8196f5 Mon Sep 17 00:00:00 2001 From: Knut Ahlers Date: Mon, 27 Jun 2022 19:41:47 +0200 Subject: [PATCH] SEC: fix usage of insecure RNG Signed-off-by: Knut Ahlers --- lib/generator.go | 26 ++++++++++++++------------ lib/generator_test.go | 2 +- lib/helpers.go | 16 ++++++++++++++++ lib/helpers_test.go | 21 +++++++++++++++++++++ lib/hibp.go | 6 +++--- lib/xkcd.go | 12 ++++++++---- lib/xkcd_test.go | 1 - 7 files changed, 63 insertions(+), 21 deletions(-) create mode 100644 lib/helpers.go create mode 100644 lib/helpers_test.go diff --git a/lib/generator.go b/lib/generator.go index ffb96ee..decb61e 100644 --- a/lib/generator.go +++ b/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 - // never be able match the security considerations in this package - ErrLengthTooLow = errors.New("Passwords with a length lower than 4 will never meet the security requirements") -) +// ErrLengthTooLow represents an error thrown if the password will +// 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") // 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 } diff --git a/lib/generator_test.go b/lib/generator_test.go index dd72c8c..3298e58 100644 --- a/lib/generator_test.go +++ b/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) { diff --git a/lib/helpers.go b/lib/helpers.go new file mode 100644 index 0000000..e4c5118 --- /dev/null +++ b/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 +} diff --git a/lib/helpers_test.go b/lib/helpers_test.go new file mode 100644 index 0000000..478a6e4 --- /dev/null +++ b/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) + } + } +} diff --git a/lib/hibp.go b/lib/hibp.go index 919a0eb..e718186 100644 --- a/lib/hibp.go +++ b/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) diff --git a/lib/xkcd.go b/lib/xkcd.go index 9c534ab..020bfb0 100644 --- a/lib/xkcd.go +++ b/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 diff --git a/lib/xkcd_test.go b/lib/xkcd_test.go index bfe4527..708f196 100644 --- a/lib/xkcd_test.go +++ b/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) }