Check extract path and ensure minimal perms

.circleci/config.yml

@@ -61,7 +61,7 @@ version: 2
 jobs:
   lint:
     docker:
-      - image: circleci/python:3.5
+      - image: circleci/python:3.7
     steps:
       - checkout
       - run:

.flake8

@@ -1,2 +1,2 @@
 [flake8]
-max-line-length = 99
+max-line-length = 100

.semgrep/custom-rules.yaml

@@ -0,0 +1,99 @@
+rules:
+
+- id: tarfile-extractall-traversal
+  languages:
+    - python
+  severity: ERROR
+  message: Possible path traversal through tarfile.open($PATH).extractall() if the source tar is controlled by an attacker.
+  patterns:
+    - pattern: "....extractall(...)"
+    - pattern-not-inside: |
+        def safe_extractall(...):
+            ...
+
+- id: tarfile-extract-traversal
+  languages:
+    - python
+  severity: ERROR
+  message: Possible path traversal through tarfile.open($PATH).extract() if the source tar is controlled by an attacker.
+  patterns:
+    - pattern: "....extract(...)"
+
+- id: gzip-extract-traversal
+  languages:
+    - python
+  severity: ERROR
+  message: Possible path traversal through gzip.open if the source zip file is controlled by an attacker.
+  patterns:
+    - pattern: |
+        with gzip.open(...) as $IN, open(...) as $OUT:
+            ...
+            copyfileobj(...)
+
+- id: gzip-open-insecure
+  languages:
+    - python
+  severity: ERROR
+  message: Possible path traversal through gzip.open if the source zip file is controlled by an attacker.
+  patterns:
+    - pattern: |
+        with gzip.open(...) as $IN, open(...) as $OUT:
+            ...
+    - pattern-not-inside: |
+        def safe_gzip_extract(...):
+            ...
+
+- id: mkdir-insecure
+  languages:
+    - python
+  severity: ERROR
+  message: Possible path traversal or insecure directory and file permissions through os.mkdir(). Use securedrop_export.utils.safe_mkdir instead.
+  patterns:
+    - pattern: "....mkdir(...)"
+    - pattern-not-inside: |
+        def safe_mkdir(...):
+            ...
+
+- id: makedirs-insecure
+  languages:
+    - python
+  severity: ERROR
+  message: Possible path traversal or insecure directory and file permissions through os.makedirs(). Use securedrop_export.utils.safe_mkdir instead.
+  patterns:
+    - pattern: "....makedirs(...)"
+    - pattern-not-inside: |
+        def safe_mkdir(...):
+            ...
+
+- id: copy-insecure
+  languages:
+    - python
+  severity: ERROR
+  message: Possible path traversal or insecure directory and file permissions through shutil.copy(). Use securedrop_export.utils.safe_copy instead.
+  patterns:
+    - pattern: "....shutil.copy(...)"
+    - pattern-not-inside: |
+        def safe_copy(...):
+            ...
+
+- id: copyfileobj-insecure
+  languages:
+    - python
+  severity: ERROR
+  message: Possible path traversal or insecure directory and file permissions through shutil.copyfileobj(). Use securedrop_export.utils.safe_copyfileobj instead.
+  patterns:
+    - pattern: "....shutil.copyfileobj(...)"
+    - pattern-not-inside: |
+        def safe_copyfileobj(...):
+            ...
+
+- id: move-insecure
+  languages:
+    - python
+  severity: ERROR
+  message: Possible path traversal or insecure directory and file permissions through shutil.move(). Use securedrop_export.utils.safe_move instead.
+  patterns:
+    - pattern: "....shutil.move(...)"
+    - pattern-not-inside: |
+        def safe_move(...):
+            ...

Makefile

@@ -24,6 +24,21 @@ test:  ## Run tests
 lint:  ## Run linter
 	flake8 securedrop_export/ tests/
 
+SEMGREP_FLAGS := --exclude "tests/" --error --strict --verbose
+
+.PHONY: semgrep
+semgrep:semgrep-community semgrep-local
+
+.PHONY: semgrep-community
+semgrep-community:
+	@echo "Running semgrep with semgrep.dev community rules..."
+	@semgrep $(SEMGREP_FLAGS) --config "p/r2c-security-audit" --config "p/r2c-ci"
+
+.PHONY: semgrep-local
+semgrep-local:
+	@echo "Running semgrep with local rules..."
+	@semgrep $(SEMGREP_FLAGS) --config ".semgrep"
+
 # Explaination of the below shell command should it ever break.
 # 1. Set the field separator to ": ##" and any make targets that might appear between : and ##
 # 2. Use sed-like syntax to remove the make targets

