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

Safely create temporary files and directories #3378

Merged
merged 2 commits into from
Nov 28, 2015
Merged

Safely create temporary files and directories #3378

merged 2 commits into from
Nov 28, 2015

Conversation

eelstork
Copy link
Contributor

With reference to #3324 I propose a modified implementation of MakeTempDir.
@futurely and @netheril96 suggested that the designated issue may be caused by a race condition.

Upon closer inspection, I find that:

  • Boost unique_path is specified to return a randomly generated path, which may or may not be unique. Boost implementations typically system level entropy sources to generate the path.
  • Typical mkdtemp implementations rely on mkdir to generate temporary directories. Race conditions are avoided by checking the return value of mkdir instead of checking whether a generated path already exists.
  • Paths generated using Boost use 4 bits of randomness per character. By comparison typical mkdtemp implementations may use 5 or 6 bits of randomness.

With this in mind the proposed solution is similar to how Python implements mkdtemp (tempfile.py, line 322).

@ronghanghu ronghanghu self-assigned this Nov 24, 2015
@ronghanghu
Copy link
Member

Thanks @eelstork ! This looks good

@eelstork eelstork changed the title Secure implementation of MakeTempDir Safely create temporary files and directories Nov 25, 2015
@eelstork
Copy link
Contributor Author

Adding a commit targeting safe creation of temporary files. In this case the approach used to create temporary directories cannot be reused:

  • mkstemp and open are not cross platform.
  • fopen provides an x flag similar to O_EXCL however this requires C11.
  • I do not know of an equivalent using standard C++ or Boost.

The proposed solution consists in (safely) creating a temporary directory. This temporary directory is then used as a container for all temporary files.
A similar approach is described in Apple's secure coding guide (thanks to @futurely for suggesting this reference).
In our case temp files are labeled using a counter, eliminating the need to test for homonyms.

Optionally, this approach may be extended to the creation of temporary directories (meaning: safely create a temp directory, then use it as a container for all temp files and directories).

@eelstork
Copy link
Contributor Author

@ronghanghu this needs a little more work on my end, not compiling in VS.

@ronghanghu
Copy link
Member

@eelstork Thank you and don't worry. I'll take a look when it's ready :)

@eelstork
Copy link
Contributor Author

@ronghanghu ready for review.

@ronghanghu
Copy link
Member

@eelstork Thanks! I'll try to take a look tomorrow

*temp_filename = boost::filesystem::unique_path(model).string();
}
static path temp_files_subpath;
static uint64_t next_temp_file = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to move this two static variables into function MakeTempFilename (while keeping them static) so that they are only accessible in that function.

@ronghanghu
Copy link
Member

@eelstork This should be ready. Let's fix the minor issue mentioned above and I'll merge.

@eelstork
Copy link
Contributor Author

Updated; looks good to me.

On 28 Nov 2015, at 10:21, Ronghang Hu notifications@github.com wrote:

@eelstork This should be ready. Let's fix the minor issue mentioned above and I'll merge.


Reply to this email directly or view it on GitHub.

ronghanghu added a commit that referenced this pull request Nov 28, 2015
Safely create temporary files and directories
@ronghanghu ronghanghu merged commit 62ed0d2 into BVLC:master Nov 28, 2015
@ronghanghu
Copy link
Member

Thanks @eelstork !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants