saml: Safer DocumentBuilderFactory and ParserPool configuration This implements safer DocumentBuilderFactory and ParserPool utilities to be used throughout the codebase to prevent potential XXE exploits. References: https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html https://www.blackhat.com/docs/us-15/materials/us-15-Wang-FileCry-The-New-Age-Of-XXE-java-wp.pdf Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com> (cherry picked from commit 8e0e68ef368ebe2793ef80e2c3821eaecb47b593) Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
diff --git a/plugins/user-authenticators/saml2/pom.xml b/plugins/user-authenticators/saml2/pom.xml index 330d508..f13008b 100644 --- a/plugins/user-authenticators/saml2/pom.xml +++ b/plugins/user-authenticators/saml2/pom.xml
@@ -31,6 +31,7 @@ <dependency> <groupId>org.opensaml</groupId> <artifactId>opensaml</artifactId> + <version>${cs.opensaml.version}</version> </dependency> <dependency> <groupId>org.apache.cloudstack</groupId>
diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/GetServiceProviderMetaDataCmd.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/GetServiceProviderMetaDataCmd.java index 89e3f31..795eae0 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/GetServiceProviderMetaDataCmd.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/GetServiceProviderMetaDataCmd.java
@@ -30,6 +30,7 @@ import org.apache.cloudstack.api.response.SAMLMetaDataResponse; import org.apache.cloudstack.saml.SAML2AuthManager; import org.apache.cloudstack.saml.SAMLProviderMetadata; +import org.apache.cloudstack.utils.security.ParserUtils; import org.apache.log4j.Logger; import org.opensaml.Configuration; import org.opensaml.DefaultBootstrap; @@ -239,7 +240,7 @@ StringWriter stringWriter = new StringWriter(); try { - DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + DocumentBuilderFactory factory = ParserUtils.getSaferDocumentBuilderFactory(); DocumentBuilder builder = factory.newDocumentBuilder(); Document document = builder.newDocument(); Marshaller out = Configuration.getMarshallerFactory().getMarshaller(spEntityDescriptor);
diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java index f0a5bab..ba85b15 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java
@@ -78,7 +78,6 @@ import org.opensaml.saml2.metadata.provider.MetadataProviderException; import org.opensaml.xml.ConfigurationException; import org.opensaml.xml.XMLObject; -import org.opensaml.xml.parse.BasicParserPool; import org.opensaml.xml.security.credential.UsageType; import org.opensaml.xml.security.keyinfo.KeyInfoHelper; import org.springframework.stereotype.Component; @@ -389,7 +388,7 @@ } } _idpMetaDataProvider.setRequireValidMetadata(true); - _idpMetaDataProvider.setParserPool(new BasicParserPool()); + _idpMetaDataProvider.setParserPool(SAMLUtils.getSaferParserPool()); _idpMetaDataProvider.initialize(); _timer.scheduleAtFixedRate(new MetadataRefreshTask(), 0, _refreshInterval * 1000);
diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java index 6110cc5..d33015e 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java
@@ -42,12 +42,15 @@ import java.security.spec.InvalidKeySpecException; import java.security.spec.PKCS8EncodedKeySpec; import java.security.spec.X509EncodedKeySpec; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.zip.Deflater; import java.util.zip.DeflaterOutputStream; import javax.servlet.http.Cookie; import javax.servlet.http.HttpServletResponse; +import javax.xml.XMLConstants; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; @@ -56,6 +59,7 @@ import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.response.LoginCmdResponse; import org.apache.cloudstack.utils.security.CertUtils; +import org.apache.cloudstack.utils.security.ParserUtils; import org.apache.log4j.Logger; import org.bouncycastle.operator.OperatorCreationException; import org.joda.time.DateTime; @@ -88,6 +92,7 @@ import org.opensaml.xml.io.Unmarshaller; import org.opensaml.xml.io.UnmarshallerFactory; import org.opensaml.xml.io.UnmarshallingException; +import org.opensaml.xml.parse.BasicParserPool; import org.opensaml.xml.signature.SignatureConstants; import org.opensaml.xml.util.Base64; import org.opensaml.xml.util.XMLHelper; @@ -227,7 +232,7 @@ public static Response decodeSAMLResponse(String responseMessage) throws ConfigurationException, ParserConfigurationException, SAXException, IOException, UnmarshallingException { - DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance(); + DocumentBuilderFactory documentBuilderFactory = ParserUtils.getSaferDocumentBuilderFactory(); documentBuilderFactory.setNamespaceAware(true); DocumentBuilder docBuilder = documentBuilderFactory.newDocumentBuilder(); byte[] base64DecodedResponse = Base64.decode(responseMessage); @@ -361,4 +366,19 @@ "CN=ApacheCloudStack", "CN=ApacheCloudStack", 3, "SHA256WithRSA"); } + + public static BasicParserPool getSaferParserPool() { + final Map<String, Boolean> features = new HashMap<>(); + features.put(XMLConstants.FEATURE_SECURE_PROCESSING, true); + features.put("http://apache.org/xml/features/disallow-doctype-decl", true); + features.put("http://xml.org/sax/features/external-general-entities", false); + features.put("http://xml.org/sax/features/external-parameter-entities", false); + features.put("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + final BasicParserPool parserPool = new BasicParserPool(); + parserPool.setXincludeAware(false); + parserPool.setIgnoreComments(true); + parserPool.setExpandEntityReferences(false); + parserPool.setBuilderFeatures(features); + return parserPool; + } }
diff --git a/pom.xml b/pom.xml index 1e5a77d..d789c77 100644 --- a/pom.xml +++ b/pom.xml
@@ -159,7 +159,7 @@ <cs.mysql.version>8.0.19</cs.mysql.version> <cs.neethi.version>2.0.4</cs.neethi.version> <cs.nitro.version>10.1</cs.nitro.version> - <cs.opensaml.version>2.6.4</cs.opensaml.version> + <cs.opensaml.version>2.6.6</cs.opensaml.version> <cs.rados-java.version>0.6.0</cs.rados-java.version> <cs.reflections.version>0.9.12</cs.reflections.version> <cs.servicemix.version>3.4.4_1</cs.servicemix.version>
diff --git a/utils/src/main/java/org/apache/cloudstack/utils/security/ParserUtils.java b/utils/src/main/java/org/apache/cloudstack/utils/security/ParserUtils.java new file mode 100644 index 0000000..0e8e9d6 --- /dev/null +++ b/utils/src/main/java/org/apache/cloudstack/utils/security/ParserUtils.java
@@ -0,0 +1,41 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.cloudstack.utils.security; + +import javax.xml.XMLConstants; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; + +public class ParserUtils { + public static DocumentBuilderFactory getSaferDocumentBuilderFactory() throws ParserConfigurationException { + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + + // REDHAT https://www.blackhat.com/docs/us-15/materials/us-15-Wang-FileCry-The-New-Age-Of-XXE-java-wp.pdf + // OWASP https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html + // and these as well, per Timothy Morgan's 2014 paper: "XML Schema, DTD, and Entity Attacks" + factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", 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.setXIncludeAware(false); + factory.setExpandEntityReferences(false); + + return factory; + } +}