Description
David Cook
2024-11-18 02:34:08 UTC
After some grepping, I've tried manipulating the Javascript formed using Template::Toolkit. In most cases, I just broke the Javascript. The filters were able to hinder my hack attempts. That said... I found one particularly vulnerable page where I created a lovely hack even without single quotes, so I'm going to have to re-visit some of these pages using my latest techniques... Note: This is one of the reasons why we shouldn't be writing Javascript code using Template::Toolkit/Perl. Created attachment 174659 [details] [review] Bug 38467: Make uri and url filters rfc3986 when using C4::Template This change invokes Template::Filters->use_rfc3986 in the C4::Template module. Test plan: 0. Apply the patch 1. Note that "uri" and "url" filters now escape single quotes Since Template::Filters->use_rfc3986() is a class method that operates at the global level, we will want to be careful with how we invoke it. We use Template->new() in C4/Letters.pm, C4/Templates.pm, C4/Creators/Template.pm, Koha/Plugins/Base.pm, Koha/TemplateUtils.pm, misc/cronjobs/rss/rss.pl, and xt/author/valid-templates.t So... we'll probably want to update more places than just C4/Templates.pm, although that one should be the widest reaching change. Note: Newer versions of Template::Toolkit (ie 2.28+) default to using rfc3986 for "uri" and "url" filters, but that upgrade is still a while away in the Debian repos... Created attachment 174660 [details] [review] Bug 38467: Make uri and url filters rfc3986 when using C4::Template This change invokes Template::Filters->use_rfc3986 in the C4::Template module. Test plan: 0. Apply the patch 1. Note that "uri" and "url" filters now escape single quotes (In reply to David Cook from comment #0) > When using Template::Toolkit within HTML attributes, that's fine, because > the important thing is to escape double quotes. Isn't that only true because we're mostly not in the habit of using single quotes in HTML? https://git.koha-community.org/Koha-community/Koha/src/commit/d7ae20f0cb4bc0850896617aae59ec22fedcc71d/koha-tmpl/intranet-tmpl/prog/en/modules/suggestion/suggestion.tt#L687 doesn't look very exploitable, but that doesn't stop someone from using single quotes in a more exploitable way tomorrow. (In reply to Phil Ringnalda from comment #9) > (In reply to David Cook from comment #0) > > When using Template::Toolkit within HTML attributes, that's fine, because > > the important thing is to escape double quotes. > > Isn't that only true because we're mostly not in the habit of using single > quotes in HTML? Yes, that's true. > https://git.koha-community.org/Koha-community/Koha/src/commit/ > d7ae20f0cb4bc0850896617aae59ec22fedcc71d/koha-tmpl/intranet-tmpl/prog/en/ > modules/suggestion/suggestion.tt#L687 doesn't look very exploitable, but > that doesn't stop someone from using single quotes in a more exploitable way > tomorrow. Yep, I agree with you there. That "html" filter should be a "uri" filter, and the single quote should be a double quote for the attribute. After talking to you yesterday, I suspect we have all kinds of these problems throughout the code. -- I think the patch for this bug should improve the situation though. I'm actually going to move it into "Needs Signoff" as I think there's enough there to get us moving along. Created attachment 174762 [details] [review] Bug 38467: Make uri and url filters rfc3986 when using C4::Template This change invokes Template::Filters->use_rfc3986 in the C4::Template module. Test plan: 0. Apply the patch 1. Note that "uri" and "url" filters now escape single quotes Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> I see a whole lot of changing | html to | uri in my future :) Please add tests. (In reply to Jonathan Druart from comment #13) > Please add tests. Could you elaborate on that one? Since it covers the entirety of web-facing Koha, I'm not sure what you have in mind. (In reply to David Cook from comment #14) > (In reply to Jonathan Druart from comment #13) > > Please add tests. > > Could you elaborate on that one? Since it covers the entirety of web-facing > Koha, I'm not sure what you have in mind. Some code in 't' that is failing before this patch, but passed once this patch is applied? :D Created attachment 174819 [details] [review] Bug 38467: Add test This test is failing, can you have a look please? # got: 'URI=console.log%28%22alert%28%27boo%21%27%29%22%29%3B, URL=console.log%28%22alert%28%27boo%21%27%29%22%29;' # expected: 'URI=console.log%28%22alert%28%27boo%21%27%29%22%29%3B, URL=console.log%28%22alert%28%27boo%21%27%29%22%29%3B' Trivial fix: don't pass the trailing semicolon. Trivial fix two: we're only adding use_rfc3986 so we get ' and " encoded, so the string to test is q{'"}, nothing else matters to this bug. More interesting fix: uri_escape has an optional second param with the characters you want to escape, so we could tailor it to do every character that TT's uri and url currently do, but then what string do we want to test? We could pass every ASCII character, and have a test that would tell us if TT changes any of the characters in the future, but is that something we want to know? (In reply to Jonathan Druart from comment #15) > Some code in 't' that is failing before this patch, but passed once this > patch is applied? :D Could you tell me which code? (In reply to Jonathan Druart from comment #17) > This test is failing, can you have a look please? > > # got: 'URI=console.log%28%22alert%28%27boo%21%27%29%22%29%3B, > URL=console.log%28%22alert%28%27boo%21%27%29%22%29;' > # expected: 'URI=console.log%28%22alert%28%27boo%21%27%29%22%29%3B, > URL=console.log%28%22alert%28%27boo%21%27%29%22%29%3B' You've just made a mistake with your test. For the "url" filter in Template::Toolkit, the ";" is not escaped with or without rfc_3986(). You can see this by running the test before/after the patch, and by consulting the documentation for Template::Manual::Filters -- On a side note: I note that Koha/Template/Plugin/SafeURL.pm uses the "URI" library which uses RFC 2396 rather than RFC 3986. It's fine when the filter is used in a HTML attribute that uses double quotes, but like I note elsewhere on this report... these filters shouldn't be used for creating Javascript (or in HTML attributes using single quotes). Even if we don't add "use_rfc3986()" ourselves, it automatically does get turned on in Template Toolkit 2.28+, which is coming. So we'll either need to make the change eventually, or fallback by using "use_rfc2396()". That said, this is a wide-reaching change, so we shouldn't just employ it "willy-nilly". As I've noted on this report and elsewhere, this isn't a cure-all anyway. We need to improve our code quality/coding practice overall. > Even if we don't add "use_rfc3986()" ourselves, it automatically does get turned on in Template Toolkit 2.28+, which is coming.
Do you mean there is a ticket for upgrading our TT version?
(In reply to Victor Grousset/tuxayo from comment #21) > > Even if we don't add "use_rfc3986()" ourselves, it automatically does get turned on in Template Toolkit 2.28+, which is coming. > > Do you mean there is a ticket for upgrading our TT version? No, I mean upstream in Debian/Ubuntu we will pull it in without realising it eventually. Probably should update our cpanfile, though: it's currently >=2.22. You need 2014's 2.26 for use_rfc3986 to even exist, and 2016's 2.27 for it to correctly encode double-quotes. (In reply to Phil Ringnalda from comment #23) > Probably should update our cpanfile, though: it's currently >=2.22. You need > 2014's 2.26 for use_rfc3986 to even exist, and 2016's 2.27 for it to > correctly encode double-quotes. Yeah, I think you're right. While most people use the Debian packages, I think we need to mandate it in the cpanfile too. Created attachment 175465 [details] [review] Bug 38467: (follow-up) Fix test Created attachment 175466 [details] [review] Bug 38467: (follow-up) Update cpanfile For use_rfc3986() to work, we need at least Template::Toolkit 2.27. So I would've sworn that newer versions of Debian/Ubuntu were using newer versions of libtemplate-perl, but looking again now and both seem set on 2.27. Maybe I was just imagining it and thinking about "the future". For the cpanfile, we might even consider doing "== 2.27" as that might be the safest option... Created attachment 175685 [details] [review] Bug 38467: Make uri and url filters rfc3986 when using C4::Template This change invokes Template::Filters->use_rfc3986 in the C4::Template module. Test plan: 0. Apply the patch 1. Note that "uri" and "url" filters now escape single quotes Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 175686 [details] [review] Bug 38467: Add test Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 175687 [details] [review] Bug 38467: (follow-up) Fix test Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 175688 [details] [review] Bug 38467: (follow-up) Update cpanfile For use_rfc3986() to work, we need at least Template::Toolkit 2.27. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> It works! :) Testing notes: manual testing: add the following in intranet-main.tt for example [% "test URI'" | uri %] [% "test URL'" | url %] automated: revert commit «Bug 38467: Make uri and url filters» and see that the test doesn't pass anymore. With the error being getting `'` instead of `%27` (In reply to David Cook from comment #20) > Even if we don't add "use_rfc3986()" ourselves, it automatically does get > turned on in Template Toolkit 2.28+, which is coming. > > So we'll either need to make the change eventually, or fallback by using > "use_rfc2396()". > > That said, this is a wide-reaching change, so we shouldn't just employ it > "willy-nilly". I always use the latest version from CPAN when building a new dev environment so I've been using a version greater than 2.27 for years and never encountered a problem caused by that. Obviously my use cases don't cover everything but I'm pretty confident about that change. > As I've noted on this report and elsewhere, this isn't a cure-all anyway. We > need to improve our code quality/coding practice overall. I agree. We should probably avoid mixing TT syntax with javascript and find a better way to pass Perl data to JS code (one that comes to mind: put data into HTML data-* attributes - using only the html filter - and do other escaping/encoding if needed in javascript) Created attachment 176135 [details] [review] Bug 38467: Make uri and url filters rfc3986 when using C4::Template This change invokes Template::Filters->use_rfc3986 in the C4::Template module. Test plan: 0. Apply the patch 1. Note that "uri" and "url" filters now escape single quotes Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Created attachment 176136 [details] [review] Bug 38467: Add test Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Created attachment 176137 [details] [review] Bug 38467: (follow-up) Fix test Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Created attachment 176138 [details] [review] Bug 38467: (follow-up) Update cpanfile For use_rfc3986() to work, we need at least Template::Toolkit 2.27. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Just a reminder that this patch *only* affects the "uri" and "url" filters - NOT the "html" filter. (In reply to David Cook from comment #39) > Just a reminder that this patch *only* affects the "uri" and "url" filters - > NOT the "html" filter. RFC 2396 Uniform Resource Identifiers (URI): Generic Syntax RFC 2732 Format for Literal IPv6 Addresses in URL's RFC 3986 Uniform Resource Identifier (URI): Generic Syntax Updates: 1738 Obsoletes: 2732, 2396, 1808 (In reply to Victor Grousset/tuxayo from comment #33) > It works! :) > > Testing notes: > manual testing: add the following in intranet-main.tt for example > [% "test URI'" | uri %] > [% "test URL'" | url %] > > automated: revert commit «Bug 38467: Make uri and url filters» and see that > the test doesn't pass anymore. With the error being getting `'` instead of > `%27` Hi Victor, We are testing the first patch for backporting to 24.05.x-security When I add those two examples to intranet-main.tt in the browser I see: test%20URI%27 test%20URL%27 Just checking is this what we should be expecting? (In reply to Alex Buckley from comment #41) > (In reply to Victor Grousset/tuxayo from comment #33) > > It works! :) > > > > Testing notes: > > manual testing: add the following in intranet-main.tt for example > > [% "test URI'" | uri %] > > [% "test URL'" | url %] > > > > automated: revert commit «Bug 38467: Make uri and url filters» and see that > > the test doesn't pass anymore. With the error being getting `'` instead of > > `%27` > > Hi Victor, > > We are testing the first patch for backporting to 24.05.x-security > > When I add those two examples to intranet-main.tt in the browser I see: > test%20URI%27 test%20URL%27 > > Just checking is this what we should be expecting? Yeah that should be right (In reply to David Cook from comment #42) > (In reply to Alex Buckley from comment #41) > > (In reply to Victor Grousset/tuxayo from comment #33) > > > It works! :) > > > > > > Testing notes: > > > manual testing: add the following in intranet-main.tt for example > > > [% "test URI'" | uri %] > > > [% "test URL'" | url %] > > > > > > automated: revert commit «Bug 38467: Make uri and url filters» and see that > > > the test doesn't pass anymore. With the error being getting `'` instead of > > > `%27` > > > > Hi Victor, > > > > We are testing the first patch for backporting to 24.05.x-security > > > > When I add those two examples to intranet-main.tt in the browser I see: > > test%20URI%27 test%20URL%27 > > > > Just checking is this what we should be expecting? > > Yeah that should be right Thank you David! Applied to 24.05.x-security Note: We ran through the test plan in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38467#c33 After adding the two examples to intranet-main.tt in the browser we see: test%20URI%27 test%20URL%27 We checked if that was expected output. We also ran the added unit tests in https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176136 Applied to 23.11.x-security branch Applied to 22.11.x-security branch for next release. (In reply to Phil Ringnalda from comment #23) > Probably should update our cpanfile, though: it's currently >=2.22. You need > 2014's 2.26 for use_rfc3986 to even exist, and 2016's 2.27 for it to > correctly encode double-quotes. we have libtemplate-perl 2.27 in debian-buster... so no problem here :) https://packages.debian.org/buster/libtemplate-perl Applied to 24.11.x-security Pushed for 25.05! Well done everyone, thank you! Katrin, the patch "Bug 38467: (follow-up) Update cpanfile" is wrong in main, it removed -recommends 'Git::Wrapper', '0.048'; -recommends 'Test::PerlTidy', '20230226'; (In reply to Jonathan Druart from comment #50) > Katrin, the patch "Bug 38467: (follow-up) Update cpanfile" is wrong in main, > it removed > -recommends 'Git::Wrapper', '0.048'; > -recommends 'Test::PerlTidy', '20230226'; OK; just revert? Created attachment 179195 [details] [review] Bug 38467: (follow-up 2) Update cpanfile - restore Git::Wrapper and Test::PerlTidy follow-up 2 pushed to main. Thanks all for investigating! It happened because auto_rebase script has been used, and that those 2 lines have been added by the tidy commits (bug 38664). The auto_rebase ignores commits from this bug on purpose. |