From e00a821a264d714b493234e968521f28fb1c216f Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Tue, 16 Jun 2026 10:12:07 -0400 Subject: [PATCH] Bug 42847: Prevent previously authenticated user After a SIP terminal logs in successfully, a later connection handled by the same worker could bypass authentication entirely. login_core() stores the account in the session before calling api_auth(), and left it in place when authentication failed. The check in raw_transport() that is meant to drop unauthenticated connections only looks at $self->{account}->{id}, which is now populated straight from the SIP config, so the connection is treated as logged in. Test Plan: 1) Create a SIP user with the username and password "term1" 2) Restart the SIP server 3) Apply the first patch 4) prove t/db_dependent/SIP/MsgType.t 5) Note the new test fails 6) Telnet to the SIP server port ( 6001 for ktd ) 5) Send a successful login: 9300CNterm1|COterm1 6) Note you get a 941 back 7) Disconnect, then telnet to localhost 6001 again 8) Send a bad login: 9300CNterm1|COterm2 9) Note you get a 940 back, but the connection stays open 10) Send a patron information request, using a real patron cardnumber: 6300120260615 110310 AOCPL|AA23529001000463 11) Note you get a 64 patron information response with the patron's data back, even though the login failed! 12) Apply the second patch 13) prove t/db_dependent/SIP/MsgType.t 14) Note the subtest now passes! 15) Restart all the things! 16) Repeat steps 4 to 10 17) Note the the patron info request returns no patron data and the connection is dropped! Signed-off-by: Nick Clemens Signed-off-by: Martin Renvoize Signed-off-by: Lucas Gass (cherry picked from commit b1f8256ab97913678aacd80c7d53deec72868876) Signed-off-by: Lucas Gass --- C4/SIP/SIPServer.pm | 11 ++++++----- C4/SIP/Sip/MsgType.pm | 20 +++++++++++++------- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/C4/SIP/SIPServer.pm b/C4/SIP/SIPServer.pm index 925d9023bf..328db7cbbb 100644 --- a/C4/SIP/SIPServer.pm +++ b/C4/SIP/SIPServer.pm @@ -276,6 +276,7 @@ sub process_request { } $self->{account} = undef; # Clear out the account from the last request, it may be different + $self->{ils} = undef; # And the ILS handle, so a reused worker can't reuse a prior session's connection $self->{logger} = set_logger( Koha::Logger->get( { interface => 'sip' } ) ); # Flush previous MDCs to prevent accidentally leaking incorrect MDC-entries @@ -475,12 +476,12 @@ sub telnet_transport { $pwd = get_clean_string($pwd); siplog( "LOG_DEBUG", "telnet_transport 2: uid length %s, pwd length %s", length($uid), length($pwd) ); - # Check if user is authorized for SIP access, then authenticate via login_core - if ( exists( $config->{accounts}->{$uid} ) ) { + # Only log the terminal in once login_core actually authenticates it + if ( exists( $config->{accounts}->{$uid} ) + && C4::SIP::Sip::MsgType::login_core( $self, $uid, $pwd ) ) + { $account = $config->{accounts}->{$uid}; - if ( C4::SIP::Sip::MsgType::login_core( $self, $uid, $pwd ) ) { - last; - } + last; } siplog( "LOG_WARNING", "Invalid login attempt: '%s'", ( $uid || '' ) ); print("Invalid login$CRLF"); diff --git a/C4/SIP/Sip/MsgType.pm b/C4/SIP/Sip/MsgType.pm index d86c0bfbf1..6385b9577d 100644 --- a/C4/SIP/Sip/MsgType.pm +++ b/C4/SIP/Sip/MsgType.pm @@ -938,13 +938,8 @@ sub login_core { return 0; } - # Store the active account configuration - $server->{account} = $server->{config}->{accounts}->{$uid}; - my $inst = $server->{account}->{institution}; - $server->{institution} = $server->{config}->{institutions}->{$inst}; - $server->{policy} = $server->{institution}->{policy}; - $server->{sip_username} = $uid; - $server->{sip_password} = $pwd; + # Authenticate before storing the account on the session + my $inst = $server->{config}->{accounts}->{$uid}->{institution}; # Authenticate using Koha's internal authentication (checks hashed password in borrowers table) my $auth_status = api_auth( $uid, $pwd, $inst ); @@ -953,9 +948,20 @@ sub login_core { "LOG_WARNING", "Authentication failed for SIP terminal '%s' of '%s': %s", $uid, $inst, ( $auth_status || 'unknown' ) ); + + # Clear any session state, including anything left over from a previous connection on + # this preforked worker, so the caller can't act as an authenticated terminal + delete $server->{$_} foreach qw( account ils institution policy sip_username sip_password ); return 0; } + # Store the active account configuration now user has authenticated + $server->{account} = $server->{config}->{accounts}->{$uid}; + $server->{institution} = $server->{config}->{institutions}->{$inst}; + $server->{policy} = $server->{institution}->{policy}; + $server->{sip_username} = $uid; + $server->{sip_password} = $pwd; + siplog( "LOG_INFO", "Successful login/auth for '%s' of '%s'", $server->{account}->{id}, $inst ); #