The http method is not checked when setting patron consents on cgi-bin/koha/opac-patron-consent.pl so the consents are updated also for GET-requests and can be triggered by passing query parameters. To test set the system preference PrivacyPolicyConsent to 'permissive' and observe how the consent is persistently changed by visiting the below paths in OPAC: /cgi-bin/koha/opac-patron-consent.pl?check_GDPR_CONSENT=1 /cgi-bin/koha/opac-patron-consent.pl?check_GDPR_CONSENT=0
Will have a look
I can't reproduce this on main
(In reply to David Cook from comment #2) > I can't reproduce this on main Because it was a bad test. It should be: /cgi-bin/koha/opac-patron-consent.pl?check_GDPR_PROCESSING=0 /cgi-bin/koha/opac-patron-consent.pl?check_GDPR_PROCESSING=1
(In reply to Marcel de Rooy from comment #1) > Will have a look Sorry to steal the assignee from you Marcel. I've got a patch all ready to go here.
Created attachment 191658 [details] [review] Bug 41662: Prevent CSRF on opac-patron-consent.pl This change prevents CSRF on opac-patron-consent.pl by adding checking of the "op". Test plan: 0. Apply the patch and koha-plack --restart kohadev 1. Set system preference PrivacyPolicyConsent to "permissive" 2. Log into the OPAC and go to this path: /cgi-bin/koha/opac-patron-consent.pl?check_GDPR_PROCESSING=1 3. Note that it doesn't change the consent 4. Go to this path: /cgi-bin/koha/opac-patron-consent.pl?check_GDPR_PROCESSING=0 5. Note that it doesn't change the consent 6. Click the "Yes" radio button and click "Save" 7. Note it changes the consent to Yes 8. Click the "No" radio button and click "Save" 9. Note it changes the consent to No
(In reply to David Cook from comment #4) > (In reply to Marcel de Rooy from comment #1) > > Will have a look > > Sorry to steal the assignee from you Marcel. I've got a patch all ready to > go here. How can I report a stolen security report ?
Created attachment 191672 [details] [review] Bug 41662: Prevent CSRF on opac-patron-consent.pl This change prevents CSRF on opac-patron-consent.pl by adding checking of the "op". Test plan: 0. Apply the patch and koha-plack --restart kohadev 1. Set system preference PrivacyPolicyConsent to "permissive" 2. Log into the OPAC and go to this path: /cgi-bin/koha/opac-patron-consent.pl?check_GDPR_PROCESSING=1 3. Note that it doesn't change the consent 4. Go to this path: /cgi-bin/koha/opac-patron-consent.pl?check_GDPR_PROCESSING=0 5. Note that it doesn't change the consent 6. Click the "Yes" radio button and click "Save" 7. Note it changes the consent to Yes 8. Click the "No" radio button and click "Save" 9. Note it changes the consent to No Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Combined SO+QA, trivial patch.
Please provide patches for all currently supported versions.
(In reply to Lucas Gass (lukeg) from comment #9) > Please provide patches for all currently supported versions. +1 I cannot apply cleanly to 22.11
(In reply to Wainui Witika-Park from comment #10) > (In reply to Lucas Gass (lukeg) from comment #9) > > Please provide patches for all currently supported versions. > > +1 I cannot apply cleanly to 22.11 doesn't look like we have CSRF tokens in 22.11 so i think missing dependencies anyway
(In reply to Wainui Witika-Park from comment #11) > (In reply to Wainui Witika-Park from comment #10) > > (In reply to Lucas Gass (lukeg) from comment #9) > > > Please provide patches for all currently supported versions. > > > > +1 I cannot apply cleanly to 22.11 > > doesn't look like we have CSRF tokens in 22.11 so i think missing > dependencies anyway That's an interesting point. I suppose 22.11 would have quite a few CSRF vulnerabilities, so I suppose we just don't backport this one to it?
Pushed to main for 26.05.00 Pushed to 25.11 for 25.11.01 Pushed to 24.11 for 24.11.12 Pushed to 22.11 for 22.11.34 Great work everyone!
Also pushed to 25.05 for 25.05.07!
Sorry for not paying attention to this issue, but only look att the op parameter as this requires that this validation is made in checkauth: # FIXME This is only needed for scripts not using plack my $op = $query->param('op'); if ( defined $op && $op =~ m{^cud-} ) { die "Cannot use GET for this request" if $request_method eq 'GET'; } The comment suggest that this check may be removed at some point. Wouldn't it be safer to validate the method directly? - if ( $op && $op eq 'cud-save' ) { + if ( uc $query->request_method eq 'POST' ) {
(In reply to Andreas Jonsson from comment #15) > Sorry for not paying attention to this issue, but only look att the op > parameter as this requires that this validation is made in checkauth: > > # FIXME This is only needed for scripts not using plack > my $op = $query->param('op'); > if ( defined $op && $op =~ m{^cud-} ) { > die "Cannot use GET for this request" > if $request_method eq 'GET'; > } Since this script would be served by Plack, the Plack middleware would've already caught it. This is irrelevant. > The comment suggest that this check may be removed at some point. Wouldn't > it be safer to validate the method directly? > > - if ( $op && $op eq 'cud-save' ) { > > + if ( uc $query->request_method eq 'POST' ) { Yes, but that's not the pattern that the rest of Koha uses. It's better to be consistent than "correct". Koha::Middleware::Plack catches the GET op=cud-save issue.
(In reply to Andreas Jonsson from comment #15) > # FIXME This is only needed for scripts not using plack > my $op = $query->param('op'); > if ( defined $op && $op =~ m{^cud-} ) { > die "Cannot use GET for this request" > if $request_method eq 'GET'; > } There's only a couple of scripts that aren't handled by Plack at this point. And if anyone is running Koha without Plack in general, they should stop, because I'm pretty sure we only recommend and support Plack-enabled Koha in the community.
(In reply to Lucas Gass (lukeg) from comment #13) > Pushed to main for 26.05.00 > > Pushed to 25.11 for 25.11.01 > > Pushed to 24.11 for 24.11.12 > > Pushed to 22.11 for 22.11.34 > > > Great work everyone! This wasn't backported to 22.11