diff --git a/src/main/java/com/ibm/cldk/artifacts/ConfigKeys.java b/src/main/java/com/ibm/cldk/artifacts/ConfigKeys.java index 707906d9..f98ed48e 100644 --- a/src/main/java/com/ibm/cldk/artifacts/ConfigKeys.java +++ b/src/main/java/com/ibm/cldk/artifacts/ConfigKeys.java @@ -8,10 +8,12 @@ import java.io.IOException; import java.io.StringReader; import java.util.ArrayList; +import java.util.Arrays; import java.util.Comparator; import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Properties; import java.util.Set; @@ -155,6 +157,9 @@ public static Result extract(JArtifact artifact, String text, boolean captureVal artifact.getId(), "env", recognizeComposeEnv(flat), captureValue, false, k -> CanId.configKeyEnvDualMintId(artifact.getId(), k))); } else if ("xml".equals(artifact.getFormat())) { + if (!isConfigXml(artifact.getPath())) { + return new Result(List.of(), true); + } keys = extractXml(artifact.getId(), text, captureValue); } else { return new Result(List.of(), true); @@ -166,6 +171,115 @@ public static Result extract(JArtifact artifact, String text, boolean captureVal } } + /** + * XML basenames worth flattening. Discovery classifies every {@code *.xml} in a repository as an + * artifact, and the flattener mints a key per element path -- which is what configuration looks + * like, and also what a data file looks like. On ThingsBoard v4.0 that produced 39,551 keys from + * 453 XML files, 30,408 of them from LwM2M object registries under {@code src/main/data/}: static + * data resources, not settings (#210). + * + *

No size threshold separates the two -- that repository's {@code pom.xml} legitimately carries + * 1332 keys while a data registry carries 615 -- so recognition is by name, not by volume. A file + * that is not recognized stays in the artifact inventory with its {@code source}, {@code sha256} + * and roles intact; only flattening is skipped, so nothing disappears from the file listing and + * the result is {@code ok=true}, not a parse failure. + * + *

A name list alone is not enough, because vendor deployment descriptors are open-ended + * ({@code ibm-web-bnd.xml}, {@code jboss-web.xml}, {@code weblogic.xml}, ...) -- daytrader8's two + * {@code ibm-web-*.xml} descriptors are real configuration and no plausible list contains them. + * So location counts too: {@code WEB-INF/} and {@code META-INF/} are directories the servlet and + * Java EE specifications define to hold descriptors, so an XML sitting directly in one is + * configuration by construction rather than by guess. Data resources do not live there. + * + *

The accepted cost is a project whose configuration lives in an unlisted XML name outside + * those directories. That file is still inventoried and still discoverable; it just contributes + * no {@code config_keys}. Adding a name here is the fix when one turns up -- prefer that to + * widening the rule to "any XML under a config-ish directory", which is how the unbounded + * behaviour arose in the first place. + */ + private static final Set CONFIG_XML_NAMES = new HashSet<>(Arrays.asList( + // build / project + "pom.xml", "build.xml", "settings.xml", "toolchains.xml", "ivy.xml", "assembly.xml", + // servlet / Java EE descriptors + "web.xml", "ejb-jar.xml", "application.xml", "application-client.xml", "persistence.xml", + "beans.xml", "faces-config.xml", "orm.xml", "validation.xml", "portlet.xml", "ra.xml", + // containers and app servers + "server.xml", "context.xml", "tomcat-users.xml", "standalone.xml", "domain.xml", + "jboss-web.xml", "jboss-ejb3.xml", "jboss-deployment-structure.xml", "weblogic.xml", + "glassfish-web.xml", "glassfish-application.xml", "blueprint.xml", + // frameworks + "struts.xml", "tiles.xml", "hibernate.cfg.xml", "quartz.xml", "ehcache.xml", + "liquibase.xml", "bindings.xml", "sun-jaxb.episode", + // logging + "logback.xml", "logback-test.xml", "logback-spring.xml", + "log4j.xml", "log4j2.xml", "log4j2-spring.xml", "log4j2-test.xml", + // static analysis / tooling + "checkstyle.xml", "spotbugs.xml", "findbugs-exclude.xml", "pmd.xml", "suppressions.xml", + // misc + "config.xml", "androidmanifest.xml")); + + /** + * Name stems that mark a configuration file whatever separator precedes them. Matching only + * hyphenated forms was the first attempt and it missed the canonical spellings of the very + * frameworks it was meant to cover: {@code applicationContext.xml} (Spring) and {@code + * UserMapper.xml} (MyBatis) both failed a {@code -context.xml}/{@code -mapper.xml} test, while + * {@code spring-servlet.xml} passed. Comparing against the lowercased stem catches camelCase, + * hyphenated and underscored spellings alike. + * + *

Deliberately excludes short stems that occur inside ordinary words -- {@code "ds"} would + * admit {@code records.xml} -- so those stay hyphen-only in {@link #CONFIG_XML_SUFFIXES}. + */ + private static final List CONFIG_XML_STEMS = + Arrays.asList("context", "config", "configuration", "settings", "beans", "servlet", + "mapper", "changelog", "datasource"); + + /** Suffixes that must keep their separator, being too short or too common to match bare. */ + private static final List CONFIG_XML_SUFFIXES = Arrays.asList("-ds.xml", ".hbm.xml"); + + /** + * Whether an XML artifact is a configuration file worth flattening: a recognized basename, a + * conventional suffix, or any XML in a descriptor directory. A suffix never matches bare -- + * {@code notweb.xml} must not pass as {@code web.xml}. + * + * @param path the artifact's project-relative path (the directory part carries the location signal) + */ + static boolean isConfigXml(String path) { + String lower = basename(path).toLowerCase(Locale.ROOT); + if (CONFIG_XML_NAMES.contains(lower)) { + return true; + } + for (String suffix : CONFIG_XML_SUFFIXES) { + // A suffix match needs something in front of it, so ".hbm.xml" itself is not a match. + if (lower.length() > suffix.length() && lower.endsWith(suffix)) { + return true; + } + } + if (lower.endsWith(".xml")) { + String stem = lower.substring(0, lower.length() - ".xml".length()); + for (String token : CONFIG_XML_STEMS) { + // The stem must be preceded by something, so a file named exactly "config.xml" is + // matched by name above rather than here, and "context.xml" likewise. + if (stem.length() > token.length() && stem.endsWith(token)) { + return true; + } + } + } + return inDescriptorDirectory(path); + } + + /** True when the file sits directly inside a {@code WEB-INF} or {@code META-INF} directory. */ + private static boolean inDescriptorDirectory(String path) { + String normalized = path.replace('\\', '/'); + int slash = normalized.lastIndexOf('/'); + if (slash < 0) { + return false; + } + String parent = normalized.substring(0, slash); + int parentSlash = parent.lastIndexOf('/'); + String dir = parentSlash < 0 ? parent : parent.substring(parentSlash + 1); + return "WEB-INF".equalsIgnoreCase(dir) || "META-INF".equalsIgnoreCase(dir); + } + // ---- properties: java.util.Properties handles key=value/key:value, `\` continuations, and // `#`/`!` comments natively -- and last-wins, since a later duplicate key simply overwrites the // earlier one on load. Using the JDK parser buys all of that for free, at the cost of position @@ -251,8 +365,7 @@ private static boolean isDigits(String s) { private static List extractXml(String artifactId, String text, boolean captureValue) throws ParserConfigurationException, SAXException, IOException { - Document doc = ManifestParsers.newSecureDocumentBuilderFactory() - .newDocumentBuilder() + Document doc = ManifestParsers.newConfigDocumentBuilder() .parse(new InputSource(new StringReader(text))); Element root = doc.getDocumentElement(); List entries = new ArrayList<>(); diff --git a/src/main/java/com/ibm/cldk/artifacts/ManifestParsers.java b/src/main/java/com/ibm/cldk/artifacts/ManifestParsers.java index 281356f4..797778f6 100644 --- a/src/main/java/com/ibm/cldk/artifacts/ManifestParsers.java +++ b/src/main/java/com/ibm/cldk/artifacts/ManifestParsers.java @@ -16,8 +16,10 @@ import org.w3c.dom.Element; import org.w3c.dom.Node; import org.w3c.dom.NodeList; +import org.xml.sax.ErrorHandler; import org.xml.sax.InputSource; import org.xml.sax.SAXException; +import org.xml.sax.SAXParseException; /** * Pure dependency-manifest and lockfile readers: text in, records out. No file I/O, no execution, @@ -174,7 +176,7 @@ private static String kindForMavenDependency(String scope, boolean optional) { } private static DocumentBuilder newSecureDocumentBuilder() throws ParserConfigurationException { - return newSecureDocumentBuilderFactory().newDocumentBuilder(); + return quiet(newSecureDocumentBuilderFactory().newDocumentBuilder()); } /** @@ -190,6 +192,11 @@ private static DocumentBuilder newSecureDocumentBuilder() throws ParserConfigura * toggles are kept as explicit defense in depth alongside it, and {@code * FEATURE_SECURE_PROCESSING} restricts external DTD/entity resource access (JAXP's {@code * accessExternalDTD}) independently of both. + * + *

A dependency manifest has no legitimate reason to declare a DOCTYPE, so this stays + * categorical. Configuration files do -- {@code logback.xml} routinely carries one -- and they + * are parsed through {@link #newConfigDocumentBuilder} instead, which permits an internal subset + * while keeping every external switch here identical. */ static DocumentBuilderFactory newSecureDocumentBuilderFactory() throws ParserConfigurationException { DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); @@ -200,6 +207,66 @@ static DocumentBuilderFactory newSecureDocumentBuilderFactory() throws ParserCon return factory; } + /** + * The parser for configuration files, which differs from {@link #newSecureDocumentBuilderFactory} + * in exactly one respect: an internal DOCTYPE subset is allowed. + * + *

Refusing every DOCTYPE rejects the whole document rather than the dangerous part. On + * ThingsBoard v4.0 that discarded 43 files, 35 of them {@code logback.xml} -- precisely the + * configuration this analyzer exists to report, silently reduced to {@code extraction=partial} + * with no keys. XXE needs an external reference to resolve, and all four external + * routes stay closed here (general entities, parameter entities, external DTD loading, + * XInclude), with {@code FEATURE_SECURE_PROCESSING} capping entity expansion so an internal + * "billion laughs" is refused rather than expanded. {@code setExpandEntityReferences(false)} + * additionally keeps entity bodies out of the walked tree. + * + *

Manifests deliberately do NOT use this: a pom.xml declaring a DOCTYPE has no legitimate + * shape, so it keeps the categorical refusal. + */ + static DocumentBuilderFactory newConfigDocumentBuilderFactory() throws ParserConfigurationException { + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + factory.setFeature("http://xml.org/sax/features/external-general-entities", false); + factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); + factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); + factory.setXIncludeAware(false); + factory.setExpandEntityReferences(false); + return factory; + } + + /** A configuration-file builder that does not narrate to stderr. */ + static DocumentBuilder newConfigDocumentBuilder() throws ParserConfigurationException { + return quiet(newConfigDocumentBuilderFactory().newDocumentBuilder()); + } + + /** + * Silence the JDK parser's own commentary. With no error handler installed it prints every + * problem to {@code System.err} itself -- raw {@code [Fatal Error] :19:10: ...} lines that + * bypass {@link com.ibm.cldk.utils.Log}, tell the user nothing actionable, and on a large + * repository arrive dozens at a time. Failures already travel in each caller's + * {@code (records, ok)} result, so the thrown exception carries the signal; this removes only + * the duplicate narration. + */ + static DocumentBuilder quiet(DocumentBuilder builder) { + builder.setErrorHandler(new ErrorHandler() { + @Override + public void warning(SAXParseException e) {} + + @Override + public void error(SAXParseException e) throws SAXException { + throw e; + } + + @Override + public void fatalError(SAXParseException e) throws SAXException { + throw e; + } + }); + return builder; + } + private static List directChildren(Element parent, String tagName) { List children = new ArrayList<>(); NodeList nodes = parent.getChildNodes(); diff --git a/src/test/java/com/ibm/cldk/artifacts/ConfigKeysTest.java b/src/test/java/com/ibm/cldk/artifacts/ConfigKeysTest.java index 3f481d71..21fc0382 100644 --- a/src/test/java/com/ibm/cldk/artifacts/ConfigKeysTest.java +++ b/src/test/java/com/ibm/cldk/artifacts/ConfigKeysTest.java @@ -35,6 +35,87 @@ private static Optional find(List keys, String key) { return keys.stream().filter(k -> k.getKey().equals(key)).findFirst(); } + // ---- xml: only recognized configuration shapes are flattened (#210) ---------------------- + + /** A data file shaped like the LwM2M object registries that produced 30k keys on ThingsBoard. */ + private static String dataXml() { + StringBuilder b = new StringBuilder(""); + for (int i = 0; i < 40; i++) { + b.append("n").append(i) + .append("Integer"); + } + return b.append("").toString(); + } + + @Test + void aRecognizedConfigXmlIsStillFlattened() { + ConfigKeys.Result r = ConfigKeys.extract(artifact("src/main/webapp/WEB-INF/web.xml", "xml"), + "mode", true); + assertTrue(r.ok); + assertFalse(r.keys.isEmpty(), "web.xml is configuration and must still be flattened"); + } + + @Test + void pomXmlIsStillFlattened() { + ConfigKeys.Result r = ConfigKeys.extract(artifact("pom.xml", "xml"), + "11", true); + assertTrue(r.ok); + assertFalse(r.keys.isEmpty(), "pom.xml carries real configuration"); + } + + @Test + void anUnrecognizedXmlYieldsNoKeysButIsNotAFailure() { + // The artifact stays in the inventory; only flattening is skipped. ok=true, exactly as for a + // format with no flattener -- marking it a parse failure would misreport a healthy file. + ConfigKeys.Result r = ConfigKeys.extract( + artifact("application/src/main/data/lwm2m-registry/10326.xml", "xml"), dataXml(), true); + assertTrue(r.ok, "skipping extraction is not a parse failure"); + assertTrue(r.keys.isEmpty(), "a data registry is not configuration and must not be flattened"); + } + + @Test + void aSuffixMatchDoesNotAdmitAnArbitraryFile() { + // "notweb.xml" ends with "web.xml" but is not it; recognition is on the whole basename. + ConfigKeys.Result r = ConfigKeys.extract(artifact("src/notweb.xml", "xml"), dataXml(), true); + assertTrue(r.ok); + assertTrue(r.keys.isEmpty(), "recognition must match the basename, not a suffix of it"); + } + + @Test + void aVendorDescriptorInWebInfIsRecognizedByItsLocation() { + // daytrader8 ships ibm-web-bnd.xml / ibm-web-ext.xml. Vendor descriptors are open-ended, so + // no name list catches them; WEB-INF is a spec-defined descriptor directory, so location does. + ConfigKeys.Result r = ConfigKeys.extract(artifact("src/main/webapp/WEB-INF/ibm-web-bnd.xml", "xml"), + "", true); + assertTrue(r.ok); + assertFalse(r.keys.isEmpty(), "an XML in WEB-INF is a deployment descriptor"); + } + + @Test + void anXmlInMetaInfIsRecognizedByItsLocation() { + ConfigKeys.Result r = ConfigKeys.extract(artifact("src/main/resources/META-INF/anything.xml", "xml"), + "1", true); + assertTrue(r.ok); + assertFalse(r.keys.isEmpty(), "META-INF is a descriptor directory"); + } + + @Test + void aDescriptorDirectoryDeeperInThePathDoesNotCount() { + // Only the immediate parent counts: data nested far below a WEB-INF is not a descriptor. + ConfigKeys.Result r = ConfigKeys.extract(artifact("WEB-INF/data/registry/10326.xml", "xml"), + dataXml(), true); + assertTrue(r.ok); + assertTrue(r.keys.isEmpty(), "the file must sit directly in the descriptor directory"); + } + + @Test + void aSuffixPatternStillMatchesItsFamily() { + ConfigKeys.Result r = ConfigKeys.extract(artifact("src/main/resources/app-context.xml", "xml"), + "", true); + assertTrue(r.ok); + assertFalse(r.keys.isEmpty(), "*-context.xml is a recognized Spring configuration shape"); + } + // ---- properties ------------------------------------------------------------------------- @Test diff --git a/src/test/java/com/ibm/cldk/artifacts/XmlHardeningTest.java b/src/test/java/com/ibm/cldk/artifacts/XmlHardeningTest.java new file mode 100644 index 00000000..21d5c59e --- /dev/null +++ b/src/test/java/com/ibm/cldk/artifacts/XmlHardeningTest.java @@ -0,0 +1,113 @@ +package com.ibm.cldk.artifacts; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.io.StringReader; +import java.nio.file.Files; +import java.nio.file.Path; +import javax.xml.parsers.ParserConfigurationException; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.w3c.dom.Document; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; + +/** + * The XML parser reads untrusted repository content, so its hardening is load-bearing and each + * guarantee is asserted separately rather than trusted as a bundle. + * + *

An internal DOCTYPE subset is permitted on purpose: refusing every DOCTYPE rejected the whole + * document rather than the dangerous part, discarding 43 real files on ThingsBoard v4.0 (35 of them + * {@code logback.xml}). These tests pin that the permission did not buy an XXE back — every + * external reference must still fail to resolve. + */ +class XmlHardeningTest { + + private static Document parse(String xml) throws ParserConfigurationException, SAXException, IOException { + return ManifestParsers.newConfigDocumentBuilder().parse(new InputSource(new StringReader(xml))); + } + + @Test + void anInternalDoctypeSubsetIsAccepted() throws Exception { + // The logback.xml shape: a DOCTYPE that references nothing outside the document. + Document d = parse("\n"); + assertNotNull(d.getDocumentElement()); + assertTrue("configuration".equals(d.getDocumentElement().getTagName())); + } + + @Test + void anExternalEntityDoesNotLeakFileContents(@TempDir Path tmp) throws Exception { + Path secret = tmp.resolve("secret.txt"); + Files.write(secret, "TOP_SECRET_VALUE".getBytes("UTF-8")); + String xml = "]>\n&xxe;"; + + String text; + try { + text = parse(xml).getDocumentElement().getTextContent(); + } catch (SAXException blocked) { + return; // Refusing outright is an equally acceptable outcome. + } + assertFalse(text.contains("TOP_SECRET_VALUE"), + "external entity must never resolve — this is the XXE the hardening exists to stop"); + } + + @Test + void anExternalParameterEntityIsNotFetched(@TempDir Path tmp) throws Exception { + Path dtd = tmp.resolve("evil.dtd"); + Files.write(dtd, "".getBytes("UTF-8")); + String xml = " %ext;]>\nok"; + try { + assertFalse(parse(xml).getDocumentElement().getTextContent().contains("PWNED"), + "external parameter entity must not be fetched"); + } catch (SAXException blocked) { + // Also fine. + } + } + + @Test + void anExternalDtdIsNotLoaded(@TempDir Path tmp) throws Exception { + Path dtd = tmp.resolve("ext.dtd"); + Files.write(dtd, "".getBytes("UTF-8")); + String xml = "\nok"; + try { + assertFalse(parse(xml).getDocumentElement().getTextContent().contains("INJECTED"), + "an external DTD must not be loaded"); + } catch (SAXException blocked) { + // Also fine. + } + } + + @Test + void aBillionLaughsExpansionIsRefusedRatherThanExhaustingMemory() { + StringBuilder dtd = new StringBuilder(""); + for (int i = 1; i <= 9; i++) { + dtd.append(""); + } + dtd.append("]>\n&lol9;"); + // FEATURE_SECURE_PROCESSING caps entity expansion, so this fails fast instead of hanging. + assertThrows(SAXException.class, () -> parse(dtd.toString()), + "unbounded entity expansion must be refused"); + } + + @Test + void theParserDoesNotWriteToStderrOnAMalformedDocument() throws Exception { + java.io.PrintStream original = System.err; + java.io.ByteArrayOutputStream captured = new java.io.ByteArrayOutputStream(); + System.setErr(new java.io.PrintStream(captured, true, "UTF-8")); + try { + assertThrows(SAXException.class, () -> parse("")); + } finally { + System.setErr(original); + } + assertTrue(captured.toString("UTF-8").isEmpty(), + "the parser must not narrate to stderr — errors travel in the result, not as [Fatal Error] spam"); + } +}