changelog.md

@@ -1,5 +1,8 @@
 # Changelog
 
+## 0.2.5
+  * Sets restrictive permissions, validates target paths
+
 ## 0.2.4
   * Removes mimetype associations and open-in-dvm desktop file
 

securedrop_export/VERSION

@@ -1 +1 @@
-0.2.4
+0.2.5

securedrop_export/__init__.py

@@ -1 +1 @@
-__version__ = '0.1.2'
+__version__ = '0.2.5'

securedrop_export/entrypoint.py

@@ -8,9 +8,12 @@
 from securedrop_export import __version__
 from securedrop_export import export
 from securedrop_export import main
+from securedrop_export.utils import safe_mkdir
 
 CONFIG_PATH = "/etc/sd-export-config.json"
 DEFAULT_HOME = os.path.join(os.path.expanduser("~"), ".securedrop_export")
+LOG_DIR_NAME = "logs"
+EXPORT_LOG_FILENAME = "export.log"
 
 logger = logging.getLogger(__name__)
 
@@ -19,11 +22,10 @@ def configure_logging():
     """
     All logging related settings are set up by this function.
     """
-    log_folder = os.path.join(DEFAULT_HOME, 'logs')
-    if not os.path.exists(log_folder):
-        os.makedirs(log_folder)
+    safe_mkdir(DEFAULT_HOME)
+    safe_mkdir(DEFAULT_HOME, LOG_DIR_NAME)
 
