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

actionpack/lib/action_controller/metal/request_forgery_protection.rb

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

actionpack/lib/action_controller/metal/strong_parameters.rb

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

actionpack/test/controller/parameters/accessors_test.rb

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

actionpack/test/controller/request_forgery_protection_test.rb

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

actionview/app/assets/javascripts/rails-ujs/utils/ajax.coffee

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

activestorage/lib/active_storage/service/s3_service.rb

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

activestorage/test/service/s3_service_test.rb

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

activesupport/lib/active_support/cache/mem_cache_store.rb

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

activesupport/lib/active_support/cache/redis_cache_store.rb

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

activesupport/test/cache/behaviors/cache_increment_decrement_behavior.rb

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

activesupport/test/cache/behaviors/cache_store_behavior.rb

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

activesupport/test/cache/behaviors/encoded_key_cache_behavior.rb

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

activesupport/test/cache/behaviors/local_cache_behavior.rb

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

activesupport/test/cache/stores/mem_cache_store_test.rb

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

activesupport/test/cache/stores/redis_cache_store_test.rb

@@ -17,6 +17,7 @@ class SlowRedis < Redis
   def get(key)
     if /latency/.match?(key)
       sleep 3
+      super
     else
       super
     end