diff --git a/CHANGELOG.md b/CHANGELOG.md index dea17edf..9f565830 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,15 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/). +## [Unreleased] + +**Security:** + +- Key files are now created with `0o600` permissions on POSIX and refuse to overwrite or + follow existing paths. `CBORSerializable.save()` now writes atomically with + `O_EXCL`/`O_CREAT`, closing a TOCTOU race and preventing world-readable key files. A new + `mode` keyword allows callers of non-secret objects to opt into wider permissions. + ## [0.8.1] - 2023-04-06 This patch contains a number of bug fixes to `v0.8.0`. diff --git a/docs/source/guides/address.rst b/docs/source/guides/address.rst index 33751da8..380fd367 100644 --- a/docs/source/guides/address.rst +++ b/docs/source/guides/address.rst @@ -50,6 +50,13 @@ A key could be saved to and loaded from a file:: >>> payment_signing_key = payment_signing_key.load("payment.skey") >>> payment_verification_key = payment_verification_key.load("payment.vkey") +.. note:: + On POSIX systems, key files are created with owner-only (``0o600``) permissions so that + other local users cannot read your secret keys. ``save()`` also refuses to overwrite or + follow an existing file. POSIX file modes are advisory on Windows, where this protection + is not enforced — Windows users should rely on NTFS ACLs or disk encryption to protect + key files. + Signing keys can sign messages (in bytes):: >>> message = b"Hello world!" diff --git a/pycardano/serialization.py b/pycardano/serialization.py index 141342b9..a9649a4a 100644 --- a/pycardano/serialization.py +++ b/pycardano/serialization.py @@ -631,27 +631,55 @@ def save( path: str, key_type: Optional[str] = None, description: Optional[str] = None, + mode: int = 0o600, **kwargs, ): """ Save the CBORSerializable object to a file in JSON format. This method writes the object's JSON representation to the specified file path. - It raises an error if the file already exists and is not empty. + The file is created atomically and refuses to overwrite or follow an existing + path. Because objects such as :class:`~pycardano.key.SigningKey` may contain + secret material, the file is created with owner-only (``0o600``) permissions by + default on POSIX systems. + + Note: + POSIX file modes are advisory on Windows, where ``os.chmod`` only toggles the + read-only bit. Owner-only enforcement is therefore POSIX-only; Windows users + should rely on NTFS ACLs or disk encryption to protect key files. Args: path (str): The file path to save the object to. key_type (str, optional): The type to use in the JSON output. Defaults to the class name. description (str, optional): The description to use in the JSON output. Defaults to the class docstring. + mode (int): The file permission bits to create the file with. Defaults to + ``0o600`` (owner read/write only), which is appropriate for files that may + hold secret key material. **kwargs: Extra key word arguments to be passed to `json.dumps()` Raises: - IOError: If the file already exists and is not empty. + IOError: If the file already exists. """ - if os.path.isfile(path) and os.stat(path).st_size > 0: + payload = self.to_json(key_type=key_type, description=description, **kwargs) + # O_EXCL makes creation atomic: it fails if the path already exists (closing the + # TOCTOU race in the old os.path.isfile check) and refuses to follow a pre-existing + # symlink. The mode is subject to the process umask, so chmod afterwards to + # guarantee the requested mode regardless of umask. + try: + fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, mode) + except FileExistsError: raise IOError(f"File {path} already exists!") - with open(path, "w") as f: - f.write(self.to_json(key_type=key_type, description=description, **kwargs)) + try: + with os.fdopen(fd, "w") as f: + os.chmod(path, mode) + f.write(payload) + except BaseException: + # The file was created by us; remove the partial/empty file on failure. + try: + os.unlink(path) + except OSError: + pass + raise @classmethod def load(cls, path: str): diff --git a/test/pycardano/test_address.py b/test/pycardano/test_address.py index ed520c10..06339417 100644 --- a/test/pycardano/test_address.py +++ b/test/pycardano/test_address.py @@ -1,6 +1,3 @@ -import os -import tempfile - import pytest from pycardano.address import Address, AddressType, PointerAddress @@ -214,18 +211,11 @@ def test_from_primitive_invalid_type_addr(): Address.from_primitive(value) -def test_save_load_address(): +def test_save_load_address(tmp_path): address_string = "addr_test1vr2p8st5t5cxqglyjky7vk98k7jtfhdpvhl4e97cezuhn0cqcexl7" address = Address.from_primitive(address_string) - # On Windows, NamedTemporaryFile keeps the file locked while open, so - # save() cannot open it for writing. Use delete=False and close the handle - # first, then clean up manually afterward. - with tempfile.NamedTemporaryFile(delete=False) as f: - tmp_path = f.name - try: - address.save(tmp_path) - loaded_address = Address.load(tmp_path) - assert address == loaded_address - finally: - os.unlink(tmp_path) + path = str(tmp_path / "address.txt") + address.save(path) + loaded_address = Address.load(path) + assert address == loaded_address diff --git a/test/pycardano/test_key.py b/test/pycardano/test_key.py index e4928541..46011261 100644 --- a/test/pycardano/test_key.py +++ b/test/pycardano/test_key.py @@ -1,7 +1,7 @@ import json import os import pathlib -import tempfile +import stat import pytest from mnemonic import Mnemonic @@ -204,43 +204,36 @@ def test_stake_pool_key_load(): assert vk == StakePoolVerificationKey.from_json(vk.to_json()) -def test_key_save(): - # On Windows, NamedTemporaryFile keeps the file locked while open. - # Use delete=False and close the handle first, then clean up manually. - with tempfile.NamedTemporaryFile(delete=False) as f: - tmp_path = f.name - try: - SK.save(tmp_path) - sk = PaymentSigningKey.load(tmp_path) - assert SK == sk - finally: - os.unlink(tmp_path) - - -def test_key_save_invalid_address(): - with tempfile.NamedTemporaryFile(delete=False) as f: - tmp_path = f.name - try: - SK.save(tmp_path) - with pytest.raises(IOError): - VK.save(tmp_path) - finally: - os.unlink(tmp_path) - - -def test_stake_pool_key_save(): - with tempfile.NamedTemporaryFile(delete=False) as skf: - sk_path = skf.name - with tempfile.NamedTemporaryFile(delete=False) as vkf: - vk_path = vkf.name - try: - SPSK.save(sk_path) - sk = StakePoolSigningKey.load(sk_path) - SPVK.save(vk_path) - vk = StakePoolSigningKey.load(vk_path) - finally: - os.unlink(sk_path) - os.unlink(vk_path) +def test_key_save(tmp_path): + path = str(tmp_path / "payment.skey") + SK.save(path) + sk = PaymentSigningKey.load(path) + assert SK == sk + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX file modes are advisory on Windows") +def test_key_save_mode_is_owner_only(tmp_path): + path = str(tmp_path / "payment.skey") + SK.save(path) + # The key file must be readable/writable by its owner only (0o600). + assert stat.S_IMODE(os.stat(path).st_mode) == 0o600 + + +def test_key_save_invalid_address(tmp_path): + path = str(tmp_path / "payment.skey") + SK.save(path) + # save() refuses to overwrite the existing file. + with pytest.raises(IOError): + VK.save(path) + + +def test_stake_pool_key_save(tmp_path): + sk_path = str(tmp_path / "cold.skey") + vk_path = str(tmp_path / "cold.vkey") + SPSK.save(sk_path) + sk = StakePoolSigningKey.load(sk_path) + SPVK.save(vk_path) + vk = StakePoolSigningKey.load(vk_path) assert SPSK == sk assert SPVK == vk diff --git a/test/pycardano/test_serialization.py b/test/pycardano/test_serialization.py index 9a8c0ed9..7b825266 100644 --- a/test/pycardano/test_serialization.py +++ b/test/pycardano/test_serialization.py @@ -1,6 +1,6 @@ import json import os -import tempfile +import stat from collections import defaultdict, deque from copy import deepcopy from dataclasses import dataclass, field @@ -1016,7 +1016,7 @@ class TestData(MapCBORSerializable): assert s_copy[0].value == 100 -def test_save_load(): +def test_save_load(tmp_path): @dataclass class Test1(CBORSerializable): a: str @@ -1048,17 +1048,13 @@ def to_shallow_primitive(self) -> Union[Primitive, CBORSerializable]: assert test1_json["description"] == "Test Description" assert test1_json["cborHex"] == test1.to_cbor_hex() - with tempfile.NamedTemporaryFile(delete=False) as f: - tmp_path = f.name - try: - test1.save(tmp_path) - loaded = Test1.load(tmp_path) - assert test1 == loaded + path = str(tmp_path / "test1.json") + test1.save(path) + loaded = Test1.load(path) + assert test1 == loaded - with pytest.raises(IOError): - test1.save(tmp_path) - finally: - os.unlink(tmp_path) + with pytest.raises(IOError): + test1.save(path) def test_ordered_set_as_key_in_dict(): @@ -1176,3 +1172,69 @@ def test_liqwid_tx(): cbor_hex = json.load(f).get("cborHex") tx = Transaction.load("test/resources/cbors/liqwid.json") assert tx.to_cbor().hex() == cbor_hex + + +@dataclass +class _SaveTest(CBORSerializable): + a: str + + @classmethod + def from_primitive( + cls: Type[CBORSerializable], value: Any, type_args: Optional[tuple] = None + ) -> CBORSerializable: + return _SaveTest(a=value["a"]) + + def to_shallow_primitive(self) -> Union[Primitive, CBORSerializable]: + return {"a": self.a} + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX file modes are advisory on Windows") +def test_save_default_mode_is_owner_only(tmp_path): + path = str(tmp_path / "k.skey") + _SaveTest(a="secret").save(path) + # Secret files default to owner read/write only (0o600). + assert stat.S_IMODE(os.stat(path).st_mode) == 0o600 + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX file modes are advisory on Windows") +def test_save_custom_mode(tmp_path): + path = str(tmp_path / "tx.json") + # Non-secret callers can opt into wider permissions via the mode keyword. + _SaveTest(a="public").save(path, mode=0o644) + assert stat.S_IMODE(os.stat(path).st_mode) == 0o644 + + +def test_save_refuses_existing_file(tmp_path): + obj = _SaveTest(a="x") + path = str(tmp_path / "k.json") + + # A non-empty existing file is rejected. + obj.save(path) + with pytest.raises(IOError): + obj.save(path) + os.unlink(path) + + # An empty pre-existing file is also rejected (O_EXCL tightening). + open(path, "w").close() + with pytest.raises(IOError): + obj.save(path) + + +@pytest.mark.skipif(os.name == "nt", reason="symlink semantics differ on Windows") +def test_save_does_not_follow_symlink(tmp_path): + target = tmp_path / "target" + link = tmp_path / "link" + target.touch() + link.symlink_to(target) + + with pytest.raises(IOError): + _SaveTest(a="x").save(str(link)) + # The pre-existing target must not have been written through the link. + assert target.stat().st_size == 0 + + +def test_save_round_trips(tmp_path): + obj = _SaveTest(a="round-trip") + path = str(tmp_path / "k.json") + obj.save(path) + assert _SaveTest.load(path) == obj diff --git a/test/pycardano/test_transaction.py b/test/pycardano/test_transaction.py index c2ffcf71..84f92135 100644 --- a/test/pycardano/test_transaction.py +++ b/test/pycardano/test_transaction.py @@ -1,5 +1,3 @@ -import os -import tempfile from dataclasses import dataclass from fractions import Fraction from test.pycardano.util import check_two_way_cbor @@ -244,7 +242,7 @@ def test_transaction(): assert expected_tx_id == signed_tx.id -def test_transaction_save_load(): +def test_transaction_save_load(tmp_path): tx_cbor = ( "84a70081825820b35a4ba9ef3ce21adcd6879d08553642224304704d206c74d3ffb3e6eed3ca28000d80018182581d60cc" "30497f4ff962f4c1dca54cceefe39f86f1d7179668009f8eb71e598200a1581cec8b7d1dd0b124e8333d3fa8d818f6eac0" @@ -264,14 +262,10 @@ def test_transaction_save_load(): ) tx = Transaction.from_cbor(tx_cbor) - with tempfile.NamedTemporaryFile(delete=False) as f: - tmp_path = f.name - try: - tx.save(tmp_path) - loaded_tx = Transaction.load(tmp_path) - assert tx == loaded_tx - finally: - os.unlink(tmp_path) + path = str(tmp_path / "tx.json") + tx.save(path) + loaded_tx = Transaction.load(path) + assert tx == loaded_tx def test_multi_asset(): diff --git a/test/pycardano/test_witness.py b/test/pycardano/test_witness.py index d2a3979c..7e9179f9 100644 --- a/test/pycardano/test_witness.py +++ b/test/pycardano/test_witness.py @@ -1,6 +1,4 @@ import json -import os -import tempfile from pycardano import ( PaymentSigningKey, @@ -12,7 +10,7 @@ ) -def test_witness_save_load(): +def test_witness_save_load(tmp_path): sk = PaymentSigningKey.generate() vk = PaymentVerificationKey.from_signing_key(sk) witness = VerificationKeyWitness( @@ -20,16 +18,12 @@ def test_witness_save_load(): signature=sk.sign(b"test"), ) - with tempfile.NamedTemporaryFile(delete=False) as f: - tmp_path = f.name - try: - witness.save(tmp_path) - loaded_witness = VerificationKeyWitness.load(tmp_path) - assert witness == loaded_witness + path = str(tmp_path / "witness.json") + witness.save(path) + loaded_witness = VerificationKeyWitness.load(path) + assert witness == loaded_witness - assert witness != vk - finally: - os.unlink(tmp_path) + assert witness != vk def test_redeemer_decode():