From d964101eb53a4f062e218771eeb6a071c1b97fb1 Mon Sep 17 00:00:00 2001 From: Marian Beermann Date: Fri, 5 May 2017 17:26:24 +0200 Subject: [PATCH 1/2] consider repokey w/o passphrase == unencrypted --- docs/changes.rst | 9 +++++++++ src/borg/cache.py | 2 +- src/borg/crypto/key.py | 8 ++++++++ src/borg/testsuite/archiver.py | 23 +++++++++++++++++++++++ 4 files changed, 41 insertions(+), 1 deletion(-) diff --git a/docs/changes.rst b/docs/changes.rst index ec226a3af..113f09097 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -128,6 +128,15 @@ The best check that everything is ok is to run a dry-run extraction:: Changelog ========= +Version 1.1.0b6 (unreleased) +---------------------------- + +Compatibility notes: + +- Repositories in a repokey mode with a blank passphrase are now treated + as unencrypted repositories for security checks + (e.g. BORG_UNKNOWN_UNENCRYPTED_REPO_ACCESS_IS_OK). + Version 1.1.0b5 (2017-04-30) ---------------------------- diff --git a/src/borg/cache.py b/src/borg/cache.py index 8f428f8ca..aeb9d3d4d 100644 --- a/src/borg/cache.py +++ b/src/borg/cache.py @@ -130,7 +130,7 @@ def assert_secure(self, manifest, key, cache): self.save(manifest, key, cache) def assert_access_unknown(self, warn_if_unencrypted, key): - if warn_if_unencrypted and isinstance(key, PlaintextKey) and not self.known(): + if warn_if_unencrypted and not key.passphrase_protected and not self.known(): msg = ("Warning: Attempting to access a previously unknown unencrypted repository!\n" + "Do you want to continue? [yN] ") if not yes(msg, false_msg="Aborting.", invalid_msg="Invalid answer, aborting.", diff --git a/src/borg/crypto/key.py b/src/borg/crypto/key.py index b2f66da9f..9469be297 100644 --- a/src/borg/crypto/key.py +++ b/src/borg/crypto/key.py @@ -234,6 +234,7 @@ class PlaintextKey(KeyBase): STORAGE = KeyBlobStorage.NO_STORAGE chunk_seed = 0 + passphrase_protected = False def __init__(self, repository): super().__init__(repository) @@ -329,6 +330,8 @@ class AESKeyBase(KeyBase): MAC = hmac_sha256 + passphrase_protected = True + def encrypt(self, chunk): data = self.compressor.compress(chunk) self.nonce_manager.ensure_reservation(num_aes_blocks(len(data))) @@ -700,6 +703,10 @@ def get_new_target(self, args): return self.repository def load(self, target, passphrase): + # While the repository is encrypted, we consider a repokey repository with a blank + # passphrase an unencrypted repository. + self.passphrase_protected = passphrase != '' + # what we get in target is just a repo location, but we already have the repo obj: target = self.repository key_data = target.load_key() @@ -710,6 +717,7 @@ def load(self, target, passphrase): return success def save(self, target, passphrase): + self.passphrase_protected = passphrase != '' key_data = self._save(passphrase) key_data = key_data.encode('utf-8') # remote repo: msgpack issue #99, giving bytes target.save_key(key_data) diff --git a/src/borg/testsuite/archiver.py b/src/borg/testsuite/archiver.py index d5dffe8d0..64107c5b1 100644 --- a/src/borg/testsuite/archiver.py +++ b/src/borg/testsuite/archiver.py @@ -607,6 +607,29 @@ def test_repository_swap_detection2_no_cache(self): with pytest.raises(Cache.RepositoryAccessAborted): self.cmd('create', self.repository_location + '_encrypted::test.2', 'input') + def test_repository_swap_detection_repokey_blank_passphrase(self): + # Check that a repokey repo with a blank passphrase is considered like a plaintext repo. + self.create_test_files() + # User initializes her repository with her passphrase + self.cmd('init', '--encryption=repokey', self.repository_location) + self.cmd('create', self.repository_location + '::test', 'input') + # Attacker replaces it with her own repository, which is encrypted but has no passphrase set + shutil.rmtree(self.repository_path) + with environment_variable(BORG_PASSPHRASE=''): + self.cmd('init', '--encryption=repokey', self.repository_location) + # Delete cache & security database, AKA switch to user perspective + self.cmd('delete', '--cache-only', self.repository_location) + repository_id = bin_to_hex(self._extract_repository_id(self.repository_path)) + shutil.rmtree(get_security_dir(repository_id)) + with environment_variable(BORG_PASSPHRASE=None): + # This is the part were the user would be tricked, e.g. she assumes that BORG_PASSPHRASE + # is set, while it isn't. Previously this raised no warning, + # since the repository is, technically, encrypted. + if self.FORK_DEFAULT: + self.cmd('create', self.repository_location + '::test.2', 'input', exit_code=EXIT_ERROR) + else: + self.assert_raises(Cache.CacheInitAbortedError, lambda: self.cmd('create', self.repository_location + '::test.2', 'input')) + def test_repository_move(self): self.cmd('init', '--encryption=repokey', self.repository_location) repository_id = bin_to_hex(self._extract_repository_id(self.repository_path)) From 1daab244c6ceb5ed722ba3447fc1f97f73fec1d4 Mon Sep 17 00:00:00 2001 From: Marian Beermann Date: Sun, 7 May 2017 22:21:40 +0200 Subject: [PATCH 2/2] testsuite.archiver: normalise pytest.raises vs. assert_raises --- src/borg/testsuite/archiver.py | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/borg/testsuite/archiver.py b/src/borg/testsuite/archiver.py index 64107c5b1..baf1c60ca 100644 --- a/src/borg/testsuite/archiver.py +++ b/src/borg/testsuite/archiver.py @@ -560,7 +560,8 @@ def test_repository_swap_detection(self): if self.FORK_DEFAULT: self.cmd('create', self.repository_location + '::test.2', 'input', exit_code=EXIT_ERROR) else: - self.assert_raises(Cache.EncryptionMethodMismatch, lambda: self.cmd('create', self.repository_location + '::test.2', 'input')) + with pytest.raises(Cache.EncryptionMethodMismatch): + self.cmd('create', self.repository_location + '::test.2', 'input') def test_repository_swap_detection2(self): self.create_test_files() @@ -573,7 +574,8 @@ def test_repository_swap_detection2(self): if self.FORK_DEFAULT: self.cmd('create', self.repository_location + '_encrypted::test.2', 'input', exit_code=EXIT_ERROR) else: - self.assert_raises(Cache.RepositoryAccessAborted, lambda: self.cmd('create', self.repository_location + '_encrypted::test.2', 'input')) + with pytest.raises(Cache.RepositoryAccessAborted): + self.cmd('create', self.repository_location + '_encrypted::test.2', 'input') def test_repository_swap_detection_no_cache(self): self.create_test_files() @@ -589,7 +591,8 @@ def test_repository_swap_detection_no_cache(self): if self.FORK_DEFAULT: self.cmd('create', self.repository_location + '::test.2', 'input', exit_code=EXIT_ERROR) else: - self.assert_raises(Cache.EncryptionMethodMismatch, lambda: self.cmd('create', self.repository_location + '::test.2', 'input')) + with pytest.raises(Cache.EncryptionMethodMismatch): + self.cmd('create', self.repository_location + '::test.2', 'input') def test_repository_swap_detection2_no_cache(self): self.create_test_files() @@ -628,7 +631,8 @@ def test_repository_swap_detection_repokey_blank_passphrase(self): if self.FORK_DEFAULT: self.cmd('create', self.repository_location + '::test.2', 'input', exit_code=EXIT_ERROR) else: - self.assert_raises(Cache.CacheInitAbortedError, lambda: self.cmd('create', self.repository_location + '::test.2', 'input')) + with pytest.raises(Cache.CacheInitAbortedError): + self.cmd('create', self.repository_location + '::test.2', 'input') def test_repository_move(self): self.cmd('init', '--encryption=repokey', self.repository_location) @@ -2255,7 +2259,8 @@ def test_key_import_errors(self): if self.FORK_DEFAULT: self.cmd('key', 'import', self.repository_location, export_file, exit_code=2) else: - self.assert_raises(NotABorgKeyFile, lambda: self.cmd('key', 'import', self.repository_location, export_file)) + with pytest.raises(NotABorgKeyFile): + self.cmd('key', 'import', self.repository_location, export_file) with open(export_file, 'w') as fd: fd.write('BORG_KEY a0a0a0\n') @@ -2263,7 +2268,8 @@ def test_key_import_errors(self): if self.FORK_DEFAULT: self.cmd('key', 'import', self.repository_location, export_file, exit_code=2) else: - self.assert_raises(RepoIdMismatch, lambda: self.cmd('key', 'import', self.repository_location, export_file)) + with pytest.raises(RepoIdMismatch): + self.cmd('key', 'import', self.repository_location, export_file) def test_key_export_paperkey(self): repo_id = 'e294423506da4e1ea76e8dcdf1a3919624ae3ae496fddf905610c351d3f09239' @@ -2676,13 +2682,13 @@ def test_remote_repo_restrict_to_path(self): self.cmd('init', '--encryption=repokey', self.repository_location) # restricted to repo directory itself, fail for other directories with same prefix: with patch.object(RemoteRepository, 'extra_test_args', ['--restrict-to-path', self.repository_path]): - self.assert_raises(PathNotAllowed, - lambda: self.cmd('init', '--encryption=repokey', self.repository_location + '_0')) + with pytest.raises(PathNotAllowed): + self.cmd('init', '--encryption=repokey', self.repository_location + '_0') # restricted to a completely different path: with patch.object(RemoteRepository, 'extra_test_args', ['--restrict-to-path', '/foo']): - self.assert_raises(PathNotAllowed, - lambda: self.cmd('init', '--encryption=repokey', self.repository_location + '_1')) + with pytest.raises(PathNotAllowed): + self.cmd('init', '--encryption=repokey', self.repository_location + '_1') path_prefix = os.path.dirname(self.repository_path) # restrict to repo directory's parent directory: with patch.object(RemoteRepository, 'extra_test_args', ['--restrict-to-path', path_prefix]):