Merge pull request #270 from twitter/ff-child-frame-src

Handle child/frame-src gracefully across different browsers and versions

Author: oreoshake

Gemfile

@@ -5,7 +5,8 @@ gemspec
 group :test do
   gem "tins", "~> 1.6.0" # 1.7 requires ruby 2.0
   gem "pry-nav"
-  gem "rack"
+  gem "json", "~> 1"
+  gem "rack", "~> 1"
   gem "rspec"
   gem "coveralls"
 end

README.md

@@ -53,20 +53,19 @@ SecureHeaders::Configuration.default do |config|
 
     # directive values: these values will directly translate into source directives
     default_src: %w(https: 'self'),
-    frame_src: %w('self' *.twimg.com itunes.apple.com),
+    base_uri: %w('self'),
+    block_all_mixed_content: true, # see http://www.w3.org/TR/mixed-content/
+    child_src: %w('self'),
     connect_src: %w(wss:),
     font_src: %w('self' data:),
+    form_action: %w('self' github.com),
+    frame_ancestors: %w('none'),
     img_src: %w(mycdn.com data:),
     media_src: %w(utoob.com),
     object_src: %w('self'),
+    plugin_types: %w(application/x-shockwave-flash),
     script_src: %w('self'),
     style_src: %w('unsafe-inline'),
-    base_uri: %w('self'),
-    child_src: %w('self'),
-    form_action: %w('self' github.com),
-    frame_ancestors: %w('none'),
-    plugin_types: %w(application/x-shockwave-flash),
-    block_all_mixed_content: true, # see http://www.w3.org/TR/mixed-content/
     upgrade_insecure_requests: true, # see https://www.w3.org/TR/upgrade-insecure-requests/
     report_uri: %w(https://report-uri.io/example-csp)
   }

lib/secure_headers/configuration.rb

@@ -203,7 +203,7 @@ def csp=(new_csp)
         raise IllegalPolicyModificationError, "You are attempting to modify CSP settings directly. Use dynamic_csp= instead."
       end
 
-      @csp = new_csp
+      @csp = self.class.send(:deep_copy_if_hash, new_csp)
     end
 
     def cookies=(cookies)
@@ -215,7 +215,7 @@ def cached_headers=(headers)
     end
 
     def hpkp=(hpkp)
-      @hpkp = hpkp
+      @hpkp = self.class.send(:deep_copy_if_hash, hpkp)
     end
 
     def hpkp_report_host=(hpkp_report_host)

lib/secure_headers/headers/content_security_policy.rb

@@ -1,18 +1,22 @@
 require_relative 'policy_management'
+require 'useragent'
 
 module SecureHeaders
   class ContentSecurityPolicyConfigError < StandardError; end
   class ContentSecurityPolicy
     include PolicyManagement
 
+    # constants to be used for version-specific UA sniffing
+    VERSION_46 = ::UserAgent::Version.new("46")
+
     def initialize(config = nil, user_agent = OTHER)
-      config = Configuration.deep_copy(DEFAULT_CONFIG) unless config
-      @config = config
+      @config = Configuration.send(:deep_copy, config || DEFAULT_CONFIG)
       @parsed_ua = if user_agent.is_a?(UserAgent::Browsers::Base)
         user_agent
       else
         UserAgent.parse(user_agent)
       end
+      normalize_child_frame_src
       @report_only = @config[:report_only]
       @preserve_schemes = @config[:preserve_schemes]
       @script_nonce = @config[:script_nonce]
@@ -42,6 +46,22 @@ def value
 
     private
 
+    # frame-src is deprecated, child-src is being implemented. They are
+    # very similar and in most cases, the same value can be used for both.
+    def normalize_child_frame_src
+      if @config[:frame_src] && @config[:child_src] && @config[:frame_src] != @config[:child_src]
+        Kernel.warn("#{Kernel.caller.first}: [DEPRECATION] both :child_src and :frame_src supplied and do not match. This can lead to inconsistent behavior across browsers.")
+      elsif @config[:frame_src]
+        Kernel.warn("#{Kernel.caller.first}: [DEPRECATION] :frame_src is deprecated, use :child_src instead. Provided: #{@config[:frame_src]}.")
+      end
+
+      if supported_directives.include?(:child_src)
+        @config[:child_src] = @config[:child_src] || @config[:frame_src]
+      else
+        @config[:frame_src] = @config[:frame_src] || @config[:child_src]
+      end
+    end
+
     # Private: converts the config object into a string representing a policy.
     # Places default-src at the first directive and report-uri as the last. All
     # others are presented in alphabetical order.
@@ -162,9 +182,19 @@ def strip_source_schemes!(source_list)
 
     # Private: determine which directives are supported for the given user agent.
     #
+    # Add UA-sniffing special casing here.
+    #
     # Returns an array of symbols representing the directives.
     def supported_directives
-      @supported_directives ||= VARIATIONS[@parsed_ua.browser] || VARIATIONS[OTHER]
+      @supported_directives ||= if VARIATIONS[@parsed_ua.browser]
+        if @parsed_ua.browser == "Firefox" && @parsed_ua.version >= VERSION_46
+          VARIATIONS["FirefoxTransitional"]
+        else
+          VARIATIONS[@parsed_ua.browser]
+        end
+      else
+        VARIATIONS[OTHER]
+      end
     end
 
     def nonces_supported?

lib/secure_headers/headers/policy_management.rb

@@ -100,10 +100,23 @@ def self.included(base)
       PLUGIN_TYPES
     ].freeze
 
+    FIREFOX_46_DEPRECATED_DIRECTIVES = [
+      FRAME_SRC
+    ].freeze
+
+    FIREFOX_46_UNSUPPORTED_DIRECTIVES = [
+      BLOCK_ALL_MIXED_CONTENT,
+      PLUGIN_TYPES
+    ].freeze
+
     FIREFOX_DIRECTIVES = (
       DIRECTIVES_2_0 + DIRECTIVES_DRAFT - FIREFOX_UNSUPPORTED_DIRECTIVES
     ).freeze
 
+    FIREFOX_46_DIRECTIVES = (
+      DIRECTIVES_2_0 + DIRECTIVES_DRAFT - FIREFOX_46_UNSUPPORTED_DIRECTIVES - FIREFOX_46_DEPRECATED_DIRECTIVES
+    ).freeze
+
     CHROME_DIRECTIVES = (
       DIRECTIVES_2_0 + DIRECTIVES_DRAFT
     ).freeze
@@ -118,6 +131,7 @@ def self.included(base)
       "Chrome" => CHROME_DIRECTIVES,
       "Opera" => CHROME_DIRECTIVES,
       "Firefox" => FIREFOX_DIRECTIVES,
+      "FirefoxTransitional" => FIREFOX_46_DIRECTIVES,
       "Safari" => SAFARI_DIRECTIVES,
       "Edge" => EDGE_DIRECTIVES,
       "Other" => CHROME_DIRECTIVES

spec/lib/secure_headers/headers/content_security_policy_spec.rb

@@ -24,7 +24,7 @@ module SecureHeaders
 
     describe "#value" do
       it "discards 'none' values if any other source expressions are present" do
-        csp = ContentSecurityPolicy.new(default_opts.merge(frame_src: %w('self' 'none')))
+        csp = ContentSecurityPolicy.new(default_opts.merge(child_src: %w('self' 'none')))
         expect(csp.value).not_to include("'none'")
       end
 
@@ -86,42 +86,68 @@ module SecureHeaders
         expect(csp.value).to eq("default-src example.org")
       end
 
+      it "emits a warning when using frame-src" do
+        expect(Kernel).to receive(:warn).with(/:frame_src is deprecated, use :child_src instead./)
+        ContentSecurityPolicy.new(default_src: %w('self'), frame_src: %w('self')).value
+      end
+
+      it "emits a warning when child-src and frame-src are supplied but are not equal" do
+        expect(Kernel).to receive(:warn).with(/both :child_src and :frame_src supplied and do not match./)
+        ContentSecurityPolicy.new(default_src: %w('self'), child_src: %w(child-src.com), frame_src: %w(frame-src,com)).value
+      end
+
+      it "will still set inconsistent child/frame-src values to be less surprising" do
+        expect(Kernel).to receive(:warn).at_least(:once)
+        firefox = ContentSecurityPolicy.new({default_src: %w('self'), child_src: %w(child-src.com), frame_src: %w(frame-src,com)}, USER_AGENTS[:firefox]).value
+        firefox_transitional = ContentSecurityPolicy.new({default_src: %w('self'), child_src: %w(child-src.com), frame_src: %w(frame-src,com)}, USER_AGENTS[:firefox46]).value
+        expect(firefox).not_to eq(firefox_transitional)
+        expect(firefox).to match(/frame-src/)
+        expect(firefox).not_to match(/child-src/)
+        expect(firefox_transitional).to match(/child-src/)
+        expect(firefox_transitional).not_to match(/frame-src/)
+      end
+
       context "browser sniffing" do
         let (:complex_opts) do
