CVE-2023-47111: Concurrent Execution using Shared Resource with Improper Synchronization ('Race Condition') (CWE-362)
value: ZITADEL provides identity infrastructure. ZITADEL provides administrators the possibility to define a `Lockout Policy` with a maximum amount of failed password check attempts. On every failed password check, the amount of failed checks is compared against the configured maximum. Exceeding the limit, will lock the user and prevent further authentication. In the affected implementation it was possible for an attacker to start multiple parallel password checks, giving him the possibility to try out more combinations than configured in the `Lockout Policy`. This vulnerability has been patched in versions 2.40.5 and 2.38.3.

CWE description: The product contains a code sequence that can run concurrently with other code, and the code sequence requires temporary, exclusive access to a shared resource, but a timing window exists in which the shared resource can be modified by another code sequence that is operating concurrently.
CVSS v3 base score: 3.7
Published: 2023-11-08T22:15Z
Repository: https://github.com/zitadel/zitadel
Language: Go
Files changed: internal/command/user_human_password.go, internal/command/user_human_password_model.go, internal/command/user_human_password_test.go, internal/query/user_password.go
Commit date: 2023-11-08 14:19:13 +0100
Commit message: Merge pull request from GHSA-7h8m-vrxx-vr4m

* fix: handle locking policy correctly for multiple simultaneous password checks

* recheck events
Diff: diff --git a/internal/command/user_human_password.go b/internal/command/user_human_password.go
index 487907212..fa87770a5 100644
--- a/internal/command/user_human_password.go
+++ b/internal/command/user_human_password.go
@@ -231,22 +231,34 @@ func (c *Commands) HumanCheckPassword(ctx context.Context, orgID, userID, passwo
 		return err
 	}
 	if wm.UserState == domain.UserStateUnspecified || wm.UserState == domain.UserStateDeleted {
 		return caos_errs.ThrowPreconditionFailed(nil, "COMMAND-3n77z", "Errors.User.NotFound")
 	}
+	if wm.UserState == domain.UserStateLocked {
+		return caos_errs.ThrowPreconditionFailed(nil, "COMMAND-JLK35", "Errors.User.Locked")
+	}
 
 	if wm.EncodedHash == "" {
-		return caos_errs.ThrowPreconditionFailed(nil, "COMMAND-3n77z", "Errors.User.Password.NotSet")
+		return caos_errs.ThrowPreconditionFailed(nil, "COMMAND-3nJ4t", "Errors.User.Password.NotSet")
 	}
 
 	userAgg := UserAggregateFromWriteModel(&wm.WriteModel)
 	ctx, spanPasswordComparison := tracing.NewNamedSpan(ctx, "passwap.Verify")
 	updated, err := c.userPasswordHasher.Verify(wm.EncodedHash, password)
 	spanPasswordComparison.EndWithError(err)
 	err = convertPasswapErr(err)
-
 	commands := make([]eventstore.Command, 0, 2)
