Prevent content type and disposition bypass in storage service URLs

* Force content-type to binary on service urls for relevant content types

We have a list of content types that must be forcibly served as binary,
but in practice this only means to serve them as attachment always. We
should also set the Content-Type to the configured binary type.

As a bonus: add text/cache-manifest to the list of content types to be
served as binary by default.

* Store content-disposition and content-type in GCS

Forcing these in the service_url when serving the file works fine for S3
and Azure, since these services include params in the signature.
However, GCS specifically excludes response-content-disposition and
response-content-type from the signature, which means an attacker can
modify these and have files that should be served as text/plain attachments
served as inline HTML for example. This makes our attempt to force
specific files to be served as binary and as attachment can be easily
bypassed.

The only way this can be forced in GCS is by storing
content-disposition and content-type in the object metadata.

* Update GCS object metadata after identifying blob

In some cases we create the blob and upload the data before identifying
the content-type, which means we can't store that in GCS right when
uploading. In these, after creating the attachment, we enqueue a job to
identify the blob, and set the content-type.

In other cases, files are uploaded to the storage service via direct
upload link. We create the blob before the direct upload, which happens
independently from the blob creation itself. We then mark the blob as
identified, but we have already the content-type we need without having
put it in the service.

In these two cases, then, we need to update the metadata in the GCS
service.

* Include content-type and disposition in the verified key for disk service

This prevents an attacker from modifying these params in the service
signed URL, which is particularly important when we want to force them
to have specific values for security reasons.

* Allow only a list of specific content types to be served inline

This is different from the content types that must be served as binary
in the sense that any content type not in this list will be always
served as attachment but with its original content type. Only types in
this list are allowed to be served either inline or as attachment.

Apart from forcing this in the service URL, for GCS we need to store the
disposition in the metadata.

Fix CVE-2018-16477.

activestorage/app/controllers/active_storage/disk_controller.rb

@@ -9,7 +9,7 @@ class ActiveStorage::DiskController < ActiveStorage::BaseController
 
   def show
     if key = decode_verified_key
-      serve_file disk_service.path_for(key), content_type: params[:content_type], disposition: params[:disposition]
+      serve_file disk_service.path_for(key[:key]), content_type: key[:content_type], disposition: key[:disposition]
     else
       head :not_found
     end

activestorage/app/models/active_storage/blob.rb

@@ -130,8 +130,8 @@ def text?
   def service_url(expires_in: ActiveStorage.service_urls_expire_in, disposition: :inline, filename: nil, **options)
     filename = ActiveStorage::Filename.wrap(filename || self.filename)
 
-    service.url key, expires_in: expires_in, filename: filename, content_type: content_type,
-      disposition: forcibly_serve_as_binary? ? :attachment : disposition, **options
+    service.url key, expires_in: expires_in, filename: filename, content_type: content_type_for_service_url,
+      disposition: forced_disposition_for_service_url || disposition, **options
   end
 
   # Returns a URL that can be used to directly upload a file for this blob on the service. This URL is intended to be
@@ -170,7 +170,7 @@ def unfurl(io, identify: true) #:nodoc:
   end
 
   def upload_without_unfurling(io) #:nodoc:
-    service.upload key, io, checksum: checksum
+    service.upload key, io, checksum: checksum, **service_metadata
   end
 
   # Downloads the file associated with this blob. If no block is given, the entire file is read into memory and returned.
@@ -239,5 +239,29 @@ def forcibly_serve_as_binary?
       ActiveStorage.content_types_to_serve_as_binary.include?(content_type)
     end
 
+    def allowed_inline?
+      ActiveStorage.content_types_allowed_inline.include?(content_type)
+    end
+
+    def content_type_for_service_url
+      forcibly_serve_as_binary? ? ActiveStorage.binary_content_type : content_type
+    end
+
+    def forced_disposition_for_service_url
+      if forcibly_serve_as_binary? || !allowed_inline?
+        :attachment
+      end
+    end
+
+    def service_metadata
+      if forcibly_serve_as_binary?
+        { content_type: ActiveStorage.binary_content_type, disposition: :attachment, filename: filename }
+      elsif !allowed_inline?
+        { content_type: content_type, disposition: :attachment, filename: filename }
+      else
+        { content_type: content_type }
+      end
+    end
+
     ActiveSupport.run_load_hooks(:active_storage_blob, self)
 end

activestorage/app/models/active_storage/blob/identifiable.rb

@@ -2,7 +2,10 @@
 
 module ActiveStorage::Blob::Identifiable
   def identify
-    update! content_type: identify_content_type, identified: true unless identified?
+    unless identified?
+      update! content_type: identify_content_type, identified: true
+      update_service_metadata
+    end
   end
 
   def identified?
