Bug 19121 - Prevent XSS in the Staff Client and the OPAC - bis
Summary: Prevent XSS in the Staff Client and the OPAC - bis
Status: RESOLVED DUPLICATE of bug 13618
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-08-15 19:20 UTC by Jonathan Druart
Modified: 2019-11-27 10:30 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 19121: [PoC] Prevent XSS - Escape variables when sent to scripts (1.81 KB, patch)
2017-08-15 19:24 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 Jonathan Druart 2017-08-15 19:20:35 UTC
On bug 13618 we tried to escape variables at template level: all variables were escaped using Template::Stash::AutoEscaping::Escaped::HTML::escape
It works well, but unfortunately it leads to performance issues (see bug 15715). Indeed in some scripts, 70k variables are displayed

Another approach would be to escape variables when they are sent to the scripts. We will get only 1 call per variable passed to the script and we will not escape unnecessarily variables several times, or variables coming from the DB (maybe we will need a script to clean the data?)
Comment 1 Jonathan Druart 2017-08-15 19:24:31 UTC
Created attachment 66045 [details] [review]
Bug 19121: [PoC] Prevent XSS - Escape variables when sent to scripts

We will need to adapt Koha::CGI->param to work in list context (even
if it is considered bad), and explicitely call ->param_raw when we do
not want the escape to be done.
Comment 2 Katrin Fischer 2017-08-16 13:02:05 UTC
Ok, not totally sure if I understand this approach right, but I talked some to  Robin this morning while I was working on the XSS patches and from what I understand changing the data on the way is probably not the answer. We might want to use the data in different contexts where different encoding might be needed. Data needs to be encoded differently for use in HTML, attributes, JavaScript or in an URL. I am also thinking of our HTML preferences, CSV and file output, MARC data etc.

Robin suggested HTML::Escape as a fast module for escaping. If we wrap that into a plugin/make our own filter, we could maybe solve the performance issues:

http://search.cpan.org/~tokuhirom/HTML-Escape-1.09/lib/HTML/Escape.pm
Comment 3 Marcel de Rooy 2017-08-16 13:25:18 UTC
Or only pragmatically remove <script>..</script> constructions from parameters now with Koha::CGI?
Comment 4 Jonathan Druart 2017-08-16 13:31:29 UTC
(In reply to Katrin Fischer from comment #2)
> Ok, not totally sure if I understand this approach right, but I talked some
> to  Robin this morning while I was working on the XSS patches and from what
> I understand changing the data on the way is probably not the answer. We
> might want to use the data in different contexts where different encoding
> might be needed. Data needs to be encoded differently for use in HTML,
> attributes, JavaScript or in an URL. I am also thinking of our HTML
> preferences, CSV and file output, MARC data etc.

That is why there is a Koha::CGI->param_raw method

> Robin suggested HTML::Escape as a fast module for escaping. If we wrap that
> into a plugin/make our own filter, we could maybe solve the performance
> issues:
> 
> http://search.cpan.org/~tokuhirom/HTML-Escape-1.09/lib/HTML/Escape.pm

Nope, IIRC it is not faster than Template::Stash::AutoEscaping::Escaped::HTML::espape (see the patch).
I tried to improve the escapement on bug 13618. The speed was not the problem, the number of variables was.
Comment 5 Jonathan Druart 2017-08-16 13:42:20 UTC
(In reply to Marcel de Rooy from comment #3)
> Or only pragmatically remove <script>..</script> constructions from
> parameters now with Koha::CGI?

It is not only script elements, we need to escape all HTML characters.
Comment 6 Robin Sheat 2017-08-16 14:10:57 UTC
You can't process the data on the way in.

You will end up with corrupt data:

* in the database
* output via APIs
* in the web display whenever you're doing anything that isn't straight HTML (JSON, javascript, URLs, attributes, ...)

HTML::Escape is XS, so likely to be faster than a pure-perl implementation. If it's the amount of iterations that is the problem, then you'll probably need to remove the filtering from those parts, assuming they're safe, and perhaps filter them on the way to the template engine if that's faster. But these should be special cases in general.

> It is not only script elements, we need to escape all HTML characters.
That isn't true though. You need to escape only in HTML, and you mustn't escape for things that aren't HTML.

For reference, the way we did this recently is loosely:

* A script added a '| maybexss' filter to all template variables[0]. This filter does not do any HTML escaping (i.e. things shouldn't break.)
* Filters were added: '|n' for things that should be HTML (i.e. no filter), '|attr' for HTML attribute values, ones for JS numbers, strings, bools.
* The default for templates was set to escape any variables.[1]
* Whenever anyone saw a '|maybexss', they removed it if it shouldn't be HTML, or replaced it with one of the above, or if possible refactored it to not output the backend-supplied content (e.g. if it's possible to make it an 'if' condition with fixed answers.)
* Measuring the amount of maybexsses remaining was an indicator of how far through the cleanup was.

[0] this was done with many one/several-file commits, so if something went really bad, it could be reverted for them.
[1] I think that this was done with a flag at the top of the file, so that it could be done progressively.
Comment 7 Jonathan Druart 2017-08-22 15:25:57 UTC
Hi Robin,

Thanks for your input!

I have to admit that I should have explained what I have in mind a bit more.
At the moment we are facing lot of XSS vulnerabilities caused by input parameters we sent to the template:
  $template->param( foo => scalar $cgi->param('foo') )
and them [% foo %]

That is our main problem.

Of course the idea was not to insert escaped strings into the DB, but to trust data from DB (inserted from staff, not OPAC), and not url parameters.
Which is wrong (!) and just move the escape problem to somewhere else (we will have to list the variable we trust and the ones we do not). I am obsoleting my proposal.

Fixing XSS incrementally scared me. First it will be long and hard to be exhaustive, then we will not be regression proof.
Comment 8 Robin Sheat 2017-08-22 15:37:50 UTC
It will be a long and annoying process, but if done right then it'll be very hard for someone to introduce a new vulnerability by accident.
Comment 9 Chris Cormack 2017-08-27 22:08:50 UTC
(In reply to Jonathan Druart from comment #7)
> Hi Robin,
> 
> Thanks for your input!
> 
> I have to admit that I should have explained what I have in mind a bit more.
> At the moment we are facing lot of XSS vulnerabilities caused by input
> parameters we sent to the template:
>   $template->param( foo => scalar $cgi->param('foo') )
> and them [% foo %]
> 
> That is our main problem.
> 
> Of course the idea was not to insert escaped strings into the DB, but to
> trust data from DB (inserted from staff, not OPAC), and not url parameters.
> Which is wrong (!) and just move the escape problem to somewhere else (we
> will have to list the variable we trust and the ones we do not). I am
> obsoleting my proposal.
> 
> Fixing XSS incrementally scared me. First it will be long and hard to be
> exhaustive, then we will not be regression proof.

This won't save us from stored XSS either, I think scripting adding the |something to all non filtered output and changing the ones we don't want is probably the safest way to go.

Long and annoying, but in the end easy to test for with QA scripts.
Comment 10 Jonathan Druart 2019-11-27 10:30:19 UTC
Hum, that was wrong!
We dealt with that on bug 13618 (second try).

*** This bug has been marked as a duplicate of bug 13618 ***