[PATCH] D37439: [MachO] Prevent heap overflow when load command extends past EOF

[Kevin Enderby via llvm-commits]

If you look at the series of checks I don’t think it is not possible that the cmdsize is so large that the addition wraps around even in 32-bit.  As I think Obj.getData().end() returns a 64-bit value so the comparison should be done in 64-bit.

Also if you start by following the checks in MachOObjectFile::MachOObjectFile().  Where SizeOfHeaders is a uint64_t and is checked against the size of the file. Then the series of checks in getFirstLoadCommandInfo() or getNextLoadCommandInfo() and the types use in the check like this:

  if (L.Ptr + L.C.cmdsize + sizeof(MachO::load_command) >
      Obj.getData().data() + HeaderSize + Obj.getHeader().sizeofcmds)

which should get promoted to be a uint64_t type for the check before the call to getLoadCommandInfo() is made.  Most of the checks in the Mach-O are built on a series of checks.  So looking at one if statement in a static routine is likely not to tell the whole story on how the checking is done.

These checks were ported and added from Apple’s otool(1) which over the decades has been hardened from such 32-bit additions problems.  Not saying that they are all fixed in libObject but it should be much better that it was when it had no checks only a few years ago.

My thoughts,
Kev

> On Sep 12, 2017, at 8:46 AM, Adrian Prantl via Phabricator <reviews at reviews.llvm.org> wrote:
> 
> aprantl added inline comments.
> 
> 
> ================
> Comment at: lib/Object/MachOObjectFile.cpp:186
>   if (auto CmdOrErr = getStructOrErr<MachO::load_command>(Obj, Ptr)) {
> +    if (CmdOrErr->cmdsize + Ptr > Obj.getData().end())
> +      return malformedError("load command " + Twine(LoadCommandIndex) +
> ----------------
> What happens on a 32-bit platform when cmdsize is so large that the addition wraps around? Or is cmdsize < 32bit ?
> 
> 
> Repository:
>  rL LLVM
> 
> https://reviews.llvm.org/D37439
> 
> 
>