check: handle duplicate archive items neatly

Signed-off-by: Thomas Waldmann <tw@waldmann-edv.de>

borg/archive.py

@@ -935,8 +935,18 @@ class ArchiveChecker:
             except (TypeError, ValueError, StopIteration):
                 continue
             if valid_archive(archive):
-                logger.info('Found archive %s', archive[b'name'].decode('utf-8'))
-                manifest.archives[archive[b'name'].decode('utf-8')] = {b'id': chunk_id, b'time': archive[b'time']}
+                name = archive[b'name'].decode()
+                logger.info('Found archive %s', name)
+                if name in manifest.archives:
+                    i = 1
+                    while True:
+                        new_name = '%s.%d' % (name, i)
+                        if new_name not in manifest.archives:
+                            break
+                        i += 1
+                    logger.warning('Duplicate archive name %s, storing as %s', name, new_name)
+                    name = new_name
+                manifest.archives[name] = {b'id': chunk_id, b'time': archive[b'time']}
         logger.info('Manifest rebuild complete.')
         return manifest
 

borg/testsuite/archiver.py

@@ -1468,6 +1468,33 @@ class ArchiverCheckTestCase(ArchiverTestCaseBase):
         self.assert_in('archive2', output)
         self.cmd('check', self.repository_location, exit_code=0)
 
+    def test_manifest_rebuild_duplicate_archive(self):
+        archive, repository = self.open_archive('archive1')
+        key = archive.key
+        with repository:
+            manifest = repository.get(Manifest.MANIFEST_ID)
+            corrupted_manifest = manifest + b'corrupted!'
+            repository.put(Manifest.MANIFEST_ID, corrupted_manifest)
+
+            archive = msgpack.packb({
+                'cmdline': [],
+                'items': [],
+                'hostname': 'foo',
+                'username': 'bar',
+                'name': 'archive1',
+                'time': '2016-12-15T18:49:51.849711',
+                'version': 1,
+            })
+            archive_id = key.id_hash(archive)
+            repository.put(archive_id, key.encrypt(archive))
+            repository.commit()
+        self.cmd('check', self.repository_location, exit_code=1)
+        self.cmd('check', '--repair', self.repository_location, exit_code=0)
+        output = self.cmd('list', self.repository_location)
+        self.assert_in('archive1', output)
+        self.assert_in('archive1.1', output)
+        self.assert_in('archive2', output)
+
     def test_extra_chunks(self):
         self.cmd('check', self.repository_location, exit_code=0)
         with Repository(self.repository_location, exclusive=True) as repository:

docs/changes.rst

@@ -134,6 +134,9 @@ Security fixes:
 - A flaw in the cryptographic authentication scheme in Borg allowed an attacker
   to spoof the manifest. See :ref:`tam_vuln` above for the steps you should
   take.
+- borg check: When rebuilding the manifest (which should only be needed very rarely)
+  duplicate archive names would be handled on a "first come first serve" basis, allowing
+  an attacker to apparently replace archives.
 
 Bug fixes: