Merge pull request #27 from mandiant/fix-hive-resident-bounds

registry: harden hive parser against malformed inputs

pkg/registry/hive.go

@@ -252,7 +252,13 @@ func (h *Hive) GetValueData(vk *VKRecord) ([]byte, error) {
 	isResident := vk.DataLen&0x80000000 != 0
 
 	if isResident {
-		// Data is stored in the DataOffset field itself (up to 4 bytes)
+		// Data is stored in the DataOffset field itself (up to 4 bytes).
+		// dataLen is attacker-controlled (lower 31 bits of vk.DataLen, read
+		// verbatim from hive bytes); reject lengths beyond the 4-byte cap
+		// before slicing to prevent a panic on malformed hives.
+		if dataLen > 4 {
+			return nil, fmt.Errorf("resident data length %d exceeds maximum of 4 bytes", dataLen)
+		}
 		data := make([]byte, 4)
 		binary.LittleEndian.PutUint32(data, vk.DataOffset)
 		return data[:dataLen], nil
@@ -376,11 +382,17 @@ func (h *Hive) enumSubKeys(nk *NKRecord) ([]subKeyInfo, error) {
 			}
 			subListOffset := int32(binary.LittleEndian.Uint32(cell[entryOff : entryOff+4]))
 
-			// Recursively process sub-list
+			// Recursively process sub-list. readCell can return a slice
+			// shorter than the 4-byte cell header (e.g. minimum-size cell with
+			// no usable bytes), so guard against an indexing panic before
+			// reading the sub-list signature and count.
 			subCell, err := h.readCell(subListOffset)
 			if err != nil {
 				continue
 			}
+			if len(subCell) < 4 {
+				continue
+			}
 
 			subSig := binary.LittleEndian.Uint16(subCell[0:2])
 			subCount := binary.LittleEndian.Uint16(subCell[2:4])
@@ -636,16 +648,32 @@ func (h *Hive) SetValueData(keyOffset int32, valueName string, newData []byte) e
 		if isResident {
 			// Data stored inline in the VK record's DataOffset field
 			// VK cell starts at cellOffset(vkOffset)+4 (skip cell size)
-			// DataOffset is at bytes 8..12 of the VK record (after sig:2 + nameLen:2 + dataLen:4)
+			// DataOffset is at bytes 8..12 of the VK record (after sig:2 + nameLen:2 + dataLen:4).
+			// Same 4-byte cap as the read path in GetValueData; without this
+			// guard a length-matching newData buffer of 5+ bytes would silently
+			// scribble into the adjacent hive cell.
+			if dataLen > 4 {
+				return fmt.Errorf("resident data length %d exceeds maximum of 4 bytes", dataLen)
+			}
 			vkPos := h.cellOffset(vkOffset) + 4 // skip cell size dword
 			dataFieldPos := vkPos + 8           // offset of DataOffset field
 			copy(h.data[dataFieldPos:dataFieldPos+int(dataLen)], newData)
 			return nil
 		}
 
-		// Data is in a separate cell; overwrite in place
-		dataPos := h.cellOffset(int32(vk.DataOffset)) + 4 // skip cell size
-		copy(h.data[dataPos:dataPos+int(dataLen)], newData)
+		// Data is in a separate cell; overwrite in place. vk.DataOffset is
+		// attacker-controlled in malformed hives, so route through readCell
+		// (which validates the cell header and bounds) and verify dataLen
+		// fits before mutating. Without this guard a hostile offset can
+		// scribble over arbitrary hive bytes including the regf header.
+		cell, err := h.readCell(int32(vk.DataOffset))
+		if err != nil {
+			return err
+		}
+		if int(dataLen) > len(cell) {
+			return fmt.Errorf("value data length %d exceeds cell size %d", dataLen, len(cell))
+		}
+		copy(cell[:dataLen], newData)
 		return nil
 	}
 

pkg/registry/hive_test.go

@@ -86,3 +86,78 @@ func TestReadCellReturnsDataForValidCell(t *testing.T) {
 		t.Fatalf("readCell returned %q, want %q", string(data), "hello, world")
 	}
 }
+
+// TestGetValueDataResidentLengthRejected covers the input that originally
+// panicked in GetValueData: a VK with the resident flag set and a lower-31
+// length greater than the 4-byte DataOffset field. Found by kajaaz via Zorya
+// (issue #25). The guard must return an error, not crash.
+func TestGetValueDataResidentLengthRejected(t *testing.T) {
+	// h.data is unused on the resident path; the guard short-circuits before
+	// any cell read happens.
+	h := &Hive{}
+
+	for _, dataLen := range []uint32{5, 8, 0xFF, 0x7FFFFFFF} {
+		t.Run("dataLen="+strings.ToUpper(uintHex(dataLen)), func(t *testing.T) {
+			vk := &VKRecord{
+				DataLen:    dataLen | 0x80000000, // set resident flag
+				DataOffset: 0xDEADBEEF,
+			}
+
+			defer func() {
+				if r := recover(); r != nil {
+					t.Fatalf("GetValueData panicked: %v", r)
+				}
+			}()
+			data, err := h.GetValueData(vk)
+			if err == nil {
+				t.Fatalf("GetValueData returned nil error; got data=%v", data)
+			}
+			if !strings.Contains(err.Error(), "exceeds maximum of 4 bytes") {
+				t.Fatalf("GetValueData error %q did not contain expected substring", err.Error())
+			}
+		})
+	}
+}
+
+// TestGetValueDataResidentLengthAllowed confirms the happy path still works:
+// a resident value of 1..4 bytes returns the inline DataOffset bytes.
+func TestGetValueDataResidentLengthAllowed(t *testing.T) {
+	h := &Hive{}
+	// DataOffset 0x44434241 = bytes 'A','B','C','D' little-endian.
+	for dataLen, want := range map[uint32]string{
+		1: "A",
+		2: "AB",
+		3: "ABC",
+		4: "ABCD",
+	} {
+		t.Run("dataLen="+uintHex(dataLen), func(t *testing.T) {
+			vk := &VKRecord{
+				DataLen:    dataLen | 0x80000000,
+				DataOffset: 0x44434241,
+			}
+			data, err := h.GetValueData(vk)
+			if err != nil {
+				t.Fatalf("GetValueData error: %v", err)
+			}
+			if string(data) != want {
+				t.Fatalf("GetValueData returned %q, want %q", string(data), want)
+			}
+		})
+	}
+}
+
+// uintHex is a tiny helper for table-driven test names.
+func uintHex(v uint32) string {
+	const hex = "0123456789abcdef"
+	if v == 0 {
+		return "0"
+	}
+	var buf [10]byte
+	i := len(buf)
+	for v > 0 {
+		i--
+		buf[i] = hex[v&0xF]
+		v >>= 4
+	}
+	return string(buf[i:])
+}