diff --git a/core/src/main/java/jenkins/security/s2m/CallableDirectionChecker.java b/core/src/main/java/jenkins/security/s2m/CallableDirectionChecker.java index a3206e7fc859..3c883dbd71c9 100644 --- a/core/src/main/java/jenkins/security/s2m/CallableDirectionChecker.java +++ b/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 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); diff --git a/pom.xml b/pom.xml index 4336e05f5b60..fa5d51c09fad 100644 --- a/pom.xml +++ b/pom.xml @@ -91,7 +91,7 @@ THE SOFTWARE. https://www.jenkins.io/changelog - 4.11 + 4.11.1 3.14 diff --git a/test/src/test/java/jenkins/security/Security2458Test.java b/test/src/test/java/jenkins/security/Security2458Test.java new file mode 100644 index 000000000000..795742aa1c31 --- /dev/null +++ b/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 { + 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 { + @Override + public Object call() throws Exception { + return null; + } + @Override + public void checkRoles(RoleChecker checker) throws SecurityException { + // Deliberately empty + } + } + + private static class EvilCallable implements Callable { + @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 { + @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 assertThrowsIOExceptionCausedBy(Class 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; + } +}