Heap Out-of-Bounds Read in IR String Constant Deserialization by bumbosiepsak · Pull Request #34489 · openvinotoolkit/openvino

bumbosiepsak · 2026-03-04T12:05:42Z

Vulnerability fixed by validation of memory access in aux_unpack_string_tensor()

Tickets:

CVS-182172

Copilot

Pull request overview

Fixes a heap out-of-bounds read vulnerability in OpenVINO IR packed string-constant deserialization by adding stronger validation of the packed buffer header and per-string offsets before constructing std::string instances.

Changes:

Validate num_strings is non-negative and ensure the computed header fits within the provided buffer.
Validate per-string begin/end offsets (non-negative, monotonic, and within the data region) before reading.
Use validated size_t offsets when constructing unpacked std::string values.

src/core/src/runtime/string_aligned_buffer.cpp

praasz · 2026-03-04T12:34:21Z

@bumbosiepsak
Please provide unit test

src/core/src/runtime/string_aligned_buffer.cpp

src/core/tests/string_unpack_tensor_test.cpp

praasz · 2026-03-09T12:22:16Z

src/core/src/runtime/string_aligned_buffer.cpp

+    // Calculate header size: [num_strings][0][offset1...offsetN]
+    const size_t num_elements = static_cast<size_t>(num_strings);
+    // Check addition overflow for header element count (2 + num_elements)
+    OPENVINO_ASSERT(num_elements <= std::numeric_limits<size_t>::max() - 2,


Is it possible if non-negative i32 is converted to the max value should be 1/2 or 1/4 of size_t

I'm not sure if I understand 😊 Could you elaborate a little, please?

src/core/src/runtime/string_aligned_buffer.cpp

praasz · 2026-03-09T12:26:12Z

src/core/src/runtime/string_aligned_buffer.cpp

    // first run over all elements: calculate total memory required to hold all strings
-    header_size = sizeof(int32_t) * (1 + 1 + num_elements);
+    // Check addition overflow for header element count (1 + 1 + num_elements)
+    OPENVINO_ASSERT(num_elements <= std::numeric_limits<size_t>::max() - 2,


Add test for assertion

I removed this check in favour of checking, that size_t is at least as big, as int32_t. (I hope we don't need to support 16-bit platforms...)

Then the positive range of size_t always sufficient then (twice as big, as int32_t).

I think 16-bit platforms are not a case for OV

praasz · 2026-03-09T12:29:19Z

src/core/tests/string_unpack_tensor_test.cpp

Test can put into string_align_buffer_test.cpp file

Actually this test file grew a little. Is it OK if I keep it separate?

I'll move content to string_align_buffer_test.cpp if you say so ;)

Is better keep all of them in same file.

src/core/tests/string_unpack_tensor_test.cpp

src/core/src/runtime/string_aligned_buffer.cpp

praasz · 2026-03-09T12:36:41Z

src/core/src/runtime/string_aligned_buffer.cpp

-    OPENVINO_ASSERT(int32_t(size) >= 4 + 4 + 4 * num_strings,
-                    "Incorrect packed string tensor format: the packed string tensor must contain first "
-                    "string offset and end indices");
+    OPENVINO_ASSERT(num_strings >= 0, "Incorrect packed string tensor format: negative number of strings");


is required to check?
if is negative it will be large number maybe updated check std::numeric_limits<size_t>::max() can detect it also?

I sticked to the convention already established in this function, that the header consists of signed integers.

Question: can we imply, that they are unsigned (and switch to treating them as such everywhere)?

- Vulnerability fixed

- Additional overflow protection added - Variable usage streamlined with type unification

bumbosiepsak · 2026-03-12T17:59:41Z

src/core/src/runtime/string_aligned_buffer.cpp

+    header_element_t* header = reinterpret_cast<header_element_t*>(data.get());
+    header[0] = static_cast<header_element_t>(strings_count);
+
+    if (strings_count > 0) {


Question: what is the wanted buffer shape in case of zero strings provided?

Do we still want to create and fill the tail (i.e. begin and end of a string, that doesn't exist)?

If Constant has shape [2] and her string count is zero the constant will be not correct as shape and number of data not match.

It looks like some kind of error case

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

src/core/tests/string_unpack_tensor_test.cpp

src/core/src/runtime/string_aligned_buffer.cpp

- Unit test added

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

evkotov · 2026-03-13T15:57:35Z

src/core/src/runtime/string_aligned_buffer.cpp

+    data = std::shared_ptr<uint8_t>(new uint8_t[header_size], std::default_delete<uint8_t[]>());
+
+    header_element_t* header = reinterpret_cast<header_element_t*>(data.get());
+    header[0] = static_cast<header_element_t>(strings_count);


Both strings_count (from get_num_elements()) and current_string_end are size_t values cast to int32_t via static_cast without overflow validation. If either value exceeds INT32_MAX, this is signed integer overflow — undefined behavior in C++. Since this is a security-focused PR, the packing side should also be hardened:

OPENVINO_ASSERT(strings_count <= static_cast<size_t>(std::numeric_limits<header_element_t>::max()),
"Too many strings to pack: strings_count exceeds int32_t range");
Similarly, current_string_end should be validated before each cast:

OPENVINO_ASSERT(current_string_end <= static_cast<size_t>(std::numeric_limits<header_element_t>::max()),
"Cumulative string size exceeds int32_t range");

- Reworked according to review remarks - Naming cleaned - Checks made tighter

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

src/core/src/runtime/string_aligned_buffer.cpp

+    using header_element_t = int32_t;  // Type of a single element in the header (strings_count and offsets)
+
+    static_assert(sizeof(header_element_t) <= sizeof(size_t),
+                  "Header element type must be able to represent offsets and number of strings as size_t");


src/core/tests/string_unpack_tensor_test.cpp

+
+/// @brief Test case for string offset exceeding buffer bounds in packed string tensor.
+/// Expecting AssertFailure with message about string offset exceeds buffer bounds.
+/// num_strings = 1, header: [1, 0, end0=10], but buffer too small


praasz · 2026-03-16T11:13:26Z

src/core/src/runtime/string_aligned_buffer.cpp

+                        "Incorrect packed string tensor format: negative string offset in the packed string tensor");
+
+        OPENVINO_ASSERT(begin_signed <= end_signed,
+                        "Incorrect packed string tensor format: begin offset greater than end offset");


"Incorrect packed string tensor format: begin offset greater than end offset");

begin offset greater than end offset this give no information as condition is serialized
Optional
Is always keep as number and message of assert as small as possible (but explain error) to not add which is not is usually used.

praasz · 2026-03-16T11:14:17Z

src/core/tests/string_unpack_tensor_test.cpp

+
+}  // namespace
+
+namespace ov {


Suggested change

namespace ov {

namespace ov::test {

praasz · 2026-03-16T11:14:36Z

src/core/tests/string_unpack_tensor_test.cpp

+using header_element_t = int32_t;  // Type of a single element in the header (strings_count and offsets)
+
+namespace {


Move all inside ov::test

praasz · 2026-03-16T11:14:59Z

src/core/tests/string_unpack_tensor_test.cpp

+/// Expecting AssertFailure with message about begin offset greater than end offset.
+/// num_strings = 2, header: [2, 0, end0=-3, end1=5]
+TEST(StringUnpackTensorTest, NegativeOffsetsFails) {
+    using testing::HasSubstr;


It can be used once in this file

praasz · 2026-03-16T11:16:04Z

src/core/tests/string_unpack_tensor_test.cpp

+/// @brief Test case for missing number of strings in packed string tensor.
+/// Expecting AssertFailure with message about missing strings count.
+/// num_strings = <missing>


These comment are not required

praasz · 2026-03-16T11:17:59Z

src/core/src/runtime/string_aligned_buffer.cpp

    // first run over all elements: calculate total memory required to hold all strings
-    header_size = sizeof(int32_t) * (1 + 1 + num_elements);
+    // Check addition overflow for header element count (1 + 1 + num_elements)
+    OPENVINO_ASSERT(num_elements <= std::numeric_limits<size_t>::max() - 2,


I think 16-bit platforms are not a case for OV

evkotov

LGTM

bumbosiepsak requested a review from a team as a code owner March 4, 2026 12:05

bumbosiepsak requested review from Copilot and removed request for a team March 4, 2026 12:05

github-actions bot added the category: Core OpenVINO Core (aka ngraph) label Mar 4, 2026

Copilot started reviewing on behalf of bumbosiepsak March 4, 2026 12:06 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

src/core/src/runtime/string_aligned_buffer.cpp Outdated Show resolved Hide resolved

src/core/src/runtime/string_aligned_buffer.cpp Outdated Show resolved Hide resolved

bumbosiepsak requested review from CuriousPanCake, evkotov and mvafin March 4, 2026 12:09

praasz self-assigned this Mar 4, 2026

praasz added the pr: needs tests PR needs tests updating label Mar 4, 2026

evkotov requested changes Mar 4, 2026

View reviewed changes

src/core/src/runtime/string_aligned_buffer.cpp Outdated Show resolved Hide resolved

bumbosiepsak force-pushed the CVS-182172-heap-out-of-bounds-read-in-ir-string-constant branch from 01760f4 to 8c65373 Compare March 9, 2026 12:11

praasz reviewed Mar 9, 2026

View reviewed changes

bumbosiepsak added 2 commits March 10, 2026 09:32

CVS-182172 Heap Out-of-Bounds Read in IR String Constant Deserialization

ee0864f

- Vulnerability fixed

CVS-182172 Heap Out-of-Bounds Read in IR String Constant Deserialization

579529d

- Additional overflow protection added - Variable usage streamlined with type unification

bumbosiepsak force-pushed the CVS-182172-heap-out-of-bounds-read-in-ir-string-constant branch 2 times, most recently from e36bd58 to adeba47 Compare March 12, 2026 17:55

bumbosiepsak commented Mar 12, 2026

View reviewed changes

bumbosiepsak requested review from Copilot, evkotov and praasz March 12, 2026 18:01

Copilot started reviewing on behalf of bumbosiepsak March 12, 2026 18:02 View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

src/core/tests/string_unpack_tensor_test.cpp Show resolved Hide resolved

src/core/src/runtime/string_aligned_buffer.cpp Outdated Show resolved Hide resolved

src/core/src/runtime/string_aligned_buffer.cpp Outdated Show resolved Hide resolved

CVS-182172 Heap Out-of-Bounds Read in IR String Constant Deserialization

bdaa8ba

- Unit test added

bumbosiepsak force-pushed the CVS-182172-heap-out-of-bounds-read-in-ir-string-constant branch from adeba47 to 2f32e86 Compare March 12, 2026 18:35

bumbosiepsak requested a review from Copilot March 12, 2026 18:37

Copilot started reviewing on behalf of bumbosiepsak March 12, 2026 18:38 View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

bumbosiepsak enabled auto-merge March 13, 2026 15:54

evkotov requested changes Mar 13, 2026

View reviewed changes

CVS-182172 Heap Out-of-Bounds Read in IR String Constant Deserialization

52cd61e

- Reworked according to review remarks - Naming cleaned - Checks made tighter

bumbosiepsak force-pushed the CVS-182172-heap-out-of-bounds-read-in-ir-string-constant branch from 2f32e86 to 52cd61e Compare March 13, 2026 16:39

mryzhov requested a review from Copilot March 16, 2026 08:57

Copilot AI reviewed Mar 16, 2026

View reviewed changes

Copilot started reviewing on behalf of mryzhov March 16, 2026 09:05 View session

praasz reviewed Mar 16, 2026

View reviewed changes

evkotov self-requested a review March 16, 2026 14:02

evkotov approved these changes Mar 16, 2026

View reviewed changes

		using header_element_t = int32_t; // Type of a single element in the header (strings_count and offsets)

		namespace {

Conversation

bumbosiepsak commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tickets:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

praasz commented Mar 4, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bumbosiepsak Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evkotov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

bumbosiepsak commented Mar 4, 2026 •

edited

Loading

bumbosiepsak Mar 12, 2026 •

edited

Loading