Skip Menu |

This queue is for tickets about the XML-LibXML-Simple CPAN distribution.

Report information
The Basics
Id: 68803
Status: resolved
Priority: 0/
Queue: XML-LibXML-Simple
People
Owner: Nobody in particular
Requestors: yann.kerherve [...] gmail.com
Cc:
AdminCc:
Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)

History Show all quoted text
  Mon Jun 13 17:39:17 2011 yann.kerherve [...] gmail.com - Ticket created
Subject: [patch] Allow parser options to be specified
This is pretty critical for downstream projects for security reasons. See suggested patch below: Thanks, Yann diff --git a/lib/XML/LibXML/Simple.pm b/lib/XML/LibXML/Simple.pm index 6b1afcc..5994afa 100644 --- a/lib/XML/LibXML/Simple.pm +++ b/lib/XML/LibXML/Simple.pm @@ -25,7 +25,7 @@ use Data::Dumper; #to be removed my %known_opts = map { ($_ => 1) } qw(keyattr keeproot forcecontent contentkey noattr searchpath forcearray grouptags nsexpand normalisespace normalizespace - valueattr nsstrip); + valueattr nsstrip parseropts); my @DefKeyAttr = qw(name key id); my $DefContentKey = qq(content); @@ -66,7 +66,8 @@ sub _get_xml($$) $source = $self->default_data_source($opts) unless defined $source; $source = \*STDIN if $source eq '-'; - my $parser = XML::LibXML->new; + my %parser_opts = %{ $self->{opts} || {} }; + my $parser = XML::LibXML->new(%parser_opts); my $xml = UNIVERSAL::isa($source,'XML::LibXML::Document') ? $source
  Tue Jun 14 04:07:58 2011 Mark [...] Overmeer.net - Correspondence added
Subject: Re: [rt.cpan.org #68803] [patch] Allow parser options to be specified
Date: Tue, 14 Jun 2011 10:07:42 +0200
To: Yann Kerherve via RT <bug-XML-LibXML-Simple [...] rt.cpan.org>
From: Mark Overmeer <mark [...] overmeer.net>
* Yann Kerherve via RT (bug-XML-LibXML-Simple@rt.cpan.org) [110613 21:39]: Show quoted text
> Mon Jun 13 17:39:17 2011: Request 68803 was acted upon. > Transaction: Ticket created by YANNK > Queue: XML-LibXML-Simple > Subject: [patch] Allow parser options to be specified > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=68803 > > > This is pretty critical for downstream projects for security reasons. > + my $parser = XML::LibXML->new(%parser_opts);
My usual "trick" is to give the users an opportunty to pass their own fully prepared parser object. That offers more flexibility. Having my own "downstream projects" with the module, I really wonder what kind of security problems you think of. -- Regards, MarkOv ------------------------------------------------------------------------ Mark Overmeer MSc MARKOV Solutions Mark@Overmeer.net solutions@overmeer.net http://Mark.Overmeer.net http://solutions.overmeer.net
  Tue Jun 14 04:08:01 2011 The RT System itself - Status changed from 'new' to 'open'
  Tue Jun 14 04:57:44 2011 Mark [...] Overmeer.net - Correspondence added
Subject: Re: [rt.cpan.org #68803] [patch] Allow parser options to be specified
Date: Tue, 14 Jun 2011 10:57:32 +0200
To: Yann Kerherve via RT <bug-XML-LibXML-Simple [...] rt.cpan.org>
From: Mark Overmeer <mark [...] overmeer.net>
* Yann Kerherve via RT (bug-XML-LibXML-Simple@rt.cpan.org) [110613 21:39]: Show quoted text
> Mon Jun 13 17:39:17 2011: Request 68803 was acted upon. > Transaction: Ticket created by YANNK > Queue: XML-LibXML-Simple > Subject: [patch] Allow parser options to be specified > > $source = $self->default_data_source($opts) unless defined $source; > $source = \*STDIN if $source eq '-'; > - my $parser = XML::LibXML->new; > + my %parser_opts = %{ $self->{opts} || {} }; > + my $parser = XML::LibXML->new(%parser_opts);
Probably, you mean Show quoted text
> + my %parser_opts = %{ $self->{parseropts} || {} };
new() also accepts the options as hash, so + my $parser = XML::LibXML->new($self->{parseropts} || {}); I put "$self->{parseropts} ||= {};" into init, so that makes: my $parser = $opts->{parser} || XML::LibXML->new($opts->{parseropts}); -- Regards, MarkOv ------------------------------------------------------------------------ Mark Overmeer MSc MARKOV Solutions Mark@Overmeer.net solutions@overmeer.net http://Mark.Overmeer.net http://solutions.overmeer.net
  Tue Jun 14 12:05:25 2011 yann.kerherve [...] gmail.com - Correspondence added
I didn't notice it was possible to pass on a custom parser. You are right about opts, I didn't test that. A downstream project: https://github.com/yannk/web- oembed/commit/84a17199e0e091b2a096f6b3306cb329c1a7518f This is an example of attack: https://github.com/yannk/p5-RPC-XML-Parser- LibXML/commit/eda00faa6a6a74bf187f19de7cf6c5b6702c475a You can find a bunch more in my github timeline. Thanks, Yann
  Tue Jun 14 16:20:16 2011 Mark [...] Overmeer.net - Correspondence added
Subject: Re: [rt.cpan.org #68803] [patch] Allow parser options to be specified
Date: Tue, 14 Jun 2011 22:20:00 +0200
To: Yann Kerherve via RT <bug-XML-LibXML-Simple [...] rt.cpan.org>
From: Mark Overmeer <mark [...] overmeer.net>
* Yann Kerherve via RT (bug-XML-LibXML-Simple@rt.cpan.org) [110614 16:05]: Show quoted text
> Queue: XML-LibXML-Simple > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=68803 > > > I didn't notice it was possible to pass on a custom parser.
Not possible yet, but will also be added. Show quoted text
> This is an example of attack: > https://github.com/yannk/p5-RPC-XML-Parser- > LibXML/commit/eda00faa6a6a74bf187f19de7cf6c5b6702c475a
Very interesting. XML::LibXML::Parser is unclear about the defaults. Your problems seem to depend on expand_entities. What is its default? Can you suggest a safe default? -- Regards, MarkOv ------------------------------------------------------------------------ drs Mark A.C.J. Overmeer MARKOV Solutions Mark@Overmeer.net solutions@overmeer.net http://Mark.Overmeer.net http://solutions.overmeer.net
  Tue Jun 14 16:32:26 2011 yann.kerherve [...] gmail.com - Correspondence added
Show quoted text
> Very interesting. XML::LibXML::Parser is unclear about the defaults. > Your problems seem to depend on expand_entities. What is its > default?
default is true Show quoted text
> Can you suggest a safe default?
That's my suggestion: no_network => 1, expand_xinclude => 0, expand_entities => 1, load_ext_dtd => 0, ext_ent_handler => sub { warn "External entities disabled."; '' }, You can adjust depending on your needs (should the parser die, or not, what kind of logging you want etc...)
  Wed Jun 15 04:26:34 2011 webmaster [...] nlnet.nl - Correspondence added
Subject: Re: [rt.cpan.org #68803] [patch] Allow parser options to be specified
Date: Wed, 15 Jun 2011 10:26:17 +0200
To: Yann Kerherve via RT <bug-XML-LibXML-Simple [...] rt.cpan.org>
From: NLnet webmaster <webmaster [...] nlnet.nl>
* Yann Kerherve via RT (bug-XML-LibXML-Simple@rt.cpan.org) [110614 20:32]: Show quoted text
> Queue: XML-LibXML-Simple > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=68803 > >
> > Very interesting. XML::LibXML::Parser is unclear about the defaults. > > Your problems seem to depend on expand_entities. What is its > > default?
> > default is true
Ah, the default is documented on a place I didn't expect. I have released XML-LibXML-Simple-0.90.tar.gz which initializes the parser with safe settings by default. I'll also release a XML::Compile with the same improvement. Probably you are not using that module. -- Regards, MarkOv ------------------------------------------------------------------------ Mark Overmeer MSc MARKOV Solutions Mark@Overmeer.net solutions@overmeer.net http://Mark.Overmeer.net http://solutions.overmeer.net
  Sat Feb 25 08:55:43 2012 MARKOV [...] cpan.org - Correspondence added
released in 0.90, june 2011
  Sat Feb 25 08:55:44 2012 MARKOV [...] cpan.org - Status changed from 'open' to 'resolved'