Merge branch 'master-sec'
* master-sec: Check that request is same-origin prior to including CSRF token in XHRs HMAC raw CSRF token before masking it, so it cannot be used to reconstruct a per-form token activesupport: Avoid Marshal.load on raw cache value in RedisCacheStore activesupport: Avoid Marshal.load on raw cache value in MemCacheStore Return self when calling #each, #each_pair, and #each_value instead of the raw @parameters hash Include Content-Length in signature for ActiveStorage direct upload
This commit is contained in:
commit
b472feddbc
@ -322,13 +322,10 @@ def masked_authenticity_token(session, form_options: {}) # :doc:
|
||||
action_path = normalize_action_path(action)
|
||||
per_form_csrf_token(session, action_path, method)
|
||||
else
|
||||
real_csrf_token(session)
|
||||
global_csrf_token(session)
|
||||
end
|
||||
|
||||
one_time_pad = SecureRandom.random_bytes(AUTHENTICITY_TOKEN_LENGTH)
|
||||
encrypted_csrf_token = xor_byte_strings(one_time_pad, raw_token)
|
||||
masked_token = one_time_pad + encrypted_csrf_token
|
||||
Base64.urlsafe_encode64(masked_token, padding: false)
|
||||
mask_token(raw_token)
|
||||
end
|
||||
|
||||
# Checks the client's masked token to see if it matches the
|
||||
@ -358,7 +355,8 @@ def valid_authenticity_token?(session, encoded_masked_token) # :doc:
|
||||
elsif masked_token.length == AUTHENTICITY_TOKEN_LENGTH * 2
|
||||
csrf_token = unmask_token(masked_token)
|
||||
|
||||
compare_with_real_token(csrf_token, session) ||
|
||||
compare_with_global_token(csrf_token, session) ||
|
||||
compare_with_real_token(csrf_token, session) ||
|
||||
valid_per_form_csrf_token?(csrf_token, session)
|
||||
else
|
||||
false # Token is malformed.
|
||||
@ -373,10 +371,21 @@ def unmask_token(masked_token) # :doc:
|
||||
xor_byte_strings(one_time_pad, encrypted_csrf_token)
|
||||
end
|
||||
|
||||
def mask_token(raw_token) # :doc:
|
||||
one_time_pad = SecureRandom.random_bytes(AUTHENTICITY_TOKEN_LENGTH)
|
||||
encrypted_csrf_token = xor_byte_strings(one_time_pad, raw_token)
|
||||
masked_token = one_time_pad + encrypted_csrf_token
|
||||
Base64.urlsafe_encode64(masked_token, padding: false)
|
||||
end
|
||||
|
||||
def compare_with_real_token(token, session) # :doc:
|
||||
ActiveSupport::SecurityUtils.fixed_length_secure_compare(token, real_csrf_token(session))
|
||||
end
|
||||
|
||||
def compare_with_global_token(token, session) # :doc:
|
||||
ActiveSupport::SecurityUtils.fixed_length_secure_compare(token, global_csrf_token(session))
|
||||
end
|
||||
|
||||
def valid_per_form_csrf_token?(token, session) # :doc:
|
||||
if per_form_csrf_tokens
|
||||
correct_token = per_form_csrf_token(
|
||||
@ -397,10 +406,21 @@ def real_csrf_token(session) # :doc:
|
||||
end
|
||||
|
||||
def per_form_csrf_token(session, action_path, method) # :doc:
|
||||
csrf_token_hmac(session, [action_path, method.downcase].join("#"))
|
||||
end
|
||||
|
||||
GLOBAL_CSRF_TOKEN_IDENTIFIER = "!real_csrf_token"
|
||||
private_constant :GLOBAL_CSRF_TOKEN_IDENTIFIER
|
||||
|
||||
def global_csrf_token(session) # :doc:
|
||||
csrf_token_hmac(session, GLOBAL_CSRF_TOKEN_IDENTIFIER)
|
||||
end
|
||||
|
||||
def csrf_token_hmac(session, identifier) # :doc:
|
||||
OpenSSL::HMAC.digest(
|
||||
OpenSSL::Digest::SHA256.new,
|
||||
real_csrf_token(session),
|
||||
[action_path, method.downcase].join("#")
|
||||
identifier
|
||||
)
|
||||
end
|
||||
|
||||
|
@ -364,6 +364,8 @@ def each_pair(&block)
|
||||
@parameters.each_pair do |key, value|
|
||||
yield [key, convert_hashes_to_parameters(key, value)]
|
||||
end
|
||||
|
||||
self
|
||||
end
|
||||
alias_method :each, :each_pair
|
||||
|
||||
@ -374,6 +376,8 @@ def each_value(&block)
|
||||
@parameters.each_pair do |key, value|
|
||||
yield convert_hashes_to_parameters(key, value)
|
||||
end
|
||||
|
||||
self
|
||||
end
|
||||
|
||||
# Attribute that keeps track of converted arrays, if any, to avoid double
|
||||
|
@ -19,6 +19,18 @@ class ParametersAccessorsTest < ActiveSupport::TestCase
|
||||
)
|
||||
end
|
||||
|
||||
test "each returns self" do
|
||||
assert_same @params, @params.each { |_| _ }
|
||||
end
|
||||
|
||||
test "each_pair returns self" do
|
||||
assert_same @params, @params.each_pair { |_| _ }
|
||||
end
|
||||
|
||||
test "each_value returns self" do
|
||||
assert_same @params, @params.each_value { |_| _ }
|
||||
end
|
||||
|
||||
test "[] retains permitted status" do
|
||||
@params.permit!
|
||||
assert_predicate @params[:person], :permitted?
|
||||
|
@ -953,6 +953,39 @@ def test_accepts_global_csrf_token
|
||||
assert_response :success
|
||||
end
|
||||
|
||||
def test_does_not_return_old_csrf_token
|
||||
get :index
|
||||
|
||||
token = @controller.send(:form_authenticity_token)
|
||||
|
||||
unmasked_token = @controller.send(:unmask_token, Base64.urlsafe_decode64(token))
|
||||
|
||||
assert_not_equal @controller.send(:real_csrf_token, session), unmasked_token
|
||||
end
|
||||
|
||||
def test_returns_hmacd_token
|
||||
get :index
|
||||
|
||||
token = @controller.send(:form_authenticity_token)
|
||||
|
||||
unmasked_token = @controller.send(:unmask_token, Base64.urlsafe_decode64(token))
|
||||
|
||||
assert_equal @controller.send(:global_csrf_token, session), unmasked_token
|
||||
end
|
||||
|
||||
def test_accepts_old_csrf_token
|
||||
get :index
|
||||
|
||||
non_hmac_token = @controller.send(:mask_token, @controller.send(:real_csrf_token, session))
|
||||
|
||||
# This is required because PATH_INFO isn't reset between requests.
|
||||
@request.env["PATH_INFO"] = "/per_form_tokens/post_one"
|
||||
assert_nothing_raised do
|
||||
post :post_one, params: { custom_authenticity_token: non_hmac_token }
|
||||
end
|
||||
assert_response :success
|
||||
end
|
||||
|
||||
def test_chomps_slashes
|
||||
get :index, params: { form_path: "/per_form_tokens/post_one?foo=bar" }
|
||||
|
||||
|
@ -52,9 +52,10 @@ createXHR = (options, done) ->
|
||||
# Sending FormData will automatically set Content-Type to multipart/form-data
|
||||
if typeof options.data is 'string'
|
||||
xhr.setRequestHeader('Content-Type', 'application/x-www-form-urlencoded; charset=UTF-8')
|
||||
xhr.setRequestHeader('X-Requested-With', 'XMLHttpRequest') unless options.crossDomain
|
||||
# Add X-CSRF-Token
|
||||
CSRFProtection(xhr)
|
||||
unless options.crossDomain
|
||||
xhr.setRequestHeader('X-Requested-With', 'XMLHttpRequest')
|
||||
# Add X-CSRF-Token
|
||||
CSRFProtection(xhr)
|
||||
xhr.withCredentials = !!options.withCredentials
|
||||
xhr.onreadystatechange = ->
|
||||
done(xhr) if xhr.readyState is XMLHttpRequest.DONE
|
||||
|
@ -80,7 +80,8 @@ def exist?(key)
|
||||
def url_for_direct_upload(key, expires_in:, content_type:, content_length:, checksum:)
|
||||
instrument :url, key: key do |payload|
|
||||
generated_url = object_for(key).presigned_url :put, expires_in: expires_in.to_i,
|
||||
content_type: content_type, content_length: content_length, content_md5: checksum, **upload_options
|
||||
content_type: content_type, content_length: content_length, content_md5: checksum
|
||||
whitelist_headers: ['content-length'], **upload_options
|
||||
|
||||
payload[:url] = generated_url
|
||||
|
||||
|
@ -55,6 +55,29 @@ class ActiveStorage::Service::S3ServiceTest < ActiveSupport::TestCase
|
||||
@service.delete key
|
||||
end
|
||||
|
||||
test "directly uploading file larger than the provided content-length does not work" do
|
||||
key = SecureRandom.base58(24)
|
||||
data = "Some text that is longer than the specified content length"
|
||||
checksum = Digest::MD5.base64digest(data)
|
||||
url = @service.url_for_direct_upload(key, expires_in: 5.minutes, content_type: "text/plain", content_length: data.size - 1, checksum: checksum)
|
||||
|
||||
uri = URI.parse url
|
||||
request = Net::HTTP::Put.new uri.request_uri
|
||||
request.body = data
|
||||
request.add_field "Content-Type", "text/plain"
|
||||
request.add_field "Content-MD5", checksum
|
||||
upload_result = Net::HTTP.start(uri.host, uri.port, use_ssl: true) do |http|
|
||||
http.request request
|
||||
end
|
||||
|
||||
assert_equal "403", upload_result.code
|
||||
assert_raises ActiveStorage::FileNotFoundError do
|
||||
@service.download(key)
|
||||
end
|
||||
ensure
|
||||
@service.delete key
|
||||
end
|
||||
|
||||
test "upload a zero byte file" do
|
||||
blob = directly_upload_file_blob filename: "empty_file.txt", content_type: nil
|
||||
user = User.create! name: "DHH", avatar: blob
|
||||
|
@ -8,7 +8,6 @@
|
||||
end
|
||||
|
||||
require "active_support/core_ext/enumerable"
|
||||
require "active_support/core_ext/marshal"
|
||||
require "active_support/core_ext/array/extract_options"
|
||||
|
||||
module ActiveSupport
|
||||
@ -29,14 +28,6 @@ class MemCacheStore < Store
|
||||
# Provide support for raw values in the local cache strategy.
|
||||
module LocalCacheWithRaw # :nodoc:
|
||||
private
|
||||
def read_entry(key, **options)
|
||||
entry = super
|
||||
if options[:raw] && local_cache && entry
|
||||
entry = deserialize_entry(entry.value)
|
||||
end
|
||||
entry
|
||||
end
|
||||
|
||||
def write_entry(key, entry, **options)
|
||||
if options[:raw] && local_cache
|
||||
raw_entry = Entry.new(entry.value.to_s)
|
||||
@ -195,9 +186,8 @@ def normalize_key(key, options)
|
||||
key
|
||||
end
|
||||
|
||||
def deserialize_entry(raw_value)
|
||||
if raw_value
|
||||
entry = Marshal.load(raw_value) rescue raw_value
|
||||
def deserialize_entry(entry)
|
||||
if entry
|
||||
entry.is_a?(Entry) ? entry : Entry.new(entry)
|
||||
end
|
||||
end
|
||||
|
@ -74,14 +74,6 @@ def self.supports_cache_versioning?
|
||||
# Support raw values in the local cache strategy.
|
||||
module LocalCacheWithRaw # :nodoc:
|
||||
private
|
||||
def read_entry(key, **options)
|
||||
entry = super
|
||||
if options[:raw] && local_cache && entry
|
||||
entry = deserialize_entry(entry.value)
|
||||
end
|
||||
entry
|
||||
end
|
||||
|
||||
def write_entry(key, entry, **options)
|
||||
if options[:raw] && local_cache
|
||||
raw_entry = Entry.new(serialize_entry(entry, raw: true))
|
||||
@ -352,7 +344,8 @@ def set_redis_capabilities
|
||||
# Read an entry from the cache.
|
||||
def read_entry(key, **options)
|
||||
failsafe :read_entry do
|
||||
deserialize_entry redis.with { |c| c.get(key) }
|
||||
raw = options&.fetch(:raw, false)
|
||||
deserialize_entry(redis.with { |c| c.get(key) }, raw: raw)
|
||||
end
|
||||
end
|
||||
|
||||
@ -368,6 +361,7 @@ def read_multi_mget(*names)
|
||||
options = names.extract_options!
|
||||
options = merged_options(options)
|
||||
return {} if names == []
|
||||
raw = options&.fetch(:raw, false)
|
||||
|
||||
keys = names.map { |name| normalize_key(name, options) }
|
||||
|
||||
@ -377,7 +371,7 @@ def read_multi_mget(*names)
|
||||
|
||||
names.zip(values).each_with_object({}) do |(name, value), results|
|
||||
if value
|
||||
entry = deserialize_entry(value)
|
||||
entry = deserialize_entry(value, raw: raw)
|
||||
unless entry.nil? || entry.expired? || entry.mismatched?(normalize_version(name, options))
|
||||
results[name] = entry.value
|
||||
end
|
||||
@ -457,10 +451,13 @@ def truncate_key(key)
|
||||
end
|
||||
end
|
||||
|
||||
def deserialize_entry(serialized_entry)
|
||||
def deserialize_entry(serialized_entry, raw:)
|
||||
if serialized_entry
|
||||
entry = Marshal.load(serialized_entry) rescue serialized_entry
|
||||
entry.is_a?(Entry) ? entry : Entry.new(entry)
|
||||
if raw
|
||||
Entry.new(serialized_entry)
|
||||
else
|
||||
Marshal.load(serialized_entry)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
|
@ -3,11 +3,11 @@
|
||||
module CacheIncrementDecrementBehavior
|
||||
def test_increment
|
||||
@cache.write("foo", 1, raw: true)
|
||||
assert_equal 1, @cache.read("foo").to_i
|
||||
assert_equal 1, @cache.read("foo", raw: true).to_i
|
||||
assert_equal 2, @cache.increment("foo")
|
||||
assert_equal 2, @cache.read("foo").to_i
|
||||
assert_equal 2, @cache.read("foo", raw: true).to_i
|
||||
assert_equal 3, @cache.increment("foo")
|
||||
assert_equal 3, @cache.read("foo").to_i
|
||||
assert_equal 3, @cache.read("foo", raw: true).to_i
|
||||
|
||||
missing = @cache.increment("bar")
|
||||
assert(missing.nil? || missing == 1)
|
||||
@ -15,11 +15,11 @@ def test_increment
|
||||
|
||||
def test_decrement
|
||||
@cache.write("foo", 3, raw: true)
|
||||
assert_equal 3, @cache.read("foo").to_i
|
||||
assert_equal 3, @cache.read("foo", raw: true).to_i
|
||||
assert_equal 2, @cache.decrement("foo")
|
||||
assert_equal 2, @cache.read("foo").to_i
|
||||
assert_equal 2, @cache.read("foo", raw: true).to_i
|
||||
assert_equal 1, @cache.decrement("foo")
|
||||
assert_equal 1, @cache.read("foo").to_i
|
||||
assert_equal 1, @cache.read("foo", raw: true).to_i
|
||||
|
||||
missing = @cache.decrement("bar")
|
||||
assert(missing.nil? || missing == -1)
|
||||
|
@ -472,8 +472,8 @@ def test_race_condition_protection
|
||||
def test_crazy_key_characters
|
||||
crazy_key = "#/:*(<+=> )&$%@?;'\"\'`~-"
|
||||
assert @cache.write(crazy_key, "1", raw: true)
|
||||
assert_equal "1", @cache.read(crazy_key)
|
||||
assert_equal "1", @cache.fetch(crazy_key)
|
||||
assert_equal "1", @cache.read(crazy_key, raw: true)
|
||||
assert_equal "1", @cache.fetch(crazy_key, raw: true)
|
||||
assert @cache.delete(crazy_key)
|
||||
assert_equal "2", @cache.fetch(crazy_key, raw: true) { "2" }
|
||||
assert_equal 3, @cache.increment(crazy_key)
|
||||
@ -497,7 +497,7 @@ def test_cache_hit_instrumentation
|
||||
@events << ActiveSupport::Notifications::Event.new(*args)
|
||||
end
|
||||
assert @cache.write(key, "1", raw: true)
|
||||
assert @cache.fetch(key) { }
|
||||
assert @cache.fetch(key, raw: true) { }
|
||||
assert_equal 1, @events.length
|
||||
assert_equal "cache_read.active_support", @events[0].name
|
||||
assert_equal :fetch, @events[0].payload[:super_operation]
|
||||
|
@ -8,8 +8,8 @@ module EncodedKeyCacheBehavior
|
||||
define_method "test_#{encoding.name.underscore}_encoded_values" do
|
||||
key = (+"foo").force_encoding(encoding)
|
||||
assert @cache.write(key, "1", raw: true)
|
||||
assert_equal "1", @cache.read(key)
|
||||
assert_equal "1", @cache.fetch(key)
|
||||
assert_equal "1", @cache.read(key, raw: true)
|
||||
assert_equal "1", @cache.fetch(key, raw: true)
|
||||
assert @cache.delete(key)
|
||||
assert_equal "2", @cache.fetch(key, raw: true) { "2" }
|
||||
assert_equal 3, @cache.increment(key)
|
||||
@ -20,8 +20,8 @@ module EncodedKeyCacheBehavior
|
||||
def test_common_utf8_values
|
||||
key = (+"\xC3\xBCmlaut").force_encoding(Encoding::UTF_8)
|
||||
assert @cache.write(key, "1", raw: true)
|
||||
assert_equal "1", @cache.read(key)
|
||||
assert_equal "1", @cache.fetch(key)
|
||||
assert_equal "1", @cache.read(key, raw: true)
|
||||
assert_equal "1", @cache.fetch(key, raw: true)
|
||||
assert @cache.delete(key)
|
||||
assert_equal "2", @cache.fetch(key, raw: true) { "2" }
|
||||
assert_equal 3, @cache.increment(key)
|
||||
|
@ -115,7 +115,7 @@ def test_local_cache_of_increment
|
||||
@cache.write("foo", 1, raw: true)
|
||||
@peek.write("foo", 2, raw: true)
|
||||
@cache.increment("foo")
|
||||
assert_equal 3, @cache.read("foo")
|
||||
assert_equal 3, @cache.read("foo", raw: true)
|
||||
end
|
||||
end
|
||||
|
||||
@ -124,7 +124,7 @@ def test_local_cache_of_decrement
|
||||
@cache.write("foo", 1, raw: true)
|
||||
@peek.write("foo", 3, raw: true)
|
||||
@cache.decrement("foo")
|
||||
assert_equal 2, @cache.read("foo")
|
||||
assert_equal 2, @cache.read("foo", raw: true)
|
||||
end
|
||||
end
|
||||
|
||||
@ -142,9 +142,9 @@ def test_local_cache_of_read_multi
|
||||
@cache.with_local_cache do
|
||||
@cache.write("foo", "foo", raw: true)
|
||||
@cache.write("bar", "bar", raw: true)
|
||||
values = @cache.read_multi("foo", "bar")
|
||||
assert_equal "foo", @cache.read("foo")
|
||||
assert_equal "bar", @cache.read("bar")
|
||||
values = @cache.read_multi("foo", "bar", raw: true)
|
||||
assert_equal "foo", @cache.read("foo", raw: true)
|
||||
assert_equal "bar", @cache.read("bar", raw: true)
|
||||
assert_equal "foo", values["foo"]
|
||||
assert_equal "bar", values["bar"]
|
||||
end
|
||||
|
@ -79,7 +79,7 @@ def test_raw_values
|
||||
def test_raw_values_with_marshal
|
||||
cache = lookup_store(raw: true)
|
||||
cache.write("foo", Marshal.dump([]))
|
||||
assert_equal [], cache.read("foo")
|
||||
assert_equal Marshal.dump([]), cache.read("foo")
|
||||
end
|
||||
|
||||
def test_local_cache_raw_values
|
||||
@ -108,7 +108,7 @@ def test_local_cache_raw_values_with_marshal
|
||||
cache = lookup_store(raw: true)
|
||||
cache.with_local_cache do
|
||||
cache.write("foo", Marshal.dump([]))
|
||||
assert_equal [], cache.read("foo")
|
||||
assert_equal Marshal.dump([]), cache.read("foo")
|
||||
end
|
||||
end
|
||||
|
||||
|
@ -17,6 +17,7 @@ class SlowRedis < Redis
|
||||
def get(key)
|
||||
if /latency/.match?(key)
|
||||
sleep 3
|
||||
super
|
||||
else
|
||||
super
|
||||
end
|
||||
|
Loading…
Reference in New Issue
Block a user