From c85cc667ebfd3c270df37c7575d580ea6462e12f Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 22 Aug 2023 09:58:43 -0700 Subject: [PATCH] Use a temporary file for storing unencrypted files while editing When we're editing the contents of encrypted files, we should use the `Tempfile` class because it creates temporary files with restrictive permissions. This prevents other users on the same system from reading the contents of those files while the user is editing them. [CVE-2023-38037] --- .../lib/active_support/encrypted_file.rb | 17 ++++++++--------- activesupport/test/encrypted_file_test.rb | 8 ++++++++ railties/lib/rails/secrets.rb | 18 ++++++++++-------- 3 files changed, 26 insertions(+), 17 deletions(-) diff --git a/activesupport/lib/active_support/encrypted_file.rb b/activesupport/lib/active_support/encrypted_file.rb index a35cc54ef5c52..dc28aa9a15fa9 100644 --- a/activesupport/lib/active_support/encrypted_file.rb +++ b/activesupport/lib/active_support/encrypted_file.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "pathname" -require "tmpdir" +require "tempfile" require "active_support/message_encryptor" module ActiveSupport @@ -69,17 +69,16 @@ def change(&block) private def writing(contents) - tmp_file = "#{Process.pid}.#{content_path.basename.to_s.chomp('.enc')}" - tmp_path = Pathname.new File.join(Dir.tmpdir, tmp_file) - tmp_path.binwrite contents + Tempfile.create(["", "-" + content_path.basename.to_s.chomp(".enc")]) do |tmp_file| + tmp_path = Pathname.new(tmp_file) + tmp_path.binwrite contents - yield tmp_path + yield tmp_path - updated_contents = tmp_path.binread + updated_contents = tmp_path.binread - write(updated_contents) if updated_contents != contents - ensure - FileUtils.rm(tmp_path) if tmp_path&.exist? + write(updated_contents) if updated_contents != contents + end end diff --git a/activesupport/test/encrypted_file_test.rb b/activesupport/test/encrypted_file_test.rb index 0050685065a9e..49f7437764fe8 100644 --- a/activesupport/test/encrypted_file_test.rb +++ b/activesupport/test/encrypted_file_test.rb @@ -49,6 +49,14 @@ class EncryptedFileTest < ActiveSupport::TestCase assert_equal "#{@content} and went by the lake", @encrypted_file.read end + test "change sets restricted permissions" do + @encrypted_file.write(@content) + @encrypted_file.change do |file| + assert_predicate file, :owned? + assert_equal "100600", file.stat.mode.to_s(8), "Incorrect mode for #{file}" + end + end + test "raise MissingKeyError when key is missing" do assert_raise ActiveSupport::EncryptedFile::MissingKeyError do encrypted_file(@content_path, key_path: "", env_key: "").read diff --git a/railties/lib/rails/secrets.rb b/railties/lib/rails/secrets.rb index 54ba53c03b981..913d5e57c1bfb 100644 --- a/railties/lib/rails/secrets.rb +++ b/railties/lib/rails/secrets.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "yaml" +require "tempfile" require "active_support/message_encryptor" module Rails @@ -87,17 +88,18 @@ def preprocess(path) end def writing(contents) - tmp_file = "#{File.basename(path)}.#{Process.pid}" - tmp_path = File.join(Dir.tmpdir, tmp_file) - IO.binwrite(tmp_path, contents) + file_name = "#{File.basename(path)}.#{Process.pid}" - yield tmp_path + Tempfile.create(["", "-" + file_name]) do |tmp_file| + tmp_path = Pathname.new(tmp_file) + tmp_path.binwrite contents - updated_contents = IO.binread(tmp_path) + yield tmp_path - write(updated_contents) if updated_contents != contents - ensure - FileUtils.rm(tmp_path) if File.exist?(tmp_path) + updated_contents = tmp_path.binread + + write(updated_contents) if updated_contents != contents + end end def encryptor