From 37dde581544b9b91aa82f4df71e34fa6261097aa Mon Sep 17 00:00:00 2001 From: Andrey Bienkowski Date: Fri, 28 Jan 2022 14:44:43 +0300 Subject: [PATCH 1/7] Add tests for path/to/repo/nonce deletion I am about to add documentation for this feature. Per the "If you liked it, you should have put a CI test on it" rule I am adding tests to detect if the feature regresses (causing a discrepancy between the docs and the real behavior). --- src/borg/testsuite/archiver.py | 41 ++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/src/borg/testsuite/archiver.py b/src/borg/testsuite/archiver.py index bad0c38f1..13aeef04d 100644 --- a/src/borg/testsuite/archiver.py +++ b/src/borg/testsuite/archiver.py @@ -3506,6 +3506,47 @@ id: 2 / e29442 3506da 4e1ea7 / 25f62a 5a3d41 - 02 self.assert_in('this-repository-does-not-exist', output) self.assert_not_in('this-repository-does-not-exist::test', output) + def test_can_read_repo_even_if_nonce_is_deleted(self): + self.create_regular_file('file1', contents=b'Hello, borg') + self.cmd('init', '--encryption=repokey', self.repository_location) + self.cmd('create', self.repository_location + '::test', 'input') + # Oops! We have removed the repo-side memory of the nonce! + # See https://github.com/borgbackup/borg/issues/5858 + os.remove(os.path.join(self.repository_path, 'nonce')) + + # The repo should still be readable + repo_info = self.cmd('info', self.repository_location) + assert 'All archives:' in repo_info + repo_list = self.cmd('list', self.repository_location) + assert 'test' in repo_list + # The archive should still be readable + archive_info = self.cmd('info', self.repository_location + '::test') + assert 'Archive name: test\n' in archive_info + archive_list = self.cmd('list', self.repository_location + '::test') + assert 'file1' in archive_list + # Extracting the archive should work + with changedir('output'): + self.cmd('extract', self.repository_location + '::test') + self.assert_dirs_equal('input', 'output/input') + + def test_recovery_from_deleted_repo_nonce(self): + """We should be able to recover if path/to/repo/nonce is deleted. + + The nonce is stored in two places: in the repo and in $HOME. + The nonce in the repo is only needed when multiple clients use the same + repo. Otherwise we can just use our own copy of the nonce. + """ + self.create_regular_file('file1', contents=b'Hello, borg') + self.cmd('init', '--encryption=repokey', self.repository_location) + self.cmd('create', self.repository_location + '::test', 'input') + # Oops! We have removed the repo-side memory of the nonce! + # See https://github.com/borgbackup/borg/issues/5858 + nonce = os.path.join(self.repository_path, 'nonce') + os.remove(nonce) + + self.cmd('create', self.repository_location + '::test2', 'input') + assert os.path.exists(nonce) + @unittest.skipUnless('binary' in BORG_EXES, 'no borg.exe available') class ArchiverTestCaseBinary(ArchiverTestCase): From e663c9aa100fde176bc065990cf7ac847dda1b0f Mon Sep 17 00:00:00 2001 From: Andrey Bienkowski Date: Fri, 28 Jan 2022 15:16:19 +0300 Subject: [PATCH 2/7] Strengthen the test: we can read data w/o nonces --- src/borg/testsuite/archiver.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/borg/testsuite/archiver.py b/src/borg/testsuite/archiver.py index 13aeef04d..ff0095cb1 100644 --- a/src/borg/testsuite/archiver.py +++ b/src/borg/testsuite/archiver.py @@ -3507,12 +3507,20 @@ id: 2 / e29442 3506da 4e1ea7 / 25f62a 5a3d41 - 02 self.assert_not_in('this-repository-does-not-exist::test', output) def test_can_read_repo_even_if_nonce_is_deleted(self): + """Nonce is only used for encrypting new data. + + It should be possible to retrieve the data from an archive even if + both the client and the server forget the nonce""" self.create_regular_file('file1', contents=b'Hello, borg') self.cmd('init', '--encryption=repokey', self.repository_location) self.cmd('create', self.repository_location + '::test', 'input') # Oops! We have removed the repo-side memory of the nonce! # See https://github.com/borgbackup/borg/issues/5858 os.remove(os.path.join(self.repository_path, 'nonce')) + # Oops! The client has lost the nonce too! + repository_id = bin_to_hex(self._extract_repository_id(self.repository_path)) + security_dir = get_security_dir(repository_id) + os.remove(os.path.join(security_dir, 'nonce')) # The repo should still be readable repo_info = self.cmd('info', self.repository_location) From 4931eadd91b23d54168641da22da009c436b71f1 Mon Sep 17 00:00:00 2001 From: Andrey Bienkowski Date: Fri, 28 Jan 2022 15:50:51 +0300 Subject: [PATCH 3/7] Refactor: extract get_security_dir() --- src/borg/testsuite/archiver.py | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/borg/testsuite/archiver.py b/src/borg/testsuite/archiver.py index ff0095cb1..eb9f715f8 100644 --- a/src/borg/testsuite/archiver.py +++ b/src/borg/testsuite/archiver.py @@ -384,6 +384,10 @@ class ArchiverTestCaseBase(BaseTestCase): class ArchiverTestCase(ArchiverTestCaseBase): requires_hardlinks = pytest.mark.skipif(not are_hardlinks_supported(), reason='hardlinks not supported') + def get_security_dir(self): + repository_id = bin_to_hex(self._extract_repository_id(self.repository_path)) + return get_security_dir(repository_id) + def test_basic_functionality(self): have_root = self.create_test_files() # fork required to test show-rc output @@ -718,8 +722,7 @@ class ArchiverTestCase(ArchiverTestCaseBase): 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)) + shutil.rmtree(self.get_security_dir()) 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, @@ -732,11 +735,10 @@ class ArchiverTestCase(ArchiverTestCaseBase): 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)) + security_dir = self.get_security_dir() os.rename(self.repository_path, self.repository_path + '_new') with environment_variable(BORG_RELOCATED_REPO_ACCESS_IS_OK='yes'): self.cmd('info', self.repository_location + '_new') - security_dir = get_security_dir(repository_id) with open(os.path.join(security_dir, 'location')) as fd: location = fd.read() assert location == Location(self.repository_location + '_new').canonical_path() @@ -751,9 +753,7 @@ class ArchiverTestCase(ArchiverTestCaseBase): def test_security_dir_compat(self): self.cmd('init', '--encryption=repokey', self.repository_location) - repository_id = bin_to_hex(self._extract_repository_id(self.repository_path)) - security_dir = get_security_dir(repository_id) - with open(os.path.join(security_dir, 'location'), 'w') as fd: + with open(os.path.join(self.get_security_dir(), 'location'), 'w') as fd: fd.write('something outdated') # This is fine, because the cache still has the correct information. security_dir and cache can disagree # if older versions are used to confirm a renamed repository. @@ -761,8 +761,6 @@ class ArchiverTestCase(ArchiverTestCaseBase): def test_unknown_unencrypted(self): self.cmd('init', '--encryption=none', self.repository_location) - repository_id = bin_to_hex(self._extract_repository_id(self.repository_path)) - security_dir = get_security_dir(repository_id) # Ok: repository is known self.cmd('info', self.repository_location) @@ -772,7 +770,7 @@ class ArchiverTestCase(ArchiverTestCaseBase): # Needs confirmation: cache and security dir both gone (eg. another host or rm -rf ~) shutil.rmtree(self.cache_path) - shutil.rmtree(security_dir) + shutil.rmtree(self.get_security_dir()) if self.FORK_DEFAULT: self.cmd('info', self.repository_location, exit_code=EXIT_ERROR) else: @@ -3518,9 +3516,7 @@ id: 2 / e29442 3506da 4e1ea7 / 25f62a 5a3d41 - 02 # See https://github.com/borgbackup/borg/issues/5858 os.remove(os.path.join(self.repository_path, 'nonce')) # Oops! The client has lost the nonce too! - repository_id = bin_to_hex(self._extract_repository_id(self.repository_path)) - security_dir = get_security_dir(repository_id) - os.remove(os.path.join(security_dir, 'nonce')) + os.remove(os.path.join(self.get_security_dir(), 'nonce')) # The repo should still be readable repo_info = self.cmd('info', self.repository_location) From 62fe1cab30ce669b91cbba9c87f86bd6301b9b7f Mon Sep 17 00:00:00 2001 From: Andrey Bienkowski Date: Fri, 28 Jan 2022 15:58:31 +0300 Subject: [PATCH 4/7] Doc: impact of deleting path/to/repo/nonce Fixes: https://github.com/borgbackup/borg/issues/5858 --- docs/faq.rst | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/docs/faq.rst b/docs/faq.rst index 7995a587d..f55060c7e 100644 --- a/docs/faq.rst +++ b/docs/faq.rst @@ -710,6 +710,31 @@ Send a private email to the :ref:`security contact ` if you think you have discovered a security issue. Please disclose security issues responsibly. +How important is path/to/repo/nonce? +------------------------------------ + +Borg uses :ref:`AES-CTR encryption `. An +essential part of AES-CTR is a sequential counter that must **never** +repeat. If the same value of the counter is used twice in the same repository, +an attacker can decrypt the data. The counter is stored in the home directory +of each user as well as in the repository. When creating a new archive borg uses +the highest of the two values. The value of the counter in the repository may be +higher than your local value if another user has created an archive more recently +than you did. + +Since the nonce is not necessary to read the data that is already encrypted, +``borg info``, ``borg list``, ``borg extract`` and ``borg mount`` should work +just fine without it. + +If the path/to/repo/nonce is lost, but you still have your local copy, +borg will recreate path/to/repo/nonce the next time you run ``borg create``. +This should be safe for repositories that are only used from one user account +on one machine. + +For repositories that are used by multiple users and/or from multiple machines +it is safest to avoid running *any* commands that modify the repository after +the nonce is deleted. + Common issues ############# From fab5024cb7d189fb0a4a8392b46b802d30c22d54 Mon Sep 17 00:00:00 2001 From: Andrey Bienkowski Date: Sat, 29 Jan 2022 08:58:05 +0300 Subject: [PATCH 5/7] Doc: warn about tampered server nonce https://github.com/borgbackup/borg/pull/6188#discussion_r794752672 > Well, guess one could also use max(list of trusted nonce values). > > The real issue is if you have lost all or some of the trusted > (client side) nonce values and you also have reason to not trust the > server side nonce, because someone might attack you on the server. --- docs/faq.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/faq.rst b/docs/faq.rst index f55060c7e..344cc29bc 100644 --- a/docs/faq.rst +++ b/docs/faq.rst @@ -733,7 +733,7 @@ on one machine. For repositories that are used by multiple users and/or from multiple machines it is safest to avoid running *any* commands that modify the repository after -the nonce is deleted. +the nonce is deleted or if you suspect it may have been tampered with. See :ref:`attack_model`. Common issues ############# From d6c16d77cb40c1c40618685001a0eb3bc590533d Mon Sep 17 00:00:00 2001 From: Andrey Bienkowski Date: Tue, 1 Feb 2022 23:52:59 +0300 Subject: [PATCH 6/7] Review suggestion: mention local nonce path > do we maybe also want to mention the specific path where the local > nonce is kept? --- docs/faq.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/faq.rst b/docs/faq.rst index 344cc29bc..74681d31a 100644 --- a/docs/faq.rst +++ b/docs/faq.rst @@ -717,7 +717,7 @@ Borg uses :ref:`AES-CTR encryption `. An essential part of AES-CTR is a sequential counter that must **never** repeat. If the same value of the counter is used twice in the same repository, an attacker can decrypt the data. The counter is stored in the home directory -of each user as well as in the repository. When creating a new archive borg uses +of each user (under $HOME/.config/borg/security) as well as in the repository. When creating a new archive borg uses the highest of the two values. The value of the counter in the repository may be higher than your local value if another user has created an archive more recently than you did. From 6bc1f48e4ea6bf478ccd196d86e7ff16f40a2e5f Mon Sep 17 00:00:00 2001 From: Andrey Bienkowski Date: Fri, 4 Feb 2022 22:50:32 +0300 Subject: [PATCH 7/7] Apply review suggestions --- docs/faq.rst | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/faq.rst b/docs/faq.rst index 74681d31a..ef9d35656 100644 --- a/docs/faq.rst +++ b/docs/faq.rst @@ -710,14 +710,15 @@ Send a private email to the :ref:`security contact ` if you think you have discovered a security issue. Please disclose security issues responsibly. -How important is path/to/repo/nonce? +How important are the nonce files? ------------------------------------ Borg uses :ref:`AES-CTR encryption `. An essential part of AES-CTR is a sequential counter that must **never** repeat. If the same value of the counter is used twice in the same repository, an attacker can decrypt the data. The counter is stored in the home directory -of each user (under $HOME/.config/borg/security) as well as in the repository. When creating a new archive borg uses +of each user ($HOME/.config/borg/security/$REPO_ID/nonce) as well as +in the repository (/path/to/repo/nonce). When creating a new archive borg uses the highest of the two values. The value of the counter in the repository may be higher than your local value if another user has created an archive more recently than you did. @@ -726,8 +727,8 @@ Since the nonce is not necessary to read the data that is already encrypted, ``borg info``, ``borg list``, ``borg extract`` and ``borg mount`` should work just fine without it. -If the path/to/repo/nonce is lost, but you still have your local copy, -borg will recreate path/to/repo/nonce the next time you run ``borg create``. +If the the nonce file stored in the repo is lost, but you still have your local copy, +borg will recreate the repository nonce file the next time you run ``borg create``. This should be safe for repositories that are only used from one user account on one machine.