Secure tempfiles #742
Merged
Secure tempfiles #742
+61
−27
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shanemcd
approved these changes
Jun 28, 2021
…dtemp() The previous code allowed for an easily exploitable race where the attacker could pre-create a known directory name to gain access to private files created by ansible-runner. Using mkdtemp() closes that hole because mkdtemp() ensures that the directory name returned has been allocated with restrictive permissions by mkdtemp itself so the attacker has no opportunity to inject their own directory. Fixes ansible#738
The previous code allowed a race where an attacker could watch for creation of a rapid creation and deletion of a temporary directory, substitute their own directory at that name, and then have access to ansible-runner's private_data_dir the next time ansible-runner made ues of the private_data_dir. This code fixes the issue by creating the directory securely using mkdtemp() and not deleting it afterwards.
* Most tests just need to be changed to account for a default private_data_dir which doesn't have a predictable name. * test_container_volmount_generation has several tests where mocking of os.path.exists for the _update_volume_mount_paths() function which was being tested interfered with proprer creation of a BaseConfig() object. Moved the mocking to occur after the BaseConfig() instantiation to make those work.
abadger
force-pushed
the
secure-tempfiles
branch
from
766fb5f
to
f1a4f78
Compare
matburt
approved these changes
Jun 29, 2021
shanemcd
approved these changes
Jun 29, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Looking through the code, I found two separate race conditions with tempfile handling which can be exploited. One of those is mentioned in #738. The other one happens because we create a tempdir and then delete it, then later reuse its name. An attacker can pre-create the tempdir in between the deletion and the second creation with the reused name. This PR attempts to fix both.
Fixes #738