The CSV file upload functionality in Koha exposes the risk of CSV files containing a JavaScript payload in a csv field. For example, the 'Quotes' feature is vulnerable to this; Uploading a CSV file of quotes and having one of those include a <Script> tag, the script will get run upon display.
Created attachment 118363 [details] [review] Bug 27942: Prevent XSS vulnerabilities in quote-upload When uploading new quotes the JS variables are not escaped correctly. Test plan: Create a new file with the following content (remove the first and last lines containing """): """ ":this is a source","this is a text" "this is another ❤one","and another text❤" "this <script>alert('foo');</script>","and <script>alert('❤');</script>" """ Go to Home › Tools › Quote editor Click Import quote Select the file Edit the third line, hit enter Import quotes
Created attachment 118867 [details] [review] Bug 27942: Prevent XSS vulnerabilities in quote-upload When uploading new quotes the JS variables are not escaped correctly. Test plan: Create a new file with the following content (remove the first and last lines containing """): """ ":this is a source","this is a text" "this is another ❤one","and another text❤" "this <script>alert('foo');</script>","and <script>alert('❤');</script>" """ Go to Home › Tools › Quote editor Click Import quote Select the file Edit the third line, hit enter Import quotes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Thanks for jumping on this Jonathan.. :) Works as expected, Signing off.
Created attachment 119043 [details] [review] Bug 27942: Prevent XSS vulnerabilities in quote-upload When uploading new quotes the JS variables are not escaped correctly. Test plan: Create a new file with the following content (remove the first and last lines containing """): """ ":this is a source","this is a text" "this is another ❤one","and another text❤" "this <script>alert('foo');</script>","and <script>alert('❤');</script>" """ Go to Home › Tools › Quote editor Click Import quote Select the file Edit the third line, hit enter Import quotes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Confirmed to affect 19.11 (upload screen, quote list, not the OPAC display of the quote) Couldn't backport after some fiddling due to the rewrite of bug 27251, which is in master. So it should be hard to backport for all branches.
tried another way but bug 27251 doesn't apply on 19.11.x
(In reply to Victor Grousset/tuxayo from comment #5) > Confirmed to affect 19.11 (upload screen, quote list, not the OPAC display > of the quote) > Couldn't backport after some fiddling due to the rewrite of bug 27251, which > is in master. So it should be hard to backport for all branches. Same for 20.11.x, bug 27251 is only in master/21.05.
Frido, does bug 27251 applies without much issues on 20.05.x?
(In reply to Victor Grousset/tuxayo from comment #8) > Frido, does bug 27251 applies without much issues on 20.05.x? You mean 20.11.x It is a medium patch with some dependancies, not a good idea to backport on a stable :/
> You mean 20.11.x indeed! > It is a medium patch with some dependancies, not a good idea to backport on a stable :/ +1 lol it depends on bug 27247 which is only in 20.05, that's why I couldn't apply bug 27251
Created attachment 120429 [details] [review] Bug 27942: [20.11] Prevent XSS vulnerabilities in quote-upload
Can you apply and test this patch on stables?
Applies to 19.11 :D However I have the error "escape_str is not defined" on Firefox and Chromium
We might have an issue with the scope of the bug. Malicious CSV files won't be an issue anymore. But the client-side can still do XSS attacks. So a librarian getting it's password stolen by - a dictionary of common passwords - social engineering - fishing - a password leak from another website - a malware Or just getting a malware doing the job directly. Will cause a breach allowing to perform XSS attacks. It seems even more probable (access complexity) than getting a malicious CSV
Publishing the fix for the CSV upload would give a hint about the XSS still possible. Even if my comments are kept private.
Created attachment 120482 [details] [review] Bug 27942: [20.11] Prevent XSS vulnerabilities in quote-upload
For escape_str see bug 27930, or create it. You need function escape_str(s){ return s != null ? s.escapeHtml() : ""; } in staff-global.js
Created attachment 120726 [details] [review] Bug 27942: [20.11] Prevent XSS vulnerabilities in quote-upload Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 120727 [details] [review] Bug 27942: [<=20.05] Add escape_str() in staff-global.js It was added in bug 27930 which in not in <= 20.05
> For escape_str see bug 27930, or create it. Thanks, it works! :) IIUC the new code in koha-tmpl/intranet-tmpl/prog/en/modules/tools/quotes.tt is too also escape the display of the quotes in the editor/list so XSS attacks won't work via the ajax request. (addresses comment 14)
Pushed to master for 21.05, thanks to everybody involved!
Hi :) Manual quote addition is broken with patch «[20.11] Prevent XSS vulnerabilities in quote-upload» See bug 28675 , any ideas/analysis/help welcome :)