|
Description
Martin Renvoize (ashimema)
2022-04-13 12:50:52 UTC
Created attachment 133249 [details] [review] Bug 30524: Core CSRF checking code Split out from bug 22990 as requested. Created attachment 133250 [details] [review] Bug 30524: Core CSRF checking code Split out from bug 22990 as requested. Created attachment 133251 [details] Bug 30524: Add xt/find-missing-csrf.t Created attachment 133420 [details] [review] Bug 30524: Core CSRF checking code Split out from bug 22990 as requested. Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 133421 [details] Bug 30524: Add xt/find-missing-csrf.t Signed-off-by: David Cook <dcook@prosentient.com.au> + $template->param( csrf_token => Koha::Token->new->generate_csrf({ session_id => scalar $sessionID }));
This is really a lazy approach. It will be generated for each page., did you benchmark it? I would prefer to generate on-demand (even if it will end with hundreds of statements) rather than producing unnecessary extra processing.
(In reply to Jonathan Druart from comment #6) > + $template->param( csrf_token => Koha::Token->new->generate_csrf({ > session_id => scalar $sessionID })); > > This is really a lazy approach. I'd like to think of it more as thorough/comprehensive than lazy. > It will be generated for each page., did you > benchmark it? I didn't but anecdotally I haven't noticed any impact. Just now the following code in my KTD indicates about .0003 to .0004 seconds to generate the token, which I would say is insignificant. use Koha::Token; use Time::HiRes qw/time/; warn time(); warn Koha::Token->new->generate_csrf({ session_id => 123456, id => 1234 }); warn time(); > I would prefer to generate on-demand (even if it will end with > hundreds of statements) rather than producing unnecessary extra processing. That sounds like it would be much more difficult to maintain though, no? I think adding .0004 seconds to processing time would be preferable to repeating ourselves many times. Marcel, what's your opinion on this? (In reply to Jonathan Druart from comment #6) > + $template->param( csrf_token => Koha::Token->new->generate_csrf({ > session_id => scalar $sessionID })); > > This is really a lazy approach. It will be generated for each page., did you > benchmark it? I would prefer to generate on-demand (even if it will end with > hundreds of statements) rather than producing unnecessary extra processing. Obviously, we dont really need the CSRF on many pages. So it is ballast. We are now checking the token in about 20 places. With about 500 templates, I think that we do not need it everywhere. (In reply to David Cook from comment #7) > I think adding .0004 seconds to processing time would be preferable to > repeating ourselves many times. Generate 5 - 6 ms, checking 1 - 2 ms. Still very small [on itself]. But all these very small adjustments we made, now make Koha respond in 1000-2000 ms. Did you run the xt test? It highlights a huge number of cases... I'm not especially worried about how we do this, but I am incredibly keen to actually move this stuff forward.. we've had these security issues in Koha for years since the reports came in. 360 places we're vulnerable and thats even after we've pushed this and bug 22990 and is at the last time I looked which is years ago: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22990#c89. (In reply to Martin Renvoize from comment #10) > Did you run the xt test? It highlights a huge number of cases... > > I'm not especially worried about how we do this, but I am incredibly keen to > actually move this stuff forward.. we've had these security issues in Koha > for years since the reports came in. No, sorry. Not seen. Will have a look. The number of cases should make the difference indeed. Suggestion:
- $open = $line if ( $l =~ m{<form} && !($l =~ m{method=('|")get('|")}));
+ $open = $line if ( $l =~ m{<form} && ($l =~ m{method=('|")post('|")}));
GET is the default when method not specified.
# $VAR293 = {
# 'errors' => [
# 'The <form> starting on line 56 is missing it\'s corresponding csrf_token include (see bug 22990)'
For 293 cases to add, I agree that this approach makes more sense !
(In reply to Marcel de Rooy from comment #13) > For 293 cases to add, I agree that this approach makes more sense ! Ultimately, anywhere that a state change happens, there should be CSRF protection. What's the plan here? Should we push bug 30524, bug 22990 and bug 30502 altogether? What about the tests here? Flag the patch as private and we deal with them on a separate bug reports? Or are we waiting for them to be fixed before we publish the 3 bug reports? (In reply to Jonathan Druart from comment #15) > What's the plan here? Should we push bug 30524, bug 22990 and bug 30502 > altogether? Not sure that there is a plan. We could always push bug 30524 first, but if we can do them all at once that would probably be ideal from a release perspective. > What about the tests here? Flag the patch as private and we deal with them > on a separate bug reports? Or are we waiting for them to be fixed before we > publish the 3 bug reports? Which tests? (In reply to David Cook from comment #16) > > What about the tests here? Flag the patch as private and we deal with them > > on a separate bug reports? Or are we waiting for them to be fixed before we > > publish the 3 bug reports? > > Which tests? xt/find-missing-csrf.t (In reply to Jonathan Druart from comment #17) > (In reply to David Cook from comment #16) > > > What about the tests here? Flag the patch as private and we deal with them > > > on a separate bug reports? Or are we waiting for them to be fixed before we > > > publish the 3 bug reports? > > > > Which tests? > > xt/find-missing-csrf.t I'll let Martin weigh in on that one. I'd probably move it to a different follow-up bug and just push these first 3 bugs as the core work. Actually, let's just QA and push bug 30524 and not worry about bug 22990 and bug 30502. I have the feeling that they'll never pass QA, so better to push this one, and then open 50 separate reports and just slowly make our way through the staff interface and OPAC one by one. Actually the first patch of bug 30502 is a bit of an extension of bug 30524, but oh well. We just can't keep sitting on these CSRF vulnerabilities though... Created attachment 138055 [details] [review] Bug 30524: Core CSRF checking code Split out from bug 22990 as requested. Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 138056 [details] Bug 30524: Add xt/find-missing-csrf.t Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Hi all. This cannot be pushed as-is right now. xt/find-missing-csrf.t will expose all the exploitable places until we fix them. So we need an action plan. And devs to take on the different places that need fixing. Maybe worth a call to talk about it through any of the videoconf tools around. (In reply to Tomás Cohen Arazi from comment #22) > Hi all. This cannot be pushed as-is right now. xt/find-missing-csrf.t will > expose all the exploitable places until we fix them. Wouldn't the solution be to move that patch to a different report and not push it? > So we need an action plan. And devs to take on the different places that > need fixing. Maybe worth a call to talk about it through any of the > videoconf tools around. Once the core code is in, it becomes very easy technically to fix. The issue is just getting sign-off and QA fast enough that patches still apply. I'm happy to be part of a trio to smash this out. It's in every vendor's best interests to fix this upstream so they don't have to do it downstream when pentests inevitably expose the issue. So surely 2 other vendors can spare 1 person each? (In reply to David Cook from comment #23) > (In reply to Tomás Cohen Arazi from comment #22) > > Hi all. This cannot be pushed as-is right now. xt/find-missing-csrf.t will > > expose all the exploitable places until we fix them. > > Wouldn't the solution be to move that patch to a different report and not > push it? That's part of a plan. > > So we need an action plan. And devs to take on the different places that > > need fixing. Maybe worth a call to talk about it through any of the > > videoconf tools around. > > Once the core code is in, it becomes very easy technically to fix. The issue > is just getting sign-off and QA fast enough that patches still apply. I like the idea. Let's do it. > I'm happy to be part of a trio to smash this out. It's in every vendor's > best interests to fix this upstream so they don't have to do it downstream > when pentests inevitably expose the issue. So surely 2 other vendors can > spare 1 person each? Will ask others so we sync. I propose we wait for next week so Martin is back and we can schedule this synced work. (In reply to Tomás Cohen Arazi from comment #24) > Will ask others so we sync. I propose we wait for next week so Martin is > back and we can schedule this synced work. Sounds like a good plan to me. Created attachment 141297 [details] [review] Bug 30524: Core CSRF checking code Split out from bug 22990 as requested. Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 141298 [details] Bug 30524: Add xt/find-missing-csrf.t Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Marking as blocked until we decide the push. (In reply to Tomás Cohen Arazi from comment #28) > Marking as blocked until we decide the push. Surely it makes sense to push the framework so that individual patches are easier to write and push out. I don't think sitting on this one is really helping. (In reply to David Cook from comment #29) > (In reply to Tomás Cohen Arazi from comment #28) > > Marking as blocked until we decide the push. > > Surely it makes sense to push the framework so that individual patches are > easier to write and push out. > > I don't think sitting on this one is really helping. "The perfect is the enemy of the good" Back to Signed off, and QAing [10:39] <marcelr> there is no module test? [10:39] <marcelr> for Auth or Output ? Created attachment 152629 [details] [review] Bug 30524: (QA follow-up) Polishing xt script Test plan: Run it again. Same results? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 152630 [details] [review] Bug 30524: Unit tests Open to suggestions if anyone has any better idea's regards the output_and_exit_if_error test.. I had some fun trying to get the tests to continue past the exit that's inside the routine.. Test::Trap seems to work well.. but it's a new dependancy. (In reply to Martin Renvoize from comment #35) > Open to suggestions if anyone has any better idea's regards the > output_and_exit_if_error test.. I had some fun trying to get the tests to > continue past the exit that's inside the routine.. Test::Trap seems to work > well.. but it's a new dependancy. Would not add a dependency for this. output_and_exit( $query, $cookie, $template, $error ) if $error; Could you not mock this routine and test $error ? Mmm, that could work. Good call Created attachment 152631 [details] [review] Bug 30524: Unit tests Created attachment 152632 [details] [review] Bug 30524: Attempt to mock Any clues why the mock option fails here? Created attachment 152634 [details] [review] Bug 30524: Unit tests Test plan: Run t/Output.t Run t/db_dependent/Auth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 152635 [details] [review] Bug 30524: Core CSRF checking code Split out from bug 22990 as requested. Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 152636 [details] Bug 30524: Add xt/find-missing-csrf.t Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 152637 [details] [review] Bug 30524: (QA follow-up) Polishing xt script Test plan: Run it again. Same results? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 152638 [details] [review] Bug 30524: Unit tests Test plan: Run t/Output.t Run t/db_dependent/Auth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 153752 [details] Bug 30524: [21.11] Core CSRF checking code Split out from bug 22990 as requested. Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Bug 30524: Add xt/find-missing-csrf.t Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Bug 30524: (QA follow-up) Polishing xt script Test plan: Run it again. Same results? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Bug 30524: Unit tests Test plan: Run t/Output.t Run t/db_dependent/Auth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> I've attached a rebased and squashed version for 21.11. I have not tested it though. Are we planning to push this soon and not wait on any work that has this as a dependency? Do we need this, in order to push (for example) bug 34368? If such is the case, I'd move the xt tests to another bug so we can push this now. Can someone confirm this (In reply to Tomás Cohen Arazi from comment #49) > Do we need this, in order to push (for example) bug 34368? If such is the > case, I'd move the xt tests to another bug so we can push this now. > > Can someone confirm this Yes, we need this to push (for example) bug 34368. (In reply to David Cook from comment #50) > (In reply to Tomás Cohen Arazi from comment #49) > > Do we need this, in order to push (for example) bug 34368? If such is the > > case, I'd move the xt tests to another bug so we can push this now. > > > > Can someone confirm this > > Yes, we need this to push (for example) bug 34368. I was wondering why no one splitted it already. I splitted it and PQA things are already in the security master branch. (In reply to Tomás Cohen Arazi from comment #51) > I was wondering why no one splitted it already. I splitted it and PQA things > are already in the security master branch. I'm not familiar with the security release process. Does that mean that it's going out in the next stable releases? Created attachment 153964 [details] [review] Bug 30524: (QA follow-up) Only generate CSRF token if it will be used Created attachment 153965 [details] [review] Bug 30524: (QA follow-up) Only generate CSRF token if it will be used This patch avoids generating CSRF tokens unless the csrf-token.inc file is included in the template. Passed token doesn't need HTML escaped. The docs for WWW::CSRF state: The returned CSRF token is in a text-only form suitable for inserting into a HTML form without further escaping (assuming you did not send in strange things to the Time option). Nice one, Kyle! However I am wondering why you don't simply pass sessionID to Koha.GenerateCSRF instead? <input type="hidden" name="csrf_token" value="[% Koha.GenerateCSRF(sessionID) | $raw %]" /> (In reply to Jonathan Druart from comment #55) > Nice one, Kyle! > > However I am wondering why you don't simply pass sessionID to > Koha.GenerateCSRF instead? > > <input type="hidden" name="csrf_token" value="[% > Koha.GenerateCSRF(sessionID) | $raw %]" /> Ah, you are implicitly saying the sessionID variable is already stashed in the templates, right? (In reply to Jonathan Druart from comment #55) > Nice one, Kyle! > > However I am wondering why you don't simply pass sessionID to > Koha.GenerateCSRF instead? > > <input type="hidden" name="csrf_token" value="[% > Koha.GenerateCSRF(sessionID) | $raw %]" /> There simply is no need. Instead of pulling it from the stash and passing it as a parameter, this patch pulls it from the stash directly. (In reply to Kyle M Hall from comment #57) > (In reply to Jonathan Druart from comment #55) > > Nice one, Kyle! > > > > However I am wondering why you don't simply pass sessionID to > > Koha.GenerateCSRF instead? > > > > <input type="hidden" name="csrf_token" value="[% > > Koha.GenerateCSRF(sessionID) | $raw %]" /> > > There simply is no need. Instead of pulling it from the stash and passing it > as a parameter, this patch pulls it from the stash directly. Yeah, even if it doesn't make much difference, I prefer not having to put the sessionID on the template explicitly. It is just less code and mistakes. Created attachment 153975 [details] [review] Bug 30524: (QA follow-up) Only generate CSRF token if it will be used This patch avoids generating CSRF tokens unless the csrf-token.inc file is included in the template. Passed token doesn't need HTML escaped. The docs for WWW::CSRF state: The returned CSRF token is in a text-only form suitable for inserting into a HTML form without further escaping (assuming you did not send in strange things to the Time option). Created attachment 153976 [details] [review] Bug 30524: (QA follow-up) Only generate CSRF token if it will be used This patch avoids generating CSRF tokens unless the csrf-token.inc file is included in the template. Passed token doesn't need HTML escaped. The docs for WWW::CSRF state: The returned CSRF token is in a text-only form suitable for inserting into a HTML form without further escaping (assuming you did not send in strange things to the Time option). Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> It would be more inline with what we usually do. (In reply to Tomás Cohen Arazi from comment #58) > It is just less code and mistakes. Well it's actually more code in the pm. New method, new tests, etc. Anyway, I don't care much. (In reply to Jonathan Druart from comment #61) > It would be more inline with what we usually do. > > (In reply to Tomás Cohen Arazi from comment #58) > > It is just less code and mistakes. > > Well it's actually more code in the pm. > New method, new tests, etc. Yeah, in a single place :-D > Anyway, I don't care much. Can you SO? Created attachment 154000 [details] [review] Bug 30524: (QA follow-up) Unit tests for GenerateCSRF() Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Pushed to master for 23.11. Nice work everyone, thanks! Created attachment 154038 [details] [review] Bug 30524: (QA follow-up) Fix tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Unless I'm missing something obvious, this last minute change to the base framework changes how it functions, and will break at least 1 patch on Bugzilla: https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=30502&attachment=138326 The "csrf_token" that was generated in C4::Auth and passed to the template gets used in URLs. (In the past, I've done comprehensive CSRF protection for Koha, and this gets used a fair bit.) At a glance, this shouldn't impact any patches that have already been pushed. For instance, bug 22990 will still work, because boraccount.pl has already been generating and sending a csrf_token to the template for years. This was a pretty big change to make last minute, although we should be able to keep the current patch(es) and just add something like the following to the top of the scripts: [% csrf_token = Koha.GenerateCSRF %] (In reply to David Cook from comment #66) > Unless I'm missing something obvious, this last minute change to the base > framework changes how it functions, and will break at least 1 patch on > Bugzilla: > > https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter. > html&bug=30502&attachment=138326 > > The "csrf_token" that was generated in C4::Auth and passed to the template > gets used in URLs. (In the past, I've done comprehensive CSRF protection for > Koha, and this gets used a fair bit.) > > At a glance, this shouldn't impact any patches that have already been > pushed. For instance, bug 22990 will still work, because boraccount.pl has > already been generating and sending a csrf_token to the template for years. > > This was a pretty big change to make last minute, although we should be able > to keep the current patch(es) and just add something like the following to > the top of the scripts: > > [% csrf_token = Koha.GenerateCSRF %] This is fixed on bug 34478 in patch "Remove generate_csrf from pl". Pushed to 23.05.x for 23.05.02 |