From 50f1f9b07059fc1ad91fddb060b5045f35edaf14 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Wed, 22 Jul 2026 15:53:33 -0700 Subject: [PATCH 1/3] Use centrally configured SchemaFactory --- .../targetedms/parser/skyaudit/SkylineAuditLogParser.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/org/labkey/targetedms/parser/skyaudit/SkylineAuditLogParser.java b/src/org/labkey/targetedms/parser/skyaudit/SkylineAuditLogParser.java index 0c38c0784..a8902e8e9 100644 --- a/src/org/labkey/targetedms/parser/skyaudit/SkylineAuditLogParser.java +++ b/src/org/labkey/targetedms/parser/skyaudit/SkylineAuditLogParser.java @@ -31,7 +31,6 @@ import org.labkey.targetedms.parser.XmlUtil; import org.xml.sax.SAXException; -import javax.xml.XMLConstants; import javax.xml.stream.XMLStreamException; import javax.xml.stream.XMLStreamReader; import javax.xml.transform.stream.StreamSource; @@ -114,10 +113,10 @@ private void validateXml() throws IOException, SAXException, AuditLogParsingExce try (InputStream schemaStream = new BufferedInputStream(openSchemaInputStream()); InputStream auditLogStream = new BufferedInputStream(new FileInputStream(_file))) { - //prepare validator - SchemaFactory schemaFactory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI); + //prepare validator, hardened against XXE (CWE-611) since the audit log is attacker-supplied + SchemaFactory schemaFactory = XmlBeansUtil.schemaFactory(); Schema schema = schemaFactory.newSchema(new StreamSource(schemaStream)); - Validator validator = schema.newValidator(); + Validator validator = XmlBeansUtil.hardenValidator(schema.newValidator()); validator.validate(new StreamSource(auditLogStream)); } } From b39a525b1685dca8b37cfc9ca27742e12fdd391f Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Thu, 6 Aug 2026 18:40:38 -0700 Subject: [PATCH 2/3] Improve test coverage --- .../labkey/targetedms/TargetedMSModule.java | 1 + .../skyaudit/SkylineAuditLogParser.java | 66 ++++++++++++++++++- 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/src/org/labkey/targetedms/TargetedMSModule.java b/src/org/labkey/targetedms/TargetedMSModule.java index 925e84906..fd5f6ff36 100644 --- a/src/org/labkey/targetedms/TargetedMSModule.java +++ b/src/org/labkey/targetedms/TargetedMSModule.java @@ -718,6 +718,7 @@ protected void startupAfterSpringConfig(ModuleContext moduleContext) ReplicateLabelMinimizer.TestCase.class, SampleFile.TestCase.class, SkylineAuditLogParser.TestCase.class, + SkylineAuditLogParser.XxeTestCase.class, TargetedMSController.TestCase.class, PrecursorManager.TestCase.class, CrossLinkedPeptideInfo.TestCase.class, diff --git a/src/org/labkey/targetedms/parser/skyaudit/SkylineAuditLogParser.java b/src/org/labkey/targetedms/parser/skyaudit/SkylineAuditLogParser.java index a8902e8e9..43326702e 100644 --- a/src/org/labkey/targetedms/parser/skyaudit/SkylineAuditLogParser.java +++ b/src/org/labkey/targetedms/parser/skyaudit/SkylineAuditLogParser.java @@ -24,6 +24,8 @@ import org.labkey.api.module.Module; import org.labkey.api.module.ModuleLoader; import org.labkey.api.resource.FileResource; +import org.labkey.api.util.ExternalReferenceProbe; +import org.labkey.api.util.FileUtil; import org.labkey.api.util.GUID; import org.labkey.api.util.Path; import org.labkey.api.util.XmlBeansUtil; @@ -44,6 +46,8 @@ import java.io.IOException; import java.io.InputStream; import java.math.BigDecimal; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; import java.time.format.DateTimeParseException; import java.util.Collections; import java.util.LinkedList; @@ -113,7 +117,7 @@ private void validateXml() throws IOException, SAXException, AuditLogParsingExce try (InputStream schemaStream = new BufferedInputStream(openSchemaInputStream()); InputStream auditLogStream = new BufferedInputStream(new FileInputStream(_file))) { - //prepare validator, hardened against XXE (CWE-611) since the audit log is attacker-supplied + // Use a factory Hardened against XXE SchemaFactory schemaFactory = XmlBeansUtil.schemaFactory(); Schema schema = schemaFactory.newSchema(new StreamSource(schemaStream)); Validator validator = XmlBeansUtil.hardenValidator(schema.newValidator()); @@ -375,5 +379,65 @@ public void testInvalidXmlFile() throws IOException //TODO: Validate against different files. } + /** + * XXE (CWE-611) coverage for {@link SkylineAuditLogParser#validateXml()}, where the hardening carries the most + * weight: unlike the SAML path nothing guards the uploaded .skyl before it reaches the validator, so if the + * hardening doesn't hold an uploader can make the server fetch a URL of their choosing. + * + *

Separate from {@link TestCase}, which needs a database for its {@code @Before} cleanup. + */ + public static class XxeTestCase extends Assert + { + /** + * {@link #validateXml} resolves the schema first, so an unavailable module resource means it throws before + * parsing any XML and every probe-based assertion below goes green while proving nothing. + */ + @Before + public void schemaMustBeResolvable() + { + Module module = ModuleLoader.getInstance().getModule(TargetedMSModule.class); + assertNotNull("TargetedMS module must be registered, otherwise validateXml() never validates", module); + assertNotNull("Bundled " + SCHEMA_FILE + " must be resolvable, otherwise validateXml() never validates", + module.getModuleResolver().lookup(Path.parse(SCHEMA_FILE))); + } + + /** Conforming apart from the injected reference, so validation gets far enough to matter. */ + private static final String AUDIT_LOG = + "" + + "hash" + + "" + + ""; + + /** + * {@code XmlBeansUtil.TestCase} does primary XXE validation. Just prove that validateXml() gets the hardened factory. + */ + @Test + public void validateXmlRefusesExternalDtdSubset() throws Exception + { + // Qualified because org.labkey.api.util.Path wins the simple name in this file + java.nio.file.Path dir = Files.createTempDirectory("skylineAuditLogXxe"); + try (ExternalReferenceProbe probe = ExternalReferenceProbe.start()) + { + File logFile = dir.resolve("audit.skyl").toFile(); + Files.writeString(logFile.toPath(), + "" + AUDIT_LOG, + StandardCharsets.UTF_8); + + // Expected to fail on this input; the assertion is about the fetch on the way there + try (SkylineAuditLogParser ignored = new SkylineAuditLogParser(logFile, LogManager.getLogger(XxeTestCase.class))) + { + fail("Should have failed"); + } + catch (Exception _) {} + + probe.assertNotContacted("Validating an uploaded Skyline audit log must not resolve an external DTD subset"); + } + finally + { + FileUtil.deleteDir(dir.toFile()); + } + } + } } From b2a191c330ed16f94b4ce409f44f2707c29954c7 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Thu, 6 Aug 2026 19:29:17 -0700 Subject: [PATCH 3/3] Improve comments --- .../targetedms/parser/skyaudit/SkylineAuditLogParser.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/org/labkey/targetedms/parser/skyaudit/SkylineAuditLogParser.java b/src/org/labkey/targetedms/parser/skyaudit/SkylineAuditLogParser.java index 43326702e..0e6e7b980 100644 --- a/src/org/labkey/targetedms/parser/skyaudit/SkylineAuditLogParser.java +++ b/src/org/labkey/targetedms/parser/skyaudit/SkylineAuditLogParser.java @@ -117,7 +117,7 @@ private void validateXml() throws IOException, SAXException, AuditLogParsingExce try (InputStream schemaStream = new BufferedInputStream(openSchemaInputStream()); InputStream auditLogStream = new BufferedInputStream(new FileInputStream(_file))) { - // Use a factory Hardened against XXE + // Use a factory hardened against XXE SchemaFactory schemaFactory = XmlBeansUtil.schemaFactory(); Schema schema = schemaFactory.newSchema(new StreamSource(schemaStream)); Validator validator = XmlBeansUtil.hardenValidator(schema.newValidator());