Bug 36863

Summary: CSRF Plack middleware doesn't handle the CONNECT HTTP method
Product: Koha Reporter: Victor Grousset/tuxayo <victor>
Component: Architecture, internals, and plumbingAssignee: Bugs List <koha-bugs>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: david, dcook, didier.gautheron, fridolin.somers, jonathan.druart, laurent.ducos, lisette, lucas, m.de.rooy, martin.renvoize, nick, philippe.blouin, tomascohen, wainuiwitikapark
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
24.11.00,24.05.02
Version(s) released in:
Circulation function:
Bug Depends on: 34478    
Bug Blocks:    
Attachments: Bug 36863: Deal with non-listed methods in CSRF plack middleware
Bug 36863: Deal with non-listed methods in CSRF plack middleware
Bug 36863: Deal with non-listed methods in CSRF plack middleware
Bug 36863: (QA follow-up): tidy up code
Bug 36863: Deal with non-listed methods in CSRF plack middleware
Bug 36863: (QA follow-up): tidy up code

Description Victor Grousset/tuxayo 2024-05-14 20:19:38 UTC
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) {...}
Comment 1 Victor Grousset/tuxayo 2024-05-14 22:41:23 UTC
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.
Comment 2 Jonathan Druart 2024-05-15 12:48:11 UTC
Created attachment 166776 [details] [review]
Bug 36863: Deal with non-listed methods in CSRF plack middleware
Comment 3 Jonathan Druart 2024-05-17 07:10:56 UTC
Created attachment 166864 [details] [review]
Bug 36863: Deal with non-listed methods in CSRF plack middleware
Comment 4 Victor Grousset/tuxayo 2024-05-20 22:55:40 UTC
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"
Comment 5 David Cook 2024-05-29 03:11:51 UTC
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.
Comment 6 Victor Grousset/tuxayo 2024-05-29 17:33:38 UTC
> 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?
Comment 7 David Cook 2024-05-30 02:51:16 UTC
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.
Comment 8 Victor Grousset/tuxayo 2024-06-03 18:11:44 UTC
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>
Comment 9 Victor Grousset/tuxayo 2024-06-03 18:11:48 UTC
Created attachment 167354 [details] [review]
Bug 36863: (QA follow-up): tidy up code
Comment 10 Chris Cormack 2024-06-29 23:25:14 UTC
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>
Comment 11 Chris Cormack 2024-06-29 23:25:34 UTC
Created attachment 168303 [details] [review]
Bug 36863: (QA follow-up): tidy up code

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 12 Wainui Witika-Park 2024-07-18 06:32:46 UTC
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
Comment 13 Victor Grousset/tuxayo 2024-07-18 22:34:16 UTC
> This branch is not supposed to have 'cud

Indeed, this ticket depends on bug 34478 which is only on 24.05.
Comment 14 Lucas Gass (lukeg) 2024-07-25 15:20:53 UTC
This has been backported to 24.05.x-security branch for 24.05.02
Comment 15 Katrin Fischer 2024-08-01 15:36:01 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 16 Fridolin Somers 2024-09-10 07:47:04 UTC
Depends on Bug 34478 not in 23.11.x
Comment 17 David Nind 2025-06-26 03:21:23 UTC
Bug fix, not covered in the manual - no update required.