Skip to content

Commit

Permalink
SECURITY-3301
Browse files Browse the repository at this point in the history
  • Loading branch information
BorisYaoA authored and raul-arabaolaza committed Feb 28, 2024
1 parent e792f5a commit 374642b
Show file tree
Hide file tree
Showing 8 changed files with 387 additions and 69 deletions.
83 changes: 27 additions & 56 deletions src/main/java/htmlpublisher/HtmlPublisherTarget.java
Original file line number Diff line number Diff line change
@@ -1,15 +1,22 @@
package htmlpublisher;

import java.io.File;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.Objects;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import com.infradna.tool.bridge_method_injector.WithBridgeMethods;
import edu.umd.cs.findbugs.annotations.NonNull;
import javax.servlet.ServletException;

import hudson.Extension;
import hudson.FilePath;
import hudson.Util;
import hudson.model.AbstractDescribableImpl;
import hudson.model.Action;
import hudson.model.AbstractItem;
import hudson.model.Run;
import hudson.model.DirectoryBrowserSupport;
import hudson.model.Job;
import hudson.model.ProminentProjectAction;
import hudson.model.AbstractBuild;
import hudson.model.InvisibleAction;
import hudson.model.Descriptor;
import hudson.util.HttpResponses;
import jenkins.model.RunAction2;
import org.apache.commons.codec.binary.Hex;
import org.apache.commons.lang.StringUtils;
import org.kohsuke.accmod.Restricted;
Expand All @@ -20,23 +27,15 @@
import org.kohsuke.stapler.StaplerResponse;
import org.owasp.encoder.Encode;

import com.infradna.tool.bridge_method_injector.WithBridgeMethods;
import javax.servlet.ServletException;
import java.io.File;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.Objects;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import hudson.Extension;
import hudson.FilePath;
import hudson.Util;
import hudson.model.AbstractBuild;
import hudson.model.AbstractDescribableImpl;
import hudson.model.AbstractItem;
import hudson.model.Action;
import hudson.model.Descriptor;
import hudson.model.DirectoryBrowserSupport;
import hudson.model.InvisibleAction;
import hudson.model.Job;
import hudson.model.ProminentProjectAction;
import hudson.model.Run;
import hudson.util.HttpResponses;
import jenkins.model.RunAction2;
import static hudson.Functions.htmlAttributeEscape;

/**
* A representation of an HTML directory to archive and publish.
Expand All @@ -48,7 +47,7 @@ public class HtmlPublisherTarget extends AbstractDescribableImpl<HtmlPublisherTa
/**
* The name of the report to display for the build/project, such as "Code Coverage"
*/
private final String reportName;
private String reportName;

/**
* The path to the HTML report directory relative to the workspace.
Expand Down Expand Up @@ -183,15 +182,8 @@ public void setReportTitles(String reportTitles) {
this.reportTitles = StringUtils.trim(reportTitles);
}

/**
* Actually not safe, this allowed directory traversal (SECURITY-784).
* @return Returns a string with replaced whitespaces by underscores.
*/
private String getLegacySanitizedName() {
String safeName = this.reportName;
safeName = safeName.replace(" ", "_");
return safeName;
}
//Add this for testing purposes
public void setReportName(String reportName) {this.reportName = StringUtils.trim(reportName);}