@@ -21,4 +24,8 @@ def download_identifiable_chunk
         ""
       end
     end
+
+    def update_service_metadata
+      service.update_metadata key, service_metadata if service_metadata.any?
+    end
 end

activestorage/lib/active_storage.rb

@@ -49,6 +49,8 @@ module ActiveStorage
   mattr_accessor :paths, default: {}
   mattr_accessor :variable_content_types, default: []
   mattr_accessor :content_types_to_serve_as_binary, default: []
+  mattr_accessor :content_types_allowed_inline, default: []
+  mattr_accessor :binary_content_type, default: "application/octet-stream"
   mattr_accessor :service_urls_expire_in, default: 5.minutes
   mattr_accessor :routes_prefix, default: "/rails/active_storage"
 

activestorage/lib/active_storage/engine.rb

@@ -40,6 +40,18 @@ class Engine < Rails::Engine # :nodoc:
       text/xml
       application/xml
       application/xhtml+xml
+      application/mathml+xml
+      text/cache-manifest
+    )
+
+    config.active_storage.content_types_allowed_inline = %w(
+      image/png
+      image/gif
+      image/jpg
+      image/jpeg
+      image/vnd.adobe.photoshop
+      image/vnd.microsoft.icon
+      application/pdf
     )
 
     config.eager_load_namespaces << ActiveStorage
@@ -57,6 +69,8 @@ class Engine < Rails::Engine # :nodoc:
         ActiveStorage.variable_content_types = app.config.active_storage.variable_content_types || []
         ActiveStorage.content_types_to_serve_as_binary = app.config.active_storage.content_types_to_serve_as_binary || []
         ActiveStorage.service_urls_expire_in = app.config.active_storage.service_urls_expire_in || 5.minutes
+        ActiveStorage.content_types_allowed_inline = app.config.active_storage.content_types_allowed_inline || []
+        ActiveStorage.binary_content_type = app.config.active_storage.binary_content_type || "application/octet-stream"
       end
     end
 

activestorage/lib/active_storage/service.rb

@@ -62,10 +62,16 @@ def build(configurator:, service: nil, **service_config) #:nodoc:
 
     # Upload the +io+ to the +key+ specified. If a +checksum+ is provided, the service will
     # ensure a match when the upload has completed or raise an ActiveStorage::IntegrityError.
-    def upload(key, io, checksum: nil)
+    def upload(key, io, checksum: nil, **options)
       raise NotImplementedError
     end
 
+    # Update metadata for the file identified by +key+ in the service.
+    # Override in subclasses only if the service needs to store specific
+    # metadata that has to be updated upon identification.
+    def update_metadata(key, **metadata)
+    end
+
     # Return the content of the file at the +key+.
     def download(key)
       raise NotImplementedError

activestorage/lib/active_storage/service/azure_storage_service.rb

@@ -17,7 +17,7 @@ def initialize(storage_account_name:, storage_access_key:, container:)
       @container = container
     end
 
-    def upload(key, io, checksum: nil)
+    def upload(key, io, checksum: nil, **)
       instrument :upload, key: key, checksum: checksum do
         handle_errors do
           blobs.create_block_blob(container, key, IO.try_convert(io) || io, content_md5: checksum)

activestorage/lib/active_storage/service/disk_service.rb

@@ -15,7 +15,7 @@ def initialize(root:)
       @root = root
     end
 
-    def upload(key, io, checksum: nil)
+    def upload(key, io, checksum: nil, **)
       instrument :upload, key: key, checksum: checksum do
         IO.copy_stream(io, make_path_for(key))
         ensure_integrity_of(key, checksum) if checksum
@@ -79,17 +79,23 @@ def exist?(key)
 
     def url(key, expires_in:, filename:, disposition:, content_type:)
       instrument :url, key: key do |payload|
-        verified_key_with_expiration = ActiveStorage.verifier.generate(key, expires_in: expires_in, purpose: :blob_key)
-
-        generated_url =
-          url_helpers.rails_disk_service_url(
-            verified_key_with_expiration,
-            host: current_host,
-            filename: filename,
-            disposition: content_disposition_with(type: disposition, filename: filename),
+        content_disposition = content_disposition_with(type: disposition, filename: filename)
+        verified_key_with_expiration = ActiveStorage.verifier.generate(
+          {
+            key: key,
+            disposition: content_disposition,
             content_type: content_type
-          )
+          },
+          { expires_in: expires_in,
+          purpose: :blob_key }
+        )
 
+        generated_url = url_helpers.rails_disk_service_url(verified_key_with_expiration,
+          host: current_host,
+          disposition: content_disposition,
+          content_type: content_type,
+          filename: filename
+        )
         payload[:url] = generated_url
 
         generated_url

