Merge pull request #10371 from ThomasWaldmann/drop-manifest-replay-check

security: drop the manifest timestamp replay check

docs/faq.rst

@@ -76,34 +76,6 @@ Warning: Using Borg with multiple repositories that have identical repository ID
 creating 1:1 repository copies) is not supported and can lead to various issues,
 for example cache coherency issues, malfunction, or data corruption.
 
-"this is either an attack or unsafe" warning
---------------------------------------------
-
-About the warning:
-
-  Cache or information obtained from the security directory is newer than the
-  repository — this is either an attack or unsafe (multiple repositories with the same ID)
-
-"unsafe": If not following the advice from the previous section, you can easily
-run into this by yourself by restoring an older copy of your repository.
-
-"attack": An attacker may have replaced your repo with an older copy, trying to
-trigger AES counter reuse and break your repo encryption.
-
-Borg users have also reported that file system issues (e.g., hardware issues or I/O errors causing
-the file system to become read-only) can cause this warning, see :issue:`7853`.
-
-If you decide to ignore this and accept unsafe operation for this repository,
-you could delete the manifest-timestamp and the local cache:
-
-::
-
-  borg repo-info   # shows the repository ID and the security directory path
-  rm SECURITY_DIRECTORY/manifest-timestamp
-  borg repo-delete --cache-only
-
-This is an unsafe and unsupported way to use Borg. You have been warned.
-
 Which file types, attributes, etc. are *not* preserved?
 -------------------------------------------------------
 

docs/internals/data-structures.rst

@@ -257,8 +257,10 @@ borg 1.x repositories have (they are supported read-only, e.g. for
 ``borg transfer``). The versions differ in the way feature flags are handled,
 described below.
 
-The *timestamp* field is used to avoid logical replay attacks where
-the server just resets the repository to a previous state.
+The *timestamp* field records when the manifest was last written. It is kept
+strictly monotonically increasing across writes (even if the clock went
+backwards) and is shown by ``borg repo-info`` as the repository's last
+modification time.
 
 The *archives* dict is always empty: the list of archives is not part of the
 manifest, each archive has its own pointer object in the ``archives/``

src/borg/cache.py

@@ -1298,12 +1298,12 @@ class AdHocWithFilesCache(FilesCacheMixin, ChunksMixin):
 
         # Warn user before sending data to a never seen before unencrypted repository
         if not self.path.exists():
-            self.security_manager.assert_access_unknown(warn_if_unencrypted, manifest, self.key)
+            self.security_manager.assert_access_unknown(warn_if_unencrypted, self.key)
             self.create()
 
         self.open()
         try:
-            self.security_manager.assert_secure(manifest, self.key)
+            self.security_manager.assert_secure(self.key)
 
             if not self.check_cache_compatibility():
                 self.wipe_cache()
@@ -1335,7 +1335,7 @@ class AdHocWithFilesCache(FilesCacheMixin, ChunksMixin):
         self.cache_config.load()
 
     def close(self):
-        self.security_manager.save(self.manifest, self.key)
+        self.security_manager.save(self.key)
         pi = ProgressIndicatorMessage(msgid="cache.close")
         if self._files is not None:
             pi.output("Saving files cache")

src/borg/manifest.py

@@ -589,7 +589,8 @@ class Manifest:
     def write(self):
         from .item import ManifestItem
 
-        # self.timestamp needs to be strictly monotonically increasing. Clocks often are not set correctly
+        # self.timestamp is the repository's "last modified" stamp (shown by "borg repo-info"). It is kept
+        # strictly monotonically increasing so it never goes backwards, as clocks often are not set correctly.
         if self.timestamp is None:
             self.timestamp = datetime.now(tz=UTC).isoformat(timespec="microseconds")
         else:

src/borg/security.py

@@ -39,6 +39,8 @@ class RepositoryAccessAborted(Error):
     exit_mcode = 62
 
 
+# The two errors below are not raised anymore: the manifest timestamp replay check that raised them was
+# removed. The classes are kept so that their return codes stay reserved and never get a different meaning.
 class RepositoryIDNotUnique(Error):
     """Cache is newer than repository - do you have multiple, independently updated repos with same ID?"""
 
@@ -54,7 +56,7 @@ class RepositoryReplay(Error):
 class SecurityManager:
     """
     Tracks repositories. Ensures that nothing bad happens (repository swaps,
-    replay attacks, unknown repositories, etc.).
+    relocations, unknown unencrypted 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
@@ -76,7 +78,6 @@ class SecurityManager:
         self.dir = Path(_security_dir(repository))
         self.key_type_file = self.dir / "key-type"
         self.location_file = self.dir / "location"
-        self.manifest_ts_file = self.dir / "manifest-timestamp"
 
     @staticmethod
     def destroy(repository, path=None):
@@ -86,7 +87,7 @@ class SecurityManager:
             shutil.rmtree(path)
 
     def known(self):
-        return all(f.exists() for f in (self.key_type_file, self.location_file, self.manifest_ts_file))
+        return all(f.exists() for f in (self.key_type_file, self.location_file))
 
     def key_matches(self, key):
         if not self.known():
@@ -100,19 +101,16 @@ class SecurityManager:
         except OSError as exc:
             logger.warning("Could not read/parse key type file: %s", exc)
 
-    def save(self, manifest, key):
+    def save(self, key):
         logger.debug("security: saving state for %s to %s", self.repository.id_str, str(self.dir))
         current_location = self.repository._location.canonical_path()
         logger.debug("security: current location   %s", current_location)
         key_type = key.TYPE if key.stored_type is None else key.stored_type
         logger.debug("security: key type           %s", str(key_type))
-        logger.debug("security: manifest timestamp %s", manifest.timestamp)
         with SaveFile(self.location_file) as fd:
             fd.write(current_location)
         with SaveFile(self.key_type_file) as fd:
             fd.write(str(key_type))
-        with SaveFile(self.manifest_ts_file) as fd:
-            fd.write(manifest.timestamp)
 
     def assert_location_matches(self):
         # Warn user before sending data to a relocated repository
@@ -152,49 +150,26 @@ class SecurityManager:
             with SaveFile(self.location_file) as fd:
                 fd.write(repository_location)
 
-    def assert_no_manifest_replay(self, manifest, key):
-
-        try:
-            with self.manifest_ts_file.open() as fd:
-                timestamp = fd.read()
-            logger.debug("security: read manifest timestamp %r", timestamp)
-        except FileNotFoundError:
-            logger.debug("security: manifest timestamp file %s not found", self.manifest_ts_file)
-            timestamp = ""
-        except OSError as exc:
-            logger.warning("Could not read previous location file: %s", exc)
-            timestamp = ""
-        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:
-            if not key.has_secret_key:
-                # unkeyed modes ("none-*", and borg 1.x "none"): the repository id is not derived
-                # from any secret, so two repositories can legitimately have the same id.
-                raise RepositoryIDNotUnique()
-            else:
-                raise RepositoryReplay()
-
     def assert_key_type(self, key):
         # Make sure an encrypted repository has not been swapped for an unencrypted repository
         if self.known() and not self.key_matches(key):
             raise EncryptionMethodMismatch()
 
-    def assert_secure(self, manifest, key, *, warn_if_unencrypted=True):
+    def assert_secure(self, key, *, 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)
-        self._assert_secure(manifest, key)
+        self.assert_access_unknown(warn_if_unencrypted, key)
+        self._assert_secure(key)
         logger.debug("security: repository checks ok, allowing access")
 
-    def _assert_secure(self, manifest, key):
+    def _assert_secure(self, key):
         self.assert_location_matches()
         self.assert_key_type(key)
-        self.assert_no_manifest_replay(manifest, key)
         if not self.known():
             logger.debug("security: remembering previously unknown repository")
-            self.save(manifest, key)
+            self.save(key)
 
-    def assert_access_unknown(self, warn_if_unencrypted, manifest, key):
+    def assert_access_unknown(self, warn_if_unencrypted, 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():
@@ -223,11 +198,11 @@ class SecurityManager:
                     logger.debug("security: remembering unknown unencrypted repository (explicitly allowed)")
                 else:
                     logger.debug("security: initializing unencrypted repository")
-                self.save(manifest, key)
+                self.save(key)
             else:
                 raise CacheInitAbortedError()
 
 
 def assert_secure(repository, manifest):
     sm = SecurityManager(repository)
-    sm.assert_secure(manifest, manifest.key)
+    sm.assert_secure(manifest.key)

src/borg/testsuite/archiver/checks_test.py

@@ -170,7 +170,7 @@ def test_repository_move(archivers, request, monkeypatch):
     # it also needs no confirmation if we have no knowledge about the previous location.
     cmd(archiver, "repo-info")
     # it will re-create security-related infos in the security dir:
-    for file in ("location", "key-type", "manifest-timestamp"):
+    for file in ("location", "key-type"):
         assert os.path.exists(os.path.join(security_dir, file))
 
 
@@ -318,3 +318,36 @@ def test_remote_repo_strip_components_doesnt_leak(remote_archiver):
         with assert_creates_file("input/dir/file"):
             res = cmd(remote_archiver, "extract", "test", "--debug", "--strip-components", "0")
             assert marker not in res
+
+
+def test_stale_manifest_timestamp_file_is_ignored(archivers, request):
+    # older borg 2 versions stored the manifest timestamp in the security dir and refused access when the
+    # repository's manifest was older than that. that check is gone: a leftover (even "future") file is ignored.
+    archiver = request.getfixturevalue(archivers)
+    cmd(archiver, "repo-create", RK_ENCRYPTION)
+    security_dir = get_security_directory(archiver.repository_path)
+    with open(os.path.join(security_dir, "manifest-timestamp"), "w") as fd:
+        fd.write("9999-01-01T00:00:00.000000")
+    cmd(archiver, "repo-info")
+    cmd(archiver, "create", "test", "input")
+
+
+def test_repository_restored_from_older_copy_is_accepted(archivers, request):
+    # restoring an older copy of the repository (a "rollback") must not lock the user out of it.
+    archiver = request.getfixturevalue(archivers)
+    if archiver.get_kind() != "local":
+        pytest.skip("needs a local repository to copy")
+    create_regular_file(archiver.input_path, "file1", size=1024)
+    cmd(archiver, "repo-create", RK_ENCRYPTION)
+    cmd(archiver, "create", "archive1", "input")
+    backup_path = archiver.repository_path + "_copy"
+    shutil.copytree(archiver.repository_path, backup_path)
+    cmd(archiver, "create", "archive2", "input")
+    shutil.rmtree(archiver.repository_path)
+    shutil.copytree(backup_path, archiver.repository_path)
+    output = cmd(archiver, "repo-list")
+    assert "archive1" in output
+    assert "archive2" not in output
+    # the files cache still knows the files from archive2 (whose chunks are gone), that must not matter.
+    cmd(archiver, "create", "archive3", "input")
+    cmd(archiver, "check")

src/borg/testsuite/archiver/transfer_cmd_test.py

@@ -368,7 +368,7 @@ def test_transfer_from_borg1_known_repo(archivers, request, monkeypatch, tmp_pat
     key_type_file = repo_security_dir / "key-type"
     key_type_file.write_text(str(KeyType.REPO))
     (repo_security_dir / "location").write_text(Location(Path(src_repo).absolute().as_uri()).canonical_path())
-    (repo_security_dir / "manifest-timestamp").write_text("1970-01-01T00:00:00.000000")
+    (repo_security_dir / "manifest-timestamp").write_text("1970-01-01T00:00:00.000000")  # borg 2 ignores it
 
     other_repo1 = f"--other-repo={src_repo}"
     cmd(archiver, "repo-create", RK_ENCRYPTION, other_repo1, "--from-borg1")

src/borg/testsuite/storelocking_test.py

@@ -249,8 +249,7 @@ class TestLock:
 
     def test_skew_warning_below_stale_threshold(self, lockstore):
         # a live lock whose writer's clock runs 10 minutes ahead of ours: far from the stale
-        # threshold, but still worth a warning (e.g. concurrent manifest writes could produce
-        # a spurious RepositoryReplay later), see #9870.
+        # threshold, but still worth a warning, see #9870.
         dt = datetime.datetime.now(datetime.UTC) + datetime.timedelta(minutes=10)
         write_raw_lock(lockstore, ID1, exclusive=False, dt=dt)  # store mtime: now
         lock = Lock(lockstore, exclusive=False, id=ID2)