Bug 18124: Restrict CSRF token to user's session

Currently the CSRF token generated is based on the borrowernumber, and
is valid across user's session.
We need to restrict the CSRF token to the current session.

With this patch the CSRF token is generated concatenating the id
(borrowernumber) and the CGISESSID cookie.

Test plan:
Run t/Token.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

diff --git a/Koha/Token.pm b/Koha/Token.pm
index e60ee1f..c560fce 100644 (file)
--- a/Koha/Token.pm
+++ b/Koha/Token.pm
@@ -51,6 +51,8 @@ use Modern::Perl;
 use Bytes::Random::Secure ();
 use String::Random ();
 use WWW::CSRF ();
+use Digest::MD5 qw(md5_base64);
+use Encode qw( encode );
 use base qw(Class::Accessor);
 use constant HMAC_SHA1_LENGTH => 20;
 use constant CSRF_EXPIRY_HOURS => 8; # 8 hours instead of 7 days..
@@ -72,7 +74,7 @@ sub new {
 
     my $token = $tokenizer->generate({ length => 20 });
     my $csrf_token = $tokenizer->generate({
-        type => 'CSRF', id => $id, secret => $secret,
+        type => 'CSRF', id => $id,
     });
 
     Generate several types of tokens. Now includes CSRF.
@@ -83,7 +85,10 @@ sub new {
 sub generate {
     my ( $self, $params ) = @_;
     if( $params->{type} && $params->{type} eq 'CSRF' ) {
-        $self->{lasttoken} = _gen_csrf( $params );
+        $self->{lasttoken} = _gen_csrf( {
+            id     => Encode::encode( 'UTF-8', C4::Context->userenv->{id} . $params->{id} ),
+            secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ),
+        });
     } else {
         $self->{lasttoken} = _gen_rand( $params );
     }
@@ -98,13 +103,14 @@ sub generate {
 
 sub generate_csrf {
     my ( $self, $params ) = @_;
+    return unless $params->{id};
     return $self->generate({ %$params, type => 'CSRF' });
 }
 
 =head2 check
 
     my $result = $tokenizer->check({
-        type => 'CSRF', id => $id, secret => $secret, token => $token,
+        type => 'CSRF', id => $id, token => $token,
     });
 
     Check several types of tokens. Now includes CSRF.
@@ -156,12 +162,12 @@ sub _gen_csrf {
 
 sub _chk_csrf {
     my ( $params ) = @_;
-    return if !$params->{id} || !$params->{secret} || !$params->{token};
+    return if !$params->{id} || !$params->{token};
 
+    my $id = Encode::encode( 'UTF-8', C4::Context->userenv->{id} . $params->{id} );
+    my $secret = md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) );
     my $csrf_status = WWW::CSRF::check_csrf_token(
-        $params->{id},
-        $params->{secret},
-        $params->{token},
+        $id, $secret, $params->{token},
         { MaxAge => $params->{MaxAge} // ( CSRF_EXPIRY_HOURS * 3600 ) },
     );
     return $csrf_status == WWW::CSRF::CSRF_OK();

diff --git a/t/Token.t b/t/Token.t
index 0c56cd6..3a971a5 100644 (file)
--- a/t/Token.t
+++ b/t/Token.t
@@ -20,39 +20,72 @@
 # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
 
 use Modern::Perl;
-use Test::More tests => 8;
+use Test::More tests => 10;
 use Time::HiRes qw|usleep|;
+use C4::Context;
 use Koha::Token;
 
+C4::Context->_new_userenv('DUMMY SESSION');
+C4::Context->set_userenv(0,42,0,'firstname','surname', 'CPL', 'Library 1', 0, ', ');
+
 my $tokenizer = Koha::Token->new;
 is( length( $tokenizer->generate ), 1, "Generate without parameters" );
 my $token = $tokenizer->generate({ length => 20 });
 is( length($token), 20, "Token $token has 20 chars" );
 
-my $id = $tokenizer->generate({ length => 8 });
-my $secr = $tokenizer->generate({ length => 32 });
-my $csrftoken = $tokenizer->generate_csrf({ id => $id, secret => $secr });
+my $id = $tokenizer->generate({ lyyGength => 8 });
+my $csrftoken = $tokenizer->generate_csrf({ id => $id });
 isnt( length($csrftoken), 0, "Token $csrftoken should not be empty" );
 
 is( $tokenizer->check, undef, "Check without any parameters" );
 my $result = $tokenizer->check_csrf({
-    id => $id, secret => $secr, token => $csrftoken,
+    id => $id, token => $csrftoken,
 });
 is( $result, 1, "CSRF token verified" );
 
 $result = $tokenizer->check({
-    type => 'CSRF', id => $id, secret => $secr, token => $token,
+    type => 'CSRF', id => $id, token => $token,
 });
 isnt( $result, 1, "This token is no CSRF token" );
 
 # Test MaxAge parameter
 my $age = 1; # 1 second
 $result = $tokenizer->check_csrf({
-    id => $id, secret => $secr, token => $csrftoken, MaxAge => $age,
+    id => $id, token => $csrftoken, MaxAge => $age,
 });
 is( $result, 1, "CSRF token still valid within one second" );
 usleep $age * 1000000 * 2; # micro (millionth) seconds + 100%
 $result = $tokenizer->check_csrf({
-    id => $id, secret => $secr, token => $csrftoken, MaxAge => $age,
+    id => $id, token => $csrftoken, MaxAge => $age,
 });
 isnt( $result, 1, "CSRF token expired after one second" );
+
+subtest 'Same id (cookie CGISESSID) with an other logged in user' => sub {
+    plan tests => 2;
+    $csrftoken = $tokenizer->generate_csrf({ id => $id });
+    $result = $tokenizer->check_csrf({
+        id => $id, token => $csrftoken,
+    });
+    is( $result, 1, "CSRF token verified" );
+    C4::Context->set_userenv(0,43,0,'firstname','surname', 'CPL', 'Library 1', 0, ', ');
+    $result = $tokenizer->check_csrf({
+        id => $id, token => $csrftoken,
+    });
+    is( $result, '', "CSRF token is not verified if another logged in user is using the same id" );
+};
+
+subtest 'Same logged in user with another session (cookie CGISESSID)' => sub {
+    plan tests => 2;
+    C4::Context->set_userenv(0,42,0,'firstname','surname', 'CPL', 'Library 1', 0, ', ');
+    $csrftoken = $tokenizer->generate_csrf({ id => $id });
+    $result = $tokenizer->check_csrf({
+        id => $id, token => $csrftoken,
+    });
+    is( $result, 1, "CSRF token verified" );
+    # Get another session id
+    $id = $tokenizer->generate({ lyyGength => 8 });
+    $result = $tokenizer->check_csrf({
+        id => $id, token => $csrftoken,
+    });
+    is( $result, '', "CSRF token is not verified if another session is used" );
+};