Skip to content

Commit d25c20b

Browse files
committed
fix(xml): harden Namespaces.Get(string) against XXE and entity expansion
Namespaces.Get(string xml) delegated straight to XmlDocument.LoadXml, whose DtdProcessing default and legacy XmlResolver behavior varied across the library's TFMs (net461–net10.0). The v2.6/v2.7 dispatch routing (fix at e96ae6e) plus the new MTConnectVersions.Max fallback widened the number of documents that flow through this parse. Route the parse through an explicit XmlReader with DtdProcessing set to Prohibit and XmlResolver set to null, so DOCTYPE-carrying payloads (including billion-laughs-style entity expansion) and external-entity (file:// / http://) references are refused before parsing rather than resolved. Catch XmlException so downstream callers see the same Max-fallback signal on malformed input rather than an exception. Also short-circuit null / empty input at the entry point instead of letting LoadXml throw ArgumentException. Ultrareview finding F-SEC-001 (HIGH, A05_security-misconfiguration).
1 parent 9fde78c commit d25c20b

2 files changed

Lines changed: 83 additions & 6 deletions

File tree

libraries/MTConnect.NET-XML/Namespaces.cs

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,34 @@ internal static class Namespaces
2121

2222
public static string Get(string xml)
2323
{
24-
var doc = new XmlDocument();
25-
doc.LoadXml(xml);
26-
if (doc != null && doc.DocumentElement != null)
24+
if (string.IsNullOrEmpty(xml)) return null;
25+
26+
// XmlDocument.LoadXml delegates to an internal XmlReader whose
27+
// DtdProcessing default varies across TFMs and whose XmlResolver
28+
// historically resolved external entities. Route the parse through
29+
// an explicit XmlReader with DTD processing prohibited and no
30+
// resolver so unknown or hostile documents cannot exercise
31+
// external-entity or entity-expansion (billion-laughs) paths.
32+
var settings = new XmlReaderSettings
2733
{
28-
return doc.DocumentElement.NamespaceURI;
34+
DtdProcessing = DtdProcessing.Prohibit,
35+
XmlResolver = null,
36+
};
37+
38+
try
39+
{
40+
using (var stringReader = new StringReader(xml))
41+
using (var xmlReader = XmlReader.Create(stringReader, settings))
42+
{
43+
var doc = new XmlDocument { XmlResolver = null };
44+
doc.Load(xmlReader);
45+
if (doc.DocumentElement != null)
46+
{
47+
return doc.DocumentElement.NamespaceURI;
48+
}
49+
}
2950
}
51+
catch (XmlException) { }
3052

3153
return null;
3254
}

tests/MTConnect.NET-XML-Tests/MTConnectVersionDispatchTests.cs

Lines changed: 57 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
// Pins the fix for the `MTConnectVersion.GetByNamespace` dispatch-chain
55
// omission: the switch capped out at `Namespaces.Version25.Match(ns)` and
66
// fell through to `return new Version()` (empty, "0.0") for any namespace
7-
// declared by a document newer than v2.5 - even though `Namespaces.Version26`
7+
// declared by a document newer than v2.5 even though `Namespaces.Version26`
88
// / `Namespaces.Version27` and `MTConnectVersions.Version26` /
99
// `MTConnectVersions.Version27` both already existed. A document declaring
1010
// `urn:mtconnect.org:MTConnectStreams:2.7` (or `:2.6`) therefore resolved to
@@ -159,7 +159,7 @@ public void GetByNamespace_returns_matching_version_for_v26_and_v27_across_docum
159159

