From e37c399aa266be1b7a43061d4afc43dc230410d2 Mon Sep 17 00:00:00 2001 From: azerr Date: Fri, 4 Oct 2019 18:08:08 +0200 Subject: [PATCH] Reject download of resource which are not in the cache folder (url which uses several ../../). Signed-off-by: azerr --- .../lsp4xml/services/XMLLanguageService.java | 51 +++++++++++-------- .../CacheResourceDownloadingException.java | 10 +++- .../uriresolver/CacheResourcesManager.java | 16 +++--- .../org/eclipse/lsp4xml/utils/FilesUtils.java | 46 +++++++++++------ .../CacheResourcesManagerTest.java | 33 ++++++++---- 5 files changed, 103 insertions(+), 53 deletions(-) diff --git a/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/services/XMLLanguageService.java b/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/services/XMLLanguageService.java index ccfdeaabf..59174de94 100644 --- a/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/services/XMLLanguageService.java +++ b/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/services/XMLLanguageService.java @@ -161,27 +161,36 @@ public CompletableFuture publishDiagnostics(DOMDocument xmlDocument, publishDiagnostics.accept(new PublishDiagnosticsParams(uri, diagnostics)); return null; } catch (CacheResourceDownloadingException e) { - // An XML Schema or DTD is being downloaded by the cache manager, but it takes - // too long. - // In this case: - // - 1) we add an information message to the document element to explain that - // validation - // cannot be performed because the XML Schema/DTD is downloading. - publishOneDiagnosticInRoot(xmlDocument, e.getMessage(), DiagnosticSeverity.Information, publishDiagnostics); - // - 2) we restart the validation only once the XML Schema/DTD is downloaded. - e.getFuture() // - .exceptionally(downloadException -> { - // Error while downloading the XML Schema/DTD - publishOneDiagnosticInRoot(xmlDocument, downloadException.getCause().getMessage(), - DiagnosticSeverity.Error, publishDiagnostics); - return null; - }) // - .thenAccept((path) -> { - if (path != null) { - triggerValidation.accept(document); - } - }); - return e.getFuture(); + CompletableFuture future = e.getFuture(); + if (future == null) { + // This case comes from when URL uses ../../ and resources is not included in + // the cache path + // To prevent from "Path Traversal leading to Remote Command Execution (RCE)" + publishOneDiagnosticInRoot(xmlDocument, e.getMessage(), DiagnosticSeverity.Error, publishDiagnostics); + } else { + // An XML Schema or DTD is being downloaded by the cache manager, but it takes + // too long. + // In this case: + // - 1) we add an information message to the document element to explain that + // validation + // cannot be performed because the XML Schema/DTD is downloading. + publishOneDiagnosticInRoot(xmlDocument, e.getMessage(), DiagnosticSeverity.Information, + publishDiagnostics); + // - 2) we restart the validation only once the XML Schema/DTD is downloaded. + future // + .exceptionally(downloadException -> { + // Error while downloading the XML Schema/DTD + publishOneDiagnosticInRoot(xmlDocument, downloadException.getCause().getMessage(), + DiagnosticSeverity.Error, publishDiagnostics); + return null; + }) // + .thenAccept((path) -> { + if (path != null) { + triggerValidation.accept(document); + } + }); + } + return future; } } diff --git a/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/uriresolver/CacheResourceDownloadingException.java b/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/uriresolver/CacheResourceDownloadingException.java index 8ad5d8ba9..816172dc0 100644 --- a/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/uriresolver/CacheResourceDownloadingException.java +++ b/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/uriresolver/CacheResourceDownloadingException.java @@ -24,10 +24,18 @@ public class CacheResourceDownloadingException extends RuntimeException { private static final String RESOURCE_LOADING_MSG = "The resource ''{0}'' is downloading."; + private static final String RESOURCE_NOT_IN_DEPLOYED_PATH_MSG = "The resource ''{0}'' cannot be downloaded in the cache path."; + private final String resourceURI; private final CompletableFuture future; - + + public CacheResourceDownloadingException(String resourceURI) { + super(MessageFormat.format(RESOURCE_NOT_IN_DEPLOYED_PATH_MSG, resourceURI)); + this.resourceURI = resourceURI; + this.future = null; + } + public CacheResourceDownloadingException(String resourceURI, CompletableFuture future) { super(MessageFormat.format(RESOURCE_LOADING_MSG, resourceURI)); this.resourceURI = resourceURI; diff --git a/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/uriresolver/CacheResourcesManager.java b/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/uriresolver/CacheResourcesManager.java index 4b03b97f0..adde04967 100644 --- a/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/uriresolver/CacheResourcesManager.java +++ b/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/uriresolver/CacheResourcesManager.java @@ -31,13 +31,12 @@ import java.util.logging.Level; import java.util.logging.Logger; -import com.google.common.cache.Cache; -import com.google.common.cache.CacheBuilder; - - import org.eclipse.lsp4xml.utils.FilesUtils; import org.eclipse.lsp4xml.utils.URIUtils; +import com.google.common.cache.Cache; +import com.google.common.cache.CacheBuilder; + /** * Cache resources manager. * @@ -98,8 +97,10 @@ public Path getResource(final String resourceURI) throws IOException { if (Files.exists(resourceCachePath)) { return resourceCachePath; } - - if(unavailableURICache.getIfPresent(resourceURI) != null) { + if (!FilesUtils.isIncludedInDeployedPath(resourceCachePath)) { + throw new CacheResourceDownloadingException(resourceURI); + } + if (unavailableURICache.getIfPresent(resourceURI) != null) { LOGGER.info("Ignored unavailable schema URI: " + resourceURI + "\n"); return null; } @@ -161,7 +162,8 @@ private CompletableFuture downloadResource(final String resourceURI, Path String error = "[" + rootCause.getClass().getTypeName() + "] " + rootCause.getMessage(); LOGGER.log(Level.SEVERE, "Error while downloading " + resourceURI + " to " + resourceCachePath + " : " + error); - throw new CacheResourceDownloadedException("Error while downloading '" + resourceURI + "' to " + resourceCachePath + ".", e); + throw new CacheResourceDownloadedException( + "Error while downloading '" + resourceURI + "' to " + resourceCachePath + ".", e); } finally { synchronized (resourcesLoading) { resourcesLoading.remove(resourceURI); diff --git a/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/utils/FilesUtils.java b/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/utils/FilesUtils.java index 7d32fac11..3251d1f30 100644 --- a/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/utils/FilesUtils.java +++ b/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/utils/FilesUtils.java @@ -20,6 +20,9 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.nio.file.attribute.PosixFilePermissions; +import java.util.logging.Level; +import java.util.logging.Logger; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -32,6 +35,8 @@ */ public class FilesUtils { + private static final Logger LOGGER = Logger.getLogger(FilesUtils.class.getName()); + public static final String FILE_SCHEME = "file://"; public static final String LSP4XML_WORKDIR_KEY = "lsp4xml.workdir"; private static String cachePathSetting = null; @@ -143,6 +148,15 @@ public static void saveToFile(String content, Path outFile) throws IOException { try (Writer writer = Files.newBufferedWriter(outFile, StandardCharsets.UTF_8)) { writer.write(content); } + // Make sure it's not executable + try { + Files.setPosixFilePermissions(outFile, PosixFilePermissions.fromString("rw-r-----")); + } catch (UnsupportedOperationException e) { + // The associated file system does not support the PosixFileAttributeView, + // ignore the error + LOGGER.log(Level.SEVERE, + "The associated file system '" + outFile + "' + does not support the PosixFileAttributeView", e); + } } public static int getOffsetAfterScheme(String uri) { @@ -162,7 +176,8 @@ public static int getOffsetAfterScheme(String uri) { * the path to that, else it will try to find the parent directory of the * givenPath. * - * **IMPORTANT** The slashes of the given paths have to match the supported OS file path slash + * **IMPORTANT** The slashes of the given paths have to match the supported OS + * file path slash * * @param parentDirectory The directory that the given path is relative to * @param givenPath Path that could be absolute or relative @@ -178,13 +193,11 @@ public static Path getNormalizedPath(String parentDirectory, String givenPath) { // in case the given path is incomplete, trim the end String givenPathCleaned; - if(lastIndexOfSlash == 0) { // Looks like `/someFileOrFolder` + if (lastIndexOfSlash == 0) { // Looks like `/someFileOrFolder` return Paths.get(SLASH); - } - else { + } else { givenPathCleaned = lastIndexOfSlash > -1 ? givenPath.substring(0, lastIndexOfSlash) : null; } - Path p; @@ -201,8 +214,6 @@ public static Path getNormalizedPath(String parentDirectory, String givenPath) { return p; } - - if (parentDirectory == null) { return null; } @@ -210,7 +221,7 @@ public static Path getNormalizedPath(String parentDirectory, String givenPath) { if (parentDirectory.endsWith(SLASH)) { parentDirectory = parentDirectory.substring(0, parentDirectory.length() - 1); } - + String combinedPath = parentDirectory + SLASH + givenPath; p = getPathIfExists(combinedPath); if (p != null) { @@ -237,8 +248,9 @@ private static Path getPathIfExists(String path) { } /** - * Returns the slash ("/" or "\") that is used by the given string. - * If no slash is given "/" is returned by default. + * Returns the slash ("/" or "\") that is used by the given string. If no slash + * is given "/" is returned by default. + * * @param text * @return */ @@ -250,18 +262,18 @@ public static String getFilePathSlash(String text) { } /** - * Ensures there is no slash before a drive letter, and - * forces use of '\' + * Ensures there is no slash before a drive letter, and forces use of '\' + * * @param pathString * @return */ public static String convertToWindowsPath(String pathString) { String pathSlash = getFilePathSlash(pathString); - if(pathString.startsWith(pathSlash) ) { - if(pathString.length() > 3) { + if (pathString.startsWith(pathSlash)) { + if (pathString.length() > 3) { char letter = pathString.charAt(1); char colon = pathString.charAt(2); - if(Character.isLetter(letter) && ':' == colon) { + if (Character.isLetter(letter) && ':' == colon) { pathString = pathString.substring(1); } } @@ -273,4 +285,8 @@ public static boolean pathEndsWithFile(String pathString) { Matcher m = endFilePattern.matcher(pathString); return m.matches(); } + + public static boolean isIncludedInDeployedPath(Path resourceCachePath) { + return resourceCachePath.normalize().startsWith(DEPLOYED_BASE_PATH.get()); + } } diff --git a/org.eclipse.lsp4xml/src/test/java/org/eclipse/lsp4xml/uriresolver/CacheResourcesManagerTest.java b/org.eclipse.lsp4xml/src/test/java/org/eclipse/lsp4xml/uriresolver/CacheResourcesManagerTest.java index 848e0a361..f56c881ee 100644 --- a/org.eclipse.lsp4xml/src/test/java/org/eclipse/lsp4xml/uriresolver/CacheResourcesManagerTest.java +++ b/org.eclipse.lsp4xml/src/test/java/org/eclipse/lsp4xml/uriresolver/CacheResourcesManagerTest.java @@ -16,23 +16,24 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.fail; +import java.io.IOException; import java.util.concurrent.TimeUnit; -import com.google.common.cache.Cache; -import com.google.common.cache.CacheBuilder; - import org.eclipse.lsp4xml.AbstractCacheBasedTest; import org.junit.After; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import com.google.common.cache.Cache; +import com.google.common.cache.CacheBuilder; + public class CacheResourcesManagerTest extends AbstractCacheBasedTest { private CacheResourcesManager cacheResourcesManager; private FileServer server; - @Before public void setup() throws Exception { cacheResourcesManager = new CacheResourcesManager(testingCache()); @@ -71,12 +72,12 @@ public void testUnavailableCache() throws Exception { } catch (CacheResourceDownloadingException ignored) { } TimeUnit.MILLISECONDS.sleep(200); - //failed to download so returns null + // failed to download so returns null assertNull(cacheResourcesManager.getResource(uri)); - TimeUnit.SECONDS.sleep(1);//wait past the cache expiration date + TimeUnit.SECONDS.sleep(1);// wait past the cache expiration date - //Manager should retry downloading + // Manager should retry downloading try { cacheResourcesManager.getResource(uri); fail("cacheResourcesManager should be busy re-downloading the url"); @@ -98,12 +99,26 @@ public void testAvailableCache() throws Exception { assertNotNull(cacheResourcesManager.getResource(uri)); server.stop(); - TimeUnit.SECONDS.sleep(1);//wait past the cache expiration date + TimeUnit.SECONDS.sleep(1);// wait past the cache expiration date - //Manager should return cached content, even if server is offline + // Manager should return cached content, even if server is offline assertNotNull(cacheResourcesManager.getResource(uri)); } + @Test + public void testGetBadResource() throws IOException { + CacheResourceDownloadingException actual = null; + try { + cacheResourcesManager.getResource("http://localhost/../../../../../test.txt"); + } catch (CacheResourceDownloadingException e) { + actual = e; + } + Assert.assertNotNull(actual); + Assert.assertEquals( + "The resource 'http://localhost/../../../../../test.txt' cannot be downloaded in the cache path.", + actual.getMessage()); + } + private Cache testingCache() { return CacheBuilder.newBuilder().expireAfterWrite(1, TimeUnit.SECONDS).maximumSize(1).build(); }