From cec49ce5d58048f66ac3fa88409a0d38dec09bf0 Mon Sep 17 00:00:00 2001 From: Daniel Beck Date: Mon, 22 Jul 2024 21:07:11 +0000 Subject: [PATCH] [SECURITY-3430] --- core/src/main/java/hudson/PluginManager.java | 16 + .../security/s2m/JarURLValidatorImpl.java | 104 ++++++ pom.xml | 2 +- test/pom.xml | 8 + .../jenkins/security/Security3430Test.java | 296 ++++++++++++++++++ 5 files changed, 425 insertions(+), 1 deletion(-) create mode 100644 core/src/main/java/jenkins/security/s2m/JarURLValidatorImpl.java create mode 100644 test/src/test/java/jenkins/security/Security3430Test.java diff --git a/core/src/main/java/hudson/PluginManager.java b/core/src/main/java/hudson/PluginManager.java index 5d80c6e623e4..adc159a47085 100644 --- a/core/src/main/java/hudson/PluginManager.java +++ b/core/src/main/java/hudson/PluginManager.java @@ -2413,6 +2413,22 @@ public String toString() { // only for debugging purpose return "classLoader " + getClass().getName(); } + + // TODO Remove this once we require post 2024-07 remoting minimum version and deleted ClassLoaderProxy#fetchJar(URL) + @SuppressFBWarnings( + value = "DMI_COLLECTION_OF_URLS", + justification = "All URLs point to local files, so no DNS lookup.") + @Restricted(NoExternalUse.class) + public boolean isPluginJar(URL jarUrl) { + for (PluginWrapper plugin : activePlugins) { + if (plugin.classLoader instanceof URLClassLoader) { + if (Set.of(((URLClassLoader) plugin.classLoader).getURLs()).contains(jarUrl)) { + return true; + } + } + } + return false; + } } @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "for script console") diff --git a/core/src/main/java/jenkins/security/s2m/JarURLValidatorImpl.java b/core/src/main/java/jenkins/security/s2m/JarURLValidatorImpl.java new file mode 100644 index 000000000000..ca8365a8a66f --- /dev/null +++ b/core/src/main/java/jenkins/security/s2m/JarURLValidatorImpl.java @@ -0,0 +1,104 @@ +/* + * The MIT License + * + * Copyright (c) 2024 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.security.s2m; + +import edu.umd.cs.findbugs.annotations.Nullable; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; +import hudson.Extension; +import hudson.PluginManager; +import hudson.remoting.Channel; +import hudson.remoting.ChannelBuilder; +import hudson.remoting.JarURLValidator; +import java.io.IOException; +import java.net.URL; +import java.net.URLClassLoader; +import java.util.Set; +import java.util.logging.Level; +import java.util.logging.Logger; +import jenkins.model.Jenkins; +import jenkins.security.ChannelConfigurator; +import jenkins.util.SystemProperties; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; + +@Restricted(NoExternalUse.class) +@Deprecated +@Extension +public class JarURLValidatorImpl extends ChannelConfigurator implements JarURLValidator { + + public static final Logger LOGGER = Logger.getLogger(JarURLValidatorImpl.class.getName()); + + @Override + public void onChannelBuilding(ChannelBuilder builder, @Nullable Object context) { + LOGGER.log(Level.CONFIG, () -> "Setting up JarURLValidatorImpl for context: " + context); + builder.withProperty(JarURLValidator.class, this); + } + + @Override + public void validate(URL url) throws IOException { + final String rejectAllProp = JarURLValidatorImpl.class.getName() + ".REJECT_ALL"; + if (SystemProperties.getBoolean(rejectAllProp)) { + LOGGER.log(Level.FINE, () -> "Rejecting URL due to configuration: " + url); + throw new IOException("The system property '" + rejectAllProp + "' has been set, so all attempts by agents to load jars from the controller are rejected." + + " Update the agent.jar of the affected agent to a version released in August 2024 or later to prevent this error."); // TODO better version spec + } + final String allowAllProp = Channel.class.getName() + ".DISABLE_JAR_URL_VALIDATOR"; + if (SystemProperties.getBoolean(allowAllProp)) { + LOGGER.log(Level.FINE, () -> "Allowing URL due to configuration: " + url); + return; + } + if (!isAllowedJar(url)) { + LOGGER.log(Level.FINE, () -> "Rejecting URL: " + url); + throw new IOException("This URL does not point to a jar file allowed to be requested by agents: " + url + "." + + " Update the agent.jar of the affected agent to a version released in August 2024 or later to prevent this error." + + " Alternatively, set the system property '" + allowAllProp + "' to 'true' if all the code built by Jenkins is as trusted as an administrator."); + } else { + LOGGER.log(Level.FINE, () -> "Allowing URL: " + url); + } + } + @SuppressFBWarnings( + value = "DMI_COLLECTION_OF_URLS", + justification = "All URLs point to local files, so no DNS lookup.") + private static boolean isAllowedJar(URL url) { + final ClassLoader classLoader = Jenkins.get().getPluginManager().uberClassLoader; + if (classLoader instanceof PluginManager.UberClassLoader) { + if (((PluginManager.UberClassLoader) classLoader).isPluginJar(url)) { + LOGGER.log(Level.FINER, () -> "Determined to be plugin jar: " + url); + return true; + } + } + + final ClassLoader coreClassLoader = Jenkins.class.getClassLoader(); + if (coreClassLoader instanceof URLClassLoader) { + if (Set.of(((URLClassLoader) coreClassLoader).getURLs()).contains(url)) { + LOGGER.log(Level.FINER, () -> "Determined to be core jar: " + url); + return true; + } + } + + LOGGER.log(Level.FINER, () -> "Neither core nor plugin jar: " + url); + return false; + } +} diff --git a/pom.xml b/pom.xml index 55dff2c11311..cbabaa78014b 100644 --- a/pom.xml +++ b/pom.xml @@ -87,7 +87,7 @@ THE SOFTWARE. https://www.jenkins.io/changelog - 3248.v65ecb_254c298 + 3248.3250.v3277a_8e88c9b_ 4.13 diff --git a/test/pom.xml b/test/pom.xml index f2cb4ee05d43..7892d7aef50c 100644 --- a/test/pom.xml +++ b/test/pom.xml @@ -323,6 +323,14 @@ THE SOFTWARE. ${project.build.outputDirectory}/old-remoting remoting-minimum-supported.jar + + org.jenkins-ci.main + remoting + 3248.v65ecb_254c298 + jar + ${project.build.outputDirectory}/old-remoting + remoting-before-SECURITY-3430-fix.jar + org.jenkins-ci.main remoting diff --git a/test/src/test/java/jenkins/security/Security3430Test.java b/test/src/test/java/jenkins/security/Security3430Test.java new file mode 100644 index 000000000000..4f8b3ae033ab --- /dev/null +++ b/test/src/test/java/jenkins/security/Security3430Test.java @@ -0,0 +1,296 @@ +package jenkins.security; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +import hudson.ExtensionList; +import hudson.model.Computer; +import hudson.remoting.Channel; +import hudson.remoting.Launcher; +import hudson.slaves.SlaveComputer; +import hudson.util.RingBufferLogHandler; +import java.io.File; +import java.io.IOException; +import java.lang.reflect.Field; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.net.URL; +import java.nio.charset.StandardCharsets; +import java.security.Security; +import java.util.List; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; +import jenkins.bouncycastle.api.InstallBouncyCastleJCAProvider; +import jenkins.security.s2m.JarURLValidatorImpl; +import jenkins.slaves.RemotingVersionInfo; +import org.apache.commons.io.FileUtils; +import org.apache.commons.io.IOUtils; +import org.hamcrest.Description; +import org.hamcrest.Matcher; +import org.hamcrest.TypeSafeMatcher; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.InboundAgentRule; +import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.RealJenkinsRule; +import org.kohsuke.args4j.Argument; +import org.kohsuke.stapler.Stapler; + +public class Security3430Test { + @Rule + public RealJenkinsRule jj = new RealJenkinsRule().withLogger(JarURLValidatorImpl.class, Level.FINEST); + + @Rule + public InboundAgentRule agents = new InboundAgentRule(); + + @Test + public void runWithOldestSupportedAgentJar() throws Throwable { + runWithRemoting(RemotingVersionInfo.getMinimumSupportedVersion().toString(), "/old-remoting/remoting-minimum-supported.jar", true); + } + + @Test + public void runWithPreviousAgentJar() throws Throwable { + runWithRemoting("3248.v65ecb_254c298", "/old-remoting/remoting-before-SECURITY-3430-fix.jar", true); + } + + @Test + public void runWithCurrentAgentJar() throws Throwable { + runWithRemoting(null, null, false); + } + + private void runWithRemoting(String expectedRemotingVersion, String remotingResourcePath, boolean requestingJarFromAgent) throws Throwable { + if (expectedRemotingVersion != null) { + FileUtils.copyURLToFile(Security3430Test.class.getResource(remotingResourcePath), new File(jj.getHome(), "agent.jar")); + } + + jj.startJenkins(); + final String agentName = "agent1"; + try { + agents.createAgent(jj, InboundAgentRule.Options.newBuilder().name(agentName).build()); + jj.runRemotely(Security3430Test::_run, agentName, expectedRemotingVersion, requestingJarFromAgent, true); + } finally { + agents.stop(jj, agentName); + } + jj.runRemotely(Security3430Test::disableJarURLValidatorImpl); + final String agentName2 = "agent2"; + try { + agents.createAgent(jj, InboundAgentRule.Options.newBuilder().name(agentName2).build()); + jj.runRemotely(Security3430Test::_run, agentName2, expectedRemotingVersion, requestingJarFromAgent, false); + } finally { + agents.stop(jj, agentName2); + } + } + + // This is quite artificial, but demonstrating that without JarURLValidatorImpl we do not allow any calls from the agent: + private static void disableJarURLValidatorImpl(JenkinsRule j) { + assertTrue(ExtensionList.lookup(ChannelConfigurator.class).remove(ExtensionList.lookupSingleton(JarURLValidatorImpl.class))); + } + + /** + * + * @param agentName the name of the agent we're working with + * @param expectedRemotingVersion The version expected for remoting, or {@code null} if we're using whatever is bundled with this Jenkins. + * @param requestingJarFromAgent {@code true} if and only if we expect to go through {@code ClassLoaderProxy#fetchJar} + * @param hasJenkinsJarURLValidator {@code true} if and only we do not expect {@link jenkins.security.s2m.JarURLValidatorImpl} to be present. Only relevant when {@code requestingJarFromAgent} is {@code true}. + */ + private static void _run(JenkinsRule j, String agentName, String expectedRemotingVersion, Boolean requestingJarFromAgent, Boolean hasJenkinsJarURLValidator) throws Throwable { + final RingBufferLogHandler logHandler = new RingBufferLogHandler(50); + Logger.getLogger(JarURLValidatorImpl.class.getName()).addHandler(logHandler); + final List logRecords = logHandler.getView(); + + final Computer computer = j.jenkins.getComputer(agentName); + assertThat(computer, instanceOf(SlaveComputer.class)); + SlaveComputer agent = (SlaveComputer) computer; + final Channel channel = agent.getChannel(); + if (expectedRemotingVersion != null) { + final String result = channel.call(new AgentVersionCallable()); + assertThat(result, is(expectedRemotingVersion)); + } + + logHandler.clear(); + + { // regular behavior + if (hasJenkinsJarURLValidator) { + // it works + assertTrue(channel.preloadJar(j.jenkins.getPluginManager().uberClassLoader, Stapler.class)); + if (requestingJarFromAgent) { + assertThat(logRecords, hasItem(logMessageContainsString("Allowing URL: file:/"))); + } else { + assertThat(logRecords, is(empty())); + } + + logHandler.clear(); + assertFalse(channel.preloadJar(j.jenkins.getPluginManager().uberClassLoader, Stapler.class)); + assertThat(logRecords, not(hasItem(logMessageContainsString("Allowing URL")))); + assertThat(logRecords, not(hasItem(logMessageContainsString("Rejecting URL")))); + } else { + // outdated remoting.jar will fail, but up to date one passes + if (requestingJarFromAgent) { + final IOException ex = assertThrows(IOException.class, () -> channel.preloadJar(j.jenkins.getPluginManager().uberClassLoader, Stapler.class)); + assertThat(ex.getMessage(), containsString("No hudson.remoting.JarURLValidator has been set for this channel, so all #fetchJar calls are rejected. This is likely a bug in Jenkins. As a workaround, try updating the agent.jar file.")); + } else { + assertTrue(channel.preloadJar(j.jenkins.getPluginManager().uberClassLoader, Stapler.class)); + assertThat(logRecords, is(empty())); + } + } + } + + logHandler.clear(); + + if (hasJenkinsJarURLValidator) { // Start rejecting everything; only applies to JarURLValidatorImpl + System.setProperty(JarURLValidatorImpl.class.getName() + ".REJECT_ALL", "true"); + + // Identify that a jar was already loaded: + assertFalse(channel.preloadJar(j.jenkins.getPluginManager().uberClassLoader, Stapler.class)); + assertThat(logRecords, not(hasItem(logMessageContainsString("Allowing URL")))); + assertThat(logRecords, not(hasItem(logMessageContainsString("Rejecting URL")))); + + logHandler.clear(); + + // different jar file than before, old remoting will fail due to call through ClassLoaderProxy#fetchJar, new remoting passes + if (requestingJarFromAgent) { + final IOException ioException = assertThrows(IOException.class, () -> channel.preloadJar(j.jenkins.getPluginManager().uberClassLoader, Argument.class)); + assertThat(ioException.getMessage(), containsString("all attempts by agents to load jars from the controller are rejected")); + assertThat(logRecords, not(hasItem(logMessageContainsString("Allowing URL")))); + assertThat(logRecords, hasItem(logMessageContainsString("Rejecting URL due to configuration: "))); + } else { + assertTrue(channel.preloadJar(j.jenkins.getPluginManager().uberClassLoader, org.kohsuke.args4j.Argument.class)); + assertThat(logRecords, not(hasItem(logMessageContainsString("Allowing URL")))); + assertThat(logRecords, not(hasItem(logMessageContainsString("Rejecting URL")))); + } + } + + logHandler.clear(); + + if (hasJenkinsJarURLValidator) { // Disable block, only applies to JarURLValidatorImpl + System.clearProperty(JarURLValidatorImpl.class.getName() + ".REJECT_ALL"); + if (requestingJarFromAgent) { + // now it works again for old remoting: + assertTrue(channel.preloadJar(j.jenkins.getPluginManager().uberClassLoader, org.kohsuke.args4j.Argument.class)); + assertThat(logRecords, hasItem(logMessageContainsString("Allowing URL: file:/"))); + } else { + // new remoting already has it. + assertFalse(channel.preloadJar(j.jenkins.getPluginManager().uberClassLoader, org.kohsuke.args4j.Argument.class)); + assertThat(logRecords, not(hasItem(logMessageContainsString("Allowing URL")))); + assertThat(logRecords, not(hasItem(logMessageContainsString("Rejecting URL")))); + } + assertThat(logRecords, not(hasItem(logMessageContainsString("Rejecting URL due to configuration: ")))); + } + + logHandler.clear(); + + if (hasJenkinsJarURLValidator || !requestingJarFromAgent) { // prepare bouncycastle-api + assertTrue(j.jenkins.getPluginManager().getPlugin("bouncycastle-api").isActive()); + InstallBouncyCastleJCAProvider.on(channel); + channel.call(new ConfirmBouncyCastleLibrary()); + } + + logHandler.clear(); + + { // Exploitation tests + final URL secretKeyFile = new File(j.jenkins.getRootDir(), "secret.key").toURI().toURL(); + final String expectedContent = IOUtils.toString(secretKeyFile, StandardCharsets.UTF_8); + { // Protection is effective when agents request non-jar files: + final InvocationTargetException itex = assertThrows(InvocationTargetException.class, () -> channel.call(new Exploit(secretKeyFile, expectedContent))); + assertThat(itex.getCause(), instanceOf(IOException.class)); + if (hasJenkinsJarURLValidator) { + assertThat(itex.getCause().getMessage(), containsString("This URL does not point to a jar file allowed to be requested by agents")); + assertThat(logRecords, not(hasItem(logMessageContainsString("Allowing URL")))); + assertThat(logRecords, hasItem(logMessageContainsString("Rejecting URL: "))); + } else { + assertThat(itex.getCause().getMessage(), containsString("No hudson.remoting.JarURLValidator has been set for this channel, so all #fetchJar calls are rejected. This is likely a bug in Jenkins. As a workaround, try updating the agent.jar file.")); + } + } + + logHandler.clear(); + + { // Disable protection and non-jar files can be accessed: + System.setProperty(Channel.class.getName() + ".DISABLE_JAR_URL_VALIDATOR", "true"); + channel.call(new Exploit(secretKeyFile, expectedContent)); + if (hasJenkinsJarURLValidator) { + assertThat(logRecords, hasItem(logMessageContainsString("Allowing URL due to configuration"))); + assertThat(logRecords, not(hasItem(logMessageContainsString("Rejecting URL")))); + } + System.clearProperty(Channel.class.getName() + ".DISABLE_JAR_URL_VALIDATOR"); + } + } + } + + private static class AgentVersionCallable extends MasterToSlaveCallable { + @Override + public String call() throws Exception { + return Launcher.VERSION; + } + } + + private static class ConfirmBouncyCastleLibrary extends MasterToSlaveCallable { + @Override + public Void call() throws Exception { + assertNotNull(Security.getProvider("BC")); + return null; + } + } + + private static class Exploit extends MasterToSlaveCallable { + private final URL controllerFilePath; + private final String expectedContent; + + public Exploit(URL controllerFilePath, String expectedContent) { + this.controllerFilePath = controllerFilePath; + this.expectedContent = expectedContent; + } + @Override + public Void call() throws Exception { + final ClassLoader ccl = Thread.currentThread().getContextClassLoader(); + System.err.println(ccl.getClass().getName()); + final Field classLoaderProxyField = ccl.getClass().getDeclaredField("proxy"); + classLoaderProxyField.setAccessible(true); + final Object theProxy = classLoaderProxyField.get(ccl); + final Method fetchJarMethod = theProxy.getClass().getDeclaredMethod("fetchJar", URL.class); + fetchJarMethod.setAccessible(true); + final byte[] fetchJarResponse = (byte[]) fetchJarMethod.invoke(theProxy, controllerFilePath); + assertThat(new String(fetchJarResponse, StandardCharsets.UTF_8), is(expectedContent)); + return null; + } + } + + // Would be nice if LoggerRule#recorded equivalents existed for use without LoggerRule, meanwhile: + private static Matcher logMessageContainsString(String needle) { + return new LogMessageContainsString(containsString(needle)); + } + + private static final class LogMessageContainsString extends TypeSafeMatcher { + private final Matcher stringMatcher; + + public LogMessageContainsString(Matcher stringMatcher) { + this.stringMatcher = stringMatcher; + } + + @Override + protected boolean matchesSafely(LogRecord item) { + return stringMatcher.matches(item.getMessage()); + } + + @Override + public void describeTo(Description description) { + description.appendText("a LogRecord with a message matching "); + stringMatcher.describeTo(description); + } + + @Override + protected void describeMismatchSafely(LogRecord item, Description mismatchDescription) { + mismatchDescription.appendText("a LogRecord with the message: "); + mismatchDescription.appendText(item.getMessage()); + } + } +}