activestorage/lib/active_storage/service/gcs_service.rb

@@ -11,16 +11,15 @@ def initialize(**config)
       @config = config
     end
 
-    def upload(key, io, checksum: nil)
+    def upload(key, io, checksum: nil, content_type: nil, disposition: nil, filename: nil)
       instrument :upload, key: key, checksum: checksum do
         begin
-          # The official GCS client library doesn't allow us to create a file with no Content-Type metadata.
-          # We need the file we create to have no Content-Type so we can control it via the response-content-type
-          # param in signed URLs. Workaround: let the GCS client create the file with an inferred
-          # Content-Type (usually "application/octet-stream") then clear it.
-          bucket.create_file(io, key, md5: checksum).update do |file|
-            file.content_type = nil
-          end
+          # GCS's signed URLs don't include params such as response-content-type response-content_disposition
+          # in the signature, which means an attacker can modify them and bypass our effort to force these to
+          # binary and attachment when the file's content type requires it. The only way to force them is to
+          # store them as object's metadata.
+          content_disposition = content_disposition_with(type: disposition, filename: filename) if disposition && filename
+          bucket.create_file(io, key, md5: checksum, content_type: content_type, content_disposition: content_disposition)
         rescue Google::Cloud::InvalidArgumentError
           raise ActiveStorage::IntegrityError
         end
@@ -43,6 +42,15 @@ def download(key, &block)
       end
     end
 
+    def update_metadata(key, content_type:, disposition: nil, filename: nil)
+      instrument :update_metadata, key: key, content_type: content_type, disposition: disposition do
+        file_for(key).update do |file|
+          file.content_type = content_type
+          file.content_disposition = content_disposition_with(type: disposition, filename: filename) if disposition && filename
+        end
+      end
+    end
+
     def download_chunk(key, range)
       instrument :download_chunk, key: key, range: range do
         begin

activestorage/lib/active_storage/service/mirror_service.rb

@@ -24,9 +24,9 @@ def initialize(primary:, mirrors:)
 
     # Upload the +io+ to the +key+ specified to all services. If a +checksum+ is provided, all services will
     # ensure a match when the upload has completed or raise an ActiveStorage::IntegrityError.
-    def upload(key, io, checksum: nil)
+    def upload(key, io, checksum: nil, **options)
       each_service.collect do |service|
-        service.upload key, io.tap(&:rewind), checksum: checksum
+        service.upload key, io.tap(&:rewind), checksum: checksum, **options
       end
     end
 

activestorage/test/controllers/disk_controller_test.rb

@@ -5,11 +5,12 @@
 
 class ActiveStorage::DiskControllerTest < ActionDispatch::IntegrationTest
   test "showing blob inline" do
-    blob = create_blob
+    blob = create_blob(filename: "hello.jpg", content_type: "image/jpg")
+
     get blob.service_url
     assert_response :ok
-    assert_equal "inline; filename=\"hello.txt\"; filename*=UTF-8''hello.txt", response.headers["Content-Disposition"]
-    assert_equal "text/plain", response.headers["Content-Type"]
+    assert_equal "inline; filename=\"hello.jpg\"; filename*=UTF-8''hello.jpg", response.headers["Content-Disposition"]
+    assert_equal "image/jpg", response.headers["Content-Type"]
     assert_equal "Hello world!", response.body
   end
 
@@ -36,6 +37,10 @@ class ActiveStorage::DiskControllerTest < ActionDispatch::IntegrationTest
     blob.delete
 
     get blob.service_url
+  end
+
+  test "showing blob with invalid key" do
+    get rails_disk_service_url(encoded_key: "Invalid key", filename: "hello.txt")
     assert_response :not_found
   end
 

activestorage/test/models/blob_test.rb

@@ -121,12 +121,21 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     end
   end
 
-  test "urls force attachment as content disposition for content types served as binary" do
+  test "urls force content_type to binary and attachment as content disposition for content types served as binary" do
     blob = create_blob(content_type: "text/html")
 
     freeze_time do
-      assert_equal expected_url_for(blob, disposition: :attachment), blob.service_url
-      assert_equal expected_url_for(blob, disposition: :attachment), blob.service_url(disposition: :inline)
+      assert_equal expected_url_for(blob, disposition: :attachment, content_type: "application/octet-stream"), blob.service_url
+      assert_equal expected_url_for(blob, disposition: :attachment, content_type: "application/octet-stream"), blob.service_url(disposition: :inline)
+    end
+  end
+
+  test "urls force attachment as content disposition when the content type is not allowed inline" do
+    blob = create_blob(content_type: "application/zip")
+
+    freeze_time do
+      assert_equal expected_url_for(blob, disposition: :attachment, content_type: "application/zip"), blob.service_url
+      assert_equal expected_url_for(blob, disposition: :attachment, content_type: "application/zip"), blob.service_url(disposition: :inline)
     end
   end
 
