CVE-2018-9159: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') (CWE-22)
value: In Spark before 2.7.2, a remote attacker can read unintended static files via various representations of absolute or relative pathnames, as demonstrated by file: URLs and directory traversal sequences. NOTE: this product is unrelated to Ignite Realtime Spark.
CWE description: The product uses external input to construct a pathname that is intended to identify a file or directory that is located underneath a restricted parent directory, but the product does not properly neutralize special elements within the pathname that can cause the pathname to resolve to a location that is outside of the restricted directory.
CVSS v3 base score: 5.3
Published: 2018-03-31T21:29Z
Repository: https://github.com/perwendel/spark
Language: Java
Files changed: src/main/java/spark/resource/ClassPathResource.java, src/test/java/spark/embeddedserver/jetty/EmbeddedJettyFactoryTest.java, src/test/java/spark/examples/staticresources/StaticResources.java
Commit date: 2018-03-07 14:43:02 +0100
Commit message: Fix for #981, patch 2 (#988)


Diff: diff --git a/src/main/java/spark/resource/ClassPathResource.java b/src/main/java/spark/resource/ClassPathResource.java
index dd8ba97..e4b19f5 100644
--- a/src/main/java/spark/resource/ClassPathResource.java
+++ b/src/main/java/spark/resource/ClassPathResource.java
@@ -21,10 +21,11 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.net.URL;
 
 import spark.utils.Assert;
 import spark.utils.ClassUtils;
