Bug 27942

Summary: QOTD: quote CSV uploads may contain JavaScript payloads (XSS)
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Julian Maurice <julian.maurice>
Severity: normal    
Priority: P5 - low CC: 1joynelson, fridolin.somers, jonathan.druart, julian.maurice, katrin.fischer, lucas, martin.renvoize, nick, victor
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27251
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.06,20.05.12,19.11.18
Bug Depends on:    
Bug Blocks: 28675    
Attachments: Bug 27942: Prevent XSS vulnerabilities in quote-upload
Bug 27942: Prevent XSS vulnerabilities in quote-upload
Bug 27942: Prevent XSS vulnerabilities in quote-upload
Bug 27942: [20.11] Prevent XSS vulnerabilities in quote-upload
Bug 27942: [20.11] Prevent XSS vulnerabilities in quote-upload
Bug 27942: [20.11] Prevent XSS vulnerabilities in quote-upload
Bug 27942: [<=20.05] Add escape_str() in staff-global.js

Description Martin Renvoize 2021-03-12 10:58:02 UTC
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.
Comment 1 Jonathan Druart 2021-03-17 10:42:18 UTC
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
Comment 2 Martin Renvoize 2021-03-26 15:25:16 UTC
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>
Comment 3 Martin Renvoize 2021-03-26 15:25:48 UTC
Thanks for jumping on this Jonathan.. :)

Works as expected, Signing off.
Comment 4 Julian Maurice 2021-03-31 12:03:23 UTC
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>
Comment 5 Victor Grousset/tuxayo 2021-04-23 17:42:25 UTC
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.
Comment 6 Victor Grousset/tuxayo 2021-04-23 17:44:35 UTC
tried another way but bug 27251 doesn't apply on 19.11.x
Comment 7 Fridolin Somers 2021-04-26 08:01:24 UTC
(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.
Comment 8 Victor Grousset/tuxayo 2021-04-29 16:14:20 UTC
Frido, does bug 27251 applies without much issues on 20.05.x?
Comment 9 Fridolin Somers 2021-04-30 14:48:18 UTC
(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 :/
Comment 10 Victor Grousset/tuxayo 2021-04-30 15:29:41 UTC
> 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
Comment 11 Jonathan Druart 2021-05-04 09:48:46 UTC
Created attachment 120429 [details] [review]
Bug 27942: [20.11] Prevent XSS vulnerabilities in quote-upload
Comment 12 Jonathan Druart 2021-05-04 09:49:21 UTC
Can you apply and test this patch on stables?
Comment 13 Victor Grousset/tuxayo 2021-05-04 22:56:27 UTC
Applies to 19.11 :D

However I have the error "escape_str is not defined" on Firefox and Chromium
Comment 14 Victor Grousset/tuxayo 2021-05-04 23:10:50 UTC
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
Comment 15 Victor Grousset/tuxayo 2021-05-04 23:12:54 UTC
Publishing the fix for the CSV upload would give a hint about the XSS still possible. Even if my comments are kept private.
Comment 16 Jonathan Druart 2021-05-05 06:58:50 UTC
Created attachment 120482 [details] [review]
Bug 27942: [20.11] Prevent XSS vulnerabilities in quote-upload
Comment 17 Jonathan Druart 2021-05-05 06:59:54 UTC
For escape_str see bug 27930, or create it.

You need
    function escape_str(s){
      return s != null ? s.escapeHtml() : "";
    }
in staff-global.js
Comment 18 Victor Grousset/tuxayo 2021-05-08 21:33:52 UTC
Created attachment 120726 [details] [review]
Bug 27942: [20.11] Prevent XSS vulnerabilities in quote-upload

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 19 Victor Grousset/tuxayo 2021-05-08 21:33:56 UTC
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
Comment 20 Victor Grousset/tuxayo 2021-05-08 21:38:44 UTC
> 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)
Comment 21 Jonathan Druart 2021-05-26 07:50:38 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 22 Victor Grousset/tuxayo 2021-07-09 01:54:25 UTC
Hi :)
Manual quote addition is broken with patch «[20.11] Prevent XSS vulnerabilities in quote-upload»

See bug 28675 , any ideas/analysis/help welcome :)