Inbound TCP Agent Protocol/3 authentication bypass in Jenkins

@@ -1,17 +1,17 @@
 package jenkins.slaves;
  
-import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import hudson.Extension;
+import hudson.ExtensionComponent;
 import hudson.ExtensionList;
-import hudson.Util;
 import hudson.model.Computer;
 import java.io.IOException;
 import java.net.Socket;
 import java.util.Collections;
+import java.util.logging.Logger;
 import javax.inject.Inject;
+ 
 import jenkins.AgentProtocol;
-import jenkins.model.Jenkins;
-import jenkins.util.SystemProperties;
+import jenkins.ExtensionFilter;
 import org.jenkinsci.Symbol;
 import org.jenkinsci.remoting.engine.JnlpConnectionState;
 import org.jenkinsci.remoting.engine.JnlpProtocol3Handler;
@@ -29,6 +29,10 @@
 @Extension
 @Symbol("jnlp3")
 public class JnlpSlaveAgentProtocol3 extends AgentProtocol {
+ 
+    private static final Logger logger = Logger.getLogger(JnlpSlaveAgentProtocol3.class.getName());
+    @Restricted(NoExternalUse.class)
+    public static final String ALLOW_UNSAFE = JnlpSlaveAgentProtocol3.class.getName() + ".ALLOW_UNSAFE";
     private NioChannelSelector hub;
  
     private JnlpProtocol3Handler handler;
@@ -73,4 +77,23 @@ public void handle(Socket socket) throws IOException, InterruptedException {
                 ExtensionList.lookup(JnlpAgentReceiver.class));
     }
  
+    @Extension
+    @Restricted(NoExternalUse.class)
+    public static class Impl extends ExtensionFilter {
+        @Override
+        public <T> boolean allows(Class<T> type, ExtensionComponent<T> component) {
+            if (Boolean.getBoolean(ALLOW_UNSAFE)) {
+                return true;
+            }
+            if (!AgentProtocol.class.isAssignableFrom(type)) {
+                return true;
+            }
+            boolean isJnlp3 = component.getInstance().getClass().isAssignableFrom(JnlpSlaveAgentProtocol3.class);
+            if (isJnlp3) {
+                logger.info("Inbound TCP Agent Protocol/3 has been forcibly disabled for additional security reasons. To enable it yet again set the system property " + ALLOW_UNSAFE);
+            }
+            return !isJnlp3;
+        }
+    }
+ 
 }

@@ -0,0 +1,135 @@
+/*
+ * The MIT License
+ *
+ * Copyright (c) 2020 CloudBees, 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 jenkins;
+ 
+import jenkins.model.Jenkins;
+import jenkins.slaves.DeprecatedAgentProtocolMonitor;
+import jenkins.slaves.JnlpSlaveAgentProtocol3;
+import org.apache.commons.lang.StringUtils;
+import org.junit.BeforeClass;
+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.recipes.LocalData;
+ 
+import javax.annotation.CheckForNull;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+ 
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.nullValue;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+ 
+/**
+ * Tests for {@link AgentProtocol}.
+ * 
+ * @author Oleg Nenashev
+ */
+public class AgentProtocolSecurity1682Test {
+    
+    @Rule
+    public JenkinsRule j = new JenkinsRule();
+ 
+    @BeforeClass
+    public static void setupClass() {
+        System.setProperty(JnlpSlaveAgentProtocol3.ALLOW_UNSAFE, "true");
+    }
+ 
+    @Test
+    @LocalData
+    @Issue("SECURITY-1682")
+    public void testShouldReallyEnable3() {
+        assertEnabled("JNLP-connect", "JNLP3-connect");
+        assertDisabled("JNLP2-connect", "JNLP4-connect");
+        assertProtocols(true, "System protocols should be always enabled", "Ping");
+        assertMonitorTriggered("JNLP-connect", "JNLP3-connect");
+    }
+ 
+    private void assertEnabled(String ... protocolNames) throws AssertionError {
+        assertProtocols(true, null, protocolNames);    
+    }
+    
+    private void assertDisabled(String ... protocolNames) throws AssertionError {
+        assertProtocols(false, null, protocolNames);    
+    }
+    
+    private void assertProtocols(boolean shouldBeEnabled, @CheckForNull String why, String ... protocolNames) {
+        assertProtocols(j.jenkins, shouldBeEnabled, why, protocolNames);
+    }
+    
+    public static void assertProtocols(Jenkins jenkins, boolean shouldBeEnabled, @CheckForNull String why, String ... protocolNames) 
+            throws AssertionError {
+        Set<String> agentProtocols = jenkins.getAgentProtocols();
+        List<String> failedChecks = new ArrayList<>();
+        for (String protocol : protocolNames) {
+            if (shouldBeEnabled && !(agentProtocols.contains(protocol))) {
+                failedChecks.add(protocol);
+            }
+            if (!shouldBeEnabled && agentProtocols.contains(protocol)) {
+                failedChecks.add(protocol);
+            }
+        }
+        
+        if (!failedChecks.isEmpty()) {
+            String message = String.format("Protocol(s) are not %s: %s. %sEnabled protocols: %s",
+                    shouldBeEnabled ? "enabled" : "disabled",
+                    StringUtils.join(failedChecks, ','),
+                    why != null ? "Reason: " + why + ". " : "",
+                    StringUtils.join(agentProtocols, ','));
+            fail(message);
+        }
+    }
+    
+    public static void assertMonitorNotActive(JenkinsRule j) {
+        DeprecatedAgentProtocolMonitor monitor = new DeprecatedAgentProtocolMonitor();
+        assertFalse("Deprecated Agent Protocol Monitor should not be activated. Current protocols: "
+                + StringUtils.join(j.jenkins.getAgentProtocols(), ","), monitor.isActivated());
+    }
+    
+    public static void assertMonitorTriggered(String ... expectedProtocols) {
+        DeprecatedAgentProtocolMonitor monitor = new DeprecatedAgentProtocolMonitor();
+        assertTrue("Deprecated Agent Protocol Monitor should be activated", monitor.isActivated());
+        String protocolList = monitor.getDeprecatedProtocols();
+        assertThat("List of the protocols should not be null", protocolList, not(nullValue()));
+        
+        List<String> failedChecks = new ArrayList<>();
+        for(String protocol : expectedProtocols) {
+            if (!protocolList.contains(protocol)) {
+                failedChecks.add(protocol);
+            }
+        }
+        
+        if (!failedChecks.isEmpty()) {
+            String message = String.format(
+                    "Protocol(s) should in the deprecated protocol list: %s. Current list: %s",
+                    StringUtils.join(expectedProtocols, ','), protocolList);
+            fail(message);
+        }
+    }
+}

