Skip to content

Commit

Permalink
Reject download of resource which are not in the cache folder (url which
Browse files Browse the repository at this point in the history 
uses several ../../).

Signed-off-by: azerr <[email protected]>
  • Loading branch information
@angelozerr
angelozerr committed Oct 7, 2019
1 parent 56fc4a8 commit e37c399
Show file tree
Hide file tree
Showing 5 changed files with 103 additions and 53 deletions.
51 changes: 30 additions & 21 deletions org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/services/XMLLanguageService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,27 +161,36 @@ public CompletableFuture<Path> 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<Path> 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;
}
}

Expand Down
10 changes: 9 additions & 1 deletion ...4xml/src/main/java/org/eclipse/lsp4xml/uriresolver/CacheResourceDownloadingException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Path> 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<Path> future) {
super(MessageFormat.format(RESOURCE_LOADING_MSG, resourceURI));
this.resourceURI = resourceURI;
Expand Down
16 changes: 9 additions & 7 deletions org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/uriresolver/CacheResourcesManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -161,7 +162,8 @@ private CompletableFuture<Path> 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);
Expand Down
46 changes: 31 additions & 15 deletions org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/utils/FilesUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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
Expand All @@ -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;

Expand All @@ -201,16 +214,14 @@ public static Path getNormalizedPath(String parentDirectory, String givenPath) {
return p;
}



if (parentDirectory == null) {
return null;
}

if (parentDirectory.endsWith(SLASH)) {
parentDirectory = parentDirectory.substring(0, parentDirectory.length() - 1);
}

String combinedPath = parentDirectory + SLASH + givenPath;
p = getPathIfExists(combinedPath);
if (p != null) {
Expand All @@ -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
*/
Expand All @@ -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);
}
}
Expand All @@ -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());
}
}
33 changes: 24 additions & 9 deletions ...ipse.lsp4xml/src/test/java/org/eclipse/lsp4xml/uriresolver/CacheResourcesManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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");
Expand All @@ -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<String, Boolean> testingCache() {
return CacheBuilder.newBuilder().expireAfterWrite(1, TimeUnit.SECONDS).maximumSize(1).build();
}
Expand Down

0 comments on commit e37c399

Please sign in to comment.