[WASM] Fix overflow when reading custom section

[JDevlieghere]

Summary

When reading a custom WASM section, it was possible that its name
extended beyond the size of the section. This resulted in a bogus value
for the section size due to the size overflowing.

Fixes heap buffer overflow detected by OSS-fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=8190

[JDevlieghere]

llvm/lib/Object/WasmObjectFile.cpp

222 ↗

(On Diff #159506)

Can this section be empty? (i.e. should I make this greater or equal)

[sbc100]

llvm/lib/Object/WasmObjectFile.cpp

222 ↗

(On Diff #159506)

I think the ReadContext is probably a better way to enforce this. readString already does this check based on the context.

This check seems a little strange since it reads the string before checking if its too long.

[JDevlieghere]

llvm/lib/Object/WasmObjectFile.cpp

222 ↗

(On Diff #159506)

I started out by doing that that but then you still have to update the current pointer and the size which made the code less intuitive. That combined with a less readable error message made me go this route. I'm happy to change it if you still think the ReadContext is better given the context. (pun not intended :-)

[sbc100]

llvm/lib/Object/WasmObjectFile.cpp

222 ↗

(On Diff #159506)

I don't particularly care about the precision and readability of the errors we generate on malformed input. Having a specific error message for this one strange case actually seems like overkill. However, if this is the most readable solution I think I'd be ok with it.

What did you earlier attempt look like. I would suggest creating an entirely new Context on the stack just for reading this Name. Is that less readable?

I'm happy to change the error massage to say "out of bounds" or something like that rather than "EOF" too.

[JDevlieghere]

Updated to use the ReadContext. If the error message doesn't matter that much then I agree this is better.

[sbc100]

I agree this feels a bit awkward, but I prefer to custom bounds checking I think.

[aheejin]

Hello. How did you generate this malformed test case? I added a new section on wasm binary format and now this case is failing for another reason before it reaches the custom section. I'd like to update the test case but don't know how this was generated.

[sbc100]

In D50387#1286459, @aheejin wrote:

Hello. How did you generate this malformed test case? I added a new section on wasm binary format and now this case is failing for another reason before it reaches the custom section. I'd like to update the test case but don't know how this was generated.

How is your new section causing this binary to fail to load? The new section is not mandatory, surly?

llvm/trunk/lib/Object/WasmObjectFile.cpp

static Error readSection(WasmSection &Section,
   uint32_t Size = readVaruint32(Ctx);
   if (Size == 0)
     return make_error<StringError>("Zero length section",
                                    object_error::parse_failed);
   if (Ctx.Ptr + Size > Ctx.End)
     return make_error<StringError>("Section too large",
                                    object_error::parse_failed);
   if (Section.Type == wasm::WASM_SEC_CUSTOM) {
-    const uint8_t *NameStart = Ctx.Ptr;
-    Section.Name = readString(Ctx);
-    Size -= Ctx.Ptr - NameStart;
+    WasmObjectFile::ReadContext SectionCtx;
+    SectionCtx.Start = Ctx.Ptr;
+    SectionCtx.Ptr = Ctx.Ptr;
+    SectionCtx.End = Ctx.Ptr + Size;
+
+    Section.Name = readString(SectionCtx);
+
+    uint32_t SectionNameSize = SectionCtx.Ptr - SectionCtx.Start;
+    Ctx.Ptr += SectionNameSize;
+    Size -= SectionNameSize;
   }
   Section.Content = ArrayRef<uint8_t>(Ctx.Ptr, Size);
   Ctx.Ptr += Size;
   return Error::success();
 }
 
 WasmObjectFile::WasmObjectFile(MemoryBufferRef Buffer, Error &Err)
     : ObjectFile(Binary::ID_Wasm, Buffer) {

llvm/trunk/test/Object/wasm-string-outside-section.test

+RUN: not llvm-objdump -s %p/Inputs/WASM/string-outside-section.wasm 2>&1 | FileCheck %s
+
+CHECK: LLVM ERROR: EOF while reading string