Bug 36863 - CSRF Plack middleware doesn't handle the CONNECT HTTP method
Summary: CSRF Plack middleware doesn't handle the CONNECT HTTP method
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 34478
Blocks:
  Show dependency treegraph
 
Reported: 2024-05-14 20:19 UTC by Victor Grousset/tuxayo
Modified: 2024-09-10 07:47 UTC (History)
13 users (show)

See Also:
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:


Attachments
Bug 36863: Deal with non-listed methods in CSRF plack middleware (771 bytes, patch)
2024-05-15 12:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36863: Deal with non-listed methods in CSRF plack middleware (943 bytes, patch)
2024-05-17 07:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36863: Deal with non-listed methods in CSRF plack middleware (999 bytes, patch)
2024-06-03 18:11 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 36863: (QA follow-up): tidy up code (882 bytes, patch)
2024-06-03 18:11 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 36863: Deal with non-listed methods in CSRF plack middleware (891 bytes, patch)
2024-06-29 23:25 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 36863: (QA follow-up): tidy up code (940 bytes, patch)
2024-06-29 23:25 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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