From ae277c8a89a59219aaf3f795f36349c56fbff65f Mon Sep 17 00:00:00 2001 From: Misha Nasledov Date: Fri, 9 Oct 2026 16:37:37 -0700 Subject: [PATCH] Don't send the account password hash in GetUser replies HandleGetUser put the stored bcrypt hash in field 106. A client that sends the field back unchanged in SetUser or UpdateUser has the hash itself hashed and stored, so saving an account without touching its password locks it out. It also hands the hashes to anyone with the Open User privilege. Reply with "x" when the account has a password and omit the field otherwise, as Account.Read already does for the user list. --- hotline/account.go | 7 +++- internal/mobius/handlers_accounts.go | 16 +++++--- internal/mobius/handlers_accounts_test.go | 48 +++++++++++++++++++++-- 3 files changed, 62 insertions(+), 9 deletions(-) diff --git a/hotline/account.go b/hotline/account.go index 895d8a0..6f5c2e3 100644 --- a/hotline/account.go +++ b/hotline/account.go @@ -30,6 +30,11 @@ func NewAccount(login, name, password string, access AccessBitmap) *Account { } } +// HasPassword reports whether the account has a non-empty password. +func (a *Account) HasPassword() bool { + return bcrypt.CompareHashAndPassword([]byte(a.Password), []byte("")) != nil +} + // Read implements io.Reader interface for Account func (a *Account) Read(p []byte) (int, error) { fields := []Field{ @@ -38,7 +43,7 @@ func (a *Account) Read(p []byte) (int, error) { NewField(FieldUserAccess, a.Access[:]), } - if bcrypt.CompareHashAndPassword([]byte(a.Password), []byte("")) != nil { + if a.HasPassword() { fields = append(fields, NewField(FieldUserPassword, []byte("x"))) } diff --git a/internal/mobius/handlers_accounts.go b/internal/mobius/handlers_accounts.go index 5014d84..ce6a87a 100644 --- a/internal/mobius/handlers_accounts.go +++ b/internal/mobius/handlers_accounts.go @@ -88,7 +88,7 @@ func HandleSetUser(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotlin // Fields used in the reply: // - 102 User name Account display name // - 105 User login Account login (encoded, each character negated) -// - 106 User password Account password +// - 106 User password "x" if the account has a password, omitted otherwise // - 110 User access User access privileges bitmap func HandleGetUser(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotline.Transaction) { if !cc.Authorize(hotline.AccessOpenUser) { @@ -100,12 +100,18 @@ func HandleGetUser(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotlin return cc.NewErrReply(t, ErrMsgAccountNotExist) } - return append(res, cc.NewReply(t, + // Never send the stored hash: it is crackable offline, and a client that sends the + // field back unchanged in SetUser would have the hash itself become the password. + fields := []hotline.Field{ hotline.NewField(hotline.FieldUserName, []byte(account.Name)), hotline.NewField(hotline.FieldUserLogin, hotline.EncodeString(t.GetField(hotline.FieldUserLogin).Data)), - hotline.NewField(hotline.FieldUserPassword, []byte(account.Password)), - hotline.NewField(hotline.FieldUserAccess, account.Access[:]), - )) + } + if account.HasPassword() { + fields = append(fields, hotline.NewField(hotline.FieldUserPassword, []byte("x"))) + } + fields = append(fields, hotline.NewField(hotline.FieldUserAccess, account.Access[:])) + + return append(res, cc.NewReply(t, fields...)) } // HandleListUsers returns a list of all user accounts on the server. diff --git a/internal/mobius/handlers_accounts_test.go b/internal/mobius/handlers_accounts_test.go index 60b4f6e..5cf952f 100644 --- a/internal/mobius/handlers_accounts_test.go +++ b/internal/mobius/handlers_accounts_test.go @@ -22,7 +22,7 @@ func TestHandleGetUser(t *testing.T) { wantRes []hotline.Transaction }{ { - name: "when account is valid", + name: "when account has a password", args: args{ cc: &hotline.ClientConn{ Account: &hotline.Account{ @@ -40,7 +40,50 @@ func TestHandleGetUser(t *testing.T) { m.On("Get", "guest").Return(&hotline.Account{ Login: "guest", Name: "Guest", - Password: "password", + Password: hotline.HashAndSalt([]byte("password")), + Access: hotline.AccessBitmap{}, + }) + return &m + }(), + }, + }, + t: hotline.NewTransaction( + hotline.TranGetUser, [2]byte{0, 1}, + hotline.NewField(hotline.FieldUserLogin, []byte("guest")), + ), + }, + wantRes: []hotline.Transaction{ + { + IsReply: 0x01, + Fields: []hotline.Field{ + hotline.NewField(hotline.FieldUserName, []byte("Guest")), + hotline.NewField(hotline.FieldUserLogin, hotline.EncodeString([]byte("guest"))), + hotline.NewField(hotline.FieldUserPassword, []byte("x")), + hotline.NewField(hotline.FieldUserAccess, []byte{0, 0, 0, 0, 0, 0, 0, 0}), + }, + }, + }, + }, + { + name: "when account has no password", + args: args{ + cc: &hotline.ClientConn{ + Account: &hotline.Account{ + Access: func() hotline.AccessBitmap { + var bits hotline.AccessBitmap + bits.Set(hotline.AccessOpenUser) + return bits + }(), + }, + Server: &hotline.Server{ + TextDecoder: charmap.Macintosh.NewDecoder(), + TextEncoder: charmap.Macintosh.NewEncoder(), + AccountManager: func() *MockAccountManager { + m := MockAccountManager{} + m.On("Get", "guest").Return(&hotline.Account{ + Login: "guest", + Name: "Guest", + Password: hotline.HashAndSalt([]byte("")), Access: hotline.AccessBitmap{}, }) return &m @@ -58,7 +101,6 @@ func TestHandleGetUser(t *testing.T) { Fields: []hotline.Field{ hotline.NewField(hotline.FieldUserName, []byte("Guest")), hotline.NewField(hotline.FieldUserLogin, hotline.EncodeString([]byte("guest"))), - hotline.NewField(hotline.FieldUserPassword, []byte("password")), hotline.NewField(hotline.FieldUserAccess, []byte{0, 0, 0, 0, 0, 0, 0, 0}), }, },