diff --git a/.semgrep/custom-rules.yml b/.semgrep/custom-rules.yml new file mode 100644 index 000000000..1e38cefb0 --- /dev/null +++ b/.semgrep/custom-rules.yml @@ -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_client.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_client.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_client.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_client.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_client.utils.safe_move instead. + patterns: + - pattern: "....shutil.move(...)" + - pattern-not-inside: | + def safe_move(...): + ... diff --git a/Makefile b/Makefile index 8d8857ae9..a4b6bfbfa 100644 --- a/Makefile +++ b/Makefile @@ -6,6 +6,21 @@ venv: ## Provision a Python 3 virtualenv for development. python3 -m venv .venv .venv/bin/pip install --require-hashes -r "requirements/dev-requirements.txt" +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" + +.PHONY: semgrep-local +semgrep-local: + @echo "Running semgrep with local rules..." + @semgrep $(SEMGREP_FLAGS) --config ".semgrep" + .PHONY: black black: ## Format Python source code with black @black setup.py securedrop_client tests diff --git a/changelog.md b/changelog.md index 146cc0c17..776b11a37 100644 --- a/changelog.md +++ b/changelog.md @@ -1,5 +1,15 @@ # Changelog +## 0.4.1 + +* Prevent path traversal in downloaded files (#1226) +* Scale source list, preview and conversation view on resize (#1211, #1206) +* (Dev) Add semgrep for static analysis, including an initial ruleset (#1226) +* (Dev) Remove obsolete MIME setup in run.sh (#1215) +* (Dev) Switch to using reproducibly built wheels (#1203) +* (Dev) Update development dependencies (#1208, #1222) +* (Docs) Incorporate Code of Conduct updates (#1216) + ## 0.4.0 * Support read/unread and track who sees a file, message, or reply (#1165) diff --git a/requirements/dev-requirements.in b/requirements/dev-requirements.in index b1acfc551..18f9c311d 100644 --- a/requirements/dev-requirements.in +++ b/requirements/dev-requirements.in @@ -1,5 +1,5 @@ atomicwrites==1.2.1 -attrs==18.2.0 +attrs==19.3.0 black==19.10b0 Click==7.0 coverage==4.5.1 @@ -29,6 +29,7 @@ pytest-random-order==1.0.4 pytest-vcr==1.0.2 pytest-xdist==1.30.0 pyyaml==5.4.1 +semgrep==0.42.0 sip==4.19.8 typed-ast==1.4.1 vcrpy==4.0.2 diff --git a/requirements/dev-requirements.txt b/requirements/dev-requirements.txt index 4f7256aec..e7f818588 100644 --- a/requirements/dev-requirements.txt +++ b/requirements/dev-requirements.txt @@ -24,13 +24,15 @@ atomicwrites==1.2.1 \ # via # -r requirements/dev-requirements.in # pytest -attrs==18.2.0 \ - --hash=sha256:10cbf6e27dbce8c30807caf056c8eb50917e0eaafe86347671b57254006c3e69 \ - --hash=sha256:ca4be454458f9dec299268d472aaa5a11f67a4ff70093396e1ceae9c76cf4bbb +attrs==19.3.0 \ + --hash=sha256:08a96c641c3a74e44eb59afb61a24f2cb9f4d7188748e76ba4bb5edfa3cb7d1c \ + --hash=sha256:f7b7ce16570fe9965acd6d30101a28f62fb4a7f9e926b3bbc9b61f8b04247e72 # via # -r requirements/dev-requirements.in # black + # jsonschema # pytest + # semgrep black==19.10b0 \ --hash=sha256:1b30e59be925fafc1ee4565e5e08abef6b03fe455102883820fe5ee2e4734e0b \ --hash=sha256:c2edb73a08e9e0e6f65a0e6af18b059b8b1cdd5bef997d7a0b181df93dc81539 @@ -54,6 +56,10 @@ click==7.0 \ # -r requirements/dev-requirements.in # black # pip-tools +colorama==0.4.4 \ + --hash=sha256:5941b2b48a20143d2267e95b1c2a7603ce057ee39fd88e7329b0c292aa16869b \ + --hash=sha256:9f47eda37229f68eee03b24b9748937c7dc3868f906e8ba69fbcbdd3bc5dc3e2 + # via semgrep coverage==4.5.1 \ --hash=sha256:03481e81d558d30d230bc12999e3edffe392d244349a90f4ef9b88425fac74ba \ --hash=sha256:0b136648de27201056c1869a6c0d4e23f464750fd9a9ba9750b8336a244429ed \ @@ -114,12 +120,20 @@ importlib-metadata==1.6.0 \ --hash=sha256:2a688cbaa90e0cc587f1df48bdc97a6eadccdcd9c35fb3f976a09e3b5016d90f \ --hash=sha256:34513a8a0c4962bc66d35b359558fd8a5e10cd472d37aec5f66858addef32c1e # via + # jsonschema # pluggy # pytest isort==4.3.21 \ --hash=sha256:54da7e92468955c4fceacd0c86bd0ec997b0e1ee80d97f67c35a78b719dccab1 \ --hash=sha256:6e811fcb295968434526407adb8796944f1988c5b65e8139058f2014cbe100fd # via -r requirements/dev-requirements.in +jsonschema==3.2.0 \ + --hash=sha256:4e5b3cf8216f577bee9ce139cbe72eca3ea4f292ec60928ff24758ce626cd163 \ + --hash=sha256:c8a85b28d377cc7737e46e2d9f2b4f44ee3c0e1deac6bf46ddefc7187d30797a + # via semgrep +junit-xml==1.9 \ + --hash=sha256:ec5ca1a55aefdd76d28fcc0b135251d156c7106fa979686a4b48d62b761b4732 + # via semgrep mako==1.0.7 \ --hash=sha256:4e02fde57bd4abb5ec400181e4c314f56ac3e49ba4fb8b0d50bba18cb27d25ae # via @@ -220,10 +234,12 @@ mypy==0.761 \ --hash=sha256:e2bb577d10d09a2d8822a042a23b8d62bc3b269667c9eb8e60a6edfa000211b1 \ --hash=sha256:f97a605d7c8bc2c6d1172c2f0d5a65b24142e11a58de689046e62c2d632ca8c1 # via -r requirements/dev-requirements.in -packaging==20.3 \ - --hash=sha256:3c292b474fda1671ec57d46d739d072bfd495a4f51ad01a055121d81e952b7a3 \ - --hash=sha256:82f77b9bee21c1bafbf35a84905d604d5d1223801d639cf3ed140bd651c08752 - # via pytest +packaging==20.9 \ + --hash=sha256:5b327ac1320dc863dca72f4514ecc086f31186744b84a230374cc1fd776feae5 \ + --hash=sha256:67714da7f7bc052e064859c05c595155bd1ee9f69f76557e21f051443c20947a + # via + # pytest + # semgrep pathlib2==2.3.2 \ --hash=sha256:8eb170f8d0d61825e09a95b38be068299ddeda82f35e96c3301a8a5e7604cb83 \ --hash=sha256:d1aa2a11ba7b8f7b21ab852b1fb5afb277e1bb99d5dfc663380b5015c0d80c5a @@ -330,6 +346,9 @@ pyqt5==5.11.3 \ pyrect==0.1.4 \ --hash=sha256:3b2fa7353ce32a11aa6b0a15495968d2a763423c8947ae248b92c037def4e202 # via pygetwindow +pyrsistent==0.17.3 \ + --hash=sha256:2e636185d9eb976a18a8a8e96efce62f2905fea90041958d8cc2a189756ebf3e + # via jsonschema pyscreeze==0.1.26 \ --hash=sha256:224963114176208eee13b0b984de1f5fdc11b7393ce48ad2b52390dc8855d346 # via pyautogui @@ -447,9 +466,52 @@ requests==2.22.0 \ # via # -r requirements/requirements.in # securedrop-sdk + # semgrep +ruamel.yaml.clib==0.2.2 \ + --hash=sha256:058a1cc3df2a8aecc12f983a48bda99315cebf55a3b3a5463e37bb599b05727b \ + --hash=sha256:1236df55e0f73cd138c0eca074ee086136c3f16a97c2ac719032c050f7e0622f \ + --hash=sha256:1f8c0a4577c0e6c99d208de5c4d3fd8aceed9574bb154d7a2b21c16bb924154c \ + --hash=sha256:2602e91bd5c1b874d6f93d3086f9830f3e907c543c7672cf293a97c3fabdcd91 \ + --hash=sha256:28116f204103cb3a108dfd37668f20abe6e3cafd0d3fd40dba126c732457b3cc \ + --hash=sha256:2d24bd98af676f4990c4d715bcdc2a60b19c56a3fb3a763164d2d8ca0e806ba7 \ + --hash=sha256:2fd336a5c6415c82e2deb40d08c222087febe0aebe520f4d21910629018ab0f3 \ + --hash=sha256:30dca9bbcbb1cc858717438218d11eafb78666759e5094dd767468c0d577a7e7 \ + --hash=sha256:44c7b0498c39f27795224438f1a6be6c5352f82cb887bc33d962c3a3acc00df6 \ + --hash=sha256:464e66a04e740d754170be5e740657a3b3b6d2bcc567f0c3437879a6e6087ff6 \ + --hash=sha256:46d6d20815064e8bb023ea8628cfb7402c0f0e83de2c2227a88097e239a7dffd \ + --hash=sha256:4df5019e7783d14b79217ad9c56edf1ba7485d614ad5a385d1b3c768635c81c0 \ + --hash=sha256:4e52c96ca66de04be42ea2278012a2342d89f5e82b4512fb6fb7134e377e2e62 \ + --hash=sha256:5254af7d8bdf4d5484c089f929cb7f5bafa59b4f01d4f48adda4be41e6d29f99 \ + --hash=sha256:52ae5739e4b5d6317b52f5b040b1b6639e8af68a5b8fd606a8b08658fbd0cab5 \ + --hash=sha256:53b9dd1abd70e257a6e32f934ebc482dac5edb8c93e23deb663eac724c30b026 \ + --hash=sha256:6c0a5dc52fc74eb87c67374a4e554d4761fd42a4d01390b7e868b30d21f4b8bb \ + --hash=sha256:73b3d43e04cc4b228fa6fa5d796409ece6fcb53a6c270eb2048109cbcbc3b9c2 \ + --hash=sha256:74161d827407f4db9072011adcfb825b5258a5ccb3d2cd518dd6c9edea9e30f1 \ + --hash=sha256:75f0ee6839532e52a3a53f80ce64925ed4aed697dd3fa890c4c918f3304bd4f4 \ + --hash=sha256:839dd72545ef7ba78fd2aa1a5dd07b33696adf3e68fae7f31327161c1093001b \ + --hash=sha256:8be05be57dc5c7b4a0b24edcaa2f7275866d9c907725226cdde46da09367d923 \ + --hash=sha256:8e8fd0a22c9d92af3a34f91e8a2594eeb35cba90ab643c5e0e643567dc8be43e \ + --hash=sha256:a873e4d4954f865dcb60bdc4914af7eaae48fb56b60ed6daa1d6251c72f5337c \ + --hash=sha256:ab845f1f51f7eb750a78937be9f79baea4a42c7960f5a94dde34e69f3cce1988 \ + --hash=sha256:b1e981fe1aff1fd11627f531524826a4dcc1f26c726235a52fcb62ded27d150f \ + --hash=sha256:b4b0d31f2052b3f9f9b5327024dc629a253a83d8649d4734ca7f35b60ec3e9e5 \ + --hash=sha256:c6ac7e45367b1317e56f1461719c853fd6825226f45b835df7436bb04031fd8a \ + --hash=sha256:daf21aa33ee9b351f66deed30a3d450ab55c14242cfdfcd377798e2c0d25c9f1 \ + --hash=sha256:e9f7d1d8c26a6a12c23421061f9022bb62704e38211fe375c645485f38df34a2 \ + --hash=sha256:f6061a31880c1ed6b6ce341215336e2f3d0c1deccd84957b6fa8ca474b41e89f + # via ruamel.yaml +ruamel.yaml==0.16.10 \ + --hash=sha256:0962fd7999e064c4865f96fb1e23079075f4a2a14849bcdc5cdba53a24f9759b \ + --hash=sha256:099c644a778bf72ffa00524f78dd0b6476bca94a1da344130f4bf3381ce5b954 + # via semgrep securedrop-sdk==0.2.0 \ --hash=sha256:c4a343077e8c0a38914e17f6369b830f1e361f9d66699b20803c07b39472357f # via -r requirements/requirements.in +semgrep==0.42.0 \ + --hash=sha256:179741ce6f8f6785d048af5402bb2452a8771d4282f8aa7cb6852a5adad79fe8 \ + --hash=sha256:376b7a25817a24b32302f49656ea0ddcb2e535de2b05fdf42646f0bd4f33957e \ + --hash=sha256:e50ac0028b98f344166d2464853009837aed9abe669deac93fec04b677b97d2c + # via -r requirements/dev-requirements.in sip==4.19.8 \ --hash=sha256:09f9a4e6c28afd0bafedb26ffba43375b97fe7207bd1a0d3513f79b7d168b331 \ --hash=sha256:105edaaa1c8aa486662226360bd3999b4b89dd56de3e314d82b83ed0587d8783 \ @@ -469,8 +531,9 @@ six==1.11.0 \ --hash=sha256:832dc0e10feb1aa2c68dcc57dbb658f1c7e65b9b61af69048abc87a2db00a0eb # via # -r requirements/requirements.in + # jsonschema + # junit-xml # more-itertools - # packaging # pathlib2 # pytest-xdist # python-dateutil @@ -484,6 +547,10 @@ toml==0.10.1 \ --hash=sha256:926b612be1e5ce0634a2ca03470f95169cf16f939018233a670519cb4ac58b0f \ --hash=sha256:bda89d5935c2eac546d648028b9901107a595863cb36bae0c73ac804a9b4ce88 # via black +tqdm==4.59.0 \ + --hash=sha256:9fdf349068d047d4cfbe24862c425883af1db29bcddf4b0eeb2524f6fbdb23c7 \ + --hash=sha256:d666ae29164da3e517fcf125e41d4fe96e5bb375cd87ff9763f6b38b5592fe33 + # via semgrep typed-ast==1.4.1 \ --hash=sha256:0666aa36131496aed8f7be0410ff974562ab7eeac11ef351def9ea6fa28f6355 \ --hash=sha256:0c2c07682d61a629b68433afb159376e24e5b2fd4641d35424e462169c0a7919 \ @@ -567,4 +634,7 @@ pip==20.3.3 \ setuptools==46.2.0 \ --hash=sha256:4df58bdc68f6c1d3527f24b89eaf09aaa977e0ed639893f485f75a9821178ec6 \ --hash=sha256:c3ca05451d860388f38572f9ff5f4f354ac9c2a1a69b2ba9dfb45a600761a481 - # via flake8 + # via + # flake8 + # jsonschema + # semgrep diff --git a/run.sh b/run.sh index d864b043e..fde2a9caf 100755 --- a/run.sh +++ b/run.sh @@ -1,6 +1,7 @@ #!/usr/bin/env bash set -eo pipefail +umask 077 while [ -n "$1" ]; do param="$1" diff --git a/securedrop_client/__init__.py b/securedrop_client/__init__.py index 6a9beea82..3d26edf77 100644 --- a/securedrop_client/__init__.py +++ b/securedrop_client/__init__.py @@ -1 +1 @@ -__version__ = "0.4.0" +__version__ = "0.4.1" diff --git a/securedrop_client/api_jobs/downloads.py b/securedrop_client/api_jobs/downloads.py index 65d9c710a..2ad639833 100644 --- a/securedrop_client/api_jobs/downloads.py +++ b/securedrop_client/api_jobs/downloads.py @@ -3,7 +3,6 @@ import logging import math import os -import shutil from tempfile import NamedTemporaryFile from typing import Any, Tuple, Type, Union @@ -20,6 +19,7 @@ mark_as_downloaded, set_message_or_reply_content, ) +from securedrop_client.utils import safe_move logger = logging.getLogger(__name__) @@ -161,14 +161,18 @@ def _download(self, api: API, db_object: Union[File, Message, Reply], session: S raise exception destination = db_object.location(self.data_dir) - os.makedirs(os.path.dirname(destination), mode=0o700, exist_ok=True) - shutil.move(download_path, destination) + safe_move(download_path, destination, self.data_dir) db_object.download_error = None mark_as_downloaded(type(db_object), db_object.uuid, session) logger.info("File downloaded to {}".format(destination)) return destination except BaseError as e: raise e + except (ValueError, FileNotFoundError, RuntimeError) as e: + logger.error(e) + raise DownloadDecryptionException( + f"Failed to download {db_object.uuid}", type(db_object), db_object.uuid + ) from e def _decrypt( self, filepath: str, db_object: Union[File, Message, Reply], session: Session @@ -184,6 +188,7 @@ def _decrypt( ) logger.info(f"File decrypted to {os.path.dirname(filepath)}") except CryptoError as e: + logger.error(e) mark_as_decrypted(type(db_object), db_object.uuid, session, is_decrypted=False) download_error = ( session.query(DownloadError) diff --git a/securedrop_client/crypto.py b/securedrop_client/crypto.py index 7889d86e4..0d21a13e7 100644 --- a/securedrop_client/crypto.py +++ b/securedrop_client/crypto.py @@ -15,19 +15,18 @@ along with this program. If not, see . """ -import gzip import logging import os -import shutil import struct import subprocess import tempfile +from pathlib import Path from sqlalchemy.orm import scoped_session from securedrop_client.config import Config from securedrop_client.db import Source -from securedrop_client.utils import safe_mkdir +from securedrop_client.utils import safe_copy, safe_gzip_extract, safe_mkdir logger = logging.getLogger(__name__) @@ -75,12 +74,16 @@ def read_gzip_header_filename(filename: str) -> str: class GpgHelper: + # We use a hardcoded temporary directory path in sd-app. Since sd-app is not a multi-user + # environment, we can safely assume that only the client is managing that filepath. + EXTRACTION_PATH = "/tmp" # nosec + def __init__(self, sdc_home: str, session_maker: scoped_session, is_qubes: bool) -> None: """ :param sdc_home: Home directory for the SecureDrop client :param is_qubes: Whether the client is running in Qubes or not """ - safe_mkdir(os.path.join(sdc_home), "gpg") + safe_mkdir(sdc_home, "gpg") self.sdc_home = sdc_home self.is_qubes = is_qubes self.session_maker = session_maker @@ -91,8 +94,13 @@ def __init__(self, sdc_home: str, session_maker: scoped_session, is_qubes: bool) def decrypt_submission_or_reply( self, filepath: str, plaintext_filepath: str, is_doc: bool = False ) -> str: - - original_filename, _ = os.path.splitext(os.path.splitext(os.path.basename(filepath))[0]) + """ + Decrypt the file located at the given filepath. If is_doc is False, store the decrypted + plaintext contents to plaintext_filepath in /tmp. Otherwise, unzip and extract the document + to the parent directory of plaintext_filepath. The document will be saved as the filename + in the gzip header if it exists otherwise the plaintext_filepath name will be used. + """ + original_filename = Path(Path(filepath).stem).stem # Remove one or two suffixes err = tempfile.NamedTemporaryFile(suffix=".message-error", delete=False) with tempfile.NamedTemporaryFile(suffix=".message") as out: @@ -119,16 +127,20 @@ def decrypt_submission_or_reply( # Delete encrypted file now that it's been successfully decrypted os.unlink(filepath) - # Store the plaintext in the file located at the specified plaintext_filepath + # If is_doc is True, unzip and extract the document to the parent directory of filepath. + # The document will be saved as the filename in the gzip header, which should contain + # the name of the original file that was gzipped. If the name is not in the header, use + # the filepath name. + # + # If is_doc is False, store the decrypted plaintext contents to the plaintext_filepath + # in /tmp that will automatically be deleted after decryption because it is a named + # temporary file. if is_doc: - original_filename = read_gzip_header_filename(out.name) or plaintext_filepath - decrypt_path = os.path.join( - os.path.dirname(filepath), os.path.basename(original_filename) - ) - with gzip.open(out.name, "rb") as infile, open(decrypt_path, "wb") as outfile: - shutil.copyfileobj(infile, outfile) + original_filename = read_gzip_header_filename(out.name) or original_filename + safe_gzip_extract(out.name, filepath, original_filename, self.sdc_home) else: - shutil.copy(out.name, plaintext_filepath) + # plaintext_filepath is a NamedTemporaryFile in /tmp so the base_dir is /tmp + safe_copy(out.name, plaintext_filepath, self.EXTRACTION_PATH) return original_filename diff --git a/securedrop_client/db.py b/securedrop_client/db.py index 867bbf27c..b4663af83 100644 --- a/securedrop_client/db.py +++ b/securedrop_client/db.py @@ -1,6 +1,7 @@ import datetime import os from enum import Enum +from pathlib import Path from typing import Any, List, Union # noqa: F401 from sqlalchemy import ( @@ -183,12 +184,12 @@ def location(self, data_dir: str) -> str: """ Return the full path to the Message's file. """ - return os.path.abspath( - os.path.join( - data_dir, - self.source.journalist_filename, - os.path.splitext(self.filename)[0] + ".txt", + return str( + Path(data_dir) + .joinpath( + Path(self.source.journalist_filename, Path(self.filename).with_suffix(".txt")) ) + .resolve() ) @property @@ -283,13 +284,16 @@ def location(self, data_dir: str) -> str: """ Return the full path to the File's file. """ - return os.path.abspath( - os.path.join( - data_dir, - self.source.journalist_filename, - "{}-{}-doc".format(self.file_counter, self.source.journalist_filename), - self.filename, + return str( + Path(data_dir) + .joinpath( + Path( + self.source.journalist_filename, + "{}-{}-doc".format(self.file_counter, self.source.journalist_filename), + self.filename, + ) ) + .resolve() ) @property @@ -390,12 +394,12 @@ def location(self, data_dir: str) -> str: """ Return the full path to the Reply's file. """ - return os.path.abspath( - os.path.join( - data_dir, - self.source.journalist_filename, - os.path.splitext(self.filename)[0] + ".txt", + return str( + Path(data_dir) + .joinpath( + Path(self.source.journalist_filename, Path(self.filename).with_suffix(".txt")) ) + .resolve() ) @property diff --git a/securedrop_client/gui/main.py b/securedrop_client/gui/main.py index 92d182bd0..3dee3bc77 100644 --- a/securedrop_client/gui/main.py +++ b/securedrop_client/gui/main.py @@ -20,6 +20,7 @@ along with this program. If not, see . """ import logging +import os from gettext import gettext as _ from typing import Dict, List, Optional # noqa: F401 @@ -55,7 +56,7 @@ def __init__(self) -> None: place for details / message contents / forms. """ super().__init__() - + os.umask(0o077) load_font("Montserrat") load_font("Source_Sans_Pro") self.setStyleSheet(load_css("sdclient.css")) diff --git a/securedrop_client/storage.py b/securedrop_client/storage.py index cef3b7351..f29f9c6f2 100644 --- a/securedrop_client/storage.py +++ b/securedrop_client/storage.py @@ -21,6 +21,7 @@ """ import logging import os +import re import shutil from datetime import datetime from pathlib import Path @@ -53,6 +54,13 @@ logger = logging.getLogger(__name__) +VALID_JOURNALIST_DESIGNATION = re.compile(r"^(?P[a-z'-]+) (?P[a-z'-]+)$").match + +VALID_FILENAME = re.compile( + r"^(?P\d+)\-[a-z0-9-_]*(?Pmsg|doc\.(gz|zip)|reply)\.gpg$" +).match + + def get_local_sources(session: Session) -> List[Source]: """ Return all source objects from the local database, newest first. @@ -112,6 +120,34 @@ def get_remote_data(api: API) -> Tuple[List[SDKSource], List[SDKSubmission], Lis return (remote_sources, remote_submissions, remote_replies) +def sanitize_submissions_or_replies( + remote_sdk_objects: Union[List[SDKSubmission], List[SDKReply]] +) -> Union[List[SDKSubmission], List[SDKReply]]: + """ + Return submissions or replies that contain invalid strings, e.g. '1-../../traversed-msg'. + """ + sanitized_sdk_objects = [] + for obj in remote_sdk_objects: + if not VALID_FILENAME(obj.filename): + logger.error(f"Malformed filename: {obj.filename}") + continue + sanitized_sdk_objects.append(obj) + return sanitized_sdk_objects + + +def sanitize_sources(remote_sdk_objects: List[SDKSource]) -> List[SDKSource]: + """ + Return sources that contain invalid strings, e.g. '1-../../traversed-msg'. + """ + sanitized_sdk_objects = [] + for obj in remote_sdk_objects: + if not VALID_JOURNALIST_DESIGNATION(obj.journalist_designation): + logger.error(f"Malformed journalist_designation: {obj.journalist_designation}") + continue + sanitized_sdk_objects.append(obj) + return sanitized_sdk_objects + + def update_local_storage( session: Session, remote_sources: List[SDKSource], @@ -124,6 +160,9 @@ def update_local_storage( replies from the SecureDrop API, ensures the local database is updated with this data. """ + remote_sources = sanitize_sources(remote_sources) + remote_submissions = sanitize_submissions_or_replies(remote_submissions) + remote_replies = sanitize_submissions_or_replies(remote_replies) remote_messages = [x for x in remote_submissions if x.filename.endswith("msg.gpg")] remote_files = [x for x in remote_submissions if not x.filename.endswith("msg.gpg")] diff --git a/securedrop_client/utils.py b/securedrop_client/utils.py index 9c568c771..c7bca364b 100644 --- a/securedrop_client/utils.py +++ b/securedrop_client/utils.py @@ -1,49 +1,174 @@ +import gzip import logging import math import os +import shutil import time from contextlib import contextmanager -from typing import Dict, Generator, Optional +from pathlib import Path +from typing import IO, Any, BinaryIO, Dict, Generator, Optional, Union from sqlalchemy.orm.session import Session from securedrop_client import db -def safe_mkdir(sdc_home: str, relative_path: str = None) -> None: +def safe_mkdir( + base_path: Union[Path, str], relative_path: Union[Optional[Path], Optional[str]] = None +) -> None: """ - Safely create directories while checking permissions along the way. + 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: - full_path = os.path.join(sdc_home, relative_path) + check_path_traversal(relative_path) + full_path = base_path.joinpath(relative_path) else: - full_path = sdc_home + 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 Window (QMainWindow) 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_gzip_extract( + gzip_file_path: str, dest_path: str, original_filename: str, base_path: str +) -> None: + """ + Safely unzip a file specified by gzip_file_path to dest_path, replacing filename with + original_filename. + """ + dest_dir = Path(dest_path).parent + safe_mkdir(base_path, str(dest_dir)) + + dest_path_with_original_filename = dest_dir.joinpath(original_filename) + with gzip.open(gzip_file_path, "rb") as src_file, open( + dest_path_with_original_filename, "wb" + ) as dest_file: + safe_copyfileobj(src_file, dest_file, base_path) + - if not full_path == os.path.abspath(full_path): - raise ValueError("Path is not absolute: {}".format(full_path)) +def safe_move(src_path: str, dest_path: str, dest_base_path: str) -> None: + """ + Safely move src_path to dest_path. + """ + check_path_traversal(src_path) + check_path_traversal(dest_path) + # Ensure directories in dest_path are created safely if they don't exist + safe_mkdir(dest_base_path, Path(dest_path).parent) + shutil.move(src_path, dest_path) + Path(dest_path).chmod(0o600) + + +def safe_copy(src_path: str, dest_path: str, dest_base_path: str) -> None: + """ + Safely copy a file specified by src_path to dest_path. - if not os.path.exists(sdc_home): - os.makedirs(sdc_home, 0o700) + TODO: Figure out a clearer way to safely copy to a temporary file that gets + deleted right away. We may need a safe_decrypt function in the future. + """ + check_path_traversal(src_path) + check_path_traversal(dest_path) + relative_filepath(dest_path, dest_base_path) + shutil.copy(src_path, dest_path) + Path(dest_path).chmod(0o600) + + +def safe_copyfileobj(src_file: IO[Any], dest_file: BinaryIO, dest_base_path: str) -> None: + """ + Safely copy src_file to dest_file. + """ + check_path_traversal(src_file.name) + check_path_traversal(dest_file.name) + # Ensure directories of dest_file are created safely if they don't exist + safe_mkdir(dest_base_path, Path(dest_file.name).parent) + shutil.copyfileobj(src_file, dest_file) + Path(dest_file.name).chmod(0o600) - check_dir_permissions(sdc_home) - if not relative_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 + # base directory, 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_components = split_path(relative_path) + Path(full_path).chmod(0o700) + check_dir_permissions(full_path) - path_so_far = sdc_home - for component in path_components: - path_so_far = os.path.join(path_so_far, component) - check_dir_permissions(path_so_far) - os.makedirs(path_so_far, 0o0700, exist_ok=True) + 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: str) -> None: +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. + 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 @@ -52,17 +177,6 @@ def check_dir_permissions(dir_path: str) -> None: raise RuntimeError("Unsafe permissions ({}) on {}".format(oct(stat_res), dir_path)) -def split_path(path: str) -> list: - out = [] - - while path: - path, tail = os.path.split(path) - out.append(tail) - - out.reverse() - return out - - def humanize_filesize(filesize: int) -> str: """ Returns a human readable string of a filesize diff --git a/setup.py b/setup.py index 485fb9c85..1fd7b6985 100644 --- a/setup.py +++ b/setup.py @@ -14,7 +14,7 @@ setuptools.setup( name="securedrop-client", - version="0.4.0", + version="0.4.1", author="Freedom of the Press Foundation", author_email="securedrop@freedom.press", description="SecureDrop Workstation client application", diff --git a/tests/api_jobs/test_downloads.py b/tests/api_jobs/test_downloads.py index 168e7b518..e24a627d7 100644 --- a/tests/api_jobs/test_downloads.py +++ b/tests/api_jobs/test_downloads.py @@ -521,6 +521,42 @@ def fake_download(sdk_obj: SdkSubmission, timeout: int) -> Tuple[str, str]: assert mock_decrypt.called +def test_FileDownloadJob_raises_on_path_traversal_attack( + mocker, homedir, session, session_maker, download_error_codes +): + source = factory.Source() + file = factory.File(source=source, is_downloaded=None, is_decrypted=None) + session.add(source) + session.add(file) + session.commit() + + gpg = GpgHelper(homedir, session_maker, is_qubes=False) + decrypt_fn = mocker.patch.object(gpg, "decrypt_submission_or_reply", side_effect=CryptoError) + api_client = mocker.MagicMock() + download_fn = mocker.patch.object(api_client, "download_reply") + + def fake_download(sdk_obj: SdkSubmission, timeout: int) -> Tuple[str, str]: + """ + :return: (etag, path-to-download) + """ + full_path = os.path.join( + homedir, "data", "1-../../../../../../../../../tmp/INJECTED_dissolved-dandelion-doc.gpg" + ) + return ("", full_path) + + api_client = mocker.MagicMock() + api_client.default_request_timeout = mocker.MagicMock() + api_client.download_submission = fake_download + + job = FileDownloadJob(file.uuid, os.path.join(homedir, "data"), gpg) + + with pytest.raises(DownloadDecryptionException): + job.call_api(api_client, session) + + download_fn.assert_not_called() + decrypt_fn.assert_not_called() + + def test_timeout_length_of_file_downloads(mocker, homedir, session, session_maker): """ Ensure that files downloads have timeouts scaled by the size of the file. diff --git a/tests/factory.py b/tests/factory.py index 90cc8dc5e..74b58a800 100644 --- a/tests/factory.py +++ b/tests/factory.py @@ -177,7 +177,7 @@ def RemoteSource(**attrs): interaction_count=0, is_flagged=False, is_starred=True, - journalist_designation="testy-mctestface", + journalist_designation="testerino testy-mctestface", key={"public": pub_key, "fingerprint": "B2FF7FB28EED8CABEBC5FB6C6179D97BCFA52E5F"}, last_updated=datetime.now().isoformat(), number_of_documents=0, diff --git a/tests/gui/test_widgets.py b/tests/gui/test_widgets.py index afe021f71..82657b428 100644 --- a/tests/gui/test_widgets.py +++ b/tests/gui/test_widgets.py @@ -2575,7 +2575,7 @@ def test_SpeechBubble_init(mocker): mock_download_error_connect = mocker.Mock() mock_download_error_signal.connect = mock_download_error_connect - sb = SpeechBubble("mock id", "hello", mock_update_signal, mock_download_error_signal, 0, 123,) + sb = SpeechBubble("mock id", "hello", mock_update_signal, mock_download_error_signal, 0, 123) sb.message.text() == "hello" assert mock_update_connect.called diff --git a/tests/migrations/utils.py b/tests/migrations/utils.py index ba49348f8..dc89bdafa 100644 --- a/tests/migrations/utils.py +++ b/tests/migrations/utils.py @@ -256,10 +256,7 @@ def add_reply(session: Session, journalist_id: int, source_id: int) -> None: def mark_file_as_seen(session: Session, file_id: int, journalist_id: int) -> None: - params = { - "file_id": file_id, - "journalist_id": journalist_id, - } + params = {"file_id": file_id, "journalist_id": journalist_id} sql = """ INSERT INTO seen_files (file_id, journalist_id) VALUES (:file_id, :journalist_id) @@ -268,10 +265,7 @@ def mark_file_as_seen(session: Session, file_id: int, journalist_id: int) -> Non def mark_message_as_seen(session: Session, message_id: int, journalist_id: int) -> None: - params = { - "message_id": message_id, - "journalist_id": journalist_id, - } + params = {"message_id": message_id, "journalist_id": journalist_id} sql = """ INSERT INTO seen_messages (message_id, journalist_id) VALUES (:message_id, :journalist_id) @@ -280,10 +274,7 @@ def mark_message_as_seen(session: Session, message_id: int, journalist_id: int) def mark_reply_as_seen(session: Session, reply_id: int, journalist_id: int): - params = { - "reply_id": reply_id, - "journalist_id": journalist_id, - } + params = {"reply_id": reply_id, "journalist_id": journalist_id} sql = """ INSERT INTO seen_replies (reply_id, journalist_id) VALUES (:reply_id, :journalist_id) diff --git a/tests/test_app.py b/tests/test_app.py index 92a4396cb..0ca659f7a 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -189,11 +189,7 @@ def test_create_app_dir_permissions(tmpdir, mocker): def func(): start_app(mock_args, mock_qt_args) - if case["should_pass"]: - func() - else: - with pytest.raises(RuntimeError): - func() + func() # stop all mocks before the next iteration mocker.stopall() diff --git a/tests/test_crypto.py b/tests/test_crypto.py index f582a4e27..7f8072b07 100644 --- a/tests/test_crypto.py +++ b/tests/test_crypto.py @@ -73,9 +73,6 @@ def test_gzip_header_without_filename(homedir, config, mocker, session_maker): gpg._import(JOURNO_KEY) mocker.patch("os.unlink") - mocker.patch("gzip.open") - mocker.patch("shutil.copy") - mocker.patch("shutil.copyfileobj") # pretend the gzipped file header lacked the original filename mock_read_gzip_header_filename = mocker.patch( diff --git a/tests/test_logic.py b/tests/test_logic.py index ac24ccb82..afa2d6f16 100644 --- a/tests/test_logic.py +++ b/tests/test_logic.py @@ -1077,14 +1077,6 @@ def test_Controller_set_activity_status(homedir, config, mocker, session_maker): mock_gui.update_activity_status.assert_called_once_with("Hello, World!", 1000) -PERMISSIONS_CASES = [ - {"should_pass": True, "home_perms": None}, - {"should_pass": True, "home_perms": 0o0700}, - {"should_pass": False, "home_perms": 0o0740}, - {"should_pass": False, "home_perms": 0o0704}, -] - - def test_create_client_dir_permissions(tmpdir, mocker, session_maker): """ Check that creating an app behaves appropriately with different @@ -1098,12 +1090,20 @@ def test_create_client_dir_permissions(tmpdir, mocker, session_maker): mock_open = mocker.patch("securedrop_client.config.open") mock_json = mocker.patch("securedrop_client.config.json.loads") - for idx, case in enumerate(PERMISSIONS_CASES): + permission_cases = [ + {"should_pass": True, "home_perms": None}, + {"should_pass": True, "home_perms": 0o0700}, + {"should_pass": False, "home_perms": 0o0740}, + {"should_pass": False, "home_perms": 0o0704}, + ] + + for idx, case in enumerate(permission_cases): sdc_home = os.path.join(str(tmpdir), "case-{}".format(idx)) # optionally create the dir if case["home_perms"] is not None: - os.mkdir(sdc_home, case["home_perms"]) + os.mkdir(sdc_home) + os.chmod(sdc_home, case["home_perms"]) def func() -> None: Controller("http://localhost", mock_gui, session_maker, sdc_home) diff --git a/tests/test_storage.py b/tests/test_storage.py index 2402fec89..aa5c3ad58 100644 --- a/tests/test_storage.py +++ b/tests/test_storage.py @@ -56,7 +56,7 @@ def make_remote_message(source_uuid, file_counter=1): source_url = "/api/v1/sources/{}".format(source_uuid) return Submission( download_url="test", - filename="{}-submission.msg.gpg".format(file_counter), + filename="{}-submission-msg.gpg".format(file_counter), is_read=False, size=123, source_url=source_url, @@ -228,14 +228,13 @@ def test_get_remote_data(mocker): def test_update_local_storage(homedir, mocker, session_maker): """ - Assuming no errors getting data, check the expected functions to update - the state of the local database are called with the necessary data. + Check that update functions are called with expected remote sources and submissions. """ remote_source = factory.RemoteSource() - remote_message = mocker.Mock(filename="1-foo.msg.gpg") - remote_file = mocker.Mock(filename="2-foo.gpg") + remote_message = mocker.Mock(filename="1-foo-msg.gpg") + remote_file = mocker.Mock(filename="2-foo-doc.gz.gpg") remote_submissions = [remote_message, remote_file] - remote_reply = mocker.MagicMock() + remote_reply = mocker.MagicMock(filename="3-foo-reply.gpg") # Some local source, submission and reply objects from the local database. mock_session = mocker.MagicMock() local_source = mocker.MagicMock() @@ -252,12 +251,36 @@ def test_update_local_storage(homedir, mocker, session_maker): msg_fn = mocker.patch("securedrop_client.storage.update_messages") update_local_storage(mock_session, [remote_source], remote_submissions, [remote_reply], homedir) + src_fn.assert_called_once_with([remote_source], [local_source], mock_session, homedir) rpl_fn.assert_called_once_with([remote_reply], [local_reply], mock_session, homedir) file_fn.assert_called_once_with([remote_file], [local_file], mock_session, homedir) msg_fn.assert_called_once_with([remote_message], [local_message], mock_session, homedir) +def test_update_local_storage_sanitizes_remote_data(mocker, homedir): + """ + Check that sanitize functions are called with expected remote sources and submissions. + """ + remote_source = factory.RemoteSource() + remote_message = mocker.Mock(filename="1-foo-msg.gpg") + remote_file = mocker.Mock(filename="2-foo-doc.gz.gpg") + remote_submissions = [remote_message, remote_file] + remote_reply = mocker.MagicMock(filename="3-foo-reply.gpg") + sanitize_sources = mocker.patch("securedrop_client.storage.sanitize_sources") + sanitize_submissions_or_replies = mocker.patch( + "securedrop_client.storage.sanitize_submissions_or_replies" + ) + + update_local_storage( + mocker.MagicMock(), [remote_source], remote_submissions, [remote_reply], homedir + ) + + sanitize_sources.assert_called_once_with([remote_source]) + sanitize_submissions_or_replies.call_args_list[0][0][0] == [remote_submissions] + sanitize_submissions_or_replies.call_args_list[1][0][0] == [remote_reply] + + def test_sync_delete_race(homedir, mocker, session_maker, session): """ Test a race between sync and source deletion (#797). @@ -458,6 +481,45 @@ def test_update_submissions_deletes_files_associated_with_the_submission(homedir assert mock_session.commit.call_count == 1 +def test_update_local_storage_does_not_call_update_functions_w_insecure_filenames(mocker, homedir): + """ + Check that a insecure journalist_designation or filename won't be written to the db. + """ + remote_source = factory.RemoteSource(journalist_designation="../../../../../../tmp/INJECTED") + remote_message = mocker.Mock( + filename="1-../../../../../../../../tmp/INJECTED_dissolved-dandelion-msg.gpg" + ) + remote_file = mocker.Mock( + filename="2-../../../../../../../../../tmp/INJECTED_dissolved-dandelion-doc.gz.gpg" + ) + remote_reply = mocker.MagicMock( + filename="1-../../../../../../../../tmp/INJECTED_dissolved-dandelion-reply.gpg" + ) + # Some local source, submission and reply objects from the local database. + session = mocker.MagicMock() + local_source = mocker.MagicMock() + local_file = mocker.MagicMock() + local_message = mocker.MagicMock() + local_reply = mocker.MagicMock() + session.query().all = mocker.Mock() + session.query().all.side_effect = [[local_file], [local_message], [local_reply]] + session.query().order_by().all = mocker.Mock() + session.query().order_by().all.side_effect = [[local_source]] + src_fn = mocker.patch("securedrop_client.storage.update_sources") + rpl_fn = mocker.patch("securedrop_client.storage.update_replies") + file_fn = mocker.patch("securedrop_client.storage.update_files") + msg_fn = mocker.patch("securedrop_client.storage.update_messages") + + update_local_storage( + session, [remote_source], [remote_message, remote_file], [remote_reply], homedir + ) + + src_fn.assert_called_once_with([], [local_source], session, homedir) + rpl_fn.assert_called_once_with([], [local_reply], session, homedir) + file_fn.assert_called_once_with([], [local_file], session, homedir) + msg_fn.assert_called_once_with([], [local_message], session, homedir) + + def test_update_replies_deletes_files_associated_with_the_reply(homedir, mocker): """ Check that: @@ -787,10 +849,7 @@ def test_update_files_marks_read_files_as_seen_without_seen_records(homedir, moc is_read=1, ) - remote_files = [ - remote_file_to_update, - remote_file_to_create, - ] + remote_files = [remote_file_to_update, remote_file_to_create] update_files(remote_files, local_files, session, data_dir) @@ -900,14 +959,11 @@ def test_update_messages_marks_read_messages_as_seen_without_seen_records(homedi source_uuid=source.uuid, source_url="/api/v1/sources/{}".format(source.uuid), file_counter=factory.FILE_COUNT + 1, - filename="{}-msg.gpg".format(factory.FILE_COUNT + 1), + filename="{}-msg.gpg".format(2), is_read=1, ) - remote_messages = [ - remote_message_to_update, - remote_message_to_create, - ] + remote_messages = [remote_message_to_update, remote_message_to_create] update_messages(remote_messages, local_messages, session, data_dir) diff --git a/tests/test_utils.py b/tests/test_utils.py index b9af07d73..c2049cfad 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -1,13 +1,17 @@ -import pytest - -from securedrop_client.utils import humanize_filesize, safe_mkdir - +import os +import tempfile +from pathlib import Path -def test_safe_makedirs_non_absolute(homedir): - with pytest.raises(ValueError) as e_info: - safe_mkdir(homedir, "..") +import pytest - assert "not absolute" in str(e_info.value) +from securedrop_client.utils import ( + check_all_permissions, + check_dir_permissions, + check_path_traversal, + humanize_filesize, + relative_filepath, + safe_mkdir, +) def test_humanize_file_size_bytes(): @@ -26,3 +30,280 @@ def test_humanize_file_size_megabytes(): expected_humanized_filesize = "123MB" actual_humanized_filesize = humanize_filesize(123 * 1024 * 1024) assert expected_humanized_filesize == actual_humanized_filesize + + +def test_safe_mkdir_with_unsafe_path(homedir): + """ + Ensure an error is raised if the path contains path traversal string. + """ + with pytest.raises(ValueError) as e_info: + safe_mkdir(homedir, "..") + + assert "Unsafe file or directory name: '..'" in str(e_info.value) + + +def test_safe_mkdir_with_base_dir_that_is_not_absolute(): + """ + Ensure an error is raised if the base dir is not an absolute path. + """ + with pytest.raises(ValueError) as e_info: + safe_mkdir("this/is/a/relative/path", "test") + + assert "Base directory 'this/is/a/relative/path' must be an absolute path" in str(e_info.value) + + +def test_safe_mkdir_with_first_param_containing_path_traversal_attack(): + """ + Test that safe_mkdir rejects a filename that could be used in a path traversal attack. + """ + with pytest.raises(ValueError) as e: + safe_mkdir("../../../../../../traversed") + assert f"traversed is not an absolute path" in str(e.value) + + +def test_safe_mkdir_with_second_param_containing_path_traversal_attack(): + """ + Test that safe_mkdir rejects a filename that could be used in a path traversal attack. + """ + with tempfile.TemporaryDirectory() as temp_dir: + with pytest.raises(ValueError) as e: + safe_mkdir(temp_dir, "../../../../../../traversed") + assert f"'/traversed' does not start with '{temp_dir}'" in str(e.value) + + +def test_safe_mkdir_with_second_param_containing_path_traversal_attack_with_preexisting_dirs(): + """ + Test that safe_mkdir rejects a filename that could be used in a path traversal attack + when the home directory already exists. + """ + with tempfile.TemporaryDirectory() as should_not_traverse_here: + homedir = os.path.join(should_not_traverse_here, "homedir") + os.mkdir(homedir) + + with pytest.raises(ValueError) as e: + safe_mkdir(homedir, "../traversed") + assert f"'{should_not_traverse_here}/traversed' does not start with '{homedir}'" in str( + e.value + ) + + +def test_safe_mkdir_with_base_dir_with_parent_dirs_that_do_not_exist(): + """ + Ensure an error is raised if the parent directories of base dir do not exist. + """ + with pytest.raises(FileNotFoundError) as e_info: + safe_mkdir("/this/does/not/exist", "test") + + assert e_info.type == FileNotFoundError + assert "No such file or directory: '/this/does/not/exist'" in str(e_info.value) + + +def test_safe_mkdir_with_base_dir_that_does_not_exist(): + """ + Ensure you can create a base dir if parent directories already exist. + """ + safe_mkdir("/tmp/does-not-exist", "test") + + +def test_safe_mkdir_happy_path_with_secure_permissions(): + """ + Test that safe_mkdir works with one param. + """ + with tempfile.TemporaryDirectory() as temp_dir: + safe_mkdir(temp_dir) + safe_mkdir(temp_dir, "test") + assert oct(os.stat(temp_dir).st_mode) == "0o40700" + assert oct(os.stat(os.path.join(temp_dir, "test")).st_mode) == "0o40700" + + with tempfile.TemporaryDirectory() as temp_dir: + safe_mkdir(temp_dir, "test") + assert oct(os.stat(temp_dir).st_mode) == "0o40700" + assert oct(os.stat(os.path.join(temp_dir, "test")).st_mode) == "0o40700" + + +def test_safe_mkdir_sets_secure_permissions_for_each_directory_in_rel_path(): + """ + Test safe directory permissions when two paths are supplied. + """ + with tempfile.TemporaryDirectory() as temp_dir: + safe_mkdir(temp_dir, "check/each/dir/in/path") + full_path = os.path.join(temp_dir, "check/each/dir/in/path") + assert os.path.exists(full_path) + assert oct(os.stat(temp_dir).st_mode) == "0o40700" + assert oct(os.stat(os.path.join(temp_dir, "check")).st_mode) == "0o40700" + assert oct(os.stat(os.path.join(temp_dir, "check/each")).st_mode) == "0o40700" + assert oct(os.stat(os.path.join(temp_dir, "check/each/dir")).st_mode) == "0o40700" + + +def test_safe_mkdir_fixes_insecure_permissions_on_base_dir(homedir): + """ + Test that safe_mkdir fixes insecure permissions on base dir. + """ + with tempfile.TemporaryDirectory() as temp_dir: + insecure_base_path = os.path.join(temp_dir, "base_dir") + os.mkdir(insecure_base_path, 0o777) + + safe_mkdir(insecure_base_path) + + assert oct(os.stat(insecure_base_path).st_mode) == "0o40700" + + +def test_safe_mkdir_leaves_parent_dir_permissions_alone_on_base_path(homedir): + """ + Test that safe_mkdir leaves base_dir parent permissions alone. + """ + with tempfile.TemporaryDirectory() as temp_dir: + os.chmod(temp_dir, 0o777) + + base_path = os.path.join(temp_dir, "base_dir") + safe_mkdir(base_path) + + assert oct(os.stat(temp_dir).st_mode) == "0o40777" + + +def test_safe_mkdir_fixes_insecure_permissions_on_rel_dir(homedir): + """ + Test that safe_mkdir raises when a parent directory has insecure permissions. + """ + with tempfile.TemporaryDirectory() as temp_dir: + os.mkdir(os.path.join(temp_dir, "rel"), 0o777) + + safe_mkdir(temp_dir, "rel") + + fixed_dir = Path(temp_dir).joinpath("rel") + assert oct(os.stat(fixed_dir).st_mode) == "0o40700" + + +def test_safe_mkdir_fixes_insecure_permissions_on_parent_dir(homedir): + """ + Test that safe_mkdir raises when a parent directory has insecure permissions. + """ + with tempfile.TemporaryDirectory() as temp_dir: + os.mkdir(os.path.join(temp_dir, "rel"), 0o700) + os.mkdir(os.path.join(temp_dir, "rel/path"), 0o700) + os.mkdir(os.path.join(temp_dir, "rel/path/test"), 0o777) + + safe_mkdir(temp_dir, "rel/path/test") + + fixed_dir = Path(temp_dir).joinpath("rel/path/test") + assert oct(os.stat(fixed_dir).st_mode) == "0o40700" + + +def test_safe_mkdir_fixes_insecure_permissions_on_inner_parent_dir(homedir): + """ + Test that safe_mkdir raises when a parent directory has insecure permissions. + """ + with tempfile.TemporaryDirectory() as temp_dir: + os.mkdir(os.path.join(temp_dir, "rel"), 0o700) + os.mkdir(os.path.join(temp_dir, "rel/path"), 0o777) + os.mkdir(os.path.join(temp_dir, "rel/path/test"), 0o700) + + safe_mkdir(temp_dir, "rel/path/test") + + fixed_dir = Path(temp_dir).joinpath("rel/path") + assert oct(os.stat(fixed_dir).st_mode) == "0o40700" + + +def test_safe_mkdir_fixes_insecure_permissions_on_last_parent_dir(homedir): + """ + Test that safe_mkdir raises when a parent directory has insecure permissions. + """ + with tempfile.TemporaryDirectory() as temp_dir: + os.mkdir(os.path.join(temp_dir, "rel"), 0o777) + os.mkdir(os.path.join(temp_dir, "rel/path"), 0o700) + os.mkdir(os.path.join(temp_dir, "rel/path/test"), 0o700) + + safe_mkdir(temp_dir, "rel/path/test") + + fixed_dir = Path(temp_dir).joinpath("rel") + assert oct(os.stat(fixed_dir).st_mode) == "0o40700" + + +def test_relative_filepath(): + p = relative_filepath("/good/path", "/good") + assert str(p) == "path" + + p = relative_filepath("/good/path", "/good/path") + assert str(p) == "." + + p = relative_filepath("/good", "/") + assert str(p) == "good" + + p = relative_filepath("/", "/") + assert str(p) == "." + + with pytest.raises(ValueError) as e: + relative_filepath("", "") + assert "'' does not start with ''" in str(e.value) # base dir must be absolute + + with pytest.raises(ValueError) as e: + relative_filepath("/base_dir", "../base_dir/must/be/absolute") + assert "'/base/dir' does not start with '../base/dir/must/be/absolute'" in str(e.value) + + with pytest.raises(ValueError) as e: + relative_filepath("/bad/path", "/basedir") + assert "Unsafe file or directory name" in str(e.value) + + with pytest.raises(ValueError) as e: + relative_filepath("/bad/path", "/basedir") + assert "Unsafe file or directory name" in str(e.value) + + +def test_check_path_traversal(): + check_path_traversal("/good/path") + check_path_traversal("good/path") + + with pytest.raises(ValueError) as e: + check_path_traversal("../traversed") + assert "Unsafe file or directory name" in str(e.value) + + with pytest.raises(ValueError) as e: + check_path_traversal("x/../../traversed") + assert "Unsafe file or directory name" in str(e.value) + + with pytest.raises(ValueError) as e: + check_path_traversal("/x/../../traversed") + assert "Unsafe file or directory name" in str(e.value) + + # Ultimately this traversal is safe but check_path_traversal only returns + # successfully if there are no traversal attempts. See check_path_traversal + # to understand behavior and reasoning behind design decision. + with pytest.raises(ValueError) as e: + check_path_traversal("x/../traversed") + assert "Unsafe file or directory name" in str(e.value) + + with pytest.raises(ValueError) as e: + check_path_traversal("/x/../traversed") + assert "Unsafe file or directory name" in str(e.value) + + +def test_check_all_permissions(): + check_all_permissions("/this/path/does/not/exist/so/just/return", "/this/path") + + with tempfile.TemporaryDirectory() as temp_dir: + os.mkdir(os.path.join(temp_dir, "bad"), 0o777) + os.mkdir(os.path.join(temp_dir, "not_good"), 0o755) + os.mkdir(os.path.join(temp_dir, "not_good/good"), 0o700) + os.mkdir(os.path.join(temp_dir, "good"), 0o700) + + check_all_permissions(os.path.join(temp_dir, "bad"), temp_dir) + assert oct(os.stat(os.path.join(temp_dir, "bad")).st_mode) == "0o40700" + + check_all_permissions(os.path.join(temp_dir, "not_good"), temp_dir) + assert oct(os.stat(os.path.join(temp_dir, "not_good")).st_mode) == "0o40700" + + check_all_permissions(os.path.join(temp_dir, "not_good/good"), temp_dir) + assert oct(os.stat(os.path.join(temp_dir, "not_good")).st_mode) == "0o40700" + + check_all_permissions(os.path.join(temp_dir, "good"), temp_dir) + + +def test_check_dir_permissions_unsafe(mocker): + check_all_permissions("/this/path/does/not/exist/so/just/return", "/this/path") + + with tempfile.TemporaryDirectory() as temp_dir: + os.mkdir(os.path.join(temp_dir, "bad")) + os.chmod(os.path.join(temp_dir, "bad"), 0o0777) + + with pytest.raises(RuntimeError): + check_dir_permissions(os.path.join(temp_dir, "bad"))