Possible undefined behavior through out-of-bounds array access

[cktii]

Introduction

Hi,

I'm working on setting up fuzzing for the public APIs that this application has to offer to improve the robustness and security of the overall system.
While doing so, I ran into the below described issue.

• Tested commit: 698d227

Bug Summary

In ImuConverter.cpp, more specifically in the toRosMsg API there exists this line of code:

const auto imuPacket = inData->packets[inData->packets.size() - 1];

In such cases where inData->packets is a valid pointer to some dai::IMUData with packets being an empty array, calling .size() on it will return 0, which in turn leads to undefined behavior as an out-of-bounds access to inData->packets[-1] takes place.

Stack trace:

./build/depthai_ctrl/imu_conv_fuzz
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1158936129
INFO: Loaded 2 modules   (14261 inline 8-bit counters): 8879 [0x752399dd7468, 0x752399dd9717), 5382 [0x58a7fe4d91f8, 0x58a7fe4da6fe),
INFO: Loaded 2 PC tables (14261 PCs): 8879 [0x752399dd9718,0x752399dfc208), 5382 [0x58a7fe4da700,0x58a7fe4ef760),
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
INFO: A corpus is not provided, starting from an empty corpus
#2      INITED exec/s: 0 rss: 136Mb
WARNING: no interesting inputs were found so far. Is the code instrumented for coverage?
This may also happen if the target rejected all inputs we tried so far
        NEW_FUNC[1/23]: 0x752399d23590 in dai::ros::ImuConverter::ImuConverter(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) /home/user/git/work/depthai_ctrl/src/ImuConverter.cpp:10
        NEW_FUNC[2/23]: 0x752399d236c0 in dai::ros::ImuConverter::~ImuConverter() /home/user/git/work/depthai_ctrl/src/ImuConverter.cpp:11
#1332   NEW    cov: 201 ft: 202 corp: 2/18b lim: 17 exec/s: 0 rss: 138Mb L: 17/17 MS: 4 InsertByte-InsertByte-CMP-InsertRepeatedBytes- DE: "\001\000\000\000"-
#1333   NEW    cov: 204 ft: 207 corp: 3/35b lim: 17 exec/s: 0 rss: 138Mb L: 17/17 MS: 1 CopyPart-
#1337   NEW    cov: 205 ft: 208 corp: 4/52b lim: 17 exec/s: 0 rss: 138Mb L: 17/17 MS: 4 ShuffleBytes-CMP-ChangeByte-ShuffleBytes- DE: "\012\000\000\000"-
/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_vector.h:1129:34: runtime error: applying non-zero offset 18446744073709551384 to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_vector.h:1129:34
/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_vector.h:1129:9: runtime error: reference binding to address 0xffffffffffffff18 with insufficient space for an object of type 'dai::IMUPacket'
0xffffffffffffff18: note: pointer points here
<memory cannot be printed>
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_vector.h:1129:9
Stack trace (most recent call last):
#11   Object "", at 0xffffffffffffffff, in
#10   Object "/home/user/git/work/depthai_ctrl/build/depthai_ctrl/imu_conv_fuzz", at 0x58a7fe31cc34, in _start
#9    Source "../csu/libc-start.c", line 360, in __libc_start_main_impl [0x75239982a28a]
#8    Source "../sysdeps/nptl/libc_start_call_main.h", line 58, in __libc_start_call_main [0x75239982a1c9]
#7    Object "/home/user/git/work/depthai_ctrl/build/depthai_ctrl/imu_conv_fuzz", at 0x58a7fe3522d6, in main
#6    Object "/home/user/git/work/depthai_ctrl/build/depthai_ctrl/imu_conv_fuzz", at 0x58a7fe327c4f, in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long))
#5    Object "/home/user/git/work/depthai_ctrl/build/depthai_ctrl/imu_conv_fuzz", at 0x58a7fe33a975, in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile> >&)
#4    Object "/home/user/git/work/depthai_ctrl/build/depthai_ctrl/imu_conv_fuzz", at 0x58a7fe339e15, in fuzzer::Fuzzer::MutateAndTestOne()
#3    Object "/home/user/git/work/depthai_ctrl/build/depthai_ctrl/imu_conv_fuzz", at 0x58a7fe338629, in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*)
#2    Object "/home/user/git/work/depthai_ctrl/build/depthai_ctrl/imu_conv_fuzz", at 0x58a7fe338f34, in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long)
#1    Source "/home/user/git/work/depthai_ctrl/fuzz/imu_conv_fuzz.cc", line 58, in LLVMFuzzerTestOneInput [0x58a7fe42e82c]
         55:   dai::ros::ImuConverter imuConverter(frameName);
         56:   dai::ros::ImuMsgs::Imu outImuMsg;
         57:   try {
      >  58:     imuConverter.toRosMsg(inData, outImuMsg);
         59:     auto rosMsgPtr = imuConverter.toRosMsgPtr(inData);
         60:   } catch (const std::exception &e) {
         61:     // Handle any exceptions that may occur
#0    Source "/home/user/git/work/depthai_ctrl/src/ImuConverter.cpp", line 19, in toRosMsg [0x752399d23c53]
         17:   outImuMsg.header.frame_id = _frameName;
         18:
      >  19:   const auto imuPacket = inData->packets[inData->packets.size() - 1];
         20:
         21:   {
         22:     const auto & rVvalues = imuPacket.rotationVector;
Segmentation fault (Signal sent by the kernel [(nil)])
watf? exit

Reproduction Steps

If desired, I can share the fuzzing setup or contribute the fuzzer to the repository itself so it can be run by anyone, or even in the CI/CD.

Expected Behavior

In such cases where inData->packets.size() returns 0, we just pre-maturely return outImuMsg as is, which in turn can be handled by the caller side.

Actual Behavior

As shown in the stack-trace, when enabling UBSAN we can quickly catch that issue. With no sanitizer compiled into the library code, the crash may not occur, but some arbitrary memory is read and set to imuData.

Impact

I'm not yet familiar enough with the ecosystem and where the IMUConverter is being used but based on the project provided CMakeLists.txt the API is part of the compiled depthai_bridge library. Whether inData can be controlled by an attacker is unknown to me, but the fix is trivial and can mitigate a potential impact without any significant overhead.

Possible Fix

• I think a check for an empty array size and returning outImuMsg with no added orientation, angular_velocity, and linear_accelration is fine.
• Alternatively, one could add an assert, but this is something you typically still don't want to run into during runtime as it still crashes the application...

Additional Information

Duplicated in JIRA as PTAD-52