SECURITY-3289

jenkinsci · Jan 10, 2024 · 14451bc · 14451bc
1 parent 530d756
commit 14451bc
Show file tree

Hide file tree

Showing 3 changed files with 103 additions and 10 deletions.
diff --git 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
@@ -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<DescriptorImpl> 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
@@ -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);