Skip to content

Force path check in TRT/RTX EP - #28099

Open
Chi Lo (chilo-ms) wants to merge 1 commit into
mainfrom
chi/path_traversal_security_fix
Open

Force path check in TRT/RTX EP#28099
Chi Lo (chilo-ms) wants to merge 1 commit into
mainfrom
chi/path_traversal_security_fix

Conversation

@chilo-ms

Copy link
Copy Markdown
Contributor

Description

For path check to prevent reading arbitrary files from the filesystem

Motivation and Context

@tianleiwu Tianlei Wu (tianleiwu) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Force path check in TRT/RTX EP

The fix in nv_tensorrt_rtx/onnx_ctx_model_helper.cc (inverting the boolean) is correct and aligns with the existing TRT EP implementation. Thank you for catching this.

However, the three false → true changes in CreateNodeComputeInfoFromGraph (both EPs) will likely break weight-stripped engine refit for the common case where users load models via an absolute file path. The path passed at those call sites is host-controlled (from graph.ModelPath()), not model-attribute-derived, so the anti-traversal guard is misplaced there. See the inline comment for a detailed trace.

Additionally, there are no tests covering the new behavior — either the security-positive case (malicious ONNX_MODEL_FILENAME attribute rejected) or the regression case (absolute model_path_ still works).

auto status = RefitEngine(model_path_,
onnx_model_folder_path_,
false /* path check for security */,
true /* path check for security */,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Regression risk: setting path_check=true here will reject absolute model paths, which is the dominant real-world usage.

Detailed trace:

  1. model_path_ (first arg) is set from graph.ModelPath().string() at nv_execution_provider.cc:1977-1983. This is the path the user supplied to InferenceSession::Load(...), typically an absolute path like /home/user/foo.onnx or C:\models\foo.onnx.

  2. onnx_model_folder_path_ (second arg) defaults to "" unless the user explicitly sets it via provider options (declared in nv_execution_provider_info.h, assigned at nv_execution_provider.cc:1028).

  3. onnx_model_bytestream_ defaults to nullptr and onnx_model_bytestream_size_ to 0 for file-based loading, so inside RefitEngine, refit_from_file = true and the path-check code is reached.

  4. Inside RefitEngine (nv_execution_provider.cc:2270-2287):

    std::filesystem::path onnx_model_path{onnx_model_folder_path};  // empty
    if (!onnx_model_filename.empty()) {
      onnx_model_path.append(onnx_model_filename);  // absolute path replaces per std::filesystem
    }
    // onnx_model_path is now "/home/user/foo.onnx" (absolute)
    if (path_check && IsAbsolutePath(onnx_model_path.string())) {
      return ORT_MAKE_STATUS(... "absolute path");  // ← fails here
    }

Key distinction: In GetEpContextFromGraph (the onnx_ctx_model_helper.cc code path), onnx_model_filename comes from attrs.at(ONNX_MODEL_FILENAME).s() — data embedded in a serialized model (potentially untrusted). Here, model_path_ comes from graph.ModelPath() — the path the host application itself opened (trusted). The anti-traversal guard is appropriate for the former but breaks the latter.

Suggestions (pick one):

  1. Revert this call site to false and rely on the onnx_ctx_model_helper.cc fix for the untrusted-input path. Add a comment explaining why.
  2. If you want defense-in-depth, validate only IsRelativePathToParentPath (reject .. traversal) but skip the IsAbsolutePath rejection, since an absolute path from the host is not a traversal vector.

The same issue applies to the two analogous sites in tensorrt_execution_provider.cc (lines 3700, 4193).

Comment thread onnxruntime/core/providers/nv_tensorrt_rtx/onnx_ctx_model_helper.cc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants