Summary: | "Public note" field in course reserve should restrict HTML usage | ||
---|---|---|---|
Product: | Koha | Reporter: | Jimmy Bell <bug-report> |
Component: | Course reserves | Assignee: | David Cook <dcook> |
Status: | RESOLVED FIXED | QA Contact: | Marcel de Rooy <m.de.rooy> |
Severity: | minor | ||
Priority: | P5 - low | CC: | 1joynelson, alexbuckley, blawlor, bug-report, dcook, didier.gautheron, fridolin.somers, jonathan.druart, julian.maurice, laurent.ducos, lucas, m.de.rooy, martin.renvoize, nick, wainuiwitikapark |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33337 | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | Trivial patch | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: |
25.05.01,25.05.00,24.11.03,24.05.08,23.11.13,22.11.25
|
Circulation function: | |
Bug Depends on: | 38488 | ||
Bug Blocks: | |||
Attachments: |
4 images, shows where payload in entered and the resulting popup when opening the course reserve or listing it
Bug 31165: Scrub "Public note" field for Course Reserves Bug 31165: Scrub "Public note" field for Course Reserves Bug 31165: Scrub "Public note" field for Course Reserves Bug 31165: Scrub "Public note" field for Course Reserves [24.05.x] Bug 31165: Scrub "Public note" field for Course Reserves |
This is the expected behaviour: commit d8187991a40f58ab815fd1e75d6a3671ca72a668 Bug 23683: Course reserves public notes on specific items should allow for HTML However we could use C4::Scrubber to reduce the HTML nodes allowed, but it's not allowing "a". From a security perspective, it would indeed be ideal to disallow the script node and its variations, like ScriPT or any lowercase/uppercase combination. (In reply to Jonathan Druart from comment #2) > However we could use C4::Scrubber to reduce the HTML nodes allowed, but it's > not allowing "a". That's an interesting idea... (In reply to Jimmy Bell from comment #3) > From a security perspective, it would indeed be ideal to disallow the script > node and its variations, like ScriPT or any lowercase/uppercase combination. Maybe although there's a million other ways of doing XSS beyond just the "script" tag. A very common other one is the "img" tag, but really you can use just about anything. It's a bit of a losing battle when you try to exclude some HTML instead of all HTML. That said, I suppose you could argue it's similar to locking the door to your house or car. It's not going to stop a determined thief, who might just break a window to gain entry, but it'll still deter the majority of thieves. Since the issue exists in the staff interface with trusted users, I don't know that I'd class this as a vulnerability (like I would for an unauthenticated public user), but scrubbing some HTML couldn't hurt. One could argue that while "OPACUserJS" gives an admin the ability to write arbitrary Javascript, perhaps someone with Course Reserve access isn't trusted to that same level as the admin, and so we need to add some further restrictions. Although if we start to pull that thread... it could get very complicated. Anyway, interesting ideas! The way I see it is that the fact that the staff interface allows only for trusted users does not make it less of a vulnerability, it just decreases the likelihood that it could be exploited. On the Koha instance I tested this on, the password policy for admin and staff on the intranet was very weak, something like a minimum of 3 characters. That would be easy to bruteforce if the username is known, and often in library settings, credentials are put on a post-it near the staff computer. I agree that all this is a complicated scenario and that this XSS is not very likely to be exploited. Deciding to patch it or not and to what extent is up to you, but in my opinion it still qualifies as a vulnerability. Session cookies are unlikely to be stolen because of their httponly and secure flags, however through XSS, users could be redirected to a malicious website and a malicious employee could trick them in giving away any kind of passwords. Whether or not you choose to patch this, having a CVE with a high privileges required vector is still advised since it can provide the community with the knowledge that it exists. Thanks :) (In reply to Jimmy Bell from comment #5) > The way I see it is that the fact that the staff interface allows only for > trusted users does not make it less of a vulnerability, it just decreases > the likelihood that it could be exploited. Academically maybe but not in real world terms. (E.g. the only secure computer is one with no network connection and no users.) Would you say that Drupal and Wordpress are vulnerable because they allow trusted users to add HTML, CSS, and Javascript? CMSes are designed to allow trusted users to add this code, and Koha has many CMS functions (e.g. "News", "HTML Customizations", etc.). In fact, I don't think you could even class this as a XSS vulnerability, because XSS implies that the attacker is external to the site and they've injected the script via a URL or stored user input (like a text comment). In your scenario, the attacker already controls the system. It's just a compromised website. > On the Koha instance I tested this on, the password policy for admin and > staff on the intranet was very weak, something like a minimum of 3 > characters. That would be easy to bruteforce if the username is known, and > often in library settings, credentials are put on a post-it near the staff > computer. Out of the box, Koha does make some unfortunate choices regarding password policy and account security. The system preference "FailedLoginAttempts" should be set out of the box, and the default password policy should be much higher and more complex. There are existing tickets for password length but they always seems to stall. And yeah people have some pretty poor password management behaviours. Of course, even when people have strong password management and 2FA, a determined attacker can still break into systems. The latest hack of Microsoft apparently involved someone triggering the 2FA challenge so many times that the personal responsible just hit "Approve" to stop the alerts from coming to their authenticator app. Humans are often the weakest link. > I agree that all this is a complicated scenario and that this XSS is not > very likely to be exploited. Deciding to patch it or not and to what extent > is up to you, but in my opinion it still qualifies as a vulnerability. > Session cookies are unlikely to be stolen because of their httponly and > secure flags, however through XSS, users could be redirected to a malicious > website and a malicious employee could trick them in giving away any kind of > passwords. Well, it's not up to me ;). You're free to submit a patch and leave it up to people to test and QA. I'm not a gatekeeper. > Whether or not you choose to patch this, having a CVE with a high privileges > required vector is still advised since it can provide the community with the > knowledge that it exists. Feel free to create a CVE. As I said above, many features in Koha rely on authenticated authorized staff users inserting HTML, Javascript, and CSS. You'd probably want to include all those features in your CVE. That all said, perhaps Koha should have a file-based configuration option on the server that controls whether or not web users can enter HTML, CSS, and Javascript which is disables those features out of the box. Feel free to send a patch with the code to achieve that. I think I'm done commenting on this one. Jonathan's scrubbing idea is interesting. It would be worthwhile checking with the folk from Bug 23683 to see what HTML elements they need, and then only allowing those through. It's not a bad idea. But this isn't a stored XSS vulnerability, so the bug report title should be changed to something like: '"Public note" field in Course Reserve should restrict HTML usage' A stored XSS vulnerability would be if an OPAC user could add a comment with Javascript to the Course Reserve. The vulnerability in the scenario about someone taking over a staff account to inject Javascript into a Course Reserve note would be with the authentication process - not the CMS feature. I see your point. I'm by no means a Koha expert, this was found during an authorized pentesting exercise where I work. If this constitutes a feature, I don't believe a CVE would be appropriate anymore. Restricting the HTML is indeed a great idea if someone ever wants to look into that. Thank you all for taking the time, hope this can help in some way or another. I will now edit the title of the Bug as David suggested. My stance on this one has softened or perhaps hardened over the past few years, and I'm more inclined to scrub user-entered HTML wherever possible, even when entered by trusted staff members. I'm going to provide a patch using a new technique. We might find we don't want to use that technique, and instead use an older technique, but going to move forward on this one anyway... Created attachment 174816 [details] [review] Bug 31165: Scrub "Public note" field for Course Reserves When displaying the "Public note" for Course Reserves, we scrub the HTML using the "note" scrubber type. Test plan: 0. Apply the patch 1. Go to http://localhost:8081/cgi-bin/koha/course_reserves/course-reserves.pl 2. Click "New course" 3. Fill in required fields 4. In staff note and public note add the following: <script>alert('boo!')</script><p>Hello!</p> 5. Click Save 6. Click "Add reserves" 7. Add "39999000001310" to Item barcode 8. Click "Submit" 9. Fill in all required fields 10. Add the following for Staff note and Public note: <script>alert('boo2!')</script><p>Item!</p> 11. Click "Save" 12. Go back to your course reserve 13. Note the HTML is scrubbed on "Public note" 14. Go to http://localhost:8081/cgi-bin/koha/course_reserves/course-reserves.pl 15. Note the HTML is scrubbed on "Public note" 16. Go to http://localhost:8080/cgi-bin/koha/opac-course-reserves.pl 17. Note the HTML is scrubbed on "Notes" 18. Click on your course reserves 19. Note the HTML is scrubbed on "Notes" I think someone could make the argument that we should be handling the scrubbing in the .pl/.pm code, but it can involve double-handling data and you have to make sure you're just scrubbing at display time and not edit time (unless you don't mind silently erasing data). Since we use the $raw filter in the templates, I think using bug 38488 as the basis for this actually makes a lot of sense. Happy to chat about this more with QA though... Created attachment 176386 [details] [review] Bug 31165: Scrub "Public note" field for Course Reserves When displaying the "Public note" for Course Reserves, we scrub the HTML using the "note" scrubber type. Test plan: 0. Apply the patch 1. Go to http://localhost:8081/cgi-bin/koha/course_reserves/course-reserves.pl 2. Click "New course" 3. Fill in required fields 4. In staff note and public note add the following: <script>alert('boo!')</script><p>Hello!</p> 5. Click Save 6. Click "Add reserves" 7. Add "39999000001310" to Item barcode 8. Click "Submit" 9. Fill in all required fields 10. Add the following for Staff note and Public note: <script>alert('boo2!')</script><p>Item!</p> 11. Click "Save" 12. Go back to your course reserve 13. Note the HTML is scrubbed on "Public note" 14. Go to http://localhost:8081/cgi-bin/koha/course_reserves/course-reserves.pl 15. Note the HTML is scrubbed on "Public note" 16. Go to http://localhost:8080/cgi-bin/koha/opac-course-reserves.pl 17. Note the HTML is scrubbed on "Notes" 18. Click on your course reserves 19. Note the HTML is scrubbed on "Notes" Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Thanks for the excellent test plan. This was a cool example to see HtmlScrubber in action! (In reply to Brendan Lawlor from comment #13) > Thanks for the excellent test plan. > > This was a cool example to see HtmlScrubber in action! Thanks, Brendan! I'd just add a note that the dependency bug 38488 needs a sign off too! Thanks! (In reply to Jonathan Druart from comment #2) > However we could use C4::Scrubber to reduce the HTML nodes allowed, but it's > not allowing "a". If I read the patches correctly, nothing is done to allow <a>. Won't it be a problem ? I imagine it can be useful in a public note. Have some fun with auto_rebase ;) Sorry, but we're all in the same boat now.. Created attachment 178125 [details] [review] Bug 31165: Scrub "Public note" field for Course Reserves When displaying the "Public note" for Course Reserves, we scrub the HTML using the "note" scrubber type. Test plan: 0. Apply the patch 1. Go to http://localhost:8081/cgi-bin/koha/course_reserves/course-reserves.pl 2. Click "New course" 3. Fill in required fields 4. In staff note and public note add the following: <script>alert('boo!')</script><p>Hello!</p> 5. Click Save 6. Click "Add reserves" 7. Add "39999000001310" to Item barcode 8. Click "Submit" 9. Fill in all required fields 10. Add the following for Staff note and Public note: <script>alert('boo2!')</script><p>Item!</p> 11. Click "Save" 12. Go back to your course reserve 13. Note the HTML is scrubbed on "Public note" 14. Go to http://localhost:8081/cgi-bin/koha/course_reserves/course-reserves.pl 15. Note the HTML is scrubbed on "Public note" 16. Go to http://localhost:8080/cgi-bin/koha/opac-course-reserves.pl 17. Note the HTML is scrubbed on "Notes" 18. Click on your course reserves 19. Note the HTML is scrubbed on "Notes" Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> (In reply to Marcel de Rooy from comment #16) > Have some fun with auto_rebase ;) Sorry, but we're all in the same boat now.. $ perl auto_rebase.pl Creating a temporary branch 'bug_31165-tmp'... Rebasing the temporary branch up to before the tidy commits... Creating a new branch 'bug_31165-rebased' starting at the end of the tidy commits... Getting commit list from branch 'bug_31165-tmp'... No number of commits passed, trying to guess it... Found 1 commits to process Processing 1 commits... Processing commit 62ca8ede53a... (1/1) Processing file koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course-details.tt... (1/4) Tidying file 1/1 (koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course-details.tt) Processing file koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course-reserves.tt... (2/4) Tidying file 1/1 (koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course-reserves.tt) Processing file koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-course-details.tt... (3/4) Tidying file 1/1 (koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-course-details.tt) Processing file koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-course-reserves.tt... (4/4) Tidying file 1/1 (koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-course-reserves.tt) Committing changes... All commits from the source branch have been applied to 'bug_31165-rebased'! Rebasing on top of main... Everything applied successfully! (In reply to Julian Maurice from comment #15) > (In reply to Jonathan Druart from comment #2) > > However we could use C4::Scrubber to reduce the HTML nodes allowed, but it's > > not allowing "a". > If I read the patches correctly, nothing is done to allow <a>. Won't it be a > problem ? I imagine it can be useful in a public note. Honestly, none of my libraries use Course Reserves, so I don't have production data to work with here. Happy for people to add more scrubber profiles. Or take a look at bug 31934 for a more customizable approach. [% course.public_note | scrub_html type => 'note' | $raw %] We dont need $raw here anymore. But no problem. Created attachment 178443 [details] [review] Bug 31165: Scrub "Public note" field for Course Reserves When displaying the "Public note" for Course Reserves, we scrub the HTML using the "note" scrubber type. Test plan: 0. Apply the patch 1. Go to http://localhost:8081/cgi-bin/koha/course_reserves/course-reserves.pl 2. Click "New course" 3. Fill in required fields 4. In staff note and public note add the following: <script>alert('boo!')</script><p>Hello!</p> 5. Click Save 6. Click "Add reserves" 7. Add "39999000001310" to Item barcode 8. Click "Submit" 9. Fill in all required fields 10. Add the following for Staff note and Public note: <script>alert('boo2!')</script><p>Item!</p> 11. Click "Save" 12. Go back to your course reserve 13. Note the HTML is scrubbed on "Public note" 14. Go to http://localhost:8081/cgi-bin/koha/course_reserves/course-reserves.pl 15. Note the HTML is scrubbed on "Public note" 16. Go to http://localhost:8080/cgi-bin/koha/opac-course-reserves.pl 17. Note the HTML is scrubbed on "Notes" 18. Click on your course reserves 19. Note the HTML is scrubbed on "Notes" Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Hi, This conflicts when applied on 24.05.x-security. --- kohadev-koha@kohadevbox:koha((v24.05.07))$ git bz apply 31165 Bug 31165 - "Public note" field in course reserve should restrict HTML usage 178443 - Bug 31165: Scrub "Public note" field for Course Reserves Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 31165: Scrub "Public note" field for Course Reserves Using index info to reconstruct a base tree... M koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course-details.tt M koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course-reserves.tt M koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-course-details.tt M koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-course-reserves.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-course-reserves.tt CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-course-reserves.tt Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-course-details.tt CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-course-details.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course-reserves.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course-reserves.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course-details.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course-details.tt error: Failed to merge in the changes. Patch failed at 0001 Bug 31165: Scrub "Public note" field for Course Reserves hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-31165-Scrub-Public-note-field-for-Course-Reser-ipqwckct.patch kohadev-koha@kohadevbox:koha((v24.05.07)|AM 1/1)$ --- Could we please have a rebase? Created attachment 179635 [details] [review] [24.05.x] Bug 31165: Scrub "Public note" field for Course Reserves When displaying the "Public note" for Course Reserves, we scrub the HTML using the "note" scrubber type. Test plan: 0. Apply the patch 1. Go to http://localhost:8081/cgi-bin/koha/course_reserves/course-reserves.pl 2. Click "New course" 3. Fill in required fields 4. In staff note and public note add the following: <script>alert('boo!')</script><p>Hello!</p> 5. Click Save 6. Click "Add reserves" 7. Add "39999000001310" to Item barcode 8. Click "Submit" 9. Fill in all required fields 10. Add the following for Staff note and Public note: <script>alert('boo2!')</script><p>Item!</p> 11. Click "Save" 12. Go back to your course reserve 13. Note the HTML is scrubbed on "Public note" 14. Go to http://localhost:8081/cgi-bin/koha/course_reserves/course-reserves.pl 15. Note the HTML is scrubbed on "Public note" 16. Go to http://localhost:8080/cgi-bin/koha/opac-course-reserves.pl 17. Note the HTML is scrubbed on "Notes" 18. Click on your course reserves 19. Note the HTML is scrubbed on "Notes" Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Thank you for the rebase. Tested, works as described. Applied to 24.05.x-security Applied to 23.11.x-security Pushed for 25.05! Well done everyone, thank you! |
Created attachment 137774 [details] 4 images, shows where payload in entered and the resulting popup when opening the course reserve or listing it The vulnerability is present on the intranet of Koha (staff-side). When adding a new course reserve, the staff member has the possibility to enter a "private/staff note" field and a "public note" field. The public note field is vulnerable to XSS. Entering <script>alert(1)</script> in this field (as written), then saving the new course reserve will cause the database to save the HTML and will trigger a popup alert at any time a user opens the course reserve. A user on the client side, such as a student, would also get the popup alert if he were to open the course reserve page. The same problem has been found in the "public note" field when adding a copy of a book to a course, for example. The staff member once again has access to this field and can inject javascript there that is then triggered for everyone viewing it. The severity appears minor since you have to have a staff account with adequate permissions to write in this "public note" field. The XSS likely could not be used to steal a session cookie considering that the cookie is set to HTTPOnly. Note that the vulnerability could be present on other "public note" fields in Koha that I couldn't validate. I strongly advise filing for a CVE for this vulnerability that I have been able to exploit on Koha 21.11.06 and Koha 21.05.11, suggesting this might be present in many versions of the software.