Bug 41662 - CSRF-vulnerability in opac-patron-consent.pl.
Summary: CSRF-vulnerability in opac-patron-consent.pl.
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major
Assignee: David Cook
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2026-01-19 15:03 UTC by Andreas Jonsson
Modified: 2026-02-24 00:58 UTC (History)
15 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Comma delimited list of Sponsors:
Crowdfunding goal: 0
Crowdfunding committed: 0
Crowdfunding contact:
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
26.05.00,25.05.07,25.11.01,24.11.12
Circulation function:


Attachments
Bug 41662: Prevent CSRF on opac-patron-consent.pl (2.74 KB, patch)
2026-01-19 23:31 UTC, David Cook
Details | Diff | Splinter Review
Bug 41662: Prevent CSRF on opac-patron-consent.pl (2.83 KB, patch)
2026-01-20 12:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Jonsson 2026-01-19 15:03:39 UTC
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
Comment 1 Marcel de Rooy 2026-01-19 15:19:00 UTC
Will have a look
Comment 2 David Cook 2026-01-19 23:21:44 UTC
I can't reproduce this on main
Comment 3 David Cook 2026-01-19 23:24:33 UTC
(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
Comment 4 David Cook 2026-01-19 23:29:25 UTC
(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.
Comment 5 David Cook 2026-01-19 23:31:32 UTC
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
Comment 6 Marcel de Rooy 2026-01-20 12:03:56 UTC
(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 ?
Comment 7 Marcel de Rooy 2026-01-20 12:19:31 UTC
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>
Comment 8 Marcel de Rooy 2026-01-20 12:19:58 UTC
Combined SO+QA, trivial patch.
Comment 9 Lucas Gass (lukeg) 2026-01-20 14:16:27 UTC
Please provide patches for all currently supported versions.
Comment 10 Wainui Witika-Park 2026-01-22 23:27:59 UTC
(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
Comment 11 Wainui Witika-Park 2026-01-23 01:49:41 UTC
(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
Comment 12 David Cook 2026-01-26 23:14:23 UTC
(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?
Comment 13 Lucas Gass (lukeg) 2026-01-29 22:19:42 UTC
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!
Comment 14 Lucas Gass (lukeg) 2026-01-29 22:23:27 UTC
Also pushed to 25.05 for 25.05.07!
Comment 15 Andreas Jonsson 2026-02-02 10:18:18 UTC
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' )  {
Comment 16 David Cook 2026-02-03 01:23:54 UTC
(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.
Comment 17 David Cook 2026-02-03 01:24:39 UTC
(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.
Comment 18 Wainui Witika-Park 2026-02-24 00:58:58 UTC
(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