From 6864890e5f3b88abb0652dabeb7cff7d90ecdeed Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 06:20:48 -0400 Subject: [PATCH] fix(notifications): prohibit DTD processing in EwsResponseParser (XXE guard) The review asked for a regression test pinning DTD-prohibited parsing. Writing it showed the premise was wrong: XDocument.Parse permits an internal DTD subset and expands its entities, so a DOCTYPE-bearing response body parsed fine and the guard did not exist (entity-expansion DoS on external input). Parse now goes through XmlReader with DtdProcessing.Prohibit and a null XmlResolver. The regression test feeds a DOCTYPE + ENTITY payload shaped as a well-formed EWS error response, so it fails if DtdProcessing is ever loosened rather than passing for the unrelated-XML reason. --- .../Ews/EwsResponseParser.cs | 16 +++++++- .../Ews/EwsResponseParserTests.cs | 39 +++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.NotificationService/Ews/EwsResponseParser.cs b/src/ZB.MOM.WW.ScadaBridge.NotificationService/Ews/EwsResponseParser.cs index c4bf7750..c209df09 100644 --- a/src/ZB.MOM.WW.ScadaBridge.NotificationService/Ews/EwsResponseParser.cs +++ b/src/ZB.MOM.WW.ScadaBridge.NotificationService/Ews/EwsResponseParser.cs @@ -1,3 +1,4 @@ +using System.Xml; using System.Xml.Linq; namespace ZB.MOM.WW.ScadaBridge.NotificationService.Ews; @@ -64,9 +65,20 @@ public static class EwsResponseParser XDocument document; try { - document = XDocument.Parse(responseBody); + // The body is external input, so DTDs are prohibited outright: LINQ-to-XML's own + // XDocument.Parse permits an internal DTD subset and expands its entities (verified), + // which is an entity-expansion DoS on a response body. A null resolver additionally + // blocks external entity/DTD fetches (XXE). + var settings = new XmlReaderSettings + { + DtdProcessing = DtdProcessing.Prohibit, + XmlResolver = null, + }; + + using var reader = XmlReader.Create(new StringReader(responseBody), settings); + document = XDocument.Load(reader); } - catch (System.Xml.XmlException) + catch (XmlException) { return UnparseableResult; } diff --git a/tests/ZB.MOM.WW.ScadaBridge.NotificationService.Tests/Ews/EwsResponseParserTests.cs b/tests/ZB.MOM.WW.ScadaBridge.NotificationService.Tests/Ews/EwsResponseParserTests.cs index 4272fd2a..c60d8994 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.NotificationService.Tests/Ews/EwsResponseParserTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.NotificationService.Tests/Ews/EwsResponseParserTests.cs @@ -112,6 +112,45 @@ public class EwsResponseParserTests Assert.Equal("An internal server error occurred.", result.MessageText); } + [Fact] + public void Parse_PayloadWithDtdEntityDeclaration_IsRejectedAsUnparseable() + { + // Pins DTD-prohibited parsing (XXE guard). The response body is external input, so the + // parser must never process a DOCTYPE. XDocument.Parse prohibits DTDs by default; a + // refactor that supplies XmlReaderSettings with a looser DtdProcessing fails here. + // + // The payload is deliberately a well-formed EWS response so the assertion has teeth: with + // DTDs prohibited the DOCTYPE itself throws and the body is Unparseable, whereas any + // DtdProcessing.Parse configuration expands the internal entity and yields Kind=Error. + // The external file:/// entity is declared alongside it as the XXE payload that must + // likewise never be reached. + const string xxe = """ + + + + ]> + + + + + + &expanded; + ErrorServerBusy + + + + + + """; + + var result = EwsResponseParser.Parse(xxe); + + Assert.Equal(EwsResponseKind.Unparseable, result.Kind); + Assert.Null(result.ResponseCode); + Assert.Null(result.MessageText); + } + [Theory] [InlineData("not xml")] [InlineData("")]