Created attachment 177159 [details] [review] Bug 38969: Prevent XSS in tags/review.pl Created attachment 177160 [details] [review] Bug 38969: Prevent JS error when adding tags with quote at the OPAC Uncaught SyntaxError: missing ) after argument list There are way more to fix in this controller... (In reply to Jonathan Druart from comment #2) > Created attachment 177160 [details] [review] [review] > Bug 38969: Prevent JS error when adding tags with quote at the OPAC > > Uncaught SyntaxError: missing ) after argument list > > There are way more to fix in this controller... Would it make sense to tackle that in a separate bug? If it only fixes errors it might be better to focus on the XSS in this bug? I tested thus: - Go to Admin > Tools > Tags - Paste "<script> alert("XSS FOUND!") </script>" into the input field under the "Check lists" heading - Click on "Test" - Get a popup saying "XSS FOUND!" - Apply the patch - restart all - Paste and click again - Get the same popup (In reply to Magnus Enger from comment #3) > (In reply to Jonathan Druart from comment #2) > > Created attachment 177160 [details] [review] [review] [review] > > Bug 38969: Prevent JS error when adding tags with quote at the OPAC > > > > Uncaught SyntaxError: missing ) after argument list > > > > There are way more to fix in this controller... > > Would it make sense to tackle that in a separate bug? If it only fixes > errors it might be better to focus on the XSS in this bug? This is to help testing, or you cannot enter the(In reply to Magnus Enger from comment #4) > I tested thus: > > - Go to Admin > Tools > Tags > - Paste "<script> alert("XSS FOUND!") </script>" into the input field under > the "Check lists" heading > - Click on "Test" > - Get a popup saying "XSS FOUND!" > - Apply the patch > - restart all > - Paste and click again > - Get the same popup Works for me: https://snipboard.io/rTwMs3.jpg (In reply to Magnus Enger from comment #4) > I tested thus: > > - Go to Admin > Tools > Tags > - Paste "<script> alert("XSS FOUND!") </script>" into the input field under > the "Check lists" heading > - Click on "Test" > - Get a popup saying "XSS FOUND!" > - Apply the patch > - restart all > - Paste and click again > - Get the same popup Works for me: https://snipboard.io/rTwMs3.jpg (In reply to Jonathan Druart from comment #6) > Works for me: https://snipboard.io/rTwMs3.jpg Sounds like I missed something then. Setting back to NSO. Created attachment 179553 [details] [review] Bug 38969: Prevent XSS in tags/review.pl Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Created attachment 179554 [details] [review] Bug 38969: Prevent JS error when adding tags with quote at the OPAC Uncaught SyntaxError: missing ) after argument list There are way more to fix in this controller... Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Note: had to use auto-rebase before signing off, idk if it matters Planning to come back to this in the coming weeks, but happy for someone else to QA if they can. I keep looking at these patches and thinking I don't love the change, but then tell myself I'll come back to it later... (In reply to David Cook from comment #12) > I keep looking at these patches and thinking I don't love the change, but > then tell myself I'll come back to it later... Please explain why you "don't love the change", otherwise it will get stuck. (In reply to Jonathan Druart from comment #13) > (In reply to David Cook from comment #12) > > I keep looking at these patches and thinking I don't love the change, but > > then tell myself I'll come back to it later... > > Please explain why you "don't love the change", otherwise it will get stuck. I suppose what I mean is that it doesn't look clean. I haven't tested it myself yet, but I imagine it works. In any case, a cleaner solution would require refactoring the code, which would take more time and effort. After all, we're patching very old code here. I was just about to take a look at this, but it looks like main might be broken at the moment... Got my KTD working again... Do we have a test plan for this? At the moment, the only test plan I see is this: Test plan: - Go to Admin > Tools > Tags - Paste "<script> alert("XSS FOUND!") </script>" into the input field under the "Check lists" heading - Click on "Test" That's not really XSS, since you're doing it to yourself. It doesn't look like that field uses stored data (so not a stored XSS) and you can't get the data into that field at all nor activate it via a GET, so it's not a reflected XSS either. Still... obviously something we want to avoid. Not sure we need the OPAC fix here, and without a test plan, I'm not sure how to test it for sure. Created attachment 179676 [details] [review] Bug 38969: [alternate] Prevent self-XSS when testing tags in staff interface This change refactors the tag test in the staff interface to use more standard Javascript/JSON for passing messages and writing HTML. Test plan: - Apply the patch and koha-plack --restart kohadev - Go to Admin > Tools > Tags - Paste "<script> alert("不好! XSS FOUND!") </script>" into the input field under the "Check lists" heading - Click on "Test" - Note that the following test appears below the Test button: <script> alert("不好! XSS FOUND!") </script> is neither permitted nor prohibited! I like my alternate patch better since it gets rid of the unnecessary URI encoding and instead treats the tag as a JSON string, which it is, and uses the OWASP recommended safe sink rather than our custom escaping function. But I'm happy to sign off on "Prevent XSS in tags/review.pl" since it does work. Just holding off the SO/QA because of "Bug 38969: Prevent JS error when adding tags with quote at the OPAC". If you can give me a test plan and steps to reproduce the problem, then I'll happily test and QA. (In reply to David Cook from comment #18) > I like my alternate patch better since it gets rid of the unnecessary URI > encoding and instead treats the tag as a JSON string, which it is, and uses > the OWASP recommended safe sink rather than our custom escaping function. > > But I'm happy to sign off on "Prevent XSS in tags/review.pl" since it does > work. > > Just holding off the SO/QA because of "Bug 38969: Prevent JS error when > adding tags with quote at the OPAC". If you can give me a test plan and > steps to reproduce the problem, then I'll happily test and QA. Enter this (with the surrounded quotes): "<script> alert("XSS FOUND!") </script>" (In reply to Jonathan Druart from comment #20) > Enter this (with the surrounded quotes): "<script> alert("XSS FOUND!") > </script>" Ah thanks for that. I can see the console error on the OPAC now with that payload. The error message looks a bit weird, but like you say... there's a lot of things that need fixing. Created attachment 179708 [details] [review] Bug 38969: Prevent XSS in tags/review.pl Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 179709 [details] [review] Bug 38969: Prevent JS error when adding tags with quote at the OPAC Uncaught SyntaxError: missing ) after argument list There are way more to fix in this controller... Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 179710 [details] [review] Bug 38969: (QA follow-up) fix tidy Created attachment 181329 [details] [review] [24.05] Bug 38969 Created attachment 181330 [details] [review] Bug 38969: Prevent XSS in tags/review.pl and Prevent JS error when adding tags with quote at the OPAC Created attachment 181331 [details] [review] [24.05] Bug 38969: Prevent XSS in tags/review.pl and Prevent JS error when adding tags with quote at the OPAC Created attachment 181332 [details] [review] [24.05] Bug 38969: Prevent XSS in tags/review.pl and Prevent JS error when adding tags with quote at the OPAC Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: wainuiwitikapark <wainuiwitikapark@catalyst.net.nz> Applied to 24.05.x-security for 24.05.09 ======================================== I applied these patches to 24.05.x-security and they had a few minor conflict issues which were easy for me to fix (syntax issues and alignment etc) I squashed my commits and attached them to this bug report in case anyone wants to use them... P.S. sorry about all the attachments and obsoletes Applied to 23.11.x-security for 23.11.14 Code is a bit different because param is "test" instead of "tag" Created attachment 181464 [details] [review] [22.11] Bug 38969: Prevent XSS in tags/review.pl and Prevent JS error when adding tags with quote at the OPAC Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: wainuiwitikapark <wainuiwitikapark@catalyst.net.nz> Applied to 24.11.x-security for 24.11.04 Applied to 22.11.x-security for 22.11.26 Pushed for 25.05! Well done everyone, thank you! |
Created attachment 177148 [details] Screenshot of XSS CWE-79: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') CVSS: 5.8 (Medium) Open Koha and navigate to /cgi-bin/koha/tags/review.pl Insert Payload: <script> alert("XSS FOUND!") </script> XSS Triggered when tag is submitted.