search

github / secure_headers

Manages application of security headers with many safe defaults
MIT License
3.16k stars 252 forks source link

nonced tag helpers including nonce directive in csp has potential to break applications #470

Open pcasaretto opened 3 years ago

pcasaretto commented 3 years ago

Bugs

Nonced tag helpers including nonce directive in csp has potential to break applications

Problem

Given an application with inline script tags, and a CSP that allows them with 'unsafe-inline', using nonced_javascript_tag will cause a nonce directive to appear in the CSP header. Modern browsers will then ignore the 'unsafe-inline' directive and all other script tags without a nonce will cease to be executed.

pcasaretto commented 3 years ago

Just to be clear, I'm more than willing to submit a PR once we decided this is a problem worth solving on this gem.

oreoshake commented 3 years ago

However, as soon as we use nonced_javascript_tag on a page, the nonce directive appears on the enforcing CSP header

This sounds like more of a bug than a feature request. I would expect it to work without having to declare it. Would you only want to apply it to the report-only configuration?

Are you using both csp and csp-report-only? I could see how that use case wasn't considered/tested.

pcasaretto commented 3 years ago

Are you using both csp and csp-report-only?

That is the case, yes. As we fix issues we find in the incoming reports, we migrate the directives from csp-report-only to csp.

oreoshake commented 3 years ago

I'm more than willing to submit a PR once we decided this is a problem worth solving on this gem.

I'm more than willing to merge and release that :+1:

pcasaretto commented 3 years ago

A teammate just pointed out to me that this behavior was the cause of a recent outage. The rollbar gem recently added CSP compatibility. When we deployed that update, it included the nonce directive in the csp header breaking all other inline scripts.

What do you think is the right way forward? From my limited point of view, I'd like to remove the "magic" from the helper tags and make it their only responsibility to output tags with the proper attribute and value. Of course this means adding functionality for declaring the use of the nonce directive on the configuration setup.

pcasaretto commented 3 years ago

With your approval, I'd like to edit the issue to make it clear that the current behavior of using nonced tags with a config using 'unsafe-inline' is a bug.

oreoshake commented 3 years ago

With your approval, I'd like to edit the issue to make it clear that the current behavior of using nonced tags with a config using 'unsafe-inline' is a bug.

:+1:

pcasaretto commented 3 years ago

Done! Any suggestions regarding how to proceed?

oreoshake commented 3 years ago

Done! Any suggestions regarding how to proceed?

I would hope it would be as straightforward as pattern matching off of the existing code. I think the original proposal was to add a new helper/config option to the existing helper? It seems like you might want that functionality if you only want nonces in your report-only header.

But the simplest place to start would be a test that shows that by default, the nonce is added to both headers. Then, add the config option to specify which header the nonce should go in.

pcasaretto commented 3 years ago

add the config option to specify which header the nonce should go in

Just to be clear: by that you mean an additional config option that would be fed into SecureHeaders::Configuration.default do |config| ? What should the default behavior be? Preserve the current behavior? In that case, should we plan for a breaking change that fixes this problem?

oreoshake commented 3 years ago

Just to be clear: by that you mean an additional config option that would be fed into SecureHeaders::Configuration.default do |config| ?

🤔 I'm not sure actually. My initial thought was to add it to nonced_javascript_tag but that could be a lot of work for not a lot of value. Maybe a global config would be better.

What should the default behavior be? Preserve the current behavior?

The default behavior.

In that case, should we plan for a breaking change that fixes this problem?

What breaking change would be introduced by preserving the current behavior?

pcasaretto commented 3 years ago

What breaking change would be introduced by preserving the current behavior?

Allow me rephrase the question. Given that

oreoshake commented 3 years ago

It still seems like we might not be on the same page.

the current behavior is dangerous, and might break apps that inadvertedly use nonced_javascript_tag

Can you elaborate on "dangerous?" It seems to me the current functionality isn't desirable for your use case but it behaves consistently and hasn't been an issue until now. You're reasonably surprised that this functionality isn't supported, but I can't think of a scenario where something becomes inadvertently broken today.

we also don't want to change the current behavior without signaling this using semantic versioning. Is it in your plans to prepare a new major version that does not include this bug?

I'm not planning to because I don't think we should change the default behavior. The current behavior doesn't address your use case but we can work around that without affecting every other installation. If I could rewrite history, I wish this would have been addressed. But forcing everyone to update their configs, even if it's easy, for this edge case, doesn't seem like the best experience for those using this gem nor will it provide any value to almost everyone.

pcasaretto commented 3 years ago

The following scenario hapenned in production with us:

It caught us offguard because of the recent update I mentioned earlier in the thread. While in this case, it was another gem that caused the issue, it's easy to imagine a scenario where the developers themselves add a nonced_javascript_tag to a page without replacing all occurrences of inline scripts.

oreoshake commented 3 years ago

I can appreciate that and I hope that the surprise was discovered during pre-production testing but again, I'm not seeing a good enough justification for affecting anyone who depends on the current behavior. Is it surprising and unfortunate, yes, but it's easily discoverable.

pcasaretto commented 3 years ago

Alas, it was not. Moving forward then, let me reiterate what I understood from your recommendations is that we are to add a global config to control where the nonced directive will appear. Is that correct? If so, with that options? enforce, report-only, none? And what would be a good name for this config? Is a README warning about this specific condition warranted?

oreoshake commented 3 years ago

If so, with that options? enforce, report-only, none? And what would be a good name for this config?

That sounds good to me. Is the none use case for transitioning from nonces to hashes?

And what would be a good name for this config?

🤔 naming is hard. nonces_applied_to? apply_nonces_for? nonce_mode? How do any of those sound to you?

This kinda points to a related problem in that this setting would apply to script and style nonces alike, but thankfully nobody actually uses style nonces in a non-trivial scenario (right?).

Is a README warning about this specific condition warranted?

I've added a small note to https://github.com/github/secure_headers/blob/main/docs/per_action_configuration.md#nonce. When we have the new functionality, we can specify the options.