-          ContentSecurityPolicy::ALL_DIRECTIVES.each_with_object({}) do |directive, hash|
-            hash[directive] = %w('self')
+          (ContentSecurityPolicy::ALL_DIRECTIVES - [:frame_src]).each_with_object({}) do |directive, hash|
+            hash[directive] = ["#{directive.to_s.gsub("_", "-")}.com"]
           end.merge({
             block_all_mixed_content: true,
             upgrade_insecure_requests: true,
             reflected_xss: "block",
-            script_src: %w('self'),
+            script_src: %w(script-src.com),
             script_nonce: 123456
           })
         end
 
         it "does not filter any directives for Chrome" do
           policy = ContentSecurityPolicy.new(complex_opts, USER_AGENTS[:chrome])
-          expect(policy.value).to eq("default-src 'self'; base-uri 'self'; block-all-mixed-content; child-src 'self'; connect-src 'self'; font-src 'self'; form-action 'self'; frame-ancestors 'self'; frame-src 'self'; img-src 'self'; media-src 'self'; object-src 'self'; plugin-types 'self'; sandbox 'self'; script-src 'self' 'nonce-123456'; style-src 'self'; upgrade-insecure-requests; report-uri 'self'")
+          expect(policy.value).to eq("default-src default-src.com; base-uri base-uri.com; block-all-mixed-content; child-src child-src.com; connect-src connect-src.com; font-src font-src.com; form-action form-action.com; frame-ancestors frame-ancestors.com; img-src img-src.com; media-src media-src.com; object-src object-src.com; plugin-types plugin-types.com; sandbox sandbox.com; script-src script-src.com 'nonce-123456'; style-src style-src.com; upgrade-insecure-requests; report-uri report-uri.com")
         end
 
         it "does not filter any directives for Opera" do
           policy = ContentSecurityPolicy.new(complex_opts, USER_AGENTS[:opera])
-          expect(policy.value).to eq("default-src 'self'; base-uri 'self'; block-all-mixed-content; child-src 'self'; connect-src 'self'; font-src 'self'; form-action 'self'; frame-ancestors 'self'; frame-src 'self'; img-src 'self'; media-src 'self'; object-src 'self'; plugin-types 'self'; sandbox 'self'; script-src 'self' 'nonce-123456'; style-src 'self'; upgrade-insecure-requests; report-uri 'self'")
+          expect(policy.value).to eq("default-src default-src.com; base-uri base-uri.com; block-all-mixed-content; child-src child-src.com; connect-src connect-src.com; font-src font-src.com; form-action form-action.com; frame-ancestors frame-ancestors.com; img-src img-src.com; media-src media-src.com; object-src object-src.com; plugin-types plugin-types.com; sandbox sandbox.com; script-src script-src.com 'nonce-123456'; style-src style-src.com; upgrade-insecure-requests; report-uri report-uri.com")
         end
 
         it "filters blocked-all-mixed-content, child-src, and plugin-types for firefox" do
           policy = ContentSecurityPolicy.new(complex_opts, USER_AGENTS[:firefox])
-          expect(policy.value).to eq("default-src 'self'; base-uri 'self'; connect-src 'self'; font-src 'self'; form-action 'self'; frame-ancestors 'self'; frame-src 'self'; img-src 'self'; media-src 'self'; object-src 'self'; sandbox 'self'; script-src 'self' 'nonce-123456'; style-src 'self'; upgrade-insecure-requests; report-uri 'self'")
+          expect(policy.value).to eq("default-src default-src.com; base-uri base-uri.com; connect-src connect-src.com; font-src font-src.com; form-action form-action.com; frame-ancestors frame-ancestors.com; frame-src child-src.com; img-src img-src.com; media-src media-src.com; object-src object-src.com; sandbox sandbox.com; script-src script-src.com 'nonce-123456'; style-src style-src.com; upgrade-insecure-requests; report-uri report-uri.com")
+        end
+
+        it "filters blocked-all-mixed-content, frame-src, and plugin-types for firefox 46 and higher" do
+          policy = ContentSecurityPolicy.new(complex_opts, USER_AGENTS[:firefox46])
+          expect(policy.value).to eq("default-src default-src.com; base-uri base-uri.com; child-src child-src.com; connect-src connect-src.com; font-src font-src.com; form-action form-action.com; frame-ancestors frame-ancestors.com; img-src img-src.com; media-src media-src.com; object-src object-src.com; sandbox sandbox.com; script-src script-src.com 'nonce-123456'; style-src style-src.com; upgrade-insecure-requests; report-uri report-uri.com")
         end
 
