Skip to content

Commit

Permalink
SECURITY-3303
Browse files Browse the repository at this point in the history
  • Loading branch information
@raul-arabaolaza
Andra Maria Puscas authored and raul-arabaolaza committed Feb 28, 2024
1 parent 9c72463 commit 6b84024
Show file tree
Hide file tree
Showing 3 changed files with 150 additions and 5 deletions.
16 changes: 16 additions & 0 deletions pom.xml
View file
Expand Up @@ -94,6 +94,22 @@
<artifactId>workflow-durable-task-step</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.testcontainers</groupId>
<artifactId>testcontainers</artifactId>
<version>1.18.3</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.kohsuke</groupId>
<artifactId>access-modifier-suppressions</artifactId>
<version>${access-modifier-checker.version}</version>
</dependency>
<dependency>
<groupId>org.kohsuke</groupId>
<artifactId>access-modifier-annotation</artifactId>
<version>1.33</version>
</dependency>
</dependencies>
<dependencyManagement>
<dependencies>
Expand Down
32 changes: 28 additions & 4 deletions src/main/java/htmlpublisher/HtmlPublisher.java
View file
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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<HtmlPublisherTarget> reportTargets;

private static final String HEADER = "/htmlpublisher/HtmlPublisher/header.html";
Expand Down Expand Up @@ -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();
Expand All @@ -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;
Expand Down Expand Up @@ -367,6 +385,12 @@ public Collection<? extends Action> 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<Publisher> {
@Override
Expand Down
107 changes: 106 additions & 1 deletion src/test/java/htmlpublisher/HtmlPublisherIntegrationTest.java
View file
Expand Up @@ -5,27 +5,63 @@
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
*/
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.
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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<String, String> 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<Void> {
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;
}
}
}

1 comment on commit 6b84024

@basil
Copy link
Member

@basil basil commented on 6b84024 Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Started bundling the following unnecessary JARs into the plugin JPI:

[INFO] Bundling direct dependency access-modifier-annotation-1.33.jar
[INFO] Bundling direct dependency access-modifier-suppressions-1.33.jar

#274

Please sign in to comment.