Bug 36382

Created attachment 163615 [details] [review]
Bug 36382: XSS in showLastPatron dropdown

1) Set borrower surname to:
    <script>alert("here comes trouble");</script>
2) Save, nothing happens
3) Enable showLastPatron
4) Reload patron
5) Note the alert popup
6) Apply this patch
7) Reload patron
8) No alert!

Created attachment 163727 [details] [review]
Bug 36382: XSS in showLastPatron dropdown

1) Set borrower surname to:
    <script>alert("here comes trouble");</script>
2) Save, nothing happens
3) Enable showLastPatron
4) Reload patron
5) Note the alert popup
6) Apply this patch
7) Reload patron
8) No alert!

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Created attachment 164119 [details] [review]
Bug 36382: XSS in showLastPatron dropdown

1) Set borrower surname to:
    <script>alert("here comes trouble");</script>
2) Save, nothing happens
3) Enable showLastPatron
4) Reload patron
5) Note the alert popup
6) Apply this patch
7) Reload patron
8) No alert!

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

+    "'": ''',
+    '"': '"'

Do you really estimate the possible regression from this? maybe we are already escaping them in some places and they will get double escaped.

(In reply to Jonathan Druart from comment #4)
> +    "'": ''',
> +    '"': '"'
> 
> Do you really estimate the possible regression from this? maybe we are
> already escaping them in some places and they will get double escaped.

Good point

Created attachment 164123 [details] [review]
Bug 36382 (QA follow-up) Don't escape quotes in escapeHtml

(In reply to Jonathan Druart from comment #4)
> +    "'": ''',
> +    '"': '"'
> 
> Do you really estimate the possible regression from this? maybe we are
> already escaping them in some places and they will get double escaped.

Where are we escaping quotes? A grep grep found function encodeHTMLEntities in supplier.tt. HTML escaping really needs to be unified, making bug 36385 all the more important.

(In reply to Kyle M Hall from comment #7)
> Where are we escaping quotes? A grep grep found function encodeHTMLEntities
> in supplier.tt. HTML escaping really needs to be unified, making bug 36385
> all the more important.

+1

Also, part of the reason we have XSS issues like these is because we work with the DOM using strings rather than objects. 

Take this particular XSS:

const el = `<li><a href="/cgi-bin/koha/circ/circulation.pl?borrowernumber=${p["borrowernumber"]}">${p["name"]} (${p["card"]})</a></li>`;

Here we could be using a "Safe Sink"[1] for the name and card to eliminate the XSS.

const el = document.createElement('li');
const a_el = document.createElement('a');
a_el.setAttribute('href',`/cgi-bin/koha/circ/circulation.pl?borrowernumber=${p["borrowernumber"]}`);
const link_text = document.createTextNode(`${p["name"]} (${p["card"]})`);
a_el.appendChild(link_text);
el.appendChild(a_el);


[1]
https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html#safe-sinks

--

Yes, it is more verbose, but it's way safer. It also doesn't need an escape_str. (Not that I think escape_str is bad. The more layers of protection the better I always say.)

(In reply to David Cook from comment #9)
> Here we could be using a "Safe Sink"[1] for the name and card to eliminate
> the XSS.

Bug 35960 has a clever way of using jquery and strings to create objects without any user input, and then using a safe sink for the user input. So there could be a balance in terms of verbosity as well.

I can't recreate the bug on 22.05 - I also can't apply the patch cleanly

(In reply to wainuiwitikapark from comment #12)
> I can't recreate the bug on 22.05 - I also can't apply the patch cleanly

Same on 22.11: previous_patrons doesn't exist on this branch

(In reply to wainuiwitikapark from comment #12)
> I can't recreate the bug on 22.05 - I also can't apply the patch cleanly

I believe the issue was introduced in 23.11.00, so that is ok!

(In reply to Kyle M Hall from comment #14)
> (In reply to wainuiwitikapark from comment #12)
> > I can't recreate the bug on 22.05 - I also can't apply the patch cleanly
> 
> I believe the issue was introduced in 23.11.00, so that is ok!

Ok cool thanks Kyle

So 22.05, 22.11 and 23.05 can ignore backporting this?

(In reply to wainuiwitikapark from comment #15)
> (In reply to Kyle M Hall from comment #14)
> > (In reply to wainuiwitikapark from comment #12)
> > > I can't recreate the bug on 22.05 - I also can't apply the patch cleanly
> > 
> > I believe the issue was introduced in 23.11.00, so that is ok!
> 
> Ok cool thanks Kyle
> 
> So 22.05, 22.11 and 23.05 can ignore backporting this?

That is correct!

Created attachment 166717 [details] [review]
Bug 36382: XSS in showLastPatron dropdown

1) Set borrower surname to:
    <script>alert("here comes trouble");</script>
2) Save, nothing happens
3) Enable showLastPatron
4) Reload patron
5) Note the alert popup
6) Apply this patch
7) Reload patron
8) No alert!

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 166718 [details] [review]
Bug 36382: (QA follow-up) Don't escape quotes in escapeHtml

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Rebased after a few hundred commits pushed to main.

Pushed for 24.05!

Well done everyone, thank you!

Bug 21246 since 23.11 so no backport to older branches