fix: use a random value for a simulated hash for built-in users (#26205)

coderd/userpassword/userpassword.go

@@ -39,11 +39,12 @@ var (
 	// used.
 	defaultSaltSize = 16
 
-	// The simulated hash is used when trying to simulate password checks for
-	// users that don't exist. It's meant to preserve the timing of the hash
-	// comparison.
+	// The simulated hash is used when comparing against an empty stored hash
+	// (e.g. nonexistent or SSO users). It hashes a random value generated on
+	// first use, so no attacker-supplied password can ever match it. It exists
+	// purely to keep failed comparisons constant-time.
 	simulatedHash = lazy.New(func() string {
-		h, err := Hash("hunter2")
+		h, err := Hash(rand.Text())
 		if err != nil {
 			panic(err)
 		}
@@ -72,10 +73,10 @@ func init() {
 // uses pbkdf2 to ensure FIPS 140-2 compliance. See:
 // https://csrc.nist.gov/csrc/media/templates/cryptographic-module-validation-program/documents/security-policies/140sp2261.pdf
 func Compare(hashed string, password string) (bool, error) {
-	// If the hased password provided is empty, simulate comparing a real hash.
+	// If the hashed password provided is empty, simulate comparing a real hash
+	// to preserve timing. The simulated hash is derived from a random value, so
+	// the comparison below can never succeed.
 	if hashed == "" {
-		// TODO: this seems ripe for creating a vulnerability where
-		// hunter2 can log into any account.
 		hashed = simulatedHash.Load()
 	}
 

coderd/userpassword/userpassword_test.go

@@ -89,6 +89,30 @@ func TestUserPasswordCompare(t *testing.T) {
 			wantErr:            true,
 			wantEqual:          false,
 		},
+		{
+			name:               "EmptyHashHunter2",
+			passwordToValidate: "",
+			password:           "hunter2",
+			shouldHash:         false,
+			wantErr:            false,
+			wantEqual:          false,
+		},
+		{
+			name:               "EmptyHashEmptyPassword",
+			passwordToValidate: "",
+			password:           "",
+			shouldHash:         false,
+			wantErr:            false,
+			wantEqual:          false,
+		},
+		{
+			name:               "EmptyHashArbitraryPassword",
+			passwordToValidate: "",
+			password:           "anyOtherPassword",
+			shouldHash:         false,
+			wantErr:            false,
+			wantEqual:          false,
+		},
 	}
 
 	for _, tt := range tests {

coderd/users_test.go

@@ -21,6 +21,7 @@ import (
 	"github.com/coder/coder/v2/coderd/coderdtest"
 	"github.com/coder/coder/v2/coderd/coderdtest/oidctest"
 	"github.com/coder/coder/v2/coderd/database"
+	"github.com/coder/coder/v2/coderd/database/dbauthz"
 	"github.com/coder/coder/v2/coderd/database/dbfake"
 	"github.com/coder/coder/v2/coderd/database/dbgen"
 	"github.com/coder/coder/v2/coderd/database/dbtime"
@@ -233,6 +234,59 @@ func TestPostLogin(t *testing.T) {
 		require.Equal(t, database.AuditActionLogin, auditor.AuditLogs()[numLogs-1].Action)
 	})
 
+	// "hunter2" was the input of the previous hardcoded simulated hash, which
+	// an empty stored hash wrongly matched; this is a regression test.
+	t.Run("NonexistentUser401", func(t *testing.T) {
+		t.Parallel()
+		client := coderdtest.New(t, nil)
+		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
+		defer cancel()
+
+		_, err := client.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{
+			Email:    "does-not-exist@coder.com",
+			Password: "hunter2",
+		})
+		var apiErr *codersdk.Error
+		require.ErrorAs(t, err, &apiErr)
+		require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode())
+		require.Equal(t, "Incorrect email or password.", apiErr.Message)
+	})
+
+	// Attempting built-in login as an SSO user returns a 401 to avoid
+	// divulging login type.
+	t.Run("SSOReturns401", func(t *testing.T) {
+		t.Parallel()
+		client, db := coderdtest.NewWithDatabase(t, nil)
+		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
+		defer cancel()
+
+		// An SSO user has no password hash stored. Create one directly in the
+		// database since the API requires OIDC to be configured. dbgen.User
+		// substitutes a random hash for an empty one, so clear it explicitly.
+		ssoUser := dbgen.User(t, db, database.User{
+			Email:     "sso-user@coder.com",
+			LoginType: database.LoginTypeOIDC,
+		})
+		//nolint:gocritic // Test setup requires a system context to clear the hash.
+		err := db.UpdateUserHashedPassword(dbauthz.AsSystemRestricted(ctx), database.UpdateUserHashedPasswordParams{
+			ID:             ssoUser.ID,
+			HashedPassword: []byte{},
+		})
+		require.NoError(t, err)
+
+		anonClient := codersdk.New(client.URL)
+		_, err = anonClient.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{
+			Email:    ssoUser.Email,
+			Password: "hunter2",
+		})
+		var apiErr *codersdk.Error
+		require.ErrorAs(t, err, &apiErr)
+		require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode())
+		require.Equal(t, "Incorrect email or password.", apiErr.Message)
+		// The login type must not be leaked.
+		require.NotContains(t, apiErr.Message, string(codersdk.LoginTypeOIDC))
+	})
+
 	t.Run("Suspended", func(t *testing.T) {
 		t.Parallel()
 		auditor := audit.NewMock()