Validate sparse tensor external file paths#28408
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens security around ONNX sparse tensor loading by validating that external_data file locations referenced by sparse tensor values/indices cannot escape the model directory (preventing path traversal).
Changes:
- Add external-data path validation for sparse tensor
valuesandindicesduringSparseTensorProtoToDenseTensorProtoconversion. - Add regression tests that attempt
../traversal via sparse tensor external data locations and expect failure.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| onnxruntime/test/framework/tensorutils_test.cc | Adds regression tests covering path traversal attempts via sparse tensor external data (values and indices). |
| onnxruntime/core/framework/tensorprotoutils.cc | Adds a helper to validate sparse tensor external-data locations and calls it for both values and indices before reading external data. |
Comments suppressed due to low confidence (1)
onnxruntime/core/framework/tensorprotoutils.cc:2092
- In
CopySparseData, theneeds_unpackbranch checksindices.raw_data().size()even whenindicesusesdata_location == EXTERNAL. For typical external-data protosraw_datawill be empty, so this path rejects valid sparse indices stored in an external file. Consider validating the size of the unpacked buffer returned byUnpackInitializerData(or the external_data.length) instead ofraw_data().size(), and apply the same fix to the INT32/INT16/INT8 branches below.
case ONNX_NAMESPACE::TensorProto_DataType_INT64:
if (needs_unpack) {
ORT_RETURN_IF_NOT(indices.raw_data().size() == SafeInt<size_t>(indices_elements) * sizeof(int64_t),
"Sparse tensor: ", name, " indices raw data size does not match expected: ",
indices_elements * sizeof(int64_t));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Please, see #28509 to check how it may affect your changes. |
There was a problem hiding this comment.
NOT Covered (Missing Tests):
- In-memory address attack (/ORT_MEM_ADDR/ location in sparse tensor values/indices) — no test exists for Vector B
- Absolute path in sparse tensor (/etc/passwd or C:\Windows...) — relies on ValidateExternalDataPath's existing absolute-path rejection, but not tested specifically through the sparse tensor path
- No positive test — no test that a legitimate sparse tensor with external data within the model directory succeeds
The CopySparseData function checked indices.raw_data().size() before unpacking, which is always 0 when data_location is EXTERNAL (data is in an external file, not in raw_data). This caused legitimate sparse tensors with external indices to always fail validation. Fix by moving UnpackInitializerData before the size check and validating unpack_buffer.size() instead, matching the pattern already used for sparse values. This works correctly for both inline raw_data and external data sources. Also remove the workaround in the indices path-traversal test that stuffed raw_data to bypass the broken check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…with external data
|
@yuslepukhin thanks for the review. I believe I addressed your comments.
This is valid existing issue. ORT does not currently validate sparse tensors with in-memory external data. This is separate from the reported issue about path validation and requires more careful thought and implementation. It is not clear to me how to properly validate in-memory address for sparse tensors, so I would recommend handling this separately as to not block this fix.
Sure. I added some more unit tests here.
Positive test actually does exist here: However, I added more positive tests now that I also updated this PR to handle the other pre-existing bug related to external indices. |
Description
Adds path traversal validation for sparse tensors with external data, closing a gap where
SparseTensorProtoToDenseTensorProtowould read external files without checking whether the path escapes the model directory.Bug fix (pre-existing)
CopySparseDataindices size check: Theraw_data().size()check was wrong for external data (whereraw_datais empty). Fixed by adding a pre-unpackraw_datasize guard for inline data and a post-unpackunpack_buffersize check for all data sources.Tests
"escapes"error.Known limitation (deferred)
ORT_MEM_ADDR in-memory external data for sparse tensors can trigger arbitrary memory reads. This is a separate issue from path validation —
LoadSparseInitializerOrtFormatlegitimately uses in-memory markers for ORT-format models, so blanket rejection would break functionality. Should be addressed in a separate PR.Motivation and Context
A malicious ONNX model could use
../path traversal in sparse tensor external data locations to read arbitrary files outside the model directory. Dense tensors already had this validation; sparse tensors did not.