From b05893e723905a3413faf5a190f6db8d4a9ee8f3 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Fri, 17 Feb 2017 05:00:37 +0100 Subject: [PATCH 1/2] borg rpc: use limited msgpack.Unpacker, fixes #2139 we do not trust the remote, so we are careful unpacking its responses. the remote could return manipulated msgpack data that announces e.g. a huge array or map or string. the local would then need to allocate huge amounts of RAM in expectation of that data (no matter whether really that much is coming or not). by using limits in the Unpacker, a ValueError will be raised if unexpected amounts of data shall get unpacked. memory DoS will be avoided. --- src/borg/archive.py | 4 ++-- src/borg/archiver.py | 4 ++-- src/borg/remote.py | 28 ++++++++++++++++++++++++---- src/borg/repository.py | 2 ++ 4 files changed, 30 insertions(+), 8 deletions(-) diff --git a/src/borg/archive.py b/src/borg/archive.py index f499f9233..c18829ed4 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -39,7 +39,7 @@ from .item import Item, ArchiveItem from .key import key_factory from .platform import acl_get, acl_set, set_flags, get_flags, swidth from .remote import cache_if_remote -from .repository import Repository +from .repository import Repository, LIST_SCAN_LIMIT has_lchmod = hasattr(os, 'lchmod') @@ -1060,7 +1060,7 @@ class ArchiveChecker: self.chunks = ChunkIndex(capacity) marker = None while True: - result = self.repository.list(limit=10000, marker=marker) + result = self.repository.list(limit=LIST_SCAN_LIMIT, marker=marker) if not result: break marker = result[-1] diff --git a/src/borg/archiver.py b/src/borg/archiver.py index 9dbbd2029..371576801 100644 --- a/src/borg/archiver.py +++ b/src/borg/archiver.py @@ -57,7 +57,7 @@ from .key import key_creator, tam_required_file, tam_required, RepoKey, Passphra from .keymanager import KeyManager from .platform import get_flags, umount, get_process_id from .remote import RepositoryServer, RemoteRepository, cache_if_remote -from .repository import Repository +from .repository import Repository, LIST_SCAN_LIMIT from .selftest import selftest from .upgrader import AtticRepositoryUpgrader, BorgRepositoryUpgrader @@ -1305,7 +1305,7 @@ class Archiver: marker = None i = 0 while True: - result = repository.list(limit=10000, marker=marker) + result = repository.list(limit=LIST_SCAN_LIMIT, marker=marker) if not result: break marker = result[-1] diff --git a/src/borg/remote.py b/src/borg/remote.py index 2567e0ea1..b13382129 100644 --- a/src/borg/remote.py +++ b/src/borg/remote.py @@ -23,7 +23,7 @@ from .helpers import sysinfo from .helpers import bin_to_hex from .helpers import replace_placeholders from .helpers import yes -from .repository import Repository +from .repository import Repository, MAX_OBJECT_SIZE, LIST_SCAN_LIMIT from .version import parse_version, format_version from .logger import create_logger @@ -57,6 +57,27 @@ def os_write(fd, data): return amount +def get_limited_unpacker(kind): + """return a limited Unpacker because we should not trust msgpack data received from remote""" + args = dict(use_list=False, # return tuples, not lists + max_bin_len=0, # not used + max_ext_len=0, # not used + max_buffer_size=3 * max(BUFSIZE, MAX_OBJECT_SIZE), + max_str_len=MAX_OBJECT_SIZE, # a chunk or other repo object + ) + if kind == 'server': + args.update(dict(max_array_len=100, # misc. cmd tuples + max_map_len=100, # misc. cmd dicts + )) + elif kind == 'client': + args.update(dict(max_array_len=LIST_SCAN_LIMIT, # result list from repo.list() / .scan() + max_map_len=100, # misc. result dicts + )) + else: + raise ValueError('kind must be "server" or "client"') + return msgpack.Unpacker(**args) + + class ConnectionClosed(Error): """Connection closed by remote host""" @@ -185,7 +206,7 @@ class RepositoryServer: # pragma: no cover # Make stderr blocking fl = fcntl.fcntl(stderr_fd, fcntl.F_GETFL) fcntl.fcntl(stderr_fd, fcntl.F_SETFL, fl & ~os.O_NONBLOCK) - unpacker = msgpack.Unpacker(use_list=False) + unpacker = get_limited_unpacker('server') while True: r, w, es = select.select([stdin_fd], [], [], 10) if r: @@ -487,8 +508,7 @@ class RemoteRepository: self.ignore_responses = set() self.responses = {} self.ratelimit = SleepingBandwidthLimiter(args.remote_ratelimit * 1024 if args and args.remote_ratelimit else 0) - - self.unpacker = msgpack.Unpacker(use_list=False) + self.unpacker = get_limited_unpacker('client') self.server_version = parse_version('1.0.8') # fallback version if server is too old to send version information self.p = None testing = location.host == '__testsuite__' diff --git a/src/borg/repository.py b/src/borg/repository.py index 4183473e2..235873559 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -33,6 +33,8 @@ TAG_PUT = 0 TAG_DELETE = 1 TAG_COMMIT = 2 +LIST_SCAN_LIMIT = 10000 # repo.list() / .scan() result count limit the borg client uses + FreeSpace = partial(defaultdict, int) From 6a25b6bdfac5d55f86be5cc64ec951a0cd2851a8 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Sat, 18 Feb 2017 07:15:53 +0100 Subject: [PATCH 2/2] update docs about limited msgpack Unpacker for RPC code --- docs/security.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/security.rst b/docs/security.rst index f84b321cb..5688744ec 100644 --- a/docs/security.rst +++ b/docs/security.rst @@ -248,8 +248,8 @@ denial of repository service. The situation were a server can create a general DoS on the client should be avoided, but might be possible by e.g. forcing the client to allocate large amounts of memory to decode large messages (or messages -that merely indicate a large amount of data follows). See issue -:issue:`2139` for details. +that merely indicate a large amount of data follows). The RPC protocol +code uses a limited msgpack Unpacker to prohibit this. We believe that other kinds of attacks, especially critical vulnerabilities like remote code execution are inhibited by the design of the protocol: