Like bug 40522, it's possible to execute stored XSS in a DataTable when you press the "Print" button in a datatable.
Created attachment 184697 [details] [review] Bug 40524: Update print format to use built-in formatter This change updates the print format to use the built-in formatter for operations like trim, strip HTML, strip newlines, and not decoding entities. Test plan: 0. Apply the patch 1. Go to http://localhost:8081/cgi-bin/koha/acqui/basket.pl?basketno=1 2. Click "add to basket" 3. Type "test" next to "From an existing record" and click "Submit" 4. On the "Gairm" row, click "Add order" 5. Fill in the required fields 6. In the "Internal note" put the following: abc123 7. Click "Save" 8. In the Orders table, click "Export" and then "Print" 9. Note that the print page loads correctly, you can see the internal note, and you cannot see lines for "Add internal note", "Edit internal note", or "Add vendor note"
Created attachment 187597 [details] [review] Bug 40524: Update print format to use built-in formatter This change updates the print format to use the built-in formatter for operations like trim, strip HTML, strip newlines, and not decoding entities. Test plan: 0. Apply the patch 1. Go to http://localhost:8081/cgi-bin/koha/acqui/basket.pl?basketno=1 2. Click "add to basket" 3. Type "test" next to "From an existing record" and click "Submit" 4. On the "Gairm" row, click "Add order" 5. Fill in the required fields 6. In the "Internal note" put the following: abc123 7. Click "Save" 8. In the Orders table, click "Export" and then "Print" 9. Note that the print page loads correctly, you can see the internal note, and you cannot see lines for "Add internal note", "Edit internal note", or "Add vendor note" Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
What is the impact on the print results here? Since you remove html..
Needs feedback. Changing status
(In reply to Marcel de Rooy from comment #5) > What is the impact on the print results here? > Since you remove html.. (In reply to Marcel de Rooy from comment #6) > Needs feedback. Changing status Yeah, good question. I should've written the test plan better. Instead of adding "abc123" to the "Internal note" try something like this: <p>This is HTML text.</p>This is plain text. When you go to print, you'll see the following: Internal note: This is HTML text. This is plain text. So rather than scrubbing the whole element, you are still getting your inner text content. Hopefully that answers the question. I'll reset the status for this one and bug 40522. Let me know if you need any more clarification.
(In reply to David Cook from comment #7) > So rather than scrubbing the whole element, you are still getting your inner > text content. > > Hopefully that answers the question. I'll reset the status for this one and > bug 40522. Let me know if you need any more clarification. Couldnt we just scrub the bad/untrusted elements/content instead of removing all html and leaving the associated styles? This feels like a completely different user experience with just plain text.
After having some issues around backporting for bug 40525, I want to refactor this to avoid headaches for RMaints (and me).
(In reply to Marcel de Rooy from comment #8) > Couldnt we just scrub the bad/untrusted elements/content instead of removing > all html and leaving the associated styles? This feels like a completely > different user experience with just plain text. I'm not sure I 100% understand what you're saying, but I think I understand. When it comes to scrubbing, it's significantly more difficult to have a deny list than an allow list. It's not really feasible to just scrub bad/untrusted elements/content. But I take your point about the UX because it might be that there are some HTML/style that Koha admins/users expect to come through. Typically in Koha we use HTML::Scrubber via C4::Scrubber which is Perl-based and it's great. But then we run into problems with DataTables when content isn't Perl-generated.
Regarding UX, I just took a look on master, and we use a "html" filter on "Internal note", so on /cgi-bin/koha/acqui/basket.pl?basketno=1 you'll see this: Internal note: <b>Internal note2</b> When you go to print, then you see the rendered HTML so it's the following where Internal note2 is bolded: Internal note: Internal note2 -- So really the UX would be able the same. The only difference being that the HTML markup is stripped rather than escaped. I think that DataTable.Buttons.stripData() is used by default in DataTables. We have to invoke it manually in this patch only because we provide our own formatter (to remove elements with .no_export class for instance). So I'm not really doing anything too crazy here. I'm just re-adding some built-in functionality.
Note also that this is the staff side equivalent of bug 40522 that already has a CVE taken out on it.
(In reply to David Cook from comment #10) > When it comes to scrubbing, it's significantly more difficult to have a deny > list than an allow list. It's not really feasible to just scrub > bad/untrusted elements/content. But I take your point about the UX because > it might be that there are some HTML/style that Koha admins/users expect to > come through. > > Typically in Koha we use HTML::Scrubber via C4::Scrubber which is Perl-based > and it's great. But then we run into problems with DataTables when content > isn't Perl-generated. Something like https://www.npmjs.com/package/sanitize-html ? Maybe the standard allowedTags are sufficient. Or https://github.com/cure53/DOMPurify
(In reply to Marcel de Rooy from comment #13) > (In reply to David Cook from comment #10) > > When it comes to scrubbing, it's significantly more difficult to have a deny > > list than an allow list. It's not really feasible to just scrub > > bad/untrusted elements/content. But I take your point about the UX because > > it might be that there are some HTML/style that Koha admins/users expect to > > come through. > > > > Typically in Koha we use HTML::Scrubber via C4::Scrubber which is Perl-based > > and it's great. But then we run into problems with DataTables when content > > isn't Perl-generated. > > Something like https://www.npmjs.com/package/sanitize-html ? > Maybe the standard allowedTags are sufficient. > Or https://github.com/cure53/DOMPurify I can't speak to https://www.npmjs.com/package/sanitize-html, but OWASP does recommend DOMPurify, and it is something I have suggested in the past in the Koha community, but no one else seemed interested. Working on security in Koha is often like shouting into the void.
(In reply to Marcel de Rooy from comment #8) > Couldnt we just scrub the bad/untrusted elements/content instead of removing > all html and leaving the associated styles? This feels like a completely > different user experience with just plain text. Regarding user experience, could you perhaps provide some screenshots or some concrete examples of how it feels like a different user experience? Because so far I think it's looked identical to me before/after the patch?
(In reply to Marcel de Rooy from comment #8) > (In reply to David Cook from comment #7) > > So rather than scrubbing the whole element, you are still getting your inner > > text content. > > > > Hopefully that answers the question. I'll reset the status for this one and > > bug 40522. Let me know if you need any more clarification. > > Couldnt we just scrub the bad/untrusted elements/content instead of removing > all html and leaving the associated styles? This feels like a completely > different user experience with just plain text. Thinking back on the amount of time and energy I've invested into this code... I'm happy to review other people's alternatives, but this is the only code I am prepared to write for this issue. I'd rather work on other issues than re-write this one.
(In reply to David Cook from comment #15) > (In reply to Marcel de Rooy from comment #8) > > Couldnt we just scrub the bad/untrusted elements/content instead of removing > > all html and leaving the associated styles? This feels like a completely > > different user experience with just plain text. > > Regarding user experience, could you perhaps provide some screenshots or > some concrete examples of how it feels like a different user experience? > Because so far I think it's looked identical to me before/after the patch? I dont have a concrete example. Just thinking about possible reactions after pushing this. We are going back to plain text. So it is obvious that we lose styles/formatting. Maybe we should now balance security versus presentation. And security always wins. (In reply to David Cook from comment #16) > Thinking back on the amount of time and energy I've invested into this > code... I'm happy to review other people's alternatives, but this is the > only code I am prepared to write for this issue. I'd rather work on other > issues than re-write this one. This is by no means meant to stop this development btw.
(In reply to David Cook from comment #14) > I can't speak to https://www.npmjs.com/package/sanitize-html, but OWASP does > recommend DOMPurify, and it is something I have suggested in the past in the > Koha community, but no one else seemed interested. > > Working on security in Koha is often like shouting into the void. Unfortunately these seem more general issues than just security.
(In reply to Marcel de Rooy from comment #17) > I dont have a concrete example. Just thinking about possible reactions after > pushing this. We are going back to plain text. So it is obvious that we lose > styles/formatting. > Maybe we should now balance security versus presentation. And security > always wins. I appreciate that you're thinking of all the different angles. I think that's a key factor in being good at QA :). Perhaps I have tunnel vision on this one. That said, in practice, I don't think that we are losing styles/formatting, but I admit that I haven't done a comprehensive review. In the basket print output, I can't see any loss of style/formatting before/after the patch. But... maybe there is a different DataTable that I haven't seen yet where that might happen. /cgi-bin/koha/admin/branches.pl seems OK /cgi-bin/koha/admin/categories.pl seems OK /cgi-bin/koha/admin/itemtypes.pl seems OK These are fairly basic tables though. Trying to think of a table with a lot of content that might be stylized... but really I can't think of when that would happen, because all the datatables should have fairly atomic data in their cells. All the DataTables are already using plain text in the print output (except for the case where I've managed to sneak in HTML to perform the XSS exploit). I guess I'm struggling with the idea that we're losing styles/formatting when there's no practical evidence to suggest that is true? > (In reply to David Cook from comment #16) > This is by no means meant to stop this development btw. Thanks< Marcel. I appreciate that. Apologies for my grumpiness.
Created attachment 188757 [details] [review] Bug 40524: Update print format to use built-in formatter This change updates the print format to use the built-in formatter for operations like trim, strip HTML, strip newlines, and not decoding entities. Test plan: 0. Apply the patch 1. Go to http://localhost:8081/cgi-bin/koha/acqui/basket.pl?basketno=1 2. Click "add to basket" 3. Type "test" next to "From an existing record" and click "Submit" 4. On the "Gairm" row, click "Add order" 5. Fill in the required fields 6. In the "Internal note" put the following: abc123 7. Click "Save" 8. In the Orders table, click "Export" and then "Print" 9. Note that the print page loads correctly, you can see the internal note, and you cannot see lines for "Add internal note", "Edit internal note", or "Add vendor note" Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pragmatic choice. In view of provided arguments no practical basis to hold this back for presentational reasons where we should favor security. Unclear enough :)
(In reply to Marcel de Rooy from comment #20) > Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> @Brendan: Would you care to test/signoff the 40522 counterpart ?
Created attachment 190054 [details] [review] Bug 40524: [24.11.x] Update print format to use built-in formatter This change updates the print format to use the built-in formatter for operations like trim, strip HTML, strip newlines, and not decoding entities. Test plan: 0. Apply the patch 1. Go to http://localhost:8081/cgi-bin/koha/acqui/basket.pl?basketno=1 2. Click "add to basket" 3. Type "test" next to "From an existing record" and click "Submit" 4. On the "Gairm" row, click "Add order" 5. Fill in the required fields 6. In the "Internal note" put the following: abc123 7. Click "Save" 8. In the Orders table, click "Export" and then "Print" 9. Note that the print page loads correctly, you can see the internal note, and you cannot see lines for "Add internal note", "Edit internal note", or "Add vendor note" Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 190055 [details] [review] Bug 40524: [24.05.x] Update print format to use built-in formatter This change updates the print format to use the built-in formatter for operations like trim, strip HTML, strip newlines, and not decoding entities. Test plan: 0. Apply the patch 1. Go to http://localhost:8081/cgi-bin/koha/acqui/basket.pl?basketno=1 2. Click "add to basket" 3. Type "test" next to "From an existing record" and click "Submit" 4. On the "Gairm" row, click "Add order" 5. Fill in the required fields 6. In the "Internal note" put the following: abc123 7. Click "Save" 8. In the Orders table, click "Export" and then "Print" 9. Note that the print page loads correctly, you can see the internal note, and you cannot see lines for "Add internal note", "Edit internal note", or "Add vendor note" Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 190056 [details] [review] Bug 40524: [22.11.x] Update print format to use built-in formatter This change updates the print format to use the built-in formatter for operations like trim, strip HTML, strip newlines, and not decoding entities. Test plan: 0. Apply the patch 1. Go to http://localhost:8081/cgi-bin/koha/acqui/basket.pl?basketno=1 2. Click "add to basket" 3. Type "test" next to "From an existing record" and click "Submit" 4. On the "Gairm" row, click "Add order" 5. Fill in the required fields 6. In the "Internal note" put the following: abc123 7. Click "Save" 8. In the Orders table, click "Export" and then "Print" 9. Note that the print page loads correctly, you can see the internal note, and you cannot see lines for "Add internal note", "Edit internal note", or "Add vendor note" Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 190061 [details] [review] Bug 40524: [24.05.x] Update print format to use built-in formatter This change updates the print format to use the built-in formatter for operations like trim, strip HTML, strip newlines, and not decoding entities. Test plan: 0. Apply the patch 1. Go to http://localhost:8081/cgi-bin/koha/acqui/basket.pl?basketno=1 2. Click "add to basket" 3. Type "test" next to "From an existing record" and click "Submit" 4. On the "Gairm" row, click "Add order" 5. Fill in the required fields 6. In the "Internal note" put the following: abc123 7. Click "Save" 8. In the Orders table, click "Export" and then "Print" 9. Note that the print page loads correctly, you can see the internal note, and you cannot see lines for "Add internal note", "Edit internal note", or "Add vendor note" Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Thank you lucas. This will be in 24.05.16
Pushed to main for 25.11.00