Skip to content

Commit

Permalink
Avoid path traversal threats (#239)
Browse files Browse the repository at this point in the history
* Introduce and make use of IOHelper.securePath() to further harden against path traversal threats.

* Limit DirectUrlWebContentProvider to only handle http and https URLs.

* Update release notes

* Rename method from securePath to secureFilePath to better clarify scope/purpose

* fixed typo in comment

Co-authored-by: chrimih <[email protected]>

---------

Co-authored-by: chrimih <[email protected]>
  • Loading branch information
ohecker and chrimih authored Mar 15, 2024
1 parent 35cf767 commit a38bf81
Show file tree
Hide file tree
Showing 11 changed files with 264 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
import java.io.InputStreamReader;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -47,7 +49,7 @@ public static String readStringFromInputStream(InputStream inp) throws IOExcepti
/**
* Assure that the directory in which the given file should be located exists. Try to create the directory if it does
* not yet exist.
*
*
* @param targetFilename the name (including path) of a file
* @see #checkAndCreateLocation(File)
*/
Expand All @@ -59,7 +61,7 @@ public static void checkAndCreateLocation(String targetFilename) {
/**
* Assure that the directory in which the given file should be located exists. Try to create the directory if it does
* not yet exist.
*
*
* @param targetFile a file
*/
public static void checkAndCreateLocation(File targetFile) {
Expand All @@ -82,6 +84,44 @@ public static void checkAndCreateLocation(File targetFile) {

}

/**
* Create a file path from the given base path and all provided relative paths. The method assures that each relative
* path applied to the already existing (intermediate) path does not point to some place outside the existing path.
*
* @param basePath the base file path
* @param relativePaths any number of relative file paths to be appended
* @return a file path built from the given arguments
* @throws IllegalArgumentException if any of the relativePaths points to some point outside the already existing path
* or if any of the relativePaths are absolute paths.
* @throws NullPointerException if the basePath or any of the relativePaths are <code>null</code>
*/
public static String secureFilePath(String basePath, String... relativePaths) {

if (basePath == null) {
throw new NullPointerException("Base path must not be null");
}
Path path = Paths.get(basePath).normalize();

for (String relative : relativePaths) {
if (relative == null) {
throw new NullPointerException("Relative paths must not be null");
}

Path relativePath = Paths.get(relative);
if (relativePath.isAbsolute()) {
throw new IllegalArgumentException(
"Given path element '" + relative + "' is absolute but only relative paths are allowed");
}
Path newPath = path.resolve(relativePath).normalize();
if (!newPath.startsWith(path)) {
throw new IllegalArgumentException(
"Given path element '" + relative + "' would result in escaping the parent tree hierarchy and is rejected");
}
path = newPath;
}
return path.toString();
}

/**
* Private constructor which prevents instantiation
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,8 @@ public enum LogMessages {
MODERN_YARN_VIRTUAL_PACKAGE(69,
"When reading yarn license info from file '{}' there was at least one virtual package encountered. Check if package resolution is correct"), //
MODERN_YARN_PATCHED_PACKAGE(70,
"When reading yarn license info from file '{}' there was at least one patched package encountered. Processing only the base package, not the patched version.");
"When reading yarn license info from file '{}' there was at least one patched package encountered. Processing only the base package, not the patched version."), //
FAILED_READING_FILE(71, "Reading file '{}' failed");

private final String message;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public C loadFromNext(String url) {

if (!url.startsWith("file:")) {
// data of URLs which resolve to local file will not be cached
File file = new File(this.resourceDirectory + "/" + getKey(url));
File file = new File(IOHelper.secureFilePath(this.resourceDirectory, getKey(url)));
File targetDir = file.getParentFile();
try {
IOHelper.checkAndCreateLocation(file);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ public class DirectUrlWebContentProvider implements ContentProvider<WebContent>

private boolean skipdownload;

private static final Pattern SUPPORTED_URL_PATTERNS = Pattern.compile("^http:.*|^https:.*|^jar:http:.*|^jar:https:.*",
Pattern.CASE_INSENSITIVE);

/**
* Constructor.
*
Expand All @@ -40,7 +43,8 @@ public DirectUrlWebContentProvider(boolean skipdownload) {
/**
* {@inheritDoc}
*
* Directly tries to access the given URL via the web.
* Directly tries to access the given URL via the web. Only URLs matching {@link #SUPPORTED_URL_PATTERNS} will be
* processed. All others return an empty WebContent.
*/
@Override
public WebContent getContentForUri(String url) {
Expand All @@ -49,6 +53,10 @@ public WebContent getContentForUri(String url) {
if (url == null) {
return new WebContent(null);
}
if (!isSupportedUrl(url)) {
LOG.debug("Accessing URL '{}' is not supported by DirectUrlWebContentProvider, returning empty WebContent", url);
return new WebContent(null);
}
if (this.skipdownload) {
LOG.info(LogMessages.SKIP_DOWNLOAD.msg(), url);
return new WebContent(null);
Expand Down Expand Up @@ -84,6 +92,17 @@ public WebContent getContentForUri(String url) {
return new WebContent(null);
}

/**
* Indicates if the given URL is supported by this {@link ContentProvider}.
*
* @param url the url to check
* @return <code>true</code> if url is supported, <code>false</code> otherwise.
*/
boolean isSupportedUrl(String url) {

return SUPPORTED_URL_PATTERNS.matcher(url).matches();
}

/**
* @param input
* @param lineInfo
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.util.Scanner;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -12,7 +13,7 @@
import org.springframework.stereotype.Component;

import com.devonfw.tools.solicitor.common.IOHelper;
import com.devonfw.tools.solicitor.common.content.web.DirectUrlWebContentProvider;
import com.devonfw.tools.solicitor.common.LogMessages;
import com.devonfw.tools.solicitor.common.packageurl.AllKindsPackageURLHandler;
import com.devonfw.tools.solicitor.componentinfo.ComponentInfoAdapterException;
import com.fasterxml.jackson.databind.JsonNode;
Expand All @@ -39,19 +40,14 @@ public class FileScancodeRawComponentInfoProvider implements ScancodeRawComponen

private AllKindsPackageURLHandler packageURLHandler;

private DirectUrlWebContentProvider contentProvider;

/**
* The constructor.
*
* @param contentProvider content provider for accessing source files of the packag
* @param packageURLHandler handler to deal with PackageURLs.
*/
@Autowired
public FileScancodeRawComponentInfoProvider(DirectUrlWebContentProvider contentProvider,
AllKindsPackageURLHandler packageURLHandler) {
public FileScancodeRawComponentInfoProvider(AllKindsPackageURLHandler packageURLHandler) {

this.contentProvider = contentProvider;
this.packageURLHandler = packageURLHandler;

}
Expand Down Expand Up @@ -81,7 +77,7 @@ public ScancodeRawComponentInfo readScancodeData(String packageUrl)
throws ComponentInfoAdapterException, ScancodeProcessingFailedException {

String packagePathPart = this.packageURLHandler.pathFor(packageUrl);
String path = this.repoBasePath + "/" + packagePathPart + "/scancode.json";
String path = IOHelper.secureFilePath(this.repoBasePath, packagePathPart, "scancode.json");

File scanCodeFile = new File(path);
if (!scanCodeFile.exists()) {
Expand Down Expand Up @@ -114,13 +110,13 @@ private void throwExceptionForDownloadOrScanningFailures(String packagePathPart)
throws ScancodeProcessingFailedException {

// Check if "sources.failed" exists
String sourcesFailedPath = this.repoBasePath + "/" + packagePathPart + "/sources.failed";
String sourcesFailedPath = IOHelper.secureFilePath(this.repoBasePath, packagePathPart, "sources.failed");
File sourcesFailedFile = new File(sourcesFailedPath);
if (sourcesFailedFile.exists()) {
throw new ScancodeProcessingFailedException("Downloading of package sources had failed.");
}
// Check if "scancodeScan.failed" exists
String scancodeScanFailedPath = this.repoBasePath + "/" + packagePathPart + "/scancodeScan.failed";
String scancodeScanFailedPath = IOHelper.secureFilePath(this.repoBasePath, packagePathPart, "scancodeScan.failed");
File scancodeScanFailedFile = new File(scancodeScanFailedPath);
if (scancodeScanFailedFile.exists()) {
throw new ScancodeProcessingFailedException("Scanning of package sources had failed.");
Expand All @@ -138,7 +134,7 @@ private void addOriginData(String packageUrl, ScancodeRawComponentInfo component
throws ComponentInfoAdapterException {

String packagePathPart = this.packageURLHandler.pathFor(packageUrl);
String path = this.repoBasePath + "/" + packagePathPart + "/origin.yaml";
String path = IOHelper.secureFilePath(this.repoBasePath, packagePathPart, "origin.yaml");

File originFile = new File(path);
if (!originFile.exists()) {
Expand Down Expand Up @@ -174,17 +170,39 @@ private void addOriginData(String packageUrl, ScancodeRawComponentInfo component
public String retrieveContent(String packageUrl, String fileUri) {

if (!fileUri.startsWith(PKG_CONTENT_SCHEMA_PREFIX)) {
// we only handle pkgcontent: URIs here!
return null;
}
if (fileUri.contains("..")) {
// prevent directory traversal
// prevent directory traversal (there are other measures to also prevent this, but lets do it here explicitly)
LOG.debug("Suspicious file traversal in URI '{}', returning null", fileUri);
return null;
}
String pathWithoutPrefix = fileUri.substring(PKG_CONTENT_SCHEMA_PREFIX.length());
String directUrl = "file:" + this.repoBasePath + "/" + this.packageURLHandler.pathFor(packageUrl) + "/"
+ SOURCES_DIR + pathWithoutPrefix;
return this.contentProvider.getContentForUri(directUrl).getContent();
String pkgContentUriWithoutPrefix = fileUri.substring(PKG_CONTENT_SCHEMA_PREFIX.length());

String relativeFilePathAndName = pkgContentUriWithoutPrefix;
String lineInfo = null;
int startOfLineInfo = pkgContentUriWithoutPrefix.indexOf("#L");
if (startOfLineInfo >= 0) {
lineInfo = pkgContentUriWithoutPrefix.substring(startOfLineInfo, pkgContentUriWithoutPrefix.length());
relativeFilePathAndName = pkgContentUriWithoutPrefix.substring(0, startOfLineInfo);
}

String fullFilePathAndName = IOHelper.secureFilePath(this.repoBasePath, this.packageURLHandler.pathFor(packageUrl),
SOURCES_DIR, relativeFilePathAndName);
File file = new File(fullFilePathAndName);
try (InputStream is = new FileInputStream(file); Scanner s = new Scanner(is)) {
s.useDelimiter("\\A");
String result = s.hasNext() ? s.next() : "";

return MultilineHelper.possiblyExtractLines(result, lineInfo);
} catch (IOException e) {
if (LOG.isDebugEnabled()) {
LOG.debug("Could not retrieve content from file '" + fullFilePathAndName + "'", e);
}
LOG.info(LogMessages.FAILED_READING_FILE.msg(), fullFilePathAndName, e.getClass().getSimpleName());
}
return null;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/**
* SPDX-License-Identifier: Apache-2.0
*/
package com.devonfw.tools.solicitor.componentinfo.scancode;

import java.util.regex.Matcher;
import java.util.regex.Pattern;

/**
* A helper class which supports extracting a range of lines from a given (multiline) string.
*/
public class MultilineHelper {

/**
* Constructor. Prevents instantiation.
*
*/
private MultilineHelper() {

}

/**
* Extracts a range of lines from the given (multiline) input.
*
* @param input the multiline input
* @param lineInfo lines to extract, given as <code>#L17-L20</code>. <code>null</code> indicates that the whole input
* should be returned.
* @return the extracted lines.
*/
public static String possiblyExtractLines(String input, String lineInfo) {

if (lineInfo == null) {
return input;
}
Pattern pattern = Pattern.compile("#L(\\d+)(-L(\\d+))?");
Matcher matcher = pattern.matcher(lineInfo);
if (matcher.find()) {
int startLine = Integer.parseInt(matcher.group(1));
int endLine = Integer.parseInt(matcher.group(3) != null ? matcher.group(3) : matcher.group(1));
String[] splitted = input.split("\\n");
StringBuffer result = new StringBuffer();
for (int i = 0; i < splitted.length; i++) {
if (i + 1 >= startLine && i + 1 <= endLine) {
result.append(splitted[i]).append("\n");
}
}
return result.toString();
} else {
throw new IllegalStateException("Regex did not find line info - this seems to be a bug.");
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
package com.devonfw.tools.solicitor.common;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

import java.io.File;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

/**
* Tests for {@link IOHelper}.
*
*/
class IOHelperTest {

/**
* @throws java.lang.Exception
*/
@BeforeEach
void setUp() throws Exception {

}

/**
* Test method for
* {@link com.devonfw.tools.solicitor.common.IOHelper#secureFilePath(java.lang.String, java.lang.String[])}.
*/
@Test
void testSecureFilePath() {

assertEquals(fixSep("base"), IOHelper.secureFilePath("base"));
assertEquals(fixSep("base/r1"), IOHelper.secureFilePath("base", "r1"));
assertEquals(fixSep("base/r1/r2"), IOHelper.secureFilePath("base", "r1", "r2"));
assertEquals(fixSep("base/r1/r2"), IOHelper.secureFilePath("base", "r1///", "r2/././"));
assertEquals(fixSep("/base/r1/r2"), IOHelper.secureFilePath("/base", "r1///", "r2/././"));

assertThrows(IllegalArgumentException.class, () -> {
IOHelper.secureFilePath("base", "/r1", "r2");
});

assertThrows(IllegalArgumentException.class, () -> {
IOHelper.secureFilePath("base", "../r1", "r2");
});

assertThrows(IllegalArgumentException.class, () -> {
IOHelper.secureFilePath("base", "r1", "../r2");
});

assertEquals(fixSep("base/r1/r2"), IOHelper.secureFilePath("base", "a/../r1", "r2"));

assertThrows(IllegalArgumentException.class, () -> {
IOHelper.secureFilePath("base", "a/../../r1", "r2");
});
}

/**
* Returns the given strings with all occurrences of <code>/</code> or <code>\\</code> to be replaced by the system
* dependent file separator character. This is required to handle differences between Windows and Unix.
*
* @param input the origin string
* @return the fixed string
*/
private static String fixSep(String input) {

return input.replace('/', File.separatorChar).replace('\\', File.separatorChar);
}

}
Loading

0 comments on commit a38bf81

Please sign in to comment.