Stebalien / tempfile

Temporary file library for rust
http://stebalien.com/projects/tempfile-rs
Apache License 2.0
1.2k stars 120 forks source link

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

Open erickguan opened 1 month ago

erickguan commented 1 month ago

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:

Motivation:

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

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 3 weeks ago

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

n0toose commented 1 week ago

Hi, I created a pull request that just draws attention to Builder::permissions and changes some of the wording in addition to https://github.com/Stebalien/tempfile/pull/309: https://github.com/Stebalien/tempfile/pull/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?

erickguan commented 1 week ago

Hi, I created a pull request that just draws attention to Builder::permissions and changes some of the wording in addition to https://github.com/Stebalien/tempfile/pull/309: https://github.com/Stebalien/tempfile/pull/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...