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.

7 files changed, 58 insertions, 22 deletions

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