Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The onnx runtime allow to load the external_data from the file outside the folder #3991

Closed
jnovikov opened this issue Feb 6, 2022 · 4 comments · Fixed by #4400
Closed

The onnx runtime allow to load the external_data from the file outside the folder #3991

jnovikov opened this issue Feb 6, 2022 · 4 comments · Fixed by #4400
Labels
enhancement Request for new feature or operator vulnerability

Comments

@jnovikov
Copy link
Contributor

jnovikov commented Feb 6, 2022

The external_data field of the tensor proto can have a path to the file which is outside the model current directory or user provided directory, for example "../../../etc/passwd".

There is no validation on this in this function:

std::string data_path = path_join(ctx.get_model_dir(), entry.value());

The python library have the _sanitize_path function which has some basic restrictions but it doesn't work when you use the default onnxruntime package to do the model execution.

I can provide POC and create a patch by request.

@jcwchen jcwchen added the enhancement Request for new feature or operator label Feb 11, 2022
@garymm
Copy link
Contributor

garymm commented Feb 23, 2022

@jnovikov please send a PR to fix this.

@jnovikov
Copy link
Contributor Author

jnovikov commented Aug 2, 2022

@garymm @jcwchen I've sent a PR to fix, pls review.

@gramalingam
Copy link
Contributor

Out of curiosity: are there any pointers to such security practices (documentation)?

@jnovikov
Copy link
Contributor Author

jnovikov commented Aug 2, 2022

@gramalingam do you mean is there any reason to do the patch ?

My main reason is that it should not be allowed by default (from the docs) as it also may affect the security of the some application or user.

About the security practices in patch:
https://owasp.org/www-community/attacks/Path_Traversal

I would say it would be sure better to have some chrooted env while loading the model, but it would be hard to implement in cross-platform way, so this should be enough as long as the clean_relative_path() has no bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Request for new feature or operator vulnerability
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants