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

Add documentation warning about pickled data #73

Closed
Cnoor0171 opened this issue Feb 19, 2022 · 4 comments · Fixed by #82
Closed

Add documentation warning about pickled data #73

Cnoor0171 opened this issue Feb 19, 2022 · 4 comments · Fixed by #82

Comments

@Cnoor0171
Copy link
Contributor

Cnoor0171 commented Feb 19, 2022

TAP offers this great feature about saving and loading all arguments, along with reproducibility info into a json file. Complex python types are encoded and automatically decoded using the pickle module. From the docs,

Note: More complex types will be encoded in JSON as a pickle string.

However, the unpickling of untrusted data is a big security risk, since it is possible to construct malicious pickle data which will execute arbitrary code during unpickling. Novice python programmers might not know about the implications of using pickle, or people might assume only json parsing is taking place, and simply not notice the note about pickle.

I think it would be beneficial to include a prominent warning for users not to run args.load('args.json') on untrusted json files. The warning could either share a short explanation of why its a security risk, or just link to the pickle documentation.

@Cnoor0171
Copy link
Contributor Author

Alternatively, the load method could refuse to unpickle data by default unless a keyword argument data_is_trusted is set to true. This would make the warning a lot easier to notice. But it would be backwards incompatible, so perhaps that's overkill.

@martinjm97
Copy link
Collaborator

Hi @Cnoor0171,

Thank you for identifying this security risk. Would you be satisfied by a change to the readme? If so, we're happy to look over a PR and we'll try to get to it soon.

Thanks again,
Kyle and Jesse

@Cnoor0171
Copy link
Contributor Author

@martinjm97 I think a warning in the readme would suffice. Just something eye catching since the note about pickling kind of easy to miss and requires the user to already know the security risk about unpickling data. Something like this at the top of the readme section for load:

⚠️
Never call args.load('args.json') on untrusted files. Argument loading uses the pickle module to decode complex types automatically. Unpickling of untrusted data is a security risk and can lead to arbitrary code execution. See the warning in the pickle docs
⚠️

Feel free to merge #82 if that sounds fine.

@martinjm97
Copy link
Collaborator

Thank you for keeping a watchful eye out wrt security. Much appreciated!

--Jesse

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 a pull request may close this issue.

2 participants