From 123d1a305bc68474034f5989362148508bdbf7f5 Mon Sep 17 00:00:00 2001 From: Jeff Halter <868228+jhalter@users.noreply.github.com> Date: Sun, 31 May 2026 14:48:31 -0700 Subject: Validate client input to prevent panics from malformed transactions A malicious or buggy client could send transaction fields with the wrong length and trigger runtime panics (slice/index out of range, slice-to-array conversion, nil deref) in the parsing and handler code. These were caught by the connection-level recover, so they dropped the client connection and dumped a stack trace to stdout rather than crashing the process, but they are still incorrect behavior, a log-flood vector, and a latent crash if the recover scope ever changes. Fix at the source and harden the safety net: - Add ClientIDFromBytes / ChatIDFromBytes helpers that return ok=false on a length mismatch, and use them in the transaction handlers instead of direct [2]byte(...) / [4]byte(...) conversions on field data. Nil-check ClientMgr.Get results, and length-guard FieldOptions and the HandleUpdateUser sub-field header. Malformed input now yields an error reply (or a clean no-op for reply-less handlers) instead of panicking. - Bounds-check FileResumeData.UnmarshalBinary (header length and fork count) and guard the ForkInfoList[0] accesses against an empty list. - Bounds-check FlatFileInformationFork parsing (reachable on upload): validate the fixed header, name, and comment lengths, and fix a latent 72+nameSize uint16 overflow. Route Write through UnmarshalBinary. - panic.go: stop printing stack traces to stdout (keep structured logging) so a client cannot flood stdout by repeatedly triggering a panic. - handleTransaction: recover per-transaction so one malformed request no longer tears down the whole client connection. Adds tests for the new helpers, the hardened resume-data and flat-file-object decoders, and handler-level malformed-ID handling. --- hotline/client_conn.go | 4 + hotline/decode.go | 22 ++++++ hotline/decode_test.go | 85 ++++++++++++++++++++ hotline/file_resume_data.go | 22 +++++- hotline/file_transfer.go | 6 +- hotline/flattened_file_object.go | 57 ++++++-------- hotline/flattened_file_object_test.go | 23 ++++++ hotline/panic.go | 7 +- internal/mobius/transaction_handlers.go | 114 ++++++++++++++++++++------- internal/mobius/transaction_handlers_test.go | 55 +++++++++++++ 10 files changed, 325 insertions(+), 70 deletions(-) create mode 100644 hotline/decode.go create mode 100644 hotline/decode_test.go diff --git a/hotline/client_conn.go b/hotline/client_conn.go index 5428ea5..9ca93a7 100644 --- a/hotline/client_conn.go +++ b/hotline/client_conn.go @@ -114,6 +114,10 @@ func (cc *ClientConn) SendAll(t [2]byte, fields ...Field) { } func (cc *ClientConn) handleTransaction(transaction Transaction) { + // Contain panics to the individual transaction so a single malformed request + // cannot tear down the whole client connection. + defer dontPanic(cc.Logger) + if handler, ok := cc.Server.handlers[transaction.Type]; ok { if transaction.Type != TranKeepAlive { cc.Logger.Info(tranTypeNames[transaction.Type]) diff --git a/hotline/decode.go b/hotline/decode.go new file mode 100644 index 0000000..385bc07 --- /dev/null +++ b/hotline/decode.go @@ -0,0 +1,22 @@ +package hotline + +// ClientIDFromBytes converts client-supplied bytes to a ClientID, returning +// ok=false if the input is not exactly the expected length. Use this instead of +// a direct [2]byte(...) conversion on field data, which panics on a length +// mismatch. +func ClientIDFromBytes(b []byte) (id ClientID, ok bool) { + if len(b) != len(id) { + return id, false + } + return ClientID(b), true +} + +// ChatIDFromBytes converts client-supplied bytes to a ChatID, returning ok=false +// if the input is not exactly the expected length. Use this instead of a direct +// [4]byte(...) conversion on field data, which panics on a length mismatch. +func ChatIDFromBytes(b []byte) (id ChatID, ok bool) { + if len(b) != len(id) { + return id, false + } + return ChatID(b), true +} diff --git a/hotline/decode_test.go b/hotline/decode_test.go new file mode 100644 index 0000000..663f7ce --- /dev/null +++ b/hotline/decode_test.go @@ -0,0 +1,85 @@ +package hotline + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestClientIDFromBytes(t *testing.T) { + tests := []struct { + name string + b []byte + want ClientID + wantOk bool + }{ + {name: "valid 2 bytes", b: []byte{0x00, 0x05}, want: ClientID{0x00, 0x05}, wantOk: true}, + {name: "nil", b: nil, wantOk: false}, + {name: "too short", b: []byte{0x01}, wantOk: false}, + {name: "too long", b: []byte{0x01, 0x02, 0x03}, wantOk: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, ok := ClientIDFromBytes(tt.b) + assert.Equal(t, tt.wantOk, ok) + if tt.wantOk { + assert.Equal(t, tt.want, got) + } + }) + } +} + +func TestChatIDFromBytes(t *testing.T) { + tests := []struct { + name string + b []byte + want ChatID + wantOk bool + }{ + {name: "valid 4 bytes", b: []byte{0x00, 0x00, 0x00, 0x09}, want: ChatID{0x00, 0x00, 0x00, 0x09}, wantOk: true}, + {name: "nil", b: nil, wantOk: false}, + {name: "too short", b: []byte{0x01, 0x02, 0x03}, wantOk: false}, + {name: "too long", b: []byte{0x01, 0x02, 0x03, 0x04, 0x05}, wantOk: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, ok := ChatIDFromBytes(tt.b) + assert.Equal(t, tt.wantOk, ok) + if tt.wantOk { + assert.Equal(t, tt.want, got) + } + }) + } +} + +func TestFileResumeData_UnmarshalBinary(t *testing.T) { + // A well-formed buffer: 42-byte header with ForkCount=1, plus one 16-byte fork. + valid := make([]byte, resumeDataHeaderLen+forkInfoLen) + valid[41] = 1 // ForkCount low byte + + t.Run("valid single-fork buffer parses", func(t *testing.T) { + var frd FileResumeData + require.NoError(t, frd.UnmarshalBinary(valid)) + assert.Len(t, frd.ForkInfoList, 1) + }) + + t.Run("buffer shorter than header returns error, no panic", func(t *testing.T) { + var frd FileResumeData + assert.Error(t, frd.UnmarshalBinary(make([]byte, 10))) + }) + + t.Run("fork count overrunning the buffer returns error, no panic", func(t *testing.T) { + b := make([]byte, resumeDataHeaderLen) // header only, no fork bytes + b[41] = 3 // claims 3 forks that aren't present + var frd FileResumeData + assert.Error(t, frd.UnmarshalBinary(b)) + }) + + t.Run("zero fork count yields an empty list", func(t *testing.T) { + b := make([]byte, resumeDataHeaderLen) + var frd FileResumeData + require.NoError(t, frd.UnmarshalBinary(b)) + assert.Empty(t, frd.ForkInfoList) + }) +} diff --git a/hotline/file_resume_data.go b/hotline/file_resume_data.go index 1926bc6..7cbfb17 100644 --- a/hotline/file_resume_data.go +++ b/hotline/file_resume_data.go @@ -76,15 +76,31 @@ func (frd *FileResumeData) BinaryMarshal() ([]byte, error) { return buf.Bytes(), nil } +// resumeDataHeaderLen is the fixed-size header (Format, Version, RSVD, ForkCount) +// that precedes the variable-length fork info list. +const resumeDataHeaderLen = 42 + +// forkInfoLen is the size of a single ForkInfoList entry. +const forkInfoLen = 16 + func (frd *FileResumeData) UnmarshalBinary(b []byte) error { + if len(b) < resumeDataHeaderLen { + return fmt.Errorf("file resume data too short: %d bytes, need at least %d", len(b), resumeDataHeaderLen) + } + frd.Format = [4]byte{b[0], b[1], b[2], b[3]} frd.Version = [2]byte{b[4], b[5]} frd.ForkCount = [2]byte{b[40], b[41]} - for i := 0; i < int(frd.ForkCount[1]); i++ { + forkCount := int(frd.ForkCount[1]) + if need := resumeDataHeaderLen + forkCount*forkInfoLen; len(b) < need { + return fmt.Errorf("file resume data truncated: %d bytes, need %d for %d forks", len(b), need, forkCount) + } + + for i := 0; i < forkCount; i++ { var fil ForkInfoList - start := 42 + i*16 - end := start + 16 + start := resumeDataHeaderLen + i*forkInfoLen + end := start + forkInfoLen r := bytes.NewReader(b[start:end]) if err := binary.Read(r, binary.BigEndian, &fil); err != nil { diff --git a/hotline/file_transfer.go b/hotline/file_transfer.go index e5898db..98999f1 100644 --- a/hotline/file_transfer.go +++ b/hotline/file_transfer.go @@ -246,7 +246,7 @@ func (fh *FileHeader) Read(p []byte) (int, error) { func DownloadHandler(w io.Writer, fullPath string, fileTransfer *FileTransfer, fs FileStore, rLogger *slog.Logger, preserveForks bool) error { var dataOffset int64 - if fileTransfer.FileResumeData != nil { + if fileTransfer.FileResumeData != nil && len(fileTransfer.FileResumeData.ForkInfoList) > 0 { dataOffset = int64(binary.BigEndian.Uint32(fileTransfer.FileResumeData.ForkInfoList[0].DataSize[:])) } @@ -458,7 +458,9 @@ func DownloadFolderHandler(rwc io.ReadWriter, fullPath string, fileTransfer *Fil if err := frd.UnmarshalBinary(resumeDataBytes); err != nil { return err } - dataOffset = int64(binary.BigEndian.Uint32(frd.ForkInfoList[0].DataSize[:])) + if len(frd.ForkInfoList) > 0 { + dataOffset = int64(binary.BigEndian.Uint32(frd.ForkInfoList[0].DataSize[:])) + } case DlFldrActionNextFile: // client asked to skip this file return nil diff --git a/hotline/flattened_file_object.go b/hotline/flattened_file_object.go index 130e917..9a4fd85 100644 --- a/hotline/flattened_file_object.go +++ b/hotline/flattened_file_object.go @@ -3,10 +3,15 @@ package hotline import ( "bytes" "encoding/binary" + "fmt" "io" "slices" ) +// flatFileInfoForkMinLen is the fixed-size portion of a FlatFileInformationFork +// that precedes the variable-length name (and optional comment). +const flatFileInfoForkMinLen = 72 + type flattenedFileObject struct { FlatFileHeader FlatFileHeader FlatFileInformationForkHeader FlatFileForkHeader @@ -156,40 +161,22 @@ func (ffif *FlatFileInformationFork) Read(p []byte) (int, error) { // Write implements the io.Writer interface for FlatFileInformationFork func (ffif *FlatFileInformationFork) Write(p []byte) (int, error) { - nameSize := p[70:72] - bs := binary.BigEndian.Uint16(nameSize) - total := 72 + bs - - ffif.Platform = [4]byte(p[0:4]) - ffif.TypeSignature = [4]byte(p[4:8]) - ffif.CreatorSignature = [4]byte(p[8:12]) - ffif.Flags = [4]byte(p[12:16]) - ffif.PlatformFlags = [4]byte(p[16:20]) - ffif.RSVD = [32]byte(p[20:52]) - ffif.CreateDate = [8]byte(p[52:60]) - ffif.ModifyDate = [8]byte(p[60:68]) - ffif.NameScript = [2]byte(p[68:70]) - ffif.NameSize = [2]byte(p[70:72]) - ffif.Name = p[72:total] - - if len(p) > int(total) { - ffif.CommentSize = [2]byte(p[total : total+2]) - commentLen := binary.BigEndian.Uint16(ffif.CommentSize[:]) - commentStartPos := int(total) + 2 - commentEndPos := int(total) + 2 + int(commentLen) - - ffif.Comment = p[commentStartPos:commentEndPos] - - //total = uint16(commentEndPos) + if err := ffif.UnmarshalBinary(p); err != nil { + return 0, err } - return len(p), nil } func (ffif *FlatFileInformationFork) UnmarshalBinary(b []byte) error { - nameSize := b[70:72] - bs := binary.BigEndian.Uint16(nameSize) - nameEnd := 72 + bs + if len(b) < flatFileInfoForkMinLen { + return fmt.Errorf("flat file information fork too short: %d bytes, need at least %d", len(b), flatFileInfoForkMinLen) + } + + bs := binary.BigEndian.Uint16(b[70:72]) + nameEnd := flatFileInfoForkMinLen + int(bs) + if len(b) < nameEnd { + return fmt.Errorf("flat file information fork name overruns buffer: need %d bytes, have %d", nameEnd, len(b)) + } ffif.Platform = [4]byte(b[0:4]) ffif.TypeSignature = [4]byte(b[4:8]) @@ -203,12 +190,18 @@ func (ffif *FlatFileInformationFork) UnmarshalBinary(b []byte) error { ffif.NameSize = [2]byte(b[70:72]) ffif.Name = b[72:nameEnd] - if len(b) > int(nameEnd) { + if len(b) > nameEnd { + if len(b) < nameEnd+2 { + return fmt.Errorf("flat file information fork comment size overruns buffer") + } ffif.CommentSize = [2]byte(b[nameEnd : nameEnd+2]) commentLen := binary.BigEndian.Uint16(ffif.CommentSize[:]) - commentStartPos := int(nameEnd) + 2 - commentEndPos := int(nameEnd) + 2 + int(commentLen) + commentStartPos := nameEnd + 2 + commentEndPos := nameEnd + 2 + int(commentLen) + if len(b) < commentEndPos { + return fmt.Errorf("flat file information fork comment overruns buffer: need %d bytes, have %d", commentEndPos, len(b)) + } ffif.Comment = b[commentStartPos:commentEndPos] } diff --git a/hotline/flattened_file_object_test.go b/hotline/flattened_file_object_test.go index 6279157..6c525c4 100644 --- a/hotline/flattened_file_object_test.go +++ b/hotline/flattened_file_object_test.go @@ -35,6 +35,29 @@ func TestFlatFileInformationFork_UnmarshalBinary(t *testing.T) { }, wantErr: assert.NoError, }, + { + name: "when the buffer is shorter than the fixed header returns an error instead of panicking", + args: args{b: make([]byte, 10)}, + wantErr: assert.Error, + }, + { + name: "when the declared name size overruns the buffer returns an error", + args: args{b: func() []byte { + b := make([]byte, 72) + binary.BigEndian.PutUint16(b[70:72], 50) // claims a 50-byte name that isn't present + return b + }()}, + wantErr: assert.Error, + }, + { + name: "when the declared comment size overruns the buffer returns an error", + args: args{b: func() []byte { + b := make([]byte, 74) // 72 header + 2 comment-size bytes, no name + binary.BigEndian.PutUint16(b[72:74], 50) // claims a 50-byte comment that isn't present + return b + }()}, + wantErr: assert.Error, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/hotline/panic.go b/hotline/panic.go index d7376db..2207911 100644 --- a/hotline/panic.go +++ b/hotline/panic.go @@ -1,15 +1,16 @@ package hotline import ( - "fmt" "log/slog" "runtime/debug" ) -// dontPanic logs panics instead of crashing +// dontPanic recovers from a panic and logs it (with a stack trace) instead of +// letting it crash the goroutine. The trace is recorded via the structured +// logger only; it is intentionally not written to stdout, so a client that can +// repeatedly trigger a panic cannot flood stdout. func dontPanic(logger *slog.Logger) { if r := recover(); r != nil { - fmt.Println("stacktrace from panic: \n" + string(debug.Stack())) logger.Error("PANIC", "err", r, "trace", string(debug.Stack())) } } diff --git a/internal/mobius/transaction_handlers.go b/internal/mobius/transaction_handlers.go index 6458bc8..b629be8 100644 --- a/internal/mobius/transaction_handlers.go +++ b/internal/mobius/transaction_handlers.go @@ -97,6 +97,8 @@ const ( ErrMsgFileResumeData = "Invalid file resume data." ErrMsgAccountNotFound = "Account not found." ErrMsgUserNotFound = "User not found." + ErrMsgInvalidUserID = "Invalid user ID." + ErrMsgInvalidChatID = "Invalid chat ID." ErrMsgCreateAlias = "Error creating alias" ErrMsgUpdateAccount = "Error updating account." ErrMsgDeleteAccount = "Error deleting account." @@ -195,9 +197,13 @@ func HandleChatSend(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotli // All clients *except* Frogblast omit this field for public chat, but Frogblast sends a value of 00 00 00 00. chatID := t.GetField(hotline.FieldChatID).Data if chatID != nil && !bytes.Equal([]byte{0, 0, 0, 0}, chatID) { + privChatID, ok := hotline.ChatIDFromBytes(chatID) + if !ok { + return cc.NewErrReply(t, ErrMsgInvalidChatID) + } // send the message to all connected clients of the private chat - for _, c := range cc.Server.ChatMgr.Members([4]byte(chatID)) { + for _, c := range cc.Server.ChatMgr.Members(privChatID) { res = append(res, hotline.NewTransaction( hotline.TranChatMsg, c.ID, @@ -240,9 +246,14 @@ func HandleSendInstantMsg(cc *hotline.ClientConn, t *hotline.Transaction) (res [ msg := t.GetField(hotline.FieldData) userID := t.GetField(hotline.FieldUserID) + targetID, ok := hotline.ClientIDFromBytes(userID.Data) + if !ok { + return cc.NewErrReply(t, ErrMsgInvalidUserID) + } + reply := hotline.NewTransaction( hotline.TranServerMsg, - [2]byte(userID.Data), + targetID, hotline.NewField(hotline.FieldData, msg.Data), hotline.NewField(hotline.FieldUserName, cc.UserName), hotline.NewField(hotline.FieldUserID, cc.ID[:]), @@ -255,7 +266,7 @@ func HandleSendInstantMsg(cc *hotline.ClientConn, t *hotline.Transaction) (res [ reply.Fields = append(reply.Fields, hotline.NewField(hotline.FieldQuotingMsg, t.GetField(hotline.FieldQuotingMsg).Data)) } - otherClient := cc.Server.ClientMgr.Get([2]byte(userID.Data)) + otherClient := cc.Server.ClientMgr.Get(targetID) if otherClient == nil { // Target user is no longer connected. The protocol defines no reply for // this transaction, so there is nothing to send back. @@ -787,6 +798,13 @@ func HandleUpdateUser(cc *hotline.ClientConn, t *hotline.Transaction) (res []hot for _, field := range t.Fields { var subFields []hotline.Field + // The first two bytes are the sub-field count; reject a malformed block + // that is too short to contain it rather than slicing out of range. + if len(field.Data) < 2 { + cc.Logger.Error("update user: malformed sub-field block", "len", len(field.Data)) + return cc.NewErrReply(t, ErrMsgUpdateAccount) + } + // Create a new scanner for parsing incoming bytes into transaction tokens scanner := bufio.NewScanner(bytes.NewReader(field.Data[2:])) scanner.Split(hotline.FieldScanner) @@ -1047,9 +1065,12 @@ func HandleGetClientInfoText(cc *hotline.ClientConn, t *hotline.Transaction) (re return cc.NewErrReply(t, ErrMsgNotAllowedGetClientInfo) } - clientID := t.GetField(hotline.FieldUserID).Data + clientID, ok := hotline.ClientIDFromBytes(t.GetField(hotline.FieldUserID).Data) + if !ok { + return cc.NewErrReply(t, ErrMsgInvalidUserID) + } - clientConn := cc.Server.ClientMgr.Get(hotline.ClientID(clientID)) + clientConn := cc.Server.ClientMgr.Get(clientID) if clientConn == nil { return cc.NewErrReply(t, ErrMsgUserNotFound) } @@ -1228,8 +1249,14 @@ func HandleDisconnectUser(cc *hotline.ClientConn, t *hotline.Transaction) (res [ return cc.NewErrReply(t, ErrMsgNotAllowedDisconnectUsers) } - clientID := [2]byte(t.GetField(hotline.FieldUserID).Data) + clientID, ok := hotline.ClientIDFromBytes(t.GetField(hotline.FieldUserID).Data) + if !ok { + return cc.NewErrReply(t, ErrMsgInvalidUserID) + } clientConn := cc.Server.ClientMgr.Get(clientID) + if clientConn == nil { + return cc.NewErrReply(t, ErrMsgUserNotFound) + } if clientConn.Authorize(hotline.AccessCannotBeDiscon) { return cc.NewErrReply(t, clientConn.Account.Login+" is not allowed to be disconnected.") @@ -1238,8 +1265,8 @@ func HandleDisconnectUser(cc *hotline.ClientConn, t *hotline.Transaction) (res [ // If FieldOptions is set, then the client IP is banned in addition to disconnected. // 00 01 = temporary ban // 00 02 = permanent ban - if t.GetField(hotline.FieldOptions).Data != nil { - switch t.GetField(hotline.FieldOptions).Data[1] { + if options := t.GetField(hotline.FieldOptions).Data; len(options) > 1 { + switch options[1] { case 1: // send message: "You are temporarily banned on this server" cc.Logger.Info("Disconnect & temporarily ban " + string(clientConn.UserName)) @@ -1645,7 +1672,9 @@ func HandleDownloadFile(cc *hotline.ClientConn, t *hotline.Transaction) (res []h return cc.NewErrReply(t, ErrMsgFileResumeData) } // TODO: handle rsrc fork offset - dataOffset = int64(binary.BigEndian.Uint32(frd.ForkInfoList[0].DataSize[:])) + if len(frd.ForkInfoList) > 0 { + dataOffset = int64(binary.BigEndian.Uint32(frd.ForkInfoList[0].DataSize[:])) + } } fullFilePath, err := hotline.ReadPath(cc.FileRoot(), filePath, fileName, cc.TextDecoder()) @@ -2037,13 +2066,20 @@ func HandleInviteNewChat(cc *hotline.ClientConn, t *hotline.Transaction) (res [] } // Client to Invite - targetID := t.GetField(hotline.FieldUserID).Data + targetID, ok := hotline.ClientIDFromBytes(t.GetField(hotline.FieldUserID).Data) + if !ok { + return cc.NewErrReply(t, ErrMsgInvalidUserID) + } + + // Check if target user has "Refuse private chat" flag + targetClient := cc.Server.ClientMgr.Get(targetID) + if targetClient == nil { + return cc.NewErrReply(t, ErrMsgUserNotFound) + } // Create a new chat with self as initial member. newChatID := cc.Server.ChatMgr.New(cc) - // Check if target user has "Refuse private chat" flag - targetClient := cc.Server.ClientMgr.Get([2]byte(targetID)) flagBitmap := big.NewInt(int64(binary.BigEndian.Uint16(targetClient.Flags[:]))) if flagBitmap.Bit(hotline.UserFlagRefusePChat) == 1 { res = append(res, @@ -2060,7 +2096,7 @@ func HandleInviteNewChat(cc *hotline.ClientConn, t *hotline.Transaction) (res [] res = append(res, hotline.NewTransaction( hotline.TranInviteToChat, - [2]byte(targetID), + targetID, hotline.NewField(hotline.FieldChatID, newChatID[:]), hotline.NewField(hotline.FieldUserName, cc.UserName), hotline.NewField(hotline.FieldUserID, cc.ID[:]), @@ -2093,13 +2129,16 @@ func HandleInviteToChat(cc *hotline.ClientConn, t *hotline.Transaction) (res []h } // Client to Invite - targetID := t.GetField(hotline.FieldUserID).Data + targetID, ok := hotline.ClientIDFromBytes(t.GetField(hotline.FieldUserID).Data) + if !ok { + return cc.NewErrReply(t, ErrMsgInvalidUserID) + } chatID := t.GetField(hotline.FieldChatID).Data return []hotline.Transaction{ hotline.NewTransaction( hotline.TranInviteToChat, - [2]byte(targetID), + targetID, hotline.NewField(hotline.FieldChatID, chatID), hotline.NewField(hotline.FieldUserName, cc.UserName), hotline.NewField(hotline.FieldUserID, cc.ID[:]), @@ -2122,7 +2161,11 @@ func HandleInviteToChat(cc *hotline.ClientConn, t *hotline.Transaction) (res []h // // Reply is not expected. func HandleRejectChatInvite(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotline.Transaction) { - chatID := [4]byte(t.GetField(hotline.FieldChatID).Data) + chatID, ok := hotline.ChatIDFromBytes(t.GetField(hotline.FieldChatID).Data) + if !ok { + cc.Logger.Error("reject chat invite: invalid chat ID") + return res + } for _, c := range cc.Server.ChatMgr.Members(chatID) { res = append(res, @@ -2147,15 +2190,18 @@ func HandleRejectChatInvite(cc *hotline.ClientConn, t *hotline.Transaction) (res // - 115 Chat subject Current chat room subject // - 300 User name with info Repeated - User information for each chat member func HandleJoinChat(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotline.Transaction) { - chatID := t.GetField(hotline.FieldChatID).Data + chatID, ok := hotline.ChatIDFromBytes(t.GetField(hotline.FieldChatID).Data) + if !ok { + return cc.NewErrReply(t, ErrMsgInvalidChatID) + } // Send TranNotifyChatChangeUser to current members of the chat to inform of new user - for _, c := range cc.Server.ChatMgr.Members([4]byte(chatID)) { + for _, c := range cc.Server.ChatMgr.Members(chatID) { res = append(res, hotline.NewTransaction( hotline.TranNotifyChatChangeUser, c.ID, - hotline.NewField(hotline.FieldChatID, chatID), + hotline.NewField(hotline.FieldChatID, chatID[:]), hotline.NewField(hotline.FieldUserName, cc.UserName), hotline.NewField(hotline.FieldUserID, cc.ID[:]), hotline.NewField(hotline.FieldUserIconID, cc.Icon), @@ -2164,12 +2210,12 @@ func HandleJoinChat(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotli ) } - cc.Server.ChatMgr.Join(hotline.ChatID(chatID), cc) + cc.Server.ChatMgr.Join(chatID, cc) - subject := cc.Server.ChatMgr.GetSubject(hotline.ChatID(chatID)) + subject := cc.Server.ChatMgr.GetSubject(chatID) replyFields := []hotline.Field{hotline.NewField(hotline.FieldChatSubject, []byte(subject))} - for _, c := range cc.Server.ChatMgr.Members([4]byte(chatID)) { + for _, c := range cc.Server.ChatMgr.Members(chatID) { b, err := io.ReadAll(&hotline.User{ ID: c.ID, Icon: c.Icon, @@ -2193,17 +2239,21 @@ func HandleJoinChat(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotli // // Reply is not expected. func HandleLeaveChat(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotline.Transaction) { - chatID := t.GetField(hotline.FieldChatID).Data + chatID, ok := hotline.ChatIDFromBytes(t.GetField(hotline.FieldChatID).Data) + if !ok { + cc.Logger.Error("leave chat: invalid chat ID") + return res + } - cc.Server.ChatMgr.Leave([4]byte(chatID), cc.ID) + cc.Server.ChatMgr.Leave(chatID, cc.ID) // Notify members of the private chat that the user has left - for _, c := range cc.Server.ChatMgr.Members(hotline.ChatID(chatID)) { + for _, c := range cc.Server.ChatMgr.Members(chatID) { res = append(res, hotline.NewTransaction( hotline.TranNotifyChatDeleteUser, c.ID, - hotline.NewField(hotline.FieldChatID, chatID), + hotline.NewField(hotline.FieldChatID, chatID[:]), hotline.NewField(hotline.FieldUserID, cc.ID[:]), ), ) @@ -2220,17 +2270,21 @@ func HandleLeaveChat(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotl // // Reply is not expected. func HandleSetChatSubject(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotline.Transaction) { - chatID := t.GetField(hotline.FieldChatID).Data + chatID, ok := hotline.ChatIDFromBytes(t.GetField(hotline.FieldChatID).Data) + if !ok { + cc.Logger.Error("set chat subject: invalid chat ID") + return res + } - cc.Server.ChatMgr.SetSubject([4]byte(chatID), string(t.GetField(hotline.FieldChatSubject).Data)) + cc.Server.ChatMgr.SetSubject(chatID, string(t.GetField(hotline.FieldChatSubject).Data)) // Notify chat members of new subject. - for _, c := range cc.Server.ChatMgr.Members([4]byte(chatID)) { + for _, c := range cc.Server.ChatMgr.Members(chatID) { res = append(res, hotline.NewTransaction( hotline.TranNotifyChatSubject, c.ID, - hotline.NewField(hotline.FieldChatID, chatID), + hotline.NewField(hotline.FieldChatID, chatID[:]), hotline.NewField(hotline.FieldChatSubject, t.GetField(hotline.FieldChatSubject).Data), ), ) diff --git a/internal/mobius/transaction_handlers_test.go b/internal/mobius/transaction_handlers_test.go index 644bdaa..3d9b95a 100644 --- a/internal/mobius/transaction_handlers_test.go +++ b/internal/mobius/transaction_handlers_test.go @@ -3002,6 +3002,61 @@ func TestHandleDelNewsArt(t *testing.T) { } } +// Malformed client-supplied ID fields (wrong length) must produce an error reply +// rather than panicking the handler goroutine. +func TestHandlers_malformedIDFieldsReturnErrorReply(t *testing.T) { + accessWith := func(bit int) hotline.AccessBitmap { + var bits hotline.AccessBitmap + bits.Set(bit) + return bits + } + + tests := []struct { + name string + handler func(*hotline.ClientConn, *hotline.Transaction) []hotline.Transaction + cc *hotline.ClientConn + tran hotline.Transaction + wantMsg string + }{ + { + name: "HandleJoinChat with a 1-byte chat ID", + handler: HandleJoinChat, + cc: &hotline.ClientConn{ID: [2]byte{0, 1}, Logger: NewTestLogger()}, + tran: hotline.NewTransaction(hotline.TranJoinChat, [2]byte{0, 1}, + hotline.NewField(hotline.FieldChatID, []byte{0x01})), + wantMsg: ErrMsgInvalidChatID, + }, + { + name: "HandleSendInstantMsg with a 1-byte user ID", + handler: HandleSendInstantMsg, + cc: &hotline.ClientConn{ID: [2]byte{0, 1}, Logger: NewTestLogger(), + Account: &hotline.Account{Access: accessWith(hotline.AccessSendPrivMsg)}}, + tran: hotline.NewTransaction(hotline.TranSendInstantMsg, [2]byte{0, 1}, + hotline.NewField(hotline.FieldUserID, []byte{0x01})), + wantMsg: ErrMsgInvalidUserID, + }, + { + name: "HandleDisconnectUser with a 3-byte user ID", + handler: HandleDisconnectUser, + cc: &hotline.ClientConn{ID: [2]byte{0, 1}, Logger: NewTestLogger(), + Account: &hotline.Account{Access: accessWith(hotline.AccessDisconUser)}}, + tran: hotline.NewTransaction(hotline.TranDisconnectUser, [2]byte{0, 1}, + hotline.NewField(hotline.FieldUserID, []byte{0x01, 0x02, 0x03})), + wantMsg: ErrMsgInvalidUserID, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotRes := tt.handler(tt.cc, &tt.tran) // must not panic + if assert.Len(t, gotRes, 1) { + assert.Equal(t, [4]byte{0, 0, 0, 1}, gotRes[0].ErrorCode) + assert.Equal(t, tt.wantMsg, string(gotRes[0].GetField(hotline.FieldError).Data)) + } + }) + } +} + func TestHandleDisconnectUser(t *testing.T) { type args struct { cc *hotline.ClientConn -- cgit