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.

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

eHerkenning users with active eHerkenning session blocked by form-action CSP rules. #4205

Closed
LaurensBurger opened this issue Apr 19, 2024 · 11 comments · Fixed by #4223
Closed
Assignees
Labels
bug Something isn't working needs-backport Fix must be backported to stable release branch
Milestone

Comments

@LaurensBurger
Copy link
Collaborator

LaurensBurger commented Apr 19, 2024

Product versie / Product version

2.5.x - 2.6.x

Omschrijf het probleem / Describe the bug

When a user has selected "remember my choice" on the eHerkenning page:
image
And starts a form, the user is redirected automatically
https://brk.eid-pp.sitewithprovidercoices.com/brk/DV1CResponder
https://sitewithprovidercoices-eid-pp.sso.eherkenning.nl/brk/EIDIdPDiscoveryProvider
https://brk.eid-pp.sitewithprovidercoices.com/brk/HM1CServiceProvider
to:
https://admr-a.providerA.nl/ad113_preprod/process
or any other of the providers listed here

But this is not allowed in the CSP from-action rules. They are only set for the "sitewithprovidercoices" domein and not for any of the redirects that are triggered.

This leads to the user being stuck, untill they remove the sessions/cookies and retry.

(can't reproduce this behavior on signicat environments)

@LaurensBurger LaurensBurger added bug Something isn't working triage Issue needs to be validated. Remove this label if the issue considered valid. labels Apr 19, 2024
@joeribekker
Copy link
Contributor

@LaurensBurger can you tell us which domains we need to whitelist? I can't make heads or tails from your example domains.

@alextreme
Copy link
Contributor

Discussed with Laurens. The problem here is that each preprod/prod domain of an eHerkenningsmiddelenleverancier needs to be added individually to the CSP formaction list because OF initiates the redirect to that supplier. We don't know in advance for which domains this is necessary. This is because there are quite a few suppliers/domains and these can change without us knowing.

CSP form-action isn't a mandary part of the DigiD audit, however it is part of internet.nl's checks ( internetstandards/Internet.nl#325 ). So I think this is a choice between working eHerkenning or a 100% score on internet.nl.

Would it work to add a form-action wildcard via the Csp settings?

@alextreme
Copy link
Contributor

Discussed, and the domains involved are:

https://auth.digidentity.eu
https://idp.eid.kpn.com
https://ad.qv-eherkenning.nl
https://idp.etoegang.reconi.nl
https://ehlogin.we-id.nl
https://authenticatiedienst.zlogin.nl

Issue can be reproduced but only using Chrome, seems related to this issue that they implement differently than Firefox: w3c/webappsec-csp#8

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/form-action

"Warning: Whether form-action should block redirects after a form submission is debated and browser implementations of this aspect are inconsistent (e.g. Firefox 57 doesn't block the redirects whereas Chrome 63 does)."

@joeribekker joeribekker removed the triage Issue needs to be validated. Remove this label if the issue considered valid. label Apr 23, 2024
@joeribekker joeribekker added this to the Release 2.7.0 milestone Apr 23, 2024
@joeribekker joeribekker added the needs-backport Fix must be backported to stable release branch label Apr 23, 2024
@joeribekker
Copy link
Contributor

@Viicos please make sure those URLs from Alex are added by default as CSP form action for the relevant provider. Also a data migration might be nice to fix existing installs.

@alextreme
Copy link
Contributor

Note that preproduction URLs may also be affected, this isn't considered in #4223

After some discussion yesterday with Joeri, a more resilient approach in my eyes would be to have a blanket 'form-action: *' if eHerkenning is used. Afaik having a form-action CSP header isn't mandatory in the DigiD audit and whitelisting eHerkenningmakelaars on a per-domain basis feels very fragile. The form-action CSP header is used differently between browsers, so in my eyes it's better to wait until there is consensus before making use of it.

I also don't see how the form-action CSP header would make Open Formulieren safer, within OIP we don't use it. But I could be overlooking an attack vector that is relevant for Open Formulieren

@Viicos
Copy link
Contributor

Viicos commented Apr 24, 2024

whitelisting eHerkenningmakelaars on a per-domain basis feels very fragile

I had the same feeling when implementing the PR, we'll probably discuss this tomorrow morning (switching to * probably requires a small refactor)

@sergei-maertens
Copy link
Member

sergei-maertens commented Apr 26, 2024

At some point there was a request to add form-action, we need to track down where that came from.

I agree with Alex - let's just tweak our Django setting CSP_FORM_ACTION to fetch the default value from the env (config("CSP_FORM_ACTION", default=["'self'", "https:"], split=True)) so that it works, Users that want to apply a stricter CSP for form action can do so at their own risk by deploying with the envvar CSP_FORM_ACTION='self',https://<explicit-domain>

This also makes it so that for dev/debug purposes we can override this to allow redirects back to http://localhost, which is currently problematic (I run into that with the DigiD pre-prod on Chromium-based browsers because the entire redirect chain is checked)

@sergei-maertens
Copy link
Member

sergei-maertens commented Apr 26, 2024

FYI - https: is also the form-action CSP used by prod & pre-prod DigiD

@joeribekker
Copy link
Contributor

Alright, let's do this

How do we deal with the solo model if it also has form-action?

@sergei-maertens
Copy link
Member

I'm not sure what you're referring to 😬

@Viicos
Copy link
Contributor

Viicos commented Apr 29, 2024

I think @joeribekker meant the additional CSP form-action directive values provided by the DigidConfiguration/EherkenningConfiguration models. But looking at the code this is handled by our UpdateCSPMiddleware, that will append the extra values (from the CSPSetting instances, which are created/updated whenever the solo models are updated).

Viicos added a commit that referenced this issue May 3, 2024
As we now allow `https:`, there's no need to whitelist specific
URLs from the configuration models (DigiD/eHerkenning)
Viicos added a commit that referenced this issue May 6, 2024
As we now allow `https:`, there's no need to whitelist specific
URLs from the configuration models (DigiD/eHerkenning)
Viicos added a commit that referenced this issue May 6, 2024
As we now allow `https:`, there's no need to whitelist specific
URLs from the configuration models (DigiD/eHerkenning)
Viicos added a commit that referenced this issue May 7, 2024
As we now allow `https:`, there's no need to whitelist specific
URLs from the configuration models (DigiD/eHerkenning)
Viicos added a commit that referenced this issue May 7, 2024
sergei-maertens added a commit that referenced this issue May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs-backport Fix must be backported to stable release branch
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants