kotovalexarian-likes-github/rails--rails
1
0
Fork 0
mirror of https://github.com/rails/rails.git synced 2022-11-09 12:12:34 -05:00
Code Releases Activity

Prevent content type and disposition bypass in storage service URLs

Browse source 
* 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.
This commit is contained in:
Rosa Gutierrez 2018-09-06 16:52:52 +02:00 committed by Rafael Mendonça França
parent 72300f9742
commit 06ab7b27ea
14 changed files with 172 additions and 38 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
activestorage/app/controllers/active_storage/disk_controller.rb
View file

@ -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

30
activestorage/app/models/active_storage/blob.rb
View file

@ -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

9
activestorage/app/models/active_storage/blob/identifiable.rb
View file

@ -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

2
activestorage/lib/active_storage.rb
View file

@ -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"

14
activestorage/lib/active_storage/engine.rb
View file

@ -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

8
activestorage/lib/active_storage/service.rb
View file

@ -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

2
activestorage/lib/active_storage/service/azure_storage_service.rb
View file

@ -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)

26
activestorage/lib/active_storage/service/disk_service.rb
View file

@ -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

24
activestorage/lib/active_storage/service/gcs_service.rb
View file

@ -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

4
activestorage/lib/active_storage/service/mirror_service.rb
View file

@ -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

11
activestorage/test/controllers/disk_controller_test.rb
View file

@ -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

27
activestorage/test/models/blob_test.rb
View file

@ -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

2
activestorage/test/models/variant_test.rb
View file

@ -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

49
activestorage/test/service/gcs_service_test.rb
View file

@ -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"))