fix: harden Telnet login stream boundaries and trace redaction

client.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"io"
 	"net"
+	"strings"
 	"sync"
 )
 
@@ -49,6 +50,8 @@ func NewClientConn(conn net.Conn, config *ClientConfig) (*Client, error) {
 
 func newClientConn(conn net.Conn, config clientConfig) (*Client, error) {
 	transport := newTransport(conn, config.transportConfig())
+	autoLogin := config.User != "" || config.Password != ""
+	transport.setTraceRedaction(autoLogin)
 	var reader io.Reader = transport
 	var writer io.Writer = transport
 	if config.WrapStream != nil {
@@ -62,21 +65,24 @@ func newClientConn(conn net.Conn, config clientConfig) (*Client, error) {
 	// Keep the same buffered reader for login and the interactive session. Login
 	// reads only through the matched prompt, so bytes that arrived in the same
 	// transport read after that prompt remain available to Client.Read.
-	bufferedReader := bufio.NewReaderSize(reader, 4096)
+	bufferedReader := bufio.NewReaderSize(reader, loginReadBufferSize)
 	client := &Client{
 		conn:      conn,
 		transport: transport,
 		reader:    bufferedReader,
 		writer:    writer,
 	}
-	if config.User != "" || config.Password != "" {
-		transport.setTraceRedaction(true)
-		err := login(conn, bufferedReader, writer, config, transport.trace)
-		transport.setTraceRedaction(false)
+	if autoLogin {
+		replay, err := login(conn, bufferedReader, writer, config, transport.trace)
 		if err != nil {
+			// Keep redaction enabled while a stream wrapper flushes on Close.
 			_ = client.Close()
 			return nil, fmt.Errorf("telnet: handshake failed: %w", err)
 		}
+		if replay != "" {
+			client.reader = io.MultiReader(strings.NewReader(replay), bufferedReader)
+		}
+		transport.setTraceRedaction(false)
 	}
 	return client, nil
 }

decoder.go

@@ -11,6 +11,8 @@ const (
 	decoderSubnegotiationOption
 	decoderSubnegotiationData
 	decoderSubnegotiationIAC
+	decoderDiscardSubnegotiation
+	decoderDiscardSubnegotiationIAC
 )
 
 type eventType uint8
@@ -43,6 +45,7 @@ func newDecoder(maxSubnegotiation int) *decoder {
 func (d *decoder) feed(input []byte) ([]byte, []telnetEvent, error) {
 	data := make([]byte, 0, len(input))
 	events := make([]telnetEvent, 0, 2)
+	var decodeErr error
 
 	for _, value := range input {
 		switch d.state {
@@ -87,37 +90,54 @@ func (d *decoder) feed(input []byte) ([]byte, []telnetEvent, error) {
 				d.state = decoderSubnegotiationIAC
 				continue
 			}
-			if err := d.appendSubByte(value); err != nil {
-				return data, events, err
+			if err := d.appendSubByte(value); err != nil && decodeErr == nil {
+				decodeErr = err
 			}
 
 		case decoderSubnegotiationIAC:
 			switch value {
 			case se:
 				events = append(events, telnetEvent{
-					typ: subnegotiationEvent,
-					option: d.subOption,
+					typ:        subnegotiationEvent,
+					option:     d.subOption,
 					parameters: append([]byte(nil), d.subData...),
 				})
 				d.subData = d.subData[:0]
 				d.state = decoderData
 			case iac:
 				if err := d.appendSubByte(iac); err != nil {
-					return data, events, err
+					if decodeErr == nil {
+						decodeErr = err
+					}
+					continue
 				}
 				d.state = decoderSubnegotiationData
 			default:
 				// Ignore malformed commands inside SB while keeping the stream framed.
 				d.state = decoderSubnegotiationData
 			}
+		case decoderDiscardSubnegotiation:
+			if value == iac {
+				d.state = decoderDiscardSubnegotiationIAC
+			}
+		case decoderDiscardSubnegotiationIAC:
+			// IAC IAC is escaped payload even while discarding. Only the
+			// actual IAC SE terminator restores the application stream.
+			if value == se {
+				d.state = decoderData
+			} else {
+				d.state = decoderDiscardSubnegotiation
+			}
 		}
 	}
-	return data, events, nil
+	return data, events, decodeErr
 }
 
 func (d *decoder) appendSubByte(value byte) error {
 	if len(d.subData) >= d.maxSubnegotiation {
-		d.state = decoderData
+		// Report once, but continue scanning this chunk and subsequent reads
+		// through IAC SE. Never expose an oversized SB's tail as terminal data.
+		d.state = decoderDiscardSubnegotiation
 		d.subData = d.subData[:0]
 		return fmt.Errorf("%w: limit %d", ErrSubnegotiationTooLong, d.maxSubnegotiation)
 	}

decoder_recovery_test.go

@@ -0,0 +1,92 @@
+package tclientlib
+
+import (
+	"bytes"
+	"errors"
+	"fmt"
+	"testing"
+)
+
+func TestOversizedSubnegotiationKeepsFramingEverySplit(t *testing.T) {
+	// The escaped IAC followed by SE is payload, not an SB terminator.
+	wire := []byte{'a', iac, sb, terminalType, 1, 2, 3, 'X', iac, iac, se, 'Y', iac, se,
+		'b', iac, will, echoOption, iac, sb, terminalType, 1, iac, se}
+	for split := 0; split <= len(wire); split++ {
+		t.Run(fmt.Sprint(split), func(t *testing.T) {
+			d := newDecoder(2)
+			var output []byte
+			var events []telnetEvent
+			errorsSeen := 0
+			for _, part := range [][]byte{wire[:split], wire[split:]} {
+				data, ev, err := d.feed(part)
+				output = append(output, data...)
+				events = append(events, ev...)
+				if err != nil {
+					if !errors.Is(err, ErrSubnegotiationTooLong) {
+						t.Fatal(err)
+					}
+					errorsSeen++
+				}
+				if len(d.subData) > 2 {
+					t.Fatal("unbounded subnegotiation")
+				}
+			}
+			if !bytes.Equal(output, []byte("ab")) {
+				t.Fatalf("application data = %q", output)
+			}
+			if errorsSeen != 1 {
+				t.Fatalf("errors = %d, want 1", errorsSeen)
+			}
+			if len(events) != 2 || events[0].typ != negotiationEvent || events[1].typ != subnegotiationEvent || !bytes.Equal(events[1].parameters, []byte{1}) {
+				t.Fatalf("events = %#v", events)
+			}
+		})
+	}
+}
+
+func TestOversizedEscapedIACEntersDiscardState(t *testing.T) {
+	d := newDecoder(1)
+	_, _, err := d.feed([]byte{iac, sb, terminalType, 1, iac, iac})
+	if !errors.Is(err, ErrSubnegotiationTooLong) {
+		t.Fatalf("got %v", err)
+	}
+	data, ev, err := d.feed([]byte{'X', iac, se, 'z'})
+	if err != nil || len(ev) != 0 || string(data) != "z" {
+		t.Fatalf("data=%q events=%v err=%v", data, ev, err)
+	}
+}
+
+func FuzzOversizedSubnegotiationRecovery(f *testing.F) {
+	f.Add([]byte{iac, se, iac, iac}, uint8(1))
+	f.Add([]byte("ignored-data"), uint8(7))
+	f.Fuzz(func(t *testing.T, payload []byte, size uint8) {
+		if len(payload) > 4096 {
+			payload = payload[:4096]
+		}
+		wire := []byte{'a', iac, sb, terminalType, 1, 2, 3}
+		wire = appendEscapedIAC(wire, payload)
+		wire = append(wire, iac, se, 'b')
+		d := newDecoder(2)
+		var output []byte
+		errorsSeen := 0
+		for _, chunk := range streamChunks(wire, int(size)+1) {
+			data, events, err := d.feed(chunk)
+			output = append(output, data...)
+			if len(events) != 0 {
+				t.Fatalf("oversized SB produced events: %v", events)
+			}
+			if err != nil {
+				if !errors.Is(err, ErrSubnegotiationTooLong) {
+					t.Fatal(err)
+				}
+				errorsSeen++
+			}
+			if len(d.subData) > 2 {
+				t.Fatal("unbounded SB data")
+			}
+		}
+		if string(output) != "ab" || errorsSeen != 1 {
+			t.Fatalf("data=%q errors=%d", output, errorsSeen)
+		}
+	})
+}

docs/login-stream.md

@@ -0,0 +1,82 @@
+# Login stream boundaries
+
+`Dial` and `NewClientConn` keep the existing public API. Login and the session
+share a 4 KiB buffered application reader, after Telnet framing and any
+`WrapStream` charset conversion.
+
+## Reads are not messages
+
+The login loop fills the buffer only when it is empty, then scans the bytes
+already available using `Peek` and `Discard`. It does not call `ReadByte` in the
+login loop, wait for a full 4 KiB block, or add sleeps. A short `Password:` prompt
+can therefore receive a response immediately.
+
+Text normalization still examines bytes to carry ANSI, UTF-8, CR/LF, and
+backspace state across chunks. It does not copy the entire current line or run
+all regular expressions after each byte.
+
+Completed CRLF/LF lines are evaluated once. At the end of an available chunk,
+only interactive prompt matching is allowed, and only for a candidate of at
+most 512 bytes. Failure messages and password-expiry narratives wait for a line
+ending, EOF, or the overall login timeout. For example:
+
+```text
+Authentication failed count: 0
+Password expired policy: disabled
+New password policy: 90 days
+```
+
+These remain banners even when a read stops immediately after `failed`,
+`expired`, or `New password`. An unterminated explicit failure can be reported
+at EOF or timeout; finalization never sends credentials or declares success on
+a closed connection.
+
+An unfinished password-change input must end in `:` or `：` before being
+classified. `New password:` is rejected without submitting the managed old
+password. Custom `PasswordChangePrompt` and `FailurePrompt` still replace their
+respective defaults; their narrative matches obey the same completion rules.
+Other custom prompts remain full matches against the current normalized line.
+
+Overlong logical lines are ignored until a line reset or ending. They are not
+converted into a sliding suffix that could accidentally look like a prompt.
+
+## Preserving session data
+
+Only the consumed portion of a chunk is discarded. Bytes after the accepted
+shell prompt remain in the same reader. When CR/LF was consumed to establish a
+line boundary, that one- or two-byte terminator is replayed before the buffered
+session data. There is no second unbounded transcript or replay buffer.
+
+For `router#\r\nALARM`, the next `Client.Read` still sees `\r\nALARM`; the accepted
+prompt is not replayed. ANSI formatting already consumed with the prompt is
+not reconstructed. The interactive stream itself is not sanitized.
+
+A delimiter-less textual prompt can always be the prefix of a longer message.
+No TCP buffer size or fixed delay can remove that ambiguity. The defaults use
+bounded prompt shapes and known-banner filtering; unusual assets should use
+precise platform-specific prompt expressions. This is not a protocol-level
+proof of authentication.
+
+## Diagnostics and malformed subnegotiations
+
+Trace redaction is applied centrally to wire/application bytes and to both
+received and generated `SB` payloads. Option names, lengths, and login-state
+events remain visible. Redaction starts before `WrapStream` is invoked and
+stays enabled through failed-login cleanup, including writer flushes. After a
+successful login, normal trace behavior resumes; post-login traffic and the
+standalone monitor can still contain secrets.
+
+An oversized `SB` reports `ErrSubnegotiationTooLong` and discards its payload
+until the real `IAC SE`, including across reads. Escaped `IAC IAC` does not end
+discarding. Following application bytes and valid negotiations in the same
+chunk are retained. A caller that continues reading after the error never
+receives the oversized subnegotiation's tail as terminal data.
+
+## Regression checks
+
+The added tests exercise deterministic one-byte, split-point, 1 KiB, and 4 KiB
+reads, interactive `net.Pipe` prompts without newlines, EOF/timeout results,
+UTF-8 and ANSI fragments, long banners, environment subnegotiation logs, and
+cleanup writes. Fuzz targets check diagnostic-banner chunking and oversized
+subnegotiation recovery. `BenchmarkLoginLongBanner` measures the 1 KiB, 4 KiB,
+and 64 KiB banner paths without adding timing assertions to unit tests.