Parameter tampering / metadata injection into internal state

Proof-of-concept (conceptual, not executed here): - Prereqs: Rails app with Active Storage and Direct Uploads enabled. - Step 1: Submit a direct upload request to the ActiveStorage Direct Uploads endpoint with metadata containing internal-use keys, e.g.: {"analyzed":"yes","identified":42,"composed":"maybe"}. - Step 2: Observe that in vulnerable versions (pre-fix), ActiveStorage::Blob would persist those keys in blob.metadata, potentially altering internal state or exposing internal flags. - Step 3: After applying the fix, the server filters these keys and the resulting blob.metadata will not include the injected keys. - Optional verification: fetch the blob and inspect metadata to confirm absence of protected keys. Note: Endpoint path is ActiveStorage Direct Uploads (e.g., POST /rails/active_storage/direct_uploads) with the standard JSON structure used by Direct Uploads.

diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb index d9fd01f9d0aef..74906f860b246 100644 --- a/activestorage/app/models/active_storage/blob.rb +++ b/activestorage/app/models/active_storage/blob.rb @@ -20,6 +20,11 @@ class ActiveStorage::Blob < ActiveStorage::Record MINIMUM_TOKEN_LENGTH = 28 has_secure_token :key, length: MINIMUM_TOKEN_LENGTH + + # FIXME: these property should never have been stored in the metadata. + # The blob table should be migrated to have dedicated columns for theses. + PROTECTED_METADATA = %w(analyzed identified composed) + private_constant :PROTECTED_METADATA store :metadata, accessors: [ :analyzed, :identified, :composed ], coder: ActiveRecord::Coders::JSON # Temporary reference to a local io during the upload flow. When set, @@ -110,6 +115,7 @@ def create_and_upload!(key: nil, io:, filename:, content_type: nil, metadata: ni # Once the form using the direct upload is submitted, the blob can be associated with the right record using # the signed ID. def create_before_direct_upload!(key: nil, filename:, byte_size:, checksum:, content_type: nil, metadata: nil, service_name: nil, record: nil) + metadata = filter_metadata(metadata) create! key: key, filename: filename, byte_size: byte_size, checksum: checksum, content_type: content_type, metadata: metadata, service_name: service_name end @@ -157,6 +163,15 @@ def compose(blobs, key: nil, filename:, content_type: nil, metadata: nil) combined_blob.save! end end + + private + def filter_metadata(metadata) + if metadata.is_a?(Hash) + metadata.without(*PROTECTED_METADATA) + else + metadata + end + end end include Analyzable diff --git a/activestorage/test/controllers/direct_uploads_controller_test.rb b/activestorage/test/controllers/direct_uploads_controller_test.rb index 54ff4e3017411..2e085551f2df4 100644 --- a/activestorage/test/controllers/direct_uploads_controller_test.rb +++ b/activestorage/test/controllers/direct_uploads_controller_test.rb @@ -133,8 +133,16 @@ class ActiveStorage::DiskDirectUploadsControllerTest < ActionDispatch::Integrati "library_ID" => "12345" } + protected_metadata = { + "analyzed" => "yolo", + "identified" => 42, + "composed" => "maybe", + } + + all_metadata = metadata.merge(protected_metadata) + post rails_direct_uploads_url, params: { blob: { - filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain", metadata: metadata } } + filename: "hello.txt", byte_size: 6, checksum: checksum, content_type: "text/plain", metadata: all_metadata } } response.parsed_body.tap do |details| assert_equal ActiveStorage::Blob.find(details["id"]), ActiveStorage::Blob.find_signed!(details["signed_id"])