fix: COFF symbol table readASCIIStringAtOffset out of bounds exception

helper.go

@@ -297,14 +297,13 @@ func (pe *File) readUnicodeStringAtRVA(rva uint32, maxLength uint32) string {
 }
 
 func (pe *File) readASCIIStringAtOffset(offset, maxLength uint32) (uint32, string) {
-	var i uint32
 	str := ""
-	buff := pe.data[offset : offset+maxLength]
+	i := uint32(0)
 	for i = 0; i < maxLength; i++ {
-		if buff[i] == 0 {
+		if i > pe.size || pe.data[offset+i] == 0 {
 			break
 		}
-		str += string(buff[i])
+		str += string(pe.data[offset+i])
 	}
 	return i, str
 }

symbol.go

@@ -19,6 +19,10 @@ const (
 	// Example:  0000e876c5b712b6b7b3ce97f757ddd918fb3dbdc5a3938e850716fbd841309f
 	MaxDefaultCOFFSymbolsCount = 0x10000
 
+	// MaxCOFFSymStrLength represents the maximum string length of a COFF symbol
+	// to read.
+	MaxCOFFSymStrLength = 0x50
+
 	//
 	// Type Representation
 	//
@@ -238,12 +242,13 @@ type COFFSymbol struct {
 	Value uint32
 
 	// The signed integer that identifies the section, using a one-based index
-	// into the section table. Some values have special meaning, as defined in section 5.4.2, "Section Number Values."
+	// into the section table. Some values have special meaning.
+	// See "Section Number Values."
 	SectionNumber int16
 
 	// A number that represents type. Microsoft tools set this field to
-	// 0x20 (function) or 0x0 (not a function).
-	// For more information, see Type Representation.
+	// 0x20 (function) or 0x0 (not a function). For more information,
+	// see Type Representation.
 	Type uint16
 
 	// An enumerated value that represents storage class.
@@ -284,10 +289,15 @@ func (pe *File) ParseCOFFSymbolTable() error {
 		return errCOFFSymbolsTooHigh
 	}
 
-	size := uint32(binary.Size(COFFSymbol{}))
+	// The location of the symbol table is indicated in the COFF header.
 	offset := pe.NtHeader.FileHeader.PointerToSymbolTable
+
+	// The symbol table is an array of records, each 18 bytes long.
+	size := uint32(binary.Size(COFFSymbol{}))
 	symbols := make([]COFFSymbol, symCount)
 
+	// Each record is either a standard or auxiliary symbol-table record.
+	// A standard record defines a symbol or name and has the COFFSymbol STRUCT format.
 	for i := uint32(0); i < symCount; i++ {
 		err := pe.structUnpack(&symbols[i], offset, size)
 		if err != nil {
@@ -329,12 +339,12 @@ func (pe *File) COFFStringTable() error {
 	// of a symbol.
 	size := uint32(binary.Size(COFFSymbol{}))
 	offset := pointerToSymbolTable + (size * symCount)
-	pe.COFF.StringTableOffset = offset
 
 	// At the beginning of the COFF string table are 4 bytes that contain the
 	// total size (in bytes) of the rest of the string table. This size
 	// includes the size field itself, so that the value in this location would
 	// be 4 if no strings were present.
+	pe.COFF.StringTableOffset = offset
 	strTableSize, err := pe.ReadUint32(offset)
 	if err != nil {
 		return err
@@ -347,11 +357,11 @@ func (pe *File) COFFStringTable() error {
 	// Following the size are null-terminated strings that are pointed to by
 	// symbols in the COFF symbol table. We create a map to map offset to
 	// string.
-	end := offset + strTableSize
-	for offset <= end {
-		len, str := pe.readASCIIStringAtOffset(offset, 0x30)
+	end := offset + strTableSize - 4
+	for offset < end {
+		len, str := pe.readASCIIStringAtOffset(offset, MaxCOFFSymStrLength)
 		if len == 0 {
-			break
+			continue
 		}
 		m[offset] = str
 		offset += len + 1
@@ -362,13 +372,14 @@ func (pe *File) COFFStringTable() error {
 	return nil
 }
 
-// String returns represenation of the symbol name.
+// String returns the representation of the symbol name.
 func (symbol *COFFSymbol) String(pe *File) (string, error) {
-	// contain the name itself, if it is not more than 8 bytes long, or the
-	// ShortName field gives an offset into the string table. To determine
-	// whether the name itself or an offset is given, test the first 4
-	// bytes for equality to zero.
 	var short, long uint32
+
+	// The ShortName field in a symbol table consists of 8 bytes
+	// that contain the name itself, if it is not more than 8
+	// bytes long, or the ShortName field gives an offset into
+	// the string table.
 	highDw := bytes.NewBuffer(symbol.Name[4:])
 	lowDw := bytes.NewBuffer(symbol.Name[:4])
 	errl := binary.Read(lowDw, binary.LittleEndian, &short)
@@ -377,7 +388,8 @@ func (symbol *COFFSymbol) String(pe *File) (string, error) {
 		return "", errCOFFSymbolOutOfBounds
 	}
 
-	// if 0, use LongName.
+	// To determine whether the name itself or an offset is given,
+	// test the first 4 bytes for equality to zero.
 	if short != 0 {
 		name := strings.Replace(string(symbol.Name[:]), "\x00", "", -1)
 		return name, nil

symbol_test.go

@@ -9,6 +9,7 @@ import "testing"
 type TestCOFFSymbol struct {
 	errTooManySymbols error
 	symbolsCount      int
+	symbolIdx         int
 	symbol            COFFSymbol
 	stringTableOffset uint32
 	symbolName        string
@@ -24,7 +25,8 @@ var symbolTests = []struct {
 		getAbsoluteFilePath("test/liblzo2-2"),
 		TestCOFFSymbol{
 			errTooManySymbols: nil,
-			symbolsCount: 50,
+			symbolsCount:      50,
+			symbolIdx:         0,
 			symbol: COFFSymbol{
 				Name:               [8]byte{0, 0, 0, 0, 4, 0, 0, 0},
 				Value:              0x2ac,
@@ -40,6 +42,27 @@ var symbolTests = []struct {
 		},
 	},
 
+	{
+		getAbsoluteFilePath("test/0103daa751660333b7ae5f098795df58f07e3031563e042d2eb415bffa71fe7a"),
+		TestCOFFSymbol{
+			errTooManySymbols: nil,
+			symbolsCount:      346,
+			symbolIdx:         3,
+			symbol: COFFSymbol{
+				Name:               [8]byte{0, 0, 0, 0, 4, 0, 0, 0},
+				Value:              0x2ac,
+				SectionNumber:      8,
+				Type:               0x0,
+				StorageClass:       0x2,
+				NumberOfAuxSymbols: 0x0,
+			},
+			stringTableOffset: 0x1b054,
+			symbolName:        "___mingw_CRTStartup",
+			sectionNumberName: ".text",
+			symbolTypeString:  "",
+		},
+	},
+
 	{
 		getAbsoluteFilePath("test/0000e876c5b712b6b7b3ce97f757ddd918fb3dbdc5a3938e850716fbd841309f"),
 		TestCOFFSymbol{
@@ -72,32 +95,32 @@ func TestParseCOFFSymbolTable(t *testing.T) {
 			}
 
 			if len(file.COFF.SymbolTable) != tt.out.symbolsCount {
-				t.Errorf("TestParseCOFFSymbolTable(%s) failed, want: %d, got: %d", tt.in, tt.out.symbolsCount, len(file.COFF.SymbolTable))
+				t.Errorf("symbolsCount assertion failed, want: %d, got: %d", tt.out.symbolsCount, len(file.COFF.SymbolTable))
 			}
 			if file.COFF.StringTableOffset != tt.out.stringTableOffset {
-				t.Errorf("TestParseCOFFSymbolTable(%s) failed, want: %d, got: %d", tt.in, tt.out.stringTableOffset, file.COFF.StringTableOffset)
+				t.Errorf("stringTableOffset assertion failed, want: %d, got: %d", tt.out.stringTableOffset, file.COFF.StringTableOffset)
 			}
 			if !stringInSlice(tt.out.symbolName, file.COFF.StringTable) {
-				t.Errorf("TestParseCOFFSymbolTable(%s) failed, want: %s, got: %v", tt.in, tt.out.symbolName, file.COFF.StringTable)
+				t.Errorf("symbolName assertion failed, want: %s, got: %v", tt.out.symbolName, file.COFF.StringTable)
 			}
 
-			coffSymbol := file.COFF.SymbolTable[0]
+			coffSymbol := file.COFF.SymbolTable[tt.out.symbolIdx]
 			symbolNameStr, err := coffSymbol.String(file)
 			if err != nil {
-				t.Errorf("TestParseCOFFSymbolTable(%s) failed, COFFSymbol.String() failed with: %v", tt.in, err)
+				t.Errorf("COFFSymbol.String() failed with: %v", err)
 			}
 			if symbolNameStr != tt.out.symbolName {
-				t.Errorf("TestParseCOFFSymbolTable(%s) failed, want: %d, got: %d", tt.in, tt.out.symbolsCount, len(file.COFF.SymbolTable))
+				t.Errorf("symbol name to string failed, want: %s, got: %s", tt.out.symbolName, symbolNameStr)
 			}
 
 			secNumName := coffSymbol.SectionNumberName(file)
 			if secNumName != tt.out.sectionNumberName {
-				t.Errorf("TestParseCOFFSymbolTable(%s) failed, want: %s, got: %s", tt.in, tt.out.sectionNumberName, secNumName)
+				t.Errorf("SectionNumberName assertion failed, want: %s, got: %s", tt.out.sectionNumberName, secNumName)
 			}
 
 			typeString := file.PrettyCOFFTypeRepresentation(coffSymbol.Type)
 			if typeString != tt.out.symbolTypeString {
-				t.Errorf("TestParseCOFFSymbolTable(%s) failed, want: %s, got: %s", tt.in, tt.out.symbolTypeString, typeString)
+				t.Errorf("PrettyCOFFTypeRepresentation assertion failed, want: %s, got: %s", tt.out.symbolTypeString, typeString)
 			}
 		})
 	}