|
Description
Marcel de Rooy
2022-01-24 10:23:39 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> 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> 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. Created attachment 129722 [details] [review] Bug 29931: (follow-up) Similar thing in opac-patron-image.pl Although less harmful indeed. No borrowernumber, no image. 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.
(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. 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> 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> 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 (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. 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'), ...
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> 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. 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> (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. (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. 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> 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> 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> Backported to 19.11.x-security 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. (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 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> (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 (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! Thanks Marcel :) 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 Applied - [PATCH] Bug 29931: [21.05.x] Check cookie status before continuing - to 19.11.x-security (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? 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.
Backported: Pushed to 20.11.x-security branch for 20.11.16 Pushed to master for 22.05 |