From 21d44071702e20b1d868fc3482bb29e1198456f3 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Mon, 12 Jun 2023 23:49:53 +0200 Subject: [PATCH 1/3] always implicitly require manifest TAMs remove a lot of complexity from the code that was just there to support legacy borg versions < 1.0.9 which did not TAM authenticate the manifest. since then, borg writes TAM authentication to the manifest, even if the repo is unencrypted. if the repo is unencrypted, it did not check the somehow pointless authentication that was generated without any secret, but if we add that fake TAM, we can also verify the fake TAM. if somebody explicitly switches off all crypto, they can not expect authentication. for everybody else, borg now always generates the TAM and also verifies it. --- src/borg/archiver/key_cmds.py | 2 +- src/borg/archiver/rcreate_cmd.py | 6 +---- src/borg/crypto/key.py | 43 +++++--------------------------- src/borg/item.pyx | 3 +-- src/borg/manifest.py | 25 +++---------------- src/borg/testsuite/key.py | 35 ++++++-------------------- 6 files changed, 19 insertions(+), 95 deletions(-) diff --git a/src/borg/archiver/key_cmds.py b/src/borg/archiver/key_cmds.py index a365848ff..bdc93a556 100644 --- a/src/borg/archiver/key_cmds.py +++ b/src/borg/archiver/key_cmds.py @@ -63,7 +63,7 @@ class KeysMixIn: print("Change not needed or not supported.") return EXIT_WARNING - for name in ("repository_id", "crypt_key", "id_key", "chunk_seed", "tam_required", "sessionid", "cipher"): + for name in ("repository_id", "crypt_key", "id_key", "chunk_seed", "sessionid", "cipher"): value = getattr(key, name) setattr(key_new, name, value) diff --git a/src/borg/archiver/rcreate_cmd.py b/src/borg/archiver/rcreate_cmd.py index 8fce7dbbe..e6545a03a 100644 --- a/src/borg/archiver/rcreate_cmd.py +++ b/src/borg/archiver/rcreate_cmd.py @@ -3,7 +3,7 @@ import argparse from ._common import with_repository, with_other_repository, Highlander from ..cache import Cache from ..constants import * # NOQA -from ..crypto.key import key_creator, key_argument_names, tam_required_file +from ..crypto.key import key_creator, key_argument_names from ..helpers import EXIT_WARNING from ..helpers import location_validator, Location from ..helpers import parse_storage_quota @@ -35,10 +35,6 @@ class RCreateMixIn: repository.commit(compact=False) with Cache(repository, manifest, warn_if_unencrypted=False): pass - if key.tam_required: - tam_file = tam_required_file(repository) - open(tam_file, "w").close() - if key.NAME != "plaintext": logger.warning( "\n" diff --git a/src/borg/crypto/key.py b/src/borg/crypto/key.py index 3fb4818aa..d6d6b32e5 100644 --- a/src/borg/crypto/key.py +++ b/src/borg/crypto/key.py @@ -136,16 +136,6 @@ def key_factory(repository, manifest_chunk, *, ro_cls=RepoObj): return identify_key(manifest_data).detect(repository, manifest_data) -def tam_required_file(repository): - security_dir = get_security_dir(bin_to_hex(repository.id), legacy=(repository.version == 1)) - return os.path.join(security_dir, "tam_required") - - -def tam_required(repository): - file = tam_required_file(repository) - return os.path.isfile(file) - - def uses_same_chunker_secret(other_key, key): """is the chunker secret the same?""" # avoid breaking the deduplication by a different chunker secret @@ -211,7 +201,6 @@ class KeyBase: self.TYPE_STR = bytes([self.TYPE]) self.repository = repository self.target = None # key location file path / repo obj - self.tam_required = True self.copy_crypt_key = False def id_hash(self, data): @@ -253,39 +242,25 @@ class KeyBase: tam["hmac"] = hmac.digest(tam_key, packed, "sha512") return msgpack.packb(metadata_dict) - def unpack_and_verify_manifest(self, data, force_tam_not_required=False): - """Unpack msgpacked *data* and return (object, did_verify).""" + def unpack_and_verify_manifest(self, data): + """Unpack msgpacked *data* and return manifest.""" if data.startswith(b"\xc1" * 4): # This is a manifest from the future, we can't read it. raise UnsupportedManifestError() - tam_required = self.tam_required - if force_tam_not_required and tam_required: - logger.warning("Manifest authentication DISABLED.") - tam_required = False data = bytearray(data) unpacker = get_limited_unpacker("manifest") unpacker.feed(data) unpacked = unpacker.unpack() if AUTHENTICATED_NO_KEY: - return unpacked, True # True is a lie. + return unpacked if "tam" not in unpacked: - if tam_required: - raise TAMRequiredError(self.repository._location.canonical_path()) - else: - logger.debug("Manifest TAM not found and not required") - return unpacked, False + raise TAMRequiredError(self.repository._location.canonical_path()) tam = unpacked.pop("tam", None) if not isinstance(tam, dict): raise TAMInvalid() tam_type = tam.get("type", "") if tam_type != "HKDF_HMAC_SHA512": - if tam_required: - raise TAMUnsupportedSuiteError(repr(tam_type)) - else: - logger.debug( - "Ignoring manifest TAM made with unsupported suite, since TAM is not required: %r", tam_type - ) - return unpacked, False + raise TAMUnsupportedSuiteError(repr(tam_type)) tam_hmac = tam.get("hmac") tam_salt = tam.get("salt") if not isinstance(tam_salt, (bytes, str)) or not isinstance(tam_hmac, (bytes, str)): @@ -299,7 +274,7 @@ class KeyBase: if not hmac.compare_digest(calculated_hmac, tam_hmac): raise TAMInvalid() logger.debug("TAM-verified manifest") - return unpacked, True + return unpacked def unpack_and_verify_archive(self, data, force_tam_not_required=False): """Unpack msgpacked *data* and return (object, did_verify).""" @@ -357,10 +332,6 @@ class PlaintextKey(KeyBase): chunk_seed = 0 logically_encrypted = False - def __init__(self, repository): - super().__init__(repository) - self.tam_required = False - @classmethod def create(cls, repository, args, **kw): logger.info('Encryption NOT enabled.\nUse the "--encryption=repokey|keyfile" to enable encryption.') @@ -526,7 +497,6 @@ class FlexiKey: self.crypt_key = key.crypt_key self.id_key = key.id_key self.chunk_seed = key.chunk_seed - self.tam_required = key.get("tam_required", tam_required(self.repository)) return True return False @@ -639,7 +609,6 @@ class FlexiKey: crypt_key=self.crypt_key, id_key=self.id_key, chunk_seed=self.chunk_seed, - tam_required=self.tam_required, ) data = self.encrypt_key_file(msgpack.packb(key.as_dict()), passphrase, algorithm) key_data = "\n".join(textwrap.wrap(b2a_base64(data).decode("ascii"))) diff --git a/src/borg/item.pyx b/src/borg/item.pyx index 5da91c2ed..04dd884f2 100644 --- a/src/borg/item.pyx +++ b/src/borg/item.pyx @@ -467,7 +467,7 @@ cdef class Key(PropDict): crypt_key = PropDictProperty(bytes) id_key = PropDictProperty(bytes) chunk_seed = PropDictProperty(int) - tam_required = PropDictProperty(bool) + tam_required = PropDictProperty(bool) # legacy. borg now implicitly always requires TAM. def update_internal(self, d): # legacy support for migration (data from old msgpacks comes in as bytes always, but sometimes we want str) @@ -650,7 +650,6 @@ class ItemDiff: self._can_compare_chunk_ids = can_compare_chunk_ids self._chunk_1 = chunk_1 self._chunk_2 = chunk_2 - self._changes = {} if self._item1.is_link() or self._item2.is_link(): diff --git a/src/borg/manifest.py b/src/borg/manifest.py index 592b53194..0768ce44c 100644 --- a/src/borg/manifest.py +++ b/src/borg/manifest.py @@ -1,6 +1,4 @@ import enum -import os -import os.path import re from collections import abc, namedtuple from datetime import datetime, timedelta, timezone @@ -229,7 +227,6 @@ class Manifest: self.repo_objs = ro_cls(key) self.repository = repository self.item_keys = frozenset(item_keys) if item_keys is not None else ITEM_KEYS - self.tam_verified = False self.timestamp = None @property @@ -241,9 +238,9 @@ class Manifest: return parse_timestamp(self.timestamp) @classmethod - def load(cls, repository, operations, key=None, force_tam_not_required=False, *, ro_cls=RepoObj): + def load(cls, repository, operations, key=None, *, ro_cls=RepoObj): from .item import ManifestItem - from .crypto.key import key_factory, tam_required_file, tam_required + from .crypto.key import key_factory from .repository import Repository try: @@ -254,9 +251,7 @@ class Manifest: key = key_factory(repository, cdata, ro_cls=ro_cls) manifest = cls(key, repository, ro_cls=ro_cls) _, data = manifest.repo_objs.parse(cls.MANIFEST_ID, cdata) - manifest_dict, manifest.tam_verified = key.unpack_and_verify_manifest( - data, force_tam_not_required=force_tam_not_required - ) + manifest_dict = key.unpack_and_verify_manifest(data) m = ManifestItem(internal_dict=manifest_dict) manifest.id = manifest.repo_objs.id_hash(data) if m.get("version") not in (1, 2): @@ -268,17 +263,6 @@ class Manifest: manifest.item_keys = ITEM_KEYS manifest.item_keys |= frozenset(m.config.get("item_keys", [])) # new location of item_keys since borg2 manifest.item_keys |= frozenset(m.get("item_keys", [])) # legacy: borg 1.x: item_keys not in config yet - - if manifest.tam_verified: - manifest_required = manifest.config.get("tam_required", False) - security_required = tam_required(repository) - if manifest_required and not security_required: - logger.debug("Manifest is TAM verified and says TAM is required, updating security database...") - file = tam_required_file(repository) - open(file, "w").close() - if not manifest_required and security_required: - logger.debug("Manifest is TAM verified and says TAM is *not* required, updating security database...") - os.unlink(tam_required_file(repository)) manifest.check_repository_compatibility(operations) return manifest @@ -310,8 +294,6 @@ class Manifest: def write(self): from .item import ManifestItem - if self.key.tam_required: - self.config["tam_required"] = True # self.timestamp needs to be strictly monotonically increasing. Clocks often are not set correctly if self.timestamp is None: self.timestamp = datetime.now(tz=timezone.utc).isoformat(timespec="microseconds") @@ -331,7 +313,6 @@ class Manifest: timestamp=self.timestamp, config=StableDict(self.config), ) - self.tam_verified = True data = self.key.pack_and_authenticate_metadata(manifest.as_dict()) self.id = self.repo_objs.id_hash(data) self.repository.put(self.MANIFEST_ID, self.repo_objs.format(self.MANIFEST_ID, {}, data)) diff --git a/src/borg/testsuite/key.py b/src/borg/testsuite/key.py index 73d9df35d..a8fd37681 100644 --- a/src/borg/testsuite/key.py +++ b/src/borg/testsuite/key.py @@ -272,39 +272,19 @@ class TestTAM: with pytest.raises(msgpack.UnpackException): key.unpack_and_verify_manifest(blob) - def test_missing_when_required(self, key): - blob = msgpack.packb({}) - with pytest.raises(TAMRequiredError): - key.unpack_and_verify_manifest(blob) - with pytest.raises(TAMRequiredError): - key.unpack_and_verify_archive(blob) - def test_missing(self, key): blob = msgpack.packb({}) - key.tam_required = False - unpacked, verified = key.unpack_and_verify_manifest(blob) - assert unpacked == {} - assert not verified - unpacked, verified, _ = key.unpack_and_verify_archive(blob) - assert unpacked == {} - assert not verified - - def test_unknown_type_when_required(self, key): - blob = msgpack.packb({"tam": {"type": "HMAC_VOLLBIT"}}) - with pytest.raises(TAMUnsupportedSuiteError): + with pytest.raises(TAMRequiredError): key.unpack_and_verify_manifest(blob) - with pytest.raises(TAMUnsupportedSuiteError): + with pytest.raises(TAMRequiredError): key.unpack_and_verify_archive(blob) def test_unknown_type(self, key): blob = msgpack.packb({"tam": {"type": "HMAC_VOLLBIT"}}) - key.tam_required = False - unpacked, verified = key.unpack_and_verify_manifest(blob) - assert unpacked == {} - assert not verified - unpacked, verified, _ = key.unpack_and_verify_archive(blob) - assert unpacked == {} - assert not verified + with pytest.raises(TAMUnsupportedSuiteError): + key.unpack_and_verify_manifest(blob) + with pytest.raises(TAMUnsupportedSuiteError): + key.unpack_and_verify_archive(blob) @pytest.mark.parametrize( "tam, exc", @@ -360,8 +340,7 @@ class TestTAM: unpacked = msgpack.unpackb(blob) assert unpacked["tam"]["type"] == "HKDF_HMAC_SHA512" - unpacked, verified = key.unpack_and_verify_manifest(blob) - assert verified + unpacked = key.unpack_and_verify_manifest(blob) assert unpacked["foo"] == "bar" assert "tam" not in unpacked From 2d78fa89a513b7a21403e259de41e21a2a475b27 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Sun, 3 Sep 2023 21:49:18 +0200 Subject: [PATCH 2/3] always implicitly require archive TAMs they must be there since the upgrade to borg 1.2.6 (or other borg versions that also have a fix for CVE-2023-36811). --- src/borg/archive.py | 9 ++++---- src/borg/cache.py | 2 +- src/borg/crypto/key.py | 30 +++++++-------------------- src/borg/testsuite/archiver/checks.py | 3 --- src/borg/testsuite/key.py | 3 +-- 5 files changed, 13 insertions(+), 34 deletions(-) diff --git a/src/borg/archive.py b/src/borg/archive.py index defbb28a0..3ce5e62c2 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -493,7 +493,7 @@ class Archive: self.name = name # overwritten later with name from archive metadata self.name_in_manifest = name # can differ from .name later (if borg check fixed duplicate archive names) self.comment = None - self.tam_verified = False + self.tam_verified = True self.numeric_ids = numeric_ids self.noatime = noatime self.noctime = noctime @@ -533,8 +533,7 @@ class Archive: def _load_meta(self, id): cdata = self.repository.get(id) _, data = self.repo_objs.parse(id, cdata) - # we do not require TAM for archives, otherwise we can not even borg list a repo with old archives. - archive, self.tam_verified, _ = self.key.unpack_and_verify_archive(data, force_tam_not_required=True) + archive, _ = self.key.unpack_and_verify_archive(data) metadata = ArchiveItem(internal_dict=archive) if metadata.version not in (1, 2): # legacy: still need to read v1 archives raise Exception("Unknown archive metadata version") @@ -1998,7 +1997,7 @@ class ArchiveChecker: # **after** doing the low-level checks and having a strong indication that we # are likely looking at an archive item here, also check the TAM authentication: try: - archive, verified, _ = self.key.unpack_and_verify_archive(data, force_tam_not_required=False) + archive, _ = self.key.unpack_and_verify_archive(data) except IntegrityError as integrity_error: # TAM issues - do not accept this archive! # either somebody is trying to attack us with a fake archive data or @@ -2269,7 +2268,7 @@ class ArchiveChecker: del self.manifest.archives[info.name] continue try: - archive, verified, salt = self.key.unpack_and_verify_archive(data, force_tam_not_required=False) + archive, salt = self.key.unpack_and_verify_archive(data) except IntegrityError as integrity_error: # looks like there is a TAM issue with this archive, this might be an attack! # when upgrading to borg 1.2.5, users are expected to TAM-authenticate all archives they diff --git a/src/borg/cache.py b/src/borg/cache.py index fc9fd1851..1f9e77521 100644 --- a/src/borg/cache.py +++ b/src/borg/cache.py @@ -755,7 +755,7 @@ class LocalCache(CacheStatsMixin): nonlocal processed_item_metadata_chunks csize, data = decrypted_repository.get(archive_id) chunk_idx.add(archive_id, 1, len(data)) - archive, verified, _ = self.key.unpack_and_verify_archive(data, force_tam_not_required=True) + archive, _ = self.key.unpack_and_verify_archive(data) archive = ArchiveItem(internal_dict=archive) if archive.version not in (1, 2): # legacy raise Exception("Unknown archive metadata version") diff --git a/src/borg/crypto/key.py b/src/borg/crypto/key.py index d6d6b32e5..ef9f86efa 100644 --- a/src/borg/crypto/key.py +++ b/src/borg/crypto/key.py @@ -15,7 +15,7 @@ import argon2.low_level from ..constants import * # NOQA from ..helpers import StableDict from ..helpers import Error, IntegrityError -from ..helpers import get_keys_dir, get_security_dir +from ..helpers import get_keys_dir from ..helpers import get_limited_unpacker from ..helpers import bin_to_hex from ..helpers.passphrase import Passphrase, PasswordRetriesExceeded, PassphraseWrong @@ -276,37 +276,21 @@ class KeyBase: logger.debug("TAM-verified manifest") return unpacked - def unpack_and_verify_archive(self, data, force_tam_not_required=False): - """Unpack msgpacked *data* and return (object, did_verify).""" - tam_required = self.tam_required - if force_tam_not_required and tam_required: - # for a long time, borg only checked manifest for "tam_required" and - # people might have archives without TAM, so don't be too annoyingly loud here: - logger.debug("Archive authentication DISABLED.") - tam_required = False + def unpack_and_verify_archive(self, data): + """Unpack msgpacked *data* and return (object, salt).""" data = bytearray(data) unpacker = get_limited_unpacker("archive") unpacker.feed(data) unpacked = unpacker.unpack() if "tam" not in unpacked: - if tam_required: - archive_name = unpacked.get("name", "") - raise ArchiveTAMRequiredError(archive_name) - else: - logger.debug("Archive TAM not found and not required") - return unpacked, False, None + archive_name = unpacked.get("name", "") + raise ArchiveTAMRequiredError(archive_name) tam = unpacked.pop("tam", None) if not isinstance(tam, dict): raise ArchiveTAMInvalid() tam_type = tam.get("type", "") if tam_type != "HKDF_HMAC_SHA512": - if tam_required: - raise TAMUnsupportedSuiteError(repr(tam_type)) - else: - logger.debug( - "Ignoring archive TAM made with unsupported suite, since TAM is not required: %r", tam_type - ) - return unpacked, False, None + raise TAMUnsupportedSuiteError(repr(tam_type)) tam_hmac = tam.get("hmac") tam_salt = tam.get("salt") if not isinstance(tam_salt, (bytes, str)) or not isinstance(tam_hmac, (bytes, str)): @@ -320,7 +304,7 @@ class KeyBase: if not hmac.compare_digest(calculated_hmac, tam_hmac): raise ArchiveTAMInvalid() logger.debug("TAM-verified archive") - return unpacked, True, tam_salt + return unpacked, tam_salt class PlaintextKey(KeyBase): diff --git a/src/borg/testsuite/archiver/checks.py b/src/borg/testsuite/archiver/checks.py index 766ac8602..bb8ca65e9 100644 --- a/src/borg/testsuite/archiver/checks.py +++ b/src/borg/testsuite/archiver/checks.py @@ -425,9 +425,6 @@ def test_check_rebuild_refcounts(archiver): repository = Repository(archiver.repository_path, exclusive=True) with repository: write_archive_without_tam(repository, "archive_no_tam") - output = cmd(archiver, "rlist", "--format='{name} tam:{tam}{NL}'") - assert "archive_tam tam:verified" in output # good - assert "archive_no_tam tam:none" in output # could be borg < 1.0.9 archive or fake cmd(archiver, "check", "--repair") output = cmd(archiver, "rlist", "--format='{name} tam:{tam}{NL}'") assert "archive_tam tam:verified" in output # TAM-verified archive still there diff --git a/src/borg/testsuite/key.py b/src/borg/testsuite/key.py index a8fd37681..749c792e4 100644 --- a/src/borg/testsuite/key.py +++ b/src/borg/testsuite/key.py @@ -352,8 +352,7 @@ class TestTAM: unpacked = msgpack.unpackb(blob) assert unpacked["tam"]["type"] == "HKDF_HMAC_SHA512" - unpacked, verified, _ = key.unpack_and_verify_archive(blob) - assert verified + unpacked, _ = key.unpack_and_verify_archive(blob) assert unpacked["foo"] == "bar" assert "tam" not in unpacked From a0f5264cbd8e328692cc0033a8f73cccad9d2328 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Sun, 3 Sep 2023 22:27:24 +0200 Subject: [PATCH 3/3] rlist: remove support for {tam} placeholder archives are now always TAM-authenticated. --- src/borg/archive.py | 1 - src/borg/helpers/parseformat.py | 4 ---- src/borg/testsuite/archiver/checks.py | 8 ++++---- 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/borg/archive.py b/src/borg/archive.py index 3ce5e62c2..eb470f960 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -493,7 +493,6 @@ class Archive: self.name = name # overwritten later with name from archive metadata self.name_in_manifest = name # can differ from .name later (if borg check fixed duplicate archive names) self.comment = None - self.tam_verified = True self.numeric_ids = numeric_ids self.noatime = noatime self.noctime = noctime diff --git a/src/borg/helpers/parseformat.py b/src/borg/helpers/parseformat.py index 65d7ac7f3..b4a455a04 100644 --- a/src/borg/helpers/parseformat.py +++ b/src/borg/helpers/parseformat.py @@ -751,7 +751,6 @@ class ArchiveFormatter(BaseFormatter): "username": partial(self.get_meta, "username", ""), "comment": partial(self.get_meta, "comment", ""), "command_line": partial(self.get_meta, "command_line", ""), - "tam": self.get_tam, "size": partial(self.get_meta, "size", 0), "nfiles": partial(self.get_meta, "nfiles", 0), "end": self.get_ts_end, @@ -797,9 +796,6 @@ class ArchiveFormatter(BaseFormatter): def get_ts_end(self): return self.format_time(self.archive.ts_end) - def get_tam(self): - return "verified" if self.archive.tam_verified else "none" - def format_time(self, ts): return OutputTimestamp(ts) diff --git a/src/borg/testsuite/archiver/checks.py b/src/borg/testsuite/archiver/checks.py index bb8ca65e9..f6311c8e3 100644 --- a/src/borg/testsuite/archiver/checks.py +++ b/src/borg/testsuite/archiver/checks.py @@ -413,8 +413,8 @@ def test_check_rebuild_manifest(archiver): repository.delete(Manifest.MANIFEST_ID) # kill manifest, so check has to rebuild it repository.commit(compact=False) cmd(archiver, "check", "--repair") - output = cmd(archiver, "rlist", "--format='{name} tam:{tam}{NL}'") - assert "archive_tam tam:verified" in output # TAM-verified archive is in rebuilt manifest + output = cmd(archiver, "rlist", "--format='{name}{NL}'") + assert "archive_tam" in output # TAM-verified archive is in rebuilt manifest assert "archive_no_tam" not in output # check got rid of untrusted not TAM-verified archive @@ -426,8 +426,8 @@ def test_check_rebuild_refcounts(archiver): with repository: write_archive_without_tam(repository, "archive_no_tam") cmd(archiver, "check", "--repair") - output = cmd(archiver, "rlist", "--format='{name} tam:{tam}{NL}'") - assert "archive_tam tam:verified" in output # TAM-verified archive still there + output = cmd(archiver, "rlist", "--format='{name}{NL}'") + assert "archive_tam" in output # TAM-verified archive still there assert "archive_no_tam" not in output # check got rid of untrusted not TAM-verified archive archive_id_post_check = cmd(archiver, "rlist", "--format='{name} {id}{NL}'") assert archive_id_post_check == archive_id_pre_check # rebuild_refcounts didn't change archive_tam archive id