[SECURITY-2458]

core/src/main/java/jenkins/security/s2m/CallableDirectionChecker.java

@@ -33,6 +33,8 @@ public class CallableDirectionChecker extends RoleChecker {
 
     private static final String BYPASS_PROP = CallableDirectionChecker.class.getName()+".allow";
 
+    private static final String ALLOW_ANY_ROLE_PROP = CallableDirectionChecker.class.getName()+".allowAnyRole";
+
     /**
      * Switch to disable all the defense mechanism completely.
      *
@@ -55,6 +57,12 @@ public void check(RoleSensitive subject, @NonNull Collection<Role> expected) thr
             return;    // known to be safe
         }
 
+        if (expected.isEmpty() && SystemProperties.getBoolean(ALLOW_ANY_ROLE_PROP, true)) {
+            // TODO Is this even something we want to support, or should all infrastructure callables be exempted from the required role check?
+            LOGGER.log(Level.FINE, "Executing {0} is allowed since it is targeted for any role", name);
+            return;
+        }
+
         if (isWhitelisted(subject,expected)) {
             // this subject is dubious, but we are letting it through as per whitelisting
             LOGGER.log(Level.FINE, "Explicitly allowing {0} to be sent from agent to controller", name);

pom.xml

@@ -91,7 +91,7 @@ THE SOFTWARE.
     <changelog.url>https://www.jenkins.io/changelog</changelog.url>
 
     <!-- Bundled Remoting version -->
-    <remoting.version>4.11</remoting.version>
+    <remoting.version>4.11.1</remoting.version>
     <!-- Minimum Remoting version, which is tested for API compatibility -->
     <remoting.minimum.supported.version>3.14</remoting.minimum.supported.version>
 

test/src/test/java/jenkins/security/Security2458Test.java

@@ -0,0 +1,116 @@
+package jenkins.security;
+
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import hudson.ExtensionList;
+import hudson.remoting.Callable;
+import java.io.IOException;
+import java.util.Objects;
+import jenkins.agents.AgentComputerUtil;
+import jenkins.security.s2m.AdminWhitelistRule;
+import jenkins.security.s2m.CallableDirectionChecker;
+import org.jenkinsci.remoting.RoleChecker;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.function.ThrowingRunnable;
+import org.jvnet.hudson.test.JenkinsRule;
+
+public class Security2458Test {
+    @Rule
+    public JenkinsRule r = new JenkinsRule();
+
+    @Before
+    public void enableAgentToControllerProtections() {
+        AdminWhitelistRule rule = ExtensionList.lookupSingleton(AdminWhitelistRule.class);
+        rule.setMasterKillSwitch(false);
+    }
+
+    @Test
+    public void rejectBadCallable() throws Throwable {
+        // If the role check is empty, fail
+        assertThrowsIOExceptionCausedBySecurityException(() -> Objects.requireNonNull(r.createOnlineSlave().getChannel()).call(new CallableCaller(new BadCallable())));
+
+        // If it performs a no-op check, pass. Never do this in your plugin.
+        Objects.requireNonNull(r.createOnlineSlave().getChannel()).call(new CallableCaller(new EvilCallable()));
+
+        // Explicit role check.
+        Objects.requireNonNull(r.createOnlineSlave().getChannel()).call(new CallableCaller(new GoodCallable()));
+
+        // No-op role checks can be disallowed
+        System.setProperty(CallableDirectionChecker.class.getName() + ".allowAnyRole", "false");
+        try {
+            assertThrowsIOExceptionCausedBySecurityException(() -> Objects.requireNonNull(r.createOnlineSlave().getChannel()).call(new CallableCaller(new EvilCallable())));
+        } finally {
+            System.clearProperty(CallableDirectionChecker.class.getName() + ".allowAnyRole");
+        }
+    }
+
+    private static class CallableCaller extends MasterToSlaveCallable<Object, Throwable> {
+        private final Callable<?, ?> callable;
+
+        CallableCaller(Callable<?, ?> callable) {
+            this.callable = callable;
+        }
+
+        @Override
+        public Object call() throws Throwable {
+            Objects.requireNonNull(AgentComputerUtil.getChannelToMaster()).call(callable);
+            return null;
+        }
+    }
+
+    private static class BadCallable implements Callable<Object, Exception> {
+        @Override
+        public Object call() throws Exception {
+            return null;
+        }
+        @Override
+        public void checkRoles(RoleChecker checker) throws SecurityException {
+            // Deliberately empty
+        }
+    }
+
+    private static class EvilCallable implements Callable<Object, Exception> {
+        @Override
+        public Object call() throws Exception {
+            return null;
+        }
+        @Override
+        public void checkRoles(RoleChecker checker) throws SecurityException {
+            checker.check(this); // Never do this
+        }
+    }
+
+    private static class GoodCallable implements Callable<Object, Exception> {
+        @Override
+        public Object call() throws Exception {
+            return null;
+        }
+        @Override
+        public void checkRoles(RoleChecker checker) throws SecurityException {
+            checker.check(this, Roles.MASTER); // Manual S2M
+        }
+    }
+
+
+    private static SecurityException assertThrowsIOExceptionCausedBySecurityException(ThrowingRunnable runnable) {
+        return assertThrowsIOExceptionCausedBy(SecurityException.class, runnable);
+    }
+
+    private static <X extends Throwable> X assertThrowsIOExceptionCausedBy(Class<X> causeClass, ThrowingRunnable runnable) {
+        try {
+            runnable.run();
+        } catch (IOException ex) {
+            final Throwable cause = ex.getCause();
+            assertTrue("IOException with message: '" + ex.getMessage() + "' wasn't caused by " + causeClass + ": " + (cause == null ? "(null)" : (cause.getClass().getName() + ": " + cause.getMessage())),
+                    cause != null && causeClass.isAssignableFrom(cause.getClass()));
+            return causeClass.cast(cause);
+        } catch (Throwable t) {
+            fail("Threw other Throwable: " + t.getClass() + " with message " + t.getMessage());
+        }
+        fail("Expected exception but passed");
+        return null;
+    }
+}