Bug 16069 - XSS issue in basket.pl page
Summary: XSS issue in basket.pl page
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Amit Gupta
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 14568
  Show dependency treegraph
 
Reported: 2016-03-14 17:21 UTC by Amit Gupta
Modified: 2019-10-14 19:56 UTC (History)
15 users (show)

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


Attachments
Bug 16069 - XSS issue in basket.pl page (1.24 KB, patch)
2017-08-11 15:56 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 16069 - XSS issue in basket.pl page (1.29 KB, patch)
2017-08-11 20:05 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 16069 - XSS issue in basket.pl page (1.36 KB, patch)
2017-08-12 22:37 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 16069 - XSS issue in basket.pl page (3.71 KB, patch)
2017-08-15 17:31 UTC, Amit Gupta
Details | Diff | Splinter Review
[SIGNED OFF] Bug 16069 - XSS issue in basket.pl page (3.78 KB, patch)
2017-08-16 09:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 16069 - XSS issue in basket.pl page (3.86 KB, patch)
2017-08-16 11:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[16.11.x] Bug 16069 - XSS issue in basket.pl page (3.85 KB, patch)
2017-08-20 15:24 UTC, Amit Gupta
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Amit Gupta 2016-03-14 17:21:03 UTC
XSS issue in basket.pl page
Comment 1 Chris Cormack 2016-03-14 19:25:54 UTC
Amit is working on a patch, will be here tomorrow.
Comment 2 Jonathan Druart 2017-08-07 12:46:47 UTC
Amit, Is this still valid?
Comment 3 Amit Gupta 2017-08-07 12:55:56 UTC
Hi Jonathan,

Currently we are doing some security related testing for one of our client. I am planning to send all the patches to community. 

Please give us some time we will check and update the same.
Comment 4 Amit Gupta 2017-08-11 15:56:49 UTC
Created attachment 65919 [details] [review]
Bug 16069 - XSS issue in basket.pl page

Used HTML::Entities - Encode or decode strings with HTML entities.
*********************************************************

1. Hit /cgi-bin/koha/acqui/basket.pl?basketno=xx<script>alert('amit')</script>.
   xx - is a basketno
2. Notice the java script is executed.
4. Apply patch.
5. Reload page, and hit the page again /cgi-bin/koha/acqui/basket.pl?basketno==xx<script>alert('amit')</script>.
   xx - is a basketno.
6. Notice it is no longer executed.
Comment 5 Chris Cormack 2017-08-11 20:05:55 UTC
Created attachment 65931 [details] [review]
Bug 16069 - XSS issue in basket.pl page

1. Hit /cgi-bin/koha/acqui/basket.pl?basketno=xx<script>alert('amit')</script>.
   xx - is a basketno
2. Notice the java script is executed.
4. Apply patch.
5. Reload page, and hit the page again /cgi-bin/koha/acqui/basket.pl?basketno==xx<script>alert('amit')</script>.
   xx - is a basketno.
6. Notice it is no longer executed.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 6 Brendan Gallagher 2017-08-12 22:37:53 UTC
Created attachment 65940 [details] [review]
Bug 16069 - XSS issue in basket.pl page

1. Hit /cgi-bin/koha/acqui/basket.pl?basketno=xx<script>alert('amit')</script>.
   xx - is a basketno
2. Notice the java script is executed.
4. Apply patch.
5. Reload page, and hit the page again /cgi-bin/koha/acqui/basket.pl?basketno==xx<script>alert('amit')</script>.
   xx - is a basketno.
6. Notice it is no longer executed.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>
Comment 7 Jonathan Druart 2017-08-15 16:31:00 UTC
On master I get:
Can't call method "active" on an undefined value at /home/vagrant/kohaclone/acqui/basket.pl line 392.

This is not exploitable.
We should handle the case the basketno does not match a basket in DB instead.
Comment 8 Jonathan Druart 2017-08-15 16:35:33 UTC
Sorry, xx needs to be a valid basketno
Comment 9 Jonathan Druart 2017-08-15 16:57:15 UTC
I would prefer to stick to the current pattern (escape using the html filter in templates), but in this case it does not seem easy (lot of occurrences in the template).
However it seems that at least booksellerid should be escaped as well.
Comment 10 Amit Gupta 2017-08-15 17:31:37 UTC
Created attachment 66032 [details] [review]
Bug 16069 - XSS issue in basket.pl page

1. Hit /cgi-bin/koha/acqui/basket.pl?basketno=xx<script>alert('amit')</script>
   xx - is a basketno
2. Notice the java script is executed.
3. Apply patch.
4. Reload page, and hit the page again /cgi-bin/koha/acqui/basket.pl?basketno==xx<script>alert('amit')</script>
   xx - is a basketno.
5. Notice it is no longer executed.

**********************************
Thanks Jonathan for your suggestion.

escape is done from the template.
Comment 11 Katrin Fischer 2017-08-16 09:07:06 UTC
Created attachment 66049 [details] [review]
[SIGNED OFF] Bug 16069 - XSS issue in basket.pl page

1. Hit /cgi-bin/koha/acqui/basket.pl?basketno=xx<script>alert('amit')</script>
   xx - is a basketno
2. Notice the java script is executed.
3. Apply patch.
4. Reload page, and hit the page again /cgi-bin/koha/acqui/basket.pl?basketno==xx<script>alert('amit')</script>
   xx - is a basketno.
5. Notice it is no longer executed.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Marcel de Rooy 2017-08-16 11:36:02 UTC
Created attachment 66068 [details] [review]
Bug 16069 - XSS issue in basket.pl page

1. Hit /cgi-bin/koha/acqui/basket.pl?basketno=xx<script>alert('amit')</script>
   xx - is a basketno
2. Notice the java script is executed.
3. Apply patch.
4. Reload page, and hit the page again /cgi-bin/koha/acqui/basket.pl?basketno==xx<script>alert('amit')</script>
   xx - is a basketno.
5. Notice it is no longer executed.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Katrin Fischer 2017-08-20 13:43:59 UTC
This doesn't apply cleanly to 16.11.x, some help appreciated:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 16069 - XSS issue in basket.pl page
fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt).
error: could not build fake ancestor
Patch failed at 0001 Bug 16069 - XSS issue in basket.pl page
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-16069---XSS-issue-in-basketpl-page-bKPt_8.patch
Comment 14 Amit Gupta 2017-08-20 15:24:41 UTC
Created attachment 66243 [details] [review]
[16.11.x] Bug 16069 - XSS issue in basket.pl page

Fixed for 16.11.x
Comment 15 Katrin Fischer 2017-08-20 16:29:16 UTC
Thx, worked perfectly!
Comment 16 Jonathan Druart 2017-08-29 15:07:08 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 17 Fridolin Somers 2017-09-05 12:44:10 UTC
Pushed to 17.05.x, is in 17.05.03
Comment 18 Katrin Fischer 2017-09-07 06:17:25 UTC
This patch has been pushed to 16.11.x and is in 16.11.11.
Comment 19 Mason James 2017-09-20 02:40:34 UTC
Pushed to 16.05.x, for 16.05.16 release - thanks Amit :0)