Description
Andreas Jonsson
2024-03-06 13:09:35 UTC
Test script for devbox: use Koha::Patrons; my $borrower = Koha::Patrons->find(1); $borrower->surname('[% USE Categories %][% Categories.all().search_related("borrowers").count() %]')->store(); my $letter = C4::Letters::GetPreparedLetter ( module => 'circulation', letter_code => 'PICKUP_RECALLED_ITEM', branchcode => 'CPL', lang => '', tables => { 'borrowers' => $borrower->borrowernumber, branches => 'CPL' }, loops => { }, substitute => {}, repeat => { }, message_transport_type => 'email', ); print $letter->{content}, "\n"; The delete_all is not a problem. We have the notice generation that is run inside a transaction (that is rollbacked), see bug 28739. However we are leaking info, all the DB I guess... This is terrible. $borrower->surname('[% USE Dumper( Indent=1, SortKeys=1 ) %][% USE Categories %][% Dumper.dump(Categories.all().search_related("borrowers").unblessed()) %]')->store(); There is NOTHING more important than fixing, testing and applying this everywhere. Please all start by: * turning off PatronSelfRegistration * turn on AutoApprovePatronProfileSettings * tell staff to be careful when approving OPAC members's self modifications Comment when sending a list is not affected. Created attachment 162866 [details] [review] Bug 36244: Unit test for tt syntax in parameters Created attachment 162867 [details] [review] Bug 36244: Do template toolkit processing first To avoid injection of template toolkit code from database fields that are controlled by untrusted sources. Test plan: * review subtest 'Template toolkit syntax in parameters' in t/db_dependent/Letters.t * Run the unit test: prove t/db_dependent/Letters.t A simple mitigation for the worst consequences of this is to perform the template toolkit processing before the parameter substitution. But there is still a problem that we are relying on regexp-soup style parsing to perform the parameter substitutions. Later passes will substitute parameters that are inserted by earlier passes. This will make it possible to leak confidential information. Maybe it is possible to use mustache to substitute the parameters. (http://mustache.github.io/) For now I'm going to suggest doing the template toolkit substitution first and open a separate issue to fix the regexp-soup. Created attachment 162868 [details] [review] Bug 36244: Prevent TT to evaluate data from DB Reversing the order could be a nice fix here (Thanks Martin for the idea). Comment on attachment 162868 [details] [review] Bug 36244: Prevent TT to evaluate data from DB Heh :) Created attachment 162871 [details] [review] Bug 36244: Unit test for tt syntax in parameters Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 162872 [details] [review] Bug 36244: Do template toolkit processing first To avoid injection of template toolkit code from database fields that are controlled by untrusted sources. Test plan: * review subtest 'Template toolkit syntax in parameters' in t/db_dependent/Letters.t * Run the unit test: prove t/db_dependent/Letters.t Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Keeping NSO status, we need more testers. Possible edge cases with this approach? I would be surprised if there aren't any libraries that is doing silly things such as: [% IF <<borrowers.categorycode>> == 'SPECIAL' %] But we cannot reasonably do anything about that. (In reply to Andreas Jonsson from comment #8) > For now I'm going to suggest doing the template toolkit substitution first > and open a separate issue to fix the regexp-soup. We should remove the historical <<>> syntax. Created attachment 162876 [details] [review] Bug 36244: Unit test for tt syntax in parameters Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 162877 [details] [review] Bug 36244: Do template toolkit processing first To avoid injection of template toolkit code from database fields that are controlled by untrusted sources. Test plan: * review subtest 'Template toolkit syntax in parameters' in t/db_dependent/Letters.t * Run the unit test: prove t/db_dependent/Letters.t Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> This is the solution I was thinking about, swapping the parsing order as you have. I think it's the quickest route to goal, but as you highlight, I'm confident it'll break some peoples setups as I'm sure I've seen <<>> inside [% %] blocks in a few places.. people will do interesting things whenever it's possible. We'll just need to advertise the breaking change at release time. As for replacing with mustache, I'm not keen on that at all.. introducing yet another template syntax.. What we should really be focusing on and should have achieved years ago frankly is removing the old <<>> syntax entirely and leaning on just TT as our chosen templating engine.. There's a lot of work still to go there given our UI still basically encourages the use of the old syntax instead of us ever haveing followed through with improved notice builders that actually advocate the use of proper TT. (In reply to Jonathan Druart from comment #15) > (In reply to Andreas Jonsson from comment #8) > > For now I'm going to suggest doing the template toolkit substitution first > > and open a separate issue to fix the regexp-soup. > > We should remove the historical <<>> syntax. I've been advocating for this for years (In reply to Jonathan Druart from comment #15) > (In reply to Andreas Jonsson from comment #8) > > For now I'm going to suggest doing the template toolkit substitution first > > and open a separate issue to fix the regexp-soup. > > We should remove the historical <<>> syntax. To get there, we might now forbid using both syntaxes in the same letter. (In reply to Martin Renvoize from comment #18) > > As for replacing with mustache, I'm not keen on that at all.. introducing > yet another template syntax.. What we should really be focusing on and > should have achieved years ago frankly is removing the old <<>> syntax > entirely and leaning on just TT as our chosen templating engine.. There's a > lot of work still to go there given our UI still basically encourages the > use of the old syntax instead of us ever haveing followed through with > improved notice builders that actually advocate the use of proper TT. Having two different templating engines is bad. But it is also not great that editing message texts is such a powerful mechanism. The privilege 'edit_notices' must be considered essentially as powerful as 'plugins/manage' or 'superlibrarian'. In theory it might be possible to sandbox template toolkit. (For instance, the template plugins should not return dbic-objects.) But I would have preferred having a simpler template engine for message texts. Created attachment 162908 [details] [review] Bug 36244: Unit test for tt syntax in parameters Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 162909 [details] [review] Bug 36244: Do template toolkit processing first To avoid injection of template toolkit code from database fields that are controlled by untrusted sources. Test plan: * review subtest 'Template toolkit syntax in parameters' in t/db_dependent/Letters.t * Run the unit test: prove t/db_dependent/Letters.t Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 162910 [details] [review] Bug 36244: Add atomic update to check for affected notices Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> (In reply to Andreas Jonsson from comment #21) > In theory it might be possible to sandbox template toolkit. (For instance, > the template plugins should not return dbic-objects.) But I would have > preferred having a simpler template engine for message texts. You don't need a different template engine. Template::Toolkit can be simple. We just use it in a problematic way. Firstly, we have it insecurely configured with EVAL_PERL and ABSOLUTE. I've complained about those for years, and I disable them locally. Secondly, as you note, we pass it DBIC objects. We could easily just pass it hashes instead and not have the security problem. That said, we'd lose convenience in other ways. It's a fine balance for sure. Being able to navigate relationships and having access to our includes and local tt filters are both very important for our end users.. I'd be very much against losing that functionality. Also, given the multi-year challenge to try and get us from <<>> to TT and the failure we've had there.. introducing yet another migration path before the first has even been completed is the road to madness in my opinion. (In reply to Martin Renvoize from comment #26) > Being able to navigate relationships and having access to our includes and > local tt filters are both very important for our end users.. I'd be very > much against losing that functionality. You should be able to do those without EVAL_PERL and ABSOLUTE. > Also, given the multi-year challenge to try and get us from <<>> to TT and > the failure we've had there.. introducing yet another migration path before > the first has even been completed is the road to madness in my opinion. Do you mean Mustache or something else? Yes, I mean Mustache for that comment. Passing just hashes, for me takes us backwards from the direction we've been moving. Giving end users less ability to get the data they want in notices and adding maintenance burden to developers as any and all additional data a librarian might want would need specific development to add to the hashes per notice. Our editor is terrible and doesn't expose at all what's actually available per notice, it's misleading at best right now. (In reply to Martin Renvoize from comment #28) > Yes, I mean Mustache for that comment. Passing just hashes, for me takes us > backwards from the direction we've been moving. Giving end users less > ability to get the data they want in notices and adding maintenance burden > to developers as any and all additional data a librarian might want would > need specific development to add to the hashes per notice. It is a tough one. I've certainly enjoyed being able to work with objects in the notices. But it makes me think about the security vs convenience balance. I could be wrong, but it seems like it could be used to circumvent a lot of our security at the moment (although the attackers would be staff rather than patrons at least). > Our editor is terrible and doesn't expose at all what's actually available > per notice, it's misleading at best right now. Yeah, I figure we get rid of <<>> and then we continue working on improving things with the Template::Toolkit. Anyway, seems like this particular issue raised by OP is taken care of at least, so that's the main thing. One concern about this fix is the fact that a Library who already have replaced all <<parameters>> with template toolkit syntax would not have been vulnerable to this issue in the first place. But by changing evaluation order we open up the possiblility to inject <<parameters>> and thus making them vulnerable to bug 36267. Please let me know if I need to backport this as it is to 22.05.x-security (In reply to wainuiwitikapark from comment #31) > Please let me know if I need to backport this as it is to 22.05.x-security Yes, this is for all stable releases. Created attachment 163255 [details] [review] Bug 36244: Unit test for tt syntax in parameters Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 163256 [details] [review] Bug 36244: Do template toolkit processing first To avoid injection of template toolkit code from database fields that are controlled by untrusted sources. Test plan: * review subtest 'Template toolkit syntax in parameters' in t/db_dependent/Letters.t * Run the unit test: prove t/db_dependent/Letters.t Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 163257 [details] [review] Bug 36244: Add atomic update to check for affected notices Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Fixed some typos in bug numbers and text. Fixed some typos in the database update (bug number especially), so re-attaching just in case. Pushed to security repository. Actually ... could we make the database update even smarter? It doesn't catch a quite common case in our databases that I know was also on the mailing list in the past: [% <<aqorders.listprice>> | $Price %] It was the only way to format prices int he ACQORDER email previous to bug 31858. Maybe check for this additionally? SELECT * FROM letter WHERE content LIKE "[%%% <<%"; This doesn't apply 100% cleanly to 22.05.x-security and I am getting the following: kohadev-koha@kohadevbox:koha((1ee9fe6a1f0...))$ prove t/db_dependent/Letters.t t/db_dependent/Letters.t .. Global symbol "$objects" requires explicit package name (did you forget to declare "my $objects"?) at /kohadevbox/koha/C4/Letters.pm line 605. Global symbol "$objects" requires explicit package name (did you forget to declare "my $objects"?) at /kohadevbox/koha/C4/Letters.pm line 616. Type of arg 1 to Try::Tiny::catch must be block or sub {} (not reference constructor) at /kohadevbox/koha/C4/Letters.pm line 1410, near "};" Type of arg 1 to Try::Tiny::try must be block or sub {} (not reference constructor) at /kohadevbox/koha/C4/Letters.pm line 1410, near "};" Type of arg 1 to Try::Tiny::catch must be block or sub {} (not reference constructor) at /kohadevbox/koha/C4/Letters.pm line 1459, near "};" Type of arg 1 to Try::Tiny::try must be block or sub {} (not reference constructor) at /kohadevbox/koha/C4/Letters.pm line 1459, near "};" Compilation failed in require at /kohadevbox/koha/C4/Reserves.pm line 31. BEGIN failed--compilation aborted at /kohadevbox/koha/C4/Reserves.pm line 31. Compilation failed in require at /kohadevbox/koha/C4/Circulation.pm line 30. BEGIN failed--compilation aborted at /kohadevbox/koha/C4/Circulation.pm line 30. Compilation failed in require at /kohadevbox/koha/Koha/Items.pm line 27. BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Items.pm line 27. Compilation failed in require at /kohadevbox/koha/Koha/Statistic.pm line 22. BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Statistic.pm line 22. Compilation failed in require at /kohadevbox/koha/Koha/Statistics.pm line 23. BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Statistics.pm line 23. Compilation failed in require at /kohadevbox/koha/C4/Stats.pm line 26. BEGIN failed--compilation aborted at /kohadevbox/koha/C4/Stats.pm line 26. Compilation failed in require at /kohadevbox/koha/C4/Accounts.pm line 23. BEGIN failed--compilation aborted at /kohadevbox/koha/C4/Accounts.pm line 23. Compilation failed in require at /kohadevbox/koha/C4/Overdues.pm line 31. BEGIN failed--compilation aborted at /kohadevbox/koha/C4/Overdues.pm line 31. Compilation failed in require at /kohadevbox/koha/Koha/Account/Line.pm line 23. BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Account/Line.pm line 23. Compilation failed in require at /kohadevbox/koha/Koha/Account/Lines.pm line 22. BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Account/Lines.pm line 22. Compilation failed in require at /kohadevbox/koha/Koha/ArticleRequest.pm line 22. BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/ArticleRequest.pm line 22. Compilation failed in require at /kohadevbox/koha/Koha/ArticleRequests.pm line 26. BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/ArticleRequests.pm line 26. Compilation failed in require at /kohadevbox/koha/Koha/Patrons.pm line 27. BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Patrons.pm line 27. Compilation failed in require at /kohadevbox/koha/Koha/Acquisition/Basket.pm line 27. BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Acquisition/Basket.pm line 27. Compilation failed in require at /kohadevbox/koha/Koha/Acquisition/Baskets.pm line 23. BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Acquisition/Baskets.pm line 23. Compilation failed in require at /kohadevbox/koha/Koha/Acquisition/Order.pm line 24. BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Acquisition/Order.pm line 24. Compilation failed in require at /kohadevbox/koha/Koha/Acquisition/Orders.pm line 24. BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Acquisition/Orders.pm line 24. Compilation failed in require at /kohadevbox/koha/Koha/Biblio.pm line 33. BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Biblio.pm line 33. Compilation failed in require at /kohadevbox/koha/Koha/Biblios.pm line 25. BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Biblios.pm line 25. Compilation failed in require at /kohadevbox/koha/Koha/Libraries.pm line 25. BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Libraries.pm line 25. Compilation failed in require at /kohadevbox/koha/Koha/Object/Limit/Library.pm line 22. BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Object/Limit/Library.pm line 22. Compilation failed in require at /usr/lib/x86_64-linux-gnu/perl-base/base.pm line 135. ...propagated at /usr/lib/x86_64-linux-gnu/perl-base/base.pm line 157. BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/AuthorisedValue.pm line 25. Compilation failed in require at /kohadevbox/koha/Koha/AuthorisedValues.pm line 25. BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/AuthorisedValues.pm line 25. Compilation failed in require at /kohadevbox/koha/C4/Koha.pm line 27. BEGIN failed--compilation aborted at /kohadevbox/koha/C4/Koha.pm line 27. Compilation failed in require at /kohadevbox/koha/C4/Biblio.pm line 91. BEGIN failed--compilation aborted at /kohadevbox/koha/C4/Biblio.pm line 91. Compilation failed in require at /kohadevbox/koha/t/lib/TestBuilder.pm line 6. BEGIN failed--compilation aborted at /kohadevbox/koha/t/lib/TestBuilder.pm line 6. Compilation failed in require at t/db_dependent/Letters.t line 51. BEGIN failed--compilation aborted at t/db_dependent/Letters.t line 51. # Looks like your test exited with 255 before it could output anything. t/db_dependent/Letters.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 86/86 subtests Test Summary Report ------------------- t/db_dependent/Letters.t (Wstat: 65280 Tests: 0 Failed: 0) Non-zero exit status: 255 Parse errors: Bad plan. You planned 86 tests but ran 0. Files=1, Tests=0, 1 wallclock secs ( 0.01 usr 0.00 sys + 0.48 cusr 0.05 csys = 0.54 CPU) Result: FAIL Created attachment 163383 [details] [review] Bug 36244: [22.05] Unit test for tt syntax in parameters Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 163384 [details] [review] Bug 36244: [22.05] Do template toolkit processing first To avoid injection of template toolkit code from database fields that are controlled by untrusted sources. Test plan: * review subtest 'Template toolkit syntax in parameters' in t/db_dependent/Letters.t * Run the unit test: prove t/db_dependent/Letters.t Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 163385 [details] [review] Bug 36244: [22.05] Add atomic update to check for affected notices Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Fixed some typos in bug numbers and text. (In reply to wainuiwitikapark from comment #40) > This doesn't apply 100% cleanly to 22.05.x-security Hello Wainui, I have attached a version for 22.05. (In reply to Jonathan Druart from comment #44) > (In reply to wainuiwitikapark from comment #40) > > This doesn't apply 100% cleanly to 22.05.x-security > > Hello Wainui, I have attached a version for 22.05. Thanks so much! I have now backported the 22.05 patches to 22.05.x-security for 22.05.20 RMaints : Don't forget to create a DBRev by moving the atomic update : installer/data/mysql/atomicupdate/bug_36244.pl (In reply to Fridolin Somers from comment #46) > RMaints : > Don't forget to create a DBRev by moving the atomic update : > installer/data/mysql/atomicupdate/bug_36244.pl oops I just set up my release but I didn't do this (In reply to wainuiwitikapark from comment #47) > (In reply to Fridolin Somers from comment #46) > > RMaints : > > Don't forget to create a DBRev by moving the atomic update : > > installer/data/mysql/atomicupdate/bug_36244.pl > > oops I just set up my release but I didn't do this Added the db rev commit to 22.05.x-security branch https://git.koha-community.org/Koha-community/security/commit/975b3886de245ed4444138842683a8af4dbdde5b Am I right to understand this has been backported but is not yet "Pushed to master" ? First Bug I see handled in that order, so I just want to validate this is eventually making it to master. Thanks (In reply to Blou from comment #49) > Am I right to understand this has been backported but is not yet "Pushed to > master" ? > > First Bug I see handled in that order, so I just want to validate this is > eventually making it to master. > > Thanks It's the usual workflow for security bugs. It will be pushed to master very soon. AH! Big thanks for the clarification. I usually am not CC'd on those. (In reply to Blou from comment #51) > AH! Big thanks for the clarification. I usually am not CC'd on those. I've just added you to the security group and default CC for security bugs. |