-        it "adds 'unsafe-inline', filters base-uri, blocked-all-mixed-content, upgrade-insecure-requests, child-src, form-action, frame-ancestors, nonce sources, hash sources, and plugin-types for Edge" do
+        it "child-src value is copied to frame-src, adds 'unsafe-inline', filters base-uri, blocked-all-mixed-content, upgrade-insecure-requests, child-src, form-action, frame-ancestors, nonce sources, hash sources, and plugin-types for Edge" do
           policy = ContentSecurityPolicy.new(complex_opts, USER_AGENTS[:edge])
-          expect(policy.value).to eq("default-src 'self'; connect-src 'self'; font-src 'self'; frame-src 'self'; img-src 'self'; media-src 'self'; object-src 'self'; sandbox 'self'; script-src 'self' 'unsafe-inline'; style-src 'self'; report-uri 'self'")
+          expect(policy.value).to eq("default-src default-src.com; connect-src connect-src.com; font-src font-src.com; frame-src child-src.com; img-src img-src.com; media-src media-src.com; object-src object-src.com; sandbox sandbox.com; script-src script-src.com 'unsafe-inline'; style-src style-src.com; report-uri report-uri.com")
         end
 
-        it "adds 'unsafe-inline', filters base-uri, blocked-all-mixed-content, upgrade-insecure-requests, child-src, form-action, frame-ancestors, nonce sources, hash sources, and plugin-types for safari" do
+        it "child-src value is copied to frame-src, adds 'unsafe-inline', filters base-uri, blocked-all-mixed-content, upgrade-insecure-requests, child-src, form-action, frame-ancestors, nonce sources, hash sources, and plugin-types for safari" do
           policy = ContentSecurityPolicy.new(complex_opts, USER_AGENTS[:safari6])
-          expect(policy.value).to eq("default-src 'self'; connect-src 'self'; font-src 'self'; frame-src 'self'; img-src 'self'; media-src 'self'; object-src 'self'; sandbox 'self'; script-src 'self' 'unsafe-inline'; style-src 'self'; report-uri 'self'")
+          expect(policy.value).to eq("default-src default-src.com; connect-src connect-src.com; font-src font-src.com; frame-src child-src.com; img-src img-src.com; media-src media-src.com; object-src object-src.com; sandbox sandbox.com; script-src script-src.com 'unsafe-inline'; style-src style-src.com; report-uri report-uri.com")
         end
       end
     end

spec/lib/secure_headers/headers/policy_management_spec.rb

@@ -23,6 +23,7 @@ module SecureHeaders
           # directive values: these values will directly translate into source directives
           default_src: %w(https: 'self'),
           frame_src: %w('self' *.twimg.com itunes.apple.com),
+          child_src: %w('self' *.twimg.com itunes.apple.com),
           connect_src: %w(wss:),
           font_src: %w('self' data:),
           img_src: %w(mycdn.com data:),
@@ -31,7 +32,6 @@ module SecureHeaders
           script_src: %w('self'),
           style_src: %w('unsafe-inline'),
           base_uri: %w('self'),
-          child_src: %w('self'),
           form_action: %w('self' github.com),
           frame_ancestors: %w('none'),
           plugin_types: %w(application/x-shockwave-flash),

spec/lib/secure_headers_spec.rb

@@ -90,8 +90,7 @@ module SecureHeaders
         config = Configuration.default do |config|
           config.csp = {
             default_src: %w('self'),
-            child_src: %w('self'), #unsupported by firefox
-            frame_src: %w('self')
+            child_src: %w('self')
           }
         end
         firefox_request = Rack::Request.new(request.env.merge("HTTP_USER_AGENT" => USER_AGENTS[:firefox]))
@@ -102,6 +101,8 @@ module SecureHeaders
         SecureHeaders.override_content_security_policy_directives(firefox_request, script_src: %w('self'))
 
         hash = SecureHeaders.header_hash_for(firefox_request)
+
+        # child-src is translated to frame-src
         expect(hash[CSP::HEADER_NAME]).to eq("default-src 'self'; frame-src 'self'; script-src 'self'")
       end
 

spec/spec_helper.rb

@@ -12,7 +12,8 @@
 
 USER_AGENTS = {
   edge: "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/42.0.2311.135 Safari/537.36 Edge/12.246",
-  firefox: 'Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:14.0) Gecko/20100101 Firefox/14.0.1',
+  firefox: "Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:14.0) Gecko/20100101 Firefox/14.0.1",
+  firefox46: "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:46.0) Gecko/20100101 Firefox/46.0",
   chrome: 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/536.5 (KHTML, like Gecko) Chrome/19.0.1084.56 Safari/536.5',
   ie: 'Mozilla/5.0 (compatible; MSIE 10.0; Windows NT 6.1; Trident/5.0)',
   opera: 'Opera/9.80 (Windows NT 6.1; U; es-ES) Presto/2.9.181 Version/12.00',