From e95c6a294de822372c35be025c6b33d3ba19f467 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Sat, 2 May 2020 08:32:09 +1000 Subject: [PATCH] [HOTFIX] Prevent Username Enumeration (#950) * [HOTFIX] Prevent Username Enumeration * thanks to TheHllm for identifying the bug: https://github.com/TheHllm * temporarily prevents username enumeration with file auth * proper calculated and very slightly random fix to come * closely replicate behaviour * allow error to bubble up * Synchronize security documentation. Co-authored-by: Clement Michaud --- README.md | 5 ++-- SECURITY.md | 4 +-- internal/authentication/file_user_provider.go | 25 ++++++++++++++++++- .../authentication/file_user_provider_test.go | 25 ++++++++++++++++--- internal/authentication/ldap_user_provider.go | 2 +- internal/authentication/password_hash.go | 17 +++++++++---- 6 files changed, 63 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 9509050b..0318486d 100644 --- a/README.md +++ b/README.md @@ -103,8 +103,9 @@ Authelia takes security very seriously. We follow the rule of [responsible disclosure](https://en.wikipedia.org/wiki/Responsible_disclosure), and we encourage the community to as well. -Would you like to report any vulnerability discovered in Authelia, please first contact -**clems4ever** on [Matrix](https://riot.im/app/#/room/#authelia:matrix.org) or by + +If you discover a vulnerability in Authelia, please first contact **clems4ever** on +[Matrix](https://riot.im/app/#/room/#authelia:matrix.org) or by [email](mailto:clement.michaud34@gmail.com). For details about security measures implemented in Authelia, please follow diff --git a/SECURITY.md b/SECURITY.md index fede542d..446f56e8 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -4,8 +4,8 @@ Authelia takes security very seriously. We follow the rule of [responsible disclosure](https://en.wikipedia.org/wiki/Responsible_disclosure), and we encourage the community to as well. -Would you like to report any vulnerability discovered in Authelia, please first contact -**clems4ever** on [Matrix](https://riot.im/app/#/room/#authelia:matrix.org) or by +If you discover a vulnerability in Authelia, please first contact **clems4ever** on +[Matrix](https://riot.im/app/#/room/#authelia:matrix.org) or by [email](mailto:clement.michaud34@gmail.com). For details about security measures implemented in Authelia, please follow diff --git a/internal/authentication/file_user_provider.go b/internal/authentication/file_user_provider.go index b2467fdb..c0e0c860 100644 --- a/internal/authentication/file_user_provider.go +++ b/internal/authentication/file_user_provider.go @@ -8,9 +8,11 @@ import ( "sync" "github.com/asaskevich/govalidator" + "github.com/simia-tech/crypt" "gopkg.in/yaml.v2" "github.com/authelia/authelia/internal/configuration/schema" + "github.com/authelia/authelia/internal/utils" ) // FileUserProvider is a provider reading details from a file. @@ -18,6 +20,9 @@ type FileUserProvider struct { configuration *schema.FileAuthenticationBackendConfiguration database *DatabaseModel lock *sync.Mutex + + // TODO: Remove this. This is only here to temporarily fix the username enumeration security flaw in #949. + fakeHash string } // UserDetailsModel is the model of user details in the file database. @@ -46,10 +51,23 @@ func NewFileUserProvider(configuration *schema.FileAuthenticationBackendConfigur panic(err.Error()) } + // TODO: Remove this. This is only here to temporarily fix the username enumeration security flaw in #949. + // This generates a hash that should be usable to do a fake CheckUserPassword + algorithm := configuration.Password.Algorithm + if configuration.Password.Algorithm == "sha512" { + algorithm = HashingAlgorithmSHA512 + } + settings := getCryptSettings(utils.RandomString(configuration.Password.SaltLength, HashingPossibleSaltCharacters), + algorithm, configuration.Password.Iterations, configuration.Password.Memory*1024, configuration.Password.Parallelism, + configuration.Password.KeyLength) + data := crypt.Base64Encoding.EncodeToString([]byte(utils.RandomString(configuration.Password.KeyLength, HashingPossibleSaltCharacters))) + fakeHash := fmt.Sprintf("%s$%s", settings, data) + return &FileUserProvider{ configuration: configuration, database: database, lock: &sync.Mutex{}, + fakeHash: fakeHash, } } @@ -95,7 +113,12 @@ func (p *FileUserProvider) CheckUserPassword(username string, password string) ( } return ok, nil } - return false, fmt.Errorf("User '%s' does not exist in database", username) + + // TODO: Remove this. This is only here to temporarily fix the username enumeration security flaw in #949. + hashedPassword := strings.ReplaceAll(p.fakeHash, "{CRYPT}", "") + _, err := CheckPassword(password, hashedPassword) + + return false, err } // GetDetails retrieve the groups a user belongs to. diff --git a/internal/authentication/file_user_provider_test.go b/internal/authentication/file_user_provider_test.go index c60f3ee1..9d9297b8 100644 --- a/internal/authentication/file_user_provider_test.go +++ b/internal/authentication/file_user_provider_test.go @@ -68,14 +68,26 @@ func TestShouldCheckUserPasswordIsWrong(t *testing.T) { }) } -func TestShouldCheckUserPasswordOfUnexistingUser(t *testing.T) { +func TestShouldCheckUserPasswordIsWrongForEnumerationCompare(t *testing.T) { WithDatabase(UserDatabaseContent, func(path string) { config := DefaultFileAuthenticationBackendConfiguration config.Path = path provider := NewFileUserProvider(&config) - _, err := provider.CheckUserPassword("fake", "password") - assert.Error(t, err) - assert.Equal(t, "User 'fake' does not exist in database", err.Error()) + + ok, err := provider.CheckUserPassword("enumeration", "wrong_password") + assert.NoError(t, err) + assert.False(t, ok) + }) +} + +func TestShouldCheckUserPasswordOfUserThatDoesNotExist(t *testing.T) { + WithDatabase(UserDatabaseContent, func(path string) { + config := DefaultFileAuthenticationBackendConfiguration + config.Path = path + provider := NewFileUserProvider(&config) + ok, err := provider.CheckUserPassword("fake", "password") + assert.NoError(t, err) + assert.Equal(t, false, ok) }) } @@ -257,6 +269,11 @@ users: james: password: "{CRYPT}$6$rounds=500000$jgiCMRyGXzoqpxS3$w2pJeZnnH8bwW3zzvoMWtTRfQYsHbWbD/hquuQ5vUeIyl9gdwBIt6RWk2S6afBA0DPakbeWgD/4SZPiS0hYtU/" email: james.dean@authelia.com + + + enumeration: + password: "$argon2id$v=19$m=131072,p=8$BpLnfgDsc2WD8F2q$O126GHPeZ5fwj7OLSs7PndXsTbje76R+QW9/EGfhkJg" + email: james.dean@authelia.com `) var MalformedUserDatabaseContent = []byte(` diff --git a/internal/authentication/ldap_user_provider.go b/internal/authentication/ldap_user_provider.go index 051bacf9..9300f2e1 100644 --- a/internal/authentication/ldap_user_provider.go +++ b/internal/authentication/ldap_user_provider.go @@ -48,7 +48,7 @@ func (p *LDAPUserProvider) connect(userDN string, password string) (LDAPConnecti if url.Scheme == "ldaps" { logging.Logger().Trace("LDAP client starts a TLS session") conn, err := p.connectionFactory.DialTLS("tcp", url.Host, &tls.Config{ - InsecureSkipVerify: p.configuration.SkipVerify, + InsecureSkipVerify: p.configuration.SkipVerify, //nolint:gosec }) if err != nil { return nil, err diff --git a/internal/authentication/password_hash.go b/internal/authentication/password_hash.go index 1ab17e55..16eacc1d 100644 --- a/internal/authentication/password_hash.go +++ b/internal/authentication/password_hash.go @@ -127,11 +127,7 @@ func HashPassword(password, salt, algorithm string, iterations, memory, parallel if salt == "" { salt = utils.RandomString(saltLength, HashingPossibleSaltCharacters) } - if algorithm == HashingAlgorithmArgon2id { - settings, _ = crypt.Argon2idSettings(memory, iterations, parallelism, keyLength, salt) - } else if algorithm == HashingAlgorithmSHA512 { - settings = fmt.Sprintf("$6$rounds=%d$%s", iterations, salt) - } + settings = getCryptSettings(salt, algorithm, iterations, memory, parallelism, keyLength) // This error can be ignored because we check for it before a user gets here hash, _ = crypt.Crypt(password, settings) @@ -150,3 +146,14 @@ func CheckPassword(password, hash string) (ok bool, err error) { } return hash == expectedHash, nil } + +func getCryptSettings(salt, algorithm string, iterations, memory, parallelism, keyLength int) (settings string) { + if algorithm == HashingAlgorithmArgon2id { + settings, _ = crypt.Argon2idSettings(memory, iterations, parallelism, keyLength, salt) + } else if algorithm == HashingAlgorithmSHA512 { + settings = fmt.Sprintf("$6$rounds=%d$%s", iterations, salt) + } else { + panic("invalid password hashing algorithm provided") + } + return settings +}