[SECURITY] Enable Delay for First Factor Post #971

james-d-elliott · 2020-05-04T02:47:17Z

Will close #949 if merged.

On startup:
If file provider: hash a randomly generated password of 20 characters with the hash password settings, we time this, plus add a random interval of time between 150-200ms. If this delay isn't above 450ms we randomly assign it a value from the next step. This is handled in FirstFactorPost() which now returns a func with the values calculated prior to that.
Otherwise: set the startup delay to 450-500ms.
When the post comes in we record the time we received it. After the authentication passes or fails, we check the authentication attempt will not be returned early and if it would be, we sleep until it should be returned.

TODO:

figure out why the tests failed when not disabling the delay and fix it, it's likely this is causing other issues
design some tests for this
documentation

authelia · 2020-05-04T02:47:20Z

Artifacts

These changes are published for testing on Buildkite and DockerHub.

Docker Container

docker pull authelia/authelia:security-delay-1fa-logins

* add config item to disable (for tests)

internal/handlers/handler_firstfactor.go

* check for '{CRYPT}' prefix on load instead of during a password check * return const error instead * remove temporary username enumeration measures * fix tests for both the enumeration checks and crypt prefix adjustments * move salt checks to their own function * move argon2id checks to their own function

…logins # Conflicts: # config.template.yml # go.mod # internal/authentication/ldap_user_provider.go # internal/configuration/schema/authentication.go # internal/configuration/validator/const.go # internal/handlers/handler_firstfactor.go # internal/handlers/handler_firstfactor_test.go

The outstanding recommendations are due to be addressed in #959 and #971 respectively.

clems4ever · 2020-05-05T21:08:53Z

config.template.yml

@@ -79,6 +79,10 @@ authentication_backend:
  # Disable both the HTML element and the API for reset password functionality
  disable_reset_password: false

+  # Disable delaying authentication to prevent username enumeration.
+  # Enabling this could allow attackers to find valid usernames. It's highly discouraged to change this to true.
+  disable_delay_auth: false


I'm not sure we really want to leave the option to admins.

clems4ever · 2020-05-05T21:10:28Z

internal/authentication/file_user_provider.go

 	}
 }

 func checkPasswordHashes(database *DatabaseModel) error {
 	for u, v := range database.Users {
+		v.HashedPassword = strings.ReplaceAll(v.HashedPassword, "{CRYPT}", "")


internal/authentication/password_hash.go

clems4ever · 2020-05-05T21:15:04Z

internal/configuration/schema/authentication.go

@@ -35,6 +35,7 @@ type PasswordConfiguration struct {
 // AuthenticationBackendConfiguration represents the configuration related to the authentication backend.
 type AuthenticationBackendConfiguration struct {
 	DisableResetPassword bool                                    `mapstructure:"disable_reset_password"`
+	DisableDelayAuth     bool                                    `mapstructure:"disable_delay_auth"`


As I said, I think we don't want to leave admins with a gun and let them shoot in their feet.

* [CI] Add gocritic linter * Implement gocritic recommendations The outstanding recommendations are due to be addressed in #959 and #971 respectively. * Fix implementation tests * Fix remaining linting issues. * Fix tests. Co-authored-by: Clément Michaud <clement.michaud34@gmail.com>

…logins # Conflicts: # internal/authentication/file_user_provider.go # internal/authentication/password_hash.go # internal/handlers/handler_firstfactor.go # internal/suites/http.go # internal/suites/scenario_backend_protection_test.go

internal/handlers/handler_firstfactor.go

internal/authentication/password_hash.go

nightah · 2020-05-06T07:05:50Z

bootstrap.sh

+else
+  if [ -z "$GOPATH" ] ; then
+    export GOPATH=$(go env GOPATH)
+  else


This else can just be dropped.

nightah · 2020-05-06T07:06:28Z

bootstrap.sh

@@ -20,24 +20,29 @@ fi
 if [[ $(id -g) = 0 ]]; then
  echo "Cannot run as root, defaulting to GID 1000"
  export GROUP_ID=1000
-elif [[ $(uname) == "Darwin" ]]; then
+elif [[ $(uname) == "Darwin" ]] ; then


Can we change this to not normalise for Darwin and instead spit an error suggesting that the Development workflow does not work for macOS?

clems4ever · 2020-05-06T19:42:35Z

internal/handlers/handler_firstfactor.go

+				algorithm, config.File.Password.Iterations,
+				config.File.Password.Memory*1024, config.File.Password.Parallelism,
+				config.File.Password.KeyLength, config.File.Password.SaltLength)
+			duration = time.Since(start) + (time.Duration(rand.Int31n(50) + 150))


the random number should be computed on each request to randomize the delay otherwise it would not bring any value imo, we can just remove it.

clems4ever · 2020-05-06T19:42:59Z

internal/handlers/handler_firstfactor.go

+		}
+
+		if duration < 450*time.Millisecond {
+			duration = time.Duration(rand.Intn(50)+450) * time.Millisecond


Same thing as above, the random number should be generated at each request.

clems4ever · 2020-05-06T20:06:52Z

internal/handlers/handler_firstfactor.go

+	// Automatically grow to the largest detected successful login.
+	if successful {
+		delay := time.Since(receivedTime)
+		if delay > *duration {


duration will never decrease back after an incident in which latency has raised drastically. It increases security overtime but decreases user experience though.

clems4ever · 2020-05-06T20:19:44Z

internal/handlers/handler_firstfactor.go

+			duration = time.Since(start) + (time.Duration(rand.Int31n(50) + 150))
+		}
+
+		if duration < 450*time.Millisecond {


The problem I see with the constant at 450ms is that if the app network is overloaded for instance the latencies of LDAP queries will increase and therefore there will be no delay anymore. If there is no delay the mitigation does not work anymore. It's pretty much the same issue as in my proposal with the threshold being exceeded.

[SECURITY] Enable Delay for First Factor Post

1a10e92

* add config item to disable (for tests)

james-d-elliott force-pushed the security-delay-1fa-logins branch from cd05dba to 1a10e92 Compare May 4, 2020 02:49

james-d-elliott added 3 commits May 4, 2020 13:30

disable delay auth for suites

b5d7511

add debug and warning logs

93159e6

add additional logging

ddd3758

autheliabot reviewed May 4, 2020

View reviewed changes

internal/handlers/handler_firstfactor.go Outdated Show resolved Hide resolved

james-d-elliott force-pushed the security-delay-1fa-logins branch from d5c9bd6 to 695e38d Compare May 4, 2020 06:01

james-d-elliott added 7 commits May 4, 2020 17:10

fix getSalt

64cf379

rename funcs and comment them

e5b96a1

ignore gosec linting on suite https skip verify

63c0e5e

refactor if/else to switch in password_hash.go

dec0981

use auth config

4282a9a

fixup post merge

9315250

nightah added a commit that referenced this pull request May 5, 2020

Implement gocritic recommendations

71c1e4c

The outstanding recommendations are due to be addressed in #959 and #971 respectively.

nightah mentioned this pull request May 5, 2020

[CI] Add gocritic linter #977

Merged

clems4ever requested changes May 5, 2020

View reviewed changes

autheliabot reviewed May 6, 2020

View reviewed changes

add missing indirect go modules and fix merge mistakes

acb9d27

james-d-elliott force-pushed the security-delay-1fa-logins branch from d3f325b to acb9d27 Compare May 6, 2020 06:15

james-d-elliott added 2 commits May 6, 2020 16:27

fix ConfigAlgoToCryptoAlgo

b56069e

remove const userBannedMessage

e6e2f0d

nightah reviewed May 6, 2020

View reviewed changes

bootstrap.sh

else

if [ -z "$GOPATH" ] ; then

export GOPATH=$(go env GOPATH)

else

Copy link

Member

nightah May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This else can just be dropped.

nightah reviewed May 6, 2020

View reviewed changes

Merge branch 'master' into security-delay-1fa-logins

66dc141

clems4ever reviewed May 6, 2020

View reviewed changes

james-d-elliott closed this May 8, 2020

nightah deleted the security-delay-1fa-logins branch May 21, 2020 02:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SECURITY] Enable Delay for First Factor Post #971

[SECURITY] Enable Delay for First Factor Post #971

james-d-elliott commented May 4, 2020 •

edited

authelia bot commented May 4, 2020

clems4ever May 5, 2020

clems4ever May 5, 2020

clems4ever May 5, 2020

nightah May 6, 2020

nightah May 6, 2020

clems4ever May 6, 2020 •

edited

clems4ever May 6, 2020

clems4ever May 6, 2020 •

edited

clems4ever May 6, 2020

[SECURITY] Enable Delay for First Factor Post #971

[SECURITY] Enable Delay for First Factor Post #971

Conversation

james-d-elliott commented May 4, 2020 • edited

authelia bot commented May 4, 2020

Artifacts

Docker Container

clems4ever May 5, 2020

Choose a reason for hiding this comment

clems4ever May 5, 2020

Choose a reason for hiding this comment

clems4ever May 5, 2020

Choose a reason for hiding this comment

nightah May 6, 2020

Choose a reason for hiding this comment

nightah May 6, 2020

Choose a reason for hiding this comment

clems4ever May 6, 2020 • edited

Choose a reason for hiding this comment

clems4ever May 6, 2020

Choose a reason for hiding this comment

clems4ever May 6, 2020 • edited

Choose a reason for hiding this comment

clems4ever May 6, 2020

Choose a reason for hiding this comment

james-d-elliott commented May 4, 2020 •

edited

clems4ever May 6, 2020 •

edited

clems4ever May 6, 2020 •

edited