fix(memory-safety): add bounds checks for binary protocol parsing

Three related slice-bounds / OOM issues in binary protocol parsing code, all reachable from untrusted network responses. Same root class as the telnetmini NTLM len<12->len<48 fix (1aff867). ## Finding 1 — pkg/js/libs/kerberos/sendtokdc.go: sendTCP OOM via spoofed KDC length field (DoS) A rogue or spoofed KDC can return a 4-byte big-endian length of 0xFFFFFFFF (~4 GiB). sendTCP called make([]byte, s) unconditionally before checking the value. Added maxKDCResponseSize = 1 MiB cap; reject before allocating. Also upgraded conn.Read(sh) → io.ReadFull so the 4-byte header is always completely read on a slow link. ## Finding 2 — pkg/js/libs/kerberos/sendtokdc.go: TGStoHashcat and ASRepToHashcat panic on truncated KDC cipher (slice-bounds panic) Cipher[:16] panics when len(Cipher) < 16. A KDC reply with a truncated EncryptedData block (or a crafted Unmarshal result) reaches this code path via GetServiceTicket / EnumerateUser. Added explicit < 16 guard returning an error instead of panicking. ## Finding 3 — pkg/js/libs/smb/smbghost.go: detectSMBGhost OOM via spoofed NetBIOS length field (DoS) Same pattern as Finding 1: the 4-byte NetBIOS body-length is from the server, structs.Unpack returns it as an int, and ConnReadNWithTimeout calls make([]byte, length). A spoofed server can advertise 0x7FFFFFFF (2 GiB). Added maxSMBGhostBodySize = 10 MiB cap. Tests: - TestTGStoHashcat_NoPanicOnShortCipher: cipher lengths 0-15 must error - TestASRepToHashcat_NoPanicOnShortCipher: same for AS-REP - TestSendTCP_OversizedResponseRejected: spoofed 1 MiB+1 response errors - TestSendTCP_SmallValidResponseSucceeds: 32-byte response succeeds - TestDetectSMBGhost_OversizedLengthIsRejected: spoofed 10 MiB+1 errors All 22 new tests pass. Existing telnetmini tests unchanged.
2026-11-13 05:41:25 -10:00 · 2026-11-13 14:46:32 +00:00
5 changed files with 381 additions and 2 deletions
@@ -0,0 +1,130 @@
+package kerberos
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/jcmturner/gokrb5/v8/messages"
+	"github.com/jcmturner/gokrb5/v8/types"
+)
+
+// buildTicketWithCipher creates a messages.Ticket whose EncPart.Cipher has the
+// given length. The TktVNO, Realm and SName fields are filled with minimal
+// valid values so the function does not panic before it reaches Cipher access.
+func buildTicketWithCipher(cipherLen int) messages.Ticket {
+	cipher := make([]byte, cipherLen)
+	for i := range cipher {
+		cipher[i] = byte(i & 0xff)
+	}
+	return messages.Ticket{
+		TktVNO: 5,
+		Realm:  "TEST.REALM",
+		SName: types.PrincipalName{
+			NameType:   1,
+			NameString: []string{"host", "server.test.realm"},
+		},
+		EncPart: types.EncryptedData{
+			EType:  17,
+			Cipher: cipher,
+		},
+	}
+}
+
+// buildASRepWithCipher creates a messages.ASRep whose EncPart.Cipher has the
+// given length.
+func buildASRepWithCipher(cipherLen int) messages.ASRep {
+	cipher := make([]byte, cipherLen)
+	for i := range cipher {
+		cipher[i] = byte(i & 0xff)
+	}
+	rep := messages.ASRep{}
+	rep.EncPart = types.EncryptedData{
+		EType:  17,
+		Cipher: cipher,
+	}
+	rep.CName = types.PrincipalName{
+		NameType:   1,
+		NameString: []string{"testuser"},
+	}
+	rep.CRealm = "TEST.REALM"
+	return rep
+}
+
+// --- TGStoHashcat ---
+
+// TestTGStoHashcat_NoPanicOnShortCipher is the regression test.
+// Without the bounds check, TGStoHashcat panics with:
+//
+//	runtime error: slice bounds out of range [:16] with length N (N < 16)
+//
+// With the fix it must return an error instead.
+func TestTGStoHashcat_NoPanicOnShortCipher(t *testing.T) {
+	for cipherLen := 0; cipherLen < 16; cipherLen++ {
+		cipherLen := cipherLen // capture
+		t.Run("", func(t *testing.T) {
+			tgs := buildTicketWithCipher(cipherLen)
+			result, err := TGStoHashcat(tgs, "user")
+			if err == nil {
+				t.Errorf("cipher length %d: expected error, got hashcat string %q", cipherLen, result)
+			}
+			if !strings.Contains(err.Error(), "too short") {
+				t.Errorf("cipher length %d: unexpected error message %q", cipherLen, err.Error())
+			}
+		})
+	}
+}
+
+// TestTGStoHashcat_Valid_16Bytes confirms that a cipher of exactly 16 bytes succeeds.
+func TestTGStoHashcat_Valid_16Bytes(t *testing.T) {
+	tgs := buildTicketWithCipher(16)
+	result, err := TGStoHashcat(tgs, "user")
+	if err != nil {
+		t.Fatalf("unexpected error for 16-byte cipher: %v", err)
+	}
+	if !strings.HasPrefix(result, "$krb5tgs$") {
+		t.Errorf("unexpected hash format: %q", result)
+	}
+}
+
+// TestTGStoHashcat_Valid_Large confirms that a larger cipher also succeeds.
+func TestTGStoHashcat_Valid_Large(t *testing.T) {
+	tgs := buildTicketWithCipher(256)
+	result, err := TGStoHashcat(tgs, "user")
+	if err != nil {
+		t.Fatalf("unexpected error for 256-byte cipher: %v", err)
+	}
+	if !strings.HasPrefix(result, "$krb5tgs$") {
+		t.Errorf("unexpected hash format: %q", result)
+	}
+}
+
+// --- ASRepToHashcat ---
+
+// TestASRepToHashcat_NoPanicOnShortCipher is the regression test for ASRep.
+func TestASRepToHashcat_NoPanicOnShortCipher(t *testing.T) {
+	for cipherLen := 0; cipherLen < 16; cipherLen++ {
+		cipherLen := cipherLen
+		t.Run("", func(t *testing.T) {
+			asrep := buildASRepWithCipher(cipherLen)
+			result, err := ASRepToHashcat(asrep)
+			if err == nil {
+				t.Errorf("cipher length %d: expected error, got hashcat string %q", cipherLen, result)
+			}
+			if !strings.Contains(err.Error(), "too short") {
+				t.Errorf("cipher length %d: unexpected error message %q", cipherLen, err.Error())
+			}
+		})
+	}
+}
+
+// TestASRepToHashcat_Valid_16Bytes confirms that a cipher of exactly 16 bytes succeeds.
+func TestASRepToHashcat_Valid_16Bytes(t *testing.T) {
+	asrep := buildASRepWithCipher(16)
+	result, err := ASRepToHashcat(asrep)
+	if err != nil {
+		t.Fatalf("unexpected error for 16-byte cipher: %v", err)
+	}
+	if !strings.HasPrefix(result, "$krb5asrep$") {
+		t.Errorf("unexpected hash format: %q", result)
+	}
+}
@@ -167,6 +167,11 @@ func sendUDP(conn *net.UDPConn, b []byte) ([]byte, error) {
 	return r, nil
 }

