https://git.koha-community.org/Koha-community/Koha/src/branch/main/Koha/Middleware/CSRF.pm#L47 The stateful list is miss CONNECT so the ifs are skipped and we end up directly at `return $self->app->($env);` Is there a way to somehow exploit this? So having a CONNECT request that can still do a meaningful operation due to an eventual lack of enforcement of allowed methods downstream. Note: the pending patch for mojolicious CSRF support (Bug 36598) dodged this by just having an if (is_stateless) {...} else {...} instead of an if (is_stateless) {...} elsif (is_stateful) {...}
I also looked at if someone uses a custom HTTP method and by default apache and nginx don't let that through. So that would be a worry for servers having allowed other methods for another app in the same server and the config having a too wide scope.
Created attachment 166776 [details] [review] Bug 36863: Deal with non-listed methods in CSRF plack middleware
Created attachment 166864 [details] [review] Bug 36863: Deal with non-listed methods in CSRF plack middleware
Is that enough to test? Sabotage the list of stateless and stateful methods to remove either GET or POST. And confirm it makes this test fail: yarn cypress run --spec t/cypress/integration/Auth/csrf.ts With the expected message in the log "unknown or unsupported method GET"/"unknown or unsupported method POST"
This sounds only hypothetical. I don't think the CONNECT method would get through Apache to Starman, as Apache would interpret the CONNECT request as for itself, and when it does that it appears to throw a 400 error. That said, I'm doing a naïve test. At a glance, Jonathan's patch should work, but I actually don't know any way of testing it, as I can't get a CONNECT through to Starman.
> but I actually don't know any way of testing it, as I can't get a CONNECT through to Starman. That's why to test I sabotaged the list of methods to remove GET and POST, so they are treated like an unknown method. And it follows the same code path as CONNECT would if it would somehow make it way to starman. Unless there is something else that would make the test not representative?
Ehhh works for me. If you want to sign off, I think we could QA stamp this fairly quickly. I don't think this code path would ever be touched, but good to have in place anyway, as you never know what could change in the future.
Created attachment 167353 [details] [review] Bug 36863: Deal with non-listed methods in CSRF plack middleware Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 167354 [details] [review] Bug 36863: (QA follow-up): tidy up code
Created attachment 168302 [details] [review] Bug 36863: Deal with non-listed methods in CSRF plack middleware Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 168303 [details] [review] Bug 36863: (QA follow-up): tidy up code Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
I am getting this failure for 23.05.x, after backporting: "xt_find_cud_t.This branch is not supposed to have 'cud-', see bug 34478. (522)" kohadev-koha@kohadevbox:koha(23.05.x-security)$ prove xt/find-cud.t xt/find-cud.t .. 1/1 # Failed test 'This branch is not supposed to have 'cud-', see bug 34478.' # at xt/find-cud.t line 25. # got: '4' # expected: '0' # $VAR1 = [ # 'Koha/Middleware/CSRF.pm: if ( $stateless_methods{$request_method} && defined $original_op && $original_op =~ m{^cud-} ) {', # 'Koha/Middleware/CSRF.pm: $error = sprintf "Programming error - op \'%s\' must not start with \'cud-\' for %s %s (referer: %s)", $original_op,', # 'Koha/Middleware/CSRF.pm: if ( defined $req->param(\'op\') && $original_op !~ m{^cud-} ) {', # 'Koha/Middleware/CSRF.pm: $error = sprintf "Programming error - op \'%s\' must start with \'cud-\' for %s %s (referer: %s)", $original_op,' # ]; # Looks like you failed 1 test of 1. xt/find-cud.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/1 subtests Test Summary Report ------------------- xt/find-cud.t (Wstat: 256 (exited 1) Tests: 1 Failed: 1) Failed test: 1 Non-zero exit status: 1 Files=1, Tests=1, 1 wallclock secs ( 0.01 usr 0.00 sys + 0.08 cusr 0.39 csys = 0.48 CPU) Result: FAIL I had the following conflict when I backported Bug 36863: Deal with non-listed methods in CSRF plack middleware: deleted by us: Koha/Middleware/CSRF.pm
> This branch is not supposed to have 'cud Indeed, this ticket depends on bug 34478 which is only on 24.05.
This has been backported to 24.05.x-security branch for 24.05.02
Pushed for 24.11! Well done everyone, thank you!
Depends on Bug 34478 not in 23.11.x