From ed112e14cc03691057afebe8097b170eed7bf326 Mon Sep 17 00:00:00 2001 From: MorquinDevlar Date: Sun, 4 Oct 2026 11:21:22 +0200 Subject: [PATCH] fix(telnet): stop a short NAWS report from crashing the server TelnetParseScreenSizePayload checked for at least 3 bytes and then read 4. A NAWS report with only 3 bytes of payload read past its end and panicked on the connection goroutine, which has no recover, so the whole server exited. Any client could trigger it before logging in. Require 4 bytes. Add a table test for the parser and a regression test that sends the short report through TelnetIACHandler. --- internal/inputhandlers/term_iac_test.go | 16 +++++++++ internal/term/telnet.go | 4 ++- internal/term/telnet_test.go | 43 +++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 internal/term/telnet_test.go diff --git a/internal/inputhandlers/term_iac_test.go b/internal/inputhandlers/term_iac_test.go index b29dd84560..1bfad1b4ee 100644 --- a/internal/inputhandlers/term_iac_test.go +++ b/internal/inputhandlers/term_iac_test.go @@ -3,7 +3,9 @@ package inputhandlers import ( "testing" + "github.com/GoMudEngine/GoMud/internal/connections" "github.com/GoMudEngine/GoMud/internal/term" + "github.com/stretchr/testify/assert" ) // newEnvironPayload builds the bytes that appear between `IAC SB NEW-ENVIRON IS` @@ -90,3 +92,17 @@ func TestNewEnvironResponseMatcher(t *testing.T) { t.Errorf("expected extracted payload to be detected as Mudlet, payload=%v", payload) } } + +// TestTelnetIACHandlerShortScreenSizeReport sends a NAWS report cut short after +// three bytes. It used to index past the payload and panic on the connection +// goroutine, which has no recover, so the whole server exited. +func TestTelnetIACHandlerShortScreenSizeReport(t *testing.T) { + clientInput := &connections.ClientInput{ + ConnectionId: 1, + DataIn: []byte{term.TELNET_IAC, term.TELNET_SB, term.TELNET_OPT_NAWS, 0, 80, 0}, + } + + assert.NotPanics(t, func() { + TelnetIACHandler(clientInput, map[string]any{}) + }) +} diff --git a/internal/term/telnet.go b/internal/term/telnet.go index 48b5e80645..3470b3e320 100644 --- a/internal/term/telnet.go +++ b/internal/term/telnet.go @@ -167,7 +167,9 @@ func TelnetDONT(what IACByte) []IACByte { func TelnetParseScreenSizePayload(info []byte) (width int, height int, err error) { - if len(info) >= 3 { + // Width and height are two bytes each, so a report shorter than four + // bytes cannot be read. Any client can send one. + if len(info) >= 4 { width = (int(info[0]) << 8) | int(info[1]) height = (int(info[2]) << 8) | int(info[3]) diff --git a/internal/term/telnet_test.go b/internal/term/telnet_test.go new file mode 100644 index 0000000000..6e60562d5d --- /dev/null +++ b/internal/term/telnet_test.go @@ -0,0 +1,43 @@ +package term + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A NAWS report is four bytes: width then height, each high byte first. A +// client can send fewer, and that must come back as an error rather than a +// read past the end of the payload. +func TestTelnetParseScreenSizePayload(t *testing.T) { + tests := []struct { + name string + payload []byte + wantWidth int + wantHeight int + wantErr bool + }{ + {name: "80x24", payload: []byte{0, 80, 0, 24}, wantWidth: 80, wantHeight: 24}, + {name: "high bytes", payload: []byte{1, 0, 0, 50}, wantWidth: 256, wantHeight: 50}, + {name: "trailing IAC SE", payload: []byte{0, 120, 0, 40, TELNET_IAC, TELNET_SE}, wantWidth: 120, wantHeight: 40}, + {name: "three bytes", payload: []byte{0, 80, 0}, wantErr: true}, + {name: "two bytes", payload: []byte{0, 80}, wantErr: true}, + {name: "empty", payload: []byte{}, wantErr: true}, + {name: "nil", payload: nil, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + width, height, err := TelnetParseScreenSizePayload(tt.payload) + if tt.wantErr { + assert.Error(t, err) + return + } + + require.NoError(t, err) + assert.Equal(t, tt.wantWidth, width) + assert.Equal(t, tt.wantHeight, height) + }) + } +}