Enforce directory path transversal issue. Issue #1332 (#1415)

* Enforce directory path transversal issue. Issue #1332

* Added unit test case

* Fixed remaining issues under Windows

 - Fixed detection logic for directory inclusion check
 - toString for Directory now indicates the root URI

---------

Co-authored-by: Jerome Louvel <[email protected]>

Author: thboileau

modules/org.restlet.test/src/main/java/org/restlet/test/data/ReferenceTestCase.java

@@ -345,6 +345,10 @@ public void testParsing() {
                 "/path", null, "frag/ment");
         testRef0("http://localhost/path?qu/ery", "http", "localhost", "/path",
                 "qu/ery", null);
+        testRef0("file:///path/to/file", "file", "", "/path/to/file",
+                null, null);
+        testRef0("file:///c:/path/to/file", "file", "", "/c:/path/to/file",
+                null, null);
 
         // Test the resolution of relative references
         testRef1(base, uri01, uri101);

modules/org.restlet.test/src/main/java/org/restlet/test/resource/DirectoryTestCase.java

@@ -62,6 +62,7 @@
 import org.restlet.engine.header.HeaderConstants;
 import org.restlet.engine.io.IoUtils;
 import org.restlet.engine.util.ReferenceUtils;
+import org.restlet.engine.util.SystemUtils;
 import org.restlet.representation.StringRepresentation;
 import org.restlet.resource.Directory;
 import org.restlet.test.RestletTestCase;
