From 333ae037726bb1dc9dea3c662ddc1549006f490a Mon Sep 17 00:00:00 2001 From: teslov Date: Mon, 11 Oct 2021 15:16:25 +0300 Subject: [PATCH 1/6] Added possibility to import file from system. --- app/models/task.py | 52 +++++++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/app/models/task.py b/app/models/task.py index c0402365..2ea771c6 100644 --- a/app/models/task.py +++ b/app/models/task.py @@ -12,7 +12,7 @@ import piexif import re import zipfile - +from shutil import copyfile import requests from PIL import Image from django.contrib.gis.gdal import GDALRaster @@ -462,35 +462,39 @@ class Task(models.Model): self.save() zip_path = self.assets_path("all.zip") - + # Import assets file from mounted system volume (media-dir), or from inside docker container. + # Import file from system in case of system installation. if self.import_url and not os.path.exists(zip_path): - try: - # TODO: this is potentially vulnerable to a zip bomb attack - # mitigated by the fact that a valid account is needed to - # import tasks - logger.info("Importing task assets from {} for {}".format(self.import_url, self)) - download_stream = requests.get(self.import_url, stream=True, timeout=10) - content_length = download_stream.headers.get('content-length') - total_length = int(content_length) if content_length is not None else None - downloaded = 0 - last_update = 0 + if "file://" in self.import_url and os.path.exists(self.import_url.replace("file://", "")): + copyfile(self.import_url.replace("file://", ""), zip_path) + else: + try: + # TODO: this is potentially vulnerable to a zip bomb attack + # mitigated by the fact that a valid account is needed to + # import tasks + logger.info("Importing task assets from {} for {}".format(self.import_url, self)) + download_stream = requests.get(self.import_url, stream=True, timeout=10) + content_length = download_stream.headers.get('content-length') + total_length = int(content_length) if content_length is not None else None + downloaded = 0 + last_update = 0 - with open(zip_path, 'wb') as fd: - for chunk in download_stream.iter_content(4096): - downloaded += len(chunk) + with open(zip_path, 'wb') as fd: + for chunk in download_stream.iter_content(4096): + downloaded += len(chunk) - if time.time() - last_update >= 2: - # Update progress - if total_length is not None: - Task.objects.filter(pk=self.id).update(running_progress=(float(downloaded) / total_length) * 0.9) + if time.time() - last_update >= 2: + # Update progress + if total_length is not None: + Task.objects.filter(pk=self.id).update(running_progress=(float(downloaded) / total_length) * 0.9) - self.check_if_canceled() - last_update = time.time() + self.check_if_canceled() + last_update = time.time() - fd.write(chunk) + fd.write(chunk) - except (requests.exceptions.Timeout, requests.exceptions.ConnectionError, ReadTimeoutError) as e: - raise NodeServerError(e) + except (requests.exceptions.Timeout, requests.exceptions.ConnectionError, ReadTimeoutError) as e: + raise NodeServerError(e) self.refresh_from_db() From 3e987bc860801e4a9d1549ca48de962b37ea6901 Mon Sep 17 00:00:00 2001 From: teslov Date: Mon, 11 Oct 2021 19:30:34 +0300 Subject: [PATCH 2/6] Added strict checking is file exists in media folder of container --- app/models/task.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/models/task.py b/app/models/task.py index 2ea771c6..abf3d8d3 100644 --- a/app/models/task.py +++ b/app/models/task.py @@ -465,8 +465,10 @@ class Task(models.Model): # Import assets file from mounted system volume (media-dir), or from inside docker container. # Import file from system in case of system installation. if self.import_url and not os.path.exists(zip_path): - if "file://" in self.import_url and os.path.exists(self.import_url.replace("file://", "")): - copyfile(self.import_url.replace("file://", ""), zip_path) + if self.import_url.startswith("file://") and os.path.exists(self.import_url.replace("file://", "")): + #check is file placed in shared media folder in /imports directory + if self.import_url.startswith(f"file://{settings.MEDIA_ROOT}/imports/"): + copyfile(self.import_url.replace("file://", ""), zip_path) else: try: # TODO: this is potentially vulnerable to a zip bomb attack From 5daf7a15c0828980542e1544ca870a6a9329c126 Mon Sep 17 00:00:00 2001 From: teslov Date: Mon, 11 Oct 2021 19:31:17 +0300 Subject: [PATCH 3/6] Added checking of .zip ending for path to file --- app/models/task.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/task.py b/app/models/task.py index abf3d8d3..bae9392b 100644 --- a/app/models/task.py +++ b/app/models/task.py @@ -467,7 +467,7 @@ class Task(models.Model): if self.import_url and not os.path.exists(zip_path): if self.import_url.startswith("file://") and os.path.exists(self.import_url.replace("file://", "")): #check is file placed in shared media folder in /imports directory - if self.import_url.startswith(f"file://{settings.MEDIA_ROOT}/imports/"): + if self.import_url.startswith(f"file://{settings.MEDIA_ROOT}/imports/") and self.import_url.endswith(".zip"): copyfile(self.import_url.replace("file://", ""), zip_path) else: try: From 984415d6dd4c393ca38053f801ba3c61957c19a4 Mon Sep 17 00:00:00 2001 From: teslov Date: Mon, 11 Oct 2021 20:16:37 +0300 Subject: [PATCH 4/6] Using traversal check for correct file placement --- app/models/task.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/app/models/task.py b/app/models/task.py index bae9392b..329d79ab 100644 --- a/app/models/task.py +++ b/app/models/task.py @@ -31,6 +31,7 @@ from django.contrib.gis.db.models.fields import GeometryField from app.cogeo import assure_cogeo from app.testwatch import testWatch +from app.api.common import path_traversal_check from nodeodm import status_codes from nodeodm.models import ProcessingNode from pyodm.exceptions import NodeResponseError, NodeConnectionError, NodeServerError, OdmError @@ -462,13 +463,16 @@ class Task(models.Model): self.save() zip_path = self.assets_path("all.zip") - # Import assets file from mounted system volume (media-dir), or from inside docker container. + # Import assets file from mounted system volume (media-dir)/imports, or from inside docker container. # Import file from system in case of system installation. if self.import_url and not os.path.exists(zip_path): - if self.import_url.startswith("file://") and os.path.exists(self.import_url.replace("file://", "")): - #check is file placed in shared media folder in /imports directory - if self.import_url.startswith(f"file://{settings.MEDIA_ROOT}/imports/") and self.import_url.endswith(".zip"): - copyfile(self.import_url.replace("file://", ""), zip_path) + if self.import_url.startswith("file://"): + imports_folder_path = os.path.join(settings.MEDIA_ROOT, "imports") + unsafe_path_to_import_file = os.path.join(settings.MEDIA_ROOT, "imports", self.import_url.replace("file://", "")) + # check is file placed in shared media folder in /imports directory without traversing + checked_path_to_file = path_traversal_check(unsafe_path_to_import_file, imports_folder_path) + if os.path.isfile(checked_path_to_file): + copyfile(checked_path_to_file, zip_path) else: try: # TODO: this is potentially vulnerable to a zip bomb attack From cd0f1dedeec805cfcf17da94b684147102d46be9 Mon Sep 17 00:00:00 2001 From: teslov Date: Mon, 11 Oct 2021 20:21:11 +0300 Subject: [PATCH 5/6] Added error checking with raising NodeServerError --- app/models/task.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/app/models/task.py b/app/models/task.py index 329d79ab..a1986fce 100644 --- a/app/models/task.py +++ b/app/models/task.py @@ -19,7 +19,7 @@ from django.contrib.gis.gdal import GDALRaster from django.contrib.gis.gdal import OGRGeometry from django.contrib.gis.geos import GEOSGeometry from django.contrib.postgres import fields -from django.core.exceptions import ValidationError +from django.core.exceptions import ValidationError, SuspiciousFileOperation from django.db import models from django.db import transaction from django.db import connection @@ -463,16 +463,19 @@ class Task(models.Model): self.save() zip_path = self.assets_path("all.zip") - # Import assets file from mounted system volume (media-dir)/imports, or from inside docker container. - # Import file from system in case of system installation. + # Import assets file from mounted system volume (media-dir)/imports by relative path. + # Import file from relative path. if self.import_url and not os.path.exists(zip_path): if self.import_url.startswith("file://"): imports_folder_path = os.path.join(settings.MEDIA_ROOT, "imports") unsafe_path_to_import_file = os.path.join(settings.MEDIA_ROOT, "imports", self.import_url.replace("file://", "")) # check is file placed in shared media folder in /imports directory without traversing - checked_path_to_file = path_traversal_check(unsafe_path_to_import_file, imports_folder_path) - if os.path.isfile(checked_path_to_file): - copyfile(checked_path_to_file, zip_path) + try: + checked_path_to_file = path_traversal_check(unsafe_path_to_import_file, imports_folder_path) + if os.path.isfile(checked_path_to_file): + copyfile(checked_path_to_file, zip_path) + except SuspiciousFileOperation as e: + raise NodeServerError(e) else: try: # TODO: this is potentially vulnerable to a zip bomb attack From d63ee48aa33930f7e752c5468bced51622681dc2 Mon Sep 17 00:00:00 2001 From: teslov Date: Mon, 11 Oct 2021 20:22:58 +0300 Subject: [PATCH 6/6] Added logging before throwing exception --- app/models/task.py | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/task.py b/app/models/task.py index a1986fce..1ff91562 100644 --- a/app/models/task.py +++ b/app/models/task.py @@ -475,6 +475,7 @@ class Task(models.Model): if os.path.isfile(checked_path_to_file): copyfile(checked_path_to_file, zip_path) except SuspiciousFileOperation as e: + logger.error("Error due importing assets from {} for {} in cause of path checking error".format(self.import_url, self)) raise NodeServerError(e) else: try: