Filtering CSP entries to prevent bypassing rules

[mvgijssel]

Consider the following Rails controller action which overwrites the frame ancestors based on some user input:

def show
  user_input_domain1 = URI.parse "https://google.com;script-src"
  user_input_domain2 = URI.parse "https://*;.;"
  user_input_domains = [user_input_domain1, user_input_domain2]

  override_content_security_policy_directives(frame_ancestors: whitelisted_domains)
end

This results into the following response header:

frame-ancestors: https://google.com;script-src *;

This shows unexpected output, because by setting the frame ancestors the user is able to change the script-src opening possibilities for XSS.

One solution to this would be to filter out the CSP rules inside of specific CSP rules:

frame_ancestor = value.gsub('script-src','').gsub('img-src', '')

[oreoshake]

Thanks for the report @mvgijssel. This is something @gregose had brought up a loooong time ago wrt to policy "injection" via semicolons.

I think the better approach would be to escape or raise errors upon seeing a semicolon mid-directive. I believe it would be better to raise an error but that could be breaking change, requiring a deprecation and major version bump.

It's easy to say "don't do that" but what you appear to be doing is absolutely supported. Accepting user input into a policy seems to bypass the whole point of CSP, but sometimes a single non-perfect opt-out can help make the other 99% of the application safer until things can be rearchitected.

I should have fixed this a looooooong time ago, tech debt!

[oreoshake]

This might be worth a security advisory as well.

[bwillis]

It's easy to say "don't do that" but what you appear to be doing is absolutely supported. Accepting user input into a policy seems to bypass the whole point of CSP, but sometimes a single non-perfect opt-out can help make the other 99% of the application safer until things can be rearchitected.

In the end your call, but it's surprising to me to be able to set a different CSP directive when you are explicitly stating frame_ancestors. I would expect that the library would prevent you from abusing it in this way, regardless of where the input is coming from. Do you have any examples of a legitimate reason to use it in this way?

[oreoshake]

I 💯 agree that this behavior should be fixed. There's no legitimate reason that I can think of.

[oreoshake]

I'm just waiting on the backport to finish building. I'll push the fixed versions for 3.x, 5.x, and 6.x and issue an advisory after.

[bwillis]

Thanks for jumping on this fix so fast @oreoshake! 🎊

[oreoshake]

I published two advisories today. One for this, and one for newline injections https://github.com/twitter/secure_headers/security/advisories

[mvgijssel]

Thanks @oreoshake!