Hi Jonathan, Sorry, I wasn't precise in my language. I know that we already use \Q...\E in Koha, but I wonder how many places in Koha we are passing variables into regular expressions without considering whether or not we might need it there. I don't know the contexts but here are some examples: authorities/authorities-home.pl: $query_cgi_history =~ s/^$path_info\?//; C4/Patroncards/Patroncard.pm: $_ =~ s/$_/$borrower_attributes->{$field}/; C4/Labels/Label.pm: elsif ( $f =~ /^($match_kohatable).*/ ) { C4/External/Syndetics.pm: if (exists $response->{$available_type} && $response->{$available_type} =~ /$available_type/) { Maybe by convention most of these won't be a problem but some of them might be. That $borrower_attributes->{$field} one could possibly be user-entered data. It could be interesting trying to craft a malicious regex to inject into that line. Most cases probably wouldn't be too problematic. This one is problematic because it's inability to work can cause infinite loops which can bring down web servers or potentially even entire servers. C4/Patroncards/Patroncard.pm: $line =~ s/$1//; I'm just thinking that maybe we should be more careful with what we're feeding into regular expressions. (Although the infinite loop is actually indicative of other problems with C4/Patroncards/Patroncard.pm...) David Cook Systems Librarian Prosentient Systems 72/330 Wattle St Ultimo, NSW 2007 Australia Office: 02 9212 0899 Direct: 02 8005 0595 -----Original Message----- From: Jonathan Druart [mailto:jonathan.druart@bugs.koha-community.org] Sent: Thursday, 7 March 2019 3:01 AM To: David Cook <dcook@prosentient.com.au> Cc: koha-devel <koha-devel@lists.koha-community.org> Subject: Re: [Koha-devel] Using unsanitized data in regular expressions We already use it in several places. I found 28 occurrences in the current code. Le lun. 4 mars 2019 à 04:00, David Cook <dcook@prosentient.com.au> a écrit :

Hi all,

I encountered a problem recently where create-pdf.pl was stuck in an infinite loop because a string passed into ‘$line =~ s/$1//;’ in C4::Patroncards::Patroncard contained “(TEST)” and it was treating the parentheses as metacharacters and not literal values.

I fixed the problem by changing the line to ‘$line =~ s/\Q$1\E//;’, so that anything in $1 would be escaped, but it got me thinking about other parts of Koha. I’m not saying that it’s necessarily a problem in other parts of Koha, but that it might be good to be mindful of it.

David Cook

Systems Librarian

Prosentient Systems

72/330 Wattle St

Ultimo, NSW 2007

Australia

Office: 02 9212 0899

Direct: 02 8005 0595

_______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/

Hi Victor, did you see *Bug 22462* <https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22462> - Crash in patron card printing ? Katrin On 07.03.19 11:22, Victor Grousset wrote:

On 19-03-07 06:43, David Cook wrote:

...

This one is problematic because it's inability to work can cause infinite loops which can bring down web servers or potentially even entire servers. C4/Patroncards/Patroncard.pm:                $line =~ s/$1//;

Indeed, because the process won't timeout and it will take on Starman worker forever (when it's used)

...

I'm just thinking that maybe we should be more careful with what we're feeding into regular expressions.

(Although the infinite loop is actually indicative of other problems with C4/Patroncards/Patroncard.pm...)

There might be a linked issue in the label creator. A colleague of mine triggered an infinite loop when creating label by setting "Lower left X coordinate" to 100. We hope to soon retry to reproduce and create a bugzilla about this.

Cheers,

On 19-03-08 06:33, David Cook wrote:

Thanks, Victor. Yeah, it’s certainly related. It’s the same block of code. If the “Lower left X coordinate” plus the length of the string is wider than the template width, it’ll try to line wrap the string.

Bug 22462 fixes an issue with the line wrapping, but your issue will still happen.

Doesn't bug 22462 describes the same thing? «this brings down the instance until the process is killed (like with a Plack restart)» -- Victor Grousset, dev support/maintenance BibLibre, Services en logiciels libres pour les bibliothèques BibLibre, Libre/Open Source software and services for libraries