@@ -77,24 +77,15 @@ public void testShouldNotDisableProtocolsForMigratedInstances() throws Exception
         assertProtocols(false, "JNLP3-connect protocol should be disabled by default", "JNLP3-connect");
         assertMonitorTriggered("JNLP-connect", "JNLP2-connect", "CLI-connect");
     }
-    
+ 
     @Test
     @LocalData
     @Issue("JENKINS-45841")
     public void testShouldNotOverrideUserConfiguration() throws Exception {
-        assertEnabled("CLI-connect", "JNLP-connect", "JNLP3-connect");
-        assertDisabled("CLI2-connect", "JNLP2-connect", "JNLP4-connect");
+        assertEnabled("JNLP-connect");
+        assertDisabled("JNLP2-connect", "JNLP4-connect");
         assertProtocols(true, "System protocols should be always enabled", "Ping");
-        assertMonitorTriggered("JNLP-connect", "JNLP3-connect", "CLI-connect");
-    }
-    
-    @Test
-    @LocalData
-    public void testShouldDisableCLIProtocolsWhenCLIisDisabled() throws Exception {
-        assertProtocols(false, "CLI is forcefully disabled, protocols should be blocked", 
-                "CLI-connect", "CLI2-connect");
-        assertEnabled("JNLP3-connect");
-        assertMonitorTriggered("JNLP3-connect");
+        assertMonitorTriggered("JNLP-connect");
     }
     
     private void assertEnabled(String ... protocolNames) throws AssertionError {

@@ -0,0 +1,47 @@
+<?xml version='1.0' encoding='UTF-8'?>
+<hudson>
+  <disabledAdministrativeMonitors/>
+  <version>1.0</version>
+  <numExecutors>2</numExecutors>
+  <mode>NORMAL</mode>
+  <useSecurity>true</useSecurity>
+  <authorizationStrategy class="hudson.security.AuthorizationStrategy$Unsecured"/>
+  <securityRealm class="hudson.security.SecurityRealm$None"/>
+  <disableRememberMe>false</disableRememberMe>
+  <projectNamingStrategy class="jenkins.model.ProjectNamingStrategy$DefaultProjectNamingStrategy"/>
+  <workspaceDir>${ITEM_ROOTDIR}/workspace</workspaceDir>
+  <buildsDir>${ITEM_ROOTDIR}/builds</buildsDir>
+  <markupFormatter class="hudson.markup.EscapedMarkupFormatter"/>
+  <jdks/>
+  <viewsTabBar class="hudson.views.DefaultViewsTabBar"/>
+  <myViewsTabBar class="hudson.views.DefaultMyViewsTabBar"/>
+  <clouds/>
+  <scmCheckoutRetryCount>0</scmCheckoutRetryCount>
+  <views>
+    <hudson.model.AllView>
+      <owner class="hudson" reference="../../.."/>
+      <name>all</name>
+      <filterExecutors>false</filterExecutors>
+      <filterQueue>false</filterQueue>
+      <properties class="hudson.model.View$PropertyList"/>
+    </hudson.model.AllView>
+  </views>
+  <primaryView>all</primaryView>
+  <slaveAgentPort>0</slaveAgentPort>
+  <!-- YOLO: unstable configuration -->
+  <enabledAgentProtocols>
+    <!-- Since removed, but may as well test robustness of load: -->
+    <string>CLI-connect</string>
+    <string>JNLP-connect</string>
+    <string>JNLP3-connect</string>
+  </enabledAgentProtocols>
+  <disabledAgentProtocols>
+    <string>JNLP4-connect</string>
+    <string>JNLP2-connect</string>
+    <!-- Ditto: -->
+    <string>CLI2-connect</string>
+  </disabledAgentProtocols>
+  <label></label>
+  <nodeProperties/>
+  <globalNodeProperties/>
+</hudson>