[WebAssembly] Limit increase of Ctx.End

[DavidKorczynski]

Extending Ctx.End beyond the original buffer leads to buffer overflows. This limits extending Ctx.End beyond OrigEnd to prevent these overflows.

Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=65432

[llvmbot]

@llvm/pr-subscribers-llvm-binary-utilities

@llvm/pr-subscribers-backend-webassembly

Author: None (DavidKorczynski)

Changes

Extending Ctx.End beyond the original buffer leads to buffer overflows. This limits extending Ctx.End beyond OrigEnd to prevent these overflows.

Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=65432

---

Full diff: https://github.com/llvm/llvm-project/pull/76676.diff

1 Files Affected:

• (modified) llvm/lib/Object/WasmObjectFile.cpp (+3)

diff --git a/llvm/lib/Object/WasmObjectFile.cpp b/llvm/lib/Object/WasmObjectFile.cpp
index 40665d686cf939..6f89e183118d63 100644
--- a/llvm/lib/Object/WasmObjectFile.cpp
+++ b/llvm/lib/Object/WasmObjectFile.cpp
@@ -546,6 +546,9 @@ Error WasmObjectFile::parseLinkingSection(ReadContext &Ctx) {
     uint32_t Size = readVaruint32(Ctx);
     LLVM_DEBUG(dbgs() << "readSubsection type=" << int(Type) << " size=" << Size
                       << "\n");
+    if ((const uint8_t *)(Ctx.Ptr + Size) > OrigEnd)
+      return make_error<GenericBinaryError>("invalid segment size",
+                                            object_error::parse_failed);
     Ctx.End = Ctx.Ptr + Size;
     switch (Type) {
     case wasm::WASM_SYMBOL_TABLE:

[dwblaikie]

Test coverage?

[DavidKorczynski]

Test coverage?

Added a regression test -- this approach of embedding the fuzz data in the test works well for small cases but may become tricky when the fuzz data gets large (tens of KBs)

[dschuff]

This looks good to me in general.
I think I like the idea of using the unittest framework for testing parsing of possibly malformed binary files (as opposed to the alternative of checking in binary files somewhere like llvm/test/Object). @sbc100 does this seem reasonable to you?

[dwblaikie]

Perhaps use sizeof(data) to avoid the magic number here?

[dwblaikie]

Should we expect a particular error here, perhaps we can check for the details in the error to ensure we're diagnosing this correctly?

[DavidKorczynski]

I'm not sure tbh -- the test is a copy of the parts of the fuzzer that is needed to trigger the overflow: https://github.com/llvm/llvm-project/blob/main/llvm/tools/llvm-dwarfdump/fuzzer/llvm-dwarfdump-fuzzer.cpp

[sbc100]

This looks good to me in general. I think I like the idea of using the unittest framework for testing parsing of possibly malformed binary files (as opposed to the alternative of checking in binary files somewhere like llvm/test/Object). @sbc100 does this seem reasonable to you?

I'm not sure which is best. Our existing tests for this kind of thing currently live in llvm/test/Object/. e.g.

llvm-project/llvm/test/Object/wasm-bad-symbol-type.test

Line 1 in f9aba82

RUN: not llvm-objdump -s %p/Inputs/WASM/bad-symbol-type.wasm 2>&1 | FileCheck %s

.

One nice think about checking in the binaries is that its easy to read and modify them. For example, I could take llvm/test/Object/Inputs/WASM/bad-symbol-type.wasm and run it though objdump. This is not true for this hex string embedded in the unittest sources.

[sbc100]

I think its probably best to add this new test alongside the existing llvm/test/Object/Inputs/WASM/bad-* tests, if only for consistency.

[DavidKorczynski]

Am not sure what the status is on this one in terms of testing: are we keeping the unit test or? if not, I can add the reproducer testcase linked to in https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=65432 to the directory mentioned by @sbc100 -- however, I suspect many fuzzers won't have a given binary in a similar way to just add the fuzzer files? Does it perhaps make more sense to create a folder next to the fuzzer with the testcases that has produced errors in the past? This would at least be more general from a fuzzing perspective.

[sbc100]

Am not sure what the status is on this one in terms of testing: are we keeping the unit test or? if not, I can add the reproducer testcase linked to in https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=65432 to the directory mentioned by @sbc100 -- however, I suspect many fuzzers won't have a given binary in a similar way to just add the fuzzer files? Does it perhaps make more sense to create a folder next to the fuzzer with the testcases that has produced errors in the past? This would at least be more general from a fuzzing perspective.

I like the test, but I'd prefer binaries to be checked in as separate files.

I'm not a fan a direct fuzzer outputs being checked in unless we can't find a way to reduce to something small/understandable (such as the one in this case).

[DavidKorczynski]

I'm not a fan a direct fuzzer outputs being checked in unless we can't find a way to reduce to something small/understandable (such as the one in this case).

OSS-Fuzz minimizes the input and the input added here is the fuzzer input as minimized by OSS-Fuzz. Similarly the minimized input produced by OSS-Fuzz in #77708 is 25 bytes and the one in #77698 is 132 bytes

[sbc100]

I'm not a fan a direct fuzzer outputs being checked in unless we can't find a way to reduce to something small/understandable (such as the one in this case).

OSS-Fuzz minimizes the input and the input added here is the fuzzer input as minimized by OSS-Fuzz. Similarly the minimized input produced by OSS-Fuzz in #77708 is 25 bytes and the one in #77698 is 132 bytes

Those both sounds fine to checkin then.

In general, If possible, I'd think still prefer to have them named something meaningful (and possibly reduced even more) once the root causes is established.

[DavidKorczynski]

I added the trigger file and adjusted the sizeof in the unit test.

I likely won't have cycles to reduce it even further or similar just because I know little of the LLVM code -- I'm coming from the fuzz world and know my way around e.g. https://github.com/google/oss-fuzz , but am not well-versed in the details of LLVMs codebase.

Am happy to monitor the LLVM's OSS-Fuzz set up to ensure it's running proper and submit fixes for the issues (which happens in various parts of the codebase) I can handle (i.e. with the limited knowledge of the codebase), but I don't have cycles to go much further than that unfortunately.

[dschuff]

Not that it necessarily needs to block this PR, but:
I've been working in this file lately and thought about this, and it occurred to me that this change is really very limited in that it only protects against one corrupted field in one section. But every section has a header, some have subsections like the linking section, and most sections have vectors with size fields, and this same kind of problem could happen in any of those cases. If we really care about defending against buffer overflows caused by corrupted or malicious input files, we should do something more comprehensive, e.g. change how Ctx.Ptr is updated to centralize this checking. And this is just the object file parser... do LLVM's other parsers have similar goals or threat models? What is our threat model that would tell us how far we should take this work?

[DavidKorczynski]

To a large extent I'm coming from the angle of an issue was reported by OSS-Fuzz so I added a fix.

I assume once this bug is fixed the fuzzer will quickly report others. I'm not sure about the threat model, but for what it's worth the majority of issues I previously fixed from this fuzzer occurred in WasmObjectFile.cpp as opposed to the other files in llvm/lib/Object/. However, for some perspective, the fuzzer that found it was added to LLVMs codebase roughly 9 years ago https://github.com/llvm/llvm-project/blob/main/llvm/tools/llvm-dwarfdump/fuzzer/llvm-dwarfdump-fuzzer.cpp The issues reported by this fuzzer are here: https://bugs.chromium.org/p/oss-fuzz/issues/list?q=llvm-dwarfdump-fuzzer&can=1&sort=-reported and are public (the OSS-Fuzz integration was configured to be public).