Bug 19089

Summary: XSS issue in supplier.pl page
Product: Koha Reporter: Amit Gupta <amitddng135>
Component: Architecture, internals, and plumbingAssignee: Amit Gupta <amitddng135>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: amitddng135, chris, fridolin.somers, jonathan.druart, katrin.fischer, m.de.rooy, mtj, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 14568    
Attachments: Bug 19089 - XSS issue in supplier.pl page
Bug 19089 - XSS issue in supplier.pl page
Bug 19089 - XSS issue in supplier.pl page
Bug 19089 - XSS issue in supplier.pl page

Description Amit Gupta 2017-08-12 03:39:05 UTC
XSS issue in supplier.pl page
Comment 1 Amit Gupta 2017-08-12 03:43:16 UTC
Created attachment 65933 [details] [review]
Bug 19089 - XSS issue in supplier.pl page

Used HTML::Entities - Encode or decode strings with HTML entities.
******************************************************************
1. Hit /cgi-bin/koha/acqui/supplier.pl?booksellerid=xx<script>alert('amit')</script>.
   xx - is a booksellerid
2. Notice the java script is executed.
4. Apply patch.
5. Reload page, and hit the page again /cgi-bin/koha/acqui/supplier.pl?booksellerid=xx<script>alert('amit')</script>.
   xx - is a booksellerid.
6. Notice it is no longer executed.
Comment 2 Amit Gupta 2017-08-12 03:46:36 UTC
Created attachment 65934 [details] [review]
Bug 19089 - XSS issue in supplier.pl page

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

1. Hit /cgi-bin/koha/acqui/supplier.pl?booksellerid=xx<script>alert('amit')</script>.
   xx - is a booksellerid
2. Notice the java script is executed.
4. Apply patch.
5. Reload page, and hit the page again /cgi-bin/koha/acqui/supplier.pl?booksellerid=xx<script>alert('amit')</script>.
   xx - is a booksellerid.
6. Notice it is no longer executed.
Comment 3 Chris Cormack 2017-08-12 04:43:29 UTC
Created attachment 65935 [details] [review]
Bug 19089 - XSS issue in supplier.pl page

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

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

We probably should use a whitelist ie, it should only be digits. Rather
than escaping it, but this works.
Comment 4 Amit Gupta 2017-08-14 14:32:24 UTC
Hi Jonathan,

Can you please add me on bug no 14568
Comment 5 Jonathan Druart 2017-08-15 15:57:52 UTC
We should stick to the pattern we use everywhere: escape from the template.

Or, and better here, I would suggest to send the param only if a vendor has been found:

my $booksellerid       = $query->param('booksellerid');
my $supplier;
if ($booksellerid) {
    $supplier = Koha::Acquisition::Booksellers->find( $booksellerid );
    # Send the booksellerid vars to template and do other actions
}


Note that here, that could be done removing the following line:
    $template->{'VARS'}->{'booksellerid'} = $booksellerid;

Second note: Currently it is not exploitable, if the $booksellerid does not correspond to a valid vendor, the ->unblessed statement will explode with
Can't call method "unblessed" on an undefined value at /home/vagrant/kohaclone/acqui/supplier.pl line 72.
Comment 6 Amit Gupta 2017-08-15 17:17:31 UTC
Created attachment 66030 [details] [review]
Bug 19089 - XSS issue in supplier.pl page

To Test

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

escape is done from the template
Comment 7 Jonathan Druart 2017-08-15 17:45:27 UTC
(In reply to Jonathan Druart from comment #5)
> Or, and better here, I would suggest to send the param only if a vendor has
> been found:
> 
> my $booksellerid       = $query->param('booksellerid');
> my $supplier;
> if ($booksellerid) {
>     $supplier = Koha::Acquisition::Booksellers->find( $booksellerid );
>     # Send the booksellerid vars to template and do other actions
> }
> 
> 
> Note that here, that could be done removing the following line:
>     $template->{'VARS'}->{'booksellerid'} = $booksellerid;
> 
> Second note: Currently it is not exploitable, if the $booksellerid does not
> correspond to a valid vendor, the ->unblessed statement will explode with
> Can't call method "unblessed" on an undefined value at
> /home/vagrant/kohaclone/acqui/supplier.pl line 72.

=> For the record: all of that was wrong:

> select count(*) from aqbooksellers where id="1<script>alert('XSS')</script>";
returns 1.
Comment 8 Katrin Fischer 2017-08-16 08:56:59 UTC
I can't reproduce the error here:
/cgi-bin/koha/acqui/supplier.pl?booksellerid=5<SCRIPT>alert("Test")</SCRIPT>
Results in bad display, but the alert is not executed.
Comment 9 Katrin Fischer 2017-08-16 12:03:54 UTC
Hm, tried again - still no luck in getting the alert executed.
Comment 10 Katrin Fischer 2017-10-07 18:33:10 UTC
Tried again, please take another look.
Comment 11 Jonathan Druart 2019-03-15 22:58:54 UTC
This has been solved by or prior bug 13618.