TOCTOU race condition in atomic file write and permission handling
Description
The commit refactors File.atomic_write to reduce TOCTOU/race-prone behavior and preserve permissions when writing a file atomically. The previous approach relied on a Tempfile-based path and a prior permission probing mechanism. The new code:
- Uses a securely named temporary file with a random hex suffix (SecureRandom.hex) in the destination directory, avoiding deterministic names that could be guessed by an attacker.
- Creates the temp file with exclusive creation (EXCL) to prevent races where a file could be created between checks and use of a non-unique name.
- Captures the original file's permissions (old_stat) beforehand and applies them to the new file, ensuring the destination ends up with the intended permissions.
- Cleans up on error (closes and unlinks the temp file) to prevent leaks.
- Removes the previous probe-based permission check (which created a temporary file in the target directory to infer default permissions), reducing side effects and potential information disclosure.
This addresses race conditions in atomic writes by ensuring the temporary file is securely created, with proper cleanup and permission handling, and by avoiding a race-prone path that could leak information or grant unintended permissions.
Commit Details
Author: Jean Boussier
Date: 2025-12-15 16:19 UTC
Message:
Merge pull request #56368 from byroot/atomic-file-exclusive
Refactor `File.atomic_write`
Triage Assessment
Vulnerability Type: Race condition / Information disclosure (via improper atomic file write and permissions handling)
Confidence: MEDIUM
Reasoning:
The patch refactors File.atomic_write to use a securely named temporary file (SecureRandom hex suffix) and to handle errors with proper cleanup, reducing TOCTOU/race-prone behavior and ensuring permission handling is preserved without leaking or inheriting unintended permissions. This addresses potential race conditions and permission) related issues in atomic writes.
Verification Assessment
Vulnerability Type: TOCTOU race condition in atomic file write and permission handling
Confidence: MEDIUM
Affected Versions: 8.1.x prior to 8.1.3 (e.g., 8.1.0 - 8.1.2)
Code Diff
diff --git a/activesupport/lib/active_support/core_ext/file/atomic.rb b/activesupport/lib/active_support/core_ext/file/atomic.rb
index b442ea3100feb..f85f3e91c7042 100644
--- a/activesupport/lib/active_support/core_ext/file/atomic.rb
+++ b/activesupport/lib/active_support/core_ext/file/atomic.rb
@@ -1,6 +1,6 @@
# frozen_string_literal: true
-require "fileutils"
+require "securerandom"
class File
# Write to a file atomically. Useful for situations where you don't
@@ -19,21 +19,18 @@ class File
# file.write('hello')
# end
def self.atomic_write(file_name, temp_dir = dirname(file_name))
- require "tempfile" unless defined?(Tempfile)
+ old_stat = begin
+ File.stat(file_name)
+ rescue SystemCallError
+ nil
+ end
- Tempfile.open(".#{basename(file_name)}", temp_dir) do |temp_file|
+ # Names can't be longer than 255B
+ tmp_suffix = ".tmp.#{SecureRandom.hex}"
+ tmp_name = ".#{basename(file_name).byteslice(0, 254 - tmp_suffix.bytesize)}"
+ tmp_path = File.join(temp_dir, tmp_name)
+ open(tmp_path, RDWR | CREAT | EXCL | SHARE_DELETE | BINARY) do |temp_file|
temp_file.binmode
- return_val = yield temp_file
- temp_file.close
-
- old_stat = if exist?(file_name)
- # Get original file permissions
- stat(file_name)
- else
- # If not possible, probe which are the default permissions in the
- # destination directory.
- probe_stat_in(dirname(file_name))
- end
if old_stat
# Set correct permissions on new file
@@ -46,27 +43,14 @@ def self.atomic_write(file_name, temp_dir = dirname(file_name))
end
end
- # Overwrite original file with temp file
+ return_val = yield temp_file
+ rescue => error
+ temp_file.close rescue nil
+ unlink(temp_file.path) rescue nil
+ raise error
+ else
rename(temp_file.path, file_name)
return_val
end
end
-
- # Private utility method.
- def self.probe_stat_in(dir) # :nodoc:
- basename = [
- ".permissions_check",
- Thread.current.object_id,
- Process.pid,
- rand(1000000)
- ].join(".")
-
- file_name = join(dir, basename)
- FileUtils.touch(file_name)
- stat(file_name)
- rescue Errno::ENOENT
- file_name = nil
- ensure
- FileUtils.rm_f(file_name) if file_name
- end
end
diff --git a/activesupport/test/core_ext/file_test.rb b/activesupport/test/core_ext/file_test.rb
index 314d38c5918c8..e8a7200c06c58 100644
--- a/activesupport/test/core_ext/file_test.rb
+++ b/activesupport/test/core_ext/file_test.rb
@@ -47,29 +47,34 @@ def test_atomic_write_preserves_file_permissions
end
def test_atomic_write_preserves_default_file_permissions
- contents = "Atomic Text"
- File.atomic_write(file_name, Dir.pwd) do |file|
- file.write(contents)
- assert_not File.exist?(file_name)
+ Dir.mktmpdir do |temp_dir|
+ File.chmod 0700, temp_dir
+ file_path = File.join(temp_dir, file_name)
+
+ File.open(file_path, "wb", &:close)
+
+ original_permissions = File.stat(file_path).mode.to_s(8)
+ File.atomic_write(file_path, &:close)
+ actual_permissions = File.stat(file_path).mode.to_s(8)
+
+ assert_equal original_permissions, actual_permissions
end
- assert File.exist?(file_name)
- assert_equal File.probe_stat_in(Dir.pwd).mode, file_mode
- assert_equal contents, File.read(file_name)
- ensure
- File.unlink(file_name) rescue nil
end
def test_atomic_write_preserves_file_permissions_same_directory
Dir.mktmpdir do |temp_dir|
File.chmod 0700, temp_dir
+ file_path = File.join(temp_dir, file_name)
- probed_permissions = File.probe_stat_in(temp_dir).mode.to_s(8)
-
- File.atomic_write(File.join(temp_dir, file_name), &:close)
+ File.open(file_path, "wb") do |f|
+ f.chmod(0067)
+ end
- actual_permissions = File.stat(File.join(temp_dir, file_name)).mode.to_s(8)
+ original_permissions = File.stat(file_path).mode.to_s(8)
+ File.atomic_write(file_path, &:close)
+ actual_permissions = File.stat(file_path).mode.to_s(8)
- assert_equal actual_permissions, probed_permissions
+ assert_equal original_permissions, actual_permissions
end
end
@@ -83,8 +88,10 @@ def test_atomic_write_returns_result_from_yielded_block
File.unlink(file_name) rescue nil
end
- def test_probe_stat_in_when_no_dir
- assert_nil File.probe_stat_in("/dir/does/not/exist")
+ def test_when_no_dir
+ assert_raises Errno::ENOENT do
+ File.atomic_write("/dir/does/not/exist/file.txt") { }
+ end
end
private