diff --git a/core/src/main/java/com/devonfw/tools/solicitor/common/IOHelper.java b/core/src/main/java/com/devonfw/tools/solicitor/common/IOHelper.java index e1bb79c4..cd188014 100644 --- a/core/src/main/java/com/devonfw/tools/solicitor/common/IOHelper.java +++ b/core/src/main/java/com/devonfw/tools/solicitor/common/IOHelper.java @@ -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; @@ -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) */ @@ -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) { @@ -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 null + */ + 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 */ diff --git a/core/src/main/java/com/devonfw/tools/solicitor/common/LogMessages.java b/core/src/main/java/com/devonfw/tools/solicitor/common/LogMessages.java index 7c7a112e..311232ac 100644 --- a/core/src/main/java/com/devonfw/tools/solicitor/common/LogMessages.java +++ b/core/src/main/java/com/devonfw/tools/solicitor/common/LogMessages.java @@ -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; diff --git a/core/src/main/java/com/devonfw/tools/solicitor/common/content/FilesystemCachingContentProvider.java b/core/src/main/java/com/devonfw/tools/solicitor/common/content/FilesystemCachingContentProvider.java index f1b1ee9b..6f6052e3 100644 --- a/core/src/main/java/com/devonfw/tools/solicitor/common/content/FilesystemCachingContentProvider.java +++ b/core/src/main/java/com/devonfw/tools/solicitor/common/content/FilesystemCachingContentProvider.java @@ -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); diff --git a/core/src/main/java/com/devonfw/tools/solicitor/common/content/web/DirectUrlWebContentProvider.java b/core/src/main/java/com/devonfw/tools/solicitor/common/content/web/DirectUrlWebContentProvider.java index c8c075c0..59812f09 100644 --- a/core/src/main/java/com/devonfw/tools/solicitor/common/content/web/DirectUrlWebContentProvider.java +++ b/core/src/main/java/com/devonfw/tools/solicitor/common/content/web/DirectUrlWebContentProvider.java @@ -26,6 +26,9 @@ public class DirectUrlWebContentProvider implements ContentProvider private boolean skipdownload; + private static final Pattern SUPPORTED_URL_PATTERNS = Pattern.compile("^http:.*|^https:.*|^jar:http:.*|^jar:https:.*", + Pattern.CASE_INSENSITIVE); + /** * Constructor. * @@ -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) { @@ -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); @@ -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 true if url is supported, false otherwise. + */ + boolean isSupportedUrl(String url) { + + return SUPPORTED_URL_PATTERNS.matcher(url).matches(); + } + /** * @param input * @param lineInfo diff --git a/core/src/main/java/com/devonfw/tools/solicitor/componentinfo/scancode/FileScancodeRawComponentInfoProvider.java b/core/src/main/java/com/devonfw/tools/solicitor/componentinfo/scancode/FileScancodeRawComponentInfoProvider.java index 990d45bb..c4397c60 100644 --- a/core/src/main/java/com/devonfw/tools/solicitor/componentinfo/scancode/FileScancodeRawComponentInfoProvider.java +++ b/core/src/main/java/com/devonfw/tools/solicitor/componentinfo/scancode/FileScancodeRawComponentInfoProvider.java @@ -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; @@ -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; @@ -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; } @@ -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()) { @@ -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."); @@ -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()) { @@ -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 diff --git a/core/src/main/java/com/devonfw/tools/solicitor/componentinfo/scancode/MultilineHelper.java b/core/src/main/java/com/devonfw/tools/solicitor/componentinfo/scancode/MultilineHelper.java new file mode 100644 index 00000000..60a29ee9 --- /dev/null +++ b/core/src/main/java/com/devonfw/tools/solicitor/componentinfo/scancode/MultilineHelper.java @@ -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 #L17-L20. null 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."); + } + } + +} diff --git a/core/src/test/java/com/devonfw/tools/solicitor/common/IOHelperTest.java b/core/src/test/java/com/devonfw/tools/solicitor/common/IOHelperTest.java new file mode 100644 index 00000000..7010c5be --- /dev/null +++ b/core/src/test/java/com/devonfw/tools/solicitor/common/IOHelperTest.java @@ -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 / or \\ 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); + } + +} diff --git a/core/src/test/java/com/devonfw/tools/solicitor/common/content/web/DirectUrlWebContentProviderTest.java b/core/src/test/java/com/devonfw/tools/solicitor/common/content/web/DirectUrlWebContentProviderTest.java new file mode 100644 index 00000000..72b42c1a --- /dev/null +++ b/core/src/test/java/com/devonfw/tools/solicitor/common/content/web/DirectUrlWebContentProviderTest.java @@ -0,0 +1,35 @@ +package com.devonfw.tools.solicitor.common.content.web; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +/** + * Tests for {@link DirectUrlWebContentProvider} + * + */ +class DirectUrlWebContentProviderTest { + + /** + * Test method for + * {@link com.devonfw.tools.solicitor.common.content.web.DirectUrlWebContentProvider#isSupportedUrl(java.lang.String)}. + */ + @Test + void testIsSupportedUrl() { + + DirectUrlWebContentProvider provider = new DirectUrlWebContentProvider(false); + + assertTrue(provider.isSupportedUrl("http:foo")); + assertTrue(provider.isSupportedUrl("https:foo")); + assertTrue(provider.isSupportedUrl("jar:http:foo")); + assertTrue(provider.isSupportedUrl("jar:https:foo")); + + assertTrue(provider.isSupportedUrl("hTTp:foo")); + + assertFalse(provider.isSupportedUrl("file:some.file")); + assertFalse(provider.isSupportedUrl("file:http:foo")); + + } + +} diff --git a/core/src/test/java/com/devonfw/tools/solicitor/componentinfo/scancode/FilteredScancodeComponentInfoProviderTests.java b/core/src/test/java/com/devonfw/tools/solicitor/componentinfo/scancode/FilteredScancodeComponentInfoProviderTests.java index 0ee9d141..909a37cb 100644 --- a/core/src/test/java/com/devonfw/tools/solicitor/componentinfo/scancode/FilteredScancodeComponentInfoProviderTests.java +++ b/core/src/test/java/com/devonfw/tools/solicitor/componentinfo/scancode/FilteredScancodeComponentInfoProviderTests.java @@ -7,7 +7,6 @@ import org.junit.jupiter.api.Test; import org.mockito.Mockito; -import com.devonfw.tools.solicitor.common.content.web.DirectUrlWebContentProvider; import com.devonfw.tools.solicitor.common.packageurl.AllKindsPackageURLHandler; import com.devonfw.tools.solicitor.componentinfo.ComponentInfo; import com.devonfw.tools.solicitor.componentinfo.ComponentInfoAdapterException; @@ -32,10 +31,8 @@ public void setup() { Mockito.when(packageURLHandler.pathFor("pkg:maven/com.devonfw.tools/test-project-for-deep-license-scan@0.1.0")) .thenReturn("pkg/maven/com/devonfw/tools/test-project-for-deep-license-scan/0.1.0"); - DirectUrlWebContentProvider contentProvider = new DirectUrlWebContentProvider(false); - this.fileScancodeRawComponentInfoProvider = new FileScancodeRawComponentInfoProvider(contentProvider, - packageURLHandler); + this.fileScancodeRawComponentInfoProvider = new FileScancodeRawComponentInfoProvider(packageURLHandler); this.fileScancodeRawComponentInfoProvider.setRepoBasePath("src/test/resources/scancodefileadapter/Source/repo"); this.singleFileCurationProvider = new SingleFileCurationProvider(packageURLHandler); diff --git a/core/src/test/java/com/devonfw/tools/solicitor/componentinfo/scancode/ScancodeComponentInfoAdapterTest.java b/core/src/test/java/com/devonfw/tools/solicitor/componentinfo/scancode/ScancodeComponentInfoAdapterTest.java index 6d62b088..789528a4 100644 --- a/core/src/test/java/com/devonfw/tools/solicitor/componentinfo/scancode/ScancodeComponentInfoAdapterTest.java +++ b/core/src/test/java/com/devonfw/tools/solicitor/componentinfo/scancode/ScancodeComponentInfoAdapterTest.java @@ -13,7 +13,6 @@ import org.mockito.ArgumentCaptor; import org.mockito.Mockito; -import com.devonfw.tools.solicitor.common.content.web.DirectUrlWebContentProvider; import com.devonfw.tools.solicitor.common.packageurl.AllKindsPackageURLHandler; import com.devonfw.tools.solicitor.componentinfo.ComponentInfo; import com.devonfw.tools.solicitor.componentinfo.ComponentInfoAdapterException; @@ -46,10 +45,11 @@ public void setup() { Mockito.when(packageURLHandler.pathFor("pkg:maven/com.devonfw.tools/test-project-for-deep-license-scan@0.1.0")) .thenReturn("pkg/maven/com/devonfw/tools/test-project-for-deep-license-scan/0.1.0"); - DirectUrlWebContentProvider contentProvider = new DirectUrlWebContentProvider(false); - this.fileScancodeRawComponentInfoProvider = new FileScancodeRawComponentInfoProvider(contentProvider, - packageURLHandler); + Mockito.when(packageURLHandler.pathFor("pkg:maven/com.devonfw.tools/unknown@0.1.0")) + .thenReturn("pkg/maven/com/devonfw/tools/unknown/0.1.0"); + + this.fileScancodeRawComponentInfoProvider = new FileScancodeRawComponentInfoProvider(packageURLHandler); this.fileScancodeRawComponentInfoProvider.setRepoBasePath("src/test/resources/scancodefileadapter/Source/repo"); this.singleFileCurationProvider = new SingleFileCurationProvider(packageURLHandler); @@ -70,7 +70,8 @@ public void setup() { } /** - * Test the {@link ScancodeComponentInfoAdapter#getComponentInfo(String,String)} method when such package is known. + * Test the {@link ScancodeComponentInfoAdapter#getComponentInfo(String,String)} method when such package is not + * known. * * @throws ComponentInfoAdapterException if something goes wrong */ diff --git a/documentation/master-solicitor.asciidoc b/documentation/master-solicitor.asciidoc index d52a448d..aadad3ff 100644 --- a/documentation/master-solicitor.asciidoc +++ b/documentation/master-solicitor.asciidoc @@ -1699,6 +1699,7 @@ Spring beans implementing this interface will be called at certain points in the [appendix] == Release Notes Changes in 1.21.0:: +* https://github.com/devonfw/solicitor/pull/239: Improving some internal components to reduce risk of path traversal attacks in case that these components are (re)used in some webservice implementation. Changes in 1.20.0:: * https://github.com/devonfw/solicitor/issues/232: Set a standard for ordering LicenseNameMapping rules. Rules with an 'or-later' suffix are put before '-only' rules.