diff --git a/internal/authentication/const.go b/internal/authentication/const.go index 21a1669f..7e38b121 100644 --- a/internal/authentication/const.go +++ b/internal/authentication/const.go @@ -60,7 +60,7 @@ const ( ) // HashingPossibleSaltCharacters represents valid hashing runes. -var HashingPossibleSaltCharacters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789+/") +var HashingPossibleSaltCharacters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789+/" // ErrUserNotFound indicates the user wasn't found in the authentication backend. var ErrUserNotFound = errors.New("user not found") diff --git a/internal/authentication/password_hash.go b/internal/authentication/password_hash.go index 5c1fb5ea..31f78366 100644 --- a/internal/authentication/password_hash.go +++ b/internal/authentication/password_hash.go @@ -126,7 +126,7 @@ func HashPassword(password, salt string, algorithm CryptAlgo, iterations, memory } if salt == "" { - salt = crypt.Base64Encoding.EncodeToString([]byte(utils.RandomString(saltLength, HashingPossibleSaltCharacters))) + salt = crypt.Base64Encoding.EncodeToString(utils.RandomBytes(saltLength, HashingPossibleSaltCharacters, true)) } settings = getCryptSettings(salt, algorithm, iterations, memory, parallelism, keyLength) diff --git a/internal/authentication/password_hash_test.go b/internal/authentication/password_hash_test.go index db23721f..79156ca1 100644 --- a/internal/authentication/password_hash_test.go +++ b/internal/authentication/password_hash_test.go @@ -58,8 +58,7 @@ func TestArgon2idHashSaltValidValues(t *testing.T) { var hash string - data := string(HashingPossibleSaltCharacters) - datas := utils.SliceString(data, 16) + datas := utils.SliceString(HashingPossibleSaltCharacters, 16) for _, salt := range datas { hash, err = HashPassword("password", salt, HashingAlgorithmArgon2id, 1, 8, 1, 32, 16) @@ -74,8 +73,7 @@ func TestSHA512HashSaltValidValues(t *testing.T) { var hash string - data := string(HashingPossibleSaltCharacters) - datas := utils.SliceString(data, 16) + datas := utils.SliceString(HashingPossibleSaltCharacters, 16) for _, salt := range datas { hash, err = HashPassword("password", salt, HashingAlgorithmSHA512, 1000, 0, 0, 0, 16) diff --git a/internal/notification/smtp_notifier.go b/internal/notification/smtp_notifier.go index 11c2cea1..28bb4784 100644 --- a/internal/notification/smtp_notifier.go +++ b/internal/notification/smtp_notifier.go @@ -133,7 +133,7 @@ func (n *SMTPNotifier) compose(recipient, subject, body, htmlBody string) error return err } - boundary := utils.RandomString(30, utils.AlphaNumericCharacters) + boundary := utils.RandomString(30, utils.AlphaNumericCharacters, true) now := time.Now() diff --git a/internal/server/template.go b/internal/server/template.go index 908c2d73..a6e65845 100644 --- a/internal/server/template.go +++ b/internal/server/template.go @@ -13,8 +13,6 @@ import ( "github.com/authelia/authelia/v4/internal/utils" ) -var alphaNumericRunes = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789") - // ServeTemplatedFile serves a templated version of a specified file, // this is utilised to pass information between the backend and frontend // and generate a nonce to support a restrictive CSP while using material-ui. @@ -55,7 +53,7 @@ func ServeTemplatedFile(publicDir, file, rememberMe, resetPassword, session, the } baseURL := scheme + "://" + string(ctx.Request.Host()) + base + "/" - nonce := utils.RandomString(32, alphaNumericRunes) + nonce := utils.RandomString(32, utils.AlphaNumericCharacters, true) switch extension := filepath.Ext(file); extension { case ".html": diff --git a/internal/session/const.go b/internal/session/const.go index 5bb9249e..a4ce709d 100644 --- a/internal/session/const.go +++ b/internal/session/const.go @@ -1,8 +1,13 @@ package session -const userSessionStorerKey = "UserSession" +const ( + testDomain = "example.com" + testExpiration = "40" + testName = "my_session" + testUsername = "john" +) -const testDomain = "example.com" -const testExpiration = "40" -const testName = "my_session" -const testUsername = "john" +const ( + userSessionStorerKey = "UserSession" + randomSessionChars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_!#$%^*" +) diff --git a/internal/session/provider_config.go b/internal/session/provider_config.go index ea7c7f74..8a2db080 100644 --- a/internal/session/provider_config.go +++ b/internal/session/provider_config.go @@ -1,6 +1,7 @@ package session import ( + "crypto/rand" "crypto/tls" "crypto/x509" "fmt" @@ -20,7 +21,15 @@ func NewProviderConfig(configuration schema.SessionConfiguration, certPool *x509 config := session.NewDefaultConfig() config.SessionIDGeneratorFunc = func() []byte { - return []byte(utils.RandomString(30, utils.AlphaNumericCharacters)) + bytes := make([]byte, 32) + + _, _ = rand.Read(bytes) + + for i, b := range bytes { + bytes[i] = randomSessionChars[b%byte(len(randomSessionChars))] + } + + return bytes } // Override the cookie name. @@ -47,7 +56,6 @@ func NewProviderConfig(configuration schema.SessionConfiguration, certPool *x509 // Ignore the error as it will be handled by validator. config.Expiration, _ = utils.ParseDurationString(configuration.Expiration) - // TODO(c.michaud): Make this configurable by giving the list of IPs that are trustable. config.IsSecureFunc = func(*fasthttp.RequestCtx) bool { return true } diff --git a/internal/utils/const.go b/internal/utils/const.go index 73ab546f..4c1ee05e 100644 --- a/internal/utils/const.go +++ b/internal/utils/const.go @@ -56,8 +56,10 @@ var ( reDuration = regexp.MustCompile(`^(?P[1-9]\d*?)(?P[smhdwMy])?$`) ) -// AlphaNumericCharacters are literally just valid alphanumeric chars. -var AlphaNumericCharacters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789") +var ( + // AlphaNumericCharacters are literally just valid alphanumeric chars. + AlphaNumericCharacters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" +) var htmlEscaper = strings.NewReplacer( "&", "&", diff --git a/internal/utils/hashing_test.go b/internal/utils/hashing_test.go index 77805051..4436d41f 100644 --- a/internal/utils/hashing_test.go +++ b/internal/utils/hashing_test.go @@ -23,7 +23,7 @@ func TestShouldHashString(t *testing.T) { assert.Equal(t, "ae448ac86c4e8e4dec645729708ef41873ae79c6dff84eff73360989487f08e5", anotherSum) assert.NotEqual(t, sum, anotherSum) - randomInput := RandomString(40, AlphaNumericCharacters) + randomInput := RandomString(40, AlphaNumericCharacters, false) randomSum := HashSHA256FromString(randomInput) assert.NotEqual(t, randomSum, sum) @@ -40,7 +40,7 @@ func TestShouldHashPath(t *testing.T) { err = os.WriteFile(filepath.Join(dir, "anotherfile"), []byte("another\n"), 0600) assert.NoError(t, err) - err = os.WriteFile(filepath.Join(dir, "randomfile"), []byte(RandomString(40, AlphaNumericCharacters)+"\n"), 0600) + err = os.WriteFile(filepath.Join(dir, "randomfile"), []byte(RandomString(40, AlphaNumericCharacters, true)+"\n"), 0600) assert.NoError(t, err) sum, err := HashSHA256FromPath(filepath.Join(dir, "myfile")) diff --git a/internal/utils/strings.go b/internal/utils/strings.go index ed930705..39ff6248 100644 --- a/internal/utils/strings.go +++ b/internal/utils/strings.go @@ -1,6 +1,7 @@ package utils import ( + crand "crypto/rand" "fmt" "math/rand" "net/url" @@ -139,19 +140,37 @@ func StringSlicesDelta(before, after []string) (added, removed []string) { return added, removed } -// RandomString generate a random string of n characters. -func RandomString(n int, characters []rune) (randomString string) { - rand.Seed(time.Now().UnixNano()) +// RandomString returns a random string with a given length with values from the provided characters. When crypto is set +// to false we use math/rand and when it's set to true we use crypto/rand. The crypto option should always be set to true +// excluding when the task is time sensitive and would not benefit from extra randomness. +func RandomString(n int, characters string, crypto bool) (randomString string) { + return string(RandomBytes(n, characters, crypto)) +} - b := make([]rune, n) - for i := range b { - b[i] = characters[rand.Intn(len(characters))] //nolint:gosec // Likely isn't necessary to use the more expensive crypto/rand for this utility func. +// RandomBytes returns a random []byte with a given length with values from the provided characters. When crypto is set +// to false we use math/rand and when it's set to true we use crypto/rand. The crypto option should always be set to true +// excluding when the task is time sensitive and would not benefit from extra randomness. +func RandomBytes(n int, characters string, crypto bool) (bytes []byte) { + bytes = make([]byte, n) + + if crypto { + _, _ = crand.Read(bytes) + } else { + _, _ = rand.Read(bytes) //nolint:gosec // As this is an option when using this function it's not necessary to be concerned about this. } - return string(b) + for i, b := range bytes { + bytes[i] = characters[b%byte(len(characters))] + } + + return bytes } // StringHTMLEscape escapes chars for a HTML body. func StringHTMLEscape(input string) (output string) { return htmlEscaper.Replace(input) } + +func init() { + rand.Seed(time.Now().UnixNano()) +} diff --git a/internal/utils/strings_test.go b/internal/utils/strings_test.go index b876b72d..5e570b2c 100644 --- a/internal/utils/strings_test.go +++ b/internal/utils/strings_test.go @@ -7,6 +7,17 @@ import ( "github.com/stretchr/testify/require" ) +func TestShouldNotGenerateSameRandomString(t *testing.T) { + randomStringOne := RandomString(10, AlphaNumericCharacters, false) + randomStringTwo := RandomString(10, AlphaNumericCharacters, false) + + randomCryptoStringOne := RandomString(10, AlphaNumericCharacters, true) + randomCryptoStringTwo := RandomString(10, AlphaNumericCharacters, true) + + assert.NotEqual(t, randomStringOne, randomStringTwo) + assert.NotEqual(t, randomCryptoStringOne, randomCryptoStringTwo) +} + func TestShouldDetectAlphaNumericString(t *testing.T) { assert.True(t, IsStringAlphaNumeric("abc")) assert.True(t, IsStringAlphaNumeric("abc123"))