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 <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
(cherry picked from commit b1f8256ab97913678aacd80c7d53deec72868876)
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This commit is contained in:
parent
5d742fa95c
commit
e00a821a26
|
|
@ -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");
|
||||
|
|
|
|||
|
|
@ -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 );
|
||||
|
||||
#
|
||||
|
|
|
|||
Loading…
Reference in New Issue