+import spark.utils.ResourceUtils;
 import spark.utils.StringUtils;
 
 /**
  * {@link Resource} implementation for class path resources.
  * Uses either a given ClassLoader or a given Class for loading resources.
@@ -72,11 +73,11 @@ public class ClassPathResource extends AbstractFileResolvingResource {
      *                    or {@code null} for the thread context class loader
      * @see ClassLoader#getResourceAsStream(String)
      */
     public ClassPathResource(String path, ClassLoader classLoader) {
         Assert.notNull(path, "Path must not be null");
-        Assert.state(doesNotContainFileColon(path), "Path must not contain 'file:'");
+        Assert.isTrue(isValid(path), "Path is not valid");
 
         String pathToUse = StringUtils.cleanPath(path);
 
         if (pathToUse.startsWith("/")) {
             pathToUse = pathToUse.substring(1);
@@ -84,12 +85,31 @@ public class ClassPathResource extends AbstractFileResolvingResource {
 
         this.path = pathToUse;
         this.classLoader = (classLoader != null ? classLoader : ClassUtils.getDefaultClassLoader());
     }
 
-    private static boolean doesNotContainFileColon(String path) {
-        return !path.contains("file:");
+    private static boolean isValid(final String path) {
+        return !isInvalidPath(path);
+    }
+
+    private static boolean isInvalidPath(String path) {
+        if (path.contains("WEB-INF") || path.contains("META-INF")) {
+            return true;
+        }
+        if (path.contains(":/")) {
+            String relativePath = (path.charAt(0) == '/' ? path.substring(1) : path);
+            if (ResourceUtils.isUrl(relativePath) || relativePath.startsWith("url:")) {
+                return true;
+            }
+        }
+        if (path.contains("")) {
+            path = StringUtils.cleanPath(path);
+            if (path.contains("../")) {
+                return true;
+            }
+        }
+        return false;
     }
 
     /**
      * Create a new ClassPathResource with optional ClassLoader and Class.
      * Only for internal usage.
@@ -234,12 +254,12 @@ public class ClassPathResource extends AbstractFileResolvingResource {
 
             ClassLoader thisLoader = this.classLoader;
             ClassLoader otherLoader = otherRes.classLoader;
 
             return (this.path.equals(otherRes.path) &&
-                thisLoader.equals(otherLoader) &&
-                this.clazz.equals(otherRes.clazz));
+                    thisLoader.equals(otherLoader) &&
+                    this.clazz.equals(otherRes.clazz));
         }
         return false;
     }
 
     /**
diff --git a/src/test/java/spark/embeddedserver/jetty/EmbeddedJettyFactoryTest.java b/src/test/java/spark/embeddedserver/jetty/EmbeddedJettyFactoryTest.java
index bf57529..341bf3d 100644
--- a/src/test/java/spark/embeddedserver/jetty/EmbeddedJettyFactoryTest.java
+++ b/src/test/java/spark/embeddedserver/jetty/EmbeddedJettyFactoryTest.java
@@ -2,33 +2,39 @@ package spark.embeddedserver.jetty;
 
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.util.thread.QueuedThreadPool;
 import org.junit.After;
 import org.junit.Test;
+
 import spark.embeddedserver.EmbeddedServer;
 import spark.route.Routes;
 import spark.staticfiles.StaticFilesConfiguration;
 
-import static org.mockito.Mockito.*;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.mockito.Mockito.when;
 
 public class EmbeddedJettyFactoryTest {
+
     private EmbeddedServer embeddedServer;
 
     @Test
     public void create() throws Exception {
         final JettyServerFactory jettyServerFactory = mock(JettyServerFactory.class);
         final StaticFilesConfiguration staticFilesConfiguration = mock(StaticFilesConfiguration.class);
         final Routes routes = mock(Routes.class);
 
-        when(jettyServerFactory.create(100,10,10000)).thenReturn(new Server());
+        when(jettyServerFactory.create(100, 10, 10000)).thenReturn(new Server());
 
         final EmbeddedJettyFactory embeddedJettyFactory = new EmbeddedJettyFactory(jettyServerFactory);
         embeddedServer = embeddedJettyFactory.create(routes, staticFilesConfiguration, false);
 
-        embeddedServer.ignite("localhost", 8080, null, 100,10,10000);
+        embeddedServer.ignite("localhost", 6757, null, 100, 10, 10000);
 
-        verify(jettyServerFactory, times(1)).create(100,10,10000);
+        verify(jettyServerFactory, times(1)).create(100, 10, 10000);
         verifyNoMoreInteractions(jettyServerFactory);
     }
 
     @Test
     public void create_withThreadPool() throws Exception {
@@ -40,11 +46,11 @@ public class EmbeddedJettyFactoryTest {
         when(jettyServerFactory.create(threadPool)).thenReturn(new Server(threadPool));
 
         final EmbeddedJettyFactory embeddedJettyFactory = new EmbeddedJettyFactory(jettyServerFactory).withThreadPool(threadPool);
         embeddedServer = embeddedJettyFactory.create(routes, staticFilesConfiguration, false);
 
-        embeddedServer.ignite("localhost", 8080, null, 0,0,0);
+        embeddedServer.ignite("localhost", 6758, null, 0, 0, 0);
 
         verify(jettyServerFactory, times(1)).create(threadPool);
         verifyNoMoreInteractions(jettyServerFactory);
     }
 
@@ -52,21 +58,23 @@ public class EmbeddedJettyFactoryTest {
     public void create_withNullThreadPool() throws Exception {
         final JettyServerFactory jettyServerFactory = mock(JettyServerFactory.class);
         final StaticFilesConfiguration staticFilesConfiguration = mock(StaticFilesConfiguration.class);
         final Routes routes = mock(Routes.class);
 
-        when(jettyServerFactory.create(100,10,10000)).thenReturn(new Server());
+        when(jettyServerFactory.create(100, 10, 10000)).thenReturn(new Server());
 
         final EmbeddedJettyFactory embeddedJettyFactory = new EmbeddedJettyFactory(jettyServerFactory).withThreadPool(null);
         embeddedServer = embeddedJettyFactory.create(routes, staticFilesConfiguration, false);
 
-        embeddedServer.ignite("localhost", 8080, null, 100,10,10000);
+        embeddedServer.ignite("localhost", 6759, null, 100, 10, 10000);
 
-        verify(jettyServerFactory, times(1)).create(100,10,10000);
+        verify(jettyServerFactory, times(1)).create(100, 10, 10000);
         verifyNoMoreInteractions(jettyServerFactory);
     }
 
     @After
     public void tearDown() throws Exception {
-        if(embeddedServer != null) embeddedServer.extinguish();
+        if (embeddedServer != null) {
+            embeddedServer.extinguish();
+        }
     }
 }
diff --git a/src/test/java/spark/examples/staticresources/StaticResources.java b/src/test/java/spark/examples/staticresources/StaticResources.java
index cacd93e..0916814 100644
--- a/src/test/java/spark/examples/staticresources/StaticResources.java
+++ b/src/test/java/spark/examples/staticresources/StaticResources.java
@@ -15,21 +15,21 @@
  * limitations under the License.
  */
 package spark.examples.staticresources;
 
 import static spark.Spark.get;
-import static spark.Spark.staticFileLocation;
+import static spark.Spark.staticFiles;
 
 /**
  * Example showing how serve static resources.
  */
 public class StaticResources {
 
     public static void main(String[] args) {
 
         // Will serve all static file are under "/public" in classpath if the route isn't consumed by others routes.
-        staticFileLocation("/public");
+        staticFiles.location("/public");
 
         get("/hello", (request, response) -> {
             return "Hello World!";
         });
     }

Vulnerable code:         Assert.state(doesNotContainFileColon(path), "Path must not contain 'file:'");
    private static boolean doesNotContainFileColon(String path) {
        return !path.contains("file:");
                thisLoader.equals(otherLoader) &&
                this.clazz.equals(otherRes.clazz));
import static org.mockito.Mockito.*;
        when(jettyServerFactory.create(100,10,10000)).thenReturn(new Server());
        embeddedServer.ignite("localhost", 8080, null, 100,10,10000);
        verify(jettyServerFactory, times(1)).create(100,10,10000);
        embeddedServer.ignite("localhost", 8080, null, 0,0,0);
        when(jettyServerFactory.create(100,10,10000)).thenReturn(new Server());
        embeddedServer.ignite("localhost", 8080, null, 100,10,10000);
        verify(jettyServerFactory, times(1)).create(100,10,10000);
        if(embeddedServer != null) embeddedServer.extinguish();
import static spark.Spark.staticFileLocation;
        staticFileLocation("/public");
Fixed code: import spark.utils.ResourceUtils;
        Assert.isTrue(isValid(path), "Path is not valid");
    private static boolean isValid(final String path) {
        return !isInvalidPath(path);
    }

    private static boolean isInvalidPath(String path) {
        if (path.contains("WEB-INF") || path.contains("META-INF")) {
            return true;
        }
        if (path.contains(":/")) {
            String relativePath = (path.charAt(0) == '/' ? path.substring(1) : path);
            if (ResourceUtils.isUrl(relativePath) || relativePath.startsWith("url:")) {
                return true;
            }
        }
        if (path.contains("")) {
            path = StringUtils.cleanPath(path);
            if (path.contains("../")) {
                return true;
            }
        }
        return false;
                    thisLoader.equals(otherLoader) &&
                    this.clazz.equals(otherRes.clazz));

import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;

        when(jettyServerFactory.create(100, 10, 10000)).thenReturn(new Server());
        embeddedServer.ignite("localhost", 6757, null, 100, 10, 10000);
        verify(jettyServerFactory, times(1)).create(100, 10, 10000);
        embeddedServer.ignite("localhost", 6758, null, 0, 0, 0);
        when(jettyServerFactory.create(100, 10, 10000)).thenReturn(new Server());
        embeddedServer.ignite("localhost", 6759, null, 100, 10, 10000);
        verify(jettyServerFactory, times(1)).create(100, 10, 10000);
        if (embeddedServer != null) {
            embeddedServer.extinguish();
        }
import static spark.Spark.staticFiles;
        staticFiles.location("/public");