From 0976c8fad6970b6182e7837bf87de07709407f25 Mon Sep 17 00:00:00 2001 From: OsamaSayegh Date: Fri, 28 Jul 2023 12:53:44 +0100 Subject: [PATCH] SECURITY: Don't reuse CSP nonce between anonymous requests --- app/helpers/application_helper.rb | 6 ++-- .../common/_google_tag_manager_head.html.erb | 2 +- config/application.rb | 3 ++ config/initializers/099-anon-cache.rb | 5 +-- lib/content_security_policy.rb | 8 ++--- lib/content_security_policy/builder.rb | 4 +-- lib/content_security_policy/default.rb | 4 +-- lib/content_security_policy/middleware.rb | 2 -- lib/middleware/anonymous_cache.rb | 22 +++++++++++- lib/middleware/gtm_script_nonce_injector.rb | 26 ++++++++++++++ spec/lib/content_security_policy_spec.rb | 4 ++- spec/lib/middleware/anonymous_cache_spec.rb | 2 ++ spec/lib/middleware/request_tracker_spec.rb | 2 ++ spec/rails_helper.rb | 2 ++ spec/requests/application_controller_spec.rb | 35 ++++++++++++++++--- 15 files changed, 105 insertions(+), 22 deletions(-) create mode 100644 lib/middleware/gtm_script_nonce_injector.rb diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 7e3c903e3d5..de0a22b0b12 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -64,8 +64,10 @@ module ApplicationHelper google_universal_analytics_json end - def self.google_tag_manager_nonce(env) - env[:discourse_content_security_policy_nonce] ||= SecureRandom.hex + def google_tag_manager_nonce_placeholder + placeholder = "[[csp_nonce_placeholder_#{SecureRandom.hex}]]" + response.headers["Discourse-GTM-Nonce-Placeholder"] = placeholder + placeholder end def shared_session_key diff --git a/app/views/common/_google_tag_manager_head.html.erb b/app/views/common/_google_tag_manager_head.html.erb index 26790ca3db7..56a77cfeed6 100644 --- a/app/views/common/_google_tag_manager_head.html.erb +++ b/app/views/common/_google_tag_manager_head.html.erb @@ -1,6 +1,6 @@ <%= preload_script 'google-tag-manager' %> diff --git a/config/application.rb b/config/application.rb index 0962bccb6ca..9664112df22 100644 --- a/config/application.rb +++ b/config/application.rb @@ -167,6 +167,9 @@ module Discourse config.middleware.swap ActionDispatch::ContentSecurityPolicy::Middleware, ContentSecurityPolicy::Middleware + require "middleware/gtm_script_nonce_injector" + config.middleware.insert_after(ActionDispatch::Flash, Middleware::GtmScriptNonceInjector) + require "middleware/discourse_public_exceptions" config.exceptions_app = Middleware::DiscoursePublicExceptions.new(Rails.public_path) diff --git a/config/initializers/099-anon-cache.rb b/config/initializers/099-anon-cache.rb index ee5e240d93a..518f3d8685c 100644 --- a/config/initializers/099-anon-cache.rb +++ b/config/initializers/099-anon-cache.rb @@ -6,10 +6,11 @@ enabled = if Rails.configuration.respond_to?(:enable_anon_caching) Rails.configuration.enable_anon_caching else - Rails.env.production? + Rails.env.production? || Rails.env.test? end if !ENV["DISCOURSE_DISABLE_ANON_CACHE"] && enabled # in an ideal world this is position 0, but mobile detection uses ... session and request and params - Rails.configuration.middleware.insert_after ActionDispatch::Flash, Middleware::AnonymousCache + Rails.configuration.middleware.insert_after Middleware::GtmScriptNonceInjector, + Middleware::AnonymousCache end diff --git a/lib/content_security_policy.rb b/lib/content_security_policy.rb index 53cf16d6103..107dc0437df 100644 --- a/lib/content_security_policy.rb +++ b/lib/content_security_policy.rb @@ -4,13 +4,13 @@ require "content_security_policy/extension" class ContentSecurityPolicy class << self - def policy(theme_id = nil, env: {}, base_url: Discourse.base_url, path_info: "/") - new.build(theme_id, env: env, base_url: base_url, path_info: path_info) + def policy(theme_id = nil, base_url: Discourse.base_url, path_info: "/") + new.build(theme_id, base_url: base_url, path_info: path_info) end end - def build(theme_id, env: {}, base_url:, path_info: "/") - builder = Builder.new(base_url: base_url, env: env) + def build(theme_id, base_url:, path_info: "/") + builder = Builder.new(base_url: base_url) Extension.theme_extensions(theme_id).each { |extension| builder << extension } Extension.plugin_extensions.each { |extension| builder << extension } diff --git a/lib/content_security_policy/builder.rb b/lib/content_security_policy/builder.rb index ddcb9db67eb..e23f55111e2 100644 --- a/lib/content_security_policy/builder.rb +++ b/lib/content_security_policy/builder.rb @@ -25,8 +25,8 @@ class ContentSecurityPolicy style_src ].freeze - def initialize(base_url:, env: {}) - @directives = Default.new(base_url: base_url, env: env).directives + def initialize(base_url:) + @directives = Default.new(base_url: base_url).directives @base_url = base_url end diff --git a/lib/content_security_policy/default.rb b/lib/content_security_policy/default.rb index ff3767c51fe..a4e6f70aefc 100644 --- a/lib/content_security_policy/default.rb +++ b/lib/content_security_policy/default.rb @@ -5,9 +5,8 @@ class ContentSecurityPolicy class Default attr_reader :directives - def initialize(base_url:, env: {}) + def initialize(base_url:) @base_url = base_url - @env = env @directives = {}.tap do |directives| directives[:upgrade_insecure_requests] = [] if SiteSetting.force_https @@ -86,7 +85,6 @@ class ContentSecurityPolicy end if SiteSetting.gtm_container_id.present? sources << "https://www.googletagmanager.com/gtm.js" - sources << "'nonce-#{ApplicationHelper.google_tag_manager_nonce(@env)}'" end sources << "'#{SplashScreenHelper.fingerprint}'" if SiteSetting.splash_screen diff --git a/lib/content_security_policy/middleware.rb b/lib/content_security_policy/middleware.rb index 9c729511cab..79ec0834275 100644 --- a/lib/content_security_policy/middleware.rb +++ b/lib/content_security_policy/middleware.rb @@ -21,13 +21,11 @@ class ContentSecurityPolicy headers["Content-Security-Policy"] = policy( theme_id, - env: env, base_url: base_url, path_info: env["PATH_INFO"], ) if SiteSetting.content_security_policy headers["Content-Security-Policy-Report-Only"] = policy( theme_id, - env: env, base_url: base_url, path_info: env["PATH_INFO"], ) if SiteSetting.content_security_policy_report_only diff --git a/lib/middleware/anonymous_cache.rb b/lib/middleware/anonymous_cache.rb index 7f1fbec9a4d..d41069c92e0 100644 --- a/lib/middleware/anonymous_cache.rb +++ b/lib/middleware/anonymous_cache.rb @@ -43,6 +43,21 @@ module Middleware env["ANON_CACHE_DURATION"] = duration end + def self.clear_all_cache! + if Rails.env.production? + raise "for perf reasons, clear_all_cache! cannot be used in production." + end + Discourse.redis.keys("ANON_CACHE_*").each { |k| Discourse.redis.del(k) } + end + + def self.disable_anon_cache + @@disabled = true + end + + def self.enable_anon_cache + @@disabled = false + end + # This gives us an API to insert anonymous cache segments class Helper RACK_SESSION = "rack.session" @@ -232,7 +247,10 @@ module Middleware end def cacheable? - !!(!has_auth_cookie? && get? && no_cache_bypass) + !!( + GlobalSetting.anon_cache_store_threshold > 0 && !has_auth_cookie? && get? && + no_cache_bypass + ) end def compress(val) @@ -326,6 +344,8 @@ module Middleware PAYLOAD_INVALID_REQUEST_METHODS = %w[GET HEAD] def call(env) + return @app.call(env) if defined?(@@disabled) && @@disabled + if PAYLOAD_INVALID_REQUEST_METHODS.include?(env[Rack::REQUEST_METHOD]) && env[Rack::RACK_INPUT].size > 0 return 413, { "Cache-Control" => "private, max-age=0, must-revalidate" }, [] diff --git a/lib/middleware/gtm_script_nonce_injector.rb b/lib/middleware/gtm_script_nonce_injector.rb new file mode 100644 index 00000000000..b7fc3fda334 --- /dev/null +++ b/lib/middleware/gtm_script_nonce_injector.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +module Middleware + class GtmScriptNonceInjector + def initialize(app, settings = {}) + @app = app + end + + def call(env) + status, headers, response = @app.call(env) + + if nonce_placeholder = headers.delete("Discourse-GTM-Nonce-Placeholder") + nonce = SecureRandom.hex + parts = [] + response.each { |part| parts << part.to_s.sub(nonce_placeholder, nonce) } + %w[Content-Security-Policy Content-Security-Policy-Report-Only].each do |name| + next if headers[name].blank? + headers[name] = headers[name].sub("script-src ", "script-src 'nonce-#{nonce}' ") + end + [status, headers, parts] + else + [status, headers, response] + end + end + end +end diff --git a/spec/lib/content_security_policy_spec.rb b/spec/lib/content_security_policy_spec.rb index 87ea6713836..338a2e1e085 100644 --- a/spec/lib/content_security_policy_spec.rb +++ b/spec/lib/content_security_policy_spec.rb @@ -106,7 +106,9 @@ RSpec.describe ContentSecurityPolicy do script_srcs = parse(policy)["script-src"] expect(script_srcs).to include("https://www.googletagmanager.com/gtm.js") - expect(script_srcs.to_s).to include("nonce-") + # nonce is added by the GtmScriptNonceInjector middleware to prevent the + # nonce from getting cached by AnonymousCache + expect(script_srcs.to_s).not_to include("nonce-") end it "allowlists CDN assets when integrated" do diff --git a/spec/lib/middleware/anonymous_cache_spec.rb b/spec/lib/middleware/anonymous_cache_spec.rb index 2369f821044..33b52642daa 100644 --- a/spec/lib/middleware/anonymous_cache_spec.rb +++ b/spec/lib/middleware/anonymous_cache_spec.rb @@ -3,6 +3,8 @@ RSpec.describe Middleware::AnonymousCache do let(:middleware) { Middleware::AnonymousCache.new(lambda { |_| [200, {}, []] }) } + before { Middleware::AnonymousCache.enable_anon_cache } + def env(opts = {}) create_request_env(path: opts.delete(:path) || "http://test.com/path?bla=1").merge(opts) end diff --git a/spec/lib/middleware/request_tracker_spec.rb b/spec/lib/middleware/request_tracker_spec.rb index d3cc4f2798e..a2ee21de7df 100644 --- a/spec/lib/middleware/request_tracker_spec.rb +++ b/spec/lib/middleware/request_tracker_spec.rb @@ -677,6 +677,8 @@ RSpec.describe Middleware::RequestTracker do after { Middleware::RequestTracker.unregister_detailed_request_logger(logger) } it "can report data from anon cache" do + Middleware::AnonymousCache.enable_anon_cache + cache = Middleware::AnonymousCache.new(app([200, {}, ["i am a thing"]])) tracker = Middleware::RequestTracker.new(cache) diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 947e63b1d0b..63e010de110 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -149,6 +149,8 @@ module TestSetup BadgeGranter.disable_queue OmniAuth.config.test_mode = false + + Middleware::AnonymousCache.disable_anon_cache end end diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb index a2f461fe868..a2d86582990 100644 --- a/spec/requests/application_controller_spec.rb +++ b/spec/requests/application_controller_spec.rb @@ -637,36 +637,63 @@ RSpec.describe ApplicationController do it "when GTM is enabled it adds the same nonce to the policy and the GTM tag" do SiteSetting.content_security_policy = true + SiteSetting.content_security_policy_report_only = true SiteSetting.gtm_container_id = "GTM-ABCDEF" get "/latest" - expect(response.headers).to include("Content-Security-Policy") script_src = parse(response.headers["Content-Security-Policy"])["script-src"] + report_only_script_src = + parse(response.headers["Content-Security-Policy-Report-Only"])["script-src"] + nonce = extract_nonce_from_script_src(script_src) + report_only_nonce = extract_nonce_from_script_src(report_only_script_src) + + expect(nonce).to eq(report_only_nonce) gtm_meta_tag = Nokogiri::HTML5.fragment(response.body).css("#data-google-tag-manager").first expect(gtm_meta_tag["data-nonce"]).to eq(nonce) end - it "doesn't reuse CSP nonces between requests" do + it "doesn't reuse nonces between requests" do + global_setting :anon_cache_store_threshold, 1 + Middleware::AnonymousCache.enable_anon_cache + Middleware::AnonymousCache.clear_all_cache! + SiteSetting.content_security_policy = true + SiteSetting.content_security_policy_report_only = true SiteSetting.gtm_container_id = "GTM-ABCDEF" get "/latest" - expect(response.headers).to include("Content-Security-Policy") + expect(response.headers["X-Discourse-Cached"]).to eq("store") + expect(response.headers).not_to include("Discourse-GTM-Nonce-Placeholder") + script_src = parse(response.headers["Content-Security-Policy"])["script-src"] + report_only_script_src = + parse(response.headers["Content-Security-Policy-Report-Only"])["script-src"] + first_nonce = extract_nonce_from_script_src(script_src) + first_report_only_nonce = extract_nonce_from_script_src(report_only_script_src) + + expect(first_nonce).to eq(first_report_only_nonce) gtm_meta_tag = Nokogiri::HTML5.fragment(response.body).css("#data-google-tag-manager").first expect(gtm_meta_tag["data-nonce"]).to eq(first_nonce) get "/latest" - expect(response.headers).to include("Content-Security-Policy") + expect(response.headers["X-Discourse-Cached"]).to eq("true") + expect(response.headers).not_to include("Discourse-GTM-Nonce-Placeholder") + script_src = parse(response.headers["Content-Security-Policy"])["script-src"] + report_only_script_src = + parse(response.headers["Content-Security-Policy-Report-Only"])["script-src"] + second_nonce = extract_nonce_from_script_src(script_src) + second_report_only_nonce = extract_nonce_from_script_src(report_only_script_src) + + expect(second_nonce).to eq(second_report_only_nonce) expect(first_nonce).not_to eq(second_nonce) gtm_meta_tag = Nokogiri::HTML5.fragment(response.body).css("#data-google-tag-manager").first