[llvm-readobj] - Validate the DT_STRSZ value to avoid crash.

[grimar]

Summary

It is possible to trigger a crash when a dynamic symbol has a
broken (too large) st_name and the DT_STRSZ is also broken.

We have the following code in the Elf_Sym_Impl<ELFT>::getName:

template <class ELFT>
Expected<StringRef> Elf_Sym_Impl<ELFT>::getName(StringRef StrTab) const {
  uint32_t Offset = this->st_name;
  if (Offset >= StrTab.size())
    return createStringError(object_error::parse_failed,
                             "st_name (0x%" PRIx32
                             ") is past the end of the string table"
                             " of size 0x%zx",
                             Offset, StrTab.size());
...

The problem is that StrTab here is a ELFDumper::DynamicStringTab member
which is not validated properly on initialization. So it is possible to bypass the
if even when the st_name is huge.

This patch fixes the issue.

[MaskRay]

Thanks!

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test

485–486

This comment implies you should have a check showing we don't dump the next symbol, but I don't see such a check.

I'd expect to see some sort of -NOT line after the error to show that we don't dump the next symbol.

[grimar]

llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test

485–486

I check the "error:..." line. Which currently means the application terminates.
If we had a warning, I'd use something like -NOT.

Does it makes sense?

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test

485–486

It makes sense, but I therefore don't think we need this symbol currently.

[grimar]

llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test

485–486

We could have a logic that, for example, collects a vector with an information about symbols and then prints this information.
(We have something like this in a few places already. For example, in getVersionDependencies: https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-readobj/ELFDumper.cpp#L634).

So technically, a error can be reported either before the symbols/after them (if we had such vector) or in the middle (with the current logic).
Here I document we report it "in the middle". Isn't it useful?

[jhenderson]

LGTM.

llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test

485–486

Okay, thanks for the explanation.

llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test

 Sections:
   - Name:    .dynamic
     Type:    SHT_DYNAMIC
     Entries:
       - Tag:   DT_SYMENT
         Value: 0x123
       - Tag:   DT_NULL
         Value: 0
+
+## Check we report a warning when the DT_STRSZ value is broken so that the dynamic string
+## table goes past the end of the file. Document we stop dumping symbols and report an error.
+
+# RUN: yaml2obj %s --docnum=13 -o %t14
+# RUN: not llvm-readobj --dyn-symbols %t14 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t14 --check-prefix=DYNSTR-INVALID-LLVM
+# RUN: not llvm-readelf --dyn-symbols %t14 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t14 --check-prefix=DYNSTR-INVALID-GNU
+
+# DYNSTR-INVALID-LLVM: warning: '[[FILE]]': the dynamic string table at 0x78 goes past the end of the file (0x2a8) with DT_STRSZ = 0xffffffff
+# DYNSTR-INVALID-LLVM:      DynamicSymbols [
+# DYNSTR-INVALID-LLVM-NEXT:   Symbol {
+# DYNSTR-INVALID-LLVM-NEXT:     Name:  (0)
+# DYNSTR-INVALID-LLVM-NEXT:     Value: 0x0
+# DYNSTR-INVALID-LLVM-NEXT:     Size: 0
+# DYNSTR-INVALID-LLVM-NEXT:     Binding: Local (0x0)
+# DYNSTR-INVALID-LLVM-NEXT:     Type: None (0x0)
+# DYNSTR-INVALID-LLVM-NEXT:     Other: 0
+# DYNSTR-INVALID-LLVM-NEXT:     Section: Undefined (0x0)
+# DYNSTR-INVALID-LLVM-NEXT:   }
+# DYNSTR-INVALID-LLVM-NEXT: error: '[[FILE]]': st_name (0xffffff00) is past the end of the string table of size 0x6
+
+# DYNSTR-INVALID-GNU: warning: '[[FILE]]': the dynamic string table at 0x78 goes past the end of the file (0x2a8) with DT_STRSZ = 0xffffffff
+# DYNSTR-INVALID-GNU:      Symbol table '.dynsym' contains 3 entries:
+# DYNSTR-INVALID-GNU-NEXT:    Num:    Value          Size Type    Bind   Vis       Ndx Name
+# DYNSTR-INVALID-GNU-NEXT:      0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT   UND
+# DYNSTR-INVALID-GNU-NEXT: error: '[[FILE]]': st_name (0xffffff00) is past the end of the string table of size 0x6
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name:    .dynstr
+    Type:    SHT_STRTAB
+    Flags:   [ SHF_ALLOC ]
+    Content: '007465737400' ## '\0', 'test', '\0'
+  - Name:  .dynamic
+    Type:  SHT_DYNAMIC
+    Flags: [ SHF_ALLOC ]
+    Link:  .dynstr
+    Entries:
+      - Tag:   DT_STRTAB
+        Value: 0x0
+      - Tag:   DT_STRSZ
+        Value: 0xffffffff
+      - Tag:   DT_NULL
+        Value: 0x0
+DynamicSymbols:
+  - StName:  0xffffff00
+## An arbitrary valid symbol to document we report an error before dumping it.
+  - StName:  0x1
+ProgramHeaders:
+  - Type:  PT_LOAD
+    Flags: [ PF_R ]
+    Sections:
+      - Section: .dynstr
+      - Section: .dynamic

llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test

 # BEFORE-THE-EOF-NEXT: {{[(]?}}RUNPATH{{[)]?}}   Library runpath: [oabc]
 # BEFORE-THE-EOF-NEXT: {{[(]?}}NULL{{[)]?}}      0x0
 
 ## Case B: the value of DT_STRSZ tag is set so that the string table goes 1 byte past the EOF.
 # RUN: yaml2obj %s -DSTRSZ=0x211 --docnum=6 -o %t9.2
 # RUN: llvm-readobj --dynamic-table %t9.2 2>&1 | FileCheck %s -DFILE=%t9.2 --check-prefix=PAST-THE-EOF
 # RUN: llvm-readelf --dynamic-table %t9.2 2>&1 | FileCheck %s -DFILE=%t9.2 --check-prefix=PAST-THE-EOF
 
-# PAST-THE-EOF:      warning: '[[FILE]]': string table at offset 0xb0 with size 0x211 goes past the end of the file (0x2c0)
+# PAST-THE-EOF:      warning: '[[FILE]]': the dynamic string table at 0xb0 goes past the end of the file (0x2c0) with DT_STRSZ = 0x211
+# PAST-THE-EOF:      warning: '[[FILE]]': string table was not found
 # PAST-THE-EOF:      {{[(]?}}NEEDED{{[)]?}}    Shared library: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}FILTER{{[)]?}}    Filter library: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}AUXILIARY{{[)]?}} Auxiliary library: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}USED{{[)]?}}      Not needed object: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}SONAME{{[)]?}}    Library soname: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}RPATH{{[)]?}}     Library rpath: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}RUNPATH{{[)]?}}   Library runpath: [<?>]
 # PAST-THE-EOF-NEXT: {{[(]?}}NULL{{[)]?}}      0x0

llvm/tools/llvm-readobj/ELFDumper.cpp

case ELF::DT_JMPREL:
       DynPLTRelRegion.Addr = toMappedAddr(Dyn.getTag(), Dyn.getPtr());
       break;
     case ELF::DT_PLTRELSZ:
       DynPLTRelRegion.Size = Dyn.getVal();
       DynPLTRelRegion.SizePrintName = "DT_PLTRELSZ value";
       break;
     }
   }
-  if (StringTableBegin)
+
+  if (StringTableBegin) {
+    const uint64_t FileSize = ObjF->getELFFile()->getBufSize();
+    const uint64_t Offset =
+        (const uint8_t *)StringTableBegin - ObjF->getELFFile()->base();
+    if (StringTableSize > FileSize - Offset)
+      reportUniqueWarning(createError(
+          "the dynamic string table at 0x" + Twine::utohexstr(Offset) +
+          " goes past the end of the file (0x" + Twine::utohexstr(FileSize) +
+          ") with DT_STRSZ = 0x" + Twine::utohexstr(StringTableSize)));
+    else
       DynamicStringTable = StringRef(StringTableBegin, StringTableSize);
+  }
+
   SOName = getDynamicString(SONameOffset);
 
   if (DynSymRegion) {
     // Often we find the information about the dynamic symbol table
     // location in the SHT_DYNSYM section header. However, the value in
     // DT_SYMTAB has priority, because it is used by dynamic loaders to
     // locate .dynsym at runtime. The location we find in the section header
     // and the location we find here should match.