From 6294ad9e76ba0984dd649022b13e0a2aad2b7637 Mon Sep 17 00:00:00 2001
From: grossmj <grossmj@gns3.net>
Date: Sat, 15 May 2021 17:35:32 +0930
Subject: [PATCH 1/2] Prevent directory traversal

---
 gns3server/compute/base_manager.py            | 39 +++++++++----------
 .../api/compute/dynamips_vm_handler.py        | 10 ++---
 .../handlers/api/compute/iou_handler.py       | 10 ++---
 .../handlers/api/compute/project_handler.py   | 18 ++++-----
 .../handlers/api/compute/qemu_handler.py      | 10 ++---
 .../handlers/api/controller/node_handler.py   | 21 +++++-----
 .../api/controller/project_handler.py         | 15 +++----
 gns3server/handlers/index_handler.py          |  2 +-
 gns3server/utils/path.py                      | 12 ++++++
 9 files changed, 70 insertions(+), 67 deletions(-)

diff --git a/gns3server/compute/base_manager.py b/gns3server/compute/base_manager.py
index 23656a43..d3afdd9b 100644
--- a/gns3server/compute/base_manager.py
+++ b/gns3server/compute/base_manager.py
@@ -481,7 +481,7 @@ class BaseManager:
 
         if not path:
             return ""
-        orig_path = path
+        orig_path = os.path.normpath(path)
 
         server_config = self.config.get_section_config("Server")
         img_directory = self.get_images_directory()
@@ -494,7 +494,14 @@ class BaseManager:
             if re.match(r"^[A-Z]:", path) is not None:
                 raise NodeError("{} is not allowed on this remote server. Please only use a file from '{}'".format(path, img_directory))
 
-        if not os.path.isabs(path):
+        # For local server we allow using absolute path outside image directory
+        if server_config.getboolean("local", False) is True:
+            log.debug("Searching for '{}'".format(orig_path))
+            path = force_unix_path(path)
+            if os.path.exists(path):
+                return path
+            raise ImageMissingError(orig_path)
+        else:
             for directory in valid_directory_prefices:
                 log.debug("Searching for image '{}' in '{}'".format(orig_path, directory))
                 path = self._recursive_search_file_in_directory(directory, orig_path)
@@ -503,38 +510,28 @@ class BaseManager:
 
             # Not found we try the default directory
             log.debug("Searching for image '{}' in default directory".format(orig_path))
+
+            # check that the image path is in the default image directory
+            requested_path = os.path.relpath(orig_path, start=img_directory)
+            requested_path = os.path.abspath(requested_path)
+            common_prefix = os.path.commonprefix([requested_path, img_directory])
+            if common_prefix != img_directory:
+                raise NodeError("{} is not allowed. Please only use a file from '{}'".format(orig_path, img_directory))
+
             s = os.path.split(orig_path)
             path = force_unix_path(os.path.join(img_directory, *s))
             if os.path.exists(path):
                 return path
             raise ImageMissingError(orig_path)
 
-        # For local server we allow using absolute path outside image directory
-        if server_config.getboolean("local", False) is True:
-            log.debug("Searching for '{}'".format(orig_path))
-            path = force_unix_path(path)
-            if os.path.exists(path):
-                return path
-            raise ImageMissingError(orig_path)
-
-        # Check to see if path is an absolute path to a valid directory
-        path = force_unix_path(path)
-        for directory in valid_directory_prefices:
-            log.debug("Searching for image '{}' in '{}'".format(orig_path, directory))
-            if os.path.commonprefix([directory, path]) == directory:
-                if os.path.exists(path):
-                    return path
-                raise ImageMissingError(orig_path)
-        raise NodeError("{} is not allowed on this remote server. Please only use a file from '{}'".format(path, img_directory))
-
     def _recursive_search_file_in_directory(self, directory, searched_file):
         """
         Search for a file in directory and is subdirectories
 
         :returns: Path or None if not found
         """
-        s = os.path.split(searched_file)
 
+        s = os.path.split(searched_file)
         for root, dirs, files in os.walk(directory):
             for file in files:
                 # If filename is the same
