search

stephane / libmodbus

A Modbus library for Linux, Mac OS, FreeBSD and Windows
http://libmodbus.org
GNU Lesser General Public License v2.1
3.52k stars 1.77k forks source link

heap-buffer-overflow in in `modbus_write_bits` #743

Closed MarkLee131 closed 2 months ago

MarkLee131 commented 7 months ago

libmodbus version

libmodbus 3.1.10

OS and/or distribution

Linux, Ubuntu 20.04

Environment

amd64

Description

A heap-buffer-overflow was discovered in the modbus_write_bits function. This issue can be triggered when the function is fed with specially crafted input, which leads to out-of-bounds read and can potentially cause a crash or other unintended behaviors.

Actual behavior if applicable

The function modbus_write_bits exhibits undefined behavior and crashes when processing certain inputs where the nb (number of bits) parameter leads to an out-of-bounds read of the input buffer. This is evident from the ASan reports indicating a heap-buffer-overflow.

Expected behavior or suggestion

The function should safely handle all input ranges and sizes without causing buffer overflows.

Steps to reproduce the behavior (commands or source code)

#include <fuzzer/FuzzedDataProvider.h>
#include "modbus.h"
#include <cstdint>
#include <cstddef>

extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
  modbus_t* ctx = modbus_new_tcp("127.0.0.1", 1502); // port as 502 is also ok.
  if (ctx == nullptr) {
    return 0;
  }

  FuzzedDataProvider provider(data, size);
  int addr = provider.ConsumeIntegral<int>();
  int nb = provider.ConsumeIntegral<int>();
  std::vector<uint8_t> bits = provider.ConsumeBytes<uint8_t>(nb);

  int ret = modbus_write_bits(ctx, addr, nb, bits.data());

  modbus_close(ctx);
  modbus_free(ctx);

  return ret;
}

DEDUP_TOKEN: modbus_write_bits--LLVMFuzzerTestOneInput--fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) 0x60200000b571 is located 0 bytes to the right of 1-byte region [0x60200000b570,0x60200000b571) allocated by thread T0 here:

0 0x569ead in operator new(unsigned long) /src/llvm-project/compiler-rt/lib/asan/asan_new_delete.cpp:95:3

#1 0x56cb55 in __libcpp_operator_new<unsigned long> /usr/local/bin/../include/c++/v1/new:245:10
#2 0x56cb55 in __libcpp_allocate /usr/local/bin/../include/c++/v1/new:271:10
#3 0x56cb55 in allocate /usr/local/bin/../include/c++/v1/__memory/allocator.h:105:38
#4 0x56cb55 in allocate /usr/local/bin/../include/c++/v1/__memory/allocator_traits.h:262:20
#5 0x56cb55 in __vallocate /usr/local/bin/../include/c++/v1/vector:931:37
#6 0x56cb55 in vector /usr/local/bin/../include/c++/v1/vector:1062:9
#7 0x56cb55 in std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > FuzzedDataProvider::ConsumeBytes<unsigned char>(unsigned long, unsigned long) /usr/local/lib/clang/15.0.0/include/fuzzer/FuzzedDataProvider.h:361:18
#8 0x56c78c in ConsumeBytes<unsigned char> /usr/local/lib/clang/15.0.0/include/fuzzer/FuzzedDataProvider.h:110:10
#9 0x56c78c in LLVMFuzzerTestOneInput /src/libmodbus/fuzz/FuzzClient.cpp:16:40
#10 0x43ded3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:611:15
#11 0x43d6ba in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:514:3
#12 0x43ed89 in fuzzer::Fuzzer::MutateAndTestOne() /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:757:19
#13 0x43fa55 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:895:5
#14 0x42edbf in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:912:6
#15 0x458412 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
#16 0x7fe3e6c85082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId: 87b331c034a6458c64ce09c03939e947212e18ce)

DEDUP_TOKEN: operator new(unsigned long)--libcpp_operator_new--libcpp_allocate SUMMARY: AddressSanitizer: heap-buffer-overflow /src/libmodbus/src/modbus.c:1461:17 in modbus_write_bits Shadow bytes around the buggy address: 0x0c047fff9650: fa fa fd fa fa fa fd fd fa fa fd fa fa fa fd fd 0x0c047fff9660: fa fa fd fa fa fa fd fd fa fa fd fa fa fa fd fd 0x0c047fff9670: fa fa fd fa fa fa fd fa fa fa fd fa fa fa fd fa 0x0c047fff9680: fa fa fd fa fa fa fd fa fa fa fd fa fa fa fd fa 0x0c047fff9690: fa fa fd fa fa fa fd fa fa fa fd fa fa fa fd fa =>0x0c047fff96a0: fa fa fd fd fa fa fd fa fa fa 00 01 fa fa[01]fa 0x0c047fff96b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff96c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff96d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff96e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff96f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==13==ABORTING MS: 2 ChangeBit-ChangeBit-; base unit: c68bed0f37ab64033b5e4e6c6e801a00b8c8f87c 0x0,0x1,0x2,0x0,0x80,0x4,0x1,0x1,0x2a, \000\001\002\000\200\004\001\001* artifact_prefix='./'; Test unit written to ./crash-05ddbd1d0695e401044d58f3388298c1138958d4 Base64: AAECAIAEAQEq stat::number_of_executed_units: 1401 stat::average_exec_per_sec: 700 stat::new_units_added: 17 stat::slowest_unit_time_sec: 0 stat::peak_rss_mb: 32


- Sample Input Causing Crash:

./tests/unit-test-server echo "AAECAIAEAQEq" | base64 -d | nc 127.0.0.1 1502

or use the attached file for reproducing:

[crash-05ddbd1d0695e401044d58f3388298c1138958d4.txt](https://github.com/stephane/libmodbus/files/15141098/crash-05ddbd1d0695e401044d58f3388298c1138958d4.txt)

## libmodbus output with debug mode enabled

Client connection accepted from 127.0.0.1. Waiting for an indication... ERROR Connection timed out: select

<00><01><02><00><80><04><01><01><2A>Quit the loop: Connection timed out ``` - The crashed line is at: https://github.com/stephane/libmodbus/blob/5c14f13944ec7394a61a7680dcb462056c4321e3/src/modbus.c#L1461
psychon commented 4 months ago

I think this issue is invalid and only found a bug in the fuzzing driver

I don't know which fuzzer/FuzzedDataProvider.h you used. I will assume https://github.com/llvm/llvm-project/blob/7868033d2e846fa30c20455ca819fad29d9d795e/compiler-rt/include/fuzzer/FuzzedDataProvider.h

Let us look at what provider.ConsumeBytes<uint8_t>(nb); actually does: https://github.com/llvm/llvm-project/blob/7868033d2e846fa30c20455ca819fad29d9d795e/compiler-rt/include/fuzzer/FuzzedDataProvider.h#L103-L111

Notice the min in there. It might return less data than what you asked for. That means that the following line is incorrect since it tells libmodbus "you might read nb bits of data" and does not handle the case when the buffer is smaller than nb:

int ret = modbus_write_bits(ctx, addr, nb, bits.data());

bits could contain less than nb bytes of data. In which case you get the read buffer overflow that your fuzzer found.

Please fix this to

int ret = modbus_write_bits(ctx, addr, bits.size(), bits.data());

(Technically, I guess this should be bits.size() * 8 since the buffer you call bits actually contains bytes, but that issue is unrelated to whether your fuzzing driver is actually correct. It would just be less effective due to this.)

Also, for the future, I suggest that you try to understand your fuzzing results before humillating yourself.