From f6cc7790a991ff0fc588b890027ac58848f36e27 Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Fri, 3 Jan 2025 14:26:00 -0500 Subject: [PATCH 01/10] Update year in messages.pot (cherry picked from commit d1183debd314fce66ebd2df284d01931bba9c911) --- client/securedrop_client/locale/messages.pot | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/securedrop_client/locale/messages.pot b/client/securedrop_client/locale/messages.pot index 570308c35..6a8980fcc 100644 --- a/client/securedrop_client/locale/messages.pot +++ b/client/securedrop_client/locale/messages.pot @@ -1,7 +1,7 @@ # Translations template for SecureDrop Client. -# Copyright (C) 2024 Freedom of the Press Foundation +# Copyright (C) 2025 Freedom of the Press Foundation # This file is distributed under the same license as the SecureDrop Client project. -# FIRST AUTHOR , 2024. +# FIRST AUTHOR , 2025. # #, fuzzy msgid "" From 120bac14649db0bcf5f24f2eb82731c76843b1ba Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Tue, 28 Jan 2025 16:03:34 -0500 Subject: [PATCH 02/10] Prevent path traversal attacks in SDK The SDK joins paths in two cases: == download_reply == A malicious server could override the filename given in the content-disposition header to trigger an arbitrary file write into e.g. ~/.config/autostart. The fix is to do what download_submission does, and use the pre-sanitized filename. We shouldn't even use the content-disposition header at all, so it's removed from StreamedResponse. == download_submission == In this case no path traversal was possible, since we correctly use submission.filename, which is already validated when it is first fetched from the server. However, we can apply an extra level of hardening by checking for path traversal before we write anything. Fixes . (cherry picked from commit 45d86bc4cdcbe6e625b27ac0c15d24f42fbd6c47) --- client/securedrop_client/sdk/__init__.py | 46 +++++++++++++----------- client/tests/sdk/test_api.py | 28 ++++++++++++++- 2 files changed, 53 insertions(+), 21 deletions(-) diff --git a/client/securedrop_client/sdk/__init__.py b/client/securedrop_client/sdk/__init__.py index f1f925605..234bf8f74 100644 --- a/client/securedrop_client/sdk/__init__.py +++ b/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 ( @@ -61,7 +62,6 @@ class StreamedResponse: 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,7 +688,7 @@ 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 @@ -699,7 +696,7 @@ def download_submission( 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,14 +919,14 @@ 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. """ @@ -932,8 +934,10 @@ def download_reply(self, reply: Reply, path: str | None = None) -> tuple[str, st 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: """ diff --git a/client/tests/sdk/test_api.py b/client/tests/sdk/test_api.py index 2c1e24223..366faa78d 100644 --- a/client/tests/sdk/test_api.py +++ b/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) From 3012bf7289389b5ec0f5f4db0f009a17dee1f586 Mon Sep 17 00:00:00 2001 From: deeplow Date: Mon, 27 Jan 2025 19:25:36 +0000 Subject: [PATCH 03/10] sd-log: don't send source vm name Prevent directory traversal attacks by no longer relying on compromized-vm-provided name. This parameter should instead be obtained by the environment variable $QREXEC_REMOTE_DOMAIN. (cherry picked from commit 9f1580cdde24365084aafffedf367c32d867df8b) --- log/log_client_files/sd-rsyslog | 23 ----------------------- log/log_server/redis_log.py | 7 ++++++- 2 files changed, 6 insertions(+), 24 deletions(-) diff --git a/log/log_client_files/sd-rsyslog b/log/log_client_files/sd-rsyslog index 545f7a9e9..ab7135bdc 100644 --- a/log/log_client_files/sd-rsyslog +++ b/log/log_client_files/sd-rsyslog @@ -81,25 +81,6 @@ 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"], @@ -107,10 +88,6 @@ def onInit(): 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 diff --git a/log/log_server/redis_log.py b/log/log_server/redis_log.py index c8e5025b0..70dbd8f4d 100755 --- a/log/log_server/redis_log.py +++ b/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"": From a4d28c0b61ac77de38194b4d96720b56d715e530 Mon Sep 17 00:00:00 2001 From: deeplow Date: Wed, 29 Jan 2025 16:56:32 +0000 Subject: [PATCH 04/10] Add comment about path traversal mitigation Validation is no longer necessary, since QREXEC_REMOTE_DOMAIN is provided by dom0 (as opposed to an attacker) and the VM name already includes validations which prevent "/" in the name. (cherry picked from commit 48382568798070c484cbcb82637f8bb42300e976) --- log/log_server/log_saver.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/log/log_server/log_saver.py b/log/log_server/log_saver.py index 2199b5058..7f7ac708a 100755 --- a/log/log_server/log_saver.py +++ b/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) From 7ab0ad8655f0d098738416e3c0e84a0959f70a51 Mon Sep 17 00:00:00 2001 From: Cory Francis Myers Date: Wed, 29 Jan 2025 18:04:44 -0800 Subject: [PATCH 05/10] docs(StreamedResponse): correct per 45d86bc (cherry picked from commit ec5ca97e45e911e3ff867c01fedc94c16bd487f3) --- client/securedrop_client/sdk/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/securedrop_client/sdk/__init__.py b/client/securedrop_client/sdk/__init__.py index 234bf8f74..a6369bacc 100644 --- a/client/securedrop_client/sdk/__init__.py +++ b/client/securedrop_client/sdk/__init__.py @@ -58,7 +58,7 @@ 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 From 0d521f5ce78f662f18da73849c18ab03de1cd1c9 Mon Sep 17 00:00:00 2001 From: Cory Francis Myers Date: Wed, 29 Jan 2025 18:26:20 -0800 Subject: [PATCH 06/10] fix(SendReplyJob): validate server-provided filename This is a compromise: We have to at least consult the server-provided filename no matter what, because that's how we reconcile our local ordering with the server's authoritative ordering within the source conversation. At that point, we might as well just use it, subject to the same validation enforced for metadata syncs in storage.sanitize_submissions_or_replies(). (cherry picked from commit 6014eece28f04092d7bcf32ebd620e9f394f07f9) --- client/securedrop_client/api_jobs/uploads.py | 17 ++++++++--------- client/tests/api_jobs/test_uploads.py | 4 ++-- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/client/securedrop_client/api_jobs/uploads.py b/client/securedrop_client/api_jobs/uploads.py index a74b0f00e..a41f27e3c 100644 --- a/client/securedrop_client/api_jobs/uploads.py +++ b/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,22 +76,21 @@ 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 diff --git a/client/tests/api_jobs/test_uploads.py b/client/tests/api_jobs/test_uploads.py index 49a712fbe..a2df7152f 100644 --- a/client/tests/api_jobs/test_uploads.py +++ b/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 From e1c6e75320560486afff4d8477976b36bc9dd53e Mon Sep 17 00:00:00 2001 From: Cory Francis Myers Date: Wed, 29 Jan 2025 18:26:41 -0800 Subject: [PATCH 07/10] refactor(SendReplyJob): clarify references to draft/final Reply objects (cherry picked from commit 884ee7fe6a1c6d94bc203576ce68bdd434d2aafa) --- client/securedrop_client/api_jobs/uploads.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/client/securedrop_client/api_jobs/uploads.py b/client/securedrop_client/api_jobs/uploads.py index a41f27e3c..9d88ea8d4 100644 --- a/client/securedrop_client/api_jobs/uploads.py +++ b/client/securedrop_client/api_jobs/uploads.py @@ -93,15 +93,12 @@ def call_api(self, api_client: API, session: Session) -> str: ) # 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, ) From b3ced68d4749fff90181bd83e1678355048e5282 Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Thu, 30 Jan 2025 13:18:11 -0500 Subject: [PATCH 08/10] SecureDrop Client 0.14.1 --- changelog.md | 11 +++++++++++ client/securedrop_client/__init__.py | 2 +- debian/changelog | 6 ++++++ export/securedrop_export/__init__.py | 2 +- 4 files changed, 19 insertions(+), 2 deletions(-) diff --git a/changelog.md b/changelog.md index f228db7d8..67dde39a9 100644 --- a/changelog.md +++ b/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 + * Don't send source VM name to sd-log + +* Miscellaneous: + * Update year in messages.pot + ## 0.14.0 * Add support for selecting and deleting multiple sources (#2208, #2188, #2230, #2252, #2293, #2299, #2300) diff --git a/client/securedrop_client/__init__.py b/client/securedrop_client/__init__.py index 9e78220f9..f075dd36a 100644 --- a/client/securedrop_client/__init__.py +++ b/client/securedrop_client/__init__.py @@ -1 +1 @@ -__version__ = "0.14.0" +__version__ = "0.14.1" diff --git a/debian/changelog b/debian/changelog index 8a9a164c0..d60331734 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,9 @@ +securedrop-client (0.14.1) unstable; urgency=medium + + * see changelog.md + + -- SecureDrop Team Thu, 30 Jan 2025 13:17:30 -0500 + securedrop-client (0.14.0) unstable; urgency=medium * see changelog.md diff --git a/export/securedrop_export/__init__.py b/export/securedrop_export/__init__.py index 9e78220f9..f075dd36a 100644 --- a/export/securedrop_export/__init__.py +++ b/export/securedrop_export/__init__.py @@ -1 +1 @@ -__version__ = "0.14.0" +__version__ = "0.14.1" From 564be9e348487a77f128c4c936aed6cbec3faab6 Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Thu, 30 Jan 2025 15:59:20 -0500 Subject: [PATCH 09/10] Document CVEs in changelog --- changelog.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/changelog.md b/changelog.md index 67dde39a9..673174f86 100644 --- a/changelog.md +++ b/changelog.md @@ -5,8 +5,8 @@ This release contains fixes for two security issues; please see our advisory for more details. * Security fixes: - * Prevent path manipulation/traversal attacks in SDK - * Don't send source VM name to sd-log + * 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 From ed3ab29dc425ddef433db04e698a53e0d7227b9c Mon Sep 17 00:00:00 2001 From: philmcmahon Date: Thu, 20 Feb 2025 14:43:17 +0000 Subject: [PATCH 10/10] fix changelog formatting --- debian/changelog | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/debian/changelog b/debian/changelog index 24af26c05..32972043d 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,4 +1,4 @@ -debian/changelog securedrop-client (106.14.1) unstable; urgency=medium +securedrop-client (106.14.1) unstable; urgency=medium * see changelog.md