From e3e087f18ee1c6fece88faad2efd80b6682499fe Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 22 Apr 2026 20:09:33 -0700 Subject: [PATCH] fix(sftpd): harden bcrypt migration and auth path - SetPassword now returns an error instead of silently truncating passwords >72 bytes (the prior fallback could also panic for short passwords when bcrypt returned any non-ErrPasswordTooLong error). - CheckPassword is now pure and reports (ok, legacy); migration moves to FileStore.migrateLegacyPassword under a write lock to fix a data race on concurrent logins and a double-check avoids redundant re-hashing. - ValidatePassword only rewrites the user store when a migration actually happened, eliminating a full file rewrite on every successful login. - Legacy plaintext comparison uses subtle.ConstantTimeCompare again, restoring the pre-PR constant-time property. - CreateUser propagates SetPassword errors and releases the write lock before calling saveUsers to avoid recursive RWMutex locking. - Add unit tests covering new-user hashing, legacy migration, no-rewrite on normal login, wrong/empty passwords, oversize password rejection, and concurrent migration under -race. --- weed/sftpd/user/filestore.go | 67 ++++++---- weed/sftpd/user/filestore_test.go | 195 ++++++++++++++++++++++++++++++ weed/sftpd/user/user.go | 36 +++--- 3 files changed, 259 insertions(+), 39 deletions(-) create mode 100644 weed/sftpd/user/filestore_test.go diff --git a/weed/sftpd/user/filestore.go b/weed/sftpd/user/filestore.go index f740d442c..b93ad00f2 100644 --- a/weed/sftpd/user/filestore.go +++ b/weed/sftpd/user/filestore.go @@ -8,6 +8,7 @@ import ( "path" "sync" + "github.com/seaweedfs/seaweedfs/weed/glog" "golang.org/x/crypto/ssh" ) @@ -147,21 +148,45 @@ func (s *FileStore) GetUser(username string) (*User, error) { return user, nil } -// ValidatePassword checks if the password is valid for the user +// ValidatePassword checks if the password is valid for the user. Legacy +// plaintext entries are transparently upgraded to bcrypt on successful match. func (s *FileStore) ValidatePassword(username string, password []byte) bool { user, err := s.GetUser(username) if err != nil { return false } - if user.CheckPassword(string(password)) { - // If legacy password was migrated to bcrypt, persist the change - if user.HashedPassword != "" && user.Password == "" { - _ = s.saveUsers() - } - return true + ok, legacy := user.CheckPassword(string(password)) + if !ok { + return false + } + if legacy { + s.migrateLegacyPassword(user, string(password)) + } + return true +} + +// migrateLegacyPassword re-hashes a legacy plaintext password under a write +// lock and persists the user store. The FileStore mutex guards both the +// re-hash (to avoid a data race with concurrent logins) and the double-check +// that another goroutine hasn't already migrated. Failures are logged but +// not surfaced — auth has already succeeded and the next login will retry. +func (s *FileStore) migrateLegacyPassword(user *User, password string) { + s.mu.Lock() + if user.HashedPassword != "" { + s.mu.Unlock() + return + } + if err := user.SetPassword(password); err != nil { + s.mu.Unlock() + glog.V(0).Infof("sftpd: failed to upgrade legacy password for %s: %v", user.Username, err) + return + } + s.mu.Unlock() + + if err := s.saveUsers(); err != nil { + glog.V(0).Infof("sftpd: failed to persist upgraded password for %s: %v", user.Username, err) } - return false } // ValidatePublicKey checks if the public key is valid for the user @@ -245,25 +270,21 @@ func (s *FileStore) ListUsers() ([]string, error) { // CreateUser creates a new user with the given username and password func (s *FileStore) CreateUser(username, password string) (*User, error) { - s.mu.Lock() - defer s.mu.Unlock() - - // Check if user already exists - if _, exists := s.users[username]; exists { - return nil, fmt.Errorf("user already exists: %s", username) - } - - // Create new user user := NewUser(username) - - // Hash password with bcrypt - user.SetPassword(password) - - // Add default permissions + if err := user.SetPassword(password); err != nil { + return nil, err + } user.Permissions[user.HomeDir] = []string{"all"} - // Save the user + s.mu.Lock() + if _, exists := s.users[username]; exists { + s.mu.Unlock() + return nil, fmt.Errorf("user already exists: %s", username) + } s.users[username] = user + s.mu.Unlock() + + // saveUsers acquires RLock, so we must release the write lock above first. if err := s.saveUsers(); err != nil { return nil, err } diff --git a/weed/sftpd/user/filestore_test.go b/weed/sftpd/user/filestore_test.go new file mode 100644 index 000000000..f28b324ec --- /dev/null +++ b/weed/sftpd/user/filestore_test.go @@ -0,0 +1,195 @@ +package user + +import ( + "bytes" + "encoding/json" + "os" + "path/filepath" + "strings" + "sync" + "testing" + + "golang.org/x/crypto/bcrypt" +) + +func newTempStore(t *testing.T) *FileStore { + t.Helper() + dir := t.TempDir() + s, err := NewFileStore(filepath.Join(dir, "users.json")) + if err != nil { + t.Fatalf("NewFileStore: %v", err) + } + return s +} + +func readUsers(t *testing.T, s *FileStore) []*User { + t.Helper() + data, err := os.ReadFile(s.filePath) + if err != nil { + t.Fatalf("read store file: %v", err) + } + var users []*User + if err := json.Unmarshal(data, &users); err != nil { + t.Fatalf("unmarshal store file: %v", err) + } + return users +} + +func TestCreateUserHashesPassword(t *testing.T) { + s := newTempStore(t) + u, err := s.CreateUser("alice", "s3cret") + if err != nil { + t.Fatalf("CreateUser: %v", err) + } + if u.Password != "" { + t.Errorf("plaintext Password leaked: %q", u.Password) + } + if u.HashedPassword == "" { + t.Fatal("HashedPassword empty") + } + if err := bcrypt.CompareHashAndPassword([]byte(u.HashedPassword), []byte("s3cret")); err != nil { + t.Fatalf("hash does not match password: %v", err) + } + stored := readUsers(t, s) + if len(stored) != 1 || stored[0].Password != "" { + t.Errorf("plaintext persisted: %+v", stored) + } +} + +func TestValidatePasswordMigratesLegacyPlaintext(t *testing.T) { + s := newTempStore(t) + legacy := NewUser("bob") + legacy.Password = "hunter2" + if err := s.SaveUser(legacy); err != nil { + t.Fatalf("SaveUser: %v", err) + } + + if !s.ValidatePassword("bob", []byte("hunter2")) { + t.Fatal("legacy login rejected") + } + u, err := s.GetUser("bob") + if err != nil { + t.Fatalf("GetUser: %v", err) + } + if u.Password != "" { + t.Errorf("legacy Password not cleared: %q", u.Password) + } + if u.HashedPassword == "" { + t.Fatal("HashedPassword not populated after migration") + } + + stored := readUsers(t, s) + if len(stored) != 1 || stored[0].Password != "" || stored[0].HashedPassword == "" { + t.Errorf("migration not persisted: %+v", stored) + } +} + +// Successful logins that don't migrate must not rewrite the store file. +// We detect a rewrite by replacing the file contents with a sentinel after +// CreateUser and verifying the sentinel is untouched after a valid login. +func TestValidatePasswordNoRewriteWithoutMigration(t *testing.T) { + s := newTempStore(t) + if _, err := s.CreateUser("carol", "pw"); err != nil { + t.Fatalf("CreateUser: %v", err) + } + + sentinel := []byte("SENTINEL_NOT_REAL_JSON") + if err := os.WriteFile(s.filePath, sentinel, 0600); err != nil { + t.Fatalf("write sentinel: %v", err) + } + + if !s.ValidatePassword("carol", []byte("pw")) { + t.Fatal("login rejected") + } + got, err := os.ReadFile(s.filePath) + if err != nil { + t.Fatalf("read: %v", err) + } + if !bytes.Equal(got, sentinel) { + t.Errorf("store file was rewritten on non-migrating login: got %q", got) + } +} + +func TestValidatePasswordWrongPassword(t *testing.T) { + s := newTempStore(t) + if _, err := s.CreateUser("dave", "correct"); err != nil { + t.Fatalf("CreateUser: %v", err) + } + if s.ValidatePassword("dave", []byte("wrong")) { + t.Error("wrong password accepted for bcrypt user") + } + + legacy := NewUser("eve") + legacy.Password = "plain" + if err := s.SaveUser(legacy); err != nil { + t.Fatal(err) + } + if s.ValidatePassword("eve", []byte("wrong")) { + t.Error("wrong password accepted for legacy user") + } + if s.ValidatePassword("eve", []byte("")) { + t.Error("empty password accepted for legacy user") + } +} + +func TestValidatePasswordUnknownUser(t *testing.T) { + s := newTempStore(t) + if s.ValidatePassword("nobody", []byte("whatever")) { + t.Error("unknown user accepted") + } +} + +func TestCheckPasswordEmptyCredentials(t *testing.T) { + u := NewUser("frank") + if ok, _ := u.CheckPassword(""); ok { + t.Error("empty password accepted on user with no credentials") + } + if ok, _ := u.CheckPassword("x"); ok { + t.Error("password accepted on user with no credentials") + } +} + +func TestSetPasswordTooLong(t *testing.T) { + u := NewUser("grace") + if err := u.SetPassword(strings.Repeat("a", 73)); err == nil { + t.Error("expected error for >72 byte password") + } + if u.HashedPassword != "" { + t.Errorf("HashedPassword set despite error: %q", u.HashedPassword) + } +} + +// Concurrent ValidatePassword calls on a legacy user must not race on the +// User fields and must converge to a single persisted bcrypt hash. +func TestValidatePasswordConcurrentMigration(t *testing.T) { + s := newTempStore(t) + legacy := NewUser("heidi") + legacy.Password = "p" + if err := s.SaveUser(legacy); err != nil { + t.Fatal(err) + } + + const N = 16 + var wg sync.WaitGroup + wg.Add(N) + for i := 0; i < N; i++ { + go func() { + defer wg.Done() + if !s.ValidatePassword("heidi", []byte("p")) { + t.Error("login rejected") + } + }() + } + wg.Wait() + + u, err := s.GetUser("heidi") + if err != nil { + t.Fatalf("GetUser: %v", err) + } + if u.Password != "" { + t.Errorf("plaintext not cleared after concurrent migration: %q", u.Password) + } + if err := bcrypt.CompareHashAndPassword([]byte(u.HashedPassword), []byte("p")); err != nil { + t.Errorf("final hash invalid: %v", err) + } +} diff --git a/weed/sftpd/user/user.go b/weed/sftpd/user/user.go index 4b1035fad..49a7e9359 100644 --- a/weed/sftpd/user/user.go +++ b/weed/sftpd/user/user.go @@ -2,6 +2,8 @@ package user import ( + "crypto/subtle" + "fmt" "math/rand/v2" "path/filepath" @@ -11,8 +13,8 @@ import ( // User represents an SFTP user with authentication and permission details type User struct { Username string `json:"Username"` - HashedPassword string `json:"HashedPassword"` // bcrypt hash - Password string `json:"Password,omitempty"` // deprecated: plaintext, migrated on next save + HashedPassword string `json:"HashedPassword"` // bcrypt hash + Password string `json:"Password,omitempty"` // deprecated: plaintext, migrated to HashedPassword on first successful login PublicKeys []string `json:"PublicKeys,omitempty"` HomeDir string `json:"HomeDir"` Permissions map[string][]string `json:"Permissions,omitempty"` @@ -36,29 +38,31 @@ func NewUser(username string) *User { } } -// SetPassword hashes and stores the password using bcrypt -func (u *User) SetPassword(password string) { +// SetPassword hashes and stores the password using bcrypt. It clears any +// legacy plaintext value. Returns an error if the password cannot be hashed +// (e.g. exceeds bcrypt's 72-byte limit). +func (u *User) SetPassword(password string) error { hash, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost) if err != nil { - // bcrypt only errors on passwords > 72 bytes; truncate if needed - hash, _ = bcrypt.GenerateFromPassword([]byte(password[:72]), bcrypt.DefaultCost) + return fmt.Errorf("hash password: %w", err) } u.HashedPassword = string(hash) - u.Password = "" // clear any legacy plaintext + u.Password = "" + return nil } -// CheckPassword verifies a password against the stored hash. -// It transparently handles legacy plaintext passwords by upgrading them on match. -func (u *User) CheckPassword(password string) bool { +// CheckPassword verifies a password against the stored credentials without +// mutating the receiver. The legacy return is true when the match succeeded +// via the deprecated plaintext field; the caller is expected to re-hash and +// persist the password under an appropriate lock. +func (u *User) CheckPassword(password string) (ok bool, legacy bool) { if u.HashedPassword != "" { - return bcrypt.CompareHashAndPassword([]byte(u.HashedPassword), []byte(password)) == nil + return bcrypt.CompareHashAndPassword([]byte(u.HashedPassword), []byte(password)) == nil, false } - // Legacy plaintext migration path - if u.Password != "" && u.Password == password { - u.SetPassword(password) // upgrade to bcrypt - return true + if u.Password != "" && subtle.ConstantTimeCompare([]byte(u.Password), []byte(password)) == 1 { + return true, true } - return false + return false, false } // AddPublicKey adds a public key to the user