Problem with using 'unsafe-inline' in script-src

[joukevandermaas]

We get consistent CSP reports while running the development server. From what I've been able to tell, this is because in https://github.com/rwjblue/ember-cli-content-security-policy/pull/122, a nonce value was unconditionally added to srcript-src in all apps with tests.

According to MDN, as soon as you set a nonce, 'unsafe-inline' is ignored. We were relying on 'unsafe-inline', since it's not trivial to get the hash of all inline scripts in our app at build time.

I have to admit I do not understand exactly what problem https://github.com/rwjblue/ember-cli-content-security-policy/pull/122 was trying to solve. For us, it would be a good solution to omit the nonce if the srcipt-src directive contains 'unsafe-inline'.

If you like that solution, I can make a PR for it.

[jelhan]

Checked with CSP Level 3 spec to confirm the description given in MDN and it seems to be correct. If a nonce is provided, unsafe-inline should be ignored by browsers implementing CSP Level 2. Providing both acts as a fallback for browsers that don't support CSP Level 2. This is described in 6.6.3.2. Does a source list allow all inline behavior for type?. It's also captured by the example given in 8.2. Usage of "'strict-dynamic'".

The nonce is used to allow an inline script added by Ember CLI for test support. It was initially added in #88 and is shipped as part of v1.1.0. #122 is not shipped as part of a release yet. Are using master instead of a stable release?

For us, it would be a good solution to omit the nonce if the srcipt-src directive contains 'unsafe-inline'.

If you like that solution, I can make a PR for it.

This is the correct solution in my opinion. Especially cause the nonce is not needed in that case. Would be awesome if you could provide a PR.

[joukevandermaas]

Are using master instead of a stable release?

I'm using 1.1.1, I guess I got confused since I thought that the nonce would not be added in development builds without #122. Either way #122 didn't fix the problem for us :smile:

Would be awesome if you could provide a PR.

Great, I will try to find some time for this today.

[sandstrom]

For us, it would be a good solution to omit the nonce if the srcipt-src directive contains 'unsafe-inline'.

Although this would solve OPs problem, I'm not sure this is a good idea. At least not for any environment except the test-environment. Otherwise people risk ending up with an unexpected CSP.

@jelhan What was the reason you wanted to add the static nonce to all tests in #122? I remember there was a good reason for it, but can't recall now.

[jelhan]

For us, it would be a good solution to omit the nonce if the srcipt-src directive contains 'unsafe-inline'.

Although this would solve OPs problem, I'm not sure this is a good idea. At least not for any environment except the test-environment. Otherwise people risk ending up with an unexpected CSP.

@jelhan What was the reason you wanted to add the static nonce to all tests in #122? I remember there was a good reason for it, but can't recall now.

Let me give some context.

Ember CLI adds an inline script into tests/index.html at build-time. This script asserts that the tests are actually loaded. It looks like this:

<script>Ember.assert('The tests file was not loaded. Make sure your tests index.html includes "assets/tests.js".', EmberENV.TESTS_FILE_LOADED);</script>

Such an inline script is forbidden by default CSP provided by this addon. It would require unsafe-inline directive or a hash-source matching that string to not violate the CSP. This was causing a CSP violation with default configuration for every test run.

88 fixed that CSP violation with default configuration by adding a nonce to the default CSP and to the script inserted by Ember CLI for test builds. That implementation was slightly changed in #122 but that changes are not important to the bug discussed in this ticket. @joukevandermaas is using v1.1.1 but #122 is not shipped in a release yet. #88 was shipped with v1.1.0.

When implementing #88 we missed an edge case: The inline scripted inserted by Ember CLI will not violate the CSP if script-src directive contains unsafe-inline cause that one allows all inline scripts. On the other hand adding a nonce to a script-src that contains unsafe-inline will change the CSP behavior cause unsafe-inline is ignored by browsers that supports nonce (CSP Level 2).

128 fixes that bug by not applying the nonce to CSP it script-src contains unsafe-inline. In that case it's not needed but would overrule the configured CSP.

[sandstrom]

@jelhan Ah, got it! Thanks for taking the time explaining!

I guess a long term fix might be to skip that inline script assert, or make it optional?

Anyway, I don't object to the fix suggested above, sounds reasonable!

[iamareebjamal]

I'm sorry but it still should not be added regardless of unsafe-inline or not. A static nonce may be added for the test config, but it should not be part of shipped library. Because of 2 reasons:

1. I don't know if this is the case in production mode as well because it renders the entire point of CSP null and void. If the attacker knows nonce value, it is of no use. It should be a random value updated on each request. I understand if this library cannot handle it right now, but at least it shouldn't make CSP unusable for consumers and let them handle it themselves.
2. No workaround required to fix internal tests should be shipped in the library in a way which effects the output code of the build. That should be considered an internal change. A change in the core library to fix a test which results a static nonce-abcde in the CSP header is just plain unnecessary and not only makes the site insecure but also impossible to work with due to tonnes of CSP violations.

Hence, either the library should provide random nonce value AND add those to all the script tags in the page or provide a way to do so, or shouldn't emit nonce in the header at all.

[sandstrom]

@iamareebjamal This only affects the tests. You can check the diff here: https://github.com/rwjblue/ember-cli-content-security-policy/pull/128/files

[iamareebjamal]

Not really, this issue wouldn't have been opened if that was the case. I also reached here since in our local build, the generated CSP contained nonce-abcde without any occurrence of that in our code base. And hence was rendering our CSP useless and filling the console with errors of violation of unsafe-inline

[iamareebjamal]

The PR you mentioned only changes the behavior that the nonce will not be added only if the CSP contains unsafe-inline. And I am saying that for the consumers of the library, it should never be added. I don't see any code which adds it only for the tests. The static nonce is present in the library code whereas it should be in the config of the test code.

I am on the latest released version, and even if it is fixed on the master branch(I doubt it), test config should not be present in library code, conditionally or unconditionally.

[sandstrom]

@iamareebjamal Alright, perhaps you can open a separate issue describing the problem as you see it, and your suggested fix.

Since this is an open-source project where people spend their leisure-time working on it, a PR with a fix would increase the odds of your desired change getting into master.

[iamareebjamal]

My issue is same as this one. And solution is to not include any nonce, even conditionally. Unfortunately, we are probably going to remove the library as it does more harm than good. Even with the specified behavior, it does not make it easier to deal with CSP. I just wanted to comment on the approach and notify that the behavior renders the security of CSP useless. However if we do continue to use the library, I'd be happy to contribute the fix.

But anyway, removing that test altogether will be better than adding a static nonce conditionally in the library code

[jelhan]

Different topics are mixed up here. I'm quiet sure there isn't a security issue. But lets first understand what we are talking about.

The presence of that nonce is different for latest release (v1.1.1) and current master. For v1.1 it also depends if looking at CSP served by development server or meta tag. As the development server should never be used in production that scenario could be ignored in my opinion.

For CSP served as meta tag the nonce is only added for test environment for both latest release and current master.

Latest release: https://github.com/rwjblue/ember-cli-content-security-policy/blob/4aabfa50496c720a76fa1e689ea43c04a40be875/index.js#L186-L188

Current master: https://github.com/rwjblue/ember-cli-content-security-policy/blob/0a4fdfe349b5e47120d8f4af41f09a2475822668/index.js#L86-L92

I agree that requiring such a static nonce is not good at all. But at these point of time there isn't a better solution yet. Without it an ember project created by blueprints would violate CSP. That's a very bad developer experience and was one of the reasons these addon got removed from default blueprints. It was added by myself in #88 quiet some time ago.

Actually the huge refactoring since v1.1.1 is motivated by adding an API that would allow ember-qunit to add it's script tag without violating CSP. See #67. But there is still some work that needs to be done before such a feature could be shipped. Contributions are highly welcome. 😉 Feel free to reach out to me on Discord to coordinate and discuss details.