A heap-based buffer overflow in function read_io_status in src/modbus.c

[GoldBinocle]

libmodbus version

latest commit b25629bfb508bdce7d519884c0fa9810b7d98d44

OS and/or distribution

Debian GNU/Linux 11 (bullseye)

Environment

x86_64

Description

There is a heap-based buffer overflow in the function read_io_status in src/modbus.c.

Actual behavior if applicable

Heap-buffer-overflow

Expected behavior or suggestion

no crash

Steps to reproduce the behavior (commands or source code)

Build with ASan

CC=clang CXX=clang++ CFLAGS="-g -fsanitize=address" CXXFLAGS="-g -fsanitize=address" ./autogen.sh
CC=clang CXX=clang++ CFLAGS="-g -fsanitize=address" CXXFLAGS="-g -fsanitize=address" ./configure && make

Asan traceback

I found a heap-buffer-overflow bug via the utility tests/unit-test-client, here is the stderr output:

ERROR Connection timed out: select
ERROR Illegal data address
=================================================================
==2758680==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x604000000035 at pc 0x7f80c1952a2a bp 0x7ffd59cbdb30 sp 0x7ffd59cbdb28
WRITE of size 1 at 0x604000000035 thread T0
    #0 0x7f80c1952a29 in read_io_status /root/libmodbus/src/modbus.c:1207:29
    #1 0x7f80c195254d in modbus_read_bits /root/libmodbus/src/modbus.c:1238:10
    #2 0x560ca1aa26b4 in main /root/libmodbus/tests/unit-test-client.c:363:10
    #3 0x7f80c15d4d09 in __libc_start_main csu/../csu/libc-start.c:308:16
    #4 0x560ca19e15c9 in _start (/root/libmodbus/tests/.libs/unit-test-client+0x255c9) (BuildId: 06a076f26a7dd417c82e6c39b9bbb21e4b70fc3e)

0x604000000035 is located 0 bytes to the right of 37-byte region [0x604000000010,0x604000000035)
allocated by thread T0 here:
    #0 0x560ca1a6616b in __interceptor_malloc (/root/libmodbus/tests/.libs/unit-test-client+0xaa16b) (BuildId: 06a076f26a7dd417c82e6c39b9bbb21e4b70fc3e)

SUMMARY: AddressSanitizer: heap-buffer-overflow /root/libmodbus/src/modbus.c:1207:29 in read_io_status
Shadow bytes around the buggy address:
  0x0c087fff7fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c087fff7fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c087fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c087fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c087fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c087fff8000: fa fa 00 00 00 00[05]fa fa fa fa fa fa fa fa fa
  0x0c087fff8010: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c087fff8020: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c087fff8030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c087fff8040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c087fff8050: 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
==2758680==ABORTING

PoC

This bug is triggered when the client executing modbus_read_bits:

libmodbus/tests/unit-test-client.c

Line 363 in b25629b

rc = modbus_read_bits(ctx, UT_BITS_ADDRESS, UT_BITS_NB + 1, tab_rp_bits);

For this operation, the corresponding normal response to tests/unit-test-client (collected by launching tests/unit-test-server) is 001100000006ff0101300026, with structure:

###[ ModbusADU ]### 
  transId   = 0x11
  protoId   = 0x0
  len       = 6
  unitId    = 0xff
###[ Read Coils Response ]### 
     funcCode  = 0x1
     byteCount = 1
     coilStatus= [48]

However, if mutating this packet by manipulating the field unitId:

###[ ModbusADU ]### 
  transId   = 0x11
  protoId   = 0x0
  len       = 6
  unitId    = 0x7c
###[ Read Coils Response ]### 
     funcCode  = 0x1
     byteCount = 1
     coilStatus= [48]

with hex stream 0011000000067c0101300023, the tests/unit-test-client crashed due to heap buffer overflow.

[GoldBinocle]

Hi, what's the status of this bug? Do you need more details from me? If so, what details should I provide? Thanks.

[carnil]

Apparently CVE-2023-26793 was assigned for this issue.

[sandeen]

I was going to take a look at this due to the CVE.
@GoldBinocle I'm not sure I completely understand your POC; are you fuzzing the response from the test server to trigger the error in the client?
@stephane probably understands all this better than I do but I'll try to help if I can.

[GoldBinocle]

I was going to take a look at this due to the CVE. @GoldBinocle I'm not sure I completely understand your POC; are you fuzzing the response from the test server to trigger the error in the client?

Yes, exactly right.

[sandeen]

I was going to take a look at this due to the CVE. @GoldBinocle I'm not sure I completely understand your POC; are you fuzzing the response from the test server to trigger the error in the client?

Yes, exactly right.

Ok, I haven't yet been able to work out how to do that, so if you have more info ...

[locka99]

Doesn't this issue just boil down to this line in the unit test?

rc = modbus_read_bits(ctx, UT_BITS_ADDRESS, UT_BITS_NB + 1, tab_rp_bits);

The buffer tab_rp_bits was been allocated to be UT_BITS_NB and the caller is lying and saying it is allocated for UT_BITS_NB + 1. So of course the buffer can overflow, because the caller lied about the length in the first place. So when the server returns a bunch of data and it is processed, the loop in the client fills more bytes than there is space. IMO this is not a bug in the implementation, rather it is a bug in the unit test and user error for not ensuring the size of the buffer matches with its actual size.