https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19121 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |In Discussion CC| |julian.maurice@biblibre.com | |, m.de.rooy@rijksmuseum.nl, | |martin.renvoize@ptfs-europe | |.com, tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19121 --- Comment #2 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19121 --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19121 --- Comment #6 from Robin Sheat <robin@kallisti.net.nz> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19121 --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19121 --- Comment #8 from Robin Sheat <robin@kallisti.net.nz> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19121 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |chris@bigballofwax.co.nz --- Comment #9 from Chris Cormack <chris@bigballofwax.co.nz> --- (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. -- You are receiving this mail because: You are watching all bug changes.