From 6b840248dd0d691bbac9b515cd750b3f925909b2 Mon Sep 17 00:00:00 2001 From: Andra Maria Puscas Date: Wed, 28 Feb 2024 17:14:43 +0100 Subject: [PATCH] SECURITY-3303 --- pom.xml | 16 +++ .../java/htmlpublisher/HtmlPublisher.java | 32 +++++- .../HtmlPublisherIntegrationTest.java | 107 +++++++++++++++++- 3 files changed, 150 insertions(+), 5 deletions(-) diff --git a/pom.xml b/pom.xml index 74e67e5..2303aa5 100644 --- a/pom.xml +++ b/pom.xml @@ -94,6 +94,22 @@ workflow-durable-task-step test + + org.testcontainers + testcontainers + 1.18.3 + test + + + org.kohsuke + access-modifier-suppressions + ${access-modifier-checker.version} + + + org.kohsuke + access-modifier-annotation + 1.33 + diff --git a/src/main/java/htmlpublisher/HtmlPublisher.java b/src/main/java/htmlpublisher/HtmlPublisher.java index 7450bc2..dc86d9e 100644 --- a/src/main/java/htmlpublisher/HtmlPublisher.java +++ b/src/main/java/htmlpublisher/HtmlPublisher.java @@ -35,6 +35,8 @@ import java.io.Reader; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; +import java.nio.file.LinkOption; +import java.nio.file.OpenOption; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; import java.util.ArrayList; @@ -44,8 +46,11 @@ import java.util.List; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; +import hudson.util.DirScanner; +import jenkins.util.SystemProperties; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; +import org.kohsuke.accmod.restrictions.suppressions.SuppressRestrictedWarnings; import org.kohsuke.stapler.AncestorInPath; import org.kohsuke.stapler.DataBoundConstructor; import org.kohsuke.stapler.QueryParameter; @@ -84,6 +89,12 @@ * @author Mike Rooney */ public class HtmlPublisher extends Recorder { + + /** + * Restores old behavior before SECURITY-3303 + */ + @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Yes it should, but this allows the ability to change it via script in runtime.") + static /*almost final*/ boolean FOLLOW_SYMLINKS = SystemProperties.getBoolean(HtmlPublisher.class.getName() + ".FOLLOW_SYMLINKS", false); private final List reportTargets; private static final String HEADER = "/htmlpublisher/HtmlPublisher/header.html"; @@ -238,8 +249,13 @@ public static boolean publishReports(Run build, FilePath workspace, TaskLi // We are only keeping one copy at the project level, so remove the old one. targetDir.deleteRecursive(); } - - if (archiveDir.copyRecursiveTo(reportTarget.getIncludes(), targetDir) == 0) { + int copied = 0; + if (FOLLOW_SYMLINKS) { + copied = archiveDir.copyRecursiveTo(reportTarget.getIncludes(), targetDir); + } else { + copied = archiveDir.copyRecursiveTo(dirScannerGlob(reportTarget.getIncludes(), null, true, LinkOption.NOFOLLOW_LINKS), targetDir, reportTarget.getIncludes()); + } + if (copied == 0) { if (!allowMissing) { listener.error("Directory '" + archiveDir + "' exists but failed copying to '" + targetDir + "'."); final Result buildResult = build.getResult(); @@ -252,8 +268,10 @@ public static boolean publishReports(Run build, FilePath workspace, TaskLi continue; } } - } catch (IOException e) { - Util.displayIOException(e, listener); + } catch (Exception e) { + if (e instanceof IOException) { + Util.displayIOException((IOException) e, listener); + } e.printStackTrace(listener.fatalError("HTML Publisher failure")); build.setResult(Result.FAILURE); return true; @@ -367,6 +385,12 @@ public Collection getProjectActions(AbstractProject proj } } + + @SuppressRestrictedWarnings(NoExternalUse.class) + public static DirScanner dirScannerGlob(String includes, String excludes, boolean useDefaultExcludes, OpenOption... openOptions) throws Exception { + return new DirScanner.Glob(includes, excludes, useDefaultExcludes, openOptions); + } + @Extension public static class DescriptorImpl extends BuildStepDescriptor { @Override diff --git a/src/test/java/htmlpublisher/HtmlPublisherIntegrationTest.java b/src/test/java/htmlpublisher/HtmlPublisherIntegrationTest.java index 933d558..67a3544 100644 --- a/src/test/java/htmlpublisher/HtmlPublisherIntegrationTest.java +++ b/src/test/java/htmlpublisher/HtmlPublisherIntegrationTest.java @@ -5,20 +5,42 @@ import hudson.Launcher; import hudson.model.*; import hudson.model.queue.QueueTaskFuture; +import hudson.remoting.VirtualChannel; +import hudson.slaves.DumbSlave; import hudson.slaves.EnvironmentVariablesNodeProperty; +import hudson.slaves.JNLPLauncher; +import hudson.slaves.RetentionStrategy; +import jenkins.MasterToSlaveFileCallable; +import org.apache.commons.io.FileUtils; +import org.junit.After; import org.junit.Rule; import org.junit.Test; +import org.jvnet.hudson.test.BuildWatcher; +import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.TemporaryDirectoryAllocator; import org.jvnet.hudson.test.TestBuilder; +import org.testcontainers.DockerClientFactory; +import org.testcontainers.containers.GenericContainer; import java.io.File; import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; +import java.nio.file.Paths; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Map; -import static org.junit.Assert.*; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.collection.IsEmptyCollection.empty; +import static org.hamcrest.core.IsNot.not; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assume.assumeTrue; /** * @author Kohsuke Kawaguchi @@ -26,6 +48,20 @@ public class HtmlPublisherIntegrationTest { @Rule public JenkinsRule j = new JenkinsRule(); + @Rule + public BuildWatcher buildWatcher = new BuildWatcher(); + + public TemporaryDirectoryAllocator tmp = new TemporaryDirectoryAllocator(); + private GenericContainer agentContainer; + private DumbSlave agent; + + @After + public void dispose() throws IOException, InterruptedException { + tmp.dispose(); + if (agentContainer != null) { + agentContainer.stop(); + } + } /** * Makes sure that the configuration survives the round trip. @@ -84,6 +120,39 @@ public boolean perform(AbstractBuild build, Launcher launcher, assertFalse(tab2Files.contains("dummy.html")); } + @Test @Issue("SECURITY-3303") + public void testNotFollowingSymlinks() throws Exception { + createDockerAgent(); + final File directoryOnController = tmp.allocate(); + FileUtils.write(new File(directoryOnController, "test.txt"), "test", StandardCharsets.UTF_8); + final String directoryOnControllerPath = directoryOnController.getAbsolutePath(); + FreeStyleProject p = j.createFreeStyleProject(); + p.getBuildersList().add(new TestBuilder() { + @Override + public boolean perform(AbstractBuild build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException { + FilePath workspace = build.getWorkspace(); + workspace.act(new MakeSymlink(directoryOnControllerPath)); + workspace.child("test3.txt").write("Hello", "UTF-8"); + return true; + } + }); + HtmlPublisherTarget target1 = new HtmlPublisherTarget("tab1", "", "tab1/test.txt,tab1/test2.txt,**/test3.txt", true, false, true); + p.getPublishersList().add(new HtmlPublisher(Arrays.asList(target1))); + p.setAssignedLabel(Label.get("agent")); + FreeStyleBuild build = j.buildAndAssertSuccess(p); + File base = new File(build.getRootDir(), "htmlreports"); + String[] list = base.list(); + assertNotNull(list); + assertThat(Arrays.asList(list), not(empty())); + File tab1 = new File(base, "tab1"); + list = tab1.list(); + assertNotNull(list); + assertThat(Arrays.asList(list), not(empty())); + + File reports = new File(tab1, "tab1"); + assertFalse(reports.exists()); + } + @Test public void testVariableExpansion() throws Exception { FreeStyleProject p = j.createFreeStyleProject("variable_job"); @@ -246,5 +315,41 @@ private void addEnvironmentVariable(String key, String value) { j.jenkins.getGlobalNodeProperties().add(prop); } + private void createDockerAgent() throws Exception { + assumeTrue("Needs Docker", DockerClientFactory.instance().isDockerAvailable()); + j.jenkins.setSlaveAgentPort(0); + int port = j.jenkins.getTcpSlaveAgentListener().getAdvertisedPort(); + synchronized (j.jenkins) { + agent = new DumbSlave("dockeragentOne", "/home/jenkins/work", new JNLPLauncher(true)); + agent.setLabelString("agent"); + agent.setRetentionStrategy(RetentionStrategy.NOOP); + j.jenkins.addNode(agent); + } + Map env = Map.of("JENKINS_URL", JNLPLauncher.getInboundAgentUrl(), + "JENKINS_SECRET", agent.getComputer().getJnlpMac(), + "JENKINS_AGENT_NAME", agent.getNodeName(), + "JENKINS_AGENT_WORKDIR", agent.getRemoteFS()); + System.out.println(env); + agentContainer = new GenericContainer<>("jenkins/inbound-agent:jdk" + System.getProperty("java.specification.version")) + .withEnv(env) + .withNetworkMode("host").withLogConsumer(outputFrame -> System.out.print(outputFrame.getUtf8String())); + //agentContainer.getHost() + agentContainer.start(); + j.waitOnline(agent); + } + + static class MakeSymlink extends MasterToSlaveFileCallable { + final String target; + + MakeSymlink(String target) { + this.target = target; + } + + @Override + public Void invoke(File f, VirtualChannel channel) throws IOException, InterruptedException { + Files.createSymbolicLink(Paths.get(f.getAbsolutePath(), "tab1"), Paths.get(target)); + return null; + } + } }