Bug 38467

Summary: Template::Toolkit filters can create risky Javascript when not using RFC3986
Product: Koha Reporter: David Cook <dcook>
Component: TemplatesAssignee: David Cook <dcook>
Status: RESOLVED FIXED QA Contact: Julian Maurice <julian.maurice>
Severity: normal    
Priority: P5 - low CC: alexbuckley, dcook, didier.gautheron, fridolin.somers, jesse, jonathan.druart, julian.maurice, katrin.fischer, laurent.ducos, lisette, lucas, m.de.rooy, martin.renvoize, mtj, nick, paul.derscheid, phil, philippe.blouin, tomascohen, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38468
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38469
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38470
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22223
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
25.05.00,24.11.02,24.05.07,23.11.12,22.11.24
Circulation function:
Bug Depends on:    
Bug Blocks: 38214    
Attachments: Bug 38467: Make uri and url filters rfc3986 when using C4::Template
Bug 38467: Make uri and url filters rfc3986 when using C4::Template
Bug 38467: Make uri and url filters rfc3986 when using C4::Template
Bug 38467: Add test
Bug 38467: (follow-up) Fix test
Bug 38467: (follow-up) Update cpanfile
Bug 38467: Make uri and url filters rfc3986 when using C4::Template
Bug 38467: Add test
Bug 38467: (follow-up) Fix test
Bug 38467: (follow-up) Update cpanfile
Bug 38467: Make uri and url filters rfc3986 when using C4::Template
Bug 38467: Add test
Bug 38467: (follow-up) Fix test
Bug 38467: (follow-up) Update cpanfile
Bug 38467: (follow-up 2) Update cpanfile - restore Git::Wrapper and Test::PerlTidy

Description David Cook 2024-11-18 02:34:08 UTC
Phil Ringnalda pointed out to me that "uri", "url", and "html" *currently* do not escape single quotes.

When using Template::Toolkit within HTML attributes, that's fine, because the important thing is to escape double quotes.

