Skip to content

Commit 4369676

Browse files
author
Gopalakrishnan Nallasamy
committed
EPContext sample helper: drop coarse traversal guard from trusted path resolution
Addresses #28624 review feedback. Now that IsResolvedPathWithinBase() does real model-directory containment for untrusted model-relative names, the lexical ContainsPathTraversal() guard is no longer applied on the trusted graph==nullptr branch of ResolveEpContextDataPath(): trusted callers already may pass absolute paths and own their paths, so there is no model directory to contain against. ContainsPathTraversal() is kept solely for ValidateEpContextDataName(), which validates the logical callback-namespace name written into the model ep_cache_context attribute (never resolved against a filesystem base). Updates the two trusted-branch tests; logical-name and model-directory containment coverage is unchanged.
1 parent 5b77af4 commit 4369676

2 files changed

Lines changed: 21 additions & 15 deletions

File tree

onnxruntime/test/autoep/ep_context_data_utils_test.cc

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -127,9 +127,10 @@ TEST(OrtEpLibrary, EpContextDataUtils_ResolvePathRejectsUnsafeNames) {
127127
const auto& api = Ort::GetApi();
128128
std::filesystem::path data_path;
129129

130-
ExpectOrtStatusError(ep_context_data_utils::ResolveEpContextDataPath(api, "../escape.ctx", nullptr, data_path),
131-
ORT_INVALID_ARGUMENT, "EPContext data file name must not contain path traversal");
132-
EXPECT_TRUE(data_path.empty());
130+
// Trusted direct callers (graph == nullptr) own the path: ".." is allowed (there is no model directory to contain
131+
// against, and absolute paths are already permitted) and the name is accepted as-is rather than rejected.
132+
ASSERT_ORTSTATUS_OK(ep_context_data_utils::ResolveEpContextDataPath(api, "../escape.ctx", nullptr, data_path));
133+
EXPECT_FALSE(data_path.empty());
133134

134135
#ifdef _WIN32
135136
const char* absolute_file_name = "C:\\temp\\escape.ctx";
@@ -151,8 +152,10 @@ TEST(OrtEpLibrary, EpContextDataUtils_ResolvePathRejectsUnsafeNames) {
151152
#endif
152153

153154
std::vector<char> data;
155+
// Trusted (graph == nullptr) reads also allow ".."; the resolver no longer rejects it, so a non-existent target now
156+
// surfaces as a normal file-open failure rather than a traversal rejection.
154157
ExpectOrtStatusError(ep_context_data_utils::ReadEpContextDataFromFile(api, "../escape.ctx", nullptr, data),
155-
ORT_INVALID_ARGUMENT, "EPContext data file name must not contain path traversal");
158+
ORT_FAIL, "Failed to open EPContext data file for read");
156159
ExpectOrtStatusError(ep_context_data_utils::WriteEpContextDataWithFileFallback(
157160
api, nullptr, absolute_file_name, "unused.ctx", nullptr, nullptr, 0),
158161
ORT_INVALID_ARGUMENT, "EPContext data file name must not be absolute or rooted");

onnxruntime/test/autoep/library/ep_context_data_utils.h

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -145,11 +145,13 @@ inline std::string PathToUtf8StringForMessage(const std::filesystem::path& path)
145145
return status.IsOK() ? utf8_path : std::string{"<path conversion failed>"};
146146
}
147147

148-
// Lexical check for a ".." component. This is a coarse guard used when there is no filesystem base directory to
149-
// contain against: logical callback-namespace names and trusted (graph == nullptr) physical paths. It is NOT a
150-
// containment mechanism: it does not resolve symlinks and it rejects benign cases such as "a/b/c/../file.txt".
151-
// Filesystem containment against a model directory is done by IsResolvedPathWithinBase() below, which the untrusted
152-
// (model-relative) resolution path uses.
148+
// Lexical check for a ".." component. Used only by ValidateEpContextDataName() to reject ".." in the logical
149+
// callback-namespace name that is written into the EPContext model's ep_cache_context attribute. That logical name is
150+
// never resolved against a filesystem base, so the model-directory containment check (IsResolvedPathWithinBase()
151+
// below) cannot apply to it; rejecting ".." up front keeps a generated model from embedding an unsafe relative
152+
// reference. It is NOT a containment mechanism: it does not resolve symlinks and it rejects benign cases such as
153+
// "a/b/c/../file.txt". Filesystem containment against a model directory is done by IsResolvedPathWithinBase(), which
154+
// the untrusted (model-relative) resolution path uses.
153155
inline bool ContainsPathTraversal(const std::filesystem::path& path) {
154156
const std::filesystem::path parent_dir{".."};
155157
for (const auto& component : path) {
@@ -232,8 +234,9 @@ inline OrtStatus* ValidateEpContextDataName(const OrtApi& api, const char* file_
232234
// Resolves `file_name` to a filesystem path for reading or writing EPContext data (used by both the read path and
233235
// the write-fallback path).
234236
//
235-
// When `graph` is null the caller is trusted and owns the path: `file_name` is returned as-is and may be absolute (a
236-
// lexical ".." is still rejected as a coarse guard). When `graph` is non-null, `file_name` originates from the
237+
// When `graph` is null the caller is trusted and owns the path: `file_name` is returned as-is and may be absolute and
238+
// may contain ".." (there is no model directory to contain against, and absolute paths are already permitted, so no
239+
// traversal check is applied). When `graph` is non-null, `file_name` originates from the
237240
// untrusted EPContext model "ep_cache_context" attribute: the graph must have a model path, the name must be
238241
// relative, and after combining it with the model's directory the result must stay within that directory. Symlinks and
239242
// ".." are resolved (via weakly_canonical), so a name that escapes the model directory - including through a symlink -
@@ -255,11 +258,11 @@ inline OrtStatus* ResolveEpContextDataPath(const OrtApi& api, const char* file_n
255258
return api.CreateStatus(ORT_INVALID_ARGUMENT, "EPContext data file name is not a valid path");
256259
}
257260

258-
// Trusted direct callers (graph == nullptr) own the path and may pass an absolute physical path.
261+
// Trusted direct callers (graph == nullptr) own the path and may pass an absolute physical path, including one with
262+
// ".." components. There is no model directory to contain against here, and absolute paths are already allowed, so
263+
// no traversal check is applied; the untrusted (model-relative) branch below is the one constrained to the model
264+
// directory.
259265
if (graph == nullptr) {
260-
if (ContainsPathTraversal(candidate_path)) {
261-
return api.CreateStatus(ORT_INVALID_ARGUMENT, "EPContext data file name must not contain path traversal");
262-
}
263266
data_path = candidate_path;
264267
return nullptr;
265268
}

0 commit comments

Comments
 (0)