Skip to content

CSP test for frame-src does not match explanation #643

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.

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

Closed
AlexHaan-i opened this issue Dec 1, 2021 · 6 comments · Fixed by #903
Closed

CSP test for frame-src does not match explanation #643

AlexHaan-i opened this issue Dec 1, 2021 · 6 comments · Fixed by #903
Assignees
Labels
bug Unexpected or unwanted behaviour of current implementations content
Milestone

Comments

@AlexHaan-i
Copy link

I implemented CSP headers for the first time.

(Ps. #577 would really help).

I followed the instructions to the lettter for frame-src. But frame-src www.example.org; was found to be invalid, while frame-src 'self' www.example.org; was correct. (with a different domain for www.example.org of course and other sections, this was the only difference).

The explanation writes:

en moet de waarde 'self', 'none' of een specifieke URL hebben,

Only the latter, a specific domain, is apparently invalid according to the test.

@baknu baknu added this to the v1.6 milestone May 6, 2022
@baknu baknu added bug Unexpected or unwanted behaviour of current implementations content labels May 6, 2022
@mxsasha mxsasha modified the milestones: v1.7, v1.8 Mar 7, 2023
@baknu
Copy link
Contributor

baknu commented Mar 7, 2023

Below text is from a mail from BK to SR and GT (dd 07 Dec 2022).
I suggest to choose option A.

Below some further observations and thoughts from my side.

We indeed used the DigiD norm document (https://www.norea.nl/uploads/bfile/63d1bbe9-a623-495a-93a2-007309264e06), but we did not follow it to the letter because of insights through the spec and other good practices. The DigiD document states:

Content-Security-Policy
De Content-Security-Policy (CSP) geeft de browser
instructies over welke resources vanaf welke locatie
mogen worden ingeladen en hoe deze mogen
worden gebruikt. Een CSP kan fijnmazige instructies
bevatten per soort resource, zoals afbeeldingen,
stylesheets en scripts. Bij het gebruik van een CSP
zijn standaard de uitvoering van inline scripts en de
eval()-functie uitgeschakeld
Te configureren waarden: default-src ‘self’; frame-
src ‘self’; frame-ancestors ‘self’; Sta geen onveilige
configuratie toe door het gebruik van 'unsafe-inline'
(tenzij gebruik wordt gemaakt van een nonce) en
'unsafe-eval'. Het is niet toegestaan bronnen
beginnend met http:// te whitelisten.

Below are relevant quotes from the CSP3 spec regarding fallback:

  • "The frame-src directive, which was deprecated in CSP Level 2, has been undeprecated, but continues to defer to child-src if not present (which defers to default-src in turn)."
  • "Note: The frame-ancestors directive’s syntax is similar to a source list, but frame-ancestors will not fall back to the default-src directive’s value if one is specified. That is, a policy that declares default-src 'none' will still allow the resource to be embedded by anyone."

Like described in #643, our current evaluation of frame-src and frame-ancestors is not consistent (that is allowing specific URL's only in combination with 'self'). So, indeed a change is needed.
I believe we have two possible solutions:

Option A. Make our CSP test less strict regarding framing:
We change the test and also allow frame-ancestors and frame-src to contain one or more specific URL's exclusively (like "frame-src www.example.org;").
The requirements for default-src stays as is, because "we want to provide secure defaults that a user can override with specific rules", like George stated.
This means that we must still check if frame-ancestors is explicitly present, because there is no fallback.
frame-src could be tested implicitly as it does fallback to (child-src and then to) default-src, and we require the latter to be present and to be strict. I believe the current code already uses these fallbacks, when frame-src is not there. See for example our own CSP that does not contain frame-src: https://internet.nl/site/internet.nl/1811002/#control-panel-29
For both frame-ancestors and frame-src (if present) we should still check their value against the general requirements (like not containing wildcards; see 4 to 8 in test explanation).

Option B. Make our CSP test stricter regarding framing:
Because we consider framing a bad practice, we might want to discourage it. Therefore we could treat it in a similar way as default-src. So we only allow 'none' or 'self' for both frame-ancestors and frame-src. Additionally, the domain itself, or a subdomain or superdomain could also be defined.
This seems to be more in line with the DigiD norm, but we might consider this approach to be too strict in general.
This approach seems also to be more in line with the X-Frame-Options test that now allows DENY or SAMEORIGING, and not ALLOW-FROM. However, we made the X-Frame-Options test optional as we prefer CSP (#503).

Last remark for now: we should also make sure that the above checks (and especially for frame-ancestors that does not fall back) are, as much as possible, in line with the to be added checks for form-action and base-uri (#524 and #525).

@whyscream
Copy link

whyscream commented Mar 9, 2023

I've been testing my CSP configuration for some days now, but only succeeded in getting a pass after reading the source code of the test.

A CSP header of default-src 'self'; frame-ancestors 'none'; does not pass the test, but a header of default-src 'self'; frame-ancestors 'none'; frame-src 'self; does.

This does not match the explanatory text which says "frame-src should be used (either by definition or by relying on the fallback to child-src and default-src)". In my observation, frame-src does not fall back to default-src in the current check, and an explicit definition of frame-src is required for a pass.

@mxsasha
Copy link
Collaborator

mxsasha commented Mar 16, 2023

@whyscream can you tell us which domain you saw this on? We use the fallback on our own domain which passes.

@mxsasha
Copy link
Collaborator

mxsasha commented Mar 16, 2023

Decision on the original issue: we will implement option A: for frame-src we will permit 'none', 'self', specific domain, or a combination.

@whyscream
Copy link

Hi @mxsasha , I tried to reproduce on https://internet.nl/site/internet-nl-csp-test.whyscream.net/1984363/, but it gives a pass now.
I based my report on working on https://internet.nl/site/mailadmin.whyscream.net/1984364/ (which now passes after all the fiddling I tried). There should be some (erh... many ;>) older tests for that domain, but I can't access any historic test runs, and I forgot to save the link. Maybe you can access the history?

@mxsasha
Copy link
Collaborator

mxsasha commented Mar 21, 2023

@whyscream I don't see any other hints in the code, and can not reproduce, so leaving this for now. #577 will help with these kind of things in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unexpected or unwanted behaviour of current implementations content
Development

Successfully merging a pull request may close this issue.

5 participants