Bug 38961 - XSS in vendor search
Summary: XSS in vendor search
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P3 major
Assignee: Phil Ringnalda
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-01-24 01:47 UTC by Phil Ringnalda
Modified: 2025-03-06 13:01 UTC (History)
17 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
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:


Attachments
Bug 38961: XSS in vendor search (2.17 KB, patch)
2025-01-24 02:05 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 38961: XSS in vendor search (2.26 KB, patch)
2025-01-24 07:24 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 38961: XSS in vendor search (2.27 KB, patch)
2025-01-27 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38961: XSS in vendor search (2.34 KB, patch)
2025-01-27 10:09 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Phil Ringnalda 2025-01-24 01:47:23 UTC
acqui/booksellers.tt and admin/currency.tt use | $HtmlTags without first filtering the string they wrap.
Comment 2 Phil Ringnalda 2025-01-24 02:05:43 UTC Comment hidden (obsolete)
Comment 4 Magnus Enger 2025-01-24 07:24:02 UTC
Created attachment 177097 [details] [review]
Bug 38961: XSS in vendor search

acqui/booksellers.tt and admin/currency.tt use | $HtmlTags without first
filtering the string they wrap.

Test plan:
1. Without the patch, load the URL in comment 1, note the alert()
2. Apply patch, load the URL in comment 1, no alert()

Sponsored-by: Chetco Community Public Library
Signed-off-by: Magnus Enger <magnus@libriotech.no>
Followed the test plan, works as advertised.
Comment 6 Jonathan Druart 2025-01-27 10:08:42 UTC
Created attachment 177171 [details] [review]
Bug 38961: XSS in vendor search

acqui/booksellers.tt and admin/currency.tt use | $HtmlTags without first
filtering the string they wrap.

Test plan:
1. Without the patch, load the URL in comment 1, note the alert()
2. Apply patch, load the URL in comment 1, no alert()

Sponsored-by: Chetco Community Public Library
Signed-off-by: Magnus Enger <magnus@libriotech.no>
Followed the test plan, works as advertised.
Comment 7 Jonathan Druart 2025-01-27 10:09:04 UTC
Created attachment 177172 [details] [review]
Bug 38961: XSS in vendor search

acqui/booksellers.tt and admin/currency.tt use | $HtmlTags without first
filtering the string they wrap.

Test plan:
1. Without the patch, load the URL in comment 1, note the alert()
2. Apply patch, load the URL in comment 1, no alert()

Sponsored-by: Chetco Community Public Library
Signed-off-by: Magnus Enger <magnus@libriotech.no>
Followed the test plan, works as advertised.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Alex Buckley 2025-02-05 02:18:45 UTC
Applied to 24.05.x-security branch.

We had to amend this patch slightly to be able to apply it on 24.05

The conflict was:

$ sed -n '/<<<<</,/>>>>>/p' koha-tmpl/intranet-tmpl/prog/en/modules/admin/currency.tt
<<<<<<< HEAD
    <div class="dialog alert">
        <h1>Cannot delete currency [% currency.currency | $HtmlTags tag='span' attributes=>'class="ex"' %]</h1>
=======
    <div class="alert alert-warning">
        <h1>Cannot delete currency [% currency.currency | html | $HtmlTags tag='span' attributes=>'class="ex"' %]</h1>
>>>>>>> Bug 38961: XSS in vendor search


We changed it to:

    <div class="dialog alert">
        <h1>Cannot delete currency [% currency.currency | html | $HtmlTags tag='span' attributes=>'class="ex"' %]</h1>
        <p>

Here is our patch on the 24.05.x-security branch: https://git.koha-community.org/Koha-community/security/commit/a5afd9c9c8bd3901597a45998bf42b257cf91690
Comment 12 Fridolin Somers 2025-02-14 14:17:32 UTC
Looks like the filter is missing in koha-tmpl/intranet-tmpl/prog/en/modules/installer/step2.tt

<li>Database [% dbname | $HtmlTags tag=>'code' %] exists.</li>

Other places use html or a static string.
Comment 13 Fridolin Somers 2025-02-14 14:20:34 UTC
Applied to 23.11.x-security branch
Comment 14 Jesse Maseto 2025-02-18 20:57:55 UTC
Applied to 22.11.x-security branch
Comment 15 Phil Ringnalda 2025-02-20 21:36:28 UTC
(In reply to Fridolin Somers from comment #12)
> Looks like the filter is missing in
> koha-tmpl/intranet-tmpl/prog/en/modules/installer/step2.tt
> 
> <li>Database [% dbname | $HtmlTags tag=>'code' %] exists.</li>
> 
> Other places use html or a static string.

True, those should be html filtered since "<script>alert('Robert\'); DROP TABLE Students;-- ');</script>" is a valid (if pathological) MySQL username. But as far as I can tell, those all come directly from koha-conf.xml, and if you've lost control over it you've lost the game. So rather than a security bug, they'll have to wait for the one about requiring a prefilter on every use of $HtmlTags.

The static strings are actually more interesting, since I presume they are translated strings (that being the point of $HtmlTags). If a translator uses an ampersand in a string, did they mean &, or did they mean &amp;? And if a translator uses &amp;, did they mean &amp;amp;? When those uses of $HtmlTags are required to be prefiltered, are they $raw or are they html?
Comment 16 Paul Derscheid 2025-02-22 18:16:20 UTC
Applied to 24.11.x-security
Comment 17 Katrin Fischer 2025-03-06 13:00:29 UTC
Pushed for 25.05!

Well done everyone, thank you!