-    log_file = os.path.join(DEFAULT_HOME, 'logs', 'export.log')
+    log_file = os.path.join(DEFAULT_HOME, LOG_DIR_NAME, EXPORT_LOG_FILENAME)
 
     # set logging format
     log_fmt = ('%(asctime)s - %(name)s:%(lineno)d(%(funcName)s) '

securedrop_export/export.py

@@ -8,10 +8,10 @@
 import shutil
 import subprocess
 import sys
-import tarfile
 import tempfile
 
 from securedrop_export.exceptions import ExportStatus
+from securedrop_export.utils import safe_extractall
 
 logger = logging.getLogger(__name__)
 
@@ -78,6 +78,7 @@ def is_valid(self):
 
 class SDExport(object):
     def __init__(self, archive, config_path):
+        os.umask(0o077)
         self.archive = archive
         self.submission_dirname = os.path.basename(self.archive).split(".")[0]
         self.target_dirname = "sd-export-{}".format(
@@ -87,10 +88,12 @@ def __init__(self, archive, config_path):
 
     def extract_tarball(self):
         try:
-            logger.info('Extracting tarball {} into {}'.format(self.archive, self.tmpdir))
-            with tarfile.open(self.archive) as tar:
-                tar.extractall(self.tmpdir)
-        except Exception:
+            logger.info(
+                "Extracting tarball {} into {}".format(self.archive, self.tmpdir)
+            )
+            safe_extractall(self.archive, self.tmpdir, self.tmpdir)
+        except Exception as ex:
+            logger.error("Unable to extract tarball: {}".format(ex))
             self.exit_gracefully(ExportStatus.ERROR_EXTRACTION.value)
 
     def exit_gracefully(self, msg, e=False):

securedrop_export/utils.py

@@ -0,0 +1,147 @@
+import os
+import tarfile
+from pathlib import Path
+from typing import Optional, Union
+
+
+def safe_mkdir(
+    base_path: Union[Path, str],
+    relative_path: Union[Optional[Path], Optional[str]] = None,
+) -> None:
+    """
+    Safely create directories with restricted 700 permissions inside the base_path directory. The
+    caller of this function should ensure that base_path comes from a hard-coded string.
+
+    Raises FileNotFoundError if base_path does not already exist or requires more than one new dir
+    Raises RuntimeError if any dir in relative_path or the last dir of base_path have insecure perms
+    Raises ValueError if any of the following conditions is true:
+      * base_dir fails path traversal check, e.g. "/home/../traversed" fails check
+      * the resolved relative_path is not a subdirectory of base_path
+      * a child directory in relative_path already exists with permissions other than 700
+    """
+    base_path = Path(base_path)
+    if not base_path.is_absolute():
+        raise ValueError(f"Base directory '{base_path}' must be an absolute path")
+
+    check_path_traversal(base_path)
+
+    if relative_path:
+        check_path_traversal(relative_path)
+        full_path = base_path.joinpath(relative_path)
+    else:
+        full_path = base_path
+
+    # Create each parent directory, including base_path, first.
+    #
+    # Note: We do not use parents=True because the parent directories will not be created with the
+    # specified mode. Parents are created using system default permissions, which we modify to be
+    # 700 via os.umask in the SDExport contructor. Creating directories one-by-one with mode=0o0700
+    # is not necessary but adds defense in depth.
+    relative_path = relative_filepath(full_path, base_path)
+    for parent in reversed(relative_path.parents):
+        base_path.joinpath(parent).mkdir(mode=0o0700, exist_ok=True)
+
+    # Now create the full_path directory.
+    full_path.mkdir(mode=0o0700, exist_ok=True)
+
+    # Check permissions after creating the directories
+    check_all_permissions(relative_path, base_path)
+
+
+def safe_extractall(archive_file_path: str, dest_path: str, base_path: str) -> None:
+    """
+    Safely extract a file specified by archive_file_path to dest_path.
+    """
+    safe_mkdir(base_path, dest_path)
+
+    with tarfile.open(archive_file_path) as tar:
+        # Tarfile types include:
+        #
+        # FIFO special file (a named pipe)
+        # Regular file
+        # Directory
+        # Symbolic link
+        # Hard link
+        # Block device
+        # Character device
+        for file_info in tar.getmembers():
+            file_info.mode = 0o600
+            if file_info.isdir():
+                file_info.mode = 0o700
+            elif file_info.islnk() or file_info.issym():
+                check_path_traversal(file_info.linkname)
+            else:
+                check_path_traversal(file_info.name)
+
+        tar.extractall(dest_path)
+
+
+def relative_filepath(filepath: Union[str, Path], base_dir: Union[str, Path]) -> Path:
+    """
+    Raise ValueError if the filepath is not relative to the supplied base_dir or if base_dir is not
+    an absolute path.
+
+    Note: resolve() will also resolve symlinks, so a symlink such as /tmp/tmp1a2s3d4f/innocent
+    that points to ../../../../../tmp/traversed will raise a ValueError if the base_dir is the
+    expected /tmp/tmp1a2s3d4f.
+    """
+    return Path(filepath).resolve().relative_to(base_dir)
+
+
+def check_path_traversal(filename_or_filepath: Union[str, Path]) -> None:
+    """
+    Raise ValueError if filename_or_filepath does any path traversal. This works on filenames,
+    relative paths, and absolute paths.
+    """
+    filename_or_filepath = Path(filename_or_filepath)
+    if filename_or_filepath.is_absolute():
+        base_path = filename_or_filepath
+    else:
+        base_path = Path().resolve()
+
+    try:
+        relative_path = relative_filepath(filename_or_filepath, base_path)
+
+        # One last check just to cover "weird/../traversals" that may not traverse past the relative
+        # base, but can still have harmful side effects to the application. If this kind of
+        # traversal is needed, then call relative_filepath instead in order to check that the
+        # desired traversal does not go past a safe base directory.
+        if (
+            relative_path != filename_or_filepath
+            and not filename_or_filepath.is_absolute()
+        ):
+            raise ValueError
+    except ValueError:
+        raise ValueError(f"Unsafe file or directory name: '{filename_or_filepath}'")
+
+
+def check_all_permissions(path: Union[str, Path], base_path: Union[str, Path]) -> None:
+    """
+    Check that the permissions of each directory between base_path and path are set to 700.
+    """
+    base_path = Path(base_path)
+    full_path = base_path.joinpath(path)
+    if not full_path.exists():
+        return
+
+    Path(full_path).chmod(0o700)
+    check_dir_permissions(full_path)
+
+    relative_path = relative_filepath(full_path, base_path)
+    for parent in relative_path.parents:
+        full_path = base_path.joinpath(parent)
+        Path(full_path).chmod(0o700)
+        check_dir_permissions(str(full_path))
+
+
+def check_dir_permissions(dir_path: Union[str, Path]) -> None:
+    """
+    Check that a directory has ``700`` as the final 3 bytes. Raises a ``RuntimeError`` otherwise.
+    """
+    if os.path.exists(dir_path):
+        stat_res = os.stat(dir_path).st_mode
+        masked = stat_res & 0o777
+        if masked & 0o077:
+            raise RuntimeError(
+                "Unsafe permissions ({}) on {}".format(oct(stat_res), dir_path)
+            )

test-requirements.in

@@ -1,6 +1,8 @@
 flake8
 pathlib2  # required by pytest for python 3.5
 pip-tools
+py>=1.9.0
 pytest
 pytest-cov
 pytest-mock
+semgrep==0.42.0