If I click in intranet "search the catalog" and type the following string "<script>alert(1);</script><script>alert(1);</script>" (with the quotes included) and then press enter I get the alert meaning javascript is executed from user input. This is bad because a library might receive a book with malicious barcode and those barcodes are automatically scanned and entered with a barcode reader directly to the search box.
Created attachment 107579 [details] [review] Bug 26102: Prevent XSS when To.json is used
This patch should fixed the issue you reported, but then I found other occurrences. It can be tricky to test them all (which I did not do), and hard to make sure it won't introduce regressions. For each of the occurrences, git history must be examined to confirm we won't reintroduce an issue we fixed earlier. Leaving the patch here for now, but it's ready to test if anyone is willing to give it a try. Basically we want to test: <script>, quotes and correct utf8 encoding
Created attachment 108083 [details] [review] Bug 26102: Prevent XSS when To.json is used: admin/preferences.tt Test that preference search term highlighting works correctly.
Created attachment 108084 [details] [review] Bug 26102: Prevent XSS when To.json is used: authorities/authorities.tt Check that mandatory tags and subfields are correctly required when editing an authority record.
Created attachment 108085 [details] [review] Bug 26102: Prevent XSS when To.json is used: authorities/blinddetail-biblio-search.tt Test the process of searching for and selecting an authority record for use in the basic MARC editor.
Created attachment 108086 [details] [review] Bug 26102: Prevent XSS when To.json is used: catalogue/results.tt To test, perform a search in the catalogue and verify that search term highlighting works correctly.
Created attachment 108087 [details] [review] Bug 26102: Prevent XSS when To.json is used: guarantor_search.tt To test, edit a patron record and go through the process of adding a guarantor. In the guarantor search results table the address should be displayed correctly.
Created attachment 108088 [details] [review] Bug 26102: Prevent XSS when To.json is used: subscription-add.tt Test the process of adding a subscription, entering both a valid vendor ID and a non-existent vendor ID. The non-existent vendor ID should trigger a validation alert.
Created attachment 108089 [details] [review] Bug 26102: Prevent XSS when To.json is used: suggester_search.tt Test the process of adding a new suggestion and changing the "created by" patron using the "Set to patron" button. The patron search results should be displayed correctly in the pop up window.
A couple of files in the original patch will break if they're updated and should be safe if left alone: koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/itemsearch.tt koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt The data being processed by To.json is coming from Koha data like branch codes, authorized values, etc. Updating the filter on members/memberentrygen.tt makes the page break. Having the filter as "$raw" leaves the possibility of an XSS attack but it's convoluted: 1. Put the XSS <script> tag in to a field of an existing patron. 2. Create a new patron. 3. Fill in details for the patron which will trigger a "duplicate patron" warning. 4. Add as a guarantor on that record the patron from step 1. 5. Save the new patron record. 6. When the patron entry form is re-displayed with the duplicate patron warning message the <script> from step 1 will run. These files which were changed in the original patch need to be addressed in a follow-up: /prog/en/includes/js_includes.inc /value_builder/unimarc_field_4XX.tt /bootstrap/en/modules/sco/sco-main.tt
I think the change to js_includes.inc and sco-main.tt are also unnecessary. The data being processed by the filter is coming directly from the Audio Alerts system preference: [{\"selector\":\".audio-alert-action\",\"id\":1}] You could insert a <script> tag into the "selector" field but you'd have to be a Koha admin to do it.
Created attachment 108090 [details] [review] Bug 26102: Prevent XSS when To.json is used: unimarc_field_4XX.tt To test, edit a MARC framework to link a subfield to the unimarc_field_4XX.tt. The process of triggering the plugin and selecting a search result from the plugin popup should work correctly.
Just to understand, in koha-tmpl/intranet-tmpl/prog/en/modules/members/tables/guarantor_search.tt -[%~ To.json( address ) | $raw ~%] +[%~ address | html | $To ~%] Why not : -[%~ To.json( address ) | $raw ~%] +[%~ To.json( address ) | html ~%]
Because it must be html escaped first. Please try: <script>alert('booh! ❤');</script> Note that: <script>alert('booh! ❤');</script> still does not work.
Because it must be html escaped first. Please try: <script>alert('booh! ❤');</script> Note that: <script>alert("booh! ❤");</script> still does not work.
(In reply to Jonathan Druart from comment #15) > Because it must be html escaped first. > > Please try: > <script>alert('booh! ❤');</script> > > Note that: <script>alert("booh! ❤");</script> > still does not work. Ah indeed i understand now, koha-tmpl/intranet-tmpl/prog/en/modules/members/tables/guarantor_search.tt generates a JSON. Thanks ;)
Ah but in other places, (In reply to Jonathan Druart from comment #15) > Because it must be html escaped first. Sorry but why not always in this order, why only for tables ?
Please try the different filter combinations to understand it :)
Created attachment 119643 [details] [review] Bug 26102: Prevent XSS when To.json is used: admin/preferences.tt Test that preference search term highlighting works correctly.
Created attachment 119644 [details] [review] Bug 26102: Prevent XSS when To.json is used: authorities/authorities.tt Check that mandatory tags and subfields are correctly required when editing an authority record.
Created attachment 119645 [details] [review] Bug 26102: Prevent XSS when To.json is used: authorities/blinddetail-biblio-search.tt Test the process of searching for and selecting an authority record for use in the basic MARC editor.
Created attachment 119646 [details] [review] Bug 26102: Prevent XSS when To.json is used: catalogue/results.tt To test, perform a search in the catalogue and verify that search term highlighting works correctly.
Created attachment 119647 [details] [review] Bug 26102: Prevent XSS when To.json is used: guarantor_search.tt To test, edit a patron record and go through the process of adding a guarantor. In the guarantor search results table the address should be displayed correctly.
Created attachment 119648 [details] [review] Bug 26102: Prevent XSS when To.json is used: subscription-add.tt Test the process of adding a subscription, entering both a valid vendor ID and a non-existent vendor ID. The non-existent vendor ID should trigger a validation alert.
Created attachment 119649 [details] [review] Bug 26102: Prevent XSS when To.json is used: unimarc_field_4XX.tt To test, edit a MARC framework to link a subfield to the unimarc_field_4XX.tt. The process of triggering the plugin and selecting a search result from the plugin popup should work correctly.
Amazingly, these patches still apply!
Created attachment 127669 [details] [review] Bug 26102: Prevent XSS when To.json is used: admin/preferences.tt Test that preference search term highlighting works correctly. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 127670 [details] [review] Bug 26102: Prevent XSS when To.json is used: authorities/authorities.tt Check that mandatory tags and subfields are correctly required when editing an authority record. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 127671 [details] [review] Bug 26102: Prevent XSS when To.json is used: authorities/blinddetail-biblio-search.tt Test the process of searching for and selecting an authority record for use in the basic MARC editor. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 127672 [details] [review] Bug 26102: Prevent XSS when To.json is used: catalogue/results.tt To test, perform a search in the catalogue and verify that search term highlighting works correctly. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 127673 [details] [review] Bug 26102: Prevent XSS when To.json is used: guarantor_search.tt To test, edit a patron record and go through the process of adding a guarantor. In the guarantor search results table the address should be displayed correctly. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 127674 [details] [review] Bug 26102: Prevent XSS when To.json is used: subscription-add.tt Test the process of adding a subscription, entering both a valid vendor ID and a non-existent vendor ID. The non-existent vendor ID should trigger a validation alert. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 127675 [details] [review] Bug 26102: Prevent XSS when To.json is used: unimarc_field_4XX.tt To test, edit a MARC framework to link a subfield to the unimarc_field_4XX.tt. The process of triggering the plugin and selecting a search result from the plugin popup should work correctly. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 128716 [details] [review] Bug 26102: Prevent XSS when To.json is used: admin/preferences.tt Test that preference search term highlighting works correctly. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 128717 [details] [review] Bug 26102: Prevent XSS when To.json is used: authorities/authorities.tt Check that mandatory tags and subfields are correctly required when editing an authority record. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 128718 [details] [review] Bug 26102: Prevent XSS when To.json is used: authorities/blinddetail-biblio-search.tt Test the process of searching for and selecting an authority record for use in the basic MARC editor. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 128719 [details] [review] Bug 26102: Prevent XSS when To.json is used: catalogue/results.tt To test, perform a search in the catalogue and verify that search term highlighting works correctly. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 128720 [details] [review] Bug 26102: Prevent XSS when To.json is used: guarantor_search.tt To test, edit a patron record and go through the process of adding a guarantor. In the guarantor search results table the address should be displayed correctly. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 128721 [details] [review] Bug 26102: Prevent XSS when To.json is used: subscription-add.tt Test the process of adding a subscription, entering both a valid vendor ID and a non-existent vendor ID. The non-existent vendor ID should trigger a validation alert. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 128722 [details] [review] Bug 26102: Prevent XSS when To.json is used: unimarc_field_4XX.tt To test, edit a MARC framework to link a subfield to the unimarc_field_4XX.tt. The process of triggering the plugin and selecting a search result from the plugin popup should work correctly. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Backported: Pushed to 20.11.x-security branch for 20.11.14 I checked that the attack doesn't work anymore on intranet search. And on the address of the guarantor search result. And was able to check that basic usage still works using what was written in the commit messages. Note: a unimarc env was used for «Prevent XSS when To.json is used: unimarc_field_4XX.tt» and 410$a is already setup to use the plugin. IMHO other RMaints not need to worry about checking this one if they don't have a straightforward way to have an unimarc env.
Backported to 19.11.x-security branch on the Security repo for 19.11.25 security release.
Pushed to security/21.05.x for 21.05.09
Reverted commits on 19.11.x
(In reply to wainuiwitikapark from comment #44) > Reverted commits on 19.11.x We have a problem reported with this patch set on bug 30044 - so we probably need a follow-up or adjusted set of patches.
Created attachment 130931 [details] [review] Bug 26102: [19.11] Prevent XSS when To.json is used: admin/preferences.tt Test that preference search term highlighting works correctly.
Created attachment 130932 [details] [review] Bug 26102: [19.11] Prevent XSS when To.json is used: authorities/authorities.tt Check that mandatory tags and subfields are correctly required when editing an authority record.
Created attachment 130933 [details] [review] Bug 26102: [19.11] Prevent XSS when To.json is used: authorities/blinddetail-biblio-search.tt Test the process of searching for and selecting an authority record for use in the basic MARC editor.
Created attachment 130934 [details] [review] Bug 26102: [19.11] Prevent XSS when To.json is used: catalogue/results.tt To test, perform a search in the catalogue and verify that search term highlighting works correctly.
Created attachment 130935 [details] [review] Bug 26102: [19.11] Prevent XSS when To.json is used: guarantor_search.tt To test, edit a patron record and go through the process of adding a guarantor. In the guarantor search results table the address should be displayed correctly.
Created attachment 130936 [details] [review] Bug 26102: [19.11] Prevent XSS when To.json is used: unimarc_field_4XX.tt To test, edit a MARC framework to link a subfield to the unimarc_field_4XX.tt. The process of triggering the plugin and selecting a search result from the plugin popup should work correctly.
Patches adjusted for 19.11. The following patch is not relevant for this branch: Bug 26102: Prevent XSS when To.json is used: subscription-add.tt
Created attachment 130999 [details] [review] Bug 26102: [19.11] Prevent XSS when To.json is used: admin/preferences.tt Test that preference search term highlighting works correctly. Signed-off-by: Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz>
Created attachment 131000 [details] [review] Bug 26102: [19.11] Prevent XSS when To.json is used: authorities/authorities.tt Check that mandatory tags and subfields are correctly required when editing an authority record. Signed-off-by: Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz>
Created attachment 131001 [details] [review] Bug 26102: [19.11] Prevent XSS when To.json is used: authorities/blinddetail-biblio-search.tt Test the process of searching for and selecting an authority record for use in the basic MARC editor. Signed-off-by: Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz>
Created attachment 131002 [details] [review] Bug 26102: [19.11] Prevent XSS when To.json is used: catalogue/results.tt To test, perform a search in the catalogue and verify that search term highlighting works correctly. Signed-off-by: Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz>
Created attachment 131003 [details] [review] Bug 26102: [19.11] Prevent XSS when To.json is used: guarantor_search.tt To test, edit a patron record and go through the process of adding a guarantor. In the guarantor search results table the address should be displayed correctly. Signed-off-by: Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz>
Created attachment 131004 [details] [review] Bug 26102: [19.11] Prevent XSS when To.json is used: unimarc_field_4XX.tt To test, edit a MARC framework to link a subfield to the unimarc_field_4XX.tt. The process of triggering the plugin and selecting a search result from the plugin popup should work correctly. Signed-off-by: Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz>
Applied 19.11 patches to 19.11.x branch