Summary: | CSRF Plack middleware doesn't handle the CONNECT HTTP method | ||
---|---|---|---|
Product: | Koha | Reporter: | Victor Grousset/tuxayo <victor> |
Component: | Architecture, internals, and plumbing | Assignee: | 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
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 Bug fix, not covered in the manual - no update required. |