From 0c6ef38fb385c5dc95c1cf1d0a5e8bba812ae3e1 Mon Sep 17 00:00:00 2001 From: Julian Maurice Date: Tue, 14 May 2024 09:15:50 +0200 Subject: [PATCH] Bug 36598: Prevent use of unsafe HTTP method with non-cud op parameter Signed-off-by: Victor Grousset/tuxayo Signed-off-by: Marcel de Rooy Signed-off-by: Lucas Gass --- Koha/App/Plugin/CSRF.pm | 11 ++++++++++- t/db_dependent/mojo/csrf.t | 26 ++++++++++++++++++++------ 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/Koha/App/Plugin/CSRF.pm b/Koha/App/Plugin/CSRF.pm index ab84834919..01ecb3897d 100644 --- a/Koha/App/Plugin/CSRF.pm +++ b/Koha/App/Plugin/CSRF.pm @@ -49,6 +49,10 @@ If the HTTP request method is safe (GET, HEAD, OPTIONS, TRACE) and the request contains an `op` parameter whose value starts with "cud-" (which means it's a Create, Update or Delete operation), it will immediately return a 400 response. +If the HTTP request method is unsafe (POST, PUT, DELETE, PATCH, CONNECT) and +the request contains an `op` parameter whose value does not start with "cud-", +it will immediately return a 400 response. + If the HTTP request method is unsafe (POST, PUT, DELETE, PATCH, CONNECT) and the CGISESSID cookie is set, the CSRF token is checked. A 403 response is immediately returned if the CSRF token is missing or invalid. @@ -69,9 +73,14 @@ sub register { if ( $method eq 'GET' || $method eq 'HEAD' || $method eq 'OPTIONS' || $method eq 'TRACE' ) { my $op = $c->req->param('op'); if ( $op && $op =~ /^cud-/ ) { - return $c->reply->exception('Incorrect use of a safe HTTP method with a "cud-" op parameter')->rendered(400); + return $c->reply->exception('Incorrect use of a safe HTTP method with an `op` parameter that starts with "cud-"')->rendered(400); } } else { + my $op = $c->req->param('op'); + if ( $op && $op !~ /^cud-/ ) { + return $c->reply->exception('Incorrect use of an unsafe HTTP method with an `op` parameter that does not start with "cud-"')->rendered(400); + } + if ( $c->cookie('CGISESSID') && !$self->is_csrf_valid( $c->req ) ) { return $c->reply->exception('Wrong CSRF token')->rendered(403); } diff --git a/t/db_dependent/mojo/csrf.t b/t/db_dependent/mojo/csrf.t index 4c67bfc2ef..70ee65a812 100755 --- a/t/db_dependent/mojo/csrf.t +++ b/t/db_dependent/mojo/csrf.t @@ -8,7 +8,7 @@ use Test::Mojo; use Koha::Database; subtest 'CSRF - Intranet' => sub { - plan tests => 5; + plan tests => 6; my $t = Test::Mojo->new('Koha::App::Intranet'); @@ -44,12 +44,19 @@ subtest 'CSRF - Intranet' => sub { plan tests => 3; $t->get_ok('/cgi-bin/koha/mainpage.pl?op=cud-login')->status_is(400) - ->content_like( qr/Incorrect use of a safe HTTP method with a/, 'Body contains correct error message' ); - } + ->content_like( qr/Incorrect use of a safe HTTP method with an `op` parameter that starts with "cud-"/, 'Body contains correct error message' ); + }; + + subtest 'POSTing what should be GET should fail' => sub { + plan tests => 3; + + $t->post_ok('/cgi-bin/koha/mainpage.pl?op=login')->status_is(400) + ->content_like( qr/Incorrect use of an unsafe HTTP method with an `op` parameter that does not start with "cud-"/, 'Body contains correct error message' ); + }; }; subtest 'CSRF - OPAC' => sub { - plan tests => 5; + plan tests => 6; my $t = Test::Mojo->new('Koha::App::Opac'); @@ -85,6 +92,13 @@ subtest 'CSRF - OPAC' => sub { plan tests => 3; $t->get_ok('/cgi-bin/koha/opac-user.pl?op=cud-login')->status_is(400) - ->content_like( qr/Incorrect use of a safe HTTP method with a/, 'Body contains correct error message' ); - } + ->content_like( qr/Incorrect use of a safe HTTP method with an `op` parameter that starts with "cud-"/, 'Body contains correct error message' ); + }; + + subtest 'POSTing what should be GET should fail' => sub { + plan tests => 3; + + $t->post_ok('/cgi-bin/koha/opac-user.pl?op=login')->status_is(400) + ->content_like( qr/Incorrect use of an unsafe HTTP method with an `op` parameter that does not start with "cud-"/, 'Body contains correct error message' ); + }; }; -- 2.39.5