@@ -148,7 +157,7 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     arguments = [
       blob.key,
       expires_in: ActiveStorage.service_urls_expire_in,
-      disposition: :inline,
+      disposition: :attachment,
       content_type: blob.content_type,
       filename: blob.filename,
       thumb_size: "300x300",
@@ -183,9 +192,13 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
   end
 
   private
-    def expected_url_for(blob, disposition: :inline, filename: nil)
+    def expected_url_for(blob, disposition: :attachment, filename: nil, content_type: nil)
       filename ||= blob.filename
-      query_string = { content_type: blob.content_type, disposition: ActionDispatch::Http::ContentDisposition.format(disposition: disposition, filename: filename.sanitized) }.to_param
-      "https://example.com/rails/active_storage/disk/#{ActiveStorage.verifier.generate(blob.key, expires_in: 5.minutes, purpose: :blob_key)}/#{filename}?#{query_string}"
+      content_type ||= blob.content_type
+
+      query = { disposition: disposition.to_s + "; #{filename.parameters}", content_type: content_type }
+      key_params = { key: blob.key }.merge(query)
+
+      "https://example.com/rails/active_storage/disk/#{ActiveStorage.verifier.generate(key_params, expires_in: 5.minutes, purpose: :blob_key)}/#{filename}?#{query.to_param}"
     end
 end

activestorage/test/models/variant_test.rb

@@ -156,7 +156,7 @@ class ActiveStorage::VariantTest < ActiveSupport::TestCase
   test "service_url doesn't grow in length despite long variant options" do
     blob = create_file_blob(filename: "racecar.jpg")
     variant = blob.variant(font: "a" * 10_000).processed
-    assert_operator variant.service_url.length, :<, 525
+    assert_operator variant.service_url.length, :<, 726
   end
 
   test "works for vips processor" do

activestorage/test/service/gcs_service_test.rb

@@ -31,6 +31,55 @@ class ActiveStorage::Service::GCSServiceTest < ActiveSupport::TestCase
       end
     end
 
+    test "upload with content_type and content_disposition" do
+      begin
+        key      = SecureRandom.base58(24)
+        data     = "Something else entirely!"
+
+        @service.upload(key, StringIO.new(data), checksum: Digest::MD5.base64digest(data), disposition: :attachment, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain")
+
+        url = @service.url(key, expires_in: 2.minutes, disposition: :inline, content_type: "text/html", filename: ActiveStorage::Filename.new("test.html"))
+        response = Net::HTTP.get_response(URI(url))
+        assert_equal "text/plain", response.content_type
+        assert_match /attachment;.*test.txt/, response["Content-Disposition"]
+      ensure
+        @service.delete key
+      end
+    end
+
+    test "upload with content_type" do
+      begin
+        key      = SecureRandom.base58(24)
+        data     = "Something else entirely!"
+
+        @service.upload(key, StringIO.new(data), checksum: Digest::MD5.base64digest(data), content_type: "text/plain")
+
+        url = @service.url(key, expires_in: 2.minutes, disposition: :inline, content_type: "text/html", filename: ActiveStorage::Filename.new("test.html"))
+        response = Net::HTTP.get_response(URI(url))
+        assert_equal "text/plain", response.content_type
+        assert_match /inline;.*test.html/, response["Content-Disposition"]
+      ensure
+        @service.delete key
+      end
+    end
+
+    test "update metadata" do
+      begin
+        key      = SecureRandom.base58(24)
+        data     = "Something else entirely!"
+        @service.upload(key, StringIO.new(data), checksum: Digest::MD5.base64digest(data), disposition: :attachment, filename: ActiveStorage::Filename.new("test.html"), content_type: "text/html")
+
+        @service.update_metadata(key, disposition: :inline, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain")
+        url = @service.url(key, expires_in: 2.minutes, disposition: :attachment, content_type: "text/html", filename: ActiveStorage::Filename.new("test.html"))
+
+        response = Net::HTTP.get_response(URI(url))
+        assert_equal "text/plain", response.content_type
+        assert_match /inline;.*test.txt/, response["Content-Disposition"]
+      ensure
+        @service.delete key
+      end
+    end
+
     test "signed URL generation" do
       assert_match(/storage\.googleapis\.com\/.*response-content-disposition=inline.*test\.txt.*response-content-type=text%2Fplain/,
         @service.url(@key, expires_in: 2.minutes, disposition: :inline, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain"))