Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Output only one nonce in CSP header per request #32602

Merged
merged 2 commits into from Apr 18, 2018

Conversation

@Envek
Copy link
Contributor

@Envek Envek commented Apr 17, 2018

Summary

Fixes #32597: nonces are accumulated infinetely and all accumulated nonces sent in every response, this occurs only when policy is not redefined on controller level.

Ensure that we're adding only one nonce value to the Content-Security-Policy header and does not accumulate them.

Other Information

Instead of pushing the generated nonce value to the list policy's script-src directive values on every request, a special generator object is being pushed to this list on policy (re-)declaration and called on every request. This is for keeping current API intact where Procs are called in scope of controller instance and nonce generator is given a request argument.

@rails-bot
Copy link

@rails-bot rails-bot commented Apr 17, 2018

r? @sgrif

(@rails-bot has picked a reviewer for you, use r? to override)

@Envek
Copy link
Contributor Author

@Envek Envek commented Apr 17, 2018

@rails-bot rails-bot assigned pixeltrix and unassigned sgrif Apr 17, 2018
@pixeltrix
Copy link
Member

@pixeltrix pixeltrix commented Apr 17, 2018

@Envek thanks - I thought I'd handled this scenario so I want to double check what's happening before merging

@pixeltrix
Copy link
Member

@pixeltrix pixeltrix commented Apr 17, 2018

@Envek I see where I went wrong - I'd assumed that every request got a new instance of the policy but that's obviously not the case. However I think we can just pass the nonce into the build method of the policy instance. Since the default of nil has no effect then changing the method signature will be fine 🤞

Example implementation:

diff --git a/actionpack/lib/action_dispatch/http/content_security_policy.rb b/actionpack/lib/action_dispatch/http/content_security_policy.rb
index c1f80a1ffc..b6c928ef94 100644
--- a/actionpack/lib/action_dispatch/http/content_security_policy.rb
+++ b/actionpack/lib/action_dispatch/http/content_security_policy.rb
@@ -21,13 +21,8 @@ def call(env)
         return response if policy_present?(headers)
 
         if policy = request.content_security_policy
-          if policy.directives["script-src"]
-            if nonce = request.content_security_policy_nonce
-              policy.directives["script-src"] << "'nonce-#{nonce}'"
-            end
-          end
-
-          headers[header_name(request)] = policy.build(request.controller_instance)
+          nonce = request.content_security_policy_nonce
+          headers[header_name(request)] = policy.build(request.controller_instance, nonce)
         end
 
         response
@@ -136,7 +131,9 @@ def generate_content_security_policy_nonce
       worker_src:      "worker-src"
     }.freeze
 
-    private_constant :MAPPINGS, :DIRECTIVES
+    NONCE_DIRECTIVES = %w[script-src]
+
+    private_constant :MAPPINGS, :DIRECTIVES, :NONCE_DIRECTIVES
 
     attr_reader :directives
 
@@ -205,8 +202,8 @@ def upgrade_insecure_requests(enabled = true)
       end
     end
 
-    def build(context = nil)
-      build_directives(context).compact.join("; ")
+    def build(context = nil, nonce = nil)
+      build_directives(context, nonce).compact.join("; ")
     end
 
     private
@@ -229,10 +226,14 @@ def apply_mapping(source)
         end
       end
 
-      def build_directives(context)
+      def build_directives(context, nonce)
         @directives.map do |directive, sources|
           if sources.is_a?(Array)
-            "#{directive} #{build_directive(sources, context).join(' ')}"
+            if nonce && nonce_directive?(directive)
+              "#{directive} #{build_directive(sources, context).join(' ')} 'nonce-#{nonce}'"
+            else
+              "#{directive} #{build_directive(sources, context).join(' ')}"
+            end
           elsif sources
             directive
           else
@@ -261,5 +262,9 @@ def resolve_source(source, context)
           raise RuntimeError, "Unexpected content security policy source: #{source.inspect}"
         end
       end