+
+	// recheck for additional events (failed password checks or locks)
+	recheckErr := c.eventstore.FilterToQueryReducer(ctx, wm)
+	if recheckErr != nil {
+		return recheckErr
+	}
+	if wm.UserState == domain.UserStateLocked {
+		return caos_errs.ThrowPreconditionFailed(nil, "COMMAND-SFA3t", "Errors.User.Locked")
+	}
+
 	if err == nil {
 		commands = append(commands, user.NewHumanPasswordCheckSucceededEvent(ctx, userAgg, authRequestDomainToAuthRequestInfo(authRequest)))
 		if updated != "" {
 			commands = append(commands, user.NewHumanPasswordHashUpdatedEvent(ctx, userAgg, updated))
 		}
@@ -257,11 +269,10 @@ func (c *Commands) HumanCheckPassword(ctx context.Context, orgID, userID, passwo
 	commands = append(commands, user.NewHumanPasswordCheckFailedEvent(ctx, userAgg, authRequestDomainToAuthRequestInfo(authRequest)))
 	if lockoutPolicy != nil && lockoutPolicy.MaxPasswordAttempts > 0 {
 		if wm.PasswordCheckFailedCount+1 >= lockoutPolicy.MaxPasswordAttempts {
 			commands = append(commands, user.NewUserLockedEvent(ctx, userAgg))
 		}
-
 	}
 	_, pushErr := c.eventstore.Push(ctx, commands...)
 	logging.OnError(pushErr).Error("error create password check failed event")
 	return err
 }
diff --git a/internal/command/user_human_password_model.go b/internal/command/user_human_password_model.go
index c4064ba15..ec4d11da2 100644
--- a/internal/command/user_human_password_model.go
+++ b/internal/command/user_human_password_model.go
@@ -63,12 +63,17 @@ func (wm *HumanPasswordWriteModel) Reduce() error {
 			}
 		case *user.HumanPasswordCheckFailedEvent:
 			wm.PasswordCheckFailedCount += 1
 		case *user.HumanPasswordCheckSucceededEvent:
 			wm.PasswordCheckFailedCount = 0
+		case *user.UserLockedEvent:
+			wm.UserState = domain.UserStateLocked
 		case *user.UserUnlockedEvent:
 			wm.PasswordCheckFailedCount = 0
+			if wm.UserState != domain.UserStateDeleted {
+				wm.UserState = domain.UserStateActive
+			}
 		case *user.UserRemovedEvent:
 			wm.UserState = domain.UserStateDeleted
 		case *user.HumanPasswordHashUpdatedEvent:
 			wm.EncodedHash = e.EncodedHash
 		}
@@ -90,10 +95,11 @@ func (wm *HumanPasswordWriteModel) Query() *eventstore.SearchQueryBuilder {
 			user.HumanEmailVerifiedType,
 			user.HumanPasswordCheckFailedType,
 			user.HumanPasswordCheckSucceededType,
 			user.HumanPasswordHashUpdatedType,
 			user.UserRemovedType,
+			user.UserLockedType,
 			user.UserUnlockedType,
 			user.UserV1AddedType,
 			user.UserV1RegisteredType,
 			user.UserV1InitialCodeAddedType,
 			user.UserV1InitializedCheckSucceededType,
@@ -106,7 +112,10 @@ func (wm *HumanPasswordWriteModel) Query() *eventstore.SearchQueryBuilder {
 		Builder()
 
 	if wm.ResourceOwner != "" {
 		query.ResourceOwner(wm.ResourceOwner)
 	}
+	if wm.WriteModel.ProcessedSequence != 0 {
+		query.SequenceGreater(wm.WriteModel.ProcessedSequence)
+	}
 	return query
 }
diff --git a/internal/command/user_human_password_test.go b/internal/command/user_human_password_test.go
index 2af86bc72..a90d97250 100644
--- a/internal/command/user_human_password_test.go
+++ b/internal/command/user_human_password_test.go
@@ -1220,10 +1220,72 @@ func TestCommandSide_CheckPassword(t *testing.T) {
 			},
 			res: res{
 				err: caos_errs.IsPreconditionFailed,
 			},
 		},