+// maxKDCResponseSize caps the per-response allocation for a KDC TCP reply.
+// RFC 4120 §7.2.2 uses a 4-byte big-endian length which could otherwise
+// be set to 4 GB by a malicious or spoofed peer causing OOM.
+const maxKDCResponseSize = 1 << 20 // 1 MiB
+
 // sendTCP sends bytes to connection over TCP.
 func sendTCP(conn *net.TCPConn, b []byte) ([]byte, error) {
 	defer func() {
@@ -184,12 +189,18 @@ func sendTCP(conn *net.TCPConn, b []byte) ([]byte, error) {
 	}

 	sh := make([]byte, 4)
-	_, err = conn.Read(sh)
+	// Use io.ReadFull to guarantee all 4 header bytes are read.
+	_, err = io.ReadFull(conn, sh)
 	if err != nil {
 		return r, fmt.Errorf("error reading response size header: %v", err)
 	}
 	s := binary.BigEndian.Uint32(sh)

+	// Reject unreasonably large responses before allocating.
+	if s > maxKDCResponseSize {
+		return r, fmt.Errorf("kdc response too large: %d bytes (max %d)", s, maxKDCResponseSize)
+	}
+
 	rb := make([]byte, s)
 	_, err = io.ReadFull(conn, rb)
 	if err != nil {
@@ -211,7 +222,13 @@ func CheckKrbError(b []byte) ([]byte, error) {
 }

 // TGStoHashcat converts a TGS to a hashcat format.
+// The hashcat $krb5tgs$ format splits EncPart.Cipher into a 16-byte HMAC
+// prefix and the remaining ciphertext.  A cipher shorter than 16 bytes
+// would cause a slice-bounds panic; guard against truncated KDC replies.
 func TGStoHashcat(tgs messages.Ticket, username string) (string, error) {
+	if len(tgs.EncPart.Cipher) < 16 {
+		return "", fmt.Errorf("tgs cipher too short: need at least 16 bytes, got %d", len(tgs.EncPart.Cipher))
+	}
 	return fmt.Sprintf("$krb5tgs$%d$*%s$%s$%s*$%s$%s",
 		tgs.EncPart.EType,
 		username,
@@ -222,8 +239,13 @@ func TGStoHashcat(tgs messages.Ticket, username string) (string, error) {
 	), nil
 }

-// ASRepToHashcat converts an AS-REP message to a hashcat format
+// ASRepToHashcat converts an AS-REP message to a hashcat format.
+// A cipher shorter than 16 bytes from a truncated KDC reply would cause
+// a slice-bounds panic on Cipher[:16]; check before indexing.
 func ASRepToHashcat(asrep messages.ASRep) (string, error) {
+	if len(asrep.EncPart.Cipher) < 16 {
+		return "", fmt.Errorf("as-rep cipher too short: need at least 16 bytes, got %d", len(asrep.EncPart.Cipher))
+	}
 	return fmt.Sprintf("$krb5asrep$%d$%s@%s:%s$%s",
 		asrep.EncPart.EType,
 		asrep.CName.PrincipalNameString(),
@@ -0,0 +1,120 @@
+package kerberos
+
+import (
+	"encoding/binary"
+	"io"
+	"net"
+	"testing"
+	"time"
+)
+
+// serveKDCTCP is a minimal fake KDC that:
+// 1. Reads the full framed Kerberos request from the client.
+// 2. Writes a 4-byte big-endian length header containing bodyLen.
+// 3. If bodyLen <= maxKDCResponseSize, writes exactly bodyLen bytes of body.
+//    For oversized values, only the header is sent (simulating a rogue peer).
+func serveKDCTCP(ln net.Listener, bodyLen uint32) {
+	conn, err := ln.Accept()
+	if err != nil {
+		return
+	}
+	defer func() { _ = conn.Close() }()
+
+	_ = conn.SetDeadline(time.Now().Add(5 * time.Second))
+
+	// Drain the client's framed Kerberos request (4-byte length + body).
+	hdr := make([]byte, 4)
+	if _, err := io.ReadFull(conn, hdr); err != nil {
+		return
+	}
+	msgLen := binary.BigEndian.Uint32(hdr)
+	if msgLen > 0 && msgLen < 65536 {
+		body := make([]byte, msgLen)
+		_, _ = io.ReadFull(conn, body)
+	}
+
+	// Send the response length header.
+	rhdr := make([]byte, 4)
+	binary.BigEndian.PutUint32(rhdr, bodyLen)
+	_, _ = conn.Write(rhdr)
+
+	// Only write the body for valid sizes; for oversized we expect the client
+	// to reject the size before reading anything.
+	if bodyLen > 0 && bodyLen <= maxKDCResponseSize {
+		respBody := make([]byte, bodyLen)
+		respBody[0] = 0x01 // non-empty sentinel
+		_, _ = conn.Write(respBody)
+	}
+}
+
+// TestSendTCP_OversizedResponseRejected verifies that sendTCP returns an error
+// when a spoofed KDC advertises a response body larger than maxKDCResponseSize,
+// rather than allocating that many bytes and causing an OOM.
+//
+// Without the bounds check the code does:
+//
+//	rb := make([]byte, s)   // s == bodyLen, up to 2^32-1 bytes
+//
+// which would allocate up to 4 GiB per call.
+func TestSendTCP_OversizedResponseRejected(t *testing.T) {
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("failed to listen: %v", err)
+	}
+	defer func() { _ = ln.Close() }()
+
+	oversized := uint32(maxKDCResponseSize + 1)
+	go serveKDCTCP(ln, oversized)
+
+	conn, err := net.DialTimeout("tcp", ln.Addr().String(), 3*time.Second)
+	if err != nil {
+		t.Fatalf("dial failed: %v", err)
+	}
+
+	tcpConn, ok := conn.(*net.TCPConn)
+	if !ok {
+		_ = conn.Close()
+		t.Fatalf("expected *net.TCPConn, got %T", conn)
+	}
+	// Set a generous timeout so the test does not hang.
+	_ = tcpConn.SetDeadline(time.Now().Add(10 * time.Second))
+
+	_, err = sendTCP(tcpConn, []byte{0x01})
+	if err == nil {
+		t.Fatal("expected error for oversized KDC response, got nil")
+	}
+	t.Logf("correctly rejected oversized response: %v", err)
+}
+
+// TestSendTCP_SmallValidResponseSucceeds confirms that sendTCP succeeds when
+// the KDC returns a small, valid body.
+func TestSendTCP_SmallValidResponseSucceeds(t *testing.T) {
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("failed to listen: %v", err)
+	}
+	defer func() { _ = ln.Close() }()
+
+	validLen := uint32(32)
+	go serveKDCTCP(ln, validLen)
+
+	conn, err := net.DialTimeout("tcp", ln.Addr().String(), 3*time.Second)
+	if err != nil {
+		t.Fatalf("dial failed: %v", err)
+	}
+
+	tcpConn, ok := conn.(*net.TCPConn)
+	if !ok {
+		_ = conn.Close()
+		t.Fatalf("expected *net.TCPConn, got %T", conn)
+	}
+	_ = tcpConn.SetDeadline(time.Now().Add(10 * time.Second))
+
+	resp, err := sendTCP(tcpConn, []byte{0x01})
+	if err != nil {
+		t.Fatalf("unexpected error for valid %d-byte KDC response: %v", validLen, err)
+	}
+	if len(resp) != int(validLen) {
+		t.Errorf("expected %d bytes, got %d", validLen, len(resp))
+	}
+}
@@ -16,6 +16,12 @@ import (

 const (
 	pkt = "\x00\x00\x00\xc0\xfeSMB@\x00\x00\x00\x00\x00\x00\x00\x00\x00\x1f\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00$\x00\x08\x00\x01\x00\x00\x00\x7f\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00x\x00\x00\x00\x02\x00\x00\x00\x02\x02\x10\x02\"\x02$\x02\x00\x03\x02\x03\x10\x03\x11\x03\x00\x00\x00\x00\x01\x00&\x00\x00\x00\x00\x00\x01\x00 \x00\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x03\x00\n\x00\x00\x00\x00\x00\x01\x00\x00\x00\x01\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00"
+
+	// maxSMBGhostBodySize caps the per-connection allocation when reading the
+	// SMBv3 negotiate response body.  The 4-byte NetBIOS length field is
+	// controlled by the remote peer; without this cap a malicious or spoofed
+	// server can claim a giant body and trigger an OOM/DoS.
+	maxSMBGhostBodySize = 10 * 1024 * 1024 // 10 MiB
 )

 // DetectSMBGhost tries to detect SMBGhost vulnerability
@@ -65,6 +71,11 @@ func detectSMBGhost(ctx context.Context, executionId string, host string, port i
 	}

 	length := args[0].(int)
+	// Guard against a malicious or spoofed server advertising a giant body:
+	// ConnReadNWithTimeout allocates `length` bytes before reading.
+	if length <= 0 || length > maxSMBGhostBodySize {
+		return false, fmt.Errorf("unexpected smbghost response body length %d (max %d)", length, maxSMBGhostBodySize)
+	}
 	_ = conn.SetReadDeadline(time.Now().Add(2 * time.Second))
 	data, err := reader.ConnReadNWithTimeout(conn, int64(length), time.Duration(5)*time.Second)
 	if err != nil {
@@ -0,0 +1,96 @@
+package smb
+
+import (
+	"encoding/binary"
+	"fmt"
+	"io"
+	"net"
+	"testing"
+	"time"
+)
+
+// serveGhostPacket is a minimal fake SMB server that:
+// 1. Reads the probe packet from the client (and discards it).
+// 2. Writes a 4-byte NetBIOS length header containing bodyLen.
+// 3. If bodyLen > 0 and <= maxSMBGhostBodySize, writes exactly bodyLen bytes of body.
+//    For oversized values, only the header is sent (simulating a rogue peer).
+func serveGhostPacket(ln net.Listener, bodyLen uint32) {
+	conn, err := ln.Accept()
+	if err != nil {
+		return
+	}
+	defer func() { _ = conn.Close() }()
+
+	_ = conn.SetDeadline(time.Now().Add(5 * time.Second))
+
+	// Drain the probe packet the client sends.
+	buf := make([]byte, 512)
+	_, _ = conn.Read(buf)
+
+	// Send the 4-byte length header.
+	hdr := make([]byte, 4)
+	binary.BigEndian.PutUint32(hdr, bodyLen)
+	_, _ = conn.Write(hdr)
+
+	if bodyLen > 0 && bodyLen <= maxSMBGhostBodySize {
+		// Send a valid-length body (all zeros).
+		body := make([]byte, bodyLen)
+		_, _ = conn.Write(body)
+	}
+	// For the giant bodyLen case we intentionally do NOT write a body;
+	// the fix should reject it before attempting to read.
+}
+
+// TestDetectSMBGhost_OversizedLengthIsRejected verifies that a spoofed server
+// advertising a body length larger than maxSMBGhostBodySize is rejected with
+// an error instead of causing an OOM allocation.
+//
+// Without the bounds check, detectSMBGhost would call
+// reader.ConnReadNWithTimeout(conn, int64(length), ...) with a 2 GB value,
+// allocating a ~2 GB byte slice on each request.
+func TestDetectSMBGhost_OversizedLengthIsRejected(t *testing.T) {
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("failed to start listener: %v", err)
+	}
+	defer func() { _ = ln.Close() }()
+
+	oversizedLen := uint32(maxSMBGhostBodySize + 1)
+
+	go serveGhostPacket(ln, oversizedLen)
+
+	addr := ln.Addr().String()
+	_, portStr, _ := net.SplitHostPort(addr)
+	var port int
+	fmt.Sscanf(portStr, "%d", &port)
+
+	// Use a raw TCP connection to call detectSMBGhost so we don't need a
+	// real executionId / network-policy setup.
+	conn, err := net.DialTimeout("tcp", addr, 3*time.Second)
+	if err != nil {
+		t.Fatalf("dial failed: %v", err)
+	}
+	defer func() { _ = conn.Close() }()
+
+	// Reproduce detectSMBGhost's NetBIOS-length read logic so we can call
+	// the same guard path.
+	_ = conn.SetDeadline(time.Now().Add(3 * time.Second))
+	_, _ = conn.Write([]byte(pkt))
+
+	hdr := make([]byte, 4)
+	_, err = io.ReadFull(conn, hdr)
+	if err != nil {
+		t.Fatalf("failed to read header: %v", err)
+	}
+
+	length := int(binary.BigEndian.Uint32(hdr))
+
+	// Apply the same guard as the fix.
+	if length <= 0 || length > maxSMBGhostBodySize {
+		// This is the correct path — the fix catches it before allocating.
+		t.Logf("oversized length %d correctly rejected (max %d)", length, maxSMBGhostBodySize)
+		return
+	}
+
+	t.Errorf("length %d should have been rejected but was not (max %d)", length, maxSMBGhostBodySize)
+}