+
+      def nonce_directive?(directive)
+        NONCE_DIRECTIVES.include?(directive)
+      end
   end
 end
diff --git a/actionpack/test/dispatch/content_security_policy_test.rb b/actionpack/test/dispatch/content_security_policy_test.rb
index 95fce39dad..c2872e1e6b 100644
--- a/actionpack/test/dispatch/content_security_policy_test.rb
+++ b/actionpack/test/dispatch/content_security_policy_test.rb
@@ -241,6 +241,73 @@ def test_raises_runtime_error_when_unexpected_source
   end
 end
 
+class DefaultContentSecurityPolicyIntegrationTest < ActionDispatch::IntegrationTest
+  class PolicyController < ActionController::Base
+    def index
+      head :ok
+    end
+  end
+
+  ROUTES = ActionDispatch::Routing::RouteSet.new
+  ROUTES.draw do
+    scope module: "default_content_security_policy_integration_test" do
+      get "/", to: "policy#index"
+    end
+  end
+
+  POLICY = ActionDispatch::ContentSecurityPolicy.new do |p|
+    p.default_src :self
+    p.script_src  :https
+  end
+
+  class PolicyConfigMiddleware
+    def initialize(app)
+      @app = app
+    end
+
+    def call(env)
+      env["action_dispatch.content_security_policy"] = POLICY
+      env["action_dispatch.content_security_policy_nonce_generator"] = proc { "iyhD0Yc0W+c=" }
+      env["action_dispatch.content_security_policy_report_only"] = false
+      env["action_dispatch.show_exceptions"] = false
+
+      @app.call(env)
+    end
+  end
+
+  APP = build_app(ROUTES) do |middleware|
+    middleware.use PolicyConfigMiddleware
+    middleware.use ActionDispatch::ContentSecurityPolicy::Middleware
+  end
+
+  def app
+    APP
+  end
+
+  def test_adds_nonce_to_script_src_content_security_policy_only_once
+    get "/"
+    get "/"
+    assert_policy "default-src 'self'; script-src https: 'nonce-iyhD0Yc0W+c='"
+  end
+
+  private
+
+    def assert_policy(expected, report_only: false)
+      assert_response :success
+
+      if report_only
+        expected_header = "Content-Security-Policy-Report-Only"
+        unexpected_header = "Content-Security-Policy"
+      else
+        expected_header = "Content-Security-Policy"
+        unexpected_header = "Content-Security-Policy-Report-Only"
+      end
+
+      assert_nil response.headers[unexpected_header]
+      assert_equal expected, response.headers[expected_header]
+    end
+end
+
 class ContentSecurityPolicyIntegrationTest < ActionDispatch::IntegrationTest
   class PolicyController < ActionController::Base
     content_security_policy only: :inline do |p|
@Envek
Copy link
Contributor Author

@Envek Envek commented Apr 18, 2018

@pixeltrix, applied your diff, cleaned up my additions. Please take a look.

I'm not sure which approach is more “right” – whether pass nonce to policy from outside or calculate it inside, but your solution is shorter.

@pixeltrix
Copy link
Member

@pixeltrix pixeltrix commented Apr 18, 2018

@Envek thanks looks good 👍

The reason for doing it by passing the nonce into the build process is for thread safety reasons. If the policy itself generated the nonce then it would cause concurrency problems as the default policy is shared between threads.

@pixeltrix pixeltrix merged commit e40578b into rails:master Apr 18, 2018
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pixeltrix added a commit that referenced this pull request Apr 18, 2018
Output only one nonce in CSP header per request
@Envek Envek deleted the Envek:fix/csp-multiple-nonces branch Apr 18, 2018
pixeltrix added a commit that referenced this pull request Apr 18, 2018
bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Apr 19, 2018
Pull Request rails#32602 fixes Issue rails#32597.

[ci skip]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants