Bug 23290: (follow-up) Disable expand_entities unless explicitly enabled
authorMarcel de Rooy <m.de.rooy@rijksmuseum.nl>
Fri, 17 Jan 2020 11:01:14 +0000 (11:01 +0000)
committerMartin Renvoize <martin.renvoize@ptfs-europe.com>
Tue, 25 Feb 2020 13:41:06 +0000 (13:41 +0000)
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>

Koha/XSLT/Security.pm

index 6c75458..55cbadd 100644 (file)
@@ -55,6 +55,7 @@ sub new {
     my ($class) = @_;
     my $self = {};
 
+    $self->{_options} = {};
     my $conf = C4::Context->config('koha_xslt_security');
     if( $conf && ref($conf) eq 'HASH' ) {
         $self->{_options} = $conf;
@@ -142,11 +143,11 @@ sub set_callbacks {
 sub set_parser_options {
     my ($self, $parser) = @_;
     my $conf = $self->{_options};
-    return if !$conf;
 
     if( $conf->{expand_entities} ) {
         _set_option($parser, 'expand_entities', 1);
     } else {
+        # If not explicitly set, we should disable expanding for security
         _set_option($parser, 'expand_entities', 0);
     }
 }