From 35b0f1f4f98d95bf394f64137d0ad9f12161b42c Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Fri, 23 Jun 2017 05:56:41 +0200 Subject: [PATCH] Manifest: use limited unpacker (cherry picked from commit 6c2c51939d30eaab7fc9791f486b54f94de50659) --- borg/helpers.py | 10 ++++++++++ borg/key.py | 8 +++++--- borg/remote.py | 12 +++++++++++- borg/testsuite/key.py | 2 +- 4 files changed, 27 insertions(+), 5 deletions(-) diff --git a/borg/helpers.py b/borg/helpers.py index 26292eb4a..8bee0bf05 100644 --- a/borg/helpers.py +++ b/borg/helpers.py @@ -39,6 +39,12 @@ import msgpack.fallback import socket + +# to use a safe, limited unpacker, we need to set a upper limit to the archive count in the manifest. +# this does not mean that you can always really reach that number, because it also needs to be less than +# MAX_DATA_SIZE or it will trigger the check for that. +MAX_ARCHIVES = 400000 + # return codes returned by borg command # when borg is killed by signal N, rc = 128 + N EXIT_SUCCESS = 0 # everything done, no problems @@ -254,6 +260,10 @@ class Manifest: prev_ts = datetime.strptime(self.timestamp, "%Y-%m-%dT%H:%M:%S.%f") incremented = (prev_ts + timedelta(microseconds=1)).isoformat() self.timestamp = max(incremented, datetime.utcnow().isoformat()) + # include checks for limits as enforced by limited unpacker (used by load()) + assert len(self.archives) <= MAX_ARCHIVES + assert all(len(name) <= 255 for name in self.archives) + assert len(self.item_keys) <= 100 m = { 'version': 1, 'archives': StableDict((name, StableDict(archive)) for name, archive in self.archives.items()), diff --git a/borg/key.py b/borg/key.py index 3540ea58f..628056947 100644 --- a/borg/key.py +++ b/borg/key.py @@ -16,6 +16,8 @@ logger = create_logger() from .crypto import AES, bytes_to_long, long_to_bytes, bytes_to_int, num_aes_blocks from .crypto import hkdf_hmac_sha512 from .compress import Compressor, CNONE +from . import remote + PREFIX = b'\0' * 8 @@ -155,9 +157,9 @@ class KeyBase: logger.warning('Manifest authentication DISABLED.') tam_required = False data = bytearray(data) - # Since we don't trust these bytes we use the slower Python unpacker, - # which is assumed to have a lower probability of security issues. - unpacked = msgpack.fallback.unpackb(data, object_hook=StableDict, unicode_errors='surrogateescape') + unpacker = remote.get_limited_unpacker('manifest') + unpacker.feed(data) + unpacked = unpacker.unpack() if b'tam' not in unpacked: if tam_required: raise TAMRequiredError(self.repository._location.canonical_path()) diff --git a/borg/remote.py b/borg/remote.py index 246ae0fca..b4db293ad 100644 --- a/borg/remote.py +++ b/borg/remote.py @@ -15,6 +15,8 @@ from . import __version__ from .helpers import Error, IntegrityError, sysinfo from .helpers import replace_placeholders from .helpers import bin_to_hex +from .helpers import StableDict +from .helpers import MAX_ARCHIVES from .repository import Repository, LIST_SCAN_LIMIT, MAX_OBJECT_SIZE from .logger import create_logger @@ -64,8 +66,16 @@ def get_limited_unpacker(kind): args.update(dict(max_array_len=LIST_SCAN_LIMIT, # result list from repo.list() / .scan() max_map_len=100, # misc. result dicts )) + elif kind == 'manifest': + args.update(dict(use_list=True, # default value + max_array_len=100, # ITEM_KEYS ~= 22 + max_map_len=MAX_ARCHIVES, # list of archives + max_str_len=255, # archive name + object_hook=StableDict, + unicode_errors='surrogateescape', + )) else: - raise ValueError('kind must be "server" or "client"') + raise ValueError('kind must be "server", "client" or "manifest"') return msgpack.Unpacker(**args) diff --git a/borg/testsuite/key.py b/borg/testsuite/key.py index 2bb9d86fe..a25dd1c42 100644 --- a/borg/testsuite/key.py +++ b/borg/testsuite/key.py @@ -123,7 +123,7 @@ class TestTAM: key.unpack_and_verify_manifest(blob) blob = b'\xc1\xc1\xc1' - with pytest.raises(msgpack.UnpackException): + with pytest.raises((ValueError, msgpack.UnpackException)): key.unpack_and_verify_manifest(blob) def test_missing_when_required(self, key):