Skip to content

Fix arbitrary memory read - #29011

Merged
apsonawane merged 4 commits into
mainfrom
asonawane/icm
Jun 29, 2026
Merged

Fix arbitrary memory read#29011
apsonawane merged 4 commits into
mainfrom
asonawane/icm

Conversation

@apsonawane

Copy link
Copy Markdown
Contributor

This pull request strengthens security around loading ONNX models by adding a defense-in-depth check that rejects Constant nodes with dense tensor attributes referencing internal ORT in-memory address markers. It also introduces a regression test to ensure this attack vector is blocked.

Security hardening:

  • Added an explicit check in ConstantNodeProtoToTensorProto to reject Constant node tensor attributes with ORT in-memory address markers, preventing crafted models from propagating unsafe pointers.

Testing:

  • Added a regression test RejectInMemoryMarkerOnConstantNodeTensorAttribute to verify that models containing Constant nodes with such in-memory markers are rejected during model load.

@apsonawane
apsonawane enabled auto-merge (squash) June 11, 2026 19:14
Comment thread onnxruntime/core/framework/tensorprotoutils.cc

@tianleiwu 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.

Thanks for the fix — the guard is correct and a useful defense-in-depth addition. Confirming it adds real coverage: ConstantNodeProtoToTensorProto is also reached by Graph::AddConstantProtoAsInitializer and the graph-API/ORT-format build path, both of which emplace directly into name_to_initial_tensor_ and bypass the dense-initializer marker check in the Graph constructor, so the new guard closes those paths.

One suggestion on the test (inline) — it currently doesn't isolate the new check, since the existing Graph-ctor initializer guard would reject the same model with the same message.

Comment thread onnxruntime/test/ir/graph_test.cc

@tianleiwu 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.

Re-reviewed at head 03876ad. The defense-in-depth guard in ConstantNodeProtoToTensorProto is correct and placed at the right chokepoint — it rejects the ORT in-memory address marker before tensor = constant_attribute.t(), blocking the crafted-pointer arbitrary-read while leaving legitimate file-backed external data untouched.

Coverage looks complete: the dense TENSOR attribute is guarded here, and the SPARSE_TENSOR path is independently guarded in SparseTensorProtoToDenseTensorProto. Enforcing in this function is the right call since bypassing paths (AddConstantProtoAsInitializer, ORT-format load) emplace directly into name_to_initial_tensor_ and skip the constructor-side check.

Both earlier concerns are addressed: the marker-only scope was clarified, and the new ConstantNodeProtoToTensorProtoMarkerTest.RejectsInMemoryMarkerOnDenseTensorAttribute exercises the guard in isolation rather than relying on the pre-existing constructor guard, while the Model::Load test remains as an end-to-end regression. LGTM.

@apsonawane
apsonawane merged commit f285b9e into main Jun 29, 2026
86 checks passed
@apsonawane
apsonawane deleted the asonawane/icm branch June 29, 2026 21:34
This was referenced Jul 25, 2026
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