Bug 11322 - Suggestion "notes" field should be sanitized or escaped
Summary: Suggestion "notes" field should be sanitized or escaped
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Target Milestone: Oldversions
Assignee: Bugs List
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-29 17:39 UTC by Jacek Ablewicz
Modified: 2018-12-16 19:34 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 11322 : XSS in suggestions (5.38 KB, patch)
2013-12-02 22:36 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11322 : XSS in suggestions - OPAC (6.05 KB, patch)
2013-12-02 22:48 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11322 : XSS in suggestions (5.46 KB, patch)
2013-12-02 22:53 UTC, David Cook
Details | Diff | Splinter Review
Bug 11322 : XSS - Scrub bad data before storing suggestions in the db (1.49 KB, patch)
2013-12-02 23:04 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11322 : XSS in suggestions - OPAC (6.11 KB, patch)
2013-12-02 23:08 UTC, David Cook
Details | Diff | Splinter Review
Bug 11322 : XSS - Scrub bad data before storing suggestions in the db (1.58 KB, patch)
2013-12-02 23:12 UTC, David Cook
Details | Diff | Splinter Review
[PASSED QA] Bug 11322 : XSS in suggestions (5.55 KB, patch)
2013-12-02 23:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11322 : XSS in suggestions - OPAC (6.19 KB, patch)
2013-12-02 23:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11322 : XSS - Scrub bad data before storing suggestions in the db (1.63 KB, patch)
2013-12-02 23:51 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jacek Ablewicz 2013-11-29 17:39:18 UTC
It's possible for patron to make purchase suggestion from OPAC with html/javascript code within Notes: field. Such injected JS code will be stored in the database, and in certain circumstances (when managing suggestions in acquisition) it may got subsequently executed in staff WWW browser.

Other suggestion fields may be affected as well, but the problem with 'notes' is potentially more severe because it's a long field - more elaborate "evil" script will fit into it.
Comment 1 Galen Charlton 2013-12-02 21:29:04 UTC
Not just notes -- most of the fields in the suggestion form (except for title) aren't using the Template Toolkit HTML filter.  A combination of using C4::Scrubber and the HTML TT filter should provide belts and suspenders.
Comment 2 Chris Cormack 2013-12-02 22:15:00 UTC
Working on some patches for this now
Comment 3 Chris Cormack 2013-12-02 22:36:59 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2013-12-02 22:48:30 UTC Comment hidden (obsolete)
Comment 5 David Cook 2013-12-02 22:53:12 UTC Comment hidden (obsolete)
Comment 6 Chris Cormack 2013-12-02 23:04:26 UTC Comment hidden (obsolete)
Comment 7 David Cook 2013-12-02 23:08:46 UTC Comment hidden (obsolete)
Comment 8 David Cook 2013-12-02 23:12:14 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2013-12-02 23:51:03 UTC
Created attachment 23247 [details] [review]
[PASSED QA] Bug 11322 : XSS in suggestions

To test
1/ Switch on purchase suggestions
2/ On the public interface (OPAC) add a suggestion, put html in every
field
3/ In the staff interface go to the suggestions page
/cgi-bin/koha/suggestion/suggestion.pl
4/ Notice the html is rendered
5/ Click on a suggestion, notice the html is rendered on the show page
also
6/ Apply the patch, check these two pages again, html should now be
escaped

Signed-off-by: David Cook <dcook@prosentient.com.au>

Works as described.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests, thx Chris!
Comment 10 Katrin Fischer 2013-12-02 23:51:07 UTC
Created attachment 23248 [details] [review]
[PASSED QA] Bug 11322 : XSS in suggestions - OPAC

1/ Add a suggestion in the opac, with lots of html
2/ View that suggestion in the OPAC, note the html is rendering
3/ Apply the patch
4/ Test again, in prog and bootstrap, no more rendered html

Signed-off-by: David Cook <dcook@prosentient.com.au>

Works as described.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 11 Katrin Fischer 2013-12-02 23:51:10 UTC
Created attachment 23249 [details] [review]
[PASSED QA] Bug 11322 : XSS - Scrub bad data before storing suggestions in the db

1/ In the public interface, add a suggestion containing html
2/ Save, notice the html is rendered (or if you have the other patches
is displayed)
3/ Apply this paget
4/ Add another suggestion
5/ Notice the html is stripped

Signed-off-by: David Cook <dcook@prosentient.com.au>

Works as described.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 12 Galen Charlton 2013-12-03 00:23:03 UTC
Pushed to master.  Thanks, Chris!
Comment 13 Jacek Ablewicz 2013-12-03 15:19:03 UTC
Works as described for me too, thanks!

On related subject, I think we may have some similiar problems with patron-submitted data in other places as well, e.g.: as a test I just submitted "personal data" update request from OPAC via opac/opac-memberentry.pl, with JS code in 'Address 2' field, and injected script seems to work in members/members-update.pl. But I guess it would be better to assign separate bug numbers for such further issues (if any)?
Comment 14 Chris Cormack 2013-12-03 20:10:05 UTC
(In reply to Jacek Ablewicz from comment #13)
> Works as described for me too, thanks!
> 
> On related subject, I think we may have some similiar problems with
> patron-submitted data in other places as well, e.g.: as a test I just
> submitted "personal data" update request from OPAC via
> opac/opac-memberentry.pl, with JS code in 'Address 2' field, and injected
> script seems to work in members/members-update.pl. But I guess it would be
> better to assign separate bug numbers for such further issues (if any)?

Yep new bug on any others you find, i checked reviews/comments and tags, they are ok. But im prepared to believe member update isn't.
At least with that you have to be logged in, so not quite as bad as the suggestions (which could be done without login if the syspref was set that way)
Comment 15 Fridolin SOMERS 2013-12-13 15:20:11 UTC
Those patches has been pushed to 3.14.x, will be in 3.14.1
Comment 16 Tomás Cohen Arazi 2013-12-23 15:29:26 UTC
This patch has been pushed to 3.12.x, will be in 3.12.8.

Thanks Chris!