Fix keytab parser panics and signed record-length framing (#1)

src/keytab/CountedOctetString.go

@@ -32,14 +32,20 @@ type CountedOctetString struct {
 // Returns:
 //   - error: An error if the parsing fails.
 func (c *CountedOctetString) FromBytes(data []byte) error {
-	c.RawBytes = data
+	if len(data) < 2 {
+		return fmt.Errorf("data too short to read counted octet string length: need 2 bytes, have %d", len(data))
+	}
 
 	c.Length = binary.BigEndian.Uint16(data[0:2])
-	data = data[2:]
 
-	c.Data = data[:c.Length]
+	if len(data) < 2+int(c.Length) {
+		return fmt.Errorf("data too short to read counted octet string data: need %d bytes, have %d", 2+int(c.Length), len(data))
+	}
+
+	c.Data = data[2 : 2+int(c.Length)]
 
 	c.RawBytesSize = 2 + uint32(c.Length)
+	c.RawBytes = data[:c.RawBytesSize]
 
 	return nil
 }

src/keytab/KeyBlock.go

@@ -29,10 +29,16 @@ type KeyBlock struct {
 // Returns:
 //   - error: An error if the parsing failed.
 func (k *KeyBlock) FromBytes(data []byte) error {
+	if len(data) < 2 {
+		return fmt.Errorf("data too short to read key block encryption type: need 2 bytes, have %d", len(data))
+	}
+
 	k.Type = EncryptionType(binary.BigEndian.Uint16(data[0:2]))
 	k.RawBytesSize = 2
 
-	k.Key.FromBytes(data[2:])
+	if err := k.Key.FromBytes(data[2:]); err != nil {
+		return err
+	}
 	k.RawBytesSize += k.Key.RawBytesSize
 
 	return nil

src/keytab/Keytab.go

@@ -34,20 +34,58 @@ func (k *Keytab) FromBytes(data []byte) error {
 	k.RawBytes = data
 	k.RawBytesSize = 0
 
+	if len(data) < 2 {
+		return fmt.Errorf("data too short to read keytab file format version: need 2 bytes, have %d", len(data))
+	}
 	k.FileFormatVersion = binary.BigEndian.Uint16(data[0:2])
-	data = data[2:]
-	k.RawBytesSize += 2
+	offset := uint32(2)
 
 	k.Entries = make([]KeytabEntry, 0)
 
-	for len(data) != 0 {
+	// Following the version, the file is a sequence of records, each prefixed by a
+	// signed 32-bit length. A positive length is a valid entry of that size, a
+	// negative length is a zero-filled hole whose size is the inverse of the length,
+	// and a length of 0 marks the end of the file.
+	for offset < uint32(len(data)) {
+		if uint32(len(data))-offset < 4 {
+			// Not enough bytes left for another record length prefix.
+			break
+		}
+
+		recordLength := int32(binary.BigEndian.Uint32(data[offset : offset+4]))
+
+		if recordLength == 0 {
+			// End-of-file marker.
+			break
+		}
+
+		if recordLength < 0 {
+			// Zero-filled hole: skip the length prefix and the hole itself.
+			holeSize := uint32(-recordLength)
+			if uint32(len(data))-offset-4 < holeSize {
+				return fmt.Errorf("keytab hole at offset %d claims %d bytes but only %d remain", offset, holeSize, uint32(len(data))-offset-4)
+			}
+			offset += 4 + holeSize
+			continue
+		}
+
+		recordSize := 4 + uint32(recordLength)
+		if uint32(len(data))-offset < recordSize {
+			return fmt.Errorf("keytab entry at offset %d claims %d bytes but only %d remain", offset, recordSize, uint32(len(data))-offset)
+		}
+
 		entry := KeytabEntry{}
-		entry.FromBytes(data)
-		data = data[entry.RawBytesSize:]
+		if err := entry.FromBytes(data[offset : offset+recordSize]); err != nil {
+			return err
+		}
 		k.Entries = append(k.Entries, entry)
-		k.RawBytesSize += entry.RawBytesSize
+
+		// Advance by the full record length defined in the file, not by the number
+		// of bytes the entry fields happened to consume.
+		offset += recordSize
 	}
 
+	k.RawBytesSize = offset
 	k.RawBytes = k.RawBytes[:k.RawBytesSize]
 
 	return nil

src/keytab/KeytabEntry.go

@@ -35,48 +35,73 @@ func (k *KeytabEntry) FromBytes(data []byte) error {
 	k.RawBytes = data
 
 	// Size
+	if len(data) < 4 {
+		return fmt.Errorf("data too short to read keytab entry size: need 4 bytes, have %d", len(data))
+	}
 	k.Size = binary.BigEndian.Uint32(data[0:4])
 	data = data[4:]
+	if uint32(len(data)) < k.Size {
+		return fmt.Errorf("keytab entry size %d exceeds available data: have %d bytes", k.Size, len(data))
+	}
 	data = data[:k.Size]
 	k.RawBytesSize += 4
 
 	// NumComponents
+	if len(data) < 2 {
+		return fmt.Errorf("data too short to read number of components: need 2 bytes, have %d", len(data))
+	}
 	k.NumComponents = binary.BigEndian.Uint16(data[0:2])
 	data = data[2:]
 	k.RawBytesSize += 2
 
 	// Realm
 	k.Realm = CountedOctetString{}
-	k.Realm.FromBytes(data)
+	if err := k.Realm.FromBytes(data); err != nil {
+		return err
+	}
 	data = data[k.Realm.RawBytesSize:]
 	k.RawBytesSize += k.Realm.RawBytesSize
 
 	// Components
+	k.Components = make([]CountedOctetString, 0, k.NumComponents)
 	for i := uint16(0); i < k.NumComponents; i++ {
 		component := CountedOctetString{}
-		component.FromBytes(data)
+		if err := component.FromBytes(data); err != nil {
+			return err
+		}
 		k.Components = append(k.Components, component)
 		data = data[component.RawBytesSize:]
 		k.RawBytesSize += component.RawBytesSize
 	}
 
 	// NameType
+	if len(data) < 4 {
+		return fmt.Errorf("data too short to read name type: need 4 bytes, have %d", len(data))
+	}
 	k.NameType = binary.BigEndian.Uint32(data[0:4])
 	data = data[4:]
 	k.RawBytesSize += 4
 
 	// Timestamp
+	if len(data) < 4 {
+		return fmt.Errorf("data too short to read timestamp: need 4 bytes, have %d", len(data))
+	}
 	k.Timestamp = binary.BigEndian.Uint32(data[0:4])
 	data = data[4:]
 	k.RawBytesSize += 4
 
 	// Vno8
+	if len(data) < 1 {
+		return fmt.Errorf("data too short to read vno8: need 1 byte, have %d", len(data))
+	}
 	k.Vno8 = data[0]
 	data = data[1:]
 	k.RawBytesSize += 1
 
 	// Key
-	k.Key.FromBytes(data)
+	if err := k.Key.FromBytes(data); err != nil {
+		return err
+	}
 	data = data[k.Key.RawBytesSize:]
 	k.RawBytesSize += k.Key.RawBytesSize
 

src/keytab/Keytab_parsing_test.go

@@ -0,0 +1,99 @@
+package keytab
+
+import (
+	"encoding/binary"
+	"testing"
+)
+
+// validEntryBytes builds a single record (4-byte length prefix + body) for a
+// minimal AES256 krbtgt entry without the 32-bit kvno extension.
+func validEntryBytes(t *testing.T) []byte {
+	t.Helper()
+	entry := KeytabEntry{
+		NumComponents: 1,
+		Realm: CountedOctetString{
+			Length: 17,
+			Data:   []byte("TESTSEGMENT.local"),
+		},
+		Components: []CountedOctetString{
+			{Length: 6, Data: []byte("krbtgt")},
+		},
+		NameType:  1,
+		Timestamp: 0,
+		Vno8:      2,
+		Key: KeyBlock{
+			Type: EncryptionType_AES256_CTS_HMAC_SHA1_96,
+			Key:  CountedOctetString{Length: 16, Data: []byte("\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f")},
+		},
+	}
+	if err := entry.UpdateSize(); err != nil {
+		t.Fatalf("UpdateSize failed: %v", err)
+	}
+	b, err := entry.ToBytes()
+	if err != nil {
+		t.Fatalf("ToBytes failed: %v", err)
+	}
+	return b
+}
+
+// Test_Keytab_TruncatedInputReturnsError ensures malformed/truncated input is
+// rejected with an error instead of panicking.
+func Test_Keytab_TruncatedInputReturnsError(t *testing.T) {
+	header := []byte{0x05, 0x02}
+	full := append(append([]byte{}, header...), validEntryBytes(t)...)
+
+	for n := 0; n < len(full); n++ {
+		func() {
+			defer func() {
+				if r := recover(); r != nil {
+					t.Fatalf("parsing truncated input of %d bytes panicked: %v", n, r)
+				}
+			}()
+			kt := Keytab{}
+			_ = kt.FromBytes(full[:n])
+		}()
+	}
+}
+
+// Test_Keytab_HoleIsSkipped ensures a negative record length (a zero-filled
+// hole) is skipped and the following entry is still parsed.
+func Test_Keytab_HoleIsSkipped(t *testing.T) {
+	header := []byte{0x05, 0x02}
+
+	var holeLen int32 = -8
+	hole := make([]byte, 4)
+	binary.BigEndian.PutUint32(hole, uint32(holeLen))
+	hole = append(hole, make([]byte, 8)...)
+
+	entry := validEntryBytes(t)
+
+	data := append(append(append([]byte{}, header...), hole...), entry...)
+
+	kt := Keytab{}
+	if err := kt.FromBytes(data); err != nil {
+		t.Fatalf("FromBytes failed: %v", err)
+	}
+	if len(kt.Entries) != 1 {
+		t.Fatalf("expected 1 entry after skipping the hole, got %d", len(kt.Entries))
+	}
+	if string(kt.Entries[0].Realm.Data) != "TESTSEGMENT.local" {
+		t.Fatalf("unexpected realm after hole: %q", kt.Entries[0].Realm.Data)
+	}
+}
+
+// Test_Keytab_EndOfFileMarker ensures a zero record length terminates parsing.
+func Test_Keytab_EndOfFileMarker(t *testing.T) {
+	header := []byte{0x05, 0x02}
+	entry := validEntryBytes(t)
+	data := append(append(append([]byte{}, header...), entry...), 0x00, 0x00, 0x00, 0x00)
+	// Trailing garbage after the EOF marker must be ignored.
+	data = append(data, 0xde, 0xad, 0xbe, 0xef)
+
+	kt := Keytab{}
+	if err := kt.FromBytes(data); err != nil {
+		t.Fatalf("FromBytes failed: %v", err)
+	}
+	if len(kt.Entries) != 1 {
+		t.Fatalf("expected 1 entry before EOF marker, got %d", len(kt.Entries))
+	}
+}