Merge pull request #4550 from MiniRML/fix/nid-buffer-validation

fix(nid): validate buffer ranges and string terminators

core/libs/CMakeLists.txt

@@ -179,6 +179,15 @@ if(BUILD_TESTING)
         set_tests_properties(guest_posix_scalar_io PROPERTIES ENVIRONMENT_MODIFICATION
             "PATH=path_list_prepend:$<TARGET_FILE_DIR:libkernel>")
     endif()
+    add_test_executable(nid_buffer_validation_tests nid/tests/BufferValidation.cpp)
+    target_include_directories(nid_buffer_validation_tests PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/nid/include)
+    target_compile_features(nid_buffer_validation_tests PRIVATE cxx_std_20)
+    if(MINGW)
+        target_compile_options(nid_buffer_validation_tests PRIVATE -fasynchronous-unwind-tables)
+    endif()
+    add_test(NAME nid_buffer_validation COMMAND nid_buffer_validation_tests)
+    set_tests_properties(nid_buffer_validation PROPERTIES TIMEOUT 10)
+
     find_package(Python3 QUIET COMPONENTS Interpreter)
     if(Python3_Interpreter_FOUND)
         add_test(NAME nid_patcher_macho COMMAND ${Python3_EXECUTABLE}

core/libs/nid/include/nid/NidPatcherUtils.hpp

@@ -61,7 +61,7 @@ inline std::string StripNidPostfix(const std::string& name) {
 
 template<typename T>
 T Read(const std::vector<std::uint8_t>& buf, std::size_t offset) {
-    if (offset + sizeof(T) > buf.size()) throw std::runtime_error("read out of bounds");
+    if (offset > buf.size() || sizeof(T) > buf.size() - offset) throw std::runtime_error("read out of bounds");
     T v;
     std::memcpy(&v, buf.data() + offset, sizeof(T));
     return v;
@@ -69,7 +69,7 @@ T Read(const std::vector<std::uint8_t>& buf, std::size_t offset) {
 
 template<typename T>
 void Write(std::vector<std::uint8_t>& buf, std::size_t offset, const T& v) {
-    if (offset + sizeof(T) > buf.size()) throw std::runtime_error("write out of bounds");
+    if (offset > buf.size() || sizeof(T) > buf.size() - offset) throw std::runtime_error("write out of bounds");
     std::memcpy(buf.data() + offset, &v, sizeof(T));
 }
 
@@ -78,6 +78,7 @@ inline std::string ReadCStr(const std::vector<std::uint8_t>& buf, std::size_t of
     std::string s;
     while (offset < buf.size() && buf[offset] != 0u)
         s.push_back(static_cast<char>(buf[offset++]));
+    if (offset == buf.size()) throw std::runtime_error("unterminated cstr");
     return s;
 }
 

core/libs/nid/tests/BufferValidation.cpp

@@ -0,0 +1,60 @@
+#include <nid/NidPatcherUtils.hpp>
+#include <cstdint>
+#include <iostream>
+#include <limits>
+#include <stdexcept>
+#include <vector>
+
+namespace {
+
+void Require(bool condition) {
+    if (!condition) throw std::runtime_error("NID buffer validation failed");
+}
+
+template<typename TAction>
+void RequireRejected(TAction action) {
+    try { action(); }
+    catch (const std::runtime_error&) { return; }
+    throw std::runtime_error("Malformed NID buffer was accepted");
+}
+
+}
+
+int main() {
+    try {
+        using namespace Nid::Internal;
+        const std::vector<std::uint8_t> unterminated{'n', 'a', 'm', 'e'};
+        RequireRejected([&] { ReadCStr(unterminated, 0); });
+        RequireRejected([&] { ReadCStr(unterminated, 3); });
+        const std::vector<std::uint8_t> strings{'a', 0, 'b', 'c', 0};
+        Require(ReadCStr(strings, 0) == "a");
+        Require(ReadCStr(strings, 1).empty());
+        Require(ReadCStr(strings, 2) == "bc");
+        Require(ReadCStr(strings, 4).empty());
+        RequireRejected([&] { ReadCStr(strings, strings.size()); });
+        RequireRejected([&] { ReadCStr({}, 0); });
+
+        std::vector<std::uint8_t> bytes(8, 0xa5);
+        const std::uint64_t value = 0x1020304050607080;
+        Write(bytes, 0, value);
+        Require(Read<std::uint64_t>(bytes, 0) == value);
+        Write<std::uint32_t>(bytes, 4, 0x12345678);
+        Require(Read<std::uint32_t>(bytes, 4) == 0x12345678);
+        const auto before = bytes;
+        for (const auto offset : {std::size_t{5}, std::size_t{8}, std::size_t{9},
+                std::numeric_limits<std::size_t>::max(), std::numeric_limits<std::size_t>::max() - 1,
+                std::numeric_limits<std::size_t>::max() - 3}) {
+            RequireRejected([&] { Read<std::uint32_t>(bytes, offset); });
+            RequireRejected([&] { Write<std::uint32_t>(bytes, offset, 0); });
+            Require(bytes == before);
+        }
+        RequireRejected([&] { Read<std::uint64_t>(bytes, 1); });
+        std::vector<std::uint8_t> empty;
+        RequireRejected([&] { Read<std::uint8_t>(empty, 0); });
+        RequireRejected([&] { Write<std::uint8_t>(empty, 0, 0); });
+        std::cout << "NID buffer validation passed\n";
+    } catch (const std::exception& error) {
+        std::cerr << error.what() << '\n';
+        return 1;
+    }
+}