Fix: OpenVINO EP DeserializeImpl allows unbounded uint64 allocation#1158
Fix: OpenVINO EP DeserializeImpl allows unbounded uint64 allocation#1158vthaniel wants to merge 1 commit into
Conversation
vthaniel
requested review from
MayureshV1,
ankitm3k,
jatinwadhwa921 and
n1harika
There was a problem hiding this comment.
Pull request overview
Hardens OpenVINO EP-context cache deserialization against malicious/corrupted OVEP_BIN payloads that can trigger unbounded allocations by validating advertised BSON/blob ranges against the actual stream size, and adds a regression test exercising the failure path.
Changes:
- Adds stream-size-based bounds checking for the BSON region prior to BSON allocation/parse.
- Adds stream-size-based bounds checking for embedded blob ranges prior to allocating/reading blob data.
- Adds a unit test that embeds a crafted OVEP_BIN header in an EPContext node and asserts deserialization rejects an oversized BSON region.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
onnxruntime/core/providers/openvino/ov_bin_manager.cc |
Adds stream-size calculation and range validation for BSON and embedded blob allocations during deserialization. |
onnxruntime/test/providers/openvino/openvino_ep_context_test.cc |
Adds regression test building an embedded EPContext model with a malicious OVEP_BIN header to ensure deserialization fails safely. |
| stream.seekg(0, std::ios::end); | ||
| ORT_ENFORCE(stream.good(), "Error: Failed to seek to end of stream."); | ||
| const uint64_t stream_size = static_cast<uint64_t>(stream.tellg()); | ||
|
|
| std::string model_data; | ||
| model.SerializeToString(&model_data); | ||
| return model_data; |
| auto& logging_manager = DefaultLoggingManager(); | ||
| logging_manager.SetDefaultLoggerSeverity(logging::Severity::kERROR); | ||
|
|
n1harika
left a comment
n1harika
left a comment
There was a problem hiding this comment.
@vthaniel LGTM. We need this change as the blob size is deserialized from disk, not memory, here:
Since the file can be corrupted, this is a robustness check.
In ABI-EP, the bin file is memory mapped first: https://github.com/intel-innersource/frameworks.ai.onnxruntime.openvino-plugin-ep/blob/39d6af4d734e0e8c91f40cf31e1b5eebef036a9f/plugin_impl/ov_bin_manager.cc#L165 and then the blob size is checked here: https://github.com/intel-innersource/frameworks.ai.onnxruntime.openvino-plugin-ep/blob/39d6af4d734e0e8c91f40cf31e1b5eebef036a9f/plugin_impl/ov_bin_manager.cc#L215
Description
Added bounds check which will throw if certain BSON fields does not lie fully within the stream.
Added testcase to simulate this scenario
Motivation and Context
OpenVINO EP DeserializeImpl allows unbounded uint64 allocation, which can create security issues
This PR adds a bounds check to fix this