Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: meta based csp #228

Closed
wants to merge 3 commits into from
Closed

feat: meta based csp #228

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fa5ee58
feat: meta based csp
davidnuescheler Jun 15, 2023
7750503
Update scripts/scripts.js
davidnuescheler Jun 19, 2023
7d428dd
chore: linting
davidnuescheler Jun 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions scripts/csp.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"default-src": [
"'self'"
],
"script-src": [
"'self'",
"https://rum.hlx.page/"
],
"connect-src": [
"'self'",
"https://rum.hlx.page/"
]
}
17 changes: 17 additions & 0 deletions scripts/scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,28 @@ async function loadEager(doc) {
}
}

/**
* sets the Content-Security-Policy meta tag to the document based on JSON file
*/

async function setCSP() {
const resp = await fetch(`${window.hlx.codeBasePath}/scripts/csp.json`);
const json = await resp.json();
Comment on lines +80 to +81
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could store the value of csp.json directly in scripts.js to avoid the additional request.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure… but still not sure why we'd do that in JS code instead of just using headers since this doesn't change much usually. It's kind of a one-time setup

Copy link

@andreituicu andreituicu Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but still not sure why we'd do that in JS code instead of just using headers since this doesn't change much usually. It's kind of a one-time setup

I completely agree that doing it via headers would be more secure. The disadvantage with JS is clear, you could always have an XSS kick in before the meta tag is set.

The only advantage that I see of the JS code is that it can be out of the box in the boilerplate and new projects would need to adapt it, so it creates awareness, that there's something they should do. We could even put a recommendation that they should move it to headers.

Alternatively, not sure how it could be done, but all new projects (to not break existing ones) could start with an out of the box CSP header, that needs to be deleted or modified.

Copy link
Collaborator

@ramboz ramboz Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm more from the "secure by default" camp, and I'd prefer having it clearly in the pre go-live checklist than letting everyone assume that the JS approach will properly protect them all the time. I think giving the community the impression that you can fully handle this client-side is setting us up for escalations down the road when someone starts exploiting the loophole. I'd also argue that this is likely an IT or sec team operation as part of the production hardening that could likely be managed at the CDN level directly by experts, and not something I'd leave up to authors or "script kiddies".

My proposal would be to do:

  1. a dedicated documentation page on this, and improve the CDN doc pages we have to cover this
  2. creating some automation for pre go-live checks, like a PSI report but for the go-live topics we want them to check
  3. clearly having those CSP set on aem.live and all reference projects we use
    • add it to the headers worksheet in the GDrive for the boilerplate
    • possibly create a dedicated sheet that mimics the JSON above and a computed value for the header if we really want to simplify the editing

Copy link
Contributor

@kptdobe kptdobe Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with the "experts" managing those at the CDN level is that they pile millions of rules, never clean it up and nobody knows what's in there. The header becomes so huge that it adds several KBs for every single request. We even had the case (several times) where it was so huge that request headers size was too large for our lambdas (more than 8KB!)...
Doing it client side would allow to cache the rules once and lighten all requests.

Why would it be less secure to do it client side ? Is this is a gut feeling or something backed with data ? If you are able to get rid of the rules if they are loaded client side, you can also get rid of the header...

Copy link

@andreituicu andreituicu Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm more from the "secure by default" camp

I feel like we are all in the same camp. I think we are simply debating what is the most efficient and effective way to make that happen, what is the best thing that we can put in place to make sure things don't remain just at documentation level.

I agree that the most secure way is to do it using http headers.

Why would it be less secure to do it client side ? Is this is a gut feeling or something backed with data ? If you are able to get rid of the rules if they are loaded client side, you can also get rid of the header...

It is more secure, because the header sets the CSP before any JS is executed.
If you set it on the client side and attacker might manage to do their XSS before the code that sets the CSP gets a chance to execute. Taking the example of the current implementation where setCSP is called in loadLazy an XSS could happen in an LCP block or something else running in loadEager, or if a customer has a script in <head> that does something before scripts.js.

I'd prefer having it clearly in the pre go-live checklist

I agree there should be an entry in the pre go-live checklist/CDN configuration docs, but I feel like if during pre-golive it is the first time a CSP is discussed, it is too late. It means that all code has already been written and tested. Setting a CSP so late means everything needs to be retested to make sure all the dynamic scripts being loaded still work.

I was thinking of the client side as a way to:

  1. Create awareness of CSPs and starting the conversation early. (e.g. dynamically loading a 3rd party script is blocked by their browser, they adjust it)
  2. Building out the CSP during the development and the golive checklist/CDN config simply saying: now move that to a header (edge delivery header or CDN header).

add it to the headers worksheet in the GDrive for the boilerplate

I agree this could work as an early signal for devs that they should start working on their CSP early and built it out during development, but only works if people actually copy that worksheet, whereas not using the boilerplate is probably less likely to happen.

like a PSI report but for the go-live topics we want them to check

btw, the PSI report is already scanning and telling us all this.

Screenshot 2024-10-21 at 12 06 16

As David above, I'm wondering if a nonce + strict-dynamic wouldn't as an out of the box solution that works for majority of customers. https://content-security-policy.com/strict-dynamic/

const directives = Object.keys(json);
const policy = directives.map((directive) => `${directive} ${json[directive].join(' ')}`).join('; ');
const meta = document.createElement('meta');
meta.setAttribute('http-equiv', 'Content-Security-Policy');
meta.setAttribute('content', policy);
document.head.appendChild(meta);
davidnuescheler marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Loads everything that doesn't need to be delayed.
* @param {Element} doc The container element
*/
async function loadLazy(doc) {
await setCSP();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd check if this is effective in blocking eagerly loaded resources that violate the CSP.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am not sure that i am really that worried about the CSPs protecting block code...
to me this probably has to be in place before we execute untrusted code, eg. martech stack, so one could argue that this is in the wrong place altogether. so to a certain extent i think i am saying i don't even know when this should be loaded, so i am not really sure that an await really makes a difference here...

i am somewhat doubtful of the allowlist based approach altogether, so maybe a nonce based strict-dynamic approach would be the right thing here, but i definitely don't know enough about the repercussions of that vis-a-vis the somewhat questionable benefit of CSPs around XSS attacks on franklin sites.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://research.google/pubs/pub45542/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... 99.34% of hosts with CSP use policies that offer no benefit against XSS.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revisiting this, I think this is right, but only the first step. The next step should be @chicharr's lightweight tag management controlling the CSP

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think setCSP should be called as the first thing in loadPage, so the CSPs can offer protection as soon as possible. The additional request to fetch the configuration could be avoided in the critical path of FCP/LCP by inlining the configuration in the function. Something like:

function setCSP() {
  const config = {
    "default-src": [
        "'self'"
    ],
    ...
  };

  const directives = Object.keys(config);
  const policy = directives.map((directive) => `${directive} ${config[directive].join(' ')}`).join('; ');
 ...
}


const main = doc.querySelector('main');
await loadBlocks(main);

Expand Down