Address security issue of loading arbitrary files as external data

onnxruntime/core/framework/tensorprotoutils.cc

@@ -306,6 +306,23 @@ Status TensorProtoWithExternalDataToTensorProto(
   return Status::OK();
 }
 
+Status ValidateExternalDataPath(const std::filesystem::path& base_dir,
+                                const std::filesystem::path& location) {
+  // Reject absolute paths
+  ORT_RETURN_IF(location.is_absolute(),
+                "Absolute paths not allowed for external data location");
+  // Resolve and verify the path stays within model directory
+  auto resolved = std::filesystem::weakly_canonical(base_dir / location);
+  auto base_canonical = std::filesystem::weakly_canonical(base_dir);
+  // Check that resolved path starts with base directory
+  auto [base_end, resolved_it] = std::mismatch(
+      base_canonical.begin(), base_canonical.end(),
+      resolved.begin(), resolved.end());
+  ORT_RETURN_IF(base_end != base_canonical.end(),
+                "External data path: ", location, " escapes model directory: ", base_dir);
+  return Status::OK();
+}
+
 Status GetExternalDataInfo(const ONNX_NAMESPACE::TensorProto& tensor_proto,
                            const std::filesystem::path& tensor_proto_dir,
                            std::basic_string<ORTCHAR_T>& external_file_path,

onnxruntime/core/framework/tensorprotoutils.h

@@ -521,6 +521,17 @@ Status TensorProtoWithExternalDataToTensorProto(
     const std::filesystem::path& model_path,
     ONNX_NAMESPACE::TensorProto& new_tensor_proto);
 
+/// <summary>
+/// The functions will make sure the 'location' specified in the external data is under the 'base_dir'.
+/// </summary>
+/// <param name="base_dir">model location directory</param>
+/// <param name="location">location is a string retrieved from TensorProto external data that is not
+///                        an in-memory tag</param>
+/// <returns>The function will fail if the resolved full path is not under the model directory
+///          or one of the subdirectories</returns>
+Status ValidateExternalDataPath(const std::filesystem::path& base_dir,
+                                const std::filesystem::path& location);
+
 #endif  // !defined(SHARED_PROVIDER)
 
 inline bool HasType(const ONNX_NAMESPACE::AttributeProto& at_proto) {

onnxruntime/core/graph/graph.cc

@@ -3724,9 +3724,14 @@ Status Graph::ConvertInitializersIntoOrtValues() {
   std::vector<Graph*> all_subgraphs;
   FindAllSubgraphs(all_subgraphs);
 
+  const auto& model_path = GetModel().ModelPath();
+  PathString model_dir;
+  if (!model_path.empty()) {
+    ORT_RETURN_IF_ERROR(GetDirNameFromFilePath(model_path, model_dir));
+  }
+
   auto put_weights_maybe_in_memory_func = [&](Graph& graph) -> Status {
     // if we have any initializers that are not in memory, put them there.
-    const auto& model_path = graph.ModelPath();
     auto& graph_proto = *graph.graph_proto_;
     for (int i = 0, lim = graph_proto.initializer_size(); i < lim; ++i) {
       auto& tensor_proto = *graph_proto.mutable_initializer(i);
@@ -3744,9 +3749,18 @@ Status Graph::ConvertInitializersIntoOrtValues() {
                                    "The model contains initializers with arbitrary in-memory references.",
                                    "This is an invalid model.");
           }
+        } else {
+          // Validate external data location
+          std::unique_ptr<onnxruntime::ExternalDataInfo> external_data_info;
+          ORT_RETURN_IF_ERROR(onnxruntime::ExternalDataInfo::Create(tensor_proto.external_data(), external_data_info));
+          const auto& location = external_data_info->GetRelPath();
+          auto st = utils::ValidateExternalDataPath(model_dir, location);
+          if (!st.IsOK()) {
+            return ORT_MAKE_STATUS(ONNXRUNTIME, FAIL,
+                                   "External data path validation failed for initializer: ", tensor_proto.name(),
+                                   ". Error: ", st.ErrorMessage());
+          }
         }
-        // ignore data on disk, that will be loaded either by EP or at session_state finalize
-        // ignore valid in-memory references
         continue;
       }
 

onnxruntime/core/providers/shared_library/provider_api.h

@@ -439,6 +439,11 @@ inline bool HasExternalDataInMemory(const ONNX_NAMESPACE::TensorProto& ten_proto
   return g_host->Utils__HasExternalDataInMemory(ten_proto);
 }
 
+inline Status ValidateExternalDataPath(const std::filesystem::path& base_dir,
+                                       const std::filesystem::path& location) {
+  return g_host->Utils__ValidateExternalDataPath(base_dir, location);
+}
+
 }  // namespace utils
 
 namespace graph_utils {

onnxruntime/core/providers/shared_library/provider_interfaces.h

@@ -38,6 +38,12 @@ struct ProviderHost;
 struct ProviderHostCPU;
 
 class ExternalDataInfo;
+#ifdef _WIN32
+using OFFSET_TYPE = int64_t;
+#else
+using OFFSET_TYPE = off_t;
+#endif
+
 class PhiloxGenerator;
 using ProviderType = const std::string&;
 class RandomGenerator;
@@ -999,6 +1005,9 @@ struct ProviderHost {
 
   virtual bool Utils__HasExternalDataInMemory(const ONNX_NAMESPACE::TensorProto& ten_proto) = 0;
 
+  virtual Status Utils__ValidateExternalDataPath(const std::filesystem::path& base_path,
+                                                 const std::filesystem::path& location) = 0;
+
   // Model
   virtual std::unique_ptr<Model> Model__construct(ONNX_NAMESPACE::ModelProto&& model_proto, const PathString& model_path,
                                                   const IOnnxRuntimeOpSchemaRegistryList* local_registries,
@@ -1136,6 +1145,15 @@ struct ProviderHost {
 
   virtual Status GraphUtils__ConvertInMemoryDataToInline(Graph& graph, const std::string& name) = 0;
 
+  // ExternalDataInfo
+  virtual void ExternalDataInfo__operator_delete(ExternalDataInfo*) = 0;
+  virtual const PathString& ExternalDataInfo__GetRelPath(const ExternalDataInfo*) const = 0;
+  virtual OFFSET_TYPE ExternalDataInfo__GetOffset(const ExternalDataInfo*) const = 0;
+  virtual size_t ExternalDataInfo__GetLength(const ExternalDataInfo*) const = 0;
+  virtual const std::string& ExternalDataInfo__GetChecksum(const ExternalDataInfo*) const = 0;
+  virtual Status ExternalDataInfo__Create(const ONNX_NAMESPACE::StringStringEntryProtos& input,
+                                          std::unique_ptr<ExternalDataInfo>& out) = 0;
+
   // Initializer
   virtual Initializer* Initializer__constructor(ONNX_NAMESPACE::TensorProto_DataType data_type,
                                                 std::string_view name,

onnxruntime/core/providers/shared_library/provider_wrappedtypes.h

@@ -1201,6 +1201,38 @@ struct ConstGraphNodes final {
   PROVIDER_DISALLOW_ALL(ConstGraphNodes)
 };
 
+class ExternalDataInfo {
+  static void operator delete(void* p) {
+    g_host->ExternalDataInfo__operator_delete(reinterpret_cast<ExternalDataInfo*>(p));
+  }
+
+  const PathString& GetRelPath() const {
+    return g_host->ExternalDataInfo__GetRelPath(this);
+  }
+
+  OFFSET_TYPE GetOffset() const {
+    return g_host->ExternalDataInfo__GetOffset(this);
+  }
+
+  size_t GetLength() const {
+    return g_host->ExternalDataInfo__GetLength(this);
+  }
+
+  const std::string& GetChecksum() const {
+    return g_host->ExternalDataInfo__GetChecksum(this);
+  }
+
+  static Status Create(
+      const ONNX_NAMESPACE::StringStringEntryProtos& input,
+      std::unique_ptr<ExternalDataInfo>& out) {
+    return g_host->ExternalDataInfo__Create(input, out);
+  }
+
+  ExternalDataInfo() = delete;
+  ExternalDataInfo(const ExternalDataInfo&) = delete;
+  ExternalDataInfo& operator=(const ExternalDataInfo& v) = delete;
+};
+
 class Initializer {
  public:
   Initializer(ONNX_NAMESPACE::TensorProto_DataType data_type,

onnxruntime/core/session/provider_bridge_ort.cc

@@ -29,6 +29,7 @@
 #include "core/framework/run_options.h"
 #include "core/framework/sparse_utils.h"
 #include "core/framework/tensorprotoutils.h"
+#include "core/framework/tensor_external_data_info.h"
 #include "core/framework/TensorSeq.h"
 #include "core/graph/constants.h"
 #include "core/graph/graph_proto_serializer.h"
@@ -1281,6 +1282,11 @@ struct ProviderHostImpl : ProviderHost {
     return onnxruntime::utils::HasExternalDataInMemory(ten_proto);
   }
 
+  Status Utils__ValidateExternalDataPath(const std::filesystem::path& base_path,
+                                         const std::filesystem::path& location) override {
+    return onnxruntime::utils::ValidateExternalDataPath(base_path, location);
+  }
+
   // Model (wrapped)
   std::unique_ptr<Model> Model__construct(ONNX_NAMESPACE::ModelProto&& model_proto, const PathString& model_path,
                                           const IOnnxRuntimeOpSchemaRegistryList* local_registries,
@@ -1487,6 +1493,25 @@ struct ProviderHostImpl : ProviderHost {
     graph_utils::MakeInitializerCopyIfNotExist(src_graph, dst_graph, name, load_in_memory);
   }
 
+  // ExternalDataInfo (wrapped)
+  void ExternalDataInfo__operator_delete(ExternalDataInfo* p) override { delete p; }
+  const PathString& ExternalDataInfo__GetRelPath(const ExternalDataInfo* p) const override {
+    return p->GetRelPath();
+  }
+  OFFSET_TYPE ExternalDataInfo__GetOffset(const ExternalDataInfo* p) const override {
+    return p->GetOffset();
+  }
+  size_t ExternalDataInfo__GetLength(const ExternalDataInfo* p) const override {
+    return p->GetLength();
+  }
+  const std::string& ExternalDataInfo__GetChecksum(const ExternalDataInfo* p) const override {
+    return p->GetChecksum();
+  }
+  Status ExternalDataInfo__Create(const ONNX_NAMESPACE::StringStringEntryProtos& input,
+                                  std::unique_ptr<ExternalDataInfo>& out) override {
+    return ExternalDataInfo::Create(input, out);
+  }
+
   // Initializer (wrapped)
   Initializer* Initializer__constructor(ONNX_NAMESPACE::TensorProto_DataType data_type,
                                         std::string_view name,

onnxruntime/test/shared_lib/test_inference.cc

@@ -4762,9 +4762,9 @@ TEST(CApiTest, custom_cast) {
                                custom_op_domain, nullptr);
 }
 
-TEST(CApiTest, ModelWithMaliciousExternalDataShouldFailToLoad) {
+TEST(CApiTest, ModelWithMaliciousExternalDataInMemoryShouldFailToLoad) {
   // Attempt to create an ORT session with the malicious model
-  // This should fail due to the invalid external data reference
+  // This should fail due to the invalid external in-memory reference
   constexpr const ORTCHAR_T* model_path = TSTR("testdata/test_evil_weights.onnx");
 
   Ort::Env env(ORT_LOGGING_LEVEL_WARNING, "test");
@@ -4785,7 +4785,7 @@ TEST(CApiTest, ModelWithMaliciousExternalDataShouldFailToLoad) {
   }
 
   // Verify that loading the model failed
-  EXPECT_TRUE(exception_thrown) << "Expected model loading to fail due to malicious external data";
+  EXPECT_TRUE(exception_thrown) << "Expected model loading to fail due to malicious in-memory data";
 
   // Verify that the exception message indicates security or external data issues
   EXPECT_TRUE(exception_message.find("in-memory") != std::string::npos ||
@@ -4794,3 +4794,36 @@ TEST(CApiTest, ModelWithMaliciousExternalDataShouldFailToLoad) {
               exception_message.find("model") != std::string::npos)
       << "Exception message should indicate external data or security issue. Got: " << exception_message;
 }
+
+TEST(CApiTest, ModelWithExternalDataOutsideModelDirectoryShouldFailToLoad) {
+  // Attempt to create an ORT session with the malicious model
+  // This should fail due to the external file that is not under model directory structure
+  // i.e. ../../../../etc/passwd
+  constexpr const ORTCHAR_T* model_path = TSTR("testdata/test_arbitrary_external_file.onnx");
+
+  Ort::Env env(ORT_LOGGING_LEVEL_WARNING, "test");
+  Ort::SessionOptions session_options;
+
+  bool exception_thrown = false;
+  std::string exception_message;
+
+  try {
+    // This should throw an exception due to malicious external data
+    Ort::Session session(env, model_path, session_options);
+  } catch (const Ort::Exception& e) {
+    exception_thrown = true;
+    exception_message = e.what();
+  } catch (const std::exception& e) {
+    exception_thrown = true;
+    exception_message = e.what();
+  }
+
+  // Verify that loading the model failed
+  EXPECT_TRUE(exception_thrown) << "Expected model loading to fail due to malicious external data";
+
+  // Verify that the exception message indicates security or external data issues
+  EXPECT_TRUE(exception_message.find("External data path escapes model directory") != std::string::npos ||
+              exception_message.find("invalid") != std::string::npos ||
+              exception_message.find("model") != std::string::npos)
+      << "Exception message should indicate external data or security issue. Got: " << exception_message;
+}

onnxruntime/test/testdata/test_arbitrary_external_file.py

@@ -0,0 +1,54 @@
+import onnx
+from onnx import TensorProto, helper
+
+
+def create_exp_model():
+    inputs = []
+    nodes = []
+    tensors = []
+    outputs = []
+
+    # Create input tensor info
+    input_ = helper.make_tensor_value_info("input", TensorProto.INT64, [None])
+    inputs.append(input_)
+
+    # Create malicious tensor with external data pointing to system file
+    evil_tensor = helper.make_tensor(name="evil_weights", data_type=TensorProto.INT64, dims=[100], vals=[1] * 100)
+    tensors.append(evil_tensor)
+
+    # Set external data location to attempt path traversal attack
+    evil_tensor.data_location = TensorProto.EXTERNAL
+
+    # Location entry - attempts to access system passwd file
+    entry1 = evil_tensor.external_data.add()
+    entry1.key = "location"
+    entry1.value = "../../../../../../../etc/passwd"
+
+    # Offset entry
+    entry2 = evil_tensor.external_data.add()
+    entry2.key = "offset"
+    entry2.value = "0"
+
+    # Length entry
+    entry3 = evil_tensor.external_data.add()
+    entry3.key = "length"
+    entry3.value = "800"
+
+    # Create constant node using the malicious tensor
+    nodes.append(helper.make_node(op_type="Constant", inputs=[], outputs=["output"], value=evil_tensor))
+
+    # Create output tensor info
+    outputs.append(helper.make_tensor_value_info("output", TensorProto.INT64, [100]))
+
+    # Build the graph
+    graph = helper.make_graph(nodes, "test", inputs, outputs, tensors)
+
+    # Create the model
+    model = helper.make_model(graph, opset_imports=[helper.make_opsetid("", 18), helper.make_opsetid("ai.onnx.ml", 3)])
+
+    return model
+
+
+if __name__ == "__main__":
+    model = create_exp_model()
+    onnx.save(model, "test_arbitrary_external_file.onnx")