From 7d04e6ab8613010e98f807fa95826451d79b2d1d Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 18 Oct 2021 10:45:10 -0400 Subject: [PATCH 1/6] news fragment --- newsfragments/LFS-01-007.security | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 newsfragments/LFS-01-007.security diff --git a/newsfragments/LFS-01-007.security b/newsfragments/LFS-01-007.security new file mode 100644 index 000000000..75d9904a2 --- /dev/null +++ b/newsfragments/LFS-01-007.security @@ -0,0 +1,2 @@ +The storage protocol operation ``add_lease`` now safely rejects an attempt to add a 4,294,967,296th lease to an immutable share. +Previously this failed with an error after recording the new lease in the share file, resulting in the share file losing track of a one previous lease. From f60bbbd3e201b1b49598b7b2b6a05ad8db8a3dfd Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 18 Oct 2021 10:45:58 -0400 Subject: [PATCH 2/6] make it possible to test this behavior of `add_lease` --- src/allmydata/storage/immutable.py | 66 ++++++++++++++++++++++++++++-- src/allmydata/test/test_storage.py | 59 +++++++++++++++++++++++++- 2 files changed, 120 insertions(+), 5 deletions(-) diff --git a/src/allmydata/storage/immutable.py b/src/allmydata/storage/immutable.py index b8b18f140..acd09854f 100644 --- a/src/allmydata/storage/immutable.py +++ b/src/allmydata/storage/immutable.py @@ -53,13 +53,64 @@ from allmydata.storage.common import UnknownImmutableContainerVersionError # then the value stored in this field will be the actual share data length # modulo 2**32. +def _fix_lease_count_format(lease_count_format): + """ + Turn a single character struct format string into a format string suitable + for use in encoding and decoding the lease count value inside a share + file, if possible. + + :param str lease_count_format: A single character format string like + ``"B"`` or ``"L"``. + + :raise ValueError: If the given format string is not suitable for use + encoding and decoding a lease count. + + :return str: A complete format string which can safely be used to encode + and decode lease counts in a share file. + """ + if len(lease_count_format) != 1: + raise ValueError( + "Cannot construct ShareFile with lease_count_format={!r}; " + "format must accept a single value".format( + lease_count_format, + ), + ) + # Make it big-endian with standard size so all platforms agree on the + # result. + fixed = ">" + lease_count_format + if struct.calcsize(fixed) > 4: + # There is only room for at most 4 bytes in the share file format so + # we can't allow any larger formats. + raise ValueError( + "Cannot construct ShareFile with lease_count_format={!r}; " + "size must be smaller than size of '>L'".format( + lease_count_format, + ), + ) + return fixed + + class ShareFile(object): + """ + Support interaction with persistent storage of a share. + + :ivar str _lease_count_format: The format string which is used to encode + and decode the lease count inside the share file. As stated in the + comment in this module there is room for at most 4 bytes in this part + of the file. A format string that works on fewer bytes is allowed to + restrict the number of leases allowed in the share file to a smaller + number than could be supported by using the full 4 bytes. This is + mostly of interest for testing. + """ LEASE_SIZE = struct.calcsize(">L32s32sL") sharetype = "immutable" - def __init__(self, filename, max_size=None, create=False): + def __init__(self, filename, max_size=None, create=False, lease_count_format="L"): """ If max_size is not None then I won't allow more than max_size to be written to me. If create=True and max_size must not be None. """ precondition((max_size is not None) or (not create), max_size, create) + + self._lease_count_format = _fix_lease_count_format(lease_count_format) + self._lease_count_size = struct.calcsize(self._lease_count_format) self.home = filename self._max_size = max_size if create: @@ -126,12 +177,21 @@ class ShareFile(object): def _read_num_leases(self, f): f.seek(0x08) - (num_leases,) = struct.unpack(">L", f.read(4)) + (num_leases,) = struct.unpack( + self._lease_count_format, + f.read(self._lease_count_size), + ) return num_leases def _write_num_leases(self, f, num_leases): + self._write_encoded_num_leases( + f, + struct.pack(self._lease_count_format, num_leases), + ) + + def _write_encoded_num_leases(self, f, encoded_num_leases): f.seek(0x08) - f.write(struct.pack(">L", num_leases)) + f.write(encoded_num_leases) def _truncate_leases(self, f, num_leases): f.truncate(self._lease_offset + num_leases * self.LEASE_SIZE) diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index d18960a1e..0a37dffc2 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -19,6 +19,7 @@ import platform import stat import struct import shutil +from functools import partial from uuid import uuid4 from twisted.trial import unittest @@ -3009,8 +3010,8 @@ class Stats(unittest.TestCase): class ShareFileTests(unittest.TestCase): """Tests for allmydata.storage.immutable.ShareFile.""" - def get_sharefile(self): - sf = ShareFile(self.mktemp(), max_size=1000, create=True) + def get_sharefile(self, **kwargs): + sf = ShareFile(self.mktemp(), max_size=1000, create=True, **kwargs) sf.write_share_data(0, b"abc") sf.write_share_data(2, b"DEF") # Should be b'abDEF' now. @@ -3039,3 +3040,57 @@ class ShareFileTests(unittest.TestCase): sf = self.get_sharefile() with self.assertRaises(IndexError): sf.cancel_lease(b"garbage") + + def test_long_lease_count_format(self): + """ + ``ShareFile.__init__`` raises ``ValueError`` if the lease count format + given is longer than one character. + """ + with self.assertRaises(ValueError): + self.get_sharefile(lease_count_format="BB") + + def test_large_lease_count_format(self): + """ + ``ShareFile.__init__`` raises ``ValueError`` if the lease count format + encodes to a size larger than 8 bytes. + """ + with self.assertRaises(ValueError): + self.get_sharefile(lease_count_format="Q") + + def test_avoid_lease_overflow(self): + """ + If the share file already has the maximum number of leases supported then + ``ShareFile.add_lease`` raises ``struct.error`` and makes no changes + to the share file contents. + """ + make_lease = partial( + LeaseInfo, + renew_secret=b"r" * 32, + cancel_secret=b"c" * 32, + expiration_time=2 ** 31, + ) + # Make it a little easier to reach the condition by limiting the + # number of leases to only 255. + sf = self.get_sharefile(lease_count_format="B") + + # Add the leases. + for i in range(2 ** 8 - 1): + lease = make_lease(owner_num=i) + sf.add_lease(lease) + + # Capture the state of the share file at this point so we can + # determine whether the next operation modifies it or not. + with open(sf.home, "rb") as f: + before_data = f.read() + + # It is not possible to add a 256th lease. + lease = make_lease(owner_num=256) + with self.assertRaises(struct.error): + sf.add_lease(lease) + + # Compare the share file state to what we captured earlier. Any + # change is a bug. + with open(sf.home, "rb") as f: + after_data = f.read() + + self.assertEqual(before_data, after_data) From df64bbb1e443cbbad272067e7716b4d9a3f3408d Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 18 Oct 2021 10:50:28 -0400 Subject: [PATCH 3/6] fail to encode the lease count *before* changing anything This preserves the failure behavior - `struct.error` is raised - but leaves the actual share file contents untouched if the new lease count cannot be encoded. There are still two separate write operations so it is conceivable that some other problem could cause `write_lease_record` to happen but `write_encoded_num_leases` not to happen. As far as I can tell we have severely limited options for addressing that problem in general as long as share files are backed by a POSIX filesystem. However, by removing the failure mode that depends on user input, it may be that this is all that is needed to close the *security* hole. --- src/allmydata/storage/immutable.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/allmydata/storage/immutable.py b/src/allmydata/storage/immutable.py index acd09854f..887ccc931 100644 --- a/src/allmydata/storage/immutable.py +++ b/src/allmydata/storage/immutable.py @@ -209,8 +209,11 @@ class ShareFile(object): def add_lease(self, lease_info): with open(self.home, 'rb+') as f: num_leases = self._read_num_leases(f) + # Before we write the new lease record, make sure we can encode + # the new lease count. + new_lease_count = struct.pack(self._lease_count_format, num_leases + 1) self._write_lease_record(f, num_leases, lease_info) - self._write_num_leases(f, num_leases+1) + self._write_encoded_num_leases(f, new_lease_count) def renew_lease(self, renew_secret, new_expire_time): for i,lease in enumerate(self.get_leases()): From d8c466e9a7ba5f121cb6d9f891569db7e01e87b6 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 22 Oct 2021 12:35:11 -0400 Subject: [PATCH 4/6] try to explain `lease_count_format` more clearly --- src/allmydata/storage/immutable.py | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/allmydata/storage/immutable.py b/src/allmydata/storage/immutable.py index 887ccc931..e23abb080 100644 --- a/src/allmydata/storage/immutable.py +++ b/src/allmydata/storage/immutable.py @@ -106,7 +106,30 @@ class ShareFile(object): sharetype = "immutable" def __init__(self, filename, max_size=None, create=False, lease_count_format="L"): - """ If max_size is not None then I won't allow more than max_size to be written to me. If create=True and max_size must not be None. """ + """ + Initialize a ``ShareFile``. + + :param Optional[int] max_size: If given, the maximum number of bytes + that this ``ShareFile`` will accept to be stored. ``write`` will + accept in total. + + :param bool create: If ``True``, create the file (and fail if it + exists already). ``max_size`` must not be ``None`` in this case. + If ``False``, open an existing file for reading. + + :param str lease_count_format: A format character to use to encode and + decode the number of leases in the share file. There are only 4 + bytes available in the file so the format must be 4 bytes or + smaller. If different formats are used at different times with + the same share file, the result will likely be nonsense. + + This parameter is intended for the test suite to use to be able to + exercise values near the maximum encodeable value without having + to create billions of leases. + + :raise ValueError: If the encoding of ``lease_count_format`` is too + large or if it is not a single format character. + """ precondition((max_size is not None) or (not create), max_size, create) self._lease_count_format = _fix_lease_count_format(lease_count_format) From bcdfb8155c28c94e75b8e7acc7344dc1f01aa798 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 22 Oct 2021 12:53:17 -0400 Subject: [PATCH 5/6] give the news fragment its proper name --- newsfragments/{LFS-01-007.security => 3821.security} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename newsfragments/{LFS-01-007.security => 3821.security} (100%) diff --git a/newsfragments/LFS-01-007.security b/newsfragments/3821.security similarity index 100% rename from newsfragments/LFS-01-007.security rename to newsfragments/3821.security From ce30f9dd0663ba22a985571f8029ad35026bb91e Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 22 Oct 2021 15:04:45 -0400 Subject: [PATCH 6/6] clean up copyediting errors --- src/allmydata/storage/immutable.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/allmydata/storage/immutable.py b/src/allmydata/storage/immutable.py index e23abb080..55bcdda64 100644 --- a/src/allmydata/storage/immutable.py +++ b/src/allmydata/storage/immutable.py @@ -110,8 +110,7 @@ class ShareFile(object): Initialize a ``ShareFile``. :param Optional[int] max_size: If given, the maximum number of bytes - that this ``ShareFile`` will accept to be stored. ``write`` will - accept in total. + that this ``ShareFile`` will accept to be stored. :param bool create: If ``True``, create the file (and fail if it exists already). ``max_size`` must not be ``None`` in this case.