From d5d17d554613e0b0c87d5b52956697f476000f0b Mon Sep 17 00:00:00 2001 From: Gary Date: Wed, 7 Oct 2026 19:57:51 +0200 Subject: [PATCH] fix(auth): store passwords as argon2id instead of unsalted MD5 New accounts, password changes and the -ru reset now store an argon2id hash (PHC string, 19 MiB, 2 passes, 1 lane). An account that still holds the legacy MD5 keeps signing in, and its first successful sign-in replaces the hash; the swap is conditional on the row still holding the hash it read, so a password change racing a sign-in is not undone. Hashing is limited to two at a time so parallel requests on endpoints without a rate limit cannot ask for gigabytes. A release older than this one cannot verify an argon2id hash: roll back with casaos-user-service -ru -user . --- README.md | 4 +- main.go | 2 +- pkg/utils/encryption/password.go | 126 ++++++++++++++++++++++++ pkg/utils/encryption/password_test.go | 134 ++++++++++++++++++++++++++ route/v1/user.go | 24 ++++- route/v1_2fa_test.go | 2 +- route/v1_password_test.go | 107 ++++++++++++++++++++ service/model/o_user.go | 2 +- service/user.go | 9 ++ 9 files changed, 401 insertions(+), 9 deletions(-) create mode 100644 pkg/utils/encryption/password.go create mode 100644 pkg/utils/encryption/password_test.go create mode 100644 route/v1_password_test.go diff --git a/README.md b/README.md index 9a40b69..c3e72e9 100644 --- a/README.md +++ b/README.md @@ -33,7 +33,9 @@ A user can protect the account with a TOTP authenticator (RFC 6238, SHA-1, six d | `POST /v1/users/2fa/disable` | access token | `{code}` or `{password}` | `200`. `400` with 4000 (neither field, or both), 10015 (wrong code or password), 10017 (not enabled). | | `GET /v1/users/current` | access token | — | unchanged, `data` gains `totp_enabled`. | -The pre-auth token is an ES256 JWT with `iss: "2fa"` and a five-minute lifetime, signed by a second key pair that is never published in the JWKS, so no other service — and no other route here — accepts it as an access token. `/2fa/verify` checks the signature against that key, then the issuer and expiry. It sits outside the service-wide login budget on purpose: without a pre-auth token, which only a correct password on the limited `/login` produces, a request costs one signature check and nothing else, and with one the attempts are bounded by the per-user budget and the token's five minutes, so a global budget there would only let anyone starve `/login` with garbage. Every factor or password check on `/2fa/*` costs one token of a per-user budget of five a minute, correct answers included; the token is taken before the check, so concurrent attempts cannot share one. A code is accepted for the current 30 s step and its two neighbours, and each step is accepted once: the last accepted step is stored, so a captured code cannot be replayed within its window. The step is recorded with a conditional update (`totp_last_step < step`), and a recovery code is removed with the same compare-and-set on the stored list, so two requests carrying one captured code cannot both pass. Every other write the routes make to the four 2FA columns is keyed the same way on the row as it was read: `setup` stores its secret only while 2FA is still off, `enable` and the clearing on login only while that pending secret is still there, `disable` only against the enrolment the code or password was checked on; a `2fa/*` write that changes no row is refused (10016 or 10017), a login whose clear changes no row re-reads the row and goes by what it finds: it stops at the factor if 2FA was enabled in between, and issues a full session if the pending secret was already gone, and a statement that fails is logged and counts as no row, never as done. That guard is monotonic: it assumes the clock only moves forward, so keep it NTP-synced. If the clock is set back — a dead RTC, a manual change — every code is refused until the clock passes the last accepted step again, and the recovery codes are the way in meanwhile; a clock jumped forward simply invalidates the codes of the skipped steps. Recovery codes are ten characters of `a-z2-7` (fifty bits), case-insensitive, hyphen optional, stored as bcrypt hashes and removed as they are used; to get a new set, disable and enable again. The TOTP seed itself cannot be hashed — the server recomputes codes from it — so it sits in clear in `user.db` next to the MD5 password hash. +The pre-auth token is an ES256 JWT with `iss: "2fa"` and a five-minute lifetime, signed by a second key pair that is never published in the JWKS, so no other service — and no other route here — accepts it as an access token. `/2fa/verify` checks the signature against that key, then the issuer and expiry. It sits outside the service-wide login budget on purpose: without a pre-auth token, which only a correct password on the limited `/login` produces, a request costs one signature check and nothing else, and with one the attempts are bounded by the per-user budget and the token's five minutes, so a global budget there would only let anyone starve `/login` with garbage. Every factor or password check on `/2fa/*` costs one token of a per-user budget of five a minute, correct answers included; the token is taken before the check, so concurrent attempts cannot share one. A code is accepted for the current 30 s step and its two neighbours, and each step is accepted once: the last accepted step is stored, so a captured code cannot be replayed within its window. The step is recorded with a conditional update (`totp_last_step < step`), and a recovery code is removed with the same compare-and-set on the stored list, so two requests carrying one captured code cannot both pass. Every other write the routes make to the four 2FA columns is keyed the same way on the row as it was read: `setup` stores its secret only while 2FA is still off, `enable` and the clearing on login only while that pending secret is still there, `disable` only against the enrolment the code or password was checked on; a `2fa/*` write that changes no row is refused (10016 or 10017), a login whose clear changes no row re-reads the row and goes by what it finds: it stops at the factor if 2FA was enabled in between, and issues a full session if the pending secret was already gone, and a statement that fails is logged and counts as no row, never as done. That guard is monotonic: it assumes the clock only moves forward, so keep it NTP-synced. If the clock is set back — a dead RTC, a manual change — every code is refused until the clock passes the last accepted step again, and the recovery codes are the way in meanwhile; a clock jumped forward simply invalidates the codes of the skipped steps. Recovery codes are ten characters of `a-z2-7` (fifty bits), case-insensitive, hyphen optional, stored as bcrypt hashes and removed as they are used; to get a new set, disable and enable again. The TOTP seed itself cannot be hashed — the server recomputes codes from it — so it sits in clear in `user.db` next to the password hash. + +Passwords are stored as argon2id hashes (PHC string, 19 MiB, two passes). An account made by an older release, or by IceWhale's, holds an unsalted MD5 until its owner signs in once, which replaces it with an argon2id hash. Rolling back to a release that predates this one then needs `casaos-user-service -ru -user ` to set a new password: it only compares MD5. Enrolment needs the password on top of the session, as disabling needs a code or the password: the JWT middleware accepts a refresh token as an access token, so a stolen token alone must not be able to add an authenticator and lock the owner out. A password login clears a pending, never-enabled secret, so an abandoned setup leaves nothing in `user.db` (and a login between `setup` and `enable` means starting the setup again). Enabling or disabling 2FA does not revoke tokens already issued; a refresh token keeps renewing a session obtained before the change. If both the authenticator and the recovery codes are lost, `casaos-user-service -ru -user ` resets the password and clears 2FA together, which is the only way back in short of editing `user.db`. diff --git a/main.go b/main.go index 660dc52..e1719ad 100644 --- a/main.go +++ b/main.go @@ -89,7 +89,7 @@ func init() { } password := random.RandomString(6, false) - userData.Password = encryption.GetMD5ByStr(password) + userData.Password = encryption.HashPassword(password) service.MyService.User().UpdateUserPassword(userData) // The shell is the way back in when the authenticator and the recovery codes are both lost. userData.TotpSecret, userData.TotpEnabled, userData.TotpLastStep, userData.RecoveryCodes = "", false, 0, nil diff --git a/pkg/utils/encryption/password.go b/pkg/utils/encryption/password.go new file mode 100644 index 0000000..658606f --- /dev/null +++ b/pkg/utils/encryption/password.go @@ -0,0 +1,126 @@ +package encryption + +import ( + "crypto/rand" + "crypto/subtle" + "encoding/base64" + "fmt" + "strings" + + "golang.org/x/crypto/argon2" +) + +// A password is stored as an argon2id hash in the PHC string format, +// +// $argon2id$v=19$m=19456,t=2,p=1$$ +// +// with the parameters OWASP gives as the minimum: 19 MiB, two passes, one lane. +// They are read back from the stored string, so they can be raised later and a +// hash made with the old ones still verifies; NeedsRehash says when one should be +// made again. +// +// What this service stored until now, and what IceWhale's still does, is an +// unsalted MD5 in hex, which a stolen user.db gives back to a wordlist in +// minutes. Such a value still verifies, so nobody is locked out by the upgrade, +// and the first successful sign-in of its owner replaces it. +const ( + argonMemory uint32 = 19 * 1024 // KiB + argonTime uint32 = 2 + argonThreads uint8 = 1 + argonKeyLen = 32 + argonSaltLen = 16 + + // What a stored hash may ask of this box. The database is the box's own, so + // this guards a damaged row, not an attacker, from asking for gigabytes. + argonMaxMemory = 256 * 1024 + argonMaxTime = 10 + argonMaxThreads = 16 +) + +var encoding = base64.RawStdEncoding + +// argonSlots bounds how many hashes are computed at once. Each takes 19 MiB, and +// not every endpoint that checks a password is rate limited, changing one's own +// for one: a hundred requests in parallel would be two gigabytes on a box that +// may have one. The others wait their turn, a few tens of milliseconds each. +var argonSlots = make(chan struct{}, 2) + +func idKey(password, salt []byte, time, memory uint32, threads uint8, keyLen uint32) []byte { + argonSlots <- struct{}{} + defer func() { <-argonSlots }() + + return argon2.IDKey(password, salt, time, memory, threads, keyLen) +} + +type argonHash struct { + memory uint32 + time uint32 + threads uint8 + salt []byte + key []byte +} + +// HashPassword is the argon2id hash to store for password. +func HashPassword(password string) string { + salt := make([]byte, argonSaltLen) + _, _ = rand.Read(salt) // never fails since Go 1.24: it ends the program rather than return weak bytes + key := idKey([]byte(password), salt, argonTime, argonMemory, argonThreads, argonKeyLen) + + return fmt.Sprintf("$argon2id$v=%d$m=%d,t=%d,p=%d$%s$%s", + argon2.Version, argonMemory, argonTime, argonThreads, encoding.EncodeToString(salt), encoding.EncodeToString(key)) +} + +// VerifyPassword reports whether candidate is the password stored as stored, an +// argon2id hash or the legacy MD5 hex, in constant time. +func VerifyPassword(stored, candidate string) bool { + if h, ok := parseArgon(stored); ok { + key := idKey([]byte(candidate), h.salt, h.time, h.memory, h.threads, uint32(len(h.key))) + return subtle.ConstantTimeCompare(key, h.key) == 1 + } + + return subtle.ConstantTimeCompare([]byte(stored), []byte(GetMD5ByStr(candidate))) == 1 +} + +// NeedsRehash is whether stored, a value VerifyPassword just accepted, should be +// replaced by a fresh hash: it is the legacy MD5, or an argon2id hash made with +// other parameters than today's. +func NeedsRehash(stored string) bool { + h, ok := parseArgon(stored) + + return !ok || h.memory != argonMemory || h.time != argonTime || h.threads != argonThreads || len(h.key) != argonKeyLen +} + +// parseArgon reads a PHC string. It is strict: the parameters are printed back +// and compared, so trailing text is not ignored, and a hash that asks for more +// than this box would give is refused rather than attempted. +func parseArgon(stored string) (argonHash, bool) { + parts := strings.Split(stored, "$") + if len(parts) != 6 || parts[0] != "" || parts[1] != "argon2id" { + return argonHash{}, false + } + + var version int + if _, err := fmt.Sscanf(parts[2], "v=%d", &version); err != nil || version != argon2.Version || parts[2] != fmt.Sprintf("v=%d", version) { + return argonHash{}, false + } + + var h argonHash + if n, err := fmt.Sscanf(parts[3], "m=%d,t=%d,p=%d", &h.memory, &h.time, &h.threads); err != nil || n != 3 || + parts[3] != fmt.Sprintf("m=%d,t=%d,p=%d", h.memory, h.time, h.threads) { + return argonHash{}, false + } + if h.threads == 0 || h.threads > argonMaxThreads || h.time == 0 || h.time > argonMaxTime || + h.memory < 8*uint32(h.threads) || h.memory > argonMaxMemory { + return argonHash{}, false + } + + var err error + if h.salt, err = encoding.DecodeString(parts[4]); err != nil || len(h.salt) < 8 { + return argonHash{}, false + } + if h.key, err = encoding.DecodeString(parts[5]); err != nil || len(h.key) < 16 || len(h.key) > 64 { + return argonHash{}, false + } + + return h, true +} diff --git a/pkg/utils/encryption/password_test.go b/pkg/utils/encryption/password_test.go new file mode 100644 index 0000000..d713e86 --- /dev/null +++ b/pkg/utils/encryption/password_test.go @@ -0,0 +1,134 @@ +package encryption + +import ( + "fmt" + "strings" + "testing" + + "golang.org/x/crypto/argon2" +) + +// madeWith is a hash of password with parameters of the caller's choosing, the way +// an older or a later release would have stored it. +func madeWith(password string, memory, time uint32, threads uint8) string { + salt := []byte("0123456789abcdef") + key := argon2.IDKey([]byte(password), salt, time, memory, threads, argonKeyLen) + + return fmt.Sprintf("$argon2id$v=%d$m=%d,t=%d,p=%d$%s$%s", + argon2.Version, memory, time, threads, encoding.EncodeToString(salt), encoding.EncodeToString(key)) +} + +func TestAHashVerifiesItsPasswordAndNoOther(t *testing.T) { + stored := HashPassword("correct horse") + + if !strings.HasPrefix(stored, "$argon2id$v=19$m=19456,t=2,p=1$") { + t.Fatalf("stored %q is not an argon2id PHC string with the expected parameters", stored) + } + if !VerifyPassword(stored, "correct horse") { + t.Fatal("the password does not verify against its own hash") + } + for _, wrong := range []string{"", "correct horse ", "Correct horse", "correct hors"} { + if VerifyPassword(stored, wrong) { + t.Fatalf("%q verifies against the hash of another password", wrong) + } + } +} + +func TestTwoHashesOfOnePasswordDiffer(t *testing.T) { + if HashPassword("same") == HashPassword("same") { + t.Fatal("the same salt was used twice: two users with one password would show it") + } +} + +func TestAnythingLongAndAnythingNotASCIIIsHashedWhole(t *testing.T) { + // bcrypt stops at 72 bytes; two passwords that differ only after them must not collide. + long := strings.Repeat("a", 100) + stored := HashPassword(long + "x") + if VerifyPassword(stored, long+"y") { + t.Fatal("a password that differs after the 72nd byte verifies") + } + if !VerifyPassword(HashPassword("pässwörd 🔑"), "pässwörd 🔑") { + t.Fatal("a password with accents and an emoji does not verify") + } +} + +func TestTheLegacyMD5StillVerifiesAndIsReplaced(t *testing.T) { + legacy := GetMD5ByStr("password") // 5f4dcc3b5aa765d61d8327deb882cf99 + + if !VerifyPassword(legacy, "password") { + t.Fatal("an unsalted MD5, as older releases and IceWhale's store it, no longer verifies: the upgrade would lock everyone out") + } + if VerifyPassword(legacy, "Password") { + t.Fatal("the legacy path accepts another password") + } + if !NeedsRehash(legacy) { + t.Fatal("a legacy MD5 is not flagged for replacement") + } + if NeedsRehash(HashPassword("password")) { + t.Fatal("a hash made with today's parameters is flagged for replacement") + } +} + +func TestAHashMadeWithOtherParametersVerifiesAndIsFlagged(t *testing.T) { + older := madeWith("secret", 8*1024, 1, 1) + + if !VerifyPassword(older, "secret") || VerifyPassword(older, "other") { + t.Fatal("the parameters are not read back from the stored hash") + } + if !NeedsRehash(older) { + t.Fatal("a hash made with weaker parameters is not flagged") + } + if !NeedsRehash(madeWith("secret", argonMemory, argonTime, 2)) { + t.Fatal("a hash made with another lane count is not flagged") + } +} + +func TestWhatIsNotAHashNeverVerifies(t *testing.T) { + good := HashPassword("secret") + parts := strings.Split(good, "$") // "", argon2id, v=19, m=..., salt, key + + for name, stored := range map[string]string{ + "empty": "", + "not a hash": "secret", + "the password itself": "5f4dcc3b5aa765d61d8327deb882cf9", // 31 hex digits + "only the prefix": "$argon2id$", + "missing the key": strings.Join(parts[:5], "$"), + "another algorithm": strings.Replace(good, "argon2id", "argon2i", 1), + "another version": strings.Replace(good, "v=19", "v=16", 1), + "trailing text": good + "$extra", + "parameters with a tail": strings.Replace(good, "p=1", "p=1x", 1), + "no lanes": strings.Replace(good, "p=1", "p=0", 1), + "no passes": strings.Replace(good, "t=2", "t=0", 1), + "too much memory": strings.Replace(good, "m=19456", "m=4000000", 1), + "too many passes": strings.Replace(good, "t=2", "t=99", 1), + "memory below 8 per lane": strings.Replace(good, "m=19456", "m=4", 1), + "salt not base64": parts[0] + "$" + parts[1] + "$" + parts[2] + "$" + parts[3] + "$!!!$" + parts[5], + "salt too short": parts[0] + "$" + parts[1] + "$" + parts[2] + "$" + parts[3] + "$AAAA$" + parts[5], + "key too short": parts[0] + "$" + parts[1] + "$" + parts[2] + "$" + parts[3] + "$" + parts[4] + "$AAAA", + } { + // the parser is what must refuse them: a value it let through, with other + // parameters or a shorter salt or key, would give another key and fail + // to verify for that reason alone, and the row would prove nothing + if _, ok := parseArgon(stored); ok { + t.Errorf("%s: %q is accepted as an argon2id hash", name, stored) + + continue + } + if VerifyPassword(stored, "secret") { + t.Errorf("%s: %q verifies", name, stored) + } + } +} + +func TestHashesComputedAtTheSameTimeAllComeOutRight(t *testing.T) { + stored := HashPassword("shared") + done := make(chan bool) + for i := 0; i < 24; i++ { + go func() { done <- VerifyPassword(stored, "shared") && !VerifyPassword(stored, "other") }() + } + for i := 0; i < 24; i++ { + if !<-done { + t.Fatal("a verification run alongside others gave the wrong answer") + } + } +} diff --git a/route/v1/user.go b/route/v1/user.go index 672aad3..32f62ef 100644 --- a/route/v1/user.go +++ b/route/v1/user.go @@ -3,7 +3,6 @@ package v1 import ( "context" "crypto/ecdsa" - "crypto/subtle" "encoding/base64" json2 "encoding/json" "image" @@ -70,7 +69,7 @@ func PostUserRegister(ctx echo.Context) error { user := model2.UserDBModel{} user.Username = username - user.Password = encryption.GetMD5ByStr(pwd) + user.Password = encryption.HashPassword(pwd) user.Role = "admin" user = service.MyService.User().CreateUser(user) @@ -86,9 +85,23 @@ func PostUserRegister(ctx echo.Context) error { // Exported so tests can lift it. var LoginLimiter = rate.NewLimiter(rate.Every(time.Minute), 5) -// passwordMatches compares the stored MD5 hex with the candidate in constant time. +// passwordMatches is whether candidate is the password stored as stored, an +// argon2id hash or, for an account not signed in to since the upgrade, the +// legacy MD5. func passwordMatches(stored, candidate string) bool { - return subtle.ConstantTimeCompare([]byte(stored), []byte(encryption.GetMD5ByStr(candidate))) == 1 + return encryption.VerifyPassword(stored, candidate) +} + +// upgradePassword stores password, which the user's hash just matched, as a fresh +// argon2id hash when that hash is the legacy MD5 or was made with other +// parameters: the owner signs in once and the weak value is gone, with nothing +// asked of them. The swap is keyed on the hash as read, so a password changed +// in the meantime is not put back; if it does not land, the next sign-in tries +// again. +func upgradePassword(user model2.UserDBModel, password string) { + if encryption.NeedsRehash(user.Password) { + service.MyService.User().ReplacePasswordHash(user.Id, user.Password, encryption.HashPassword(password)) + } } // @Summary login @@ -131,6 +144,7 @@ func PostUserLogin(ctx echo.Context) error { return ctx.JSON(common_err.CLIENT_ERROR, model.Result{Success: common_err.USER_NOT_EXIST_OR_PWD_INVALID, Message: common_err.GetMsg(common_err.USER_NOT_EXIST_OR_PWD_INVALID)}) } + upgradePassword(user, password) if !user.TotpEnabled && user.TotpSecret != "" { // An abandoned /2fa/setup does not leave its secret in user.db. The // clear is conditional on the row as read: if /2fa/enable won the race @@ -350,7 +364,7 @@ func PutUserPassword(ctx echo.Context) error { if !passwordMatches(user.Password, oldPwd) { return ctx.JSON(common_err.CLIENT_ERROR, model.Result{Success: common_err.PWD_INVALID_OLD, Message: common_err.GetMsg(common_err.PWD_INVALID_OLD)}) } - user.Password = encryption.GetMD5ByStr(pwd) + user.Password = encryption.HashPassword(pwd) service.MyService.User().UpdateUserPassword(user) user.Password = "" return ctx.JSON(common_err.SUCCESS, model.Result{Success: common_err.SUCCESS, Message: common_err.GetMsg(common_err.SUCCESS), Data: user}) diff --git a/route/v1_2fa_test.go b/route/v1_2fa_test.go index 6e99383..9d8a2ae 100644 --- a/route/v1_2fa_test.go +++ b/route/v1_2fa_test.go @@ -261,7 +261,7 @@ func TestTwoFactorFlow(t *testing.T) { c.expect(200, 200, "POST", "/v1/users/2fa/verify", "", map[string]string{"pre_auth_token": pre, "recovery_code": strings.ToUpper(recovery[0])}) c.expect(400, 10015, "POST", "/v1/users/2fa/verify", "", map[string]string{"pre_auth_token": pre, "recovery_code": recovery[0]}) - // Disable needs exactly one factor; the password path is constant-time MD5. + // Disable needs exactly one factor; the password path is a constant-time comparison. c.expect(400, 4000, "POST", "/v1/users/2fa/disable", access2, map[string]string{}) c.expect(400, 4000, "POST", "/v1/users/2fa/disable", access2, map[string]string{"code": valid, "password": password}) c.expect(400, 10015, "POST", "/v1/users/2fa/disable", access2, map[string]string{"password": "wrong"}) diff --git a/route/v1_password_test.go b/route/v1_password_test.go new file mode 100644 index 0000000..d9dfbf8 --- /dev/null +++ b/route/v1_password_test.go @@ -0,0 +1,107 @@ +package route_test + +import ( + "strings" + "testing" + + "github.com/ReCasaOS/CasaOS-UserService/pkg/config" + "github.com/ReCasaOS/CasaOS-UserService/pkg/utils/encryption" + "github.com/ReCasaOS/CasaOS-UserService/service" + model2 "github.com/ReCasaOS/CasaOS-UserService/service/model" +) + +const argonPrefix = "$argon2id$" + +func storedPassword(t *testing.T, users service.UserService, name string) string { + t.Helper() + user := users.GetUserAllInfoByName(name) + if user.Id == 0 { + t.Fatalf("no user %q", name) + } + return user.Password +} + +// An account from an older release, or from IceWhale's, holds an unsalted MD5. +// It signs in, and the sign-in leaves an argon2id hash in its place. +func TestASignInReplacesTheLegacyMD5(t *testing.T) { + users, c := newRig(t) + const password = "correct horse" + legacy := encryption.GetMD5ByStr(password) + users.CreateUser(model2.UserDBModel{Username: "admin", Password: legacy, Role: "admin"}) + + c.expect(200, 200, "POST", "/v1/users/login", "", map[string]string{"username": "admin", "password": password}) + + stored := storedPassword(t, users, "admin") + if !strings.HasPrefix(stored, argonPrefix) { + t.Fatalf("after a sign-in the password is still %q", stored) + } + // and it is still the same password, for this sign-in and the next + c.expect(200, 200, "POST", "/v1/users/login", "", map[string]string{"username": "admin", "password": password}) + c.expect(400, 10013, "POST", "/v1/users/login", "", map[string]string{"username": "admin", "password": "wrong"}) + if again := storedPassword(t, users, "admin"); again != stored { + t.Fatal("a second sign-in made a new hash: only the legacy one is replaced") + } +} + +func TestAWrongPasswordLeavesTheLegacyHashAlone(t *testing.T) { + users, c := newRig(t) + legacy := encryption.GetMD5ByStr("correct horse") + users.CreateUser(model2.UserDBModel{Username: "admin", Password: legacy, Role: "admin"}) + + c.expect(400, 10013, "POST", "/v1/users/login", "", map[string]string{"username": "admin", "password": "wrong"}) + + if stored := storedPassword(t, users, "admin"); stored != legacy { + t.Fatalf("a failed sign-in changed the stored password to %q", stored) + } +} + +func TestANewAccountIsStoredAsArgon2id(t *testing.T) { + users, c := newRig(t) + defer func(old string) { config.AppInfo.UserDataPath = old }(config.AppInfo.UserDataPath) + config.AppInfo.UserDataPath = t.TempDir() + service.UserRegisterHash["k"] = "k" + + c.expect(200, 200, "POST", "/v1/users/register", "", map[string]string{"username": "owner", "password": "a long enough one", "key": "k"}) + + stored := storedPassword(t, users, "owner") + if !strings.HasPrefix(stored, argonPrefix) { + t.Fatalf("a new account's password is stored as %q", stored) + } + c.expect(200, 200, "POST", "/v1/users/login", "", map[string]string{"username": "owner", "password": "a long enough one"}) +} + +func TestChangingThePasswordStoresArgon2idAndRetiresTheOldOne(t *testing.T) { + users, c := newRig(t) + users.CreateUser(model2.UserDBModel{Username: "admin", Password: encryption.GetMD5ByStr("old password"), Role: "admin"}) + access, _ := tokens(t, c.expect(200, 200, "POST", "/v1/users/login", "", map[string]string{"username": "admin", "password": "old password"})) + + c.expect(200, 200, "PUT", "/v1/users/current/password", access, map[string]string{"old_password": "old password", "password": "new password"}) + + if stored := storedPassword(t, users, "admin"); !strings.HasPrefix(stored, argonPrefix) || encryption.VerifyPassword(stored, "old password") { + t.Fatalf("after the change the stored password is %q", stored) + } + c.expect(400, 10013, "POST", "/v1/users/login", "", map[string]string{"username": "admin", "password": "old password"}) + c.expect(200, 200, "POST", "/v1/users/login", "", map[string]string{"username": "admin", "password": "new password"}) +} + +// A sign-in that read the legacy hash before a password change wrote its own +// must not put the old password back when it comes to replace the hash. +func TestAStaleSignInDoesNotRestoreAChangedPassword(t *testing.T) { + users, _ := newRig(t) + legacy := encryption.GetMD5ByStr("old password") + user := users.CreateUser(model2.UserDBModel{Username: "admin", Password: legacy, Role: "admin"}) + + changed := encryption.HashPassword("new password") + user.Password = changed + users.UpdateUserPassword(user) // the change lands between the sign-in's read and its write + + if users.ReplacePasswordHash(user.Id, legacy, encryption.HashPassword("old password")) { + t.Fatal("the swap landed on a row that no longer held the hash it was made from") + } + if stored := storedPassword(t, users, "admin"); stored != changed { + t.Fatalf("the changed password was overwritten by %q", stored) + } + if !users.ReplacePasswordHash(user.Id, changed, encryption.HashPassword("new password")) { + t.Fatal("the swap does not land on a row that still holds the hash it was made from") + } +} diff --git a/service/model/o_user.go b/service/model/o_user.go index 20b027e..c786239 100644 --- a/service/model/o_user.go +++ b/service/model/o_user.go @@ -26,7 +26,7 @@ type UserDBModel struct { // TotpSecret is the base32 TOTP seed. It cannot be hashed: the server must // recompute codes from it at every login, so it lives in clear next to the - // (MD5) password hash and is protected only by the DB file permissions. + // password hash and is protected only by the DB file permissions. // json:"-" on the secret fields is load-bearing: login serialises the whole // row, and PUT /v1/users/current binds the request body into this struct. TotpSecret string `gorm:"default:''" json:"-"` diff --git a/service/user.go b/service/user.go index f0b8c85..e6ded88 100644 --- a/service/user.go +++ b/service/user.go @@ -39,6 +39,7 @@ type UserService interface { GetUserInfoByUserName(userName string) (m model.UserDBModel) GetAllUserName() (list []model.UserDBModel) UpdateUserTOTP(m model.UserDBModel) + ReplacePasswordHash(id int, current, replacement string) bool EnableUserTOTP(id int, secret string, step int64, hashes []string) bool ClearPendingTOTP(id int, secret string) bool SetPendingTOTP(id int, secret string) bool @@ -108,6 +109,14 @@ func (u *userService) EnableUserTOTP(id int, secret string, step int64, hashes [ return changedOne("enable", u.db.Model(&model.UserDBModel{Id: id}).Select("totp_enabled", "totp_last_step", "recovery_codes").Where("totp_enabled = ? AND totp_secret = ?", false, secret).Updates(&model.UserDBModel{TotpEnabled: true, TotpLastStep: step, RecoveryCodes: hashes})) } +// ReplacePasswordHash swaps a stored hash for another hash of the same password +// and reports whether it did. The write is keyed on the hash as read: a sign-in +// that read the row before a password change wrote it must not put the old +// password back. +func (u *userService) ReplacePasswordHash(id int, current, replacement string) bool { + return changedOne("replace password hash", u.db.Model(&model.UserDBModel{Id: id}).Where("password = ?", current).Update("password", replacement)) +} + // ClearPendingTOTP removes a never-enabled secret and reports whether it did. // The write is keyed on the row being still disabled with the secret as read: // a login that read the row before /2fa/enable wrote it must not zero an