From 2fc50229712955aa7557c49af2614988cf938e03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Schild?= Date: Fri, 24 Jul 2026 15:07:46 +0200 Subject: [PATCH] Security: fix XXE in XML parsing and remove credentials from request URLs 1) XMLAnswerParser now unmarshals through a hardened StAX reader with DTD support and external entity resolution disabled, so a malicious or MITM'd server response can no longer trigger XXE (local file read / SSRF / entity expansion DoS). 2) ConnectorCommon no longer embeds the basic-auth login/password in the request URL userinfo; credentials are already supplied via the request context, so this only removed a leak vector (logs/proxies/exceptions). Adds XMLAnswerParserSecurityTest covering both the XXE rejection and that valid XML still parses. Co-Authored-By: Claude Opus 4.8 --- Changelog.md | 5 ++ .../nextcloud/api/utils/ConnectorCommon.java | 7 +-- .../nextcloud/api/utils/XMLAnswerParser.java | 31 ++++++++- .../utils/XMLAnswerParserSecurityTest.java | 63 +++++++++++++++++++ 4 files changed, 99 insertions(+), 7 deletions(-) create mode 100644 src/test/java/org/aarboard/nextcloud/api/utils/XMLAnswerParserSecurityTest.java diff --git a/Changelog.md b/Changelog.md index 5e6350f..27a66b1 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,6 +1,11 @@ # Changelog for nextcloud api ## Version 14.2.0 +- Security: harden XML response parsing against XXE by disabling DTDs and + external entities in the StAX parser +- Security: no longer embed the basic-auth credentials in the request URL + (they are sent via the request context instead), avoiding password leakage + through logs, proxies or exceptions - Add support for the Group Folders app: create, rename, delete and list group folders, grant/revoke group access, set group permissions and set the folder quota via the new `GroupFolders` connector and `NextcloudConnector` methods diff --git a/src/main/java/org/aarboard/nextcloud/api/utils/ConnectorCommon.java b/src/main/java/org/aarboard/nextcloud/api/utils/ConnectorCommon.java index b2d0237..cd9a061 100644 --- a/src/main/java/org/aarboard/nextcloud/api/utils/ConnectorCommon.java +++ b/src/main/java/org/aarboard/nextcloud/api/utils/ConnectorCommon.java @@ -134,10 +134,9 @@ private URI buildUrl(String subPath, List queryParams, boolean us .setPort(serverConfig.getPort()) .setPath(subPath); - if (serverConfig.getAuthenticationConfig().usesBasicAuthentication()) { - uB.setUserInfo(serverConfig.getAuthenticationConfig().getLoginName(), - serverConfig.getAuthenticationConfig().getPassword()); - } + // Basic auth credentials are supplied via the request context + // (see prepareContext()); they are deliberately NOT embedded in the URL + // to avoid leaking the password through logs, proxies or exceptions. if (queryParams != null) { uB.addParameters(queryParams); diff --git a/src/main/java/org/aarboard/nextcloud/api/utils/XMLAnswerParser.java b/src/main/java/org/aarboard/nextcloud/api/utils/XMLAnswerParser.java index 1191dc6..1671a72 100644 --- a/src/main/java/org/aarboard/nextcloud/api/utils/XMLAnswerParser.java +++ b/src/main/java/org/aarboard/nextcloud/api/utils/XMLAnswerParser.java @@ -7,6 +7,9 @@ import java.io.Reader; import java.util.HashMap; import java.util.Map; +import javax.xml.stream.XMLInputFactory; +import javax.xml.stream.XMLStreamException; +import javax.xml.stream.XMLStreamReader; import org.aarboard.nextcloud.api.exception.NextcloudApiException; import org.aarboard.nextcloud.api.utils.ConnectorCommon.ResponseParser; @@ -14,8 +17,24 @@ public class XMLAnswerParser implements ResponseParser { private static final Map> PARSERS = new HashMap<>(); + /** + * Shared, hardened StAX factory. DTD support and external entity resolution + * are disabled to prevent XXE attacks from a malicious or MITM'd server + * response. The factory is only configured once here and thereafter used + * read-only, which is safe to share across threads. + */ + private static final XMLInputFactory XML_INPUT_FACTORY = createHardenedInputFactory(); + private final JAXBContext jAXBContext; + private static XMLInputFactory createHardenedInputFactory() + { + XMLInputFactory factory = XMLInputFactory.newFactory(); + factory.setProperty(XMLInputFactory.SUPPORT_DTD, false); + factory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); + return factory; + } + public XMLAnswerParser(Class answerClass) { try { @@ -57,9 +76,15 @@ public A parseResponse(Reader xmlStream) } @SuppressWarnings("unchecked") - private A tryParseAnswer(Reader xmlStream) throws JAXBException { + private A tryParseAnswer(Reader xmlStream) throws JAXBException, XMLStreamException { Unmarshaller unmarshaller = jAXBContext.createUnmarshaller(); - Object result = unmarshaller.unmarshal(xmlStream); - return (A) result; + // Unmarshal through the hardened StAX reader (not the raw Reader) so + // DTDs / external entities are never processed. + XMLStreamReader xmlStreamReader = XML_INPUT_FACTORY.createXMLStreamReader(xmlStream); + try { + return (A) unmarshaller.unmarshal(xmlStreamReader); + } finally { + xmlStreamReader.close(); + } } } diff --git a/src/test/java/org/aarboard/nextcloud/api/utils/XMLAnswerParserSecurityTest.java b/src/test/java/org/aarboard/nextcloud/api/utils/XMLAnswerParserSecurityTest.java new file mode 100644 index 0000000..09a79b5 --- /dev/null +++ b/src/test/java/org/aarboard/nextcloud/api/utils/XMLAnswerParserSecurityTest.java @@ -0,0 +1,63 @@ +/* + * Copyright (C) 2026 a.schild + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.aarboard.nextcloud.api.utils; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; + +import java.io.StringReader; + +import org.aarboard.nextcloud.api.exception.NextcloudApiException; +import org.aarboard.nextcloud.api.filesharing.SharesXMLAnswer; +import org.junit.Test; + +/** + * Security tests for {@link XMLAnswerParser}: a malicious/MITM'd server + * response must not be able to trigger XXE, and valid XML must still parse. + * + * @author a.schild + */ +public class XMLAnswerParserSecurityTest { + + /** + * A response declaring a DOCTYPE / external entity must be rejected (DTDs + * are disabled), so the entity is never resolved (no file read / SSRF). + */ + @Test(expected = NextcloudApiException.class) + public void testExternalEntityIsNotResolved() { + String malicious = "" + + "]>" + + "&xxe;"; + XMLAnswerParser.getInstance(SharesXMLAnswer.class) + .parseResponse(new StringReader(malicious)); + } + + /** + * Regression guard: hardening the parser must not break normal parsing. + */ + @Test + public void testValidXmlStillParses() { + String valid = "" + + "ok100" + + "1/test"; + SharesXMLAnswer answer = XMLAnswerParser.getInstance(SharesXMLAnswer.class) + .parseResponse(new StringReader(valid)); + assertNotNull(answer.getShares()); + assertEquals(1, answer.getShares().size()); + assertEquals("/test", answer.getShares().get(0).getPath()); + } +}