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.

14 files changed, 172 insertions, 38 deletions

diff --git a/activestorage/app/controllers/active_storage/disk_controller.rb b/activestorage/app/controllers/active_storage/disk_controller.rb
index 99982202dd..df8d73cc91 100644
--- a/activestorage/app/controllers/active_storage/disk_controller.rb
+++ b/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
diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb
index 53aa9f0237..d9bdb27b38 100644
--- a/activestorage/app/models/active_storage/blob.rb
+++ b/activestorage/app/models/active_storage/blob.rb
@@ -130,8 +130,8 @@ class ActiveStorage::Blob < ActiveRecord::Base
   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 @@ class ActiveStorage::Blob < ActiveRecord::Base
   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 @@ class ActiveStorage::Blob < ActiveRecord::Base
       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
diff --git a/activestorage/app/models/active_storage/blob/identifiable.rb b/activestorage/app/models/active_storage/blob/identifiable.rb
index 2c17ddc25f..924bd06131 100644
--- a/activestorage/app/models/active_storage/blob/identifiable.rb
+++ b/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 @@ module ActiveStorage::Blob::Identifiable
         ""
       end
     end
+
+    def update_service_metadata
+      service.update_metadata key, service_metadata if service_metadata.any?
+    end
 end
diff --git a/activestorage/lib/active_storage.rb b/activestorage/lib/active_storage.rb
index a94ef626f2..19c7f6be2a 100644
--- a/activestorage/lib/active_storage.rb
+++ b/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"
 
diff --git a/activestorage/lib/active_storage/engine.rb b/activestorage/lib/active_storage/engine.rb
index d51e806220..b3bcf48d6f 100644
--- a/activestorage/lib/active_storage/engine.rb
+++ b/activestorage/lib/active_storage/engine.rb
@@ -40,6 +40,18 @@ module ActiveStorage
       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 @@ module ActiveStorage
         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
 
diff --git a/activestorage/lib/active_storage/service.rb b/activestorage/lib/active_storage/service.rb
index 54ba08fb87..c18fccbb1d 100644
--- a/activestorage/lib/active_storage/service.rb
+++ b/activestorage/lib/active_storage/service.rb
@@ -62,10 +62,16 @@ module ActiveStorage
 
     # 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
diff --git a/activestorage/lib/active_storage/service/azure_storage_service.rb b/activestorage/lib/active_storage/service/azure_storage_service.rb
index 8de3889cb5..a2c4b4d57c 100644
--- a/activestorage/lib/active_storage/service/azure_storage_service.rb
+++ b/activestorage/lib/active_storage/service/azure_storage_service.rb
@@ -17,7 +17,7 @@ module ActiveStorage
       @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)
diff --git a/activestorage/lib/active_storage/service/disk_service.rb b/activestorage/lib/active_storage/service/disk_service.rb
index 52f3a3df16..2588c41760 100644
--- a/activestorage/lib/active_storage/service/disk_service.rb
+++ b/activestorage/lib/active_storage/service/disk_service.rb
@@ -15,7 +15,7 @@ module ActiveStorage
       @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 @@ module ActiveStorage
 
     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
diff --git a/activestorage/lib/active_storage/service/gcs_service.rb b/activestorage/lib/active_storage/service/gcs_service.rb
index 18c0f14cfc..e2590aa35d 100644
--- a/activestorage/lib/active_storage/service/gcs_service.rb
+++ b/activestorage/lib/active_storage/service/gcs_service.rb
@@ -11,16 +11,15 @@ module ActiveStorage
       @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 @@ module ActiveStorage
       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
diff --git a/activestorage/lib/active_storage/service/mirror_service.rb b/activestorage/lib/active_storage/service/mirror_service.rb
index 6002ef5a00..75274f81b3 100644
--- a/activestorage/lib/active_storage/service/mirror_service.rb
+++ b/activestorage/lib/active_storage/service/mirror_service.rb
@@ -24,9 +24,9 @@ module ActiveStorage
 
     # 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
 
diff --git a/activestorage/test/controllers/disk_controller_test.rb b/activestorage/test/controllers/disk_controller_test.rb
index 7b5e989699..0aa8648db3 100644
--- a/activestorage/test/controllers/disk_controller_test.rb
+++ b/activestorage/test/controllers/disk_controller_test.rb
@@ -5,11 +5,12 @@ require "database/setup"
 
 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
 
diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb
index 1a6a89de56..0eac622d72 100644
--- a/activestorage/test/models/blob_test.rb
+++ b/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
diff --git a/activestorage/test/models/variant_test.rb b/activestorage/test/models/variant_test.rb
index 6577f1cd9f..8552080e7b 100644
--- a/activestorage/test/models/variant_test.rb
+++ b/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
diff --git a/activestorage/test/service/gcs_service_test.rb b/activestorage/test/service/gcs_service_test.rb
index 2ba2f8b346..73337ef8e1 100644
--- a/activestorage/test/service/gcs_service_test.rb
+++ b/activestorage/test/service/gcs_service_test.rb
@@ -31,6 +31,55 @@ if SERVICE_CONFIGURATIONS[:gcs]
       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"))