From ea41b3f86a24ab398a588bde6a4eada869bed391 Mon Sep 17 00:00:00 2001 From: cizezsy Date: Tue, 3 Mar 2020 04:56:57 +0100 Subject: [PATCH] [SECURITY-1668] Co-authored-by: Federico Pellegrin --- .../hudson/plugins/cobertura/IOUtils.java | 24 +++++++++ .../renderers/SourceCodePainter.java | 7 ++- .../cobertura/targets/CoverageResult.java | 20 ++++++-- .../hudson/plugins/cobertura/IOUtilsTest.java | 50 +++++++++++++++++++ 4 files changed, 96 insertions(+), 5 deletions(-) create mode 100644 src/test/java/hudson/plugins/cobertura/IOUtilsTest.java diff --git a/src/main/java/hudson/plugins/cobertura/IOUtils.java b/src/main/java/hudson/plugins/cobertura/IOUtils.java index 43dfb1a6..cb1ce528 100644 --- a/src/main/java/hudson/plugins/cobertura/IOUtils.java +++ b/src/main/java/hudson/plugins/cobertura/IOUtils.java @@ -2,6 +2,11 @@ import java.io.Closeable; +import com.google.common.base.Charsets; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import org.apache.commons.codec.binary.Hex; + /** * General IO stream manipulation utilities. */ @@ -20,4 +25,23 @@ public static void closeQuietly(Closeable closeable) { } } } + + /** + * Sanitizes filename to prevent directory trasversals or other security threats + * Transforms every non alphanumeric character in its ascii number in the _XX format + * + * @param inputName the name to sanitize + * @return the sanitized string + */ + public static String sanitizeFilename(String inputName) { + Pattern p = Pattern.compile("[^a-zA-Z0-9-]"); + Matcher m = p.matcher(inputName); + StringBuffer sb = new StringBuffer(); + while (m.find()) { + String match = m.group(); + m.appendReplacement(sb, "_" + Hex.encodeHexString(match.getBytes(Charsets.UTF_8))); + } + m.appendTail(sb); + return sb.toString(); + } } diff --git a/src/main/java/hudson/plugins/cobertura/renderers/SourceCodePainter.java b/src/main/java/hudson/plugins/cobertura/renderers/SourceCodePainter.java index 220fd191..41b6b9d9 100644 --- a/src/main/java/hudson/plugins/cobertura/renderers/SourceCodePainter.java +++ b/src/main/java/hudson/plugins/cobertura/renderers/SourceCodePainter.java @@ -4,6 +4,7 @@ import hudson.FilePath; import hudson.model.TaskListener; +import hudson.plugins.cobertura.IOUtils; import hudson.plugins.cobertura.targets.CoveragePaint; import hudson.remoting.VirtualChannel; import jenkins.MasterToSlaveFileCallable; @@ -114,7 +115,7 @@ public void paintSourceCode(File source, CoveragePaint paint, FilePath canvas) t * {@inheritDoc} */ public Boolean invoke(File workspaceDir, VirtualChannel channel) throws IOException { - final List trialPaths = new ArrayList(sourcePaths.size()); + final List trialPaths = new ArrayList<>(sourcePaths.size()); for (String sourcePath : sourcePaths) { final File trialPath = new File(sourcePath); if (trialPath.exists()) { @@ -134,7 +135,9 @@ public Boolean invoke(File workspaceDir, VirtualChannel channel) throws IOExcept } if (source.isFile()) { try { - paintSourceCode(source, entry.getValue(), destination.child(entry.getKey())); + // sanitizing file name to avoid arbitrarily write + String sanitizedName = IOUtils.sanitizeFilename(entry.getKey()); + paintSourceCode(source, entry.getValue(), destination.child(sanitizedName)); } catch (IOException e) { // We made our best shot at generating painted source code, // but alas, we failed. Log the error and continue. We diff --git a/src/main/java/hudson/plugins/cobertura/targets/CoverageResult.java b/src/main/java/hudson/plugins/cobertura/targets/CoverageResult.java index 8511d9ad..231dd70b 100755 --- a/src/main/java/hudson/plugins/cobertura/targets/CoverageResult.java +++ b/src/main/java/hudson/plugins/cobertura/targets/CoverageResult.java @@ -8,6 +8,7 @@ import hudson.plugins.cobertura.Chartable; import hudson.plugins.cobertura.CoberturaBuildAction; import hudson.plugins.cobertura.CoverageChart; +import hudson.plugins.cobertura.IOUtils; import hudson.plugins.cobertura.Ratio; import hudson.util.Graph; import hudson.util.TextFile; @@ -29,6 +30,7 @@ import java.util.TreeMap; import java.util.TreeSet; +import javax.annotation.Nullable; import org.jfree.chart.JFreeChart; import org.kohsuke.stapler.StaplerRequest; import org.kohsuke.stapler.StaplerResponse; @@ -171,9 +173,16 @@ public void paint(int line, int hits, int branchHits, int branchTotal) { * * @return The file where the source file should be (if it exists) */ + @Nullable private File getSourceFile() { if (hasPermission()) { - return new File(owner.getParent().getRootDir(), "cobertura/" + relativeSourcePath); + File sourceFile = new File(owner.getParent().getRootDir(), "cobertura/" + IOUtils.sanitizeFilename(relativeSourcePath)); + if (sourceFile.exists()) { + return sourceFile; + } else { + // keep compatibility + return new File(owner.getParent().getRootDir(), "cobertura/" + relativeSourcePath); + } } return null; } @@ -185,7 +194,8 @@ private File getSourceFile() { */ public boolean isSourceFileAvailable() { if (hasPermission()) { - return owner == owner.getParent().getLastSuccessfulBuild() && getSourceFile().exists(); + File sourceFile = getSourceFile(); + return owner == owner.getParent().getLastSuccessfulBuild() && sourceFile != null && sourceFile.exists(); } return false; } @@ -202,7 +212,11 @@ public boolean hasPermission() { public String getSourceFileContent() { if (hasPermission()) { try { - return new TextFile(getSourceFile()).read(); + File sourceFile = getSourceFile(); + if (sourceFile == null) { + return null; + } + return new TextFile(sourceFile).read(); } catch (IOException e) { return null; } diff --git a/src/test/java/hudson/plugins/cobertura/IOUtilsTest.java b/src/test/java/hudson/plugins/cobertura/IOUtilsTest.java new file mode 100644 index 00000000..bb56866f --- /dev/null +++ b/src/test/java/hudson/plugins/cobertura/IOUtilsTest.java @@ -0,0 +1,50 @@ +package hudson.plugins.cobertura; + +import static org.junit.Assert.assertEquals; + +import org.junit.Test; + +public class IOUtilsTest { + + @Test + public void testSanitizeRelativePathInUnix() { + String fileName = "../aaa/bbb"; + String sanitizedName = IOUtils.sanitizeFilename(fileName); + + assertEquals("_2e_2e_2faaa_2fbbb", sanitizedName); + + fileName = "aaa/../bbb"; + sanitizedName = IOUtils.sanitizeFilename(fileName); + + assertEquals("aaa_2f_2e_2e_2fbbb", sanitizedName); + } + + @Test + public void testSanitizeRelativePathInWindows() { + String fileName = "..\\aaa\\bbb"; + String sanitizedName = IOUtils.sanitizeFilename(fileName); + + assertEquals("_2e_2e_5caaa_5cbbb", sanitizedName); + + fileName = "aaa\\..\\bbb"; + sanitizedName = IOUtils.sanitizeFilename(fileName); + assertEquals( "aaa_5c_2e_2e_5cbbb", sanitizedName); + } + + + @Test + public void testSanitizeAbsolutePathInUnix() { + String fileName = "/aaa/bbb"; + String sanitizedFileName = IOUtils.sanitizeFilename(fileName); + + assertEquals("_2faaa_2fbbb", sanitizedFileName); + } + + @Test + public void testSanitizeAbsolutePathInWindows() { + String fileName = "C:\\aaa\\bbb"; + String sanitizedName = IOUtils.sanitizeFilename(fileName); + + assertEquals("C_3a_5caaa_5cbbb", sanitizedName); + } +}