Merge pull request #33 from guardian/pm-fpf-0.14.1

Release version 0.14.1 of freedomofpress/securedrop-client

changelog.md

@@ -1,5 +1,16 @@
 # Changelog
 
+## 0.14.1
+
+This release contains fixes for two security issues; please see our advisory for more details.
+
+* Security fixes:
+  * Prevent path manipulation/traversal attacks in SDK (CVE-2025-24888)
+  * Don't send source VM name to sd-log (CVE-2025-24889)
+
+* Miscellaneous:
+  * Update year in messages.pot
+
 ## 0.14.0
 
 * Add support for selecting and deleting multiple sources (#2208, #2188, #2230, #2252, #2293, #2299, #2300)

client/securedrop_client/__init__.py

@@ -1 +1 @@
-__version__ = "106.14.0"
+__version__ = "106.14.1"

client/securedrop_client/api_jobs/uploads.py

@@ -16,7 +16,7 @@
     User,
 )
 from securedrop_client.sdk import API, RequestTimeoutError, ServerConnectionError
-from securedrop_client.storage import update_draft_replies
+from securedrop_client.storage import VALID_FILENAME, update_draft_replies
 
 logger = logging.getLogger(__name__)
 
@@ -76,33 +76,29 @@ def call_api(self, api_client: API, session: Session) -> str:
             encrypted_reply = self.gpg.encrypt_to_source(self.source_uuid, self.message)
             sdk_reply = self._make_call(encrypted_reply, api_client)
 
-            # Create a new reply object with an updated filename and file counter
-            interaction_count = source.interaction_count + 1
-            filename = f"{interaction_count}-{source.journalist_designation}-reply.gpg"
-
+            # Create a new reply object.  Since the server is authoritative for
+            # the ordering (Reply.file_count) embedded in the filename, we might
+            # as well use the filename it returns, but let's validate it just as
+            # we do in storage.sanitize_submissions_or_replies().
+            if not VALID_FILENAME(sdk_reply.filename):
+                raise ValueError(f"Malformed filename: {sdk_reply.filename}")
             reply_db_object = Reply(
                 uuid=self.reply_uuid,
                 source_id=source.id,
-                filename=filename,
+                filename=sdk_reply.filename,
                 journalist_id=sender.id,
                 content=self.message,
                 is_downloaded=True,
                 is_decrypted=True,
             )
-            new_file_counter = int(sdk_reply.filename.split("-")[0])
-            reply_db_object.file_counter = new_file_counter
-            reply_db_object.filename = sdk_reply.filename
 
             # Update following draft replies for the same source to reflect the new reply count
-            draft_file_counter = draft_reply_db_object.file_counter
-            draft_timestamp = draft_reply_db_object.timestamp
-
             update_draft_replies(
                 session,
                 source.id,
-                draft_timestamp,
-                draft_file_counter,
-                new_file_counter,
+                draft_reply_db_object.timestamp,
+                draft_reply_db_object.file_counter,
+                reply_db_object.file_counter,
                 commit=False,
             )
 

client/securedrop_client/sdk/__init__.py

@@ -10,6 +10,7 @@
 from pathlib import Path
 from typing import Any
 
