Bug 37816: Stop SIP2 from logging passwords
Koha's SIP2 server does a lot of logging, including all incoming requests, in full. This means that passwords are logged, both for the user the SIP2 client uses for logging into Koha, as well as for the end users who provide a password to e.g. check something out. This patch replaces passwords with three asterisks in log strings, before they are written to the log. To test, in ktd: - Run the new tests: $ prove t/db_dependent/SIP/Sip.t - Tail the SIP2 logs: $ sudo tail -f /var/log/koha/kohadev/sip*.log - Telnet into the SIP2 server: $ telnet localhost 6001 - Try logging in by pasting this into the telnet session: "9300CNterm1|COmypassword|CPCPL|" - Verify that "mypassword" is replaced by "***" in the logs - Try different values for the password, including the correct password which is "term1" in ktd - Try other SIP2 messages that include password fields (AC, AD, CO) Update 2024-12-03: Fix issues pointed out by QA. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
This commit is contained in:
Magnus Enger
parent
57687742e3
commit
cb9c2146db
GPG key ID:
0EF6E2C03357A834
2 changed files with 103 additions and 4 deletions
|
|
@ -1,6 +1,9 @@
|
|||
#
|
||||
# Sip.pm: General Sip utility functions
|
||||
#
|
||||
|
||||
=head1 NAME
|
||||
|
||||
C4::SIP::Sip - General Sip utility functions
|
||||
|
||||
=cut
|
||||
|
||||
package C4::SIP::Sip;
|
||||
|
||||
|
|
@ -239,8 +242,53 @@ sub siplog {
|
|||
|
||||
my $message = @args ? sprintf( $mask, @args ) : $mask;
|
||||
|
||||
$message = remove_password_from_message($message);
|
||||
|
||||
my $logger = C4::SIP::Logger::get_logger();
|
||||
$logger->$method($message) if $logger;
|
||||
}
|
||||
|
||||
=head2 remove_password_from_message
|
||||
|
||||
my $safe_message = remove_password_from_message( $message );
|
||||
|
||||
Look for the fields AC, AD and CO, and replace contents with three asterisks.
|
||||
|
||||
We do this by looking for:
|
||||
|
||||
delimiter + AC/AD/CO + something + delimiter
|
||||
|
||||
and replcing it with:
|
||||
|
||||
delimiter + field name + asterisks + delimiter
|
||||
|
||||
Messages and fields that can contain passwords:
|
||||
|
||||
23 Patron Status Request - AC terminal password - AD patron password
|
||||
11 Checkout - AC terminal password - AD patron password
|
||||
09 Checkin - AC terminal password
|
||||
01 Block Patron - AC terminal password
|
||||
93 Login - CO login password
|
||||
63 Patron Information - AC terminal password - AD patron password
|
||||
35 End Patron Session - AC terminal password - AD patron password
|
||||
37 Fee Paid - AC terminal password - AD patron password
|
||||
17 Item Information - AC terminal password
|
||||
19 Item Status Update - AC terminal password
|
||||
25 Patron Enable - AC terminal password- AD patron password
|
||||
15 Hold - AC terminal password - AD patron password
|
||||
29 Renew - AC terminal password - AD patron password
|
||||
65 Renew All - AC terminal password - AD patron password
|
||||
|
||||
=cut
|
||||
|
||||
sub remove_password_from_message {
|
||||
my ($message) = @_;
|
||||
|
||||
$message =~ s/\Q${field_delimiter}\EAC.*?\Q${field_delimiter}\E/${field_delimiter}AC***${field_delimiter}/g;
|
||||
$message =~ s/\Q${field_delimiter}\EAD.*?\Q${field_delimiter}\E/${field_delimiter}AD***${field_delimiter}/g;
|
||||
$message =~ s/\Q${field_delimiter}\ECO.*?\Q${field_delimiter}\E/${field_delimiter}CO***${field_delimiter}/g;
|
||||
|
||||
return $message;
|
||||
}
|
||||
|
||||
1;
|
||||
|
|
|
|||
53
t/SIP/Sip.t
53
t/SIP/Sip.t
|
|
@ -17,7 +17,7 @@
|
|||
|
||||
use Modern::Perl;
|
||||
|
||||
use Test::More tests => 9;
|
||||
use Test::More tests => 10;
|
||||
use Test::Warn;
|
||||
|
||||
BEGIN {
|
||||
|
|
@ -55,3 +55,54 @@ warning_is { $invalidTest = C4::SIP::Sip::Checksum::verify_cksum("1234567") }
|
|||
'verify_cksum prints the expected warning for an invalid checksum';
|
||||
is( $invalidTest, 0, "Checksum: 1234567 is invalid as expected" );
|
||||
|
||||
subtest 'remove_password_from_message' => sub {
|
||||
plan tests => 8;
|
||||
|
||||
is(
|
||||
C4::SIP::Sip::remove_password_from_message("INPUT MSG: '9300CNterm1|COterm1|CPCPL|'"),
|
||||
"INPUT MSG: '9300CNterm1|CO***|CPCPL|'", "93 Login - password"
|
||||
);
|
||||
|
||||
is(
|
||||
C4::SIP::Sip::remove_password_from_message("INPUT MSG: '9300CNterm1|COt|CPCPL|'"),
|
||||
"INPUT MSG: '9300CNterm1|CO***|CPCPL|'", "93 Login - short password"
|
||||
);
|
||||
|
||||
is(
|
||||
C4::SIP::Sip::remove_password_from_message("INPUT MSG: '9300CNterm1|CO%&/()=+qwerty123456|CPCPL|'"),
|
||||
"INPUT MSG: '9300CNterm1|CO***|CPCPL|'", "93 Login - Complex password"
|
||||
);
|
||||
|
||||
is(
|
||||
C4::SIP::Sip::remove_password_from_message("INPUT MSG: '9300CNterm1|CO1234|CPCPL|'"),
|
||||
"INPUT MSG: '9300CNterm1|CO***|CPCPL|'", "93 Login - PIN"
|
||||
);
|
||||
|
||||
is(
|
||||
C4::SIP::Sip::remove_password_from_message(
|
||||
"11YN20240903 134450 AOCPL|AA23529000035676|AB39999000003697|AC1234|AD1234|BON"),
|
||||
'11YN20240903 134450 AOCPL|AA23529000035676|AB39999000003697|AC***|AD***|BON',
|
||||
"11 Checkout"
|
||||
);
|
||||
|
||||
is(
|
||||
C4::SIP::Sip::remove_password_from_message(
|
||||
"11YN20240903 134450 AOCPL|AA23529000035676|AB39999000003697|AC|AD1234|BON"),
|
||||
'11YN20240903 134450 AOCPL|AA23529000035676|AB39999000003697|AC***|AD***|BON',
|
||||
"11 Checkout - empty AC"
|
||||
);
|
||||
|
||||
is(
|
||||
C4::SIP::Sip::remove_password_from_message(
|
||||
"11YN20240903 134450 AOCPL|AA23529000035676|AB39999000003697|AC1234|AD|BON"),
|
||||
'11YN20240903 134450 AOCPL|AA23529000035676|AB39999000003697|AC***|AD***|BON',
|
||||
"11 Checkout - empty AD"
|
||||
);
|
||||
|
||||
is(
|
||||
C4::SIP::Sip::remove_password_from_message(
|
||||
"11YN20240903 134450 AOCPL|AA23529000035676|AB39999000003697|AC|AD|BON"),
|
||||
'11YN20240903 134450 AOCPL|AA23529000035676|AB39999000003697|AC***|AD***|BON',
|
||||
"11 Checkout - empty AC and AND"
|
||||
);
|
||||
};
|
||||
|
|
|
|||
Loading…
Reference in a new issue