Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 38 additions & 21 deletions onnxruntime/core/framework/tensorprotoutils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1565,13 +1565,35 @@ static Status GetFileContent(const Env& env, const std::filesystem::path& file_p
}
#endif

// Backstop validation for callers that load external data outside Graph::Resolve (e.g. training
// checkpoints, custom-op initializers). Passes through ORT's in-memory address markers — those are
// validated at higher layers (Graph::ConvertInitializersIntoOrtValues for dense; markers on sparse
// sub-tensors are rejected outright in SparseTensorProtoToDenseTensorProto). For declared file paths,
// defers to ValidateExternalDataPath, which rejects absolute paths and paths that escape the model
// directory. Callers must have already verified the tensor has external data.
static Status ValidateExternalFilePathForTensor(const ONNX_NAMESPACE::TensorProto& tensor_proto,
const std::filesystem::path& model_path) {
if (HasExternalDataInMemory(tensor_proto)) {
return Status::OK();
}

std::unique_ptr<ExternalDataInfo> external_data_info;
ORT_RETURN_IF_ERROR(ExternalDataInfo::Create(tensor_proto.external_data(), external_data_info));
return utils::ValidateExternalDataPath(model_path, external_data_info->GetRelPath());
}

Status GetExtDataFromTensorProto(const Env& env,
const std::filesystem::path& model_path,
const ONNX_NAMESPACE::TensorProto& tensor_proto,
OrtValue& ort_value, PrepackedWeightsForGraph* prepacked_info) {
ORT_ENFORCE(HasExternalData(tensor_proto), "TensorProto for: ",
tensor_proto.name(), "Expected to have external data");

// Defense-in-depth: reject absolute or directory-escaping external data paths even when this
// function is reached outside Graph::Resolve (e.g. training checkpoint load, custom-op init).
// In-memory address markers are passed through; their validity is enforced upstream.
ORT_RETURN_IF_ERROR(ValidateExternalFilePathForTensor(tensor_proto, model_path));

std::basic_string<ORTCHAR_T> tensor_proto_dir;
if (!model_path.empty()) {
ORT_RETURN_IF_ERROR(GetDirNameFromFilePath(model_path, tensor_proto_dir));
Expand Down Expand Up @@ -1735,6 +1757,9 @@ Status LoadExtDataToTensorFromTensorProto(const Env& env, const std::filesystem:
const IExternalDataLoader& ext_data_loader,
Tensor& tensor) {
ORT_ENFORCE(HasExternalData(tensor_proto));
// Defense-in-depth path validation for callers reaching this function outside Graph::Resolve.
// In-memory markers are passed through; rejected explicitly below as unsupported for this path.
ORT_RETURN_IF_ERROR(ValidateExternalFilePathForTensor(tensor_proto, model_path));
std::basic_string<ORTCHAR_T> tensor_proto_dir;
if (!model_path.empty()) {
ORT_RETURN_IF_ERROR(GetDirNameFromFilePath(model_path, tensor_proto_dir));
Expand Down Expand Up @@ -2098,30 +2123,22 @@ void MakeCpuTensorCopy(const Tensor& src_tensor, Tensor& dst_tensor) {

#if !defined(DISABLE_SPARSE_TENSORS)

// Validates that a TensorProto's external data path does not escape the model directory.
// Also validates that the file exists when filesystem access is available (skipped on WASM without a virtual FS).
// Returns Status::OK() (no-op) for tensors that do not use file-based external data.
static Status ValidateExternalDataPathForTensor(const ONNX_NAMESPACE::TensorProto& tensor_proto,
const std::filesystem::path& model_path) {
// Gates on data_location == EXTERNAL directly instead of using HasExternalData()/HasExternalDataInFile(),
// which also require data_type != UNDEFINED. That check is appropriate for data processing (can't unpack
// without a type), but too narrow for security validation: we must validate any declared external path
// regardless of data_type.
if (tensor_proto.data_location() != ONNX_NAMESPACE::TensorProto_DataLocation_EXTERNAL) {
// Validates the external data declaration on a sub-tensor of a SparseTensorProto (values or indices).
// Validates that any file path stays within the model directory. In-memory address markers are
// passed through here — they are an ORT-internal mechanism that legitimately appears on sparse
// sub-tensors loaded from a trusted ORT-format flatbuffer (where the marker points into the trusted
// mmap'd / loaded buffer). Rejection of untrusted markers on sparse sub-tensors coming from a raw
// ONNX protobuf is enforced at the protobuf entry point (Graph ctor) before this function runs.
// Returns Status::OK() (no-op) for sub-tensors that do not use external data.
static Status ValidateSparseSubTensorExternalDataPath(const ONNX_NAMESPACE::TensorProto& tensor_proto,
const std::filesystem::path& model_path) {
Comment thread
yuslepukhin marked this conversation as resolved.
if (!HasExternalData(tensor_proto) || HasExternalDataInMemory(tensor_proto)) {
return Status::OK();
}

std::unique_ptr<ExternalDataInfo> external_data_info;
ORT_RETURN_IF_ERROR(ExternalDataInfo::Create(tensor_proto.external_data(), external_data_info));
const auto& rel_path = external_data_info->GetRelPath();

// In-memory external data uses special marker locations — skip file path validation for those.
if (rel_path == kTensorProtoLittleEndianMemoryAddressTag ||
rel_path == kTensorProtoNativeEndianMemoryAddressTag) {
return Status::OK();
}

return utils::ValidateExternalDataPath(model_path, rel_path);
return utils::ValidateExternalDataPath(model_path, external_data_info->GetRelPath());
}

static Status CopySparseData(const std::string& name,
Expand Down Expand Up @@ -2371,8 +2388,8 @@ common::Status SparseTensorProtoToDenseTensorProto(const ONNX_NAMESPACE::SparseT
// Validate external data paths before any early returns or allocations.
// This ensures malicious paths are rejected even for zero-element tensors,
// and prevents large allocations before an invalid path is caught.
ORT_RETURN_IF_ERROR(ValidateExternalDataPathForTensor(sparse_values, model_path));
ORT_RETURN_IF_ERROR(ValidateExternalDataPathForTensor(indices, model_path));
ORT_RETURN_IF_ERROR(ValidateSparseSubTensorExternalDataPath(sparse_values, model_path));
ORT_RETURN_IF_ERROR(ValidateSparseSubTensorExternalDataPath(indices, model_path));

if (dense_elements == 0) {
// if there are no elements in the dense tensor, we can return early with an empty tensor proto
Expand Down
33 changes: 33 additions & 0 deletions onnxruntime/core/graph/graph.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1265,6 +1265,21 @@ Graph::Graph(const Model& owning_model,
continue;
}

#if !defined(DISABLE_SPARSE_TENSORS)
// Reject ORT in-memory address markers on a sparse-tensor Constant attribute before the
// sparse-to-dense conversion runs — those markers are an in-process ORT sentinel and must
// never appear in a deserialized protobuf. See note on the dense initializer loop below.
if (node.attribute_size() > 0 &&
node.attribute(0).type() == AttributeProto_AttributeType_SPARSE_TENSOR) {
const auto& s = node.attribute(0).sparse_tensor();
ORT_ENFORCE(!utils::HasExternalDataInMemory(s.values()) &&
!utils::HasExternalDataInMemory(s.indices()),
"Constant node '", node.name(),
"' sparse-tensor attribute references an ORT in-memory address marker, "
"which is not allowed in a model protobuf.");
}
#endif

const gsl::not_null<TensorProto*> tensor{graph_proto_->add_initializer()};
ORT_THROW_IF_ERROR(utils::ConstantNodeProtoToTensorProto(node, model_path, *tensor));

Expand Down Expand Up @@ -1304,6 +1319,16 @@ Graph::Graph(const Model& owning_model,
if (graph_proto_->sparse_initializer_size() > 0) {
for (const auto& sparse_tensor : graph_proto_->sparse_initializer()) {
ORT_ENFORCE(utils::HasName(sparse_tensor), "Sparse initializer must have a name. This model is invalid");
// Reject ORT's in-memory address markers on sparse sub-tensors arriving via the protobuf
// path. Such markers are an internal ORT optimization set by trusted loaders (e.g. ORT-format
// flatbuffer load) and must never appear in a SparseTensorProto deserialized from an .onnx
// protobuf; if they do, the model is crafted and would cause ORT to dereference an
// attacker-supplied pointer during sparse-to-dense conversion.
for (const auto* sub : {&sparse_tensor.values(), &sparse_tensor.indices()}) {
ORT_ENFORCE(!utils::HasExternalDataInMemory(*sub),
"Sparse initializer '", sparse_tensor.values().name(),
"' references an ORT in-memory address marker, which is not allowed in a model protobuf.");
}
const gsl::not_null<TensorProto*> tensor{graph_proto_->add_initializer()};
auto status = utils::SparseTensorProtoToDenseTensorProto(sparse_tensor, model_path, *tensor);
ORT_ENFORCE(status.IsOK(), status.ToString());
Expand Down Expand Up @@ -1345,6 +1370,14 @@ Graph::Graph(const Model& owning_model,

// Copy initial tensors to a map.
for (auto& tensor : graph_proto_->initializer()) {
// ORT in-memory address markers are an in-process sentinel: they can only be planted by ORT
// itself (e.g. when constructing a TensorProto that aliases an mmap'd .ort buffer or an OrtValue).
// They must never appear in a TensorProto deserialized from an .onnx protobuf — if they do, the
// model is crafted and would cause ORT to dereference an attacker-supplied pointer when
// resolving the initializer.
ORT_ENFORCE(!utils::HasExternalDataInMemory(tensor),
"Initializer '", tensor.name(),
"' references an ORT in-memory address marker, which is not allowed in a model protobuf.");
auto p = name_to_initial_tensor_.emplace(tensor.name(), &tensor);
if (!p.second) {
LOGS(logger_, WARNING) << "Duplicate initializer (dense, sparse or ConstantNode): '" << tensor.name()
Expand Down
62 changes: 62 additions & 0 deletions onnxruntime/test/framework/tensorutils_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1153,6 +1153,68 @@ TEST_F(PathValidationTest, SparseTensorExternalDataPathTraversalBlocked_ZeroNNZ)

#endif // !defined(DISABLE_SPARSE_TENSORS)

// Defense-in-depth: GetExtDataFromTensorProto must reject absolute external paths even when
// called with an empty model_path (e.g. from training checkpoint or custom-op init paths).
// Previously, ValidateExternalDataPath was only invoked from Graph::ConvertInitializersIntoOrtValues,
// so direct callers of GetExtDataFromTensorProto could load arbitrary files.
TEST(GetExtDataFromTensorProtoTest, RejectsAbsoluteExternalPathWithEmptyModelPath) {
ONNX_NAMESPACE::TensorProto tensor_proto;
tensor_proto.set_name("abs_external");
tensor_proto.set_data_type(ONNX_NAMESPACE::TensorProto_DataType_FLOAT);
tensor_proto.add_dims(2);
tensor_proto.set_data_location(ONNX_NAMESPACE::TensorProto_DataLocation_EXTERNAL);

auto* loc = tensor_proto.add_external_data();
loc->set_key("location");
#ifdef _WIN32
loc->set_value("C:\\data.bin");
#else
loc->set_value("/etc/passwd");
#endif

auto* off = tensor_proto.add_external_data();
off->set_key("offset");
off->set_value("0");

auto* len = tensor_proto.add_external_data();
len->set_key("length");
len->set_value(std::to_string(2 * sizeof(float)));

OrtValue value;
Status status = utils::GetExtDataFromTensorProto(Env::Default(), {}, tensor_proto, value);
ASSERT_FALSE(status.IsOK()) << "Absolute external path must be rejected even with empty model_path.";
EXPECT_THAT(status.ErrorMessage(), ::testing::HasSubstr("Absolute path not allowed"));
}

// Defense-in-depth: GetExtDataFromTensorProto must reject directory-escaping external paths even
// when the caller passes a non-empty model_path. This guards callers outside Graph::Resolve.
TEST(GetExtDataFromTensorProtoTest, RejectsEscapingExternalPath) {
ONNX_NAMESPACE::TensorProto tensor_proto;
tensor_proto.set_name("escape_external");
tensor_proto.set_data_type(ONNX_NAMESPACE::TensorProto_DataType_FLOAT);
tensor_proto.add_dims(2);
tensor_proto.set_data_location(ONNX_NAMESPACE::TensorProto_DataLocation_EXTERNAL);

auto* loc = tensor_proto.add_external_data();
loc->set_key("location");
loc->set_value("../escape.bin");

auto* off = tensor_proto.add_external_data();
off->set_key("offset");
off->set_value("0");

auto* len = tensor_proto.add_external_data();
len->set_key("length");
len->set_value(std::to_string(2 * sizeof(float)));

OrtValue value;
// Pass a synthetic model_path so the validator has a model directory to compare against.
std::filesystem::path model_path = std::filesystem::temp_directory_path() / "sub" / "model.onnx";
Status status = utils::GetExtDataFromTensorProto(Env::Default(), model_path, tensor_proto, value);
ASSERT_FALSE(status.IsOK()) << "Directory-escaping external path must be rejected.";
EXPECT_THAT(status.ErrorMessage(), ::testing::HasSubstr("escapes"));
}

TEST(TensorProtoUtilsTest, GetNodeProtoLayeringAnnotation) {
// Case 1: Annotation exists
{
Expand Down
118 changes: 118 additions & 0 deletions onnxruntime/test/ir/graph_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1362,8 +1362,126 @@ TEST_F(GraphTest, UnusedSparseInitializerIsIgnored) {
auto& graph_proto = graph2.ToGraphProto();
ASSERT_TRUE(graph_proto.sparse_initializer().empty());
}

// Regression test for issue #28617: a SparseTensorProto loaded from a model protobuf must not
// be allowed to carry an ORT in-memory address marker on its values or indices sub-tensors.
// Those markers are an ORT-internal mechanism for trusted in-memory buffers (.ort flatbuffer
// load). Accepting them on a crafted .onnx protobuf would let the model make ORT dereference
// an attacker-supplied pointer during sparse-to-dense conversion.
static void RunRejectInMemoryMarkerOnSparseInitializerTest(bool marker_on_indices,
const onnxruntime::logging::Logger& logger) {
Model model("RejectInMemoryMarkerOnSparseInitializer", false, logger);
auto model_proto = model.ToProto();
auto* m_graph = model_proto.mutable_graph();
ConstructASimpleAddGraph(*m_graph, nullptr);

auto* m_sparse_initializer = m_graph->add_sparse_initializer();
ConstructSparseTensor("in_memory_marker_sparse", *m_sparse_initializer);

// Overwrite either values or indices to declare external data pointing at an in-memory marker.
// Allocate a real backing buffer so even an accidental dereference of "offset" stays in-process.
static std::vector<uint8_t> backing(64, 0);
auto* sub = marker_on_indices ? m_sparse_initializer->mutable_indices()
: m_sparse_initializer->mutable_values();
sub->clear_raw_data();
sub->clear_int64_data();
sub->clear_float_data();
sub->set_data_location(ONNX_NAMESPACE::TensorProto_DataLocation_EXTERNAL);
auto* loc = sub->add_external_data();
loc->set_key("location");
loc->set_value(ToUTF8String(onnxruntime::utils::kTensorProtoLittleEndianMemoryAddressTag));
auto* off = sub->add_external_data();
off->set_key("offset");
off->set_value(std::to_string(reinterpret_cast<intptr_t>(backing.data())));
auto* len = sub->add_external_data();
len->set_key("length");
len->set_value(std::to_string(backing.size()));

std::string s1;
model_proto.SerializeToString(&s1);

ModelProto model_proto_1;
ASSERT_TRUE(model_proto_1.ParseFromString(s1));

std::shared_ptr<onnxruntime::Model> p_tmp_model;
// The Graph ctor must reject the marker — Model::Load is expected to return a non-OK status
// (Graph ctor's ORT_THROW is caught at the C++/Status boundary).
ORT_TRY {
auto status = onnxruntime::Model::Load(model_proto_1, p_tmp_model, nullptr, logger);
EXPECT_FALSE(status.IsOK()) << "Loading a model with an in-memory marker on a sparse "
<< (marker_on_indices ? "indices" : "values")
<< " sub-tensor must fail.";
if (!status.IsOK()) {
EXPECT_THAT(status.ErrorMessage(),
::testing::HasSubstr("in-memory address marker"));
}
}
ORT_CATCH(const std::exception& ex) {
ORT_HANDLE_EXCEPTION([&]() {
EXPECT_THAT(std::string(ex.what()),
::testing::HasSubstr("in-memory address marker"));
});
}
}

TEST_F(GraphTest, RejectInMemoryMarkerOnSparseInitializerValues) {
RunRejectInMemoryMarkerOnSparseInitializerTest(/*marker_on_indices=*/false, *logger_);
}

TEST_F(GraphTest, RejectInMemoryMarkerOnSparseInitializerIndices) {
RunRejectInMemoryMarkerOnSparseInitializerTest(/*marker_on_indices=*/true, *logger_);
}
#endif // !defined(DISABLE_SPARSE_TENSORS)

// Regression test: ORT in-memory address markers are an in-process sentinel only; they must never
// appear in a dense initializer deserialized from an .onnx protobuf. The Graph ctor must reject
// such a model.
TEST_F(GraphTest, RejectInMemoryMarkerOnDenseInitializer) {
Model model("RejectInMemoryMarkerOnDenseInitializer", false, *logger_);
auto model_proto = model.ToProto();
auto* m_graph = model_proto.mutable_graph();
ConstructASimpleAddGraph(*m_graph, nullptr);

static std::vector<uint8_t> backing(64, 0);

auto* init = m_graph->add_initializer();
init->set_name("in_memory_marker_dense");
init->set_data_type(ONNX_NAMESPACE::TensorProto_DataType_FLOAT);
init->add_dims(static_cast<int64_t>(backing.size() / sizeof(float)));
init->set_data_location(ONNX_NAMESPACE::TensorProto_DataLocation_EXTERNAL);
auto* loc = init->add_external_data();
loc->set_key("location");
loc->set_value(ToUTF8String(onnxruntime::utils::kTensorProtoLittleEndianMemoryAddressTag));
auto* off = init->add_external_data();
off->set_key("offset");
off->set_value(std::to_string(reinterpret_cast<intptr_t>(backing.data())));
auto* len = init->add_external_data();
len->set_key("length");
len->set_value(std::to_string(backing.size()));

std::string s1;
model_proto.SerializeToString(&s1);

ModelProto model_proto_1;
ASSERT_TRUE(model_proto_1.ParseFromString(s1));

std::shared_ptr<onnxruntime::Model> p_tmp_model;
ORT_TRY {
auto status = onnxruntime::Model::Load(model_proto_1, p_tmp_model, nullptr, *logger_);
EXPECT_FALSE(status.IsOK()) << "Loading a model with an in-memory marker on a dense initializer must fail.";
if (!status.IsOK()) {
EXPECT_THAT(status.ErrorMessage(),
::testing::HasSubstr("in-memory address marker"));
}
}
ORT_CATCH(const std::exception& ex) {
ORT_HANDLE_EXCEPTION([&]() {
EXPECT_THAT(std::string(ex.what()),
::testing::HasSubstr("in-memory address marker"));
});
}
}

TEST_F(GraphTest, GraphConstruction_CheckIsNotAcyclic) {
// A cyclic graph
// SouceNode
Expand Down
5 changes: 4 additions & 1 deletion onnxruntime/test/optimizer/initializer_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,10 @@ TEST(OptimizerInitializerTest, LoadExternalData) {

// bad model paths
EXPECT_THROW(Initializer i(tensor_proto_base, std::filesystem::path()), OnnxRuntimeException);
EXPECT_THROW(Initializer i(tensor_proto_base, ORT_TSTR("invalid/directory")), std::filesystem::filesystem_error);
// ValidateExternalDataPath in GetExtDataFromTensorProto now rejects this earlier with an
// ORT error ("External data path does not exist") instead of letting a downstream
// std::filesystem call throw filesystem_error.
EXPECT_THROW(Initializer i(tensor_proto_base, ORT_TSTR("invalid/directory")), OnnxRuntimeException);

// bad length
{
Expand Down
Loading