Glob injection / Path traversal

# Proof-of-concept PoC illustrating the glob-injection risk and the mitigation # This is a standalone Ruby demonstration showing the before/after behavior # of Dir.glob when the path contains unescaped glob metacharacters in a key. require 'tmpdir' require 'fileutils' Dir.mktmpdir("rails-glob-poc") do |root| # Setup: create two distinct blob directories FileUtils.mkdir_p(File.join(root, "blob[1]")) File.write(File.join(root, "blob[1]", "danger.txt"), "delete me") FileUtils.mkdir_p(File.join(root, "blobnormal")) File.write(File.join(root, "blobnormal", "safe.txt"), "keep me") # Vulnerable path usage (unescaped glob metacharacters in the key) prefix = File.join(root, "blob[1]") puts "[Before fix] Globbing with unescaped prefix: #{prefix}/*" Dir.glob("#{prefix}/*").each { |p| FileUtils.rm_rf(p) } puts "Remaining after vulnerable glob: #{Dir.glob("#{root}/**/*").sort.inspect}" # Reset state for a safe demonstration of the fix FileUtils.mkdir_p(File.join(root, "blob[1]")) File.write(File.join(root, "blob[1]", "danger.txt"), "delete me") # Escaped path usage (the fix escapes glob chars) escaped_prefix = prefix.gsub(/[\[\]\*\?\{\}\\]/) { |c| "\\#{c}" } puts "[After fix] Globbing with escaped prefix: #{escaped_prefix}/*" Dir.glob("#{escaped_prefix}/*").each { |p| FileUtils.rm_rf(p) } puts "Remaining after patched glob: #{Dir.glob("#{root}/**/*").sort.inspect}" end

diff --git a/activestorage/lib/active_storage/service/disk_service.rb b/activestorage/lib/active_storage/service/disk_service.rb index 163712cf01f09..10c9b27352245 100644 --- a/activestorage/lib/active_storage/service/disk_service.rb +++ b/activestorage/lib/active_storage/service/disk_service.rb @@ -60,7 +60,16 @@ def delete(key) def delete_prefixed(prefix) instrument :delete_prefixed, prefix: prefix do - Dir.glob(path_for("#{prefix}*")).each do |path| + prefix_path = path_for(prefix) + + # File.expand_path (called within path_for) strips trailing slashes. + # Restore trailing separator if the original prefix had one, so that + # the glob "prefix/*" matches files inside the directory, not siblings + # whose names start with the prefix string. + prefix_path += "/" if prefix.end_with?("/") + + escaped = escape_glob_metacharacters(prefix_path) + Dir.glob("#{escaped}*").each do |path| FileUtils.rm_rf(path) end end @@ -187,6 +196,10 @@ def folder_for(key) [ key[0..1], key[2..3] ].join("/") end + def escape_glob_metacharacters(path) + path.gsub(/[\[\]*?{}\\]/) { |c| "\\#{c}" } + end + def make_path_for(key) path_for(key).tap { |path| FileUtils.mkdir_p File.dirname(path) } end diff --git a/activestorage/test/service/disk_service_test.rb b/activestorage/test/service/disk_service_test.rb index 305a728e05034..3663a08a168a1 100644 --- a/activestorage/test/service/disk_service_test.rb +++ b/activestorage/test/service/disk_service_test.rb @@ -161,6 +161,52 @@ class ActiveStorage::Service::DiskServiceTest < ActiveSupport::TestCase end end + test "path_for escapes all glob metacharacters" do + assert_equal "\\[", @service.send(:escape_glob_metacharacters, "[") + assert_equal "\\]", @service.send(:escape_glob_metacharacters, "]") + assert_equal "\\*", @service.send(:escape_glob_metacharacters, "*") + assert_equal "\\?", @service.send(:escape_glob_metacharacters, "?") + assert_equal "\\{", @service.send(:escape_glob_metacharacters, "{") + assert_equal "\\}", @service.send(:escape_glob_metacharacters, "}") + assert_equal "\\\\", @service.send(:escape_glob_metacharacters, "\\") + assert_equal "hello", @service.send(:escape_glob_metacharacters, "hello") + assert_equal "/path/to/\\[brackets\\]/file", @service.send(:escape_glob_metacharacters, "/path/to/[brackets]/file") + end + + test "delete_prefixed with glob metacharacters only deletes matching files" do + base_key = SecureRandom.base58(24) + bracket_key = "#{base_key}[1]/file" + plain_key = "#{base_key}1/file" + + @service.upload(bracket_key, StringIO.new("bracket")) + @service.upload(plain_key, StringIO.new("plain")) + + @service.delete_prefixed("#{base_key}[1]/") + + assert @service.exist?(plain_key), "file should not be deleted" + assert_not @service.exist?(bracket_key), "file should be deleted" + ensure + @service.delete(bracket_key) rescue nil + @service.delete(plain_key) rescue nil + end + + test "delete_prefixed with trailing slash only deletes files inside the directory" do + base_key = SecureRandom.base58(24) + inside_key = "#{base_key}/file" + sibling_key = "#{base_key}_sibling" + + @service.upload(inside_key, StringIO.new("inside")) + @service.upload(sibling_key, StringIO.new("sibling")) + + @service.delete_prefixed("#{base_key}/") + + assert @service.exist?(sibling_key), "sibling file should not be deleted" + assert_not @service.exist?(inside_key), "file inside directory should be deleted" + ensure + @service.delete(inside_key) rescue nil + @service.delete(sibling_key) rescue nil + end + test "can change root" do tmp_path_2 = File.join(Dir.tmpdir, "active_storage_2") @service.root = tmp_path_2