From 19b7e292eed994b777eaeb2edcca98a16e895124 Mon Sep 17 00:00:00 2001 From: Kirk Wang Date: Mon, 10 Aug 2026 09:53:21 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=20Fix=20command=20injection,=20pat?= =?UTF-8?q?h=20traversal,=20and=20file=20disclosure=20in=20deposits?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attacker-controlled Content-Disposition filenames and deposited archives flowed unsanitized into shell commands and filesystem paths. - Remove both backtick shell sinks: md5sum -> Digest::MD5.file, file --mime-type -> Marcel (content-only detection) (CWE-78) - Parse Content-Disposition with a quote-aware parameter parser (filename* precedence, exact name match, ASCII-8BIT-safe) and sanitize the result: File.basename + strip control/bidi chars, blocking path traversal - Reject deposits whose extracted archive contains a symlink, which the bagit copy would otherwise dereference to ingest arbitrary server files Co-Authored-By: Claude Opus 4.8 --- .../concerns/willow_sword/fetch_headers.rb | 44 ++++++++-- .../concerns/willow_sword/process_request.rb | 4 +- .../concerns/willow_sword/save_data.rb | 39 +++++++-- .../willow_sword/fetch_headers_spec.rb | 83 +++++++++++++++++++ .../concerns/willow_sword/save_data_spec.rb | 45 ++++++++++ 5 files changed, 204 insertions(+), 11 deletions(-) create mode 100644 spec/controllers/concerns/willow_sword/fetch_headers_spec.rb create mode 100644 spec/controllers/concerns/willow_sword/save_data_spec.rb diff --git a/app/controllers/concerns/willow_sword/fetch_headers.rb b/app/controllers/concerns/willow_sword/fetch_headers.rb index 826f0e1..1a64950 100644 --- a/app/controllers/concerns/willow_sword/fetch_headers.rb +++ b/app/controllers/concerns/willow_sword/fetch_headers.rb @@ -25,16 +25,50 @@ def fetch_content_type end def fetch_filename - @headers[:filename] = nil cd = request.headers.fetch('Content-Disposition', '') - if cd.include? '=' - @headers[:filename] = cd.split('=')[-1].strip() + @headers[:filename] = sanitize_filename(extract_cd_filename(cd)) + end + + # Prefers RFC 5987/6266 filename* over filename, matching parameter names + # exactly (case-insensitive) and respecting quoted-string boundaries so a + # filename*= inside a quoted value or a name like xfilename* can't win. + # RFC 2231 continuations (filename*0, filename*1) are not reassembled. + def extract_cd_filename(cd) + return nil if cd.blank? + params = {} + cd.scan(/(?:\A|;)\s*([^\s=;]+)\s*=\s*("(?:[^"\\]|\\.)*"|[^;]*)/) do |name, val| + val = val.start_with?('"') ? val[1..-2].gsub(/\\(.)/, '\1') : val.strip + params[name.downcase] ||= val end - if @headers[:filename].blank? - @headers[:filename] = SecureRandom.uuid + if params.key?('filename*') + decode_ext_value(params['filename*']) + elsif params.key?('filename') + params['filename'] end end + # RFC 5987 ext-value: charset'lang'percent-encoded-value + def decode_ext_value(val) + charset, _lang, encoded = val.split("'", 3) + encoded ||= charset # no charset'lang' prefix; treat whole thing as value + bytes = encoded.gsub(/%([0-9a-fA-F]{2})/) { $1.hex.chr } + src = charset.to_s.casecmp?('iso-8859-1') ? Encoding::ISO_8859_1 : Encoding::UTF_8 + bytes.force_encoding(src).encode(Encoding::UTF_8, invalid: :replace, undef: :replace) + end + + # Blocks path traversal and control/bidi chars. Spaces, unicode, and shell + # metacharacters are preserved and safe only because nothing shells out with + # this value (see validate_payload, get_content_type) - keep it that way. + def sanitize_filename(name) + # Header bytes arrive as ASCII-8BIT; tag as UTF-8 so the control/bidi + # regex below is encoding-compatible, then scrub any invalid sequences. + name = name.to_s.dup.force_encoding(Encoding::UTF_8).scrub('').strip + name = name.gsub(/[\u0000-\u001f\u007f\u200e\u200f\u202a-\u202e\u2066-\u2069]/, %q()) + name = File.basename(name).tr('/\\', '_') + return SecureRandom.uuid if name.blank? || name == '.' || name == '..' + name + end + def fetch_md5hash @headers[:md5hash] = request.headers.fetch('Content-MD5', nil) end diff --git a/app/controllers/concerns/willow_sword/process_request.rb b/app/controllers/concerns/willow_sword/process_request.rb index 5cefa2d..0a98c2d 100644 --- a/app/controllers/concerns/willow_sword/process_request.rb +++ b/app/controllers/concerns/willow_sword/process_request.rb @@ -1,3 +1,5 @@ +require 'digest' + module WillowSword module ProcessRequest extend ActiveSupport::Concern @@ -34,7 +36,7 @@ def validate_and_save_request def validate_payload return true if @headers[:md5hash].nil? - md5 = `md5sum "#{@file.path}" | awk '{ print $1 }'`.strip + md5 = Digest::MD5.file(@file.path).hexdigest if md5 == @headers[:md5hash] true else diff --git a/app/controllers/concerns/willow_sword/save_data.rb b/app/controllers/concerns/willow_sword/save_data.rb index bc3c245..4c970ca 100644 --- a/app/controllers/concerns/willow_sword/save_data.rb +++ b/app/controllers/concerns/willow_sword/save_data.rb @@ -1,5 +1,6 @@ require 'fileutils' require 'securerandom' +require 'marcel' module WillowSword module SaveData @@ -72,7 +73,7 @@ def fetch_data(data, type, is_metadata) if is_metadata new_file_name = 'metadata.xml' else - new_file_name = data.original_filename + new_file_name = sanitize_filename(data.original_filename) end path = File.join(@dir, new_file_name) tmp = data.tempfile @@ -105,6 +106,9 @@ def organize_data(file_path) if content_type == 'application/zip' zp = WillowSword::ZipPackage.new(file_path, contents_path) zp.unzip_file + # A symlink in the archive would be dereferenced by the later bagit copy, + # ingesting the contents of whatever server file it points at. + reject_symlinks!(contents_path) validate_bagit(zp.dst) if @headers[:packaging] == 'http://purl.org/net/sword/package/BagIt' else # Copy file to contents dir @@ -124,9 +128,33 @@ def verify_data end def get_content_type(file_path) - # @extension = Rack::Mime::MIME_TYPES.invert[mime_type] - # Not matching content_type and packaging from headers with that computed. - return `file --b --mime-type "#{file_path}"`.strip + # Detect by content only (open as IO, no name hint) so a misleading + # extension can't steer archive handling, matching the old `file` check. + File.open(file_path, 'rb') { |io| Marcel::MimeType.for(io) } + end + + def reject_symlinks!(dir) + link = find_symlink(dir) + return unless link + message = "Archive contains a symbolic link, which is not allowed" + # Set @error (the work controller's local rescue keeps status only when + # it's present) and raise a real exception wrapper for the other paths. + @error = WillowSword::Error.new(message, :unprocessable_entity) + raise WillowSword::SwordError.new(@error) + end + + # Recursively finds the first symlink under dir without following symlinked + # directories (the symlink? check precedes the directory? descent). + def find_symlink(dir) + Dir.each_child(dir) do |name| + path = File.join(dir, name) + return path if File.symlink?(path) + if File.directory?(path) + found = find_symlink(path) + return found if found + end + end + nil end def validate_bagit(file_path) @@ -135,7 +163,8 @@ def validate_bagit(file_path) unless bag.valid? error_details = bag.errors.any? ? bag.errors.errors.values.join('; ') : '' message = "Invalid BagIt package: #{error_details}" - raise @error = WillowSword::Error.new(message, :unprocessable_entity) + @error = WillowSword::Error.new(message, :unprocessable_entity) + raise WillowSword::SwordError.new(@error) end true diff --git a/spec/controllers/concerns/willow_sword/fetch_headers_spec.rb b/spec/controllers/concerns/willow_sword/fetch_headers_spec.rb new file mode 100644 index 0000000..38f2bd2 --- /dev/null +++ b/spec/controllers/concerns/willow_sword/fetch_headers_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# Exercised through a controller that includes the FetchHeaders concern. +RSpec.describe WillowSword::V2::WorksController, type: :controller do + describe '#fetch_filename (Content-Disposition sanitization)' do + def filename_for(cd) + controller.request.headers['Content-Disposition'] = cd + controller.instance_variable_set(:@headers, {}) + controller.send(:fetch_filename) + controller.instance_variable_get(:@headers)[:filename] + end + + it 'strips path separators from an injection-style payload' do + name = filename_for('attachment; filename=x"; touch /tmp/pwned; echo "') + expect(name).not_to match(%r{[/\\]}) + end + + it 'prevents path traversal' do + expect(filename_for('attachment; filename="../../../../etc/passwd"')).to eq('passwd') + expect(filename_for('attachment; filename="..\\..\\windows\\x"')).not_to match(%r{[/\\]}) + end + + it 'preserves spaces and unicode in the filename' do + expect(filename_for('attachment; filename="my report.pdf"')).to eq('my report.pdf') + expect(filename_for('attachment; filename="café.pdf"')).to eq('café.pdf') + end + + it 'keeps a quoted filename containing "=" intact (old split truncated it)' do + expect(filename_for('attachment; filename="a=b.pdf"')).to eq('a=b.pdf') + end + + it 'ignores trailing non-filename parameters' do + expect(filename_for('attachment; filename="report.pdf"; foo=bar')).to eq('report.pdf') + end + + it 'does not treat filename*= inside a quoted value as the extended param' do + expect(filename_for('attachment; filename="notes filename*=draft.pdf"')).to eq('notes filename*=draft.pdf') + end + + it 'matches parameter names exactly (xfilename* is not filename*)' do + expect(filename_for('attachment; xfilename*=draft.pdf')).to match(/\A[0-9a-f-]{36}\z/) + end + + it 'prefers and percent-decodes filename* (RFC 5987/6266)' do + expect(filename_for("attachment; filename*=UTF-8''%e2%82%ac%20rates.pdf")).to eq('€ rates.pdf') + end + + it 'decodes filename* with a non-empty language tag' do + expect(filename_for("attachment; filename*=UTF-8'en'%E2%82%AC.pdf")).to eq('€.pdf') + end + + it 'does not leave a path separator from a slash-only filename' do + expect(filename_for('attachment; filename="/"')).not_to match(%r{[/\\]}) + end + + it 'handles a raw ASCII-8BIT filename with high bytes without raising' do + # Rack delivers header values as ASCII-8BIT; a UTF-8 regex against those + # bytes raised Encoding::CompatibilityError before the force_encoding fix. + raw = 'café.pdf'.dup.force_encoding('ASCII-8BIT') + expect(controller.send(:sanitize_filename, raw)).to eq('café.pdf') + end + + it 'matches the parameter name case-insensitively' do + expect(filename_for('attachment; FileName="Doc.pdf"')).to eq('Doc.pdf') + end + + it 'falls back to a UUID when no filename is present' do + expect(filename_for('attachment')).to match(/\A[0-9a-f-]{36}\z/) + expect(filename_for('')).to match(/\A[0-9a-f-]{36}\z/) + end + + it 'falls back to a UUID when sanitization leaves nothing usable' do + expect(filename_for('attachment; filename="../"')).to match(/\A[0-9a-f-]{36}\z/) + end + + it 'does not raise on invalid UTF-8 bytes from a decoded filename*' do + # regression: strip ran before scrub and raised ArgumentError -> unauthenticated 500 + expect { filename_for("attachment; filename*=UTF-8''%ff%fe.txt") }.not_to raise_error + end + end +end diff --git a/spec/controllers/concerns/willow_sword/save_data_spec.rb b/spec/controllers/concerns/willow_sword/save_data_spec.rb new file mode 100644 index 0000000..f95eaf3 --- /dev/null +++ b/spec/controllers/concerns/willow_sword/save_data_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'rails_helper' +require 'tmpdir' + +RSpec.describe WillowSword::V2::WorksController, type: :controller do + describe '#reject_symlinks! (zip symlink file-disclosure guard)' do + around do |example| + Dir.mktmpdir { |d| @dir = d; example.run } + end + + it 'rejects the deposit (422) when an extracted archive contains a symlink' do + File.write(File.join(@dir, 'ok.txt'), 'fine') + File.symlink('/etc/passwd', File.join(@dir, 'exfil.txt')) + + expect { controller.send(:reject_symlinks!, @dir) } + .to raise_error(WillowSword::SwordError) { |e| expect(e.sword_error.code).to eq(422) } + # @error must be set: the works controller's local rescue only keeps the + # 422 status when @error is already present, else it defaults to 400. + expect(controller.instance_variable_get(:@error).code).to eq(422) + end + + it 'detects a symlink nested in a real subdirectory' do + Dir.mkdir(File.join(@dir, 'sub')) + File.symlink('/etc/passwd', File.join(@dir, 'sub', 'exfil.txt')) + + expect { controller.send(:reject_symlinks!, @dir) }.to raise_error(StandardError) + end + + it 'does not descend into a symlinked directory (detects it, does not follow)' do + File.symlink('/etc', File.join(@dir, 'evil')) + # returns the symlink itself; must not raise SystemStackError from recursing into /etc + expect(controller.send(:find_symlink, @dir)).to eq(File.join(@dir, 'evil')) + end + + it 'passes cleanly when no symlinks are present' do + File.write(File.join(@dir, 'a.txt'), 'x') + Dir.mkdir(File.join(@dir, 'sub')) + File.write(File.join(@dir, 'sub', 'b.txt'), 'y') + + expect { controller.send(:reject_symlinks!, @dir) }.not_to raise_error + expect(controller.instance_variable_get(:@error)).to be_nil + end + end +end