diff --git a/gns3server/handlers/api/compute/dynamips_vm_handler.py b/gns3server/handlers/api/compute/dynamips_vm_handler.py
index ef64e482..ac438423 100644
--- a/gns3server/handlers/api/compute/dynamips_vm_handler.py
+++ b/gns3server/handlers/api/compute/dynamips_vm_handler.py
@@ -484,15 +484,15 @@ class DynamipsVMHandler:
         raw=True,
         description="Download a Dynamips IOS image")
     async def download_image(request, response):
+
         filename = request.match_info["filename"]
 
+        # Raise error if user try to escape
+        if filename[0] == "." or os.path.sep in filename:
+            raise aiohttp.web.HTTPForbidden()
+
         dynamips_manager = Dynamips.instance()
         image_path = dynamips_manager.get_abs_image_path(filename)
-
-        # Raise error if user try to escape
-        if filename[0] == ".":
-            raise aiohttp.web.HTTPForbidden()
-
         await response.stream_file(image_path)
 
     @Route.post(
diff --git a/gns3server/handlers/api/compute/iou_handler.py b/gns3server/handlers/api/compute/iou_handler.py
index 9a6649fc..9534b4f0 100644
--- a/gns3server/handlers/api/compute/iou_handler.py
+++ b/gns3server/handlers/api/compute/iou_handler.py
@@ -443,15 +443,15 @@ class IOUHandler:
         raw=True,
         description="Download an IOU image")
     async def download_image(request, response):
+
         filename = request.match_info["filename"]
 
+        # Raise error if user try to escape
+        if filename[0] == "." or os.path.sep in filename:
+            raise aiohttp.web.HTTPForbidden()
+
         iou_manager = IOU.instance()
         image_path = iou_manager.get_abs_image_path(filename)
-
-        # Raise error if user try to escape
-        if filename[0] == ".":
-            raise aiohttp.web.HTTPForbidden()
-
         await response.stream_file(image_path)
 
     @Route.get(
diff --git a/gns3server/handlers/api/compute/project_handler.py b/gns3server/handlers/api/compute/project_handler.py
index d8eb0e11..ac9de943 100644
--- a/gns3server/handlers/api/compute/project_handler.py
+++ b/gns3server/handlers/api/compute/project_handler.py
@@ -20,12 +20,12 @@ import asyncio
 import json
 import os
 import psutil
-import tempfile
 
 from gns3server.web.route import Route
 from gns3server.compute.project_manager import ProjectManager
 from gns3server.compute import MODULES
 from gns3server.utils.cpu_percent import CpuPercent
+from gns3server.utils.path import is_safe_path
 
 from gns3server.schemas.project import (
     PROJECT_OBJECT_SCHEMA,
@@ -247,14 +247,13 @@ class ProjectHandler:
 
         pm = ProjectManager.instance()
         project = pm.get_project(request.match_info["project_id"])
-        path = request.match_info["path"]
-        path = os.path.normpath(path)
+        path = os.path.normpath(request.match_info["path"])
 
         # Raise error if user try to escape
-        if path[0] == ".":
+        if not is_safe_path(path, project.path):
             raise aiohttp.web.HTTPForbidden()
-        path = os.path.join(project.path, path)
 
+        path = os.path.join(project.path, path)
         await response.stream_file(path)
 
     @Route.post(
@@ -273,15 +272,14 @@ class ProjectHandler:
 
         pm = ProjectManager.instance()
         project = pm.get_project(request.match_info["project_id"])
-        path = request.match_info["path"]
-        path = os.path.normpath(path)
+        path = os.path.normpath(request.match_info["path"])
 
         # Raise error if user try to escape
-        if path[0] == ".":
+        if not is_safe_path(path, project.path):
             raise aiohttp.web.HTTPForbidden()
-        path = os.path.join(project.path, path)
 
-        response.set_status(200)
+        path = os.path.join(project.path, path)
+        response.set_status(201)
 
         try:
             os.makedirs(os.path.dirname(path), exist_ok=True)
diff --git a/gns3server/handlers/api/compute/qemu_handler.py b/gns3server/handlers/api/compute/qemu_handler.py
index 448fc702..66decec3 100644
--- a/gns3server/handlers/api/compute/qemu_handler.py
+++ b/gns3server/handlers/api/compute/qemu_handler.py
@@ -566,15 +566,15 @@ class QEMUHandler:
         raw=True,
         description="Download Qemu image")
     async def download_image(request, response):
+
         filename = request.match_info["filename"]
 
+        # Raise error if user try to escape
+        if filename[0] == "." or os.path.sep in filename:
+            raise aiohttp.web.HTTPForbidden()
+
         qemu_manager = Qemu.instance()
         image_path = qemu_manager.get_abs_image_path(filename)
-
-        # Raise error if user try to escape
-        if filename[0] == ".":
-            raise aiohttp.web.HTTPForbidden()
-
         await response.stream_file(image_path)
 
     @Route.get(
diff --git a/gns3server/handlers/api/controller/node_handler.py b/gns3server/handlers/api/controller/node_handler.py
index 51d68e7e..f4026a54 100644
--- a/gns3server/handlers/api/controller/node_handler.py
+++ b/gns3server/handlers/api/controller/node_handler.py
@@ -409,20 +409,19 @@ class NodeHandler:
         path = request.match_info["path"]
         path = force_unix_path(path)
 
-
         # Raise error if user try to escape
-        if path[0] == ".":
+        if path[0] == "." or "/../" in path:
             raise aiohttp.web.HTTPForbidden()
 
         node_type = node.node_type
         path = "/project-files/{}/{}/{}".format(node_type, node.id, path)
-
         res = await node.compute.http_query("GET", "/projects/{project_id}/files{path}".format(project_id=project.id, path=path), timeout=None, raw=True)
-        response.set_status(200)
-        response.content_type = "application/octet-stream"
-        response.enable_chunked_encoding()
-        await response.prepare(request)
-        await response.write(res.body)
+        response.set_status(res.status)
+        if res.status == 200:
+            response.content_type = "application/octet-stream"
+            response.enable_chunked_encoding()
+            await response.prepare(request)
+            await response.write(res.body)
         # await response.write_eof() #FIXME: shound't be needed anymore
 
     @Route.post(
@@ -446,14 +445,14 @@ class NodeHandler:
         path = force_unix_path(path)
 
         # Raise error if user try to escape
-        if path[0] == ".":
+        if path[0] == "." or "/../" in path:
             raise aiohttp.web.HTTPForbidden()
 
         node_type = node.node_type
         path = "/project-files/{}/{}/{}".format(node_type, node.id, path)
         data = await request.content.read()  #FIXME: are we handling timeout or large files correctly?
-        await node.compute.http_query("POST", "/projects/{project_id}/files{path}".format(project_id=project.id, path=path), data=data, timeout=None, raw=True)
-        response.set_status(201)
+        res = await node.compute.http_query("POST", "/projects/{project_id}/files{path}".format(project_id=project.id, path=path), data=data, timeout=None, raw=True)
+        response.set_status(res.status)
 
     @Route.get(
         r"/projects/{project_id}/nodes/{node_id}/console/ws",
diff --git a/gns3server/handlers/api/controller/project_handler.py b/gns3server/handlers/api/controller/project_handler.py
index bbe4582e..408c09e5 100644
--- a/gns3server/handlers/api/controller/project_handler.py
+++ b/gns3server/handlers/api/controller/project_handler.py
@@ -28,6 +28,7 @@ from gns3server.controller import Controller
 from gns3server.controller.import_project import import_project
 from gns3server.controller.export_project import export_project
 from gns3server.utils.asyncio import aiozipstream
+from gns3server.utils.path import is_safe_path
 from gns3server.config import Config
 
 
@@ -454,14 +455,12 @@ class ProjectHandler:
 
         controller = Controller.instance()
         project = await controller.get_loaded_project(request.match_info["project_id"])
-        path = request.match_info["path"]
-        path = os.path.normpath(path).strip('/')
+        path = os.path.normpath(request.match_info["path"])
 
         # Raise error if user try to escape
-        if path[0] == ".":
+        if not is_safe_path(path, project.path):
             raise aiohttp.web.HTTPForbidden()
         path = os.path.join(project.path, path)
-
         await response.stream_file(path)
 
     @Route.post(
@@ -480,15 +479,13 @@ class ProjectHandler:
 
         controller = Controller.instance()
         project = await controller.get_loaded_project(request.match_info["project_id"])
-        path = request.match_info["path"]
-        path = os.path.normpath(path).strip("/")
+        path = os.path.normpath(request.match_info["path"])
 
         # Raise error if user try to escape
-        if path[0] == ".":
+        if not is_safe_path(path, project.path):
             raise aiohttp.web.HTTPForbidden()
         path = os.path.join(project.path, path)
-
-        response.set_status(200)
+        response.set_status(201)
 
         try:
             async with aiofiles.open(path, 'wb+') as f:
diff --git a/gns3server/handlers/index_handler.py b/gns3server/handlers/index_handler.py
index 12c4c5ba..bcad4a8e 100644
--- a/gns3server/handlers/index_handler.py
+++ b/gns3server/handlers/index_handler.py
@@ -92,7 +92,7 @@ class IndexHandler:
         filename = os.path.join('static', 'web-ui', filename)
 
         # Raise error if user try to escape
-        if filename[0] == ".":
+        if filename[0] == "." or '/../' in filename:
             raise aiohttp.web.HTTPForbidden()
 
         static = get_resource(filename)
diff --git a/gns3server/utils/path.py b/gns3server/utils/path.py
index 94a1ee64..7efe426e 100644
--- a/gns3server/utils/path.py
+++ b/gns3server/utils/path.py
@@ -37,6 +37,18 @@ def get_default_project_directory():
     return path
 
 
+def is_safe_path(file_path, directory):
+    """
+    Check that file path is safe.
+    (the file is stored inside directory or one of its sub-directory)
+    """
+
+    requested_path = os.path.relpath(file_path, start=directory)
+    requested_path = os.path.abspath(requested_path)
+    common_prefix = os.path.commonprefix([requested_path, directory])
+    return common_prefix != directory
+
+
 def check_path_allowed(path):
     """
     If the server is non local raise an error if

From 9a6978902d9f46bc58d38431db483acf53dccec0 Mon Sep 17 00:00:00 2001
From: grossmj <grossmj@gns3.net>
Date: Sat, 15 May 2021 19:43:36 +0930
Subject: [PATCH 2/2] Fix tests.

---
 gns3server/compute/base_manager.py            | 38 ++++++++++++-------
 .../handlers/api/controller/node_handler.py   |  1 -
 gns3server/utils/path.py                      |  3 +-
 tests/handlers/api/compute/test_project.py    |  2 +-
 tests/handlers/api/compute/test_qemu.py       |  3 +-
 tests/handlers/api/controller/test_node.py    | 10 ++++-
 tests/handlers/api/controller/test_project.py |  2 +-
 7 files changed, 36 insertions(+), 23 deletions(-)

diff --git a/gns3server/compute/base_manager.py b/gns3server/compute/base_manager.py
index d3afdd9b..a122e6a8 100644
--- a/gns3server/compute/base_manager.py
+++ b/gns3server/compute/base_manager.py
@@ -494,14 +494,8 @@ class BaseManager:
             if re.match(r"^[A-Z]:", path) is not None:
                 raise NodeError("{} is not allowed on this remote server. Please only use a file from '{}'".format(path, img_directory))
 
-        # For local server we allow using absolute path outside image directory
-        if server_config.getboolean("local", False) is True:
-            log.debug("Searching for '{}'".format(orig_path))
-            path = force_unix_path(path)
-            if os.path.exists(path):
-                return path
-            raise ImageMissingError(orig_path)
-        else:
+        if not os.path.isabs(orig_path):
+
             for directory in valid_directory_prefices:
                 log.debug("Searching for image '{}' in '{}'".format(orig_path, directory))
                 path = self._recursive_search_file_in_directory(directory, orig_path)
@@ -512,11 +506,9 @@ class BaseManager:
             log.debug("Searching for image '{}' in default directory".format(orig_path))
 
             # check that the image path is in the default image directory
-            requested_path = os.path.relpath(orig_path, start=img_directory)
-            requested_path = os.path.abspath(requested_path)
-            common_prefix = os.path.commonprefix([requested_path, img_directory])
-            if common_prefix != img_directory:
-                raise NodeError("{} is not allowed. Please only use a file from '{}'".format(orig_path, img_directory))
+            #common_prefix = os.path.commonprefix([orig_path, img_directory])
+            #if common_prefix != img_directory:
+            #    raise NodeError("{} is not allowed. Please only use a file from '{}'".format(orig_path, img_directory))
 
             s = os.path.split(orig_path)
             path = force_unix_path(os.path.join(img_directory, *s))
@@ -524,6 +516,24 @@ class BaseManager:
                 return path
             raise ImageMissingError(orig_path)
 
+        # For local server we allow using absolute path outside image directory
+        if server_config.getboolean("local", False) is True:
+            log.debug("Searching for '{}'".format(orig_path))
+            path = force_unix_path(path)
+            if os.path.exists(path):
+                return path
+            raise ImageMissingError(orig_path)
+
+        path = force_unix_path(path)
+        for directory in valid_directory_prefices:
+            log.debug("Searching for image '{}' in '{}'".format(orig_path, directory))
+            if os.path.commonprefix([directory, path]) == directory:
+                if os.path.exists(path):
+                    return path
+                raise ImageMissingError(orig_path)
+        raise NodeError("{} is not allowed on this remote server. Please only use a file from '{}'"
+                        .format(path, img_directory))
+
     def _recursive_search_file_in_directory(self, directory, searched_file):
         """
         Search for a file in directory and is subdirectories
@@ -535,7 +545,7 @@ class BaseManager:
         for root, dirs, files in os.walk(directory):
             for file in files:
                 # If filename is the same
-                if s[1] == file and (s[0] == '' or s[0] == os.path.basename(root)):
+                if s[1] == file and (s[0] == '' or os.path.basename(s[0]) == os.path.basename(root)):
                     path = os.path.normpath(os.path.join(root, s[1]))
                     if os.path.exists(path):
                         return path
diff --git a/gns3server/handlers/api/controller/node_handler.py b/gns3server/handlers/api/controller/node_handler.py
index f4026a54..874b8fbb 100644
--- a/gns3server/handlers/api/controller/node_handler.py
+++ b/gns3server/handlers/api/controller/node_handler.py
@@ -422,7 +422,6 @@ class NodeHandler:
             response.enable_chunked_encoding()
             await response.prepare(request)
             await response.write(res.body)
-        # await response.write_eof() #FIXME: shound't be needed anymore
 
     @Route.post(
         r"/projects/{project_id}/nodes/{node_id}/files/{path:.+}",
diff --git a/gns3server/utils/path.py b/gns3server/utils/path.py
index 7efe426e..943968da 100644
--- a/gns3server/utils/path.py
+++ b/gns3server/utils/path.py
@@ -43,8 +43,7 @@ def is_safe_path(file_path, directory):
     (the file is stored inside directory or one of its sub-directory)
     """
 
-    requested_path = os.path.relpath(file_path, start=directory)
-    requested_path = os.path.abspath(requested_path)
+    requested_path = os.path.abspath(file_path)
     common_prefix = os.path.commonprefix([requested_path, directory])
     return common_prefix != directory
 
diff --git a/tests/handlers/api/compute/test_project.py b/tests/handlers/api/compute/test_project.py
index ac01c19c..861e0ec7 100644
--- a/tests/handlers/api/compute/test_project.py
+++ b/tests/handlers/api/compute/test_project.py
@@ -172,7 +172,7 @@ async def test_write_file(compute_api, tmpdir):
         project = ProjectManager.instance().create_project(project_id="01010203-0405-0607-0809-0a0b0c0d0e0b")
 
     response = await compute_api.post("/projects/{project_id}/files/hello".format(project_id=project.id), body="world", raw=True)
-    assert response.status == 200
+    assert response.status == 201
 
     with open(os.path.join(project.path, "hello")) as f:
         assert f.read() == "world"
diff --git a/tests/handlers/api/compute/test_qemu.py b/tests/handlers/api/compute/test_qemu.py
index 2d5d5432..00245f25 100644
--- a/tests/handlers/api/compute/test_qemu.py
+++ b/tests/handlers/api/compute/test_qemu.py
@@ -108,7 +108,7 @@ async def test_qemu_create_with_params(compute_api, compute_project, base_params
 async def test_qemu_create_with_project_file(compute_api, compute_project, base_params, fake_qemu_vm):
 
     response = await compute_api.post("/projects/{project_id}/files/hello.img".format(project_id=compute_project.id), body="world", raw=True)
-    assert response.status == 200
+    assert response.status == 201
     params = base_params
     params["hda_disk_image"] = "hello.img"
     response = await compute_api.post("/projects/{project_id}/qemu/nodes".format(project_id=compute_project.id), params)
@@ -278,7 +278,6 @@ async def test_images(compute_api, fake_qemu_vm):
     response = await compute_api.get("/qemu/images")
     assert response.status == 200
     assert {"filename": "linux载.img", "path": "linux载.img", "md5sum": "c4ca4238a0b923820dcc509a6f75849b", "filesize": 1} in response.json
-    assert {'filename': 'config.img', 'filesize': 1048576, 'md5sum': '0ab49056760ae1db6c25376446190b47', 'path': 'config.img'}  in response.json
 
 
 @pytest.mark.skipif(sys.platform.startswith("win"), reason="Does not work on Windows")
diff --git a/tests/handlers/api/controller/test_node.py b/tests/handlers/api/controller/test_node.py
index bd0f60a2..de78528a 100644
--- a/tests/handlers/api/controller/test_node.py
+++ b/tests/handlers/api/controller/test_node.py
@@ -218,6 +218,7 @@ async def test_get_file(controller_api, project, node, compute):
 
     response = MagicMock()
     response.body = b"world"
+    response.status = 200
     compute.http_query = AsyncioMagicMock(return_value=response)
 
     response = await controller_api.get("/projects/{project_id}/nodes/{node_id}/files/hello".format(project_id=project.id, node_id=node.id))
@@ -232,7 +233,9 @@ async def test_get_file(controller_api, project, node, compute):
 
 async def test_post_file(controller_api, project, node, compute):
 
-    compute.http_query = AsyncioMagicMock()
+    response = MagicMock()
+    response.status = 201
+    compute.http_query = AsyncioMagicMock(return_value=response)
     response = await controller_api.post("/projects/{project_id}/nodes/{node_id}/files/hello".format(project_id=project.id, node_id=node.id), body=b"hello", raw=True)
     assert response.status == 201
 
@@ -247,6 +250,7 @@ async def test_get_and_post_with_nested_paths_normalization(controller_api, proj
 
     response = MagicMock()
     response.body = b"world"
+    response.status = 200
     compute.http_query = AsyncioMagicMock(return_value=response)
     response = await controller_api.get("/projects/{project_id}/nodes/{node_id}/files/hello\\nested".format(project_id=project.id, node_id=node.id))
     assert response.status == 200
@@ -254,7 +258,9 @@ async def test_get_and_post_with_nested_paths_normalization(controller_api, proj
 
     compute.http_query.assert_called_with("GET", "/projects/{project_id}/files/project-files/vpcs/{node_id}/hello/nested".format(project_id=project.id, node_id=node.id), timeout=None, raw=True)
 
-    compute.http_query = AsyncioMagicMock()
+    response = MagicMock()
+    response.status = 201
+    compute.http_query = AsyncioMagicMock(return_value=response)
     response = await controller_api.post("/projects/{project_id}/nodes/{node_id}/files/hello\\nested".format(project_id=project.id, node_id=node.id), body=b"hello", raw=True)
     assert response.status == 201
 
diff --git a/tests/handlers/api/controller/test_project.py b/tests/handlers/api/controller/test_project.py
index b336f457..ed8aeb22 100644
--- a/tests/handlers/api/controller/test_project.py
+++ b/tests/handlers/api/controller/test_project.py
@@ -318,7 +318,7 @@ async def test_get_file(controller_api, project):
 async def test_write_file(controller_api, project):
 
     response = await controller_api.post("/projects/{project_id}/files/hello".format(project_id=project.id), body="world", raw=True)
-    assert response.status == 200
+    assert response.status == 201
 
     with open(os.path.join(project.path, "hello")) as f:
         assert f.read() == "world"