diff --git a/flow-server/src/main/java/com/vaadin/flow/server/DevModeHandler.java b/flow-server/src/main/java/com/vaadin/flow/server/DevModeHandler.java index f1b22dcbeea..0237eddd3ee 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/DevModeHandler.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/DevModeHandler.java @@ -305,6 +305,13 @@ public boolean serveDevModeRequest(HttpServletRequest request, // a valid request for webpack-dev-server should start with '/VAADIN/' String requestFilename = request.getPathInfo(); + if (HandlerHelper.isPathUnsafe(requestFilename)) { + getLogger().info(HandlerHelper.UNSAFE_PATH_ERROR_MESSAGE_PATTERN, + requestFilename); + response.setStatus(HttpServletResponse.SC_FORBIDDEN); + return true; + } + HttpURLConnection connection = prepareConnection(requestFilename, request.getMethod()); diff --git a/flow-server/src/main/java/com/vaadin/flow/server/HandlerHelper.java b/flow-server/src/main/java/com/vaadin/flow/server/HandlerHelper.java index caa553a3744..ebb564cb6cd 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/HandlerHelper.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/HandlerHelper.java @@ -16,8 +16,12 @@ package com.vaadin.flow.server; import java.io.Serializable; +import java.io.UnsupportedEncodingException; +import java.net.URLDecoder; +import java.nio.charset.StandardCharsets; import java.util.Locale; import java.util.function.BiConsumer; +import java.util.regex.Pattern; import com.vaadin.flow.component.UI; import com.vaadin.flow.shared.ApplicationConstants; @@ -35,6 +39,15 @@ public class HandlerHelper implements Serializable { */ static final SystemMessages DEFAULT_SYSTEM_MESSAGES = new SystemMessages(); + /** + * The pattern of error message shown when the URL path contains unsafe + * double encoding. + */ + static final String UNSAFE_PATH_ERROR_MESSAGE_PATTERN = "Blocked attempt to access file: {}"; + + private static final Pattern PARENT_DIRECTORY_REGEX = Pattern + .compile("(/|\\\\)\\.\\.(/|\\\\)?", Pattern.CASE_INSENSITIVE); + /** * Framework internal enum for tracking the type of a request. */ @@ -176,4 +189,26 @@ public static String getCancelingRelativePath(String pathToCancel) { return sb.toString(); } + /** + * Checks if the given URL path contains the directory change instruction + * (dot-dot), taking into account possible double encoding in hexadecimal + * format, which can be injected maliciously. + * + * @param path + * the URL path to be verified. + * @return {@code true}, if the given path has a directory change + * instruction, {@code false} otherwise. + */ + public static boolean isPathUnsafe(String path) { + // Check that the path does not have '/../', '\..\', %5C..%5C, + // %2F..%2F, nor '/..', '\..', %5C.., %2F.. + try { + path = URLDecoder.decode(path, StandardCharsets.UTF_8.name()); + } catch (UnsupportedEncodingException e) { + throw new RuntimeException("An error occurred during decoding URL.", + e); + } + return PARENT_DIRECTORY_REGEX.matcher(path).find(); + } + } diff --git a/flow-server/src/main/java/com/vaadin/flow/server/StaticFileServer.java b/flow-server/src/main/java/com/vaadin/flow/server/StaticFileServer.java index dc97dd7cadd..fc22465361b 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/StaticFileServer.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/StaticFileServer.java @@ -20,11 +20,8 @@ import java.io.IOException; import java.io.InputStream; -import java.io.UnsupportedEncodingException; import java.net.URL; import java.net.URLConnection; -import java.net.URLDecoder; -import java.nio.charset.StandardCharsets; import java.util.regex.Pattern; import org.slf4j.Logger; @@ -55,8 +52,6 @@ public class StaticFileServer implements StaticFileHandler { + "fixIncorrectWebjarPaths"; private static final Pattern INCORRECT_WEBJAR_PATH_REGEX = Pattern .compile("^/frontend[-\\w/]*/webjars/"); - private static final Pattern PARENT_DIRECTORY_REGEX = Pattern - .compile("(/|\\\\)\\.\\.(/|\\\\)", Pattern.CASE_INSENSITIVE); private final ResponseWriter responseWriter; private final VaadinServletService servletService; @@ -108,10 +103,10 @@ public boolean serveStaticResource(HttpServletRequest request, HttpServletResponse response) throws IOException { String filenameWithPath = getRequestFilename(request); - if (!isPathSafe(filenameWithPath)) { - getLogger().info("Blocked attempt to access file: {}", + if (HandlerHelper.isPathUnsafe(filenameWithPath)) { + getLogger().info(HandlerHelper.UNSAFE_PATH_ERROR_MESSAGE_PATTERN, filenameWithPath); - response.setStatus(HttpServletResponse.SC_NOT_MODIFIED); + response.setStatus(HttpServletResponse.SC_FORBIDDEN); return true; } @@ -190,18 +185,6 @@ private String fixIncorrectWebjarPath(String requestFilename) { .replaceAll("/webjars/"); } - private boolean isPathSafe(String path) { - // Check that the path does not have '/../', '\..\', %5C..%5C, or - // %2F..%2F - try { - path = URLDecoder.decode(path, StandardCharsets.UTF_8.name()); - } catch (UnsupportedEncodingException e) { - throw new RuntimeException("An error occurred during decoding URL.", - e); - } - return !PARENT_DIRECTORY_REGEX.matcher(path).find(); - } - /** * Check if it is ok to serve the requested file from the classpath. *

diff --git a/flow-server/src/test/java/com/vaadin/flow/server/DevModeHandlerTest.java b/flow-server/src/test/java/com/vaadin/flow/server/DevModeHandlerTest.java index a40eea53738..eef05383455 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/DevModeHandlerTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/DevModeHandlerTest.java @@ -63,6 +63,7 @@ import static com.vaadin.flow.server.InitParameters.SERVLET_PARAMETER_DEVMODE_WEBPACK_TIMEOUT; import static com.vaadin.flow.server.frontend.NodeUpdateTestUtil.WEBPACK_TEST_OUT_FILE; import static com.vaadin.flow.server.frontend.NodeUpdateTestUtil.createStubWebpackServer; +import static java.net.HttpURLConnection.HTTP_FORBIDDEN; import static java.net.HttpURLConnection.HTTP_NOT_FOUND; import static java.net.HttpURLConnection.HTTP_NOT_MODIFIED; import static java.net.HttpURLConnection.HTTP_OK; @@ -580,6 +581,74 @@ public void devModeNotReady_handleRequest_returnsHtml() throws Exception { Mockito.verify(response).setContentType("text/html;charset=utf-8"); } + @Test + public void serveDevModeRequest_uriWithDirectoryChangeWithSlash_returnsImmediatelyAndSetsForbiddenStatus() + throws IOException { + verifyServeDevModeRequestReturnsTrueAndSetsProperStatusCode( + "/VAADIN/build/../vaadin-bundle-1234.cache.js"); + } + + @Test + public void serveDevModeRequest_uriWithDirectoryChangeWithBackslash_returnsImmediatelyAndSetsForbiddenStatus() + throws IOException { + verifyServeDevModeRequestReturnsTrueAndSetsProperStatusCode( + "/VAADIN/build/something\\..\\vaadin-bundle-1234.cache.js"); + } + + @Test + public void serveDevModeRequest_uriWithDirectoryChangeWithEncodedBackslashUpperCase_returnsImmediatelyAndSetsForbiddenStatus() + throws IOException { + verifyServeDevModeRequestReturnsTrueAndSetsProperStatusCode( + "/VAADIN/build/something%5C..%5Cvaadin-bundle-1234.cache.js"); + } + + @Test + public void serveDevModeRequest_uriWithDirectoryChangeWithEncodedBackslashLowerCase_returnsImmediatelyAndSetsForbiddenStatus() + throws IOException { + verifyServeDevModeRequestReturnsTrueAndSetsProperStatusCode( + "/VAADIN/build/something%5c..%5cvaadin-bundle-1234.cache.js"); + } + + @Test + public void serveDevModeRequest_uriWithDirectoryChangeInTheEndWithSlash_returnsImmediatelyAndSetsForbiddenStatus() + throws IOException { + verifyServeDevModeRequestReturnsTrueAndSetsProperStatusCode( + "/VAADIN/build/.."); + } + + @Test + public void serveDevModeRequest_uriWithDirectoryChangeInTheEndWithBackslash_returnsImmediatelyAndSetsForbiddenStatus() + throws IOException { + verifyServeDevModeRequestReturnsTrueAndSetsProperStatusCode( + "/VAADIN/build/something\\.."); + } + + @Test + public void serveDevModeRequest_uriWithDirectoryChangeInTheEndWithEncodedBackslashUpperCase_returnsImmediatelyAndSetsForbiddenStatus() + throws IOException { + verifyServeDevModeRequestReturnsTrueAndSetsProperStatusCode( + "/VAADIN/build/something%5C.."); + } + + @Test + public void serveDevModeRequest_uriWithDirectoryChangeInTheEndWithEncodedBackslashLowerCase_returnsImmediatelyAndSetsForbiddenStatus() + throws IOException { + verifyServeDevModeRequestReturnsTrueAndSetsProperStatusCode( + "/VAADIN/build/something%5c.."); + } + + private void verifyServeDevModeRequestReturnsTrueAndSetsProperStatusCode( + String uri) throws IOException { + HttpServletRequest request = prepareRequest(uri); + HttpServletResponse response = prepareResponse(); + DevModeHandler handler = DevModeHandler.start(configuration, npmFolder, + CompletableFuture.completedFuture(null)); + handler.join(); + assertTrue(handler.serveDevModeRequest(request, response)); + + Assert.assertEquals(HTTP_FORBIDDEN, responseStatus); + } + private VaadinServlet prepareServlet(int port) throws ServletException, IOException { DevModeHandler.start(port, configuration, npmFolder, diff --git a/flow-server/src/test/java/com/vaadin/flow/server/StaticFileServerTest.java b/flow-server/src/test/java/com/vaadin/flow/server/StaticFileServerTest.java index 651df1616dd..b9f683ede3d 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/StaticFileServerTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/StaticFileServerTest.java @@ -537,36 +537,66 @@ private void staticBuildResourceWithDirectoryChange_nothingServed( Assert.assertTrue(fileServer.serveStaticResource(request, response)); Assert.assertEquals(0, out.getOutput().length); + Assert.assertEquals(HttpServletResponse.SC_FORBIDDEN, + responseCode.get()); } @Test - public void staticBuildResourceWithDirectoryChangeWithSlash_nothingServed() + public void serveStaticResource_uriWithDirectoryChangeWithSlash_returnsImmediatelyAndSetsForbiddenStatus() throws IOException { staticBuildResourceWithDirectoryChange_nothingServed( "/VAADIN/build/../vaadin-bundle-1234.cache.js"); } @Test - public void staticBuildResourceWithDirectoryChangeWithBackslash_nothingServed() + public void serveStaticResource_uriWithDirectoryChangeWithBackslash_returnsImmediatelyAndSetsForbiddenStatus() throws IOException { staticBuildResourceWithDirectoryChange_nothingServed( "/VAADIN/build/something\\..\\vaadin-bundle-1234.cache.js"); } @Test - public void staticBuildResourceWithDirectoryChangeWithEncodedBackslashUpperCase_nothingServed() + public void serveStaticResource_uriWithDirectoryChangeWithEncodedBackslashUpperCase_returnsImmediatelyAndSetsForbiddenStatus() throws IOException { staticBuildResourceWithDirectoryChange_nothingServed( "/VAADIN/build/something%5C..%5Cvaadin-bundle-1234.cache.js"); } @Test - public void staticBuildResourceWithDirectoryChangeWithEncodedBackslashLowerCase_nothingServed() + public void serveStaticResource_uriWithDirectoryChangeWithEncodedBackslashLowerCase_returnsImmediatelyAndSetsForbiddenStatus() throws IOException { staticBuildResourceWithDirectoryChange_nothingServed( "/VAADIN/build/something%5c..%5cvaadin-bundle-1234.cache.js"); } + @Test + public void serveStaticResource_uriWithDirectoryChangeInTheEndWithSlash_returnsImmediatelyAndSetsForbiddenStatus() + throws IOException { + staticBuildResourceWithDirectoryChange_nothingServed( + "/VAADIN/build/.."); + } + + @Test + public void serveStaticResource_uriWithDirectoryChangeInTheEndWithBackslash_returnsImmediatelyAndSetsForbiddenStatus() + throws IOException { + staticBuildResourceWithDirectoryChange_nothingServed( + "/VAADIN/build/something\\.."); + } + + @Test + public void serveStaticResource_uriWithDirectoryChangeInTheEndWithEncodedBackslashUpperCase_returnsImmediatelyAndSetsForbiddenStatus() + throws IOException { + staticBuildResourceWithDirectoryChange_nothingServed( + "/VAADIN/build/something%5C.."); + } + + @Test + public void serveStaticResource_uriWithDirectoryChangeInTheEndWithEncodedBackslashLowerCase_returnsImmediatelyAndSetsForbiddenStatus() + throws IOException { + staticBuildResourceWithDirectoryChange_nothingServed( + "/VAADIN/build/something%5c.."); + } + @Test public void customStaticBuildResource_isServed() throws IOException { String pathInfo = "/VAADIN/build/my-text.txt"; diff --git a/flow-tests/test-dev-mode/src/test/java/com/vaadin/flow/uitest/ui/UrlValidationIT.java b/flow-tests/test-dev-mode/src/test/java/com/vaadin/flow/uitest/ui/UrlValidationIT.java new file mode 100644 index 00000000000..a32585a1a12 --- /dev/null +++ b/flow-tests/test-dev-mode/src/test/java/com/vaadin/flow/uitest/ui/UrlValidationIT.java @@ -0,0 +1,50 @@ +/* + * Copyright 2000-2020 Vaadin Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +package com.vaadin.flow.uitest.ui; + +import java.io.IOException; +import java.net.HttpURLConnection; +import java.net.URL; + +import com.vaadin.flow.testutil.ChromeBrowserTest; +import org.junit.Assert; +import org.junit.Test; + +public class UrlValidationIT extends ChromeBrowserTest { + + @Test + public void devModeUriValidation_uriWithDirectoryChange_statusForbidden() throws IOException { + sendRequestAndValidateResponseStatusForbidden( + "/VAADIN/build/%252E%252E/"); + } + + @Test + public void staticResourceUriValidation_uriWithDirectoryChange_statusForbidden() throws IOException { + sendRequestAndValidateResponseStatusForbidden( + "/VAADIN/build/%252E%252E/some-resource.css"); + } + + private void sendRequestAndValidateResponseStatusForbidden(String pathToResource) throws IOException { + final String urlString = getRootURL() + "/view" + pathToResource; + URL url = new URL(urlString); + HttpURLConnection connection = (HttpURLConnection) url.openConnection(); + connection.setRequestMethod("GET"); + int responseCode = connection.getResponseCode(); + Assert.assertEquals("HTTP 403 Forbidden expected for urls with " + + "directory change", HttpURLConnection.HTTP_FORBIDDEN, responseCode); + } +}