public String getSanitizedName() {
return sanitizeReportName(this.reportName, getEscapeUnderscores());
Expand Down Expand Up @@ -313,11 +305,6 @@ protected File dir() {
if (run != null) {
File javadocDir = getBuildArchiveDir(run);

if (!javadocDir.exists()) {
javadocDir = getBuildArchiveDir(run, getLegacySanitizedName());
}
// TODO not sure about this change

if (javadocDir.exists()) {
for (HTMLBuildAction a : run.getActions(HTMLBuildAction.class)) {
if (a.getHTMLTarget().getReportName().equals(getHTMLTarget().getReportName())) {
Expand All @@ -329,15 +316,7 @@ protected File dir() {
}
}

// SECURITY-784: prefer safe over legacy, but if neither exists, return safe dir
File projectArchiveDir = getProjectArchiveDir(this.project);
if (projectArchiveDir.exists()) {
return projectArchiveDir;
}
File legacyProjectArchiveDir = getProjectArchiveDir(this.project, getLegacySanitizedName());
if (legacyProjectArchiveDir.exists()) {
return legacyProjectArchiveDir;
}
return projectArchiveDir;
}

Expand Down Expand Up @@ -440,15 +419,7 @@ public String getBackToUrl() {

@Override
protected File dir() {
// SECURITY-784: prefer safe over legacy, but if neither exists, return safe dir
File buildArchiveDir = getBuildArchiveDir(this.build);
if (buildArchiveDir.exists()) {
return buildArchiveDir;
}
File legacyBuildArchiveDir = getBuildArchiveDir(this.build, getLegacySanitizedName());
if (legacyBuildArchiveDir.exists()) {
return legacyBuildArchiveDir;
}
return buildArchiveDir;
}

Expand Down
74 changes: 74 additions & 0 deletions src/test/java/htmlpublisher/Security3301Test.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package htmlpublisher;

import hudson.model.FreeStyleProject;
import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.recipes.LocalData;

import java.io.File;

import static hudson.Functions.isWindows;
import static org.junit.Assume.assumeFalse;

public class Security3301Test {

@Rule
public JenkinsRule j = new JenkinsRule();

@Test
@LocalData
public void security3301sanitizeTest() throws Exception {


// Skip on windows
assumeFalse(isWindows());

FreeStyleProject job = j.jenkins.getItemByFullName("testJob", FreeStyleProject.class);

Assert.assertTrue(new File(job.getRootDir(), "htmlreports/HTML_20Report").exists());

j.buildAndAssertSuccess(job);

changeJobReportName(job,"HTML_20Report/javascript:alert(1)");

job.save();

j.buildAndAssertSuccess(job);

HtmlPublisherTarget.HTMLAction action = job.getAction(HtmlPublisherTarget.HTMLAction.class);
Assert.assertNotNull(action);

//Check that the report name is escaped for the Url
Assert.assertEquals("HTML_20Report/javascript:alert(1)", action.getHTMLTarget().getReportName());
Assert.assertEquals("HTML_5f20Report_2fjavascript_3aalert_281_29", action.getUrlName());

Assert.assertTrue(new File(job.getRootDir(), "htmlreports/HTML_5f20Report_2fjavascript_3aalert_281_29").exists());

FreeStyleProject anotherJob = j.jenkins.getItemByFullName("anotherJob", FreeStyleProject.class);

Assert.assertTrue(new File(anotherJob.getRootDir(), "htmlreports/HTML_20Report").exists());

j.buildAndAssertSuccess(anotherJob);

changeJobReportName(job,"../../anotherJob/htmlreports/HTML_20Report");

job.save();

//Check that the build reports is not from the new job (anotherJob)
Assert.assertEquals("../../anotherJob/htmlreports/HTML_20Report", action.getHTMLTarget().getReportName());
Assert.assertEquals("_2e_2e_2f_2e_2e_2fanotherJob_2fhtmlreports_2fHTML_5f20Report", action.getUrlName());
Assert.assertFalse(new File(job.getRootDir(), "htmlreports/_2e_2e_2f_2e_2e_2fanotherJob_2fhtmlreports_2fHTML_5f20Report/test.txt").exists());

}

public void changeJobReportName(FreeStyleProject job, String newName) {
for (Object publisher : job.getPublishersList()) {
if (publisher instanceof HtmlPublisher) {
HtmlPublisher existingPublishHTML = (HtmlPublisher) publisher;
existingPublishHTML.getReportTargets().get(0).setReportName(newName);
}
}
}
}
15 changes: 2 additions & 13 deletions src/test/java/htmlpublisher/Security784Test.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,7 @@ public void security784upgradeTest() throws Exception {

Assert.assertTrue(new File(job.getRootDir(), "htmlreports/foo!!!!bar/index.html").exists());

HtmlPublisherTarget.HTMLAction action = job.getAction(HtmlPublisherTarget.HTMLAction.class);
Assert.assertNotNull(action);
Assert.assertEquals("foo!!!!bar", action.getHTMLTarget().getReportName());
Assert.assertEquals("foo!!!!bar", action.getUrlName()); // legacy

JenkinsRule.WebClient client = j.createWebClient();
HtmlPage page = client.getPage(job, "foo!!!!bar/index.html");
String text = page.getWebResponse().getContentAsString();
Assert.assertEquals("Sun Mar 25 15:42:10 CEST 2018", text.trim());

job.getBuildersList().clear();
String newDate = new Date().toString();
Expand All @@ -44,16 +36,13 @@ public void security784upgradeTest() throws Exception {

Assert.assertTrue(new File(job.getRootDir(), "htmlreports/foo_21_21_21_21bar/index.html").exists());

action = job.getAction(HtmlPublisherTarget.HTMLAction.class);
HtmlPublisherTarget.HTMLAction action = job.getAction(HtmlPublisherTarget.HTMLAction.class);
Assert.assertNotNull(action);
Assert.assertEquals("foo!!!!bar", action.getHTMLTarget().getReportName());
Assert.assertEquals("foo_21_21_21_21bar", action.getUrlName()); // new

text = client.goTo("job/thejob/foo_21_21_21_21bar/index.html").getWebResponse().getContentAsString();
String text = client.goTo("job/thejob/foo_21_21_21_21bar/index.html").getWebResponse().getContentAsString();
Assert.assertEquals(newDate, text.trim());

// leftovers from legacy naming
Assert.assertTrue(new File(job.getRootDir(), "htmlreports/foo!!!!bar/index.html").exists());
}

@Test
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
<?xml version='1.1' encoding='UTF-8'?>
<project>
<actions/>
<description></description>
<keepDependencies>false</keepDependencies>
<properties/>
<scm class="hudson.scm.NullSCM"/>
<canRoam>true</canRoam>
<disabled>false</disabled>
<blockBuildWhenDownstreamBuilding>false</blockBuildWhenDownstreamBuilding>
<blockBuildWhenUpstreamBuilding>false</blockBuildWhenUpstreamBuilding>
<triggers/>
<concurrentBuild>false</concurrentBuild>
<builders>
<hudson.tasks.Shell>
<command>echo &quot;Test999&quot; &gt; test.txt</command>
<configuredLocalRules/>
</hudson.tasks.Shell>
</builders>
<publishers>
<htmlpublisher.HtmlPublisher plugin="htmlpublisher@1.33-SNAPSHOT">
<reportTargets>
<htmlpublisher.HtmlPublisherTarget>
<reportName>HTML Report</reportName>
<reportDir></reportDir>
<reportFiles>test.txt</reportFiles>
<alwaysLinkToLastBuild>false</alwaysLinkToLastBuild>
<reportTitles></reportTitles>
<keepAll>false</keepAll>
<allowMissing>false</allowMissing>
<includes>**/*</includes>
<escapeUnderscores>true</escapeUnderscores>
<useWrapperFileDirectly>true</useWrapperFileDirectly>
</htmlpublisher.HtmlPublisherTarget>
</reportTargets>
</htmlpublisher.HtmlPublisher>
</publishers>
<buildWrappers/>
</project>
Loading

0 comments on commit 374642b

Please sign in to comment.