+from securedrop_client import utils
 from securedrop_client.config import Config
 
 from .sdlocalobjects import (
@@ -57,11 +58,10 @@ def __reduce__(self) -> tuple[type, tuple]:
 
 @dataclass(frozen=True)
 class StreamedResponse:
-    """Container for streamed data along with the filename and ETag checksum sent by the server."""
+    """Container for streamed data along with the ETag checksum sent by the server."""
 
     contents: bytes
     sha256sum: str
-    filename: str
 
 
 @dataclass(frozen=True)
@@ -238,16 +238,13 @@ def _streaming_download(
                         raise BaseError("Unable to parse headers (stderr) JSON") from err
 
                     sha256sum = stderr["headers"]["etag"]
-                    filename = stderr["headers"]["content-disposition"]
                 else:
                     # This should never happen because we should always have an stderr
                     sha256sum = ""
-                    filename = ""
 
                 return StreamedResponse(
                     contents=contents,
                     sha256sum=sha256sum,
-                    filename=filename,
                 )
 
             except subprocess.TimeoutExpired as err:
@@ -691,15 +688,15 @@ def delete_submission(self, submission: Submission) -> bool:
         return False
 
     def download_submission(
-        self, submission: Submission, path: str | None = None, timeout: int | None = None
+        self, submission: Submission, original_path: str | None = None, timeout: int | None = None
     ) -> tuple[str, str]:
         """
         Returns a tuple of etag (format is algorithm:checksum) and file path for
         a given Submission object. This method requires a directory path
         at which to save the submission file.
 
         :param submission: Submission object
-        :param path: Local directory path to save the submission, if None, use ~/Downloads
+        :param original_path: Local directory path to save the submission, if None, use ~/Downloads
 
         :returns: Tuple of etag and path of the saved submission.
         """
@@ -708,10 +705,12 @@ def download_submission(
         )
         method = "GET"
 
-        if not path:
-            path = os.path.expanduser("~/Downloads")
+        if original_path:
+            path = Path(original_path)
+        else:
+            path = Path("~/Downloads").expanduser()
 
-        if not os.path.isdir(path):
+        if not path.is_dir():
             raise BaseError(f"Specified path isn't a directory: {path}")
 
         response = self._send_json_request(
@@ -728,10 +727,13 @@ def download_submission(
             else:
                 raise BaseError(f"Unknown error, status code: {response.status}")
 
-        filepath = os.path.join(path, submission.filename)
-        Path(filepath).write_bytes(response.contents)
+        filepath = path / submission.filename
+        # submission.filename should have already been validated, but let's
+        # double check before we write anything
+        utils.check_path_traversal(filepath)
+        filepath.write_bytes(response.contents)
 
-        return response.sha256sum.strip('"'), filepath
+        return response.sha256sum.strip('"'), str(filepath)
 
     def flag_source(self, source: Source) -> bool:
         """
@@ -917,23 +919,25 @@ def get_all_replies(self) -> list[Reply]:
 
         return result
 
-    def download_reply(self, reply: Reply, path: str | None = None) -> tuple[str, str]:
+    def download_reply(self, reply: Reply, original_path: str | None = None) -> tuple[str, str]:
         """
         Returns a tuple of etag (format is algorithm:checksum) and file path for
         a given Reply object. This method requires a directory path
         at which to save the reply file.
 
         :param reply: Reply object
-        :param path: Local directory path to save the reply
+        :param original_path: Local directory path to save the reply
 
         :returns: Tuple of etag and path of the saved Reply.
         """
         path_query = f"api/v1/sources/{reply.source_uuid}/replies/{reply.uuid}/download"
 
         method = "GET"
 
-        if not path:
-            path = os.path.expanduser("~/Downloads")
+        if original_path:
+            path = Path(original_path)
+        else:
+            path = Path("~/Downloads").expanduser()
 
         if not os.path.isdir(path):
             raise BaseError(f"Specified path isn't a directory: {path}")
@@ -952,11 +956,13 @@ def download_reply(self, reply: Reply, path: str | None = None) -> tuple[str, st
             else:
                 raise BaseError(f"Unknown error, status code: {response.status}")
 
-        # This is where we will save our downloaded file
-        filepath = os.path.join(path, response.filename.split("attachment; filename=")[1])
-        Path(filepath).write_bytes(response.contents)
+        filepath = path / reply.filename
+        # reply.filename should have already been validated, but let's
+        # double check before we write anything
+        utils.check_path_traversal(filepath)
+        filepath.write_bytes(response.contents)
 
-        return response.sha256sum.strip('"'), filepath
+        return response.sha256sum.strip('"'), str(filepath)
 
     def delete_reply(self, reply: Reply) -> bool:
         """

client/tests/api_jobs/test_uploads.py

@@ -41,7 +41,7 @@ def test_send_reply_success(homedir, mocker, session, session_maker, reply_statu
     mock_encrypt = mocker.patch.object(gpg, "encrypt_to_source", return_value=encrypted_reply)
     msg = "wat"
 
-    mock_reply_response = sdk.Reply(uuid=msg_uuid, filename="5-dummy-reply")
+    mock_reply_response = sdk.Reply(uuid=msg_uuid, filename="5-dummy-reply.gpg")
     api_client.reply_source = mocker.MagicMock()
     api_client.reply_source.return_value = mock_reply_response
 
@@ -147,7 +147,7 @@ def test_drafts_ordering(homedir, mocker, session, session_maker, reply_status_c
     mock_encrypt = mocker.patch.object(gpg, "encrypt_to_source", return_value=encrypted_reply)
     msg = "wat"
 
-    mock_reply_response = sdk.Reply(uuid=msg_uuid, filename="2-dummy-reply")
+    mock_reply_response = sdk.Reply(uuid=msg_uuid, filename="2-dummy-reply.gpg")
     api_client.reply_source = mocker.MagicMock()
     api_client.reply_source.return_value = mock_reply_response
 

client/tests/sdk/test_api.py

@@ -11,7 +11,7 @@
 from test_shared import TestShared
 from utils import VCRAPI
 
-from securedrop_client.sdk import API, RequestTimeoutError, ServerConnectionError
+from securedrop_client.sdk import API, RequestTimeoutError, ServerConnectionError, StreamedResponse
 from securedrop_client.sdk.sdlocalobjects import (
     AuthError,
     BaseError,
@@ -393,6 +393,32 @@ def test_download_reply_timeout(mocker):
         api.download_reply(r, tmpdir)
 
 
+def test_stream_download_path_traversal(mocker, tmp_path):
+    api = API("mock", "mock", "mock", "mock", proxy=False)
+    mocker.patch(
+        "securedrop_client.sdk.API._send_json_request",
+        return_value=StreamedResponse(
+            contents=b"12345",
+            sha256sum="12345",
+        ),
+    )
+    r = Reply(uuid="12345", filename="../.config/autostart/pwned")
+    with pytest.raises(ValueError):
+        api.download_reply(r, tmp_path)
+    s = Submission(
+        uuid="12345",
+        filename="../.config/autostart/pwned",
+        download_url="http://mock",
+        is_read=False,
+        size=1000,
+        source_url="http://mock",
+        submission_url="http://mock",
+        seen_by=False,
+    )
+    with pytest.raises(ValueError):
+        api.download_submission(s, tmp_path)
+
+
 def test_download_submission_timeout(mocker):
     api = API("mock", "mock", "mock", "mock", proxy=False)
     mocker.patch("securedrop_client.sdk.API._send_json_request", side_effect=RequestTimeoutError)

debian/changelog

@@ -1,4 +1,10 @@
-securedrop-client (106.14.0) unstable; urgency=medium
+securedrop-client (106.14.1) unstable; urgency=medium
+
+  * see changelog.md
+
+ -- SecureDrop Team <[email protected]>  Thu, 30 Jan 2025 13:17:30 -0500
+
+securedrop-client (0.14.0) unstable; urgency=medium
 
   * see changelog.md
 

export/securedrop_export/__init__.py

@@ -1 +1 @@
-__version__ = "106.14.0"
+__version__ = "0.14.1"

log/log_client_files/sd-rsyslog

@@ -81,36 +81,13 @@ def onInit():
     config = configparser.ConfigParser()
     config.read('/etc/sd-rsyslog.conf')
     logvmname = config['sd-rsyslog']['remotevm']
-    localvmname = config['sd-rsyslog'].get('localvm', None)
-
-    # If no localvm name is specified, it must be supplied by Qubes OS. If this
-    # fails, we exit, to avoid falsely identified logs.
-    if localvmname is None:
-        try:
-            get_vm_name_process = Popen(["/usr/bin/qubesdb-read", "/name"],
-                                        stdout=PIPE, stderr=PIPE)
-            vm_name_output, vm_name_error = get_vm_name_process.communicate()
-            if vm_name_error != b"":
-                logging.exception("Error obtaining VM name via qubesdb-read:")
-                logging.exception(vm_name_error.decode("utf-8").strip())
-                sys.exit(1)
-            localvmname = vm_name_output.decode("utf-8").strip()
-        except FileNotFoundError:  # not on Qubes?
-            logging.exception("Could not run qubesdb-read command to obtain VM name.")
-            logging.exception("Note that sd-rsyslog must be run on Qubes OS if no "
-                              "localvm name is specified in the configuration.")
-            sys.exit(1)
 
     process = Popen(
             ["/usr/lib/qubes/qrexec-client-vm", logvmname, "securedrop.Log"],
             stdin=PIPE,
             stdout=PIPE,
             stderr=PIPE,
     )
-    process.stdin.write(localvmname.encode("utf-8"))
-    process.stdin.write(b"\n")
-    process.stdin.flush()
-
 
 def onMessage(msg):
     """Process one log message received from rsyslog (e.g. send it to a

log/log_server/log_saver.py

@@ -25,7 +25,7 @@ def main():
                 filepath = os.path.join(
                     os.getenv("HOME", "/"),
                     "QubesIncomingLogs",
-                    f"{vmname}",
+                    f"{vmname}",  # Restricted to r"\A[0-9_.-].*\Z" by qubes.vm.validate_name()
                     "syslog.log",
                 )
                 dirpath = os.path.dirname(filepath)

log/log_server/redis_log.py

@@ -1,6 +1,7 @@
 #!/opt/venvs/securedrop-log/bin/python3
 
 
+import os
 import sys
 
 import redis
@@ -27,7 +28,11 @@ def main():
 
     # the first line is always the remote vm name
     untrusted_line = stdin.readline()
-    qrexec_remote = untrusted_line.rstrip(b"\n").decode("utf-8")
+    qrexec_remote = os.getenv("QREXEC_REMOTE_DOMAIN")
+    if not qrexec_remote:
+        print("ERROR: QREXEC_REMOTE_DOMAIN not set", file=sys.stderr)
+        sys.exit(1)
+
     while True:
         untrusted_line = stdin.readline()
         if untrusted_line == b"":