From c23e1e28c63be1efcd7b21ebf08c56a2aff27f2b Mon Sep 17 00:00:00 2001 From: Marian Beermann Date: Wed, 10 May 2017 18:34:22 +0200 Subject: [PATCH] use assert_secure for all commands that use the manifest This already excludes the debug commands that we wouldn't want this on. --- src/borg/archiver.py | 8 +++-- src/borg/cache.py | 63 ++++++++++++++++++++++++++-------- src/borg/testsuite/archiver.py | 2 +- 3 files changed, 55 insertions(+), 18 deletions(-) diff --git a/src/borg/archiver.py b/src/borg/archiver.py index 0ddfe1fe5..601c9848f 100644 --- a/src/borg/archiver.py +++ b/src/borg/archiver.py @@ -36,7 +36,7 @@ from .algorithms.crc32 import crc32 from .archive import Archive, ArchiveChecker, ArchiveRecreater, Statistics, is_special from .archive import BackupOSError, backup_io -from .cache import Cache +from .cache import Cache, assert_secure from .constants import * # NOQA from .compress import CompressionSpec from .crypto.key import key_creator, tam_required_file, tam_required, RepoKey, PassphraseKey @@ -86,7 +86,8 @@ def argument(args, str_or_bool): return str_or_bool -def with_repository(fake=False, invert_fake=False, create=False, lock=True, exclusive=False, manifest=True, cache=False): +def with_repository(fake=False, invert_fake=False, create=False, lock=True, exclusive=False, manifest=True, cache=False, + secure=True): """ Method decorator for subcommand-handling methods: do_XYZ(self, args, repository, …) @@ -97,6 +98,7 @@ def with_repository(fake=False, invert_fake=False, create=False, lock=True, excl :param exclusive: (str or bool) lock repository exclusively (for writing) :param manifest: load manifest and key, pass them as keyword arguments :param cache: open cache, pass it as keyword argument (implies manifest) + :param secure: do assert_secure after loading manifest """ def decorator(method): @functools.wraps(method) @@ -117,6 +119,8 @@ def wrapper(self, args, **kwargs): kwargs['manifest'], kwargs['key'] = Manifest.load(repository) if 'compression' in args: kwargs['key'].compressor = args.compression.compressor + if secure: + assert_secure(repository, kwargs['manifest']) if cache: with Cache(repository, kwargs['key'], kwargs['manifest'], do_files=getattr(args, 'cache_files', False), diff --git a/src/borg/cache.py b/src/borg/cache.py index ecca1ab74..882d98631 100644 --- a/src/borg/cache.py +++ b/src/borg/cache.py @@ -32,6 +32,25 @@ class SecurityManager: + """ + Tracks repositories. Ensures that nothing bad happens (repository swaps, + replay attacks, unknown repositories etc.). + + This is complicated by the Cache being initially used for this, while + only some commands actually use the Cache, which meant that other commands + did not perform these checks. + + Further complications were created by the Cache being a cache, so it + could be legitimately deleted, which is annoying because Borg didn't + recognize repositories after that. + + Therefore a second location, the security database (see get_security_dir), + was introduced which stores this information. However, this means that + the code has to deal with a cache existing but no security DB entry, + or inconsistencies between the security DB and the cache which have to + be reconciled, and also with no cache existing but a security DB entry. + """ + def __init__(self, repository): self.repository = repository self.dir = get_security_dir(repository.id_str) @@ -66,19 +85,19 @@ def save(self, manifest, key): with open(self.manifest_ts_file, 'w') as fd: fd.write(manifest.timestamp) - def assert_location_matches(self, cache_config): + def assert_location_matches(self, cache_config=None): # Warn user before sending data to a relocated repository try: with open(self.location_file) as fd: previous_location = fd.read() - logger.debug('security: read previous_location %r', previous_location) + logger.debug('security: read previous location %r', previous_location) except FileNotFoundError: - logger.debug('security: previous_location file %s not found', self.location_file) + logger.debug('security: previous location file %s not found', self.location_file) previous_location = None except OSError as exc: logger.warning('Could not read previous location file: %s', exc) previous_location = None - if cache_config.previous_location and previous_location != cache_config.previous_location: + if cache_config and cache_config.previous_location and previous_location != cache_config.previous_location: # Reconcile cache and security dir; we take the cache location. previous_location = cache_config.previous_location logger.debug('security: using previous_location of cache: %r', previous_location) @@ -95,9 +114,10 @@ def assert_location_matches(self, cache_config): logger.debug('security: updating location stored in cache and security dir') with open(self.location_file, 'w') as fd: fd.write(repository_location) - cache_config.save() + if cache_config: + cache_config.save() - def assert_no_manifest_replay(self, manifest, key, cache_config): + def assert_no_manifest_replay(self, manifest, key, cache_config=None): try: with open(self.manifest_ts_file) as fd: timestamp = fd.read() @@ -108,7 +128,8 @@ def assert_no_manifest_replay(self, manifest, key, cache_config): except OSError as exc: logger.warning('Could not read previous location file: %s', exc) timestamp = '' - timestamp = max(timestamp, cache_config.timestamp or '') + if cache_config: + timestamp = max(timestamp, cache_config.timestamp or '') logger.debug('security: determined newest manifest timestamp as %s', timestamp) # If repository is older than the cache or security dir something fishy is going on if timestamp and timestamp > manifest.timestamp: @@ -117,30 +138,39 @@ def assert_no_manifest_replay(self, manifest, key, cache_config): else: raise Cache.RepositoryReplay() - def assert_key_type(self, key, cache): + def assert_key_type(self, key, cache_config=None): # Make sure an encrypted repository has not been swapped for an unencrypted repository - if cache.key_type is not None and cache.key_type != str(key.TYPE): + if cache_config and cache_config.key_type is not None and cache_config.key_type != str(key.TYPE): raise Cache.EncryptionMethodMismatch() if self.known() and not self.key_matches(key): raise Cache.EncryptionMethodMismatch() def assert_secure(self, manifest, key, *, cache_config=None, warn_if_unencrypted=True): + # warn_if_unencrypted=False is only used for initializing a new repository. + # Thus, avoiding asking about a repository that's currently initializing. self.assert_access_unknown(warn_if_unencrypted, manifest, key) if cache_config: self._assert_secure(manifest, key, cache_config) else: - with CacheConfig(self.repository): - self._assert_secure(manifest, key, cache_config) + cache_config = CacheConfig(self.repository) + if cache_config.exists(): + with cache_config: + self._assert_secure(manifest, key, cache_config) + else: + self._assert_secure(manifest, key) + logger.debug('security: repository checks ok, allowing access') - def _assert_secure(self, manifest, key, cache_config): + def _assert_secure(self, manifest, key, cache_config=None): self.assert_location_matches(cache_config) self.assert_key_type(key, cache_config) self.assert_no_manifest_replay(manifest, key, cache_config) if not self.known(): - logger.debug('security: saving state for previously unknown repository') + logger.debug('security: remembering previously unknown repository') self.save(manifest, key) def assert_access_unknown(self, warn_if_unencrypted, manifest, key): + # warn_if_unencrypted=False is only used for initializing a new repository. + # Thus, avoiding asking about a repository that's currently initializing. if not key.logically_encrypted and not self.known(): msg = ("Warning: Attempting to access a previously unknown unencrypted repository!\n" + "Do you want to continue? [yN] ") @@ -149,9 +179,9 @@ def assert_access_unknown(self, warn_if_unencrypted, manifest, key): retry=False, env_var_override='BORG_UNKNOWN_UNENCRYPTED_REPO_ACCESS_IS_OK') if allow_access: if warn_if_unencrypted: - logger.debug('security: saving state for unknown unencrypted repository (explicitly granted)') + logger.debug('security: remembering unknown unencrypted repository (explicitly allowed)') else: - logger.debug('security: saving state for unknown unencrypted repository') + logger.debug('security: initializing unencrypted repository') self.save(manifest, key) else: raise Cache.CacheInitAbortedError() @@ -197,6 +227,9 @@ def __enter__(self): def __exit__(self, exc_type, exc_val, exc_tb): self.close() + def exists(self): + return os.path.exists(self.config_path) + def create(self): assert not self.exists() config = configparser.ConfigParser(interpolation=None) diff --git a/src/borg/testsuite/archiver.py b/src/borg/testsuite/archiver.py index 294601894..8f47b0b96 100644 --- a/src/borg/testsuite/archiver.py +++ b/src/borg/testsuite/archiver.py @@ -1709,7 +1709,7 @@ def test_common_options(self): self.create_test_files() self.cmd('init', '--encryption=repokey', self.repository_location) log = self.cmd('--debug', 'create', self.repository_location + '::test', 'input') - assert 'security: read previous_location' in log + assert 'security: read previous location' in log def _get_sizes(self, compression, compressible, size=10000): if compressible: