Bug 16929: Prevent opac-memberentry waiting for random chars
authorMarcel de Rooy <m.de.rooy@rijksmuseum.nl>
Fri, 15 Jul 2016 12:16:07 +0000 (14:16 +0200)
committerKyle M Hall <kyle@bywatersolutions.com>
Wed, 10 Aug 2016 13:25:59 +0000 (13:25 +0000)
Move calls to WWW::CSRF to Koha::Token.
Send a safe random string to WWW::CSRF instead of letting CSRF make a
blocking call to Bytes::Random::Secure. If your server has not enough
entropy, opac-memberentry will hang waiting for more characters in
dev/random. Koha::Token uses Bytes::Random::Secure with the NonBlocking
flag.

Test plan:
[1] Do not yet apply this patch.
[2] If your server has not enough entropy, calling opac-memberentry may
    take a while. But this not may be the case for you (no worries).
[3] Apply this patch.
[4] Verify that opac-memberentry still works as expected.
[5] Run t/Token.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Yes, my server had entropy trouble (reason for finding the problem).
This patch resolves the delay.

Tested all 3 patches together, works as expected.
Signed-off-by: Marc <veron@veron.ch>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Koha/Token.pm [new file with mode: 0644]
opac/opac-memberentry.pl
t/Token.t [new file with mode: 0644]

diff --git a/Koha/Token.pm b/Koha/Token.pm
new file mode 100644 (file)
index 0000000..ddec453
--- /dev/null
@@ -0,0 +1,161 @@
+package Koha::Token;
+
+# Created as wrapper for CSRF tokens, but designed for more general use
+
+# Copyright 2016 Rijksmuseum
+#
+# This file is part of Koha.
+#
+# Koha is free software; you can redistribute it and/or modify it under the
+# terms of the GNU General Public License as published by the Free Software
+# Foundation; either version 3 of the License, or (at your option) any later
+# version.
+#
+# Koha is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE.  See the GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with Koha; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+=head1 NAME
+
+Koha::Token - Tokenizer
+
+=head1 SYNOPSIS
+
+    use Koha::Token;
+    my $tokenizer = Koha::Token->new;
+    my $token = $tokenizer->generate({ length => 20 });
+
+    # safely generate a CSRF token (nonblocking)
+    my $csrf_token = $tokenizer->generate({
+        CSRF => 1, id => $id, secret => $secret,
+    });
+
+    # or check a CSRF token
+    my $result = $tokenizer->check({
+        CSRF => 1, id => $id, secret => $secret, token => $token,
+    });
+
+=head1 DESCRIPTION
+
+    Designed for providing general tokens.
+    Created due to the need for a nonblocking call to Bytes::Random::Secure
+    when generating a CSRF token.
+
+=cut
+
+use Modern::Perl;
+use base qw(Class::Accessor);
+use constant HMAC_SHA1_LENGTH => 20;
+
+=head1 METHODS
+
+=head2 new
+
+    Create object (via Class::Accessor).
+
+=cut
+
+sub new {
+    my ( $class ) = @_;
+    return $class->SUPER::new();
+}
+
+=head2 generate
+
+    my $token = $tokenizer->generate({ length => 20 });
+    my $csrf_token = $tokenizer->generate({
+        CSRF => 1, id => $id, secret => $secret,
+    });
+
+    Generate several types of tokens. Now includes CSRF.
+    Room for future extension.
+
+=cut
+
+sub generate {
+    my ( $self, $params ) = @_;
+    if( $params->{CSRF} ) {
+        $self->{lasttoken} = _gen_csrf( $params );
+    } else {
+        $self->{lasttoken} = _gen_rand( $params );
+    }
+    return $self->{lasttoken};
+}
+
+=head2 check
+
+    my $result = $tokenizer->check({
+        CSRF => 1, id => $id, secret => $secret, token => $token,
+    });
+
+    Check several types of tokens. Now includes CSRF.
+    Room for future extension.
+
+=cut
+
+sub check {
+    my ( $self, $params ) = @_;
+    if( $params->{CSRF} ) {
+        return _chk_csrf( $params );
+    }
+    return;
+}
+
+# --- Internal routines ---
+
+sub _gen_csrf {
+
+# Since WWW::CSRF::generate_csrf_token does not use the NonBlocking
+# parameter of Bytes::Random::Secure, we are passing random bytes from
+# a non blocking source to WWW::CSRF via its Random parameter.
+
+    my ( $params ) = @_;
+    return if !$params->{id} || !$params->{secret};
+
+    require Bytes::Random::Secure;
+    require WWW::CSRF;
+
+    my $randomizer = Bytes::Random::Secure->new( NonBlocking => 1 );
+        # this is most fundamental: do not use /dev/random since it is
+        # blocking, but use /dev/urandom !
+    my $random = $randomizer->bytes( HMAC_SHA1_LENGTH );
+    my $token = WWW::CSRF::generate_csrf_token(
+        $params->{id}, $params->{secret}, { Random => $random },
+    );
+
+    return $token;
+}
+
+sub _chk_csrf {
+    my ( $params ) = @_;
+    return if !$params->{id} || !$params->{secret} || !$params->{token};
+
+    require WWW::CSRF;
+    my $csrf_status = WWW::CSRF::check_csrf_token(
+        $params->{id},
+        $params->{secret},
+        $params->{token},
+    );
+    return $csrf_status == WWW::CSRF::CSRF_OK();
+}
+
+sub _gen_rand {
+    my ( $params ) = @_;
+    my $length = $params->{length} || 1;
+    $length = 1 unless $length > 0;
+
+    require String::Random;
+    return String::Random::random_string( '.' x $length );
+}
+
+=head1 AUTHOR
+
+    Marcel de Rooy, Rijksmuseum Amsterdam, The Netherlands
+
+=cut
+
+1;
index 61cfaa1..399fea6 100755 (executable)
@@ -20,7 +20,6 @@ use Modern::Perl;
 use CGI qw ( -utf8 );
 use Digest::MD5 qw( md5_base64 md5_hex );
 use String::Random qw( random_string );
-use WWW::CSRF qw(generate_csrf_token check_csrf_token CSRF_OK);
 use HTML::Entities;
 
 use C4::Auth;
@@ -34,6 +33,7 @@ use C4::Scrubber;
 use Email::Valid;
 use Koha::DateUtils;
 use Koha::Patron::Images;
+use Koha::Token;
 
 my $cgi = new CGI;
 my $dbh = C4::Context->dbh;
@@ -182,8 +182,13 @@ if ( $action eq 'create' ) {
 elsif ( $action eq 'update' ) {
 
     my $borrower = GetMember( borrowernumber => $borrowernumber );
-    my $csrf_status = check_csrf_token($borrower->{userid}, md5_base64(C4::Context->config('pass')), scalar $cgi->param('csrf_token'));
-    die "Wrong CSRF token" unless ($csrf_status == CSRF_OK);
+    die "Wrong CSRF token"
+        unless Koha::Token->new->check({
+            CSRF   => 1,
+            id     => $borrower->{userid},
+            secret => md5_base64( C4::Context->config('pass') ),
+            token  => scalar $cgi->param('csrf_token'),
+        });
 
     my %borrower = ParseCgiForBorrower($cgi);
 
@@ -197,7 +202,11 @@ elsif ( $action eq 'update' ) {
             empty_mandatory_fields => \@empty_mandatory_fields,
             invalid_form_fields    => $invalidformfields,
             borrower               => \%borrower,
-            csrf_token             => generate_csrf_token($borrower->{userid}, md5_base64(C4::Context->config('pass'))),
+            csrf_token             => Koha::Token->new->generate({
+                CSRF   => 1,
+                id     => $borrower->{userid},
+                secret => md5_base64( C4::Context->config('pass') ),
+            }),
         );
 
         $template->param( action => 'edit' );
@@ -229,7 +238,11 @@ elsif ( $action eq 'update' ) {
                 action => 'edit',
                 nochanges => 1,
                 borrower => GetMember( borrowernumber => $borrowernumber ),
-                csrf_token => generate_csrf_token($borrower->{userid}, md5_base64(C4::Context->config('pass')))
+                csrf_token => Koha::Token->new->generate({
+                    CSRF   => 1,
+                    id     => $borrower->{userid},
+                    secret => md5_base64( C4::Context->config('pass') ),
+                }),
             );
         }
     }
@@ -249,7 +262,11 @@ elsif ( $action eq 'edit' ) {    #Display logged in borrower's data
         borrower  => $borrower,
         guarantor => scalar Koha::Patrons->find($borrowernumber)->guarantor(),
         hidden => GetHiddenFields( $mandatory, 'modification' ),
-        csrf_token => generate_csrf_token($borrower->{userid}, md5_base64(C4::Context->config('pass')))
+        csrf_token => Koha::Token->new->generate({
+            CSRF   => 1,
+            id     => $borrower->{userid},
+            secret => md5_base64( C4::Context->config('pass') ),
+        }),
     );
 
     if (C4::Context->preference('OPACpatronimages')) {
diff --git a/t/Token.t b/t/Token.t
new file mode 100644 (file)
index 0000000..642342f
--- /dev/null
+++ b/t/Token.t
@@ -0,0 +1,45 @@
+#!/usr/bin/perl
+
+# tests for Koha::Token
+
+# Copyright 2016 Rijksmuseum
+#
+# This file is part of Koha.
+#
+# Koha is free software; you can redistribute it and/or modify it under the
+# terms of the GNU General Public License as published by the Free Software
+# Foundation; either version 3 of the License, or (at your option) any later
+# version.
+#
+# Koha is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE.  See the GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with Koha; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+use Modern::Perl;
+use Test::More tests => 6;
+use Koha::Token;
+
+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 => 1, id => $id, secret => $secr });
+isnt( length($csrftoken), 0, "Token $csrftoken should not be empty" );
+
+is( $tokenizer->check, undef, "Check without any parameters" );
+my $result = $tokenizer->check({
+    CSRF => 1, id => $id, secret => $secr, token => $csrftoken,
+});
+is( $result, 1, "CSRF token verified" );
+
+$result = $tokenizer->check({
+    CSRF => 1, id => $id, secret => $secr, token => $token,
+});
+isnt( $result, 1, "This token is no CSRF token" );