Merge pull request #1226 from freedomofpress/check-paths

Check paths and ensure minimal perms
freedomofpress · Mar 17, 2021 · 94efe51 · 94efe51
2 parents 7e71b30 + b27707c
commit 94efe51
Show file tree

Hide file tree

Showing 23 changed files with 860 additions and 132 deletions.
diff --git a/.semgrep/custom-rules.yml 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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)