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

Mention override_temp_dir and custom permissions on the main crate documentation page #303

Open
erickguan opened this issue Oct 27, 2024 · 8 comments

Comments

@erickguan
Copy link

erickguan commented Oct 27, 2024

I propose updating the front page of the crate documentation to include brief mentions of key features that are currently documented but might not be immediately visible to users. Specifically:

  • Programmatically setting the temporary directory using tempfile::env::override_temp_dir
  • Setting custom permissions for temporary files and directories using the Builder pattern

Motivation:

While these features are already detailed elsewhere in the documentation, mentioning them upfront would:

  • Improve visibility: New users can quickly become aware of these capabilities.
  • Promote best practices: Encourages developers to consider security and control from the start.
  • Enhance usability: Makes the crate’s features more accessible and easier to discover.

Draft of a new section or as new examples:

Set custom permissions with the Builder pattern

Create temporary files or directories with custom permissions to enhance security.

use tempfile::Builder;
use std::fs::Permissions;
use std::os::unix::fs::PermissionsExt;

// Create a temporary directory with 0o700 permissions
let temp_dir = Builder::new()
    .permissions(Permissions::from_mode(0o700))
    .tempdir()?;

// The directory is now only accessible by the owner

Programmatically set the temporary directory
You can override the default temporary directory within your application without relying solely on environment variables.

use tempfile::env;
use std::path::PathBuf;

// Override the temporary directory. read more in `env`.
env::override_temp_dir(PathBuf::from("/custom/temp/dir"));

// Now, any temporary files or directories will be created in "/custom/temp/dir"
let temp_file = tempfile::NamedTempFile::new()?;

// explicitly check if we deleted the file successfully. read more in `NamedTempFile`.
temp_file.close()?;
@Stebalien
Copy link
Owner

These are two different things:

  1. mkdtemp just creates a temporary directory with a mask of 0o700, you can currently do that with Builder::new().permissions(Permissions::from_mode(0o700)).tempdir().
  2. In terms of picking a secure directory, tempfile does what you tell it to do. Ideally you'll have configured TMPDIR correctly but, if not, it's possible for an application to override it with tempfile::env::override_temp_dir. E.g., an application can create a temporary directory then set the application-wide temporary directory with 0o700 permissions to be that new temporary directory.

@erickguan
Copy link
Author

Thanks so much for the quick reply!

  1. You make an excellent point about the Builder and how it already allows setting a secure mask with Permissions::from_mode(0o700). I hadn't considered that. I agree with you.

  2. On the topic of setting a secure default directory, I see TMPDIR as an approach and overridden using tempfile::env::override_temp_dir is an approach too. I'm wondering an improvement on a tempfile API. When use the API, created temp directory doesn't rely on environment variables. This would be similar to Python's approach, where users can set tempfile.tempdir directly. Two advantages:

    • providing more control
    • reducing potential security risks for developers who might overlook configuring environment variables correctly.

As an example, I am thinking an addition behavior similar to tempfile.tempdir. When set manually, tempfile creates temporary directory at given locations:

import tempfile
import os

print(tempfile.tempdir)
# None

print(tempfile.mkdtemp())
# /home/erickg/Dev/tmp

tempfile.tempdir = '/home/erickg/Dev/tmp'
print(tempfile.tempdir)
# /home/erickg/Dev/tmp

print(tempfile.mkdtemp())
# /home/erickg/Dev/tmp/tmp6wwa41b7

tempfile.tempdir = None
os.environ["TMPDIR"] = "/home/erickg/Dev/container"
print(tempfile.mkdtemp())
# /home/erickg/Dev/container/tmpye6ruw3

I will update my original post to make it more accurate.

@Stebalien
Copy link
Owner

That's what tempfile::env::override_temp_dir does, although it should only be used when the user hasn't already specified a sane default (e.g., check if the temporary directory is world accessible and, if it is, override it).

@erickguan
Copy link
Author

erickguan commented Oct 28, 2024

Right, sorry for not understanding words.

it should only be used when the user hasn't already specified a sane default (e.g., check if the temporary directory is world accessible and, if it is, override it).

Of course, this is a user process's responsibility.

For better visibility of these features, can I add a section in the crate documentation? e.g., mentioning a few "advanced" use cases with examples such as:

  1. TMPDIR and tempfile::env::override_temp_dir
  2. a few builder patterns.

They should be a few sentences but linked to the detailed documentation.

@erickguan erickguan changed the title Allow use a safer folder other than /tmp on unix-like system Mention override_temp_dir and custom permissions on the main crate documentation page Oct 29, 2024
@erickguan
Copy link
Author

Thanks for all your answers. I have updated the issue title and description. Tell me if it's a good idea. Otherwise, I am happy for the answers to close the issue.

@Stebalien
Copy link
Owner

If you can think of a good, succinct way to document it, sure.

n0toose added a commit to n0toose/tempfile-crate that referenced this issue Nov 17, 2024
@n0toose
Copy link
Contributor

n0toose commented Nov 17, 2024

Hi, I created a pull request that just draws attention to Builder::permissions and changes some of the wording in addition to #309: #311

The OWASP Foundation's resource mentions potential attack vectors, such as the attacker being able to predict the name of a temporary file.

On Unix based systems an even more insidious attack is possible if the attacker pre-creates the file as a link to another important file. Then, if the application truncates or writes data to the file, it may unwittingly perform damaging operations for the attacker. This is an especially serious threat if the program operates with elevated permissions.

However, if an attacker is able to accurately predict a sequence of temporary file names, then the application may be prevented from opening necessary temporary storage causing a denial of service (DoS) attack.

This may be a particular problem if a file is closed and reused at a later point - despite the documentation recommending against so. In my use case, I somewhat got away with it by setting the tempdir's permissions to 700 (Unix-like only) and "assuming that the user specifically is not compromised". How is the name randomized? Is there anything that the documentation could recommend to mitigate this attack vector?

n0toose added a commit to n0toose/tempfile-crate that referenced this issue Nov 17, 2024
Minor wording fixes. Increases visibility of `Builder::permissions`,
the `NamedTempFile` Security documentation and
`env::override_temp_dir`.

Partially addresses Stebalien#303.
n0toose added a commit to n0toose/tempfile-crate that referenced this issue Nov 17, 2024
Minor wording fixes. Increases visibility of `Builder::permissions`,
the `NamedTempFile` Security documentation and
`env::override_temp_dir`.

Partially addresses Stebalien#303.
@erickguan
Copy link
Author

Hi, I created a pull request that just draws attention to Builder::permissions and changes some of the wording in addition to #309: #311

Nice! I'm happy to build on top of your PRs.

This may be a particular problem if a file is closed and reused at a later point - despite the documentation recommending against so. How is the name randomized? Is there anything that the documentation could recommend to mitigate this attack vector?

Check out Builder::prefix and Builder::suffix. You can use these to build a random string for the directory or the filename.

In my use case, I somewhat got away with it by setting the tempdir's permissions to 700 (Unix-like only) and "assuming that the user specifically is not compromised".

If an attacker gains user or root privilege, tempfile alone isn't very effective at mitigation. Technically, I could purge /tmp every 100ms to disturb other processes. In reality, though, common practices work reasonably well. They don't add too much compute overhead or make debugging difficult. e.g., a randomized tempfile works okay in /tmp. Furthermore, macOS has per-user /tmp helps this further. Developers can choose to have a per-application temp file directories, etc...

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

No branches or pull requests

3 participants