Path traversal vulnerability in Jenkins agent names

@@ -35,6 +35,7 @@
 import hudson.slaves.OfflineCause;
 import java.util.concurrent.Callable;
  
+import jenkins.util.SystemProperties;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
  
@@ -64,6 +65,13 @@
 @Restricted(NoExternalUse.class) // for now, we may make it public later
 public class Nodes implements Saveable {
  
+    /**
+     * Determine if we need to enforce the name restrictions during node creation or replacement.
+     * Should be enabled (default) to prevent SECURITY-2021.
+     */
+    @Restricted(NoExternalUse.class)
+    private static final boolean ENFORCE_NAME_RESTRICTIONS = SystemProperties.getBoolean(Nodes.class.getName() + ".enforceNameRestrictions", true);
+ 
     /**
      * The {@link Jenkins} instance that we are tracking nodes for.
      */
@@ -127,6 +135,10 @@ public void run() {
      * @throws IOException if the list of nodes could not be persisted.
      */
     public void addNode(final @NonNull Node node) throws IOException {
+        if (ENFORCE_NAME_RESTRICTIONS) {
+            Jenkins.checkGoodName(node.getNodeName());
+        }
+ 
         Node oldNode = nodes.get(node.getNodeName());
         if (node != oldNode) {
             // TODO we should not need to lock the queue for adding nodes but until we have a way to update the
@@ -228,6 +240,10 @@ public Boolean call() throws Exception {
      * @since 2.8
      */
     public boolean replaceNode(final Node oldOne, final @NonNull Node newOne) throws IOException {
+        if (ENFORCE_NAME_RESTRICTIONS) {
+            Jenkins.checkGoodName(newOne.getNodeName());
+        }
+ 
         if (oldOne == nodes.get(oldOne.getNodeName())) {
             // use the queue lock until Nodes has a way of directly modifying a single node.
             Queue.withLock(new Runnable() {

@@ -0,0 +1,72 @@
+/*
+ * The MIT License
+ *
+ * Copyright 2013 Red Hat, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+ 
+package hudson.cli;
+ 
+import hudson.model.Computer;
+import hudson.model.Messages;
+import jenkins.model.Jenkins;
+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 static hudson.cli.CLICommandInvoker.Matcher.failedWith;
+import static hudson.cli.CLICommandInvoker.Matcher.hasNoStandardOutput;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.junit.Assert.assertEquals;
+ 
+//TODO to be merged into base test after security release
+public class CreateNodeCommandSEC2021Test {
+ 
+    private CLICommandInvoker command;
+ 
+    @Rule public final JenkinsRule j = new JenkinsRule();
+ 
+    @Before public void setUp() {
+ 
+        command = new CLICommandInvoker(j, new CreateNodeCommand());
+    }
+ 
+    @Test 
+    @Issue("SECURITY-2021")
+    public void createNodeShouldFailIfNodeIsNotGood() throws Exception {
+        int nodeListSizeBefore = j.jenkins.getNodes().size();
+        
+        final CLICommandInvoker.Result result = command
+                .authorizedTo(Computer.CREATE, Jenkins.READ)
+                .withStdin(CreateNodeCommandSEC2021Test.class.getResourceAsStream("node_sec2021.xml"))
+                .invoke()
+        ;
+ 
+        assertThat(result.stderr(), containsString(Messages.Hudson_UnsafeChar('/')));
+        assertThat(result, hasNoStandardOutput());
+        assertThat(result, failedWith(1));
+ 
+        // ensure not side effects
+        assertEquals(nodeListSizeBefore, j.jenkins.getNodes().size());
+    }
+}

@@ -0,0 +1,77 @@
+/*
+ * The MIT License
+ *
+ * Copyright 2013 Red Hat, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+ 
+package hudson.cli;
+ 
+import hudson.model.Computer;
+import hudson.model.Messages;
+import hudson.model.Slave;
+import jenkins.model.Jenkins;
+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 static hudson.cli.CLICommandInvoker.Matcher.failedWith;
+import static hudson.cli.CLICommandInvoker.Matcher.hasNoStandardOutput;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.junit.Assert.assertEquals;
+ 
+//TODO to be merged into base test after security release
+public class UpdateNodeCommandSEC2021Test {
+ 
+    private CLICommandInvoker command;
+ 
+    @Rule public final JenkinsRule j = new JenkinsRule();
+ 
+    @Before public void setUp() {
+ 
+        command = new CLICommandInvoker(j, new UpdateNodeCommand());
+    }
+ 
+    @Test
+    @Issue("SECURITY-2021")
+    public void updateNodeShouldFailForDotDot() throws Exception {
+        String okName = "MyNode";
+        Slave node = j.createSlave(okName, null, null);
+        // currently <dummy>, but doing so will be a bit more future-proof
+        String defaultDescription = node.getNodeDescription();
+ 
+        final CLICommandInvoker.Result result = command
+                .authorizedTo(Computer.CONFIGURE, Jenkins.READ)
+                .withStdin(UpdateNodeCommandSEC2021Test.class.getResourceAsStream("node_sec2021.xml"))
+                .invokeWithArgs(okName)
+                ;
+ 
+        assertThat(result.stderr(), containsString(Messages.Hudson_UnsafeChar('/')));
+        assertThat(result, hasNoStandardOutput());
+        assertThat(result, failedWith(1));
+ 
+        assertEquals(okName, node.getNodeName());
+        // ensure the other data were not saved
+        assertEquals(defaultDescription, node.getNodeDescription());
+    }
+}

@@ -0,0 +1,52 @@
+package hudson.model;
+ 
+import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
+import com.gargoylesoftware.htmlunit.HttpMethod;
+import com.gargoylesoftware.htmlunit.WebRequest;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+ 
+import java.io.File;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+ 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.not;
+import static org.junit.Assert.fail;
+ 
+public class ComputerConfigDotXmlSEC2021Test {
+ 
+    @Rule public final JenkinsRule rule = new JenkinsRule();
+ 
+    @Issue("SECURITY-2021")
+    @Test
+    public void nodeNameReferencesParentDir() throws Exception {
+        Computer computer = rule.createSlave("anything", null).toComputer();
+ 
+        JenkinsRule.WebClient wc = rule.createWebClient();
+        WebRequest req = new WebRequest(wc.createCrumbedUrl(String.format("%s/config.xml", computer.getUrl())), HttpMethod.POST);
+        req.setAdditionalHeader("Content-Type", "application/xml");
+        req.setRequestBody(VALID_XML_BAD_NAME_XML);
+ 
+        try {
+            wc.getPage(req);
+            fail("Should have returned failure.");
+        } catch (FailingHttpStatusCodeException e) {
+            assertThat(e.getStatusCode(), equalTo(400));
+        }
+        File configDotXml = new File(rule.jenkins.getRootDir(), "config.xml");
+        String configDotXmlContents = new String(Files.readAllBytes(configDotXml.toPath()), StandardCharsets.UTF_8);
+ 
+        assertThat(configDotXmlContents, not(containsString("<name>../</name>")));
+    }
+ 
+    private static final String VALID_XML_BAD_NAME_XML =
+            "<slave>\n" +
+                    "  <name>../</name>\n" +
+                    "</slave>";
+ 
+}

@@ -195,7 +195,7 @@ public void laxParsing() {
     @Test
     public void dataCompatibilityWithHostNameWithWhitespace() throws Exception {
         assumeFalse("Windows can't have paths with colons, skipping", Functions.isWindows());
-        DumbSlave slave = new DumbSlave("abc def (xyz) : test", "dummy",
+        DumbSlave slave = new DumbSlave("abc def (xyz) test", "dummy",
                 j.createTmpDir().getPath(), "1", Mode.NORMAL, "", j.createComputerLauncher(null), RetentionStrategy.NOOP, Collections.EMPTY_LIST);
         j.jenkins.addNode(slave);
  

@@ -0,0 +1,17 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<slave>
+  <name>../</name>
+  <description>Payload from XML</description>
+  <remoteFS>/user/hudson/workspace</remoteFS>
+  <numExecutors>42</numExecutors>
+  <mode>NORMAL</mode>
+  <retentionStrategy class="hudson.slaves.RetentionStrategy$1">
+    <DESCRIPTOR>
+      <outer-class reference="../.."/>
+    </DESCRIPTOR>
+  </retentionStrategy>
+  <launcher class="hudson.slaves.JNLPLauncher"/>
+  <label></label>
+  <nodeProperties/>
+  <userId>SYSTEM</userId>
+</slave>