diff --git a/pom.xml b/pom.xml index f7e971a..daa5deb 100644 --- a/pom.xml +++ b/pom.xml @@ -28,6 +28,15 @@ 2.361.4 + + + com.github.tomakehurst + wiremock-jre8-standalone + 2.35.0 + test + + + repo.jenkins-ci.org diff --git a/src/main/java/hudson/model/ExternalRun.java b/src/main/java/hudson/model/ExternalRun.java index 92345e8..d602294 100644 --- a/src/main/java/hudson/model/ExternalRun.java +++ b/src/main/java/hudson/model/ExternalRun.java @@ -48,6 +48,8 @@ * @author Kohsuke Kawaguchi */ public class ExternalRun extends Run { + + public static final String ENABLE_DTD_PROPERTY_NAME = ExternalRun.class + ".supportDTD"; /** * Loads a run from a log file. * @param owner @@ -142,8 +144,9 @@ public Result run(BuildListener listener) throws Exception { PrintStream logger = new PrintStream(new DecodingStream(listener.getLogger())); XMLInputFactory xif = XMLInputFactory.newInstance(); - XMLStreamReader p = xif.createXMLStreamReader(in); + xif.setProperty(XMLInputFactory.SUPPORT_DTD, Boolean.parseBoolean(System.getProperty(ENABLE_DTD_PROPERTY_NAME))); + XMLStreamReader p = xif.createXMLStreamReader(in); p.nextTag(); // get to the p.nextTag(); // get to the @@ -172,7 +175,6 @@ else if(p.getLocalName().equals("description")) { } } } while (!(p.getEventType() == END_ELEMENT && p.getLocalName().equals("run"))); - return r; } diff --git a/src/test/java/hudson/model/Security3133Test.java b/src/test/java/hudson/model/Security3133Test.java new file mode 100644 index 0000000..b35d61c --- /dev/null +++ b/src/test/java/hudson/model/Security3133Test.java @@ -0,0 +1,96 @@ +package hudson.model; + +import com.github.tomakehurst.wiremock.client.WireMock; +import com.github.tomakehurst.wiremock.junit.WireMockRule; +import java.io.IOException; +import java.io.InputStream; +import java.net.URL; +import java.nio.charset.StandardCharsets; +import org.apache.commons.io.IOUtils; +import org.htmlunit.HttpMethod; +import org.htmlunit.WebRequest; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.FlagRule; +import org.jvnet.hudson.test.Issue; +import org.jvnet.hudson.test.JenkinsRule; + +import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; +import static com.github.tomakehurst.wiremock.client.WireMock.urlPathMatching; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.options; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.notNullValue; + +public class Security3133Test { + + @Rule + public JenkinsRule j = new JenkinsRule(); + @Rule + public WireMockRule wireMockRule = new WireMockRule(options().dynamicPort()); + @Rule + public FlagRule enableDtd = FlagRule.systemProperty(ExternalRun.ENABLE_DTD_PROPERTY_NAME); + @Issue("SECURITY-3133") + @Test + public void testExternalJobXXE() throws Throwable { + System.setProperty(ExternalRun.ENABLE_DTD_PROPERTY_NAME, "true"); + tryXXE(1,1); + } + + @Issue("SECURITY-3133") + @Test + public void testExternalJobXXEProtectedWithPropertyFalse() throws Throwable { + System.setProperty(ExternalRun.ENABLE_DTD_PROPERTY_NAME, "false"); + tryXXE(0,0); + } + + @Issue("SECURITY-3133") + @Test + public void testExternalJobXXEProtectedDefault() throws Throwable { + assertThat("Escape hatch property for SECURITY-3133 not null", System.getProperty(ExternalRun.ENABLE_DTD_PROPERTY_NAME) == null); + tryXXE(0,0); + } + + private void tryXXE(int dtdDownloads, int xxeTimes) throws Exception { + wireMockRule.stubFor(WireMock.get(urlPathMatching("/evil.dtd.*")) + .willReturn(aResponse() + .withBody(getDTDFile(wireMockRule.baseUrl())))); + wireMockRule.stubFor(WireMock.get(urlPathMatching("/file.*")) + .willReturn(aResponse())); + wireMockRule.start(); + + setUpJobAndMakeEvilRequest(j, wireMockRule.baseUrl()); + + wireMockRule.verify(dtdDownloads, WireMock.getRequestedFor(urlPathMatching("/evil.dtd.*"))); + wireMockRule.verify(xxeTimes, WireMock.getRequestedFor(urlEqualTo("/file?x=local"))); + wireMockRule.shutdown(); + } + private String getDTDFile(String base_url) throws Exception { + String toLoad = Security3133Test.class.getSimpleName() + "/evil.dtd"; + try (InputStream resource = Security3133Test.class.getResourceAsStream(toLoad)) { + assertThat("could not load resource " + toLoad, resource, notNullValue()); + return replaceLocalFile(IOUtils.toString(resource, StandardCharsets.UTF_8).replace("BASE_URL", base_url)); + } + } + private void setUpJobAndMakeEvilRequest(JenkinsRule j, String base_url) throws IOException { + j.jenkins.createProject(ExternalJob.class, "externalJob"); + String xml = (" %xxe;]>bar") + .replace("BASE_URL", base_url); + System.out.println(xml); + try (JenkinsRule.WebClient webClient = j.createWebClient().withThrowExceptionOnFailingStatusCode(false)) { + URL postURL = webClient.createCrumbedUrl("job/externalJob/postBuildResult"); + webClient.loadWebResponse(createRequest(postURL, xml)).getStatusCode(); + } + } + private WebRequest createRequest(URL URLtoCall, String xml) { + WebRequest postRequest = new WebRequest(URLtoCall, HttpMethod.POST); + + postRequest.setAdditionalHeader("Content-Type", "application/xml"); + postRequest.setRequestBody(xml); + return postRequest; + } + private String replaceLocalFile(String string) { + URL url = Security3133Test.class.getResource(Security3133Test.class.getSimpleName() + "/local.txt"); + return string.replaceAll("LOCAL_FILE", url.toString()); + } +} diff --git a/src/test/resources/hudson/model/Security3133Test/evil.dtd b/src/test/resources/hudson/model/Security3133Test/evil.dtd new file mode 100644 index 0000000..439707c --- /dev/null +++ b/src/test/resources/hudson/model/Security3133Test/evil.dtd @@ -0,0 +1,4 @@ + +"> +%eval; +%exfiltrate; \ No newline at end of file diff --git a/src/test/resources/hudson/model/Security3133Test/local.txt b/src/test/resources/hudson/model/Security3133Test/local.txt new file mode 100644 index 0000000..c2c027f --- /dev/null +++ b/src/test/resources/hudson/model/Security3133Test/local.txt @@ -0,0 +1 @@ +local \ No newline at end of file