From 7759714c5ad3790332046962060b884143bb18ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Ferr=C3=A3o?= <2031761+viniciusferrao@users.noreply.github.com> Date: Tue, 1 Sep 2026 21:49:11 -0300 Subject: [PATCH 1/5] refactor(xcatd): drop the XML parser options that never reach the parser Both parser constructors passed a list of options to XML::Parser as an array reference: XML::Parser->new(Style => 'Tree', [ load_ext_dtd => 0, ... ]); XML::Parser->new takes a flat list of pairs. The reference is one value in that list, so the constructor reads the pairs as Style => 'Tree' and then the reference as the name of an option with no value. Every option inside the reference is dropped. The names are also the names that XML::LibXML uses, not the names that XML::Parser uses, so the parser would ignore them even if it received them. The options therefore never did anything, and they give the reader the impression that the parser refuses an external entity because of them. The handler on the next line is what refuses an external entity. Remove them. Behaviour does not change. --- xCAT-server/lib/perl/xCAT/XML.pm | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/xCAT-server/lib/perl/xCAT/XML.pm b/xCAT-server/lib/perl/xCAT/XML.pm index 9c3a2afdc..cce54bcdc 100644 --- a/xCAT-server/lib/perl/xCAT/XML.pm +++ b/xCAT-server/lib/perl/xCAT/XML.pm @@ -50,12 +50,7 @@ sub build_tree_xml_parser { carp "'nsexpand' option requires XML::SAX"; } - my $xp = XML::Parser->new(Style => 'Tree', - [ load_ext_dtd => 0, - ext_ent_handler => undef, - no_network => 1, - expand_entities => 0, - ]); + my $xp = XML::Parser->new(Style => 'Tree'); $xp->setHandlers(ExternEnt => sub { return $_[2] }); my($tree); if($filename) { @@ -72,12 +67,7 @@ sub build_tree_xml_parser { sub new_xml_parser { my($self) = @_; - my $xp = XML::Parser->new(Style => 'Tree', - [ load_ext_dtd => 0, - ext_ent_handler => undef, - no_network => 1, - expand_entities => 0, - ]); + my $xp = XML::Parser->new(Style => 'Tree'); $xp->setHandlers(ExternEnt => sub {return $_[2]}); return $xp; } From cfb54fcf1aeb3121b36fa935945e9c53e273ca9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Ferr=C3=A3o?= <2031761+viniciusferrao@users.noreply.github.com> Date: Tue, 1 Sep 2026 22:05:56 -0300 Subject: [PATCH 2/5] test(xcatd): assert the outcome of an external entity, not the route The test required the parser to parse a payload that names an external entity and to leave the system identifier of that entity in the document. That is one way to keep the contents of the named file out of the document, and it is the way the parser behaves today, but it is not the contract. The contract is that the contents never arrive. Assert that instead: the contents reach neither the document nor the error. A parser that refuses the payload keeps the contract as well as a parser that parses it and leaves the entity alone. Behaviour does not change. The test passes against this branch and against the current parser. --- xCAT-test/unit/xml_external_entity.t | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/xCAT-test/unit/xml_external_entity.t b/xCAT-test/unit/xml_external_entity.t index 02f68605c..cff387e14 100644 --- a/xCAT-test/unit/xml_external_entity.t +++ b/xCAT-test/unit/xml_external_entity.t @@ -38,23 +38,23 @@ sub parsed_tree { return ($tree, $@); } -# A parser path must parse the payload, replace the external entity with its -# system identifier, and never read the file contents. +# The contract this file guards is that the contents of the file an external +# entity names never reach the parsed document. A parser path may deliver that +# either by parsing the payload and leaving the entity unresolved, or by +# refusing the payload. Assert the outcome, not the route. sub check_path { my ($label) = @_; my ($tree, $error) = parsed_tree(); - is($error, '', "$label: the payload parses without error"); - ok(defined($tree), "$label: the parser returns a tree"); my $dump = defined($tree) ? Data::Dumper::Dumper($tree) : ''; - like($dump, qr{\Q$secret_path\E}, - "$label: the external entity is replaced by its system identifier"); unlike($dump, qr/SECRET-CONTENT-DO-NOT-LEAK/, "$label: the external entity content is not read"); + unlike($error, qr/SECRET-CONTENT-DO-NOT-LEAK/, + "$label: the external entity content does not reach the error either"); } # The modern path: XML::Simple with new_xml_parser. SKIP: { - skip 'XML::Simple lacks new_xml_parser on this system', 4 + skip 'XML::Simple lacks new_xml_parser on this system', 2 unless exists &{'XML::Simple::new_xml_parser'}; check_path('modern path'); } From b53ffaf190d3487c06d32995b64849b700386603 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Ferr=C3=A3o?= <2031761+viniciusferrao@users.noreply.github.com> Date: Tue, 1 Sep 2026 22:06:39 -0300 Subject: [PATCH 3/5] fix(xcatd): refuse an XML request that carries a document type declaration The daemon reads the XML of every request through this parser. A request could declare an entity in its own document type declaration, and the parser expanded it. An entity that refers to other entities grows on each level, so a short request expands into a large document and consumes the memory and the time of the daemon. A client holds a certificate before it can send a request, so this needs an account, but the daemon should not accept the work. Refuse the declaration itself. The option that stops the parser from expanding an entity does not cover an entity that a request names inside an attribute, so it leaves the same growth available through a different part of the document. Measured on XML::Parser 2.46, a request of 204 bytes that names its entity in an attribute still grew to 1014 bytes with that option set, which is what the parser does without it. No request that xCAT sends carries a document type declaration. The client builds every request with XML::Simple, which does not write one. The handler that refuses an external entity stays, so a parser that reaches it by another route still refuses to read the named file. --- xCAT-server/lib/perl/xCAT/XML.pm | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/xCAT-server/lib/perl/xCAT/XML.pm b/xCAT-server/lib/perl/xCAT/XML.pm index cce54bcdc..a570c87fc 100644 --- a/xCAT-server/lib/perl/xCAT/XML.pm +++ b/xCAT-server/lib/perl/xCAT/XML.pm @@ -51,7 +51,8 @@ sub build_tree_xml_parser { } my $xp = XML::Parser->new(Style => 'Tree'); - $xp->setHandlers(ExternEnt => sub { return $_[2] }); + $xp->setHandlers(ExternEnt => sub { return $_[2] }, + Doctype => sub { croak 'XML document type declaration is not accepted' }); my($tree); if($filename) { # $tree = $xp->parsefile($filename); # Changed due to prob w/mod_perl @@ -68,7 +69,8 @@ sub build_tree_xml_parser { sub new_xml_parser { my($self) = @_; my $xp = XML::Parser->new(Style => 'Tree'); - $xp->setHandlers(ExternEnt => sub {return $_[2]}); + $xp->setHandlers(ExternEnt => sub {return $_[2]}, + Doctype => sub { croak 'XML document type declaration is not accepted' }); return $xp; } 1; From 82c1ea93dd012d5d9acf2803f283f375bc334bc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Ferr=C3=A3o?= <2031761+viniciusferrao@users.noreply.github.com> Date: Tue, 1 Sep 2026 22:07:19 -0300 Subject: [PATCH 4/5] test(xcatd): pin the refusal of a document type declaration Cover the three shapes a declaration can take: one that names an entity from element text, one that names it from an attribute value, and one that carries no entity at all. The parser must refuse each, and nothing may expand. The attribute case is the one that matters most. The option that stops the parser expanding an entity leaves an attribute alone, so a test that only used element text would pass against a parser that still grows a request through an attribute. Cover an ordinary request as well, so a refusal that is too wide is visible: the command, the node range and the argument must still arrive. Run all of it against the parser of a recent XML::Simple and against the parser this module builds for an older XML::Simple. --- xCAT-test/unit/xml_doctype_refused.t | 93 ++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 xCAT-test/unit/xml_doctype_refused.t diff --git a/xCAT-test/unit/xml_doctype_refused.t b/xCAT-test/unit/xml_doctype_refused.t new file mode 100644 index 000000000..9b125aff1 --- /dev/null +++ b/xCAT-test/unit/xml_doctype_refused.t @@ -0,0 +1,93 @@ +#!/usr/bin/env perl +use strict; +use warnings; + +use FindBin; +use Test::More; + +my $libdir = "$FindBin::Bin/../../xCAT-server/lib/perl"; +my $xmlpm = "$libdir/xCAT/XML.pm"; +plan skip_all => 'xCAT::XML not found' unless -r $xmlpm; +eval { require XML::Simple; require XML::Parser; 1 } + or plan skip_all => 'XML::Simple and XML::Parser are required'; + +# xCAT::XML loads xCAT::MsgUtils, which loads much of the xCAT tree. The parser +# paths never call it. Stub it before loading xCAT::XML. +BEGIN { $INC{'xCAT/MsgUtils.pm'} = 1; } +{ package xCAT::MsgUtils; } + +unshift @INC, $libdir; +require xCAT::XML; +require Data::Dumper; + +# An entity that names another entity grows the document on every level. +my $in_text = <<'XML'; + + ]> +&b; +XML + +# The same growth, with the reference inside an attribute value. The option +# that stops the parser expanding an entity does not reach an attribute, so +# this is the case that a refusal of the declaration has to cover. +my $in_attribute = <<'XML'; + + ]> +t +XML + +# A declaration that carries no entity at all is still refused. +my $bare_doctype = <<'XML'; + + +rpower +XML + +# What a request normally looks like. +my $ordinary = + 'rpower' + . 'n1stat'; + +sub parse_doc { + my ($doc) = @_; + my $tree = eval { xCAT::XML->new->XMLin($doc, SuppressEmpty => undef, ForceArray => 1) }; + return ($@, defined($tree) ? Data::Dumper::Dumper($tree) : ''); +} + +sub check_path { + my ($label) = @_; + + foreach my $case ([ 'in element text', $in_text ], + [ 'in an attribute', $in_attribute ], + [ 'with no entity', $bare_doctype ]) { + my ($name, $doc) = @$case; + my ($err, $dump) = parse_doc($doc); + isnt($err, '', "$label: a declaration $name is refused"); + unlike($dump, qr/AAAAAAAAAA/, "$label: nothing expands for a declaration $name"); + } + + my ($err, $dump) = parse_doc($ordinary); + is($err, '', "$label: an ordinary request parses"); + like($dump, qr/rpower/, "$label: the command of an ordinary request survives"); + like($dump, qr/n1/, "$label: the noderange of an ordinary request survives"); + like($dump, qr/stat/, "$label: the argument of an ordinary request survives"); +} + +# The modern path: XML::Simple with new_xml_parser. +SKIP: { + skip 'XML::Simple lacks new_xml_parser on this system', 10 + unless exists &{'XML::Simple::new_xml_parser'}; + check_path('modern path'); +} + +# Force the older compatibility path (build_tree_xml_parser's own code) by +# removing new_xml_parser, as on XML::Simple 2.20-2.24. +{ + no strict 'refs'; + no warnings 'redefine'; + undef *{'XML::Simple::new_xml_parser'} if exists &{'XML::Simple::new_xml_parser'}; +} +ok(!exists &{'XML::Simple::new_xml_parser'}, 'compatibility path is forced'); +check_path('compatibility path'); + +done_testing(); From 43495f7e562c7e497385873702265ea8b142c9b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Ferr=C3=A3o?= <2031761+viniciusferrao@users.noreply.github.com> Date: Tue, 1 Sep 2026 22:40:22 -0300 Subject: [PATCH 5/5] refactor(xcatd): build the parser of both entry points in one place The module overrides two methods of XML::Simple, one for a recent version and one for an older version, and each built its own parser and set its own handlers. The two bodies were the same apart from spacing, so a change to one refusal had to be repeated in the other, and a reader had to compare them to see that they agreed. Build the parser in one routine that both call. Behaviour does not change. --- xCAT-server/lib/perl/xCAT/XML.pm | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/xCAT-server/lib/perl/xCAT/XML.pm b/xCAT-server/lib/perl/xCAT/XML.pm index a570c87fc..42548b487 100644 --- a/xCAT-server/lib/perl/xCAT/XML.pm +++ b/xCAT-server/lib/perl/xCAT/XML.pm @@ -50,9 +50,7 @@ sub build_tree_xml_parser { carp "'nsexpand' option requires XML::SAX"; } - my $xp = XML::Parser->new(Style => 'Tree'); - $xp->setHandlers(ExternEnt => sub { return $_[2] }, - Doctype => sub { croak 'XML document type declaration is not accepted' }); + my $xp = _hardened_parser(); my($tree); if($filename) { # $tree = $xp->parsefile($filename); # Changed due to prob w/mod_perl @@ -68,8 +66,12 @@ sub build_tree_xml_parser { sub new_xml_parser { my($self) = @_; + return _hardened_parser(); +} + +sub _hardened_parser { my $xp = XML::Parser->new(Style => 'Tree'); - $xp->setHandlers(ExternEnt => sub {return $_[2]}, + $xp->setHandlers(ExternEnt => sub { return $_[2] }, Doctype => sub { croak 'XML document type declaration is not accepted' }); return $xp; }