Bug 33568 changed the staff interface so that the items are rendered using Javascript. While escape_str will prevent some XSS by escaping some characters, it's not a comprehensive solution, and it's trivial to bypass.
Created attachment 170486 [details] [review] Bug 37681: Fix XSS in staff interface item URLs on detail page This patch uses Javascript objects and safe sinks to prevent XSS in the item URLs on the staff interface detail page. It also makes sure those URLs don't get double-escaped. Yippee! Test plan: 0. Apply the patch 1. Add/edit an item with the following URL: http://prosentient.com.au?q=http%3A%2F%2Fprosentient.com.au 2. Add/edit a different item with the following URLs: http://prosentient.com.au?q=http%3A%2F%2Fprosentient.com.au | http://prosentient.com.au?q=http%3A%2F%2Fprosentient.com.au 3. Go to the staff interface detail page 4. Notice that the URLs are not double-encoded! 5. Try out a malicious payload (talk to QA/security about this) 6. Confirm that the malicious payload fails to execute the XSS 7. Celebrate!
Created attachment 170525 [details] [review] Bug 37681: Fix XSS in staff interface item URLs on detail page This patch uses Javascript objects and safe sinks to prevent XSS in the item URLs on the staff interface detail page. It also makes sure those URLs don't get double-escaped. Yippee! Test plan: 0. Apply the patch 1. Add/edit an item with the following URL: http://prosentient.com.au?q=http%3A%2F%2Fprosentient.com.au 2. Add/edit a different item with the following URLs: http://prosentient.com.au?q=http%3A%2F%2Fprosentient.com.au | http://prosentient.com.au?q=http%3A%2F%2Fprosentient.com.au 3. Go to the staff interface detail page 4. Notice that the URLs are not double-encoded! 5. Try out a malicious payload (talk to QA/security about this) 6. Confirm that the malicious payload fails to execute the XSS 7. Celebrate! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
All looking good and works as expected.. I do wonder however if we should move that safe_url function up into a shared js so we can utilize it elsewhere in the future? I feel like we probably have more cases of this exposure lurking.
+ //console.error('Invalid URL:', e); At least debug maybe? Ignoring an exception is hardly ever a good idea.
(In reply to Jonathan Druart from comment #7) > + //console.error('Invalid URL:', e); > > At least debug maybe? Ignoring an exception is hardly ever a good idea. Since this is client side, it's just going to be shown to end users who won't be able to do anything useful about it. In this case, we're not really ignoring the exception. We could set url_str to '#' when there is an exemption, but we don't need to, because we already have that as a default value. I just left the commented out logging there so that if anyone was wanting to do some troubleshooting later they could do it more easily.
Not convinced but ok, I don't want to deviate the discussion. But if this is reused to sanitize an input and for whatever reason an exception is raised, the user save the form and the data are lost... silently :)
(In reply to Jonathan Druart from comment #9) > Not convinced but ok, I don't want to deviate the discussion. But if this is > reused to sanitize an input and for whatever reason an exception is raised, > the user save the form and the data are lost... silently :) Previously, I indicated that this exact function shouldn't be re-used elsewhere. It's a context-specific function. Since this is display only, no data is lost. If you want to re-add the console.error() here, I'm fine with it. I left it there so that people could think about it. I don't think it's necessary, but that's just my opinion. Happy for someone to disagree and do something different with the console.error().
(In reply to David Cook from comment #10) > (In reply to Jonathan Druart from comment #9) > > Not convinced but ok, I don't want to deviate the discussion. But if this is > > reused to sanitize an input and for whatever reason an exception is raised, > > the user save the form and the data are lost... silently :) > > Previously, I indicated that this exact function shouldn't be re-used > elsewhere. It's a context-specific function. Where is it written? In a comment in bugzilla? :) If you really think something should not be reused, then you should add that info in the code. See bug 37637 that is going to reuse this code, wouldn't they want to reuse it? Copy/paste the function in another template? Or move it to a global .js file to avoid that. Then it's available from everywhere and the "do not reuse this" info is totally lost.
(In reply to Jonathan Druart from comment #11) > > Previously, I indicated that this exact function shouldn't be re-used > > elsewhere. It's a context-specific function. > > Where is it written? In a comment in bugzilla? :) If you really think > something should not be reused, then you should add that info in the code. You're probably right. I must've written it in Mattermost. > See bug 37637 that is going to reuse this code, wouldn't they want to reuse > it? I don't understand what you're saying here. The use of escape_str there is different. They're escaping the entire string rather than parts of a string. That's what I mean about context. > Copy/paste the function in another template? Or move it to a global .js > file to avoid that. Then it's available from everywhere and the "do not > reuse this" info is totally lost. It could make sense to copy/paste this function in certain contexts. I'm not saying this function is "the best" function. It just works well here. If the function were moved to global.js, then the function would need to be changed significantly, and then it ends up being pointless, because we're just putting a wrapper around the native APIs. But if people want to do that... they can go for it.
safe sinks ?
(In reply to David Cook from comment #8) > (In reply to Jonathan Druart from comment #7) > > + //console.error('Invalid URL:', e); > > > > At least debug maybe? Ignoring an exception is hardly ever a good idea. > > Since this is client side, it's just going to be shown to end users who > won't be able to do anything useful about it. > > In this case, we're not really ignoring the exception. We could set url_str > to '#' when there is an exemption, but we don't need to, because we already > have that as a default value. > > I just left the commented out logging there so that if anyone was wanting to > do some troubleshooting later they could do it more easily. I would just remove the comments here. A developer can see the error directly, most end users wont see it or even know how to find it. I would not recommend adding commented statements. We have them elsewhere in the codebase (in perl) too. So nice ;) And yes, this is just one line.
Created attachment 171117 [details] [review] Bug 37681: Fix XSS in staff interface item URLs on detail page This patch uses Javascript objects and safe sinks to prevent XSS in the item URLs on the staff interface detail page. It also makes sure those URLs don't get double-escaped. Yippee! Test plan: 0. Apply the patch 1. Add/edit an item with the following URL: http://prosentient.com.au?q=http%3A%2F%2Fprosentient.com.au 2. Add/edit a different item with the following URLs: http://prosentient.com.au?q=http%3A%2F%2Fprosentient.com.au | http://prosentient.com.au?q=http%3A%2F%2Fprosentient.com.au 3. Go to the staff interface detail page 4. Notice that the URLs are not double-encoded! 5. Try out a malicious payload (talk to QA/security about this) 6. Confirm that the malicious payload fails to execute the XSS 7. Celebrate! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Is this required for 23.05? I get a merge conflict when backporting: deleted by us: koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers/tables/items/catalogue_detail.inc Meaning this file doesn't exist in 23.05?
Depends on Bug 33568 only for >= 24.05.x
(In reply to Fridolin Somers from comment #17) > Depends on Bug 33568 only for >= 24.05.x Oh thanks Fridolin, I will not apply to 23.05.x-security
Backported to 24.05.x for 24.05.04
Pushed for 24.11! Well done everyone, thank you!