diff --git a/changelog.md b/changelog.md index f228db7d8..673174f86 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 (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) diff --git a/client/securedrop_client/__init__.py b/client/securedrop_client/__init__.py index 5b7fcbae7..1cf48bc15 100644 --- a/client/securedrop_client/__init__.py +++ b/client/securedrop_client/__init__.py @@ -1 +1 @@ -__version__ = "106.14.0" +__version__ = "106.14.1" diff --git a/client/securedrop_client/api_jobs/uploads.py b/client/securedrop_client/api_jobs/uploads.py index a74b0f00e..9d88ea8d4 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,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, ) diff --git a/client/securedrop_client/sdk/__init__.py b/client/securedrop_client/sdk/__init__.py index f1f925605..a6369bacc 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 ( @@ -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,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/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 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) diff --git a/debian/changelog b/debian/changelog index 13f12dc97..32972043d 100644 --- a/debian/changelog +++ b/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 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 5b7fcbae7..f075dd36a 100644 --- a/export/securedrop_export/__init__.py +++ b/export/securedrop_export/__init__.py @@ -1 +1 @@ -__version__ = "106.14.0" +__version__ = "0.14.1" 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/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) 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"":