Bugzilla – Attachment 143841 Details for
Bug 32178
query parameters in check_api_auth lets anyone assume a user id
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 32178: Remove security breach introduced in bug 31378
Bug-32178-Remove-security-breach-introduced-in-bug.patch (text/plain), 7.74 KB, created by
Kyle M Hall (khall)
on 2022-11-14 15:38:55 UTC
(
hide
)
Description:
Bug 32178: Remove security breach introduced in bug 31378
Filename:
MIME Type:
Creator:
Kyle M Hall (khall)
Created:
2022-11-14 15:38:55 UTC
Size:
7.74 KB
patch
obsolete
>From a927a44d97a693044eacf42d894b4a78ec5a63e9 Mon Sep 17 00:00:00 2001 >From: Agustin Moyano <agustinmoyano@theke.io> >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 <dcook@prosentient.com.au> > >Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> >--- > 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.30.2
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 32178
:
143799
|
143816
|
143841
|
143842
|
143899
|
144069
|
144070