However, we also use Template::Toolkit filters in other contexts to create Javascript. In some of these situations, it's possible to inject single quotes, which can alter the structure of the Javascript.
Comment 1 David Cook 2024-11-18 02:35:21 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...
Comment 3 David Cook 2024-11-18 03:13:56 UTC
Note: This is one of the reasons why we shouldn't be writing Javascript code using Template::Toolkit/Perl.
Comment 6 David Cook 2024-11-18 05:38:44 UTC
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
Comment 7 David Cook 2024-11-18 05:41:51 UTC
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...
Comment 8 David Cook 2024-11-18 05:47:08 UTC
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
Comment 9 Phil Ringnalda 2024-11-18 23:05:38 UTC
(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.
Comment 10 David Cook 2024-11-18 23:34:12 UTC
(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.
Comment 11 Phil Ringnalda 2024-11-19 04:17:48 UTC
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>
Comment 12 Phil Ringnalda 2024-11-19 04:18:51 UTC
I see a whole lot of changing | html to | uri in my future :)
Comment 13 Jonathan Druart 2024-11-19 06:15:18 UTC
Please add tests.
Comment 14 David Cook 2024-11-19 22:00:35 UTC
(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.
Comment 15 Jonathan Druart 2024-11-20 08:54:18 UTC
(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
Comment 16 Jonathan Druart 2024-11-20 08:57:23 UTC
Created attachment 174819 [details] [review]
Bug 38467: Add test
Comment 17 Jonathan Druart 2024-11-20 08:57:51 UTC
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'
Comment 18 Phil Ringnalda 2024-11-20 16:24:13 UTC
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?
Comment 19 David Cook 2024-11-22 03:46:24 UTC
(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).
Comment 20 David Cook 2024-11-22 03:51:04 UTC
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.
Comment 21 Victor Grousset/tuxayo 2024-11-29 23:51:04 UTC
> 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?
Comment 22 David Cook 2024-12-01 22:28:24 UTC
(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.
Comment 23 Phil Ringnalda 2024-12-02 02:09:55 UTC
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.
Comment 24 David Cook 2024-12-09 00:16:46 UTC
(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.
Comment 25 Phil Ringnalda 2024-12-13 18:31:56 UTC
Created attachment 175465 [details] [review]
Bug 38467: (follow-up) Fix test
Comment 26 Phil Ringnalda 2024-12-13 18:36:07 UTC
Created attachment 175466 [details] [review]
Bug 38467: (follow-up) Update cpanfile

For use_rfc3986() to work, we need at least Template::Toolkit 2.27.
Comment 27 Phil Ringnalda 2024-12-13 18:37:25 UTC
Adding Mason per https://wiki.koha-community.org/wiki/Release_management#Dependency_changes
Comment 28 David Cook 2024-12-16 01:41:00 UTC
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...
Comment 29 Victor Grousset/tuxayo 2024-12-19 09:26:09 UTC
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>
Comment 30 Victor Grousset/tuxayo 2024-12-19 09:26:13 UTC
Created attachment 175686 [details] [review]
Bug 38467: Add test

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 31 Victor Grousset/tuxayo 2024-12-19 09:26:17 UTC
Created attachment 175687 [details] [review]
Bug 38467: (follow-up) Fix test

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 32 Victor Grousset/tuxayo 2024-12-19 09:26:21 UTC
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>
Comment 33 Victor Grousset/tuxayo 2024-12-19 09:29:24 UTC
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`
Comment 34 Julian Maurice 2025-01-03 16:40:09 UTC
(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)
Comment 35 Julian Maurice 2025-01-03 16:40:39 UTC
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>
Comment 36 Julian Maurice 2025-01-03 16:40:42 UTC
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>
Comment 37 Julian Maurice 2025-01-03 16:40:45 UTC
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>
Comment 38 Julian Maurice 2025-01-03 16:40:48 UTC
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>
Comment 39 David Cook 2025-01-12 23:32:03 UTC
Just a reminder that this patch *only* affects the "uri" and "url" filters - NOT the "html" filter.
Comment 40 David Cook 2025-01-12 23:37:31 UTC
(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
Comment 41 Alex Buckley 2025-02-04 00:53:52 UTC
(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?
Comment 42 David Cook 2025-02-05 04:32:35 UTC
(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
Comment 43 Alex Buckley 2025-02-12 00:42:29 UTC
(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!
Comment 44 Alex Buckley 2025-02-12 01:39:48 UTC
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
Comment 45 Fridolin Somers 2025-02-14 14:20:26 UTC
Applied to 23.11.x-security branch
Comment 46 Jesse Maseto 2025-02-19 16:08:54 UTC
Applied to 22.11.x-security branch for next release.
Comment 47 Mason James 2025-02-20 02:50:15 UTC
(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
Comment 48 Paul Derscheid 2025-02-22 18:18:54 UTC
Applied to 24.11.x-security
Comment 49 Katrin Fischer 2025-03-06 12:56:21 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 50 Jonathan Druart 2025-03-12 08:11:57 UTC
Katrin, the patch "Bug 38467: (follow-up) Update cpanfile" is wrong in main, it removed 
-recommends 'Git::Wrapper', '0.048';
-recommends 'Test::PerlTidy', '20230226';
Comment 51 Katrin Fischer 2025-03-12 08:12:47 UTC
(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?
Comment 52 Jonathan Druart 2025-03-12 08:47:35 UTC
Created attachment 179195 [details] [review]
Bug 38467: (follow-up 2) Update cpanfile - restore Git::Wrapper and Test::PerlTidy
Comment 53 Katrin Fischer 2025-03-12 08:56:33 UTC
follow-up 2 pushed to main. Thanks all for investigating!
Comment 54 Jonathan Druart 2025-03-12 08:57:01 UTC
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.