From 14451bc0fd4240fed322178655bb3361cc6d4296 Mon Sep 17 00:00:00 2001 From: Robert Sandell Date: Wed, 10 Jan 2024 10:00:28 +0100 Subject: [PATCH] SECURITY-3289 --- .../java/hudson/matrix/MatrixProject.java | 42 +++++++++++++ src/main/java/hudson/matrix/TextAxis.java | 10 +++ src/test/java/hudson/matrix/AxisTest.java | 61 ++++++++++++++++--- 3 files changed, 103 insertions(+), 10 deletions(-) diff --git a/src/main/java/hudson/matrix/MatrixProject.java b/src/main/java/hudson/matrix/MatrixProject.java index 7a89252f..cf02328e 100644 --- a/src/main/java/hudson/matrix/MatrixProject.java +++ b/src/main/java/hudson/matrix/MatrixProject.java @@ -64,14 +64,18 @@ import hudson.tasks.test.AggregatedTestResultAction; import hudson.triggers.Trigger; import hudson.util.AlternativeUiTextProvider; +import hudson.util.AtomicFileWriter; import hudson.util.CopyOnWriteMap; import hudson.util.DescribableList; import hudson.util.FormValidation; import hudson.util.FormValidation.Kind; +import java.io.BufferedReader; import java.io.File; import java.io.FileFilter; +import java.io.FileWriter; import java.io.IOException; +import java.nio.file.Files; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -91,9 +95,15 @@ import edu.umd.cs.findbugs.annotations.Nullable; import javax.servlet.ServletException; +import javax.servlet.http.HttpServletResponse; +import javax.xml.transform.Source; +import javax.xml.transform.TransformerException; +import javax.xml.transform.stream.StreamResult; +import javax.xml.transform.stream.StreamSource; import jenkins.model.Jenkins; import jenkins.scm.SCMCheckoutStrategyDescriptor; +import jenkins.util.xml.XMLUtils; import net.sf.json.JSONObject; import org.kohsuke.accmod.Restricted; @@ -103,6 +113,7 @@ import org.kohsuke.stapler.StaplerResponse; import org.kohsuke.stapler.TokenList; import org.kohsuke.stapler.export.Exported; +import org.xml.sax.SAXException; /** * {@link Job} that allows you to run multiple different configurations @@ -958,6 +969,37 @@ protected void submit(StaplerRequest req, StaplerResponse rsp) throws IOExceptio rebuildConfigurations(null); } + @Override + public void doConfigDotXml(StaplerRequest req, StaplerResponse rsp) throws IOException { + if (req.getMethod().equals("POST")) { + checkPermission(CONFIGURE); + File test = Files.createTempFile("test", "matrix-project.xml").toFile(); + BufferedReader reader = req.getReader(); + reader.mark(10000000); //10MB shouldn't blow up the heap right? + try(FileWriter out = new FileWriter(test)) { + try { + XMLUtils.safeTransform(new StreamSource(reader), new StreamResult(out)); + } catch (TransformerException | SAXException e) { + throw new IOException("Failed to persist config.xml", e); + } + // try to reflect the changes by reloading + Object o = new XmlFile(Items.XSTREAM, test).unmarshalNullingOut(null); + if (!(o instanceof MatrixProject)) { + throw FormValidation.error("Expecting " + this.getClass() + " but got " + (o != null ? o.getClass() : "null") + " instead"); + } + try { + checkAxes(((MatrixProject) o).getAxes()); + } catch (FormException e) { + throw FormValidation.error(e.getMessage()); + } + } finally { + test.delete(); // don't leave anything behind + reader.reset(); + } + } + super.doConfigDotXml(req, rsp); + } + public AggregatedTestResultAction getAggregatedTestResultAction() { MatrixBuild b = getLastCompletedBuild(); return b != null ? b.getAction(AggregatedTestResultAction.class) : null; diff --git a/src/main/java/hudson/matrix/TextAxis.java b/src/main/java/hudson/matrix/TextAxis.java index 6b59d5bb..9f7fd77a 100644 --- a/src/main/java/hudson/matrix/TextAxis.java +++ b/src/main/java/hudson/matrix/TextAxis.java @@ -1,6 +1,7 @@ package hudson.matrix; import hudson.Extension; +import hudson.ExtensionList; import hudson.util.FormValidation; import org.kohsuke.accmod.Restricted; @@ -29,6 +30,15 @@ public TextAxis(String name, String valueString) { super(name, valueString); } + @Override + public AxisDescriptor getDescriptor() { + ExtensionList lookup = ExtensionList.lookup(DescriptorImpl.class); + if (lookup.isEmpty()) { + return new DescriptorImpl(); + } + return lookup.get(0); + } + @Extension public static class DescriptorImpl extends AxisDescriptor { @Override diff --git a/src/test/java/hudson/matrix/AxisTest.java b/src/test/java/hudson/matrix/AxisTest.java index a138f33e..8400eabc 100644 --- a/src/test/java/hudson/matrix/AxisTest.java +++ b/src/test/java/hudson/matrix/AxisTest.java @@ -23,27 +23,36 @@ */ package hudson.matrix; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.equalTo; -import static org.junit.Assert.assertEquals; -import static org.junit.Assume.assumeTrue; - +import edu.umd.cs.findbugs.annotations.NonNull; +import hudson.Util; import hudson.model.JDK; import hudson.util.VersionNumber; import java.util.List; import jenkins.model.Jenkins; - import org.hamcrest.collection.IsEmptyCollection; +import org.htmlunit.HttpMethod; +import org.htmlunit.WebRequest; +import org.htmlunit.WebResponse; +import org.htmlunit.html.HtmlForm; +import org.htmlunit.html.HtmlInput; +import org.htmlunit.html.HtmlPage; +import org.htmlunit.xml.XmlPage; import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.JenkinsRule.WebClient; +import org.xml.sax.SAXException; -import org.htmlunit.html.HtmlForm; -import org.htmlunit.html.HtmlInput; -import org.htmlunit.html.HtmlPage; +import java.io.IOException; +import java.net.URL; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.junit.Assert.assertEquals; +import static org.junit.Assume.assumeTrue; public class AxisTest { @@ -131,6 +140,38 @@ public void emptyAxisValueListResultInNoConfigurations() throws Exception { } } + @Test @Issue("SECURITY-3289") + public void testHaxorNameFromConfigXml() throws IOException, SAXException { + p.getAxes().add(new TextAxis("CHANGEME", p.getName())); + p.save(); + XmlPage xmlPage = wc.goToXml(p.getUrl() + "config.xml"); + String xml = xmlPage.asXml(); + String goodxml = xml.replaceFirst("\\s*CHANGEME\\s*", "a").replaceFirst("\\s+" + p.getName() + "\\s+", p.getName()); + String badxml = xml.replaceFirst("\\s*CHANGEME\\s*", "a/../../../").replaceFirst("\\s+" + p.getName() + "\\s+", p.getName()); + System.out.println("Good XML:\n" + goodxml); + WebResponse response = postXML(p.getUrl() + "config.xml", goodxml); + assertEquals(200, response.getStatusCode()); + + System.out.println("Bad XML:\n" + badxml); + response = postXML(p.getUrl() + "config.xml", badxml); + assertEquals(200, response.getStatusCode()); //FormValidation wants to render stuff so it sends a 200 + assertThat(response.getContentAsString(), containsString(Util.escape("Matrix axis name 'a/../../../' is invalid: ‘/’ is an unsafe character"))); + } + + public WebResponse postXML(@NonNull String path, @NonNull String xml) throws IOException { + assert !path.startsWith("/"); + + URL URLtoCall = wc.createCrumbedUrl(path); + WebRequest postRequest = new WebRequest(URLtoCall, HttpMethod.POST); + + postRequest.setAdditionalHeader("Content-Type", "application/xml"); + postRequest.setAdditionalHeader("Accept", "application/xml"); + postRequest.setAdditionalHeader("Accept-Encoding", "*"); + + postRequest.setRequestBody(xml); + return wc.loadWebResponse(postRequest); + } + private HtmlPage withName(String value, String axis) throws Exception { HtmlForm form = addAxis(axis); form.getInputByName("_.name").setValue(value);