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 0c38c0784..0e6e7b980 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; @@ -31,7 +33,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; @@ -45,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; @@ -114,10 +117,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); + // Use a factory hardened against XXE + 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)); } } @@ -376,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()); + } + } + } }