From 50d3f8d0424f45b12d579b23a170d264a65dc862 Mon Sep 17 00:00:00 2001 From: Magnus Enger Date: Wed, 4 Sep 2024 08:13:05 +0200 Subject: [PATCH] Bug 37816: Stop SIP2 from logging passwords MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- C4/SIP/Sip.pm | 54 ++++++++++++++++++++++++++++++++++++++++++++++++--- t/SIP/Sip.t | 53 +++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 103 insertions(+), 4 deletions(-) diff --git a/C4/SIP/Sip.pm b/C4/SIP/Sip.pm index c29bff282f..6c16e88764 100644 --- a/C4/SIP/Sip.pm +++ b/C4/SIP/Sip.pm @@ -1,6 +1,9 @@ -# -# Sip.pm: General Sip utility functions -# + +=head1 NAME + +C4::SIP::Sip - General Sip utility functions + +=cut package C4::SIP::Sip; @@ -231,8 +234,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; diff --git a/t/SIP/Sip.t b/t/SIP/Sip.t index 1dc7ed1684..8f58f46bb8 100755 --- a/t/SIP/Sip.t +++ b/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" + ); +}; -- 2.39.5