+		{
+			name: "user locked, precondition error",
+			fields: fields{
+				eventstore: eventstoreExpect(
+					t,
+					expectFilter(
+						eventFromEventPusher(
+							org.NewLoginPolicyAddedEvent(context.Background(),
+								&org.NewAggregate("org1").Aggregate,
+								true,
+								false,
+								false,
+								false,
+								false,
+								false,
+								false,
+								false,
+								false,
+								false,
+								domain.PasswordlessTypeNotAllowed,
+								"",
+								time.Hour*1,
+								time.Hour*2,
+								time.Hour*3,
+								time.Hour*4,
+								time.Hour*5,
+							),
+						),
+					),
+					expectFilter(
+						eventFromEventPusher(
+							user.NewHumanAddedEvent(context.Background(),
+								&user.NewAggregate("user1", "org1").Aggregate,
+								"username",
+								"firstname",
+								"lastname",
+								"nickname",
+								"displayname",
+								language.German,
+								domain.GenderUnspecified,
+								"email@test.ch",
+								true,
+							),
+						),
+						eventFromEventPusher(
+							user.NewUserLockedEvent(context.Background(),
+								&user.NewAggregate("user1", "org1").Aggregate,
+							),
+						),
+					),
+				),
+			},
+			args: args{
+				ctx:           context.Background(),
+				userID:        "user1",
+				resourceOwner: "org1",
+				password:      "password",
+			},
+			res: res{
+				err: caos_errs.IsPreconditionFailed,
+			},
+		},
 		{
 			name: "existing password empty, precondition error",
 			fields: fields{
 				eventstore: eventstoreExpect(
 					t,
@@ -1334,10 +1396,11 @@ func TestCommandSide_CheckPassword(t *testing.T) {
 								&user.NewAggregate("user1", "org1").Aggregate,
 								"$plain$x$password",
 								false,
 								"")),
 					),
+					expectFilter(),
 					expectPush(
 						user.NewHumanPasswordCheckFailedEvent(context.Background(),
 							&user.NewAggregate("user1", "org1").Aggregate,
 							&user.AuthRequestInfo{
 								ID:          "request1",
@@ -1415,12 +1478,14 @@ func TestCommandSide_CheckPassword(t *testing.T) {
 						eventFromEventPusher(
 							user.NewHumanPasswordChangedEvent(context.Background(),
 								&user.NewAggregate("user1", "org1").Aggregate,
 								"$plain$x$password",
 								false,
-								"")),
+								""),
+						),
 					),
+					expectFilter(),
 					expectPush(
 						user.NewHumanPasswordCheckFailedEvent(context.Background(),
 							&user.NewAggregate("user1", "org1").Aggregate,
 							&user.AuthRequestInfo{
 								ID:          "request1",
@@ -1505,10 +1570,11 @@ func TestCommandSide_CheckPassword(t *testing.T) {
 								&user.NewAggregate("user1", "org1").Aggregate,
 								"$plain$x$password",
 								false,
 								"")),
 					),
+					expectFilter(),
 					expectPush(
 						user.NewHumanPasswordCheckSucceededEvent(context.Background(),
 							&user.NewAggregate("user1", "org1").Aggregate,
 							&user.AuthRequestInfo{
 								ID:          "request1",
@@ -1585,10 +1651,11 @@ func TestCommandSide_CheckPassword(t *testing.T) {
 								&user.NewAggregate("user1", "org1").Aggregate,
 								"$plain$v$password",
 								false,
 								"")),
 					),
+					expectFilter(),
 					expectPush(
 						user.NewHumanPasswordCheckSucceededEvent(
 							context.Background(),
 							&user.NewAggregate("user1", "org1").Aggregate,
 							&user.AuthRequestInfo{
@@ -1614,10 +1681,90 @@ func TestCommandSide_CheckPassword(t *testing.T) {
 					AgentID: "agent1",
 				},
 			},
 			res: res{},
 		},
+		{
+			name: "check password ok, locked in the mean time",
+			fields: fields{
+				eventstore: eventstoreExpect(
+					t,
+					expectFilter(
+						eventFromEventPusher(
+							org.NewLoginPolicyAddedEvent(context.Background(),
+								&org.NewAggregate("org1").Aggregate,
+								true,
+								false,
+								false,
+								false,
+								false,
+								false,
+								false,
+								false,
+								false,
+								false,
+								domain.PasswordlessTypeNotAllowed,
+								"",
+								time.Hour*1,
+								time.Hour*2,
+								time.Hour*3,
+								time.Hour*4,
+								time.Hour*5,
+							),
+						),
+					),
+					expectFilter(
+						eventFromEventPusher(
+							user.NewHumanAddedEvent(context.Background(),
+								&user.NewAggregate("user1", "org1").Aggregate,
+								"username",
+								"firstname",
+								"lastname",
+								"nickname",
+								"displayname",
+								language.German,
+								domain.GenderUnspecified,
+								"email@test.ch",
+								true,
+							),
+						),
+						eventFromEventPusher(
+							user.NewHumanEmailVerifiedEvent(context.Background(),
+								&user.NewAggregate("user1", "org1").Aggregate,
+							),
+						),
+						eventFromEventPusher(
+							user.NewHumanPasswordChangedEvent(context.Background(),
+								&user.NewAggregate("user1", "org1").Aggregate,
+								"$plain$x$password",
+								false,
+								"")),
+					),
+					expectFilter(
+						eventFromEventPusher(
+							user.NewUserLockedEvent(context.Background(),
+								&user.NewAggregate("user1", "org1").Aggregate,
+							),
+						),
+					),
+				),
+				userPasswordHasher: mockPasswordHasher("x"),
+			},
+			args: args{
+				ctx:           context.Background(),
+				userID:        "user1",
+				resourceOwner: "org1",
+				password:      "password",
+				authReq: &domain.AuthRequest{
+					ID:      "request1",
+					AgentID: "agent1",
+				},
+			},
+			res: res{
+				err: caos_errs.IsPreconditionFailed,
+			},
+		},
 		{
 			name: "regression test old version event",
 			fields: fields{
 				eventstore: eventstoreExpect(
 					t,
@@ -1680,10 +1827,11 @@ func TestCommandSide_CheckPassword(t *testing.T) {
 								},
 								ChangeRequired: false,
 							},
 						),
 					),
+					expectFilter(),
 					expectPush(
 						user.NewHumanPasswordCheckSucceededEvent(context.Background(),
 							&user.NewAggregate("user1", "org1").Aggregate,
 							&user.AuthRequestInfo{
 								ID:          "request1",
diff --git a/internal/query/user_password.go b/internal/query/user_password.go
index 0e9da8a5e..5f991cf00 100644
--- a/internal/query/user_password.go
+++ b/internal/query/user_password.go
@@ -98,12 +98,17 @@ func (wm *HumanPasswordReadModel) Reduce() error {
 			}
 		case *user.HumanPasswordCheckFailedEvent:
 			wm.PasswordCheckFailedCount += 1
 		case *user.HumanPasswordCheckSucceededEvent:
 			wm.PasswordCheckFailedCount = 0
+		case *user.UserLockedEvent:
+			wm.UserState = domain.UserStateLocked
 		case *user.UserUnlockedEvent:
 			wm.PasswordCheckFailedCount = 0
+			if wm.UserState != domain.UserStateDeleted {
+				wm.UserState = domain.UserStateActive
+			}
 		case *user.UserRemovedEvent:
 			wm.UserState = domain.UserStateDeleted
 		case *user.HumanPasswordHashUpdatedEvent:
 			wm.EncodedHash = e.EncodedHash
 		}
@@ -127,10 +132,11 @@ func (wm *HumanPasswordReadModel) Query() *eventstore.SearchQueryBuilder {
 			user.HumanEmailVerifiedType,
 			user.HumanPasswordCheckFailedType,
 			user.HumanPasswordCheckSucceededType,
 			user.HumanPasswordHashUpdatedType,
 			user.UserRemovedType,
+			user.UserLockedType,
 			user.UserUnlockedType,
 			user.UserV1AddedType,
 			user.UserV1RegisteredType,
 			user.UserV1InitialCodeAddedType,
 			user.UserV1InitializedCheckSucceededType,

Vulnerable code: 		return caos_errs.ThrowPreconditionFailed(nil, "COMMAND-3n77z", "Errors.User.Password.NotSet")


								"")),
Fixed code: 	if wm.UserState == domain.UserStateLocked {
		return caos_errs.ThrowPreconditionFailed(nil, "COMMAND-JLK35", "Errors.User.Locked")
	}
		return caos_errs.ThrowPreconditionFailed(nil, "COMMAND-3nJ4t", "Errors.User.Password.NotSet")

	// recheck for additional events (failed password checks or locks)
	recheckErr := c.eventstore.FilterToQueryReducer(ctx, wm)
	if recheckErr != nil {
		return recheckErr
	}
	if wm.UserState == domain.UserStateLocked {
		return caos_errs.ThrowPreconditionFailed(nil, "COMMAND-SFA3t", "Errors.User.Locked")
	}

		case *user.UserLockedEvent:
			wm.UserState = domain.UserStateLocked
			if wm.UserState != domain.UserStateDeleted {
				wm.UserState = domain.UserStateActive
			}
			user.UserLockedType,
	if wm.WriteModel.ProcessedSequence != 0 {
		query.SequenceGreater(wm.WriteModel.ProcessedSequence)
	}
		{
			name: "user locked, precondition error",
			fields: fields{
				eventstore: eventstoreExpect(
					t,
					expectFilter(
						eventFromEventPusher(
							org.NewLoginPolicyAddedEvent(context.Background(),
								&org.NewAggregate("org1").Aggregate,
								true,
								false,
								false,
								false,
								false,
								false,
								false,
								false,
								false,
								false,
								domain.PasswordlessTypeNotAllowed,
								"",
								time.Hour*1,
								time.Hour*2,
								time.Hour*3,
								time.Hour*4,
								time.Hour*5,
							),
						),
					),
					expectFilter(
						eventFromEventPusher(
							user.NewHumanAddedEvent(context.Background(),
								&user.NewAggregate("user1", "org1").Aggregate,
								"username",
								"firstname",
								"lastname",
								"nickname",
								"displayname",
								language.German,
								domain.GenderUnspecified,
								"email@test.ch",
								true,
							),
						),
						eventFromEventPusher(
							user.NewUserLockedEvent(context.Background(),
								&user.NewAggregate("user1", "org1").Aggregate,
							),
						),
					),
				),
			},
			args: args{
				ctx:           context.Background(),
				userID:        "user1",
				resourceOwner: "org1",
				password:      "password",
			},
			res: res{
				err: caos_errs.IsPreconditionFailed,
			},
		},
					expectFilter(),
								""),
						),
					expectFilter(),
					expectFilter(),
					expectFilter(),
		{
			name: "check password ok, locked in the mean time",
			fields: fields{
				eventstore: eventstoreExpect(
					t,
					expectFilter(
						eventFromEventPusher(
							org.NewLoginPolicyAddedEvent(context.Background(),
								&org.NewAggregate("org1").Aggregate,
								true,
								false,
								false,
								false,
								false,
								false,
								false,
								false,
								false,
								false,
								domain.PasswordlessTypeNotAllowed,
								"",
								time.Hour*1,
								time.Hour*2,
								time.Hour*3,
								time.Hour*4,
								time.Hour*5,
							),
						),
					),
					expectFilter(
						eventFromEventPusher(
							user.NewHumanAddedEvent(context.Background(),
								&user.NewAggregate("user1", "org1").Aggregate,
								"username",
								"firstname",
								"lastname",
								"nickname",
								"displayname",
								language.German,
								domain.GenderUnspecified,
								"email@test.ch",
								true,
							),
						),
						eventFromEventPusher(
							user.NewHumanEmailVerifiedEvent(context.Background(),
								&user.NewAggregate("user1", "org1").Aggregate,
							),
						),
						eventFromEventPusher(
							user.NewHumanPasswordChangedEvent(context.Background(),
								&user.NewAggregate("user1", "org1").Aggregate,
								"$plain$x$password",
								false,
								"")),
					),
					expectFilter(
						eventFromEventPusher(
							user.NewUserLockedEvent(context.Background(),
								&user.NewAggregate("user1", "org1").Aggregate,
							),
						),
					),
				),
				userPasswordHasher: mockPasswordHasher("x"),
			},
			args: args{
				ctx:           context.Background(),
				userID:        "user1",
				resourceOwner: "org1",
				password:      "password",
				authReq: &domain.AuthRequest{
					ID:      "request1",
					AgentID: "agent1",
				},
			},
			res: res{
				err: caos_errs.IsPreconditionFailed,
			},
		},
					expectFilter(),
		case *user.UserLockedEvent:
			wm.UserState = domain.UserStateLocked
			if wm.UserState != domain.UserStateDeleted {
				wm.UserState = domain.UserStateActive
			}
			user.UserLockedType,