Skip to content

Commit 6879532

Browse files
jdomeracki-codersreyaf0ssel
authored
fix: use a random value for a simulated hash for built-in users (#26205) (#26257)
Cherry-pick backport to `release/2.34`. Co-authored-by: Jon Ayers <jon@coder.com> Co-authored-by: Garrett Delfosse <delfossegarrett@gmail.com>
1 parent b78ec31 commit 6879532

3 files changed

Lines changed: 86 additions & 7 deletions

File tree

‎coderd/userpassword/userpassword.go‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -39,11 +39,12 @@ var (
3939
// used.
4040
defaultSaltSize = 16
4141

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

‎coderd/userpassword/userpassword_test.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,30 @@ func TestUserPasswordCompare(t *testing.T) {
8989
wantErr: true,
9090
wantEqual: false,
9191
},
92+
{
93+
name: "EmptyHashHunter2",
94+
passwordToValidate: "",
95+
password: "hunter2",
96+
shouldHash: false,
97+
wantErr: false,
98+
wantEqual: false,
99+
},
100+
{
101+
name: "EmptyHashEmptyPassword",
102+
passwordToValidate: "",
103+
password: "",
104+
shouldHash: false,
105+
wantErr: false,
106+
wantEqual: false,
107+
},
108+
{
109+
name: "EmptyHashArbitraryPassword",
110+
passwordToValidate: "",
111+
password: "anyOtherPassword",
112+
shouldHash: false,
113+
wantErr: false,
114+
wantEqual: false,
115+
},
92116
}
93117

94118
for _, tt := range tests {

‎coderd/users_test.go‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"github.com/coder/coder/v2/coderd/coderdtest"
2222
"github.com/coder/coder/v2/coderd/coderdtest/oidctest"
2323
"github.com/coder/coder/v2/coderd/database"
24+
"github.com/coder/coder/v2/coderd/database/dbauthz"
2425
"github.com/coder/coder/v2/coderd/database/dbfake"
2526
"github.com/coder/coder/v2/coderd/database/dbgen"
2627
"github.com/coder/coder/v2/coderd/database/dbtime"
@@ -233,6 +234,59 @@ func TestPostLogin(t *testing.T) {
233234
require.Equal(t, database.AuditActionLogin, auditor.AuditLogs()[numLogs-1].Action)
234235
})
235236

237+
// "hunter2" was the input of the previous hardcoded simulated hash, which
238+
// an empty stored hash wrongly matched; this is a regression test.
239+
t.Run("NonexistentUser401", func(t *testing.T) {
240+
t.Parallel()
241+
client := coderdtest.New(t, nil)
242+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
243+
defer cancel()
244+
245+
_, err := client.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{
246+
Email: "does-not-exist@coder.com",
247+
Password: "hunter2",
248+
})
249+
var apiErr *codersdk.Error
250+
require.ErrorAs(t, err, &apiErr)
251+
require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode())
252+
require.Equal(t, "Incorrect email or password.", apiErr.Message)
253+
})
254+
255+
// Attempting built-in login as an SSO user returns a 401 to avoid
256+
// divulging login type.
257+
t.Run("SSOReturns401", func(t *testing.T) {
258+
t.Parallel()
259+
client, db := coderdtest.NewWithDatabase(t, nil)
260+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
261+
defer cancel()
262+
263+
// An SSO user has no password hash stored. Create one directly in the
264+
// database since the API requires OIDC to be configured. dbgen.User
265+
// substitutes a random hash for an empty one, so clear it explicitly.
266+
ssoUser := dbgen.User(t, db, database.User{
267+
Email: "sso-user@coder.com",
268+
LoginType: database.LoginTypeOIDC,
269+
})
270+
//nolint:gocritic // Test setup requires a system context to clear the hash.
271+
err := db.UpdateUserHashedPassword(dbauthz.AsSystemRestricted(ctx), database.UpdateUserHashedPasswordParams{
272+
ID: ssoUser.ID,
273+
HashedPassword: []byte{},
274+
})
275+
require.NoError(t, err)
276+
277+
anonClient := codersdk.New(client.URL)
278+
_, err = anonClient.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{
279+
Email: ssoUser.Email,
280+
Password: "hunter2",
281+
})
282+
var apiErr *codersdk.Error
283+
require.ErrorAs(t, err, &apiErr)
284+
require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode())
285+
require.Equal(t, "Incorrect email or password.", apiErr.Message)
286+
// The login type must not be leaked.
287+
require.NotContains(t, apiErr.Message, string(codersdk.LoginTypeOIDC))
288+
})
289+
236290
t.Run("Suspended", func(t *testing.T) {
237291
t.Parallel()
238292
auditor := audit.NewMock()

0 commit comments

Comments
 (0)