From e7d6834dcbde11f74e487feeb7f98912dc1ef52f Mon Sep 17 00:00:00 2001 From: flink Date: Thu, 16 Jul 2026 01:39:18 -0400 Subject: [PATCH] fix: DetectPostHandshakeRecordsLens background probe bugs Three bugs found while running this in production (invoked unconditionally from NewListener() for every REALITY inbound): 1. PostHandshakeRecordDetectConn.Read() did `data = data[length:]` with no bounds check. A truncated final record from the disguise dest (network cut/RST/read-deadline firing mid-record) can legitimately report a length longer than what actually arrived - io.ReadAll's error is discarded, so `data` may be a partial read. Without the check, `data[length:]` panics with "slice bounds out of range", and since this runs unrecovered in a background goroutine spawned by NewListener, it takes down the entire process - on every single REALITY inbound, not just this calibration probe. Fixed with a length-vs-len(data) check before slicing. 2. Neither background probe goroutine recovers from a panic (this one or any other). Since NewListener spawns them with no caller to propagate an error to, an unrecovered panic here is a process-wide crash triggered by nothing more than normal TLS behavior from the disguise dest. Added `defer func() { recover() }()` to both. 3. Smaller issues found while reviewing the same function: - The two `net.Dial`'d probe connections were never Close()'d - a bounded (one per unique dest/SNI/alpn key, gated by the existing LoadOrStore) but real leak of a goroutine+socket on every REALITY listener startup. Added `defer target.Close()` to both. - CCSDetectConn.Write's background reader goroutine assigned into `err` - Write's own named return value - racing unsynchronized against every `return c.Conn.Write(b)` in the same method (flagged by `go test -race`). Switched to a local variable; nothing here needs to surface the read error to the caller. Verified #1 and #2 with a standalone repro (not included in this diff): a net.Pipe()-backed PostHandshakeRecordDetectConn fed a truncated final record panics on the unpatched code and returns a clean EOF after the fix. --- record_detect.go | 44 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/record_detect.go b/record_detect.go index dc41e4e..0499ffb 100644 --- a/record_detect.go +++ b/record_detect.go @@ -24,6 +24,11 @@ func DetectPostHandshakeRecordsLens(config *Config) { key := config.Dest + " " + sni + " " + strconv.Itoa(alpn) if _, loaded := GlobalPostHandshakeRecordsLens.LoadOrStore(key, false); !loaded { go func() { + // Best-effort background calibration probe against the + // disguise dest — any panic here (this one or an unknown + // future one) must not take down the whole process, since + // this goroutine has no caller to propagate an error to. + defer func() { recover() }() defer func() { val, _ := GlobalPostHandshakeRecordsLens.Load(key) if _, ok := val.(bool); ok { @@ -34,6 +39,11 @@ func DetectPostHandshakeRecordsLens(config *Config) { if err != nil { return } + // Never closed before this fix - a bounded (one per + // unique dest/SNI/alpn key, gated by the LoadOrStore + // above) but real leak of a goroutine+socket per REALITY + // listener startup. + defer target.Close() if config.Xver == 1 || config.Xver == 2 { if _, err = proxyproto.HeaderProxyFromAddrs(config.Xver, target.LocalAddr(), target.RemoteAddr()).WriteTo(target); err != nil { return @@ -64,10 +74,16 @@ func DetectPostHandshakeRecordsLens(config *Config) { io.Copy(io.Discard, uConn) }() go func() { + // Same reasoning as the sibling probe goroutine above: + // best-effort, no caller to report to, must not crash + // the process. + defer func() { recover() }() target, err := net.Dial(config.Type, config.Dest) if err != nil { return } + // Same leak as the sibling probe goroutine above. + defer target.Close() if config.Xver == 1 || config.Xver == 2 { if _, err = proxyproto.HeaderProxyFromAddrs(config.Xver, target.LocalAddr(), target.RemoteAddr()).WriteTo(target); err != nil { return @@ -124,6 +140,17 @@ func (c *PostHandshakeRecordDetectConn) Read(b []byte) (n int, err error) { for { if len(data) >= 5 && bytes.Equal(data[:3], []byte{23, 3, 3}) { length := int(binary.BigEndian.Uint16(data[3:5])) + 5 + // A truncated final record (network cut/RST/read-deadline firing + // mid-record against the disguise dest) can legitimately report a + // length longer than what actually arrived — io.ReadAll's error + // is discarded above, so "data" may be a partial read. Without + // this check, data[length:] panics ("slice bounds out of range"), + // which — since this runs unrecovered in NewListener's + // background goroutine — takes down the whole process on every + // REALITY inbound, not just this probe. + if length > len(data) { + break + } postHandshakeRecordsLens = append(postHandshakeRecordsLens, length) data = data[length:] } else { @@ -145,11 +172,24 @@ func (c *CCSDetectConn) Write(b []byte) (n int, err error) { if len(b) >= 3 && bytes.Equal(b[:3], []byte{20, 3, 3}) { var hasAlert atomic.Bool go func() { + // Used to assign into `err` - Write's own named return value - + // racing unsynchronized against every `return c.Conn.Write(b)` + // below that also assigns it (go test -race flags this). Use a + // local instead; nothing here needs to surface a read error to + // the caller anyway. + // + // hasAlert is deliberately set on ANY read outcome (a genuine + // TLS alert byte, but also EOF/RST/any other error) - kept as-is + // on purpose: this is a conservative disguise-site probe, and + // treating "the site disconnected after our garbage CCS spam, + // for whatever reason" as "assume it noticed" is the safer + // failure mode for a DPI-camouflage heuristic than assuming + // tolerance it can't actually confirm. defer hasAlert.Store(true) buf := make([]byte, 512) for { - _, err = c.Conn.Read(buf) - if err != nil { + _, readErr := c.Conn.Read(buf) + if readErr != nil { return } if buf[0] == 0x15 {