From 8d26807573be897e739c7f974e6f21dd9a8298db Mon Sep 17 00:00:00 2001
From: Daniel Markstedt <daniel@mindani.net>
Date: Sat, 11 Nov 2023 20:46:31 +0900
Subject: [PATCH] Correct upload dir path validation logic (#1338)

* Correct upload and download  dir path validation logic

* Improve file download labels

* Clean up tmp file before attempting to upload again
---
 python/web/src/templates/index.html  |  4 ++--
 python/web/src/templates/upload.html |  4 ++--
 python/web/src/web.py                | 21 ++++++++++++++++-----
 python/web/src/web_utils.py          | 28 +++++++++-------------------
 4 files changed, 29 insertions(+), 28 deletions(-)

diff --git a/python/web/src/templates/index.html b/python/web/src/templates/index.html
index d1baf4d7..01e4967b 100644
--- a/python/web/src/templates/index.html
+++ b/python/web/src/templates/index.html
@@ -395,8 +395,8 @@
     <label for="download_url">{{ _("Download file from URL:") }}</label>
     <input name="url" id="download_url" required="" type="url">
     <label for="disk_images" class="hidden">{{ _("Disk Images") }}</label>
+    <label for="images_subdir">{{ _("To directory:") }}</label>
     <input type="radio" name="destination" id="disk_images" value="disk_images" checked="checked">
-    <label for="images_subdir" class="hidden">{{ _("Directory") }}</label>
     <select name="images_subdir" id="images_subdir">
     {% for dir in images_subdirs %}
     <option value="{{dir}}">{{env['image_root_dir']}}/{{dir}}</option>
@@ -406,7 +406,7 @@
     {% if file_server_dir_exists %}
     <label for="shared_files" class="hidden">{{ _("Shared Files") }}</label>
     <input type="radio" name="destination" id="shared_files" value="shared_files">
-    <label for="shared_subdir" class="hidden">{{ _("Directory") }}</label>
+    <label for="shared_subdir" class="hidden">{{ _("To directory:") }}</label>
     <select name="shared_subdir" id="shared_subdir">
     {% for dir in shared_subdirs %}
     <option value="{{dir}}">{{env['shared_root_dir']}}/{{dir}}</option>
diff --git a/python/web/src/templates/upload.html b/python/web/src/templates/upload.html
index 87ad2b67..2a3b9b3e 100644
--- a/python/web/src/templates/upload.html
+++ b/python/web/src/templates/upload.html
@@ -16,7 +16,7 @@
     <legend>{{ _("Destination") }}</legend>
     <label for="disk_images" class="hidden">{{ _("Disk Images") }}</label>
     <input type="radio" name="destination" id="disk_images" value="disk_images" checked="checked">
-    <label for="images_subdir" class="hidden">{{ _("Directory") }}</label>
+    <label for="images_subdir" class="hidden">{{ _("To directory:") }}</label>
     <select name="images_subdir" id="images_subdir">
     {% for dir in images_subdirs %}
     <option value="{{dir}}">{{ env['image_root_dir'] }}/{{dir}}</option>
@@ -26,7 +26,7 @@
     {% if file_server_dir_exists %}
     <label for="shared_files" class="hidden">{{ _("Shared Files") }}</label>
     <input type="radio" name="destination" id="shared_files" value="shared_files">
-    <label for="shared_subdir" class="hidden">{{ _("Directory") }}</label>
+    <label for="shared_subdir" class="hidden">{{ _("To directory:") }}</label>
     <select name="shared_subdir" id="shared_subdir">
     {% for dir in shared_subdirs %}
     <option value="{{dir}}">{{ env['shared_root_dir'] }}/{{dir}}</option>
diff --git a/python/web/src/web.py b/python/web/src/web.py
index 2e85722c..349d9aaa 100644
--- a/python/web/src/web.py
+++ b/python/web/src/web.py
@@ -58,7 +58,6 @@ from web_utils import (
     auth_active,
     is_bridge_configured,
     is_safe_path,
-    validate_target_dir,
     browser_supports_modern_themes,
 )
 from settings import (
@@ -996,15 +995,19 @@ def download_file():
     images_subdir = request.form.get("images_subdir")
     shared_subdir = request.form.get("shared_subdir")
     if destination == "disk_images":
+        safe_path = is_safe_path(Path(images_subdir))
+        if not safe_path["status"]:
+            return response(error=True, message=safe_path["msg"])
         server_info = piscsi_cmd.get_server_info()
         destination_dir = Path(server_info["image_dir"]) / images_subdir
     elif destination == "shared_files":
+        safe_path = is_safe_path(Path(shared_subdir))
+        if not safe_path["status"]:
+            return response(error=True, message=safe_path["msg"])
         destination_dir = Path(FILE_SERVER_DIR) / shared_subdir
     else:
         return response(error=True, message=_("Unknown destination"))
 
-    validate_target_dir(destination_dir)
-
     process = file_cmd.download_to_dir(url, Path(destination_dir) / Path(url).name)
     process = ReturnCodeMapper.add_msg(process)
     if process["status"]:
@@ -1034,17 +1037,21 @@ def upload_file():
     images_subdir = request.form.get("images_subdir")
     shared_subdir = request.form.get("shared_subdir")
     if destination == "disk_images":
+        safe_path = is_safe_path(Path(images_subdir))
+        if not safe_path["status"]:
+            return make_response(safe_path["msg"], 403)
         server_info = piscsi_cmd.get_server_info()
         destination_dir = Path(server_info["image_dir"]) / images_subdir
     elif destination == "shared_files":
+        safe_path = is_safe_path(Path(shared_subdir))
+        if not safe_path["status"]:
+            return make_response(safe_path["msg"], 403)
         destination_dir = Path(FILE_SERVER_DIR) / shared_subdir
     elif destination == "piscsi_config":
         destination_dir = Path(CFG_DIR)
     else:
         return make_response(_("Unknown destination"), 403)
 
-    validate_target_dir(destination_dir)
-
     log = logging.getLogger("pydrop")
     file_object = request.files["file"]
     file_name = secure_filename(file_object.filename)
@@ -1059,6 +1066,10 @@ def upload_file():
     if path.exists(save_path) and current_chunk == 0:
         return make_response(_("The file already exists!"), 400)
 
+    if path.exists(tmp_save_path) and current_chunk == 0:
+        log.info("Deleting existing temporary file before uploading...")
+        file_cmd.delete_file(tmp_save_path)
+
     try:
         with open(tmp_save_path, "ab") as save:
             save.seek(int(request.form["dzchunkbyteoffset"]))
diff --git a/python/web/src/web_utils.py b/python/web/src/web_utils.py
index 69af618b..bc779836 100644
--- a/python/web/src/web_utils.py
+++ b/python/web/src/web_utils.py
@@ -8,7 +8,7 @@ from pathlib import Path
 from ua_parser import user_agent_parser
 from re import findall
 
-from flask import request, abort, make_response
+from flask import request, abort
 from flask_babel import _
 from werkzeug.utils import secure_filename
 
@@ -302,37 +302,27 @@ def is_bridge_configured(interface):
     return {"status": True, "msg": return_msg + ", ".join(to_configure)}
 
 
-def is_safe_path(file_name):
+def is_safe_path(path_name):
     """
-    Takes (Path) file_name with the path to a file on the file system
-    Returns True if the path is safe
-    Returns False if the path is either absolute, or tries to traverse the file system
+    Takes (Path) path_name with the relative path to a file on the file system.
+    Returns False if the path is either absolute, or tries to traverse the file system.
+    Returns True if neither of the above criteria are met.
     """
     error_message = ""
-    if file_name.is_absolute():
+    if path_name.is_absolute():
         error_message = _("Path must not be absolute")
-    elif "../" in str(file_name):
+    elif "../" in str(path_name):
         error_message = _("Path must not traverse the file system")
-    elif str(file_name)[0] == "~":
+    elif str(path_name)[0] == "~":
         error_message = _("Path must not start in the home directory")
 
     if error_message:
-        logging.error("Not an allowed path: %s", str(file_name))
+        logging.error("Not an allowed path: %s", str(path_name))
         return {"status": False, "msg": error_message}
 
     return {"status": True, "msg": ""}
 
 
-def validate_target_dir(target_dir):
-    """
-    Takes (Path) target_dir on the host and validates that it is a valid dir for uploading.
-    """
-    safe_path = is_safe_path(Path(".") / target_dir)
-    if not safe_path["status"]:
-        return make_response(safe_path["msg"], 403)
-    return True
-
-
 def browser_supports_modern_themes():
     """
     Determines if the browser supports the HTML/CSS/JS features used in non-legacy themes.