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.
2026-09-20 13:30:46 +02:00 · 2026-04-22 20:09:33 -07:00
parent 9b077d6f1f
commit e3e087f18e
3 changed files with 259 additions and 39 deletions
@@ -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
 	}
@@ -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)
+	}
+}
@@ -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