@@ -410,6 +411,34 @@ private void testParentDirectoryInaccessible(MyApplication application, Director
                 .baseRef(this.webSiteURL)
                 .handle(GET);
         assertEquals(CLIENT_ERROR_FORBIDDEN, response.getStatus());
+
+        response = new TestRequest(this.webSiteURL, "%2e%2e%2fprivate.txt")
+                .baseRef(this.webSiteURL)
+                .handle(DELETE);
+        assertEquals(CLIENT_ERROR_FORBIDDEN, response.getStatus());
+
+        if (SystemUtils.isWindows()) {
+            // Try with Windows "\" separator in the URL
+            response = new TestRequest(this.webSiteURL, "..%5cchild%20dir%5cfile.txt")
+                    .baseRef(this.webSiteURL)
+                    .handle(GET);
+            // assert no content as the file is empty
+            assertEquals(SUCCESS_NO_CONTENT, response.getStatus());
+
+            response = new TestRequest(this.webSiteURL, "..%5c..%5c..%5cWindows%5cnotepad.exe")
+                    .baseRef(this.webSiteURL)
+                    .handle(GET);
+            assertEquals(CLIENT_ERROR_FORBIDDEN, response.getStatus());
+        }
+
+        directory.setModifiable(true);
+        response = new TestRequest(this.webSiteURL, "../child%20dir/file.txt")
+                .baseRef(this.webSiteURL)
+                .handle(DELETE);
+
+        // assert file is deleted
+        assertEquals(Status.SUCCESS_NO_CONTENT, response.getStatus());
+
     }
 
     /**

modules/org.restlet/src/main/java/org/restlet/data/Reference.java

@@ -1094,11 +1094,11 @@ public String getHostDomain() {
     }
 
     /**
-     * Returns the optionnally decoded host domain name component.
+     * Returns the optionally decoded host domain name component.
      * 
      * @param decode
      *            Indicates if the result should be decoded using the {@link #decode(String)} method.
-     * @return The optionnally decoded host domain name component.
+     * @return The optionally decoded host domain name component.
      * @see #getHostDomain()
      */
     public String getHostDomain(boolean decode) {

modules/org.restlet/src/main/java/org/restlet/engine/local/DirectoryServerResource.java

@@ -130,7 +130,7 @@ public Representation delete() throws ResourceException {
         if (this.directoryTarget && !this.indexTarget) {
             // let the client handle the directory's deletion
             contextRequest.setResourceRef(this.targetUri);
-            getClientDispatcher().handle(contextRequest, contextResponse);
+            dispatchRequest(contextRequest, contextResponse);
 
             setStatus(contextResponse.getStatus());
             return null;
@@ -144,7 +144,7 @@ public Representation delete() throws ResourceException {
         } else if (this.uniqueReference != null) {
             // only one representation
             contextRequest.setResourceRef(this.uniqueReference);
-            getClientDispatcher().handle(contextRequest, contextResponse);
+            dispatchRequest(contextRequest, contextResponse);
             setStatus(contextResponse.getStatus());
         } else {
             // several variants found, but not the right one
@@ -365,18 +365,6 @@ private ReferenceList tryToConvertAsReferenceList(Representation entity) throws
         }
     }
 
-    /**
-     * Prevent the client from accessing resources in upper directories
-     */
-    public void preventUpperDirectoryAccess() {
-        String targetUriPath = new Reference(Reference.decode(targetUri))
-                .normalize()
-                .toString();
-        if (!targetUriPath.startsWith(directory.getRootRef().toString())) {
-            throw new ResourceException(Status.CLIENT_ERROR_FORBIDDEN);
-        }
-    }
-
     @Override
     protected Representation get() throws ResourceException {
         // Content negotiation has been disabled
@@ -468,8 +456,7 @@ public String getDirectoryUri() {
      * @return A response with the representation if success.
      */
     private Response getRepresentation(String resourceUri) {
-        return getClientDispatcher().handle(
-                new Request(Method.GET, resourceUri));
+        return dispatchRequest(new Request(Method.GET, resourceUri));
     }
 
     /**
@@ -489,7 +476,7 @@ protected Response getRepresentation(String resourceUri, MediaType acceptedMedia
             request.getClientInfo().accept(acceptedMediaType);
         }
 
-        return getClientDispatcher().handle(request);
+        return dispatchRequest(request);
     }
 
     /**
@@ -757,6 +744,50 @@ public boolean isFileTarget() {
         return this.fileTarget;
     }
 
+    /**
+     * Transmit the given request to the clientDispatcher.<br>
+     * It completes the request's attributes map with the current Directory ("org.restlet.directory" key).
+     *
+     * @param request
+     *         The request to send.
+     * @return The response
+     */
+    private Response dispatchRequest(final Request request) {
+        final Response response = new Response(request);
+        dispatchRequest(request, response);
+
+        if (response.getStatus().equals(Status.CLIENT_ERROR_FORBIDDEN)) {
+            throw new ResourceException(response.getStatus());
+        }
+
+        return response;
+    }
+
+    /**
+     * Transmit the given request to the clientDispatcher.<br>
+     * It completes the request's attributes map with the current Directory ("org.restlet.directory" key).
+     *
+     * @param request
+     *         The request to send.
+     * @param response
+     *         The related response.
+     */
+    private void dispatchRequest(final Request request, final Response response) {
+        request.getAttributes().put("org.restlet.directory", this.directory);
+        getClientDispatcher().handle(request, response);
+    }
+
+    /**
+     * Prevent the client from accessing resources in upper directories
+     */
+    public void preventUpperDirectoryAccess() {
+		String targetUriPath = Reference.decode(targetUri);
+
+		if (!targetUriPath.startsWith(Reference.decode(directory.getRootRef().toString()))) {
+			throw new ResourceException(Status.CLIENT_ERROR_FORBIDDEN);
+		}
+    }
+
     @Override
     public Representation put(Representation entity) throws ResourceException {
         if (!this.directory.isModifiable()) {
@@ -772,7 +803,7 @@ public Representation put(Representation entity) throws ResourceException {
         contextRequest.setEntity(entity);
         Response contextResponse = new Response(contextRequest);
         contextRequest.setResourceRef(this.targetUri);
-        getClientDispatcher().handle(contextRequest, contextResponse);
+        dispatchRequest(contextRequest, contextResponse);
         setStatus(contextResponse.getStatus());
 
         return null;

modules/org.restlet/src/main/java/org/restlet/engine/local/FileClientHelper.java

@@ -44,11 +44,11 @@
 
 import java.io.File;
 import java.io.FileFilter;
-import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.RandomAccessFile;
 import java.nio.file.Files;
-import java.nio.file.StandardCopyOption;
+import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Iterator;
@@ -68,6 +68,7 @@
 import org.restlet.engine.io.IoUtils;
 import org.restlet.representation.Representation;
 import org.restlet.representation.Variant;
+import org.restlet.resource.Directory;
 
 /**
  * Connector to the file resources accessible. Here is the list of parameters
@@ -171,16 +172,27 @@ private boolean checkMetadataConsistency(String fileName, Representation represe
 
     @Override
     public Entity getEntity(String decodedPath) {
-        // Take care of the file separator.
         return new FileEntity(
-                new File(LocalReference.localizePath(decodedPath)),
+                getFileWithLocalizedPath(decodedPath),
                 getMetadataService());
     }
 
+    /**
+     * Returns a new {@link File} instance for the given path name.<br>
+     * It ensures to translate the "/" to the local supported file separator (mainly useful for Windows OS).
+     *
+     * @param path
+     *         The Path of the file
+     * @return a new {@link File} instance for the given path name.
+     */
+    private static File getFileWithLocalizedPath(final String path) {
+        return new File(LocalReference.localizePath(path));
+    }
+
     /**
      * Returns the name of the extension to use to store the temporary content
      * while uploading content via the PUT method. Defaults to "tmp".
-     * 
+     *
      * @return The name of the extension to use to store the temporary content.
      */
     public String getTemporaryExtension() {
@@ -201,13 +213,18 @@ protected void handleLocal(Request request, Response response,
     }
 
     protected void handleFile(Request request, Response response, String decodedPath) {
-        if (GET.equals(request.getMethod())
+        final Directory directory = (Directory) request.getAttributes().get("org.restlet.directory");
+        final File fileWithLocalizedPath = getFileWithLocalizedPath(decodedPath);
+
+        if (!isFileInDirectory(directory, fileWithLocalizedPath)) {
+            response.setStatus(CLIENT_ERROR_FORBIDDEN);
+        } else if (GET.equals(request.getMethod())
                 || HEAD.equals(request.getMethod())) {
             handleEntityGet(request, response, getEntity(decodedPath));
         } else if (PUT.equals(request.getMethod())) {
-            handleFilePut(request, response, decodedPath, new File(decodedPath));
+            handleFilePut(request, response, decodedPath, fileWithLocalizedPath);
         } else if (DELETE.equals(request.getMethod())) {
-            handleFileDelete(response, new File(decodedPath));
+            handleFileDelete(response, fileWithLocalizedPath);
         } else {
             response.setStatus(CLIENT_ERROR_METHOD_NOT_ALLOWED);
             response.getAllowedMethods().add(GET);
@@ -217,17 +234,48 @@ protected void handleFile(Request request, Response response, String decodedPath
         }
     }
 
+    /**
+     * Indicates whether the given file is located inside the root directory.
+     *
+     * @param directory
+     *         The root directory
+     * @param file
+     *         The file.
+     * @return True if the path is located under the root directory, false otherwise.
+     */
+    private static boolean isFileInDirectory(final Directory directory, final File file) {
+        boolean result = true;
+
+        if (directory != null) {
+        	final String fileAbsolute = directory.getRootRef().getPath(true);
+        	final String filePath;
+        	
+        	if(fileAbsolute.indexOf(':') == 2 | fileAbsolute.indexOf('|') == 2) {
+        		filePath = fileAbsolute.substring(1);
+        	}else {
+        		filePath = fileAbsolute;
+        	}
+        	
+            final Path rootDirectoryPath = Paths.get(filePath).normalize();
+            final Path actualFilePath = file.toPath().normalize();
+            result = !rootDirectoryPath.relativize(actualFilePath).toString().startsWith("..");
+        }
+
+        return result;
+    }
+
     /**
      * Handles a DELETE call for the FILE protocol.
-     * 
+     *
      * @param response
      *            The response to update.
      * @param file
      *            The file or directory to delete.
      */
     protected void handleFileDelete(Response response, File file) {
         if (file.isDirectory()) {
-            if (file.listFiles().length == 0) {
+            final File[] files = file.listFiles();
+            if (files == null || files.length == 0) {
                 if (IoUtils.delete(file)) {
                     response.setStatus(SUCCESS_NO_CONTENT);
                 } else {
@@ -436,7 +484,9 @@ private Status updateFileWithPartialContent(Request request, File file, Range ra
             return replaceFileByTemporaryFile(request, file, tmp);
         } catch (IOException ioe) {
             getLogger().log(WARNING, "Unable to create the temporary file", ioe);
-            cleanTemporaryFileIfUploadNotResumed(tmp);
+            if (tmp != null) {
+                cleanTemporaryFileIfUploadNotResumed(tmp);
+            }
             return new Status(SERVER_ERROR_INTERNAL, "Unable to create a temporary file");
         }
     }
@@ -508,9 +558,6 @@ private Status createFileWithPartialContent(Request request, File file, Range ra
                 return SUCCESS_CREATED;
             }
             return SUCCESS_NO_CONTENT;
-        } catch (FileNotFoundException fnfe) {
-            getLogger().log(WARNING, "Unable to create the new file", fnfe);
-            return new Status(SERVER_ERROR_INTERNAL, fnfe);
         } catch (IOException ioe) {
             getLogger().log(WARNING, "Unable to create the new file", ioe);
             return new Status(SERVER_ERROR_INTERNAL, ioe);
@@ -536,7 +583,7 @@ private Status createFile(Request request, File file) {
         getLogger().warning(message);
         return new Status(SERVER_ERROR_INTERNAL, message);
     }
-    
+
     private void cleanTemporaryFileIfUploadNotResumed(File tmp) {
         if (tmp.exists() && !isResumeUpload()) {
             IoUtils.delete(tmp);
@@ -591,7 +638,7 @@ private void updateFileExtension(StringBuilder fileName, Metadata metadata) {
             String extension = getMetadataService().getExtension(metadata);
 
             if (extension != null) {
-                fileName.append("." + extension);
+                fileName.append(".").append(extension);
             } else {
                 if (metadata.getParent() != null) {
                     updateFileExtension(fileName, metadata.getParent());

modules/org.restlet/src/main/java/org/restlet/engine/util/SystemUtils.java

@@ -127,7 +127,7 @@ public static int hashCode(Object... objects) {
      * @return True if the current operating system is in the Windows family.
      */
     public static boolean isWindows() {
-        return System.getProperty("os.name").toLowerCase().indexOf("win") >= 0;
+        return System.getProperty("os.name").toLowerCase().contains("win");
     }
 
     /**

modules/org.restlet/src/main/java/org/restlet/resource/Directory.java

@@ -353,6 +353,11 @@ public void setRootRef(Reference rootRef) {
         this.rootRef = rootRef;
     }
 
+    @Override
+    public String toString() {    
+    	return getRootRef() == null ? super.toString() : super.toString() + ": " + getRootRef();
+    }
+
     /**
      * Sets the reference comparator based on classic alphabetical order.
      *