CORGI-283: Tweak Content-Security-Policy to simplify ESS compliance#206
CORGI-283: Tweak Content-Security-Policy to simplify ESS compliance#206
Conversation
juspence
changed the title
|
Sorry for the late reply here. Can you please provide a link for the ESS with more details of the requirement you are trying to achieve with these changes? I think overall the changes look fine, I just want to understand the justification for the changes better. |
|
Suggestion: I'm definitely not an expert on Content Security Policy, but I visited the home page using your branch in my dev environment, and grabbed the I gave some suggestion in how it could be further improved, see this screenshot. |
Yes, I'll add details of the requirement to the Jira ticket since I don't want to paste internal links here.
Thank you! I'll add the "object-src: 'none'" directive and do another review in a bit. Note that it seems like our templates here were copied from our other tools, and we use Bootstrap / jQuery / PatternFly across all of these to make things look prettier. They come with some built-in CSS and JS niceness we use, if I'm understanding the template right. So removing the cdnjs.cloudflare.com line is technically possible, but it will make things ugly and might even break things in other places. I also don't think removing them greatly enhances security, since we have SRI hashing in place to prevent some kind of resource substitution / attacker running their own code. Removing the 'unsafe-inline' directive is not possible yet, as noted in the MR description and the comments I added in the third commit.
Yep, I just worked on this Friday while blocked on other tickets, so didn't look at pipeline before pushing (or yesterday while handling CORGI-295). I may let this sit for a while since other more urgent tasks are on my plate now, but will fix before merging. |
juspence
force-pushed
the
fix-csp-for-ess
branch
2 times, most recently
from
1f4e4bf to
a97d7f4
Compare
juspence
force-pushed
the
fix-csp-for-ess
branch
2 times, most recently
from
ba9585d to
babd20a
Compare
JimFuller-RedHat
left a comment
JimFuller-RedHat
left a comment
There was a problem hiding this comment.
Please consider adding to PR description rationale for inclusion of drf_spectacular_sidecar (as replacement for drf_spectacular) ... otherwise LGTM and as this PR has been around for a while best to get in as soon as. Nice work!
Thanks for reviewing! Unfortunately this still isn't done - I need to make more changes based on Jason's feedback above. I'll tag and request review again once this is ready. drf_spectacular_sidecar isn't replacing drf_spectacular. As noted in the third commit message, we want to avoid 'unsafe-inline' in our Content-Security-Policy, which drf-spectacular without the sidecar requires. |
juspence
force-pushed
the
fix-csp-for-ess
branch
2 times, most recently
from
a2ef8d6 to
6ed75c3
Compare
|
@RedHatProductSecurity/corgi-devs OK, round two. Sorry for the churn here - Jason's response above had some good feedback which led to some changes:
The reason to avoid 'unsafe-inline' is twofold. The first reason is that I don't think I can convince the ESS auditors that unsafe-inline is actually safe, even though DRF's serializers probably prevent injection bugs that would lead to a CSP bypass / code execution. The second reason is that after reading more on this topic, I'm not 100% sure myself that our configuration is safe. At the end of the day, a stronger CSP is better for us and easier to explain in an audit. Have a great weekend! |
juspence
force-pushed
the
fix-csp-for-ess
branch
from
6ed75c3 to
fa2040e
Compare
"cdnjs.cloudflare.com" hosts Angular libraries. If there is any HTML injection bug in Corgi, whitelisting that domain lets an attacker include the Angular libs and achieve code execution. Best practices suggest using hash- or nonce-based whitelisting instead of host-based whitelisting. But external stylesheet hashes aren't supported in Chrome / Firefox. Only inline style elements / style attributes can be allowed via a hash. External script hashes are supported in Chrome, but not in Firefox. So we list the path instead. SubResource Integrity hashes on the remote resource protect us when an attacker changes the content of the remote file on the CDN, but don't protect us from HTML injection bugs in Corgi itself (attackers just inject new dependencies with no / a valid SRI hash). Whitelisting the exact path in our CSP does prevent the above. So both together should be secure.
Jason pointed out https://csp-evaluator.withgoogle.com/ which suggests adding "object-src: 'none'". But if we add "connect-src: 'self'", we can just use "default-src: 'none'" which covers the object-src case and some others. Finally, recommended sandbox / other directives were added that don't inherit from default-src and must be explicitly specified.
juspence
force-pushed
the
fix-csp-for-ess
branch
from
fa2040e to
88922a2
Compare
4 participants
...is what I would like this MR to do, but it's not complete yet. "unsafe-inline" in our CSP config means that we aren't actually protected from Cross-Site Scripting attacks. But it seems unlikely this is exploitable.
An attacker who includes JavaScript code in a Brew build license string or Erratum description could theoretically run that JavaScript code when it's displayed in Corgi. However, we can probably rely on the Django-REST-Framework serializer to prevent this kind of code execution, and I'll note that in the ESS assessment.
I was sort of hoping to fully rule out this issue by writing stronger CSP directives, but these break Django-REST-Framework because it requires inline scripts and CSS styles. There are open PRs that fix this, but they haven't gotten much attention:
encode/django-rest-framework#7016
@RedHatProductSecurity/corgi-devs Please review. The last commit is optional and could be excluded if we like the logo even less than the console spam it fixes.