[OpenVINO EP] Validate that EPContext binary path is within model directory#28605
Closed
adrianlizarraga wants to merge 5 commits into
Closed
[OpenVINO EP] Validate that EPContext binary path is within model directory#28605adrianlizarraga wants to merge 5 commits into
adrianlizarraga wants to merge 5 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds validation for the OpenVINO EPContext external-blob path (ep_cache_context) to reduce path traversal risk when loading EPContext artifacts.
Changes:
- Introduces filesystem-based helpers to classify
ep_cache_contextas absolute vs. relative. - Adds
ORT_ENFORCEguards to reject absolute paths and paths containing..before resolving blob/context file locations. - Applies these validations in both EPContext blob reading (
GetModelBlobStream) and shared context initialization (Initialize).
Comments suppressed due to low confidence (2)
onnxruntime/core/providers/openvino/onnx_ctx_model_helper.cc:142
- These checks ensure
ep_cache_contextis not absolute and doesn't contain "..", but they still allow directory escape if the combined path resolves via symlinks, and they don't guard the case whereblob_filepath(base model/context path) is empty (then the lookup becomes relative to the process CWD). Consider computing a canonical/weakly_canonical combined path and enforcing it is within the canonical base directory, and ORT_ENFORCE that the base path is non-empty before joining.
// ep_cache_context must be a relative path within the context model directory.
ORT_ENFORCE(!IsAbsolutePath(ep_cache_context),
"ep_cache_context must be a relative path, but got absolute path: ", ep_cache_context);
ORT_ENFORCE(!IsRelativePathToParentPath(ep_cache_context),
"ep_cache_context must not contain '..'; it cannot point outside the model directory.");
blob_filepath = blob_filepath.parent_path() / ep_cache_context;
ORT_ENFORCE(std::filesystem::exists(blob_filepath), "Blob file not found: ", blob_filepath.string());
onnxruntime/core/providers/openvino/onnx_ctx_model_helper.cc:275
- Same path validation issue as in
GetModelBlobStream: rejecting absolute paths and ".." is not sufficient to guarantee the resolvedep_context_pathstays within the model directory (symlink escape, andGetOutputModelPath()can be empty so the base directory becomes empty). Consider enforcing a non-empty base path and verifying the canonicalized resolved path has the base directory as its prefix.
// ep_cache_context must be a relative path within the context model directory.
ORT_ENFORCE(!IsAbsolutePath(ep_cache_context),
"ep_cache_context must be a relative path, but got absolute path: ", ep_cache_context);
ORT_ENFORCE(!IsRelativePathToParentPath(ep_cache_context),
"ep_cache_context must not contain '..'; it cannot point outside the model directory.");
std::filesystem::path ep_context_path = session_context.GetOutputModelPath().parent_path() / ep_cache_context;
if (ep_context_path.extension() != ".xml") {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
onnxruntime/core/providers/openvino/onnx_ctx_model_helper.cc:145
- Before calling IsModelStreamXML, please validate that ep_cache_context is non-empty and that the resolved path is a regular file and readable. std::filesystem::exists() returns true for directories too, and std::ifstream can fail to open (permissions, directory path), which currently results in a less actionable failure inside IsModelStreamXML (tellg/seekg/size checks).
ORT_ENFORCE(!IsRelativePathToParentPath(ep_cache_context),
"ep_cache_context must not contain '..'; it cannot point outside the model directory.");
blob_filepath = blob_filepath.parent_path() / ep_cache_context;
ORT_ENFORCE(std::filesystem::exists(blob_filepath), "Blob file not found: ", blob_filepath.string());
result.reset((std::istream*)new std::ifstream(blob_filepath, std::ios_base::binary | std::ios_base::in));
}
Contributor
|
Closing in favor of #28725 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds path validation for the
ep_cache_contextattribute in the OpenVINO Execution Provider to prevent path traversal attacks. A malicious ONNX model could craft anep_cache_contextvalue (e.g.,../../../../etc/passwd) to read arbitrary files from the host filesystem when usingembed_mode=0(external blob file).This aligns the OpenVINO EP with the existing validation already present in the TensorRT EP and NV TensorRT RTX EP.
Changes
ValidateEpCacheContextPath()which:ep_cache_contextvalues/and WindowsC:\style)std::filesystem::weakly_canonicalto prevent symlink-based traversalep_cache_contextas a file path:GetModelBlobStream()— blob loading during inferenceInitialize()— shared context initializationMotivation
Without this validation, a crafted ONNX model with a path-traversing
ep_cache_contextattribute could cause the OpenVINO EP to open and read arbitrary files accessible to the process. This is a security hardening change to ensure the EP only loads blob files from within the model's own directory tree.Testing
../-based traversal, and symlink escapes are rejected