Skip to content
Permalink
Browse files

Merge pull request #141 from dwnusbaum/harden-against-xxe

[JENKINS-50765] Defend SecretHandler from XXE attacks
  • Loading branch information...
dwnusbaum committed Apr 18, 2018
2 parents 132c99f + 1de266c commit 59261ba0355b403f42e6a8f205f87a6d5758d3d9
@@ -9,6 +9,7 @@
import org.xml.sax.SAXException;
import org.xml.sax.XMLReader;

import javax.xml.XMLConstants;
import javax.xml.transform.OutputKeys;
import javax.xml.transform.Result;
import javax.xml.transform.Source;
@@ -17,10 +18,12 @@
import javax.xml.transform.TransformerFactory;
import javax.xml.transform.sax.SAXSource;
import javax.xml.transform.stream.StreamResult;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.File;
import java.io.IOException;
import java.io.StringReader;
import java.nio.charset.StandardCharsets;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

@@ -37,6 +40,7 @@
* our placeholder
*/
protected static final String SECRET_MARKER = "#secret#";
protected static final String XXE_MARKER = "#XXE#";
public static final String OUTPUT_ENCODING = "UTF-8";
public static final Pattern SECRET_PATTERN = Pattern.compile(">\\{(.*)\\}<|>(.*)\\=<");

@@ -90,10 +94,12 @@ public void characters(char[] ch, int start, int length) throws SAXException {
}
};
String str = FileUtils.readFileToString(xmlFile);
Source src = new SAXSource(xr, new InputSource(new StringReader(str)));
Source src = createSafeSource(xr, new InputSource(new StringReader(str)));
final ByteArrayOutputStream result = new ByteArrayOutputStream();
Result res = new StreamResult(result);
Transformer transformer = TransformerFactory.newInstance().newTransformer();
TransformerFactory factory = TransformerFactory.newInstance();
factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
Transformer transformer = factory.newTransformer();
//omit xml declaration because of https://bugs.openjdk.java.net/browse/JDK-8035437
transformer.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "yes");
transformer.setOutputProperty(OutputKeys.ENCODING, OUTPUT_ENCODING);
@@ -125,5 +131,26 @@ private static String findSecretFallback(String xml) {
return xml;
}

/**
* Converts a Source into a Source that is protected against XXE attacks.
* @see jenkins.util.xml.XMLUtils#safeTransform
*/
private static Source createSafeSource(XMLReader reader, InputSource source) {
try {
reader.setFeature("http://xml.org/sax/features/external-general-entities", false);
} catch (SAXException ignored) {
/* Fallback entity resolver will be used */
}
try {
reader.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
} catch (SAXException ignored) {
/* Fallback entity resolver will be used */
}
// Fallback in case the above features are not supported by the underlying XML library.
reader.setEntityResolver((publicId, systemId) -> {
return new InputSource(new ByteArrayInputStream(XXE_MARKER.getBytes(StandardCharsets.US_ASCII)));
});
return new SAXSource(reader, source);
}

}
@@ -11,7 +11,10 @@

import java.io.File;

import static org.hamcrest.Matchers.anyOf;
import static org.hamcrest.Matchers.containsString;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThat;

public class SecretHandlerTest {

@@ -88,4 +91,23 @@ public void shouldPutAPlaceHolderInsteadOfSecret() throws Exception {
FileUtils.writeStringToFile(file, xml);
String patchedXml = SecretHandler.findSecrets(file);
assertEquals(expectedXml, patchedXml);
}}
}

@Test
@Issue("JENKINS-50765")
public void shouldNotResolveExternalEntities() throws Exception {
final String xxeXml = "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n" +
"<!DOCTYPE test [ \n" +
" <!ENTITY xxeattack SYSTEM \"file:///\"> \n" +
"]>\n" +
"<xxx>&xxeattack;</xxx>";
File file = File.createTempFile("test", ".xml");
FileUtils.writeStringToFile(file, xxeXml);
String redactedXxeXml = SecretHandler.findSecrets(file);
// Either the XML library understands the XXE disabling features, and removes XXEs completely,
// or our custom EntityResolver is used which replaces them with a placeholder.
assertThat(redactedXxeXml, anyOf(
containsString("<xxx/>"),
containsString("<xxx>" + SecretHandler.XXE_MARKER + "</xxx>")));
}
}

0 comments on commit 59261ba

Please sign in to comment.
You can’t perform that action at this time.