160160
// Regression guard for the specific bug: before the fix, a v2.7
161161
// namespace fell all the way through the chain (Version25 was the
162-
// highest branch present) and returned `new Version()` - equal to
162+
// highest branch present) and returned `new Version()` equal to
163163
// "0.0", not `MTConnectVersions.Version27`.
164164
/// <summary>Pins that a v2.7 namespace does not fall through to an empty version.</summary>
165165
[Test]
@@ -276,5 +276,60 @@ public void Get_v27_xml_does_not_fall_through_to_empty_version()
276276
Assert.That(actual.Major, Is.EqualTo(2));
277277
Assert.That(actual.Minor, Is.EqualTo(7));
278278
}
279+
280+
/// <summary>Pins that <see cref="MTConnectVersion.Get(string)"/> rejects a document that declares a DTD — the hardened <c>Namespaces.Get</c> sets <c>DtdProcessing = Prohibit</c>, so any DOCTYPE-carrying payload (including billion-laughs entity-expansion attempts) is refused rather than parsed. Guards the XXE / entity-expansion surface introduced by <c>XmlDocument.LoadXml</c>'s historical default settings.</summary>
281+
[Test]
282+
public void Get_rejects_document_with_dtd_and_defaults_to_Max()
283+
{
284+
var xml = "<?xml version=\"1.0\"?>" +
285+
"<!DOCTYPE root [<!ELEMENT root ANY>]>" +
286+
"<root xmlns=\"urn:mtconnect.org:MTConnectStreams:2.7\" />";
287+
var actual = MTConnectVersion.Get(xml);
288+
Assert.That(actual, Is.EqualTo(MTConnectVersions.Max));
289+
}
290+
291+
/// <summary>Pins that <see cref="MTConnectVersion.Get(string)"/> rejects a billion-laughs entity-expansion payload without parsing it — the hardened <c>Namespaces.Get</c> refuses the DOCTYPE up-front, so the exponentially-expanding entity chain never materialises. Regression guard for the XXE / entity-expansion surface.</summary>
292+
[Test]
293+
public void Get_rejects_billion_laughs_payload_and_defaults_to_Max()
294+
{
295+
var xml = "<?xml version=\"1.0\"?>" +
296+
"<!DOCTYPE lolz [" +
297+
" <!ENTITY lol \"lol\">" +
298+
" <!ENTITY lol2 \"&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;\">" +
299+
" <!ENTITY lol3 \"&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;\">" +
300+
"]>" +
301+
"<lolz>&lol3;</lolz>";
302+
var actual = MTConnectVersion.Get(xml);
303+
Assert.That(actual, Is.EqualTo(MTConnectVersions.Max));
304+
}
305+
306+
/// <summary>Pins that <see cref="MTConnectVersion.Get(string)"/> rejects a document that references an external entity without resolving it — the hardened <c>Namespaces.Get</c> sets <c>XmlResolver = null</c>, so file:// / http:// references cannot exfiltrate host data. Regression guard for the classic XXE surface.</summary>
307+
[Test]
308+
public void Get_rejects_external_entity_reference_and_defaults_to_Max()
309+
{
310+
var xml = "<?xml version=\"1.0\"?>" +
311+
"<!DOCTYPE foo [<!ENTITY xxe SYSTEM \"file:///etc/passwd\">]>" +
312+
"<foo>&xxe;</foo>";
313+
var actual = MTConnectVersion.Get(xml);
314+
Assert.That(actual, Is.EqualTo(MTConnectVersions.Max));
315+
}
316+
317+
/// <summary>Pins that <see cref="MTConnectVersion.Get(string)"/> returns the Max fallback on malformed input rather than propagating an <c>XmlException</c>. Guards downstream callers from having to catch XML-parse errors on every dispatch call.</summary>
318+
[Test]
319+
public void Get_returns_Max_on_malformed_xml()
320+
{
321+
var actual = MTConnectVersion.Get("<not-well-formed");
322+
Assert.That(actual, Is.EqualTo(MTConnectVersions.Max));
323+
}
324+
325+
/// <summary>Pins that <see cref="MTConnectVersion.Get(string)"/> returns the Max fallback on a null or empty input rather than throwing an <c>ArgumentException</c>.</summary>
326+
/// <param name="xml">The input under test.</param>
327+
[TestCase(null)]
328+
[TestCase("")]
329+
public void Get_returns_Max_on_null_or_empty_input(string xml)
330+
{
331+
var actual = MTConnectVersion.Get(xml);
332+
Assert.That(actual, Is.EqualTo(MTConnectVersions.Max));
333+
}
279334
}
280335
}

0 commit comments

Comments
 (0)