Bug 29931

Summary: Script plugins-enable.pl should check the cookie status before running plugins
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Plugin architectureAssignee: Marcel de Rooy <m.de.rooy>
Status: Pushed to oldoldoldstable --- QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: 1joynelson, andrewfh, dcook, fridolin.somers, jonathan.druart, katrin.fischer, lucas, m.de.rooy, martin.renvoize, nick, victor, wainuiwitikapark
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29914
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.03,21.05.12,20.11.16,19.11.27
Bug Depends on: 22053    
Bug Blocks:    
Attachments: Bug 29931: Check cookie status before continuing
Bug 29931: Check cookie status before continuing
Bug 29931: (follow-up) Similar thing in opac-patron-image.pl
Bug 29931: (follow-up) Similar thing in opac-patron-image.pl
Bug 29931: (follow-up) Fix svc/checkouts and return_claims too
Bug 29931: Check cookie status before continuing
Bug 29931: (follow-up) Similar thing in opac-patron-image.pl
Bug 29931: (follow-up) Fix svc/checkouts and return_claims too
Bug 29931: Check cookie status before continuing
Bug 29931: (follow-up) Similar thing in opac-patron-image.pl
Bug 29931: (follow-up) Fix svc/checkouts and return_claims too
Bug 29931: [21.05.x] Check cookie status before continuing

Description Marcel de Rooy 2022-01-24 10:23:39 UTC
Found this while testing bug 29914
Comment 1 Marcel de Rooy 2022-01-24 10:31:31 UTC
Created attachment 129717 [details] [review]
Bug 29931: Check cookie status before continuing

Test plan:
Logout from staff.
Try to run plugins-enable (you should have some active plugin).
Like: https://yourserver:staffport/cgi-bin/koha/plugins/plugins-enable.pl?class=Koha::Plugin::Test&method=enable
Replace class and method as appropriate.
Verify that with this patch, you will be redirected to 401 page.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Jonathan Druart 2022-01-24 12:52:05 UTC
Created attachment 129721 [details] [review]
Bug 29931: Check cookie status before continuing

Test plan:
Logout from staff.
Try to run plugins-enable (you should have some active plugin).
Like: https://yourserver:staffport/cgi-bin/koha/plugins/plugins-enable.pl?class=Koha::Plugin::Test&method=enable
Replace class and method as appropriate.
Verify that with this patch, you will be redirected to 401 page.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 3 Jonathan Druart 2022-01-24 12:59:29 UTC
This one is unusual as well, $auth_status is not used later:
 38 my ($auth_status) = check_cookie_auth($sessid, $needed_flags);

Not harmful at first glance anyway.
Comment 4 Marcel de Rooy 2022-01-24 13:09:05 UTC
Created attachment 129722 [details] [review]
Bug 29931: (follow-up) Similar thing in opac-patron-image.pl

Although less harmful indeed. No borrowernumber, no image.
Comment 5 Fridolin Somers 2022-01-24 22:16:54 UTC
Looking at $auth_status usage :

1)
I also see that those files dont use it :
svc/checkouts
svc/return_claims

They look for session :
  my $session   = get_session($sessionID);
  my $userid   = $session->param('id');

If called without been authenticated, returns error :
  Can't call method "param" on an undefined value at /kohadevbox/koha/svc/checkouts line 39

Should we add like others :
if ( $auth_status ne "ok" ) {
    ...
}

2)
File opac/opac-ratings-ajax.pl looks strange.
Not sure authentication is checked in Ajax mode.

If we prefer I can open new bug reports for that.
Comment 6 Marcel de Rooy 2022-01-25 12:06:26 UTC
Looking at comment5
Comment 7 Marcel de Rooy 2022-01-25 12:21:26 UTC
(In reply to Fridolin Somers from comment #5)
> svc/checkouts
> svc/return_claims
> 
> If called without been authenticated, returns error :
>   Can't call method "param" on an undefined value at
> /kohadevbox/koha/svc/checkouts line 39

Yes it 'works' but is not nice. I will add them in a follow-up.

> File opac/opac-ratings-ajax.pl looks strange.
> Not sure authentication is checked in Ajax mode.

This script certainly needs a better authentication check but it is less trivial than the others.
The block with Koha::Ratings calls should be within a check on loggedinuser.
The whole script is a weird mix between an ajax script and a regular one.

This statement will crash on an undefined borrowernumber (SQL constraint):
  Koha::Rating->new( { biblionumber => $biblionumber, borrowernumber => $loggedinuser, rating_value => $rating_value, })->store;

I opened bug 29939 for that one. Does not seem to need the Koha security protection. It works but is bad coding.
Comment 8 Marcel de Rooy 2022-01-25 12:23:58 UTC
Created attachment 129746 [details] [review]
Bug 29931: (follow-up) Similar thing in opac-patron-image.pl

Although less harmful indeed. No borrowernumber, no image.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2022-01-25 12:24:03 UTC
Created attachment 129747 [details] [review]
Bug 29931: (follow-up) Fix svc/checkouts and return_claims too

Adding the same auth_status check here too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Jonathan Druart 2022-01-26 13:41:06 UTC
I am seeing a 404 if I hit http://kohadev.mydnsname.org:8080/cgi-bin/koha/opac-patron-image.pl

and an error in the log: Can't call method "value" on an undefined value at /kohadevbox/koha/opac/opac-patron-image.pl line 37
Comment 11 Jonathan Druart 2022-01-26 13:44:23 UTC
(In reply to Jonathan Druart from comment #10)
> I am seeing a 404 if I hit
> http://kohadev.mydnsname.org:8080/cgi-bin/koha/opac-patron-image.pl
> 
> and an error in the log: Can't call method "value" on an undefined value at
> /kohadevbox/koha/opac/opac-patron-image.pl line 37

The error in the log is from master actually.

I am not sure we should actually call check_cookie_auth in opac-patron-image.pl, we are not passing needed permissions.

It can simply return 404 if the user is not logged in.
Comment 12 Fridolin Somers 2022-01-26 22:11:55 UTC
Error in opac-patron-image.pl is on :
  my %cookies = CGI::Cookie->fetch;
  my $sessid = $cookies{'CGISESSID'}->value;

Maybe we should call like in other places :
  check_cookie_auth( $input->cookie('CGISESSID'), ...
Comment 13 Marcel de Rooy 2022-01-27 07:35:58 UTC
Created attachment 129840 [details] [review]
Bug 29931: Check cookie status before continuing

Test plan:
Logout from staff.
Try to run plugins-enable (you should have some active plugin).
Like: https://yourserver:staffport/cgi-bin/koha/plugins/plugins-enable.pl?class=Koha::Plugin::Test&method=enable
Replace class and method as appropriate.
Verify that with this patch, you will be redirected to 401 page.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Marcel de Rooy 2022-01-27 07:36:02 UTC
Created attachment 129841 [details] [review]
Bug 29931: (follow-up) Similar thing in opac-patron-image.pl

Although less harmful indeed. No borrowernumber, no image.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested: logged in, logged out, prefs toggled. All fine.
Comment 15 Marcel de Rooy 2022-01-27 07:36:06 UTC
Created attachment 129842 [details] [review]
Bug 29931: (follow-up) Fix svc/checkouts and return_claims too

Adding the same auth_status check here too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2022-01-27 07:36:32 UTC
(In reply to Fridolin Somers from comment #12)
> Error in opac-patron-image.pl is on :
>   my %cookies = CGI::Cookie->fetch;
>   my $sessid = $cookies{'CGISESSID'}->value;
> 
> Maybe we should call like in other places :
>   check_cookie_auth( $input->cookie('CGISESSID'), ...

Certainly, that is more consistent.
Comment 17 Marcel de Rooy 2022-01-27 07:37:48 UTC
(In reply to Jonathan Druart from comment #11)
> (In reply to Jonathan Druart from comment #10)
> > I am seeing a 404 if I hit
> > http://kohadev.mydnsname.org:8080/cgi-bin/koha/opac-patron-image.pl
> > 
> > and an error in the log: Can't call method "value" on an undefined value at
> > /kohadevbox/koha/opac/opac-patron-image.pl line 37
> 
> The error in the log is from master actually.
> 
> I am not sure we should actually call check_cookie_auth in
> opac-patron-image.pl, we are not passing needed permissions.
> 
> It can simply return 404 if the user is not logged in.

Fixed. Calling check_cookie_auth gives me the needed userenv.
Comment 18 Jonathan Druart 2022-01-27 08:24:37 UTC
Created attachment 129843 [details] [review]
Bug 29931: Check cookie status before continuing

Test plan:
Logout from staff.
Try to run plugins-enable (you should have some active plugin).
Like: https://yourserver:staffport/cgi-bin/koha/plugins/plugins-enable.pl?class=Koha::Plugin::Test&method=enable
Replace class and method as appropriate.
Verify that with this patch, you will be redirected to 401 page.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Jonathan Druart 2022-01-27 08:24:42 UTC
Created attachment 129844 [details] [review]
Bug 29931: (follow-up) Similar thing in opac-patron-image.pl

Although less harmful indeed. No borrowernumber, no image.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested: logged in, logged out, prefs toggled. All fine.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Jonathan Druart 2022-01-27 08:24:46 UTC
Created attachment 129845 [details] [review]
Bug 29931: (follow-up) Fix svc/checkouts and return_claims too

Adding the same auth_status check here too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 wainuiwitikapark 2022-02-15 01:14:00 UTC
Backported to 19.11.x-security
Comment 22 Andrew Fuerste-Henry 2022-03-07 18:01:03 UTC
I'm getting QA failures after applying these to 21.05.x. Can someone please take a look?
Processing files after patches
|========================>| 4 / 4 (100.00%)

 OK	opac/opac-patron-image.pl
 OK	plugins/plugins-enable.pl
 FAIL	svc/checkouts
   FAIL	  valid
		"my" variable $session masks earlier declaration in same scope 
		Global symbol "$sessionID" requires explicit package name (did you forget to declare "my $sessionID"?) 
		svc/checkouts had compilation errors.

 FAIL	svc/return_claims
   FAIL	  valid
		"my" variable $session masks earlier declaration in same scope 
		Global symbol "$sessionID" requires explicit package name (did you forget to declare "my $sessionID"?) 
		svc/return_claims had compilation errors.
Comment 23 Marcel de Rooy 2022-03-08 12:07:05 UTC
(In reply to Andrew Fuerste-Henry from comment #22)
> I'm getting QA failures after applying these to 21.05.x. Can someone please
> take a look?
> Processing files after patches
> |========================>| 4 / 4 (100.00%)
> 
>  OK	opac/opac-patron-image.pl
>  OK	plugins/plugins-enable.pl
>  FAIL	svc/checkouts
>    FAIL	  valid
> 		"my" variable $session masks earlier declaration in same scope 
> 		Global symbol "$sessionID" requires explicit package name (did you forget
> to declare "my $sessionID"?) 
> 		svc/checkouts had compilation errors.
> 
>  FAIL	svc/return_claims
>    FAIL	  valid
> 		"my" variable $session masks earlier declaration in same scope 
> 		Global symbol "$sessionID" requires explicit package name (did you forget
> to declare "my $sessionID"?) 
> 		svc/return_claims had compilation errors.

Will have a look later
Comment 24 Marcel de Rooy 2022-03-08 14:42:52 UTC
Created attachment 131483 [details] [review]
Bug 29931: [21.05.x] Check cookie status before continuing

Test plan:
Logout from staff.
Try to run plugins-enable (you should have some active plugin).
Like: https://yourserver:staffport/cgi-bin/koha/plugins/plugins-enable.pl?class=Koha::Plugin::Test&method=enable
Replace class and method as appropriate.
Verify that with this patch, you will be redirected to 401 page.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 29931: (follow-up) Similar thing in opac-patron-image.pl

Although less harmful indeed. No borrowernumber, no image.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested: logged in, logged out, prefs toggled. All fine.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 29931: (follow-up) Fix svc/checkouts and return_claims too

Adding the same auth_status check here too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 25 Marcel de Rooy 2022-03-08 14:43:35 UTC
(In reply to Andrew Fuerste-Henry from comment #22)
> I'm getting QA failures after applying these to 21.05.x. Can someone please
> take a look?

Rebased and squashed. No qa failures on the 21.05.x patch
Comment 26 Andrew Fuerste-Henry 2022-03-08 15:29:13 UTC
(In reply to Marcel de Rooy from comment #25)
> (In reply to Andrew Fuerste-Henry from comment #22)
> > I'm getting QA failures after applying these to 21.05.x. Can someone please
> > take a look?
> 
> Rebased and squashed. No qa failures on the 21.05.x patch

Thank you!
Comment 27 Victor Grousset/tuxayo 2022-03-08 21:51:48 UTC
Thanks Marcel :)
Comment 28 Victor Grousset/tuxayo 2022-03-08 22:23:44 UTC
Does anyone know how to test the patch "Check cookie status before continuing" or "Fix svc/checkouts and return_claims too" ?

For the first one I installed the kitchen-sink plugin but don't know with what class and method from the plugin I should replace the example with. And what is the before and after result.

https://github.com/bywatersolutions/dev-koha-plugin-kitchen-sink/releases
Comment 29 wainuiwitikapark 2022-03-09 01:45:13 UTC
Applied - [PATCH] Bug 29931: [21.05.x] Check cookie status before continuing - to 19.11.x-security
Comment 30 Marcel de Rooy 2022-03-09 12:07:34 UTC
(In reply to Victor Grousset/tuxayo from comment #28)
> Does anyone know how to test the patch "Check cookie status before
> continuing" or "Fix svc/checkouts and return_claims too" ?
> 
> For the first one I installed the kitchen-sink plugin but don't know with
> what class and method from the plugin I should replace the example with. And
> what is the before and after result.
> 
> https://github.com/bywatersolutions/dev-koha-plugin-kitchen-sink/releases

A simple check on the returned status should do. Do you get a 401 or does the script continue while not being logged in, and are you able to enable or disable plugins?
Comment 31 Victor Grousset/tuxayo 2022-03-10 16:07:15 UTC
Thanks, it works!

> does the script continue while not being logged in, and are you able to enable or disable plugins?

testing notes: this means installing a plugin, going to  "Home › Tools › Plugins" going to "actions" and on the enable/disable link, do right click, copy link.
Then you can when logged out use this URL to disable/enable the plugin without auth!
And with the patch, the attack doesn't work anymore.
Comment 32 Victor Grousset/tuxayo 2022-03-11 04:32:02 UTC
Backported: Pushed to 20.11.x-security branch for 20.11.16
Comment 33 Fridolin Somers 2022-03-16 08:10:50 UTC
Pushed to master for 22.05