From ca57674700d020b539f5da2b05fc31df8a6a6652 Mon Sep 17 00:00:00 2001 From: Agustin Moyano Date: Fri, 11 Nov 2022 22:22:23 +0000 Subject: [PATCH] Bug 32178: Remove security breach introduced in bug 31378 This patch removes a security breach in C4::Auth::check_api_auth introduced by bug 31378, where when someone called an api with the parameters userid and auth_client_login, check_api_auth would automatically asume the user calling was that userid. This patch also introduces C4::Auth::create_basic_session(), a function that creates a session and adds the minimum basic parameters. Signed-off-by: David Cook Signed-off-by: Kyle M Hall Signed-off-by: Nick Clemens Signed-off-by: Tomas Cohen Arazi --- C4/Auth.pm | 37 +++++++++++++++++++++++++++++++----- Koha/REST/Plugin/Auth/IdP.pm | 23 +++++++--------------- Koha/REST/V1/OAuth/Client.pm | 4 ++-- t/db_dependent/Auth.t | 24 ++++++++++++++++++++++- 4 files changed, 64 insertions(+), 24 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index ac73f2d018..785291f050 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -74,7 +74,7 @@ BEGIN { @EXPORT_OK = qw( checkauth check_api_auth get_session check_cookie_auth checkpw checkpw_internal checkpw_hash get_all_subpermissions get_user_subpermissions track_login_daily in_iprange - get_template_and_user haspermission + get_template_and_user haspermission create_basic_session ); $ldap = C4::Context->config('useldapserver') || 0; @@ -1561,7 +1561,6 @@ sub check_api_auth { # new login my $userid = $query->param('userid'); my $password = $query->param('password'); - my $auth_client_login = $query->param('auth_client_login'); my ( $return, $cardnumber, $cas_ticket ); # Proxy CAS auth @@ -1571,9 +1570,6 @@ sub check_api_auth { # In case of a CAS authentication, we use the ticket instead of the password my $PT = $query->param('PT'); ( $return, $cardnumber, $userid, $cas_ticket ) = check_api_auth_cas( $PT, $query ); # EXTERNAL AUTH - } elsif ( $auth_client_login && ( $userid || $query->param('cardnumber') ) ) { - $cardnumber = $query->param('cardnumber'); - $return = 1; } else { # User / password auth @@ -1871,6 +1867,37 @@ sub get_session { return $session; } +=head2 create_basic_session + +my $session = create_basic_session({ patron => $patron, interface => $interface }); + +Creates a session and adds all basic parameters for a session to work + +=cut + +sub create_basic_session { + my $params = shift; + my $patron = $params->{patron}; + my $interface = $params->{interface}; + + my $session = get_session(""); + + $session->param( 'number', $patron->borrowernumber ); + $session->param( 'id', $patron->userid ); + $session->param( 'cardnumber', $patron->cardnumber ); + $session->param( 'firstname', $patron->firstname ); + $session->param( 'surname', $patron->surname ); + $session->param( 'branch', $patron->branchcode ); + $session->param( 'branchname', $patron->library->branchname ); + $session->param( 'flags', $patron->flags ); + $session->param( 'emailaddress', $patron->email ); + $session->param( 'ip', $session->remote_addr() ); + $session->param( 'lasttime', time() ); + $session->param( 'interface', $interface); + + return $session; +} + # FIXME no_set_userenv may be replaced with force_branchcode_for_userenv # (or something similar) diff --git a/Koha/REST/Plugin/Auth/IdP.pm b/Koha/REST/Plugin/Auth/IdP.pm index e1a0e2f593..501f7b549e 100644 --- a/Koha/REST/Plugin/Auth/IdP.pm +++ b/Koha/REST/Plugin/Auth/IdP.pm @@ -81,24 +81,15 @@ Generates a new session. $app->helper( 'auth.session' => sub { - my ( $c, $patron ) = @_; - my $userid = $patron->userid; - my $cardnumber = $patron->cardnumber; - my $cgi = CGI->new; - - $cgi->param( userid => $userid ); - $cgi->param( cardnumber => $cardnumber ); - $cgi->param( auth_client_login => 1 ); - - my ( $status, $cookie, $session_id ) = C4::Auth::check_api_auth($cgi); - - Koha::Exceptions::UnderMaintenance->throw( code => 503 ) - if $status eq "maintenance"; + my ( $c, $params ) = @_; + my $patron = $params->{patron}; + my $interface = $params->{interface}; + my $provider = $params->{provider}; - Koha::Exceptions::Auth::CannotCreateSession->throw( code => 500 ) - unless $status eq "ok"; + my $session = C4::Auth::create_basic_session({ patron => $patron, interface => $interface }); + $session->param('idp_code', $provider); - return ( $status, $cookie, $session_id ); + return $session->id; } ); } diff --git a/Koha/REST/V1/OAuth/Client.pm b/Koha/REST/V1/OAuth/Client.pm index 19079a31e1..5378cec0fe 100644 --- a/Koha/REST/V1/OAuth/Client.pm +++ b/Koha/REST/V1/OAuth/Client.pm @@ -65,7 +65,7 @@ sub login { $uri = '/cgi-bin/koha/mainpage.pl'; } - unless ( $provider_config ) { + unless ( $provider_config && $provider_config->{authorize_url} ) { my $error = "No configuration found for your provider"; return $c->redirect_to($uri."?auth_error=$error"); } @@ -99,7 +99,7 @@ sub login { ); } - my ( $status, $cookie, $session_id ) = $c->auth->session($patron); + my $session_id = $c->auth->session({ patron => $patron, interface => $interface, provider => $provider }); $c->cookie( CGISESSID => $session_id, { path => "/" } ); diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t index c8da8bd89d..197dec5d95 100755 --- a/t/db_dependent/Auth.t +++ b/t/db_dependent/Auth.t @@ -10,7 +10,7 @@ use CGI qw ( -utf8 ); use Test::MockObject; use Test::MockModule; use List::MoreUtils qw/all any none/; -use Test::More tests => 16; +use Test::More tests => 17; use Test::Warn; use t::lib::Mocks; use t::lib::TestBuilder; @@ -833,4 +833,26 @@ subtest 'Userenv clearing in check_cookie_auth' => sub { is( C4::Context->userenv, undef, 'Environment should be cleared again' ); }; +subtest 'create_basic_session tests' => sub { + plan tests => 12; + + my $patron = $builder->build_object({ class => 'Koha::Patrons' }); + my $interface = 'opac'; + + my $session = C4::Auth::create_basic_session({ patron => $patron, interface => $interface }); + + isnt($session->id, undef, 'A new sessionID was created'); + is( $session->param('number'), $patron->borrowernumber, 'Session parameter number matches' ); + is( $session->param('id'), $patron->userid, 'Session parameter id matches' ); + is( $session->param('cardnumber'), $patron->cardnumber, 'Session parameter cardnumber matches' ); + is( $session->param('firstname'), $patron->firstname, 'Session parameter firstname matches' ); + is( $session->param('surname'), $patron->surname, 'Session parameter surname matches' ); + is( $session->param('branch'), $patron->branchcode, 'Session parameter branch matches' ); + is( $session->param('branchname'), $patron->library->branchname, 'Session parameter branchname matches' ); + is( $session->param('flags'), $patron->flags, 'Session parameter flags matches' ); + is( $session->param('emailaddress'), $patron->email, 'Session parameter emailaddress matches' ); + is( $session->param('ip'), $session->remote_addr(), 'Session parameter ip matches' ); + is( $session->param('interface'), $interface, 'Session parameter interface matches' ); +}; + $schema->storage->txn_rollback; -- 2.39.5