From 0ac568e290518c3bbe781459978720dde7fab224 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Wed, 24 Jun 2026 13:57:15 +0200 Subject: [PATCH 1/7] extract: security fix: refuse unsafe parent paths (symlinked dir or embedded "..") CVE-2026-62268 Fix 1/2 A maliciously crafted archive could contain a symlink (e.g. "evil" -> "/etc") followed by an item below it ("evil/passwd"), or an item whose path embeds ".." (e.g. "a/../../etc/passwd"). When extracting such items, borg followed the symlink / resolved the ".." in both the "remove existing file" step and the open()/mkdir()/ symlink() step, so it could delete or overwrite files outside the extraction directory. Note: sounds bad, but no big issue in practice: to create such a malicious archive, the attacker would need repository access and (for encrypted or authenticated repos) also the borg key and passphrase. borg create never produces such archives (it does not follow symlinks and stores normalized relative paths), so extract_item() now verifies, before any destructive operation, that the item's parent path contains no ".." and no symlinked (or other non-directory) component. Unsafe items are skipped with a warning (EXIT_WARNING) and extraction continues. Verified-safe parent directories are cached per extraction (Archive.safe_dirs) so each directory is lstat'd at most once. Added regression tests for both attack vectors and a benign deep-tree extraction. Also: skip redundant make_parent stat using safe_dirs cache Performance optimization building on the parent-path security fix: the parent path guard in extract_item already lstat's every existing parent directory and records it in Archive.safe_dirs. make_parent therefore no longer needs to call os.path.exists() for a parent that is already known to be a real directory - it returns early on a safe_dirs hit, and caches directories it has to create (and dest) so later items skip the stat too. This roughly halves the per-item directory-metadata syscalls in the common case (deep tree, shared parents) without changing behavior. Co-Authored-By: Claude Opus 4.8 --- src/borg/archive.py | 71 ++++++++++++++++++++++++++++++++ src/borg/testsuite/archiver.py | 75 ++++++++++++++++++++++++++++++++++ 2 files changed, 146 insertions(+) diff --git a/src/borg/archive.py b/src/borg/archive.py index 3e0f0f8710..4bccd58c20 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -183,6 +183,23 @@ class BackupError(Exception): """ +class BackupSymlinkParentError(BackupError): + """ + Refusing to extract an item below a symlinked (or otherwise non-directory) parent directory. + + This would write outside the extraction directory. borg create never produces such items, + so this can only come from a malicious or corrupted archive. + """ + + +class BackupPathTraversalError(BackupError): + """ + Refusing to extract an item whose path contains "..", which could escape the extraction directory. + + borg create never produces such items, so this can only come from a malicious or corrupted archive. + """ + + class BackupOSError(Exception): """ Wrapper for OSError raised while accessing backup files. @@ -444,6 +461,9 @@ def __init__(self, repository, key, manifest, name, cache=None, create=False, self.cache = cache self.manifest = manifest self.hard_links = {} + # cache of parent directory paths verified (during extraction) to be real + # directories and not symlinks, so we do not have to lstat them again and again. + self.safe_dirs = set() self.stats = Statistics(output_json=log_json, iec=iec) self.iec = iec self.show_progress = progress @@ -732,6 +752,44 @@ def add(id): stats.csize = self.metadata.csize return stats + def _check_safe_parent(self, archived_path): + """Refuse *archived_path* if its parent directory chain is not safe for extraction. + + *archived_path* is a path as stored in the archive, relative to self.cwd. As elsewhere + in borg, it is split on os.sep (the supported platforms are POSIX, where os.sep == "/"). + borg create never produces a path containing ".." or a path below a symlinked directory, + so such a path can only come from a malicious or corrupted archive and could be used to + access a location outside the extraction directory. + + Raises BackupPathTraversalError if a parent component is "..", or + BackupSymlinkParentError if an existing parent component is a symlink (or other + non-directory). Directories verified to be safe are cached in self.safe_dirs, so each + directory is only lstat'ed once per extraction. + """ + parent_components = [c for c in archived_path.split(os.sep)[:-1] if c not in ('', '.')] + # Reject ".." up front: this keeps the lstat walk below from climbing above self.cwd, + # and makes the early "break" on a not-yet-existing component safe (it can not skip a + # later ".."). + if '..' in parent_components: + raise BackupPathTraversalError('not extracted, path contains "../" (malicious or corrupted archive)') + # Every *existing* parent directory component must be a real directory, not a symlink. + current = self.cwd + for component in parent_components: + current = os.path.join(current, component) + if current in self.safe_dirs: + continue + try: + st = os.lstat(current) + except FileNotFoundError: + # parent does not exist yet, it will be created (as a real directory) below an + # already-verified-safe chain. + break + if not stat.S_ISDIR(st.st_mode): + # os.lstat does not follow symlinks, so a symlinked parent shows up here as a + # non-directory. Refuse to follow it out of the extraction directory. + raise BackupSymlinkParentError('not extracted, a parent directory is a symlink (malicious or corrupted archive)') + self.safe_dirs.add(current) # verified real directory, skip re-stat for later items + @contextmanager def extract_helper(self, dest, item, path, stripped_components, original_path, hardlink_masters): hardlink_set = False @@ -800,6 +858,12 @@ def extract_item(self, item, restore_attrs=True, dry_run=False, stdout=False, sp if item.path.startswith(('/', '../')): raise Exception('Path should be relative and local') path = os.path.join(dest, item.path) + # Refuse to extract items whose parent directory path is not safe (a symlinked parent or + # a path containing ".."). Without this check, the "remove existing file" block and the + # open()/mkdir()/symlink() calls below would follow such a parent path and could delete or + # overwrite files outside the extraction directory (e.g. /etc/passwd). + self.safe_dirs.discard(path) # path is about to be (re)created; never trust a stale entry for it + self._check_safe_parent(item.path) # Attempt to remove existing files, ignore errors on failure try: st = os.stat(path, follow_symlinks=False) @@ -814,8 +878,15 @@ def extract_item(self, item, restore_attrs=True, dry_run=False, stdout=False, sp def make_parent(path): parent_dir = os.path.dirname(path) + if parent_dir in self.safe_dirs: + # the parent path guard above already verified (this extraction) that parent_dir + # exists and is a real directory, so there is nothing to do and no need to stat it. + return if not os.path.exists(parent_dir): os.makedirs(parent_dir) + # remember parent_dir as a real directory (it either existed - e.g. dest - or we just + # created it below an already-verified-safe chain), so later items can skip the stat. + self.safe_dirs.add(parent_dir) mode = item.mode if stat.S_ISREG(mode): diff --git a/src/borg/testsuite/archiver.py b/src/borg/testsuite/archiver.py index 6af41a967f..382a85022b 100644 --- a/src/borg/testsuite/archiver.py +++ b/src/borg/testsuite/archiver.py @@ -522,6 +522,81 @@ def test_symlink_extract(self): self.cmd('extract', self.repository_location + '::test') assert os.readlink('input/link1') == 'somewhere' + def _create_malicious_archive(self, name, items): + # Inject raw Items directly into an archive, bypassing "borg create" (which would never + # produce a child below a symlink or a path with embedded ".."). + repository = Repository(self.repository_path, exclusive=True) + with repository: + manifest, key = Manifest.load(repository, Manifest.NO_OPERATION_CHECK) + with Cache(repository, key, manifest) as cache: + archive = Archive(repository, key, manifest, name, cache=cache, create=True) + for item in items: + archive.items_buffer.add(item) + archive.save() + + def test_extract_through_symlinked_parent(self): + # Security: an archive with a symlink "evil" -> "/etc", followed by a regular file + # "evil/passwd", must NOT overwrite/delete files in the symlink target directory. + if self.prefix: + pytest.skip('malicious archive is crafted via direct (local) repository access') + self.cmd('init', '--encryption=repokey', self.repository_location) + victim_dir = os.path.join(self.tmpdir, 'victim') + os.mkdir(victim_dir) + victim_file = os.path.join(victim_dir, 'passwd') + with open(victim_file, 'w') as fd: + fd.write('original') + attrs = dict(mtime=0, uid=0, gid=0, user='root', group='root') + self._create_malicious_archive('evil', [ + Item(path='evil', mode=stat.S_IFLNK | 0o777, source=victim_dir, **attrs), + Item(path='evil/passwd', mode=stat.S_IFREG | 0o644, chunks=[], **attrs), + ]) + with changedir('output'): + output = self.cmd('extract', self.repository_location + '::evil', exit_code=EXIT_WARNING) + assert 'parent directory is a symlink' in output + # the out-of-tree victim must be untouched (neither deleted nor overwritten) + assert os.path.exists(victim_file) + with open(victim_file) as fd: + assert fd.read() == 'original' + # the symlink item itself was restored faithfully + assert os.path.islink(os.path.join(self.output_path, 'evil')) + + def test_extract_path_with_embedded_dotdot(self): + # Security: a regular file with an embedded ".." (e.g. a/../../victim/passwd) must not + # be extracted outside the destination directory. + if self.prefix: + pytest.skip('malicious archive is crafted via direct (local) repository access') + self.cmd('init', '--encryption=repokey', self.repository_location) + victim_dir = os.path.join(self.tmpdir, 'victim') + os.mkdir(victim_dir) + victim_file = os.path.join(victim_dir, 'passwd') + with open(victim_file, 'w') as fd: + fd.write('original') + attrs = dict(mtime=0, uid=0, gid=0, user='root', group='root') + # from inside the output dir, "a/../../victim/passwd" resolves to /victim/passwd + self._create_malicious_archive('evil', [ + Item(path='a/../../victim/passwd', mode=stat.S_IFREG | 0o644, chunks=[], **attrs), + ]) + with changedir('output'): + output = self.cmd('extract', self.repository_location + '::evil', exit_code=EXIT_WARNING) + assert 'path contains "../"' in output + assert os.path.exists(victim_file) + with open(victim_file) as fd: + assert fd.read() == 'original' + + def test_extract_deep_tree_ok(self): + # The parent-path guard (and its safe_dirs cache) must not break normal extraction of a + # deep tree where many files share the same parent directories. + self.cmd('init', '--encryption=repokey', self.repository_location) + self.create_regular_file('d1/d2/d3/file1', size=10) + self.create_regular_file('d1/d2/d3/file2', size=10) + self.create_regular_file('d1/d2/other', size=10) + self.cmd('create', self.repository_location + '::test', 'input') + with changedir('output'): + self.cmd('extract', self.repository_location + '::test') + assert os.path.exists('input/d1/d2/d3/file1') + assert os.path.exists('input/d1/d2/d3/file2') + assert os.path.exists('input/d1/d2/other') + @pytest.mark.skipif(not is_utime_fully_supported(), reason='cannot properly setup and execute test without utime') def test_directory_timestamps1(self): self.create_test_files() From d907f2cb57337041c4fa55ca4682895535e5076b Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Thu, 25 Jun 2026 15:00:28 +0200 Subject: [PATCH 2/7] extract: security fix: refuse hardlinks with an unsafe source path CVE-2026-62268 Fix 2/2 The hardlink source (item.source) was used unvalidated as the os.link() target during extraction. A maliciously crafted archive could set a source that traverses a symlink (e.g. "evil/secret" with "evil" -> "/etc") or contains "..", causing borg to hardlink an arbitrary external file (e.g. /etc/shadow) into the extracted tree - an information disclosure, especially dangerous when restoring as root. Note: sounds bad, but no big issue in practice: to create such a malicious archive, the attacker would need repository access and (for encrypted or authenticated repos) also the borg key and passphrase. extract_helper now validates the hardlink source path with the same Archive._check_safe_parent() check used for item paths, refusing unsafe ones with a warning (and continuing with the rest of the archive). os.link() is now called with follow_symlinks=False (where supported) so a symlinked final source component links the symlink itself - a faithful restore - rather than the external file it targets. Adds BackupHardlinkSourceError and regression tests for both the symlinked-source and embedded-".." source vectors. Co-Authored-By: Claude Opus 4.8 --- src/borg/archive.py | 29 ++++++++++++++++++++--- src/borg/testsuite/archiver.py | 43 ++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 3 deletions(-) diff --git a/src/borg/archive.py b/src/borg/archive.py index 4bccd58c20..47ddbab465 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -200,6 +200,15 @@ class BackupPathTraversalError(BackupError): """ +class BackupHardlinkSourceError(BackupError): + """ + Refusing to create a hardlink whose source path contains ".." or is below a symlinked directory. + + Such a source could point outside the extraction directory. borg create never produces such + items, so this can only come from a malicious or corrupted archive. + """ + + class BackupOSError(Exception): """ Wrapper for OSError raised while accessing backup files. @@ -795,12 +804,26 @@ def extract_helper(self, dest, item, path, stripped_components, original_path, h hardlink_set = False # Hard link? if 'source' in item: - source = os.path.join(dest, *item.source.split(os.sep)[stripped_components:]) + # item.source is attacker-controlled and used as the hardlink target below. Refuse + # to follow a symlinked parent or ".." out of the extraction directory - otherwise a + # crafted archive could os.link() an arbitrary external file (e.g. /etc/shadow) into + # the extracted tree. Split on os.sep, consistent with the rest of borg. + source_components = item.source.split(os.sep)[stripped_components:] + try: + self._check_safe_parent(os.sep.join(source_components)) + except BackupError: + raise BackupHardlinkSourceError('not extracted, hardlink source path is unsafe (malicious or corrupted archive)') from None + source = os.path.join(dest, *source_components) chunks, link_target = hardlink_masters.get(item.source, (None, source)) if link_target and has_link: - # Hard link was extracted previously, just link + # Hard link was extracted previously, just link. + # follow_symlinks=False: if the final source component is a symlink, link the + # symlink itself (a faithful restore) rather than the external file it targets. with backup_io('link'): - os.link(link_target, path) + if os.link in os.supports_follow_symlinks: + os.link(link_target, path, follow_symlinks=False) + else: + os.link(link_target, path) hardlink_set = True elif chunks is not None: # assign chunks to this item, since the item which had the chunks was not extracted diff --git a/src/borg/testsuite/archiver.py b/src/borg/testsuite/archiver.py index 382a85022b..9266e8b2bc 100644 --- a/src/borg/testsuite/archiver.py +++ b/src/borg/testsuite/archiver.py @@ -583,6 +583,49 @@ def test_extract_path_with_embedded_dotdot(self): with open(victim_file) as fd: assert fd.read() == 'original' + def test_extract_hardlink_through_symlinked_source(self): + # Security: a hardlink whose source traverses a symlink (here "evil" -> victim dir, + # source "evil/secret") must NOT link an external file into the extracted tree. + if self.prefix: + pytest.skip('malicious archive is crafted via direct (local) repository access') + self.cmd('init', '--encryption=repokey', self.repository_location) + victim_dir = os.path.join(self.tmpdir, 'victim') + os.mkdir(victim_dir) + secret = os.path.join(victim_dir, 'secret') + with open(secret, 'w') as fd: + fd.write('secret') + attrs = dict(mtime=0, uid=0, gid=0, user='root', group='root') + self._create_malicious_archive('evil', [ + Item(path='evil', mode=stat.S_IFLNK | 0o777, source=victim_dir, **attrs), + Item(path='pwned', mode=stat.S_IFREG | 0o644, source='evil/secret', **attrs), + ]) + with changedir('output'): + output = self.cmd('extract', self.repository_location + '::evil', exit_code=EXIT_WARNING) + assert 'hardlink source path is unsafe' in output + # no hardlink to the external file was created + assert not os.path.exists(os.path.join(self.output_path, 'pwned')) + with open(secret) as fd: + assert fd.read() == 'secret' + + def test_extract_hardlink_source_with_embedded_dotdot(self): + # Security: a hardlink whose source contains ".." must not be linked. + if self.prefix: + pytest.skip('malicious archive is crafted via direct (local) repository access') + self.cmd('init', '--encryption=repokey', self.repository_location) + victim_dir = os.path.join(self.tmpdir, 'victim') + os.mkdir(victim_dir) + secret = os.path.join(victim_dir, 'secret') + with open(secret, 'w') as fd: + fd.write('secret') + attrs = dict(mtime=0, uid=0, gid=0, user='root', group='root') + self._create_malicious_archive('evil', [ + Item(path='pwned', mode=stat.S_IFREG | 0o644, source='a/../../victim/secret', **attrs), + ]) + with changedir('output'): + output = self.cmd('extract', self.repository_location + '::evil', exit_code=EXIT_WARNING) + assert 'hardlink source path is unsafe' in output + assert not os.path.exists(os.path.join(self.output_path, 'pwned')) + def test_extract_deep_tree_ok(self): # The parent-path guard (and its safe_dirs cache) must not break normal extraction of a # deep tree where many files share the same parent directories. From fd05aa95c7efdd65e301bcf4466c00d0ffda5d9f Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Sun, 19 Jul 2026 14:54:44 +0200 Subject: [PATCH 3/7] create: don't wrap repository writes in backup_io('read') process_file() ran process_file_chunks() inside `with backup_io('read')`. That block is meant to guard reading the *source* file, but the source reads are already guarded individually by backup_io_iter(). The outer wrapper also caught add_chunk()'s (and maybe_checkpoint()'s) *repository* writes, so a repository IO failure -- e.g. the repo running out of space -- was wrapped into a per-file BackupOSError tagged "read". Borg then emitted misleading ": read: [Errno 28] No space left on device" warnings, pointlessly retried the file, and only treated a critical repository error as a non-critical per-file one, contrary to the BackupOSError docstring ("Any unwrapped IO error is critical and aborts execution (for example repository IO failure)"). In 1.4 the transactional repository still rolls the partial transaction back on the eventual failure, so this is not data loss here (unlike borg2, where it silently commits a corrupt archive). But the misclassification -- wrong warning text and needless read-retries of a repository-full condition -- is wrong regardless. Drop the outer backup_io('read') wrapper. Source reads stay per-file warnings (backup_io_iter is unchanged); repository OSErrors are now left unwrapped and critical, aborting promptly with the correct error. Verified on a space-limited macOS ramdisk: before, create emitted many "read: [Errno 28]" retry warnings then rolled back; after, it aborts immediately ("No space left on device, cleaning up partial transaction"), commits no archive, and the repo stays consistent. Normal backups and unreadable-source-file handling (per-file warning) are unchanged. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_013bFWgq2KV6oYxmfH6Zqvp3 --- src/borg/archive.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/borg/archive.py b/src/borg/archive.py index 47ddbab465..fc6d515b63 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -1545,8 +1545,16 @@ def process_file(self, *, path, parent_fd, name, st, cache, flags=flags_normal): if chunks is not None: item.chunks = chunks else: - with backup_io('read'): - self.process_file_chunks(item, cache, self.stats, self.show_progress, backup_io_iter(self.chunker.chunkify(None, fd))) + # Do NOT wrap this in backup_io('read'): the source-file reads are already + # guarded individually by backup_io_iter() below. Wrapping the whole call would + # also wrap add_chunk()'s (and maybe_checkpoint()'s) *repository* writes, so a + # repository IO failure (e.g. the repo running out of space) would be misreported + # as a per-file "read" error and only warned about (then the file skipped), + # instead of aborting. An unwrapped repository OSError is critical (see the + # BackupOSError docstring). In 1.4 the transactional repository still rolls the + # partial transaction back, so this is not data loss here, but the misclassified + # warning and pointless read-retries are wrong regardless. + self.process_file_chunks(item, cache, self.stats, self.show_progress, backup_io_iter(self.chunker.chunkify(None, fd))) if is_win32: changed_while_backup = False # TODO else: From 864d37158a0ff9aaf1f1e22cb0515cba8e360f82 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Fri, 24 Apr 2026 10:13:00 +0200 Subject: [PATCH 4/7] LRUCache fixes fix: resolve KeyError and memory leaks in LRUCache - __setitem__: assign value before popping from _lru to avoid KeyError when exceeding capacity. - clear(): clear the _lru list also to prevent stale keys causing KeyErrors during future evictions. (cherry picked from commit 32cfddc34ca0b6bcaa80646eaba1e8866c258fc5) --- src/borg/lrucache.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/borg/lrucache.py b/src/borg/lrucache.py index 4f7f1f8296..abab1a5738 100644 --- a/src/borg/lrucache.py +++ b/src/borg/lrucache.py @@ -12,10 +12,10 @@ def __setitem__(self, key, value): assert key not in self._cache, ( "Unexpected attempt to replace a cached item," " without first deleting the old item.") + self._cache[key] = value self._lru.append(key) while len(self._lru) > self._capacity: del self[self._lru[0]] - self._cache[key] = value def __getitem__(self, key): value = self._cache[key] # raise KeyError if not found @@ -49,6 +49,7 @@ def clear(self): for value in self._cache.values(): self._dispose(value) self._cache.clear() + self._lru.clear() def items(self): return self._cache.items() From 1b5ca69928ca64b9935edf8207e931ff93b64147 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Mon, 20 Apr 2026 03:37:28 +0200 Subject: [PATCH 5/7] _chunker.c: fix some bugs - better check return value of fd.read(n) and reject if it returns more bytes than requested. - avoid giving len<=0 to posix_fadvise(), which could drop the rest of the file from cache. - buzhash: check for len == 0 edge case - correctly Py_DECREF in cases of errors - check for malloc/calloc failures (cherry picked from commit 37f66f11ab9122eb685101a2435a79303cb4d739) --- src/borg/_chunker.c | 35 +++++++++++++++++++++++++++++------ src/borg/chunker.pyx | 2 ++ 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/src/borg/_chunker.c b/src/borg/_chunker.c index e5d9b54f81..4c1756b868 100644 --- a/src/borg/_chunker.c +++ b/src/borg/_chunker.c @@ -87,6 +87,9 @@ buzhash(const unsigned char *data, size_t len, const uint32_t *h) { uint32_t i; uint32_t sum = 0, imod; + if (len == 0) { + return 0; + } for(i = len - 1; i > 0; i--) { imod = i & 0x1f; @@ -118,12 +121,24 @@ static Chunker * chunker_init(size_t window_size, uint32_t chunk_mask, size_t min_size, size_t max_size, uint32_t seed) { Chunker *c = calloc(sizeof(Chunker), 1); + if(!c) { + return NULL; + } c->window_size = window_size; c->chunk_mask = chunk_mask; c->min_size = min_size; c->table = buzhash_init_table(seed); + if(!c->table) { + free(c); + return NULL; + } c->buf_size = max_size; c->data = malloc(c->buf_size); + if(!c->data) { + free(c->table); + free(c); + return NULL; + } c->fh = -1; return c; } @@ -219,7 +234,9 @@ chunker_fill(Chunker *c) overshoot = 0; } - posix_fadvise(c->fh, offset & ~pagemask, length - overshoot, POSIX_FADV_DONTNEED); + if (length - overshoot > 0 || length == 0) { + posix_fadvise(c->fh, offset & ~pagemask, length - overshoot, POSIX_FADV_DONTNEED); + } #endif PyEval_RestoreThread(thread_state); @@ -230,15 +247,21 @@ chunker_fill(Chunker *c) if(!data) { return 0; } - n = PyBytes_Size(data); + ssize_t read_bytes = PyBytes_Size(data); if(PyErr_Occurred()) { // we wanted bytes(), but got something else + Py_DECREF(data); return 0; } - if(n) { - memcpy(c->data + c->position + c->remaining, PyBytes_AsString(data), n); - c->remaining += n; - c->bytes_read += n; + if(read_bytes > n) { + Py_DECREF(data); + PyErr_SetString(PyExc_ValueError, "read() returned too many bytes"); + return 0; + } + if(read_bytes) { + memcpy(c->data + c->position + c->remaining, PyBytes_AsString(data), read_bytes); + c->remaining += read_bytes; + c->bytes_read += read_bytes; } else { c->eof = 1; diff --git a/src/borg/chunker.pyx b/src/borg/chunker.pyx index ee9773be4c..ba00c3c24c 100644 --- a/src/borg/chunker.pyx +++ b/src/borg/chunker.pyx @@ -247,6 +247,8 @@ cdef class Chunker: assert hash_window_size + min_size + 1 <= max_size, "too small max_size" hash_mask = (1 << hash_mask_bits) - 1 self.chunker = chunker_init(hash_window_size, hash_mask, min_size, max_size, seed & 0xffffffff) + if not self.chunker: + raise MemoryError('chunker_init failed') def chunkify(self, fd, fh=-1): """ From 1e805c3b35ccfe6fa88467ca84269b8c7e65bd92 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Tue, 14 Apr 2026 01:30:54 +0200 Subject: [PATCH 6/7] _hashindex.c: more fixes - avoid buckets_length integer overflow on 32bit systems via huge num_buckets - always initialize index-> min_empty and num_empty - correctly free memory when header validation fails. this is a minor issue, because borg will terminate in that case anyway. - make it possible to lookup in compacted hashtables - deal safely with empty index: we must use num_buckets = 1 to avoid division by zero and sanity check in hashindex_read. - reinitialize upper/lower limit and min_empty after compact - fix size_idx / fit_size / grow_size / shrink_size (mind array bounds) - deal with growing when already at max capacity - hashindex_resize: replace num_entries assertion, rather return error - BaseIndex.clear: always stay in valid state Do not free the old index before we successfully have allocated a new one. This is a minor issue as the Exception raised would terminate borg anyway. (cherry picked from commit cd2f5a0648faa89bd1727af8e61b6e313b7978bf) --- src/borg/_hashindex.c | 100 ++++++++++++++++++++++---------- src/borg/hashindex.pyx | 7 ++- src/borg/testsuite/hashindex.py | 3 +- 3 files changed, 74 insertions(+), 36 deletions(-) diff --git a/src/borg/_hashindex.c b/src/borg/_hashindex.c index a62955c298..96a73e87d5 100644 --- a/src/borg/_hashindex.c +++ b/src/borg/_hashindex.c @@ -178,14 +178,9 @@ hashindex_lookup(HashIndex *index, const unsigned char *key, int *start_idx) if (idx >= index->num_buckets) { /* triggers at == already */ idx = 0; } - /* When idx == start, we have done a full pass over all buckets. - * - We did not find a bucket with the key we searched for. - * - We did not find an empty bucket either. - * So all buckets are either full or deleted/tombstones. - * This is an invalid state we never should get into, see - * upper_limit and min_empty. - */ - assert(idx != start); + if (idx == start) { + break; + } } /* we get here if we did not find a bucket with the key we searched for. */ if (start_idx != NULL) { @@ -215,7 +210,12 @@ hashindex_resize(HashIndex *index, int capacity) return 0; } } - assert(index->num_entries == new->num_entries); + + if(index->num_entries != new->num_entries) { + /* This detects structurally corrupt indices maliciously supplied with wrong num_entries */ + hashindex_free(new); + return 0; + } hashindex_free_buckets(index); index->buckets = new->buckets; @@ -251,9 +251,15 @@ int get_min_empty(int num_buckets){ int size_idx(int size){ /* find the smallest hash_sizes index with entry >= size */ - int i = NELEMS(hash_sizes) - 1; + int i, max_idx = NELEMS(hash_sizes) - 1; + i = max_idx; while(i >= 0 && hash_sizes[i] >= size) i--; - return i + 1; + i++; + /* if size is larger than any hash_sizes entry, return the largest one */ + if (i > max_idx) { + return max_idx; + } + return i; } int fit_size(int current){ @@ -262,17 +268,17 @@ int fit_size(int current){ } int grow_size(int current){ + int max_idx = NELEMS(hash_sizes) - 1; int i = size_idx(current) + 1; - int elems = NELEMS(hash_sizes); - if (i >= elems) - return hash_sizes[elems - 1]; + if (i > max_idx) + i = max_idx; return hash_sizes[i]; } int shrink_size(int current){ int i = size_idx(current) - 1; if (i < 0) - return hash_sizes[0]; + i = 0; return hash_sizes[i]; } @@ -377,30 +383,35 @@ hashindex_read(PyObject *file_py, int permit_compact, int expected_key_size, int if (expected_key_size != -1 && header->key_size != expected_key_size) { PyErr_Format(PyExc_ValueError, "Expected key size %d, got %d.", expected_key_size, header->key_size); - goto fail_decref_header; + goto fail_release_header_buffer; } if (expected_value_size != -1 && header->value_size != expected_value_size) { PyErr_Format(PyExc_ValueError, "Expected value size %d, got %d.", expected_value_size, header->value_size); - goto fail_decref_header; + goto fail_release_header_buffer; } // in any case, the key and value sizes must be both >= 4, the code assumes this. if (header->key_size < 4) { PyErr_Format(PyExc_ValueError, "Expected key size >= 4, got %d.", header->key_size); - goto fail_decref_header; + goto fail_release_header_buffer; } if (header->value_size < 4) { PyErr_Format(PyExc_ValueError, "Expected value size >= 4, got %d.", header->value_size); - goto fail_decref_header; + goto fail_release_header_buffer; } // sanity check for num_buckets and num_entries. if (header_num_buckets < 1) { PyErr_Format(PyExc_ValueError, "Expected num_buckets >= 1, got %d.", header_num_buckets); - goto fail_decref_header; + goto fail_release_header_buffer; } if ((header_num_entries < 0) || (header_num_entries > header_num_buckets)) { PyErr_Format(PyExc_ValueError, "Expected 0 <= num_entries <= num_buckets, got %d.", header_num_entries); - goto fail_decref_header; + goto fail_release_header_buffer; + } + + if ((Py_ssize_t)header_num_buckets > (PY_SSIZE_T_MAX - (Py_ssize_t)sizeof(HashHeader)) / (header->key_size + header->value_size)) { + PyErr_Format(PyExc_ValueError, "Hash index size overflows Py_ssize_t."); + goto fail_release_header_buffer; } buckets_length = (Py_ssize_t)header_num_buckets * (header->key_size + header->value_size); @@ -448,8 +459,9 @@ hashindex_read(PyObject *file_py, int permit_compact, int expected_key_size, int } index->buckets = index->buckets_buffer.buf; + index->min_empty = get_min_empty(index->num_buckets); + if(!permit_compact) { - index->min_empty = get_min_empty(index->num_buckets); index->num_empty = count_empty(index); if(index->num_empty < index->min_empty) { @@ -459,6 +471,8 @@ hashindex_read(PyObject *file_py, int permit_compact, int expected_key_size, int goto fail_free_buckets; } } + } else { + index->num_empty = index->num_buckets - index->num_entries; /* upper bound */ } /* @@ -612,16 +626,22 @@ hashindex_set(HashIndex *index, const unsigned char *key, const void *value) if(idx < 0) { if(index->num_entries > index->upper_limit) { - /* hashtable too full, grow it! */ - if(!hashindex_resize(index, grow_size(index->num_buckets))) { + int next_size = grow_size(index->num_buckets); + if(next_size != index->num_buckets) { + /* hashtable too full, grow it! */ + if(!hashindex_resize(index, next_size)) { + return 0; + } + /* we have just built a fresh hashtable and removed all tombstones, + * so we only have EMPTY or USED buckets, but no DELETED ones any more. + */ + idx = hashindex_lookup(index, key, &start_idx); + assert(idx < 0); + assert(BUCKET_IS_EMPTY(index, start_idx)); + } else if (index->num_entries >= index->num_buckets) { + /* Table is maximally sized and 100% full, cannot insert cleanly. */ return 0; } - /* we have just built a fresh hashtable and removed all tombstones, - * so we only have EMPTY or USED buckets, but no DELETED ones any more. - */ - idx = hashindex_lookup(index, key, &start_idx); - assert(idx < 0); - assert(BUCKET_IS_EMPTY(index, start_idx)); } idx = start_idx; if(BUCKET_IS_EMPTY(index, idx)){ @@ -699,13 +719,25 @@ hashindex_compact(HashIndex *index) int begin_used_idx; int empty_slot_count, count, buckets_to_copy; int compact_tail_idx = 0; - uint64_t saved_size = (index->num_buckets - index->num_entries) * (uint64_t)index->bucket_size; + int new_num_buckets = index->num_entries == 0 ? 1 : index->num_entries; + uint64_t saved_size = (index->num_buckets - new_num_buckets) * (uint64_t)index->bucket_size; - if(index->num_buckets - index->num_entries == 0) { + if(index->num_buckets == new_num_buckets) { /* already compact */ return 0; } + if (index->num_entries == 0) { + index->num_buckets = 1; + memset(BUCKET_ADDR(index, 0), 0, index->bucket_size); + BUCKET_MARK_EMPTY(index, 0); + index->lower_limit = get_lower_limit(index->num_buckets); + index->upper_limit = get_upper_limit(index->num_buckets); + index->min_empty = get_min_empty(index->num_buckets); + index->num_empty = 1; + return saved_size; + } + while(idx < index->num_buckets) { /* Phase 1: Find some empty slots */ start_idx = idx; @@ -744,6 +776,10 @@ hashindex_compact(HashIndex *index) } index->num_buckets = index->num_entries; + index->lower_limit = get_lower_limit(index->num_buckets); + index->upper_limit = get_upper_limit(index->num_buckets); + index->min_empty = get_min_empty(index->num_buckets); + index->num_empty = 0; return saved_size; } diff --git a/src/borg/hashindex.pyx b/src/borg/hashindex.pyx index cf86993351..796cd7ed95 100644 --- a/src/borg/hashindex.pyx +++ b/src/borg/hashindex.pyx @@ -122,10 +122,11 @@ cdef class IndexBase: hashindex_write(self.index, path) def clear(self): - hashindex_free(self.index) - self.index = hashindex_init(0, self.key_size, self.value_size) - if not self.index: + new_index = hashindex_init(0, self.key_size, self.value_size) + if not new_index: raise Exception('hashindex_init failed') + hashindex_free(self.index) + self.index = new_index def setdefault(self, key, value): if not key in self: diff --git a/src/borg/testsuite/hashindex.py b/src/borg/testsuite/hashindex.py index eb94e17a4a..2750b93c7e 100644 --- a/src/borg/testsuite/hashindex.py +++ b/src/borg/testsuite/hashindex.py @@ -493,7 +493,8 @@ def test_empty(self): compact_index = self.index_from_data_compact_to_data() - self.index(num_entries=0, num_buckets=0) + self.index(num_entries=0, num_buckets=1) + self.write_empty(b'\0' * 32) assert compact_index == self.index_data.getvalue() def test_merge(self): From c2d08441cfcc610856017ead668608e31cbcd780 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Fri, 24 Apr 2026 02:20:46 +0200 Subject: [PATCH 7/7] crypto low_level: fix freeing of memory The previous code performed allocations and buffer acquisitions before the `try` block. If a later allocation or buffer acquisition failed, execution did not enter the `finally` block, so resources acquired earlier in the setup path could leak. Move allocation and buffer acquisition into the guarded block, initialize raw output pointers to `NULL`, and only call `PyMem_Free` or `PyBuffer_Release` for resources that were actually acquired. (cherry picked from commit 2d7d5f2482e4ee24b3f0b0be0f3918480ef093c3) --- src/borg/crypto/low_level.pyx | 96 +++++++++++++++++++++++++---------- 1 file changed, 68 insertions(+), 28 deletions(-) diff --git a/src/borg/crypto/low_level.pyx b/src/borg/crypto/low_level.pyx index 158a0af4a7..ec272cf54a 100644 --- a/src/borg/crypto/low_level.pyx +++ b/src/borg/crypto/low_level.pyx @@ -253,15 +253,25 @@ cdef class AES256_CTR_BASE: assert hlen == self.header_len cdef int aoffset = self.aad_offset cdef int alen = hlen - aoffset - cdef unsigned char *odata = PyMem_Malloc(hlen + self.mac_len + self.iv_len_short + - ilen + self.cipher_blk_len) # play safe, 1 extra blk - if not odata: - raise MemoryError + cdef Py_buffer idata + cdef bint idata_acquired = False + cdef Py_buffer hdata + cdef bint hdata_acquired = False + cdef unsigned char *odata = NULL cdef int olen cdef int offset - cdef Py_buffer idata = ro_buffer(data) - cdef Py_buffer hdata = ro_buffer(header) + try: + odata = PyMem_Malloc(hlen + self.mac_len + self.iv_len_short + + ilen + self.cipher_blk_len) # play safe, 1 extra blk + if not odata: + raise MemoryError + + idata = ro_buffer(data) + idata_acquired = True + hdata = ro_buffer(header) + hdata_acquired = True + offset = 0 for i in range(hlen): odata[offset+i] = header[i] @@ -286,9 +296,12 @@ cdef class AES256_CTR_BASE: self.blocks += self.block_count(ilen) return odata[:offset] finally: - PyMem_Free(odata) - PyBuffer_Release(&hdata) - PyBuffer_Release(&idata) + if odata: + PyMem_Free(odata) + if hdata_acquired: + PyBuffer_Release(&hdata) + if idata_acquired: + PyBuffer_Release(&idata) def decrypt(self, envelope): """ @@ -299,15 +312,22 @@ cdef class AES256_CTR_BASE: assert hlen == self.header_len cdef int aoffset = self.aad_offset cdef int alen = hlen - aoffset - cdef unsigned char *odata = PyMem_Malloc(ilen + self.cipher_blk_len) # play safe, 1 extra blk - if not odata: - raise MemoryError + cdef Py_buffer idata + cdef bint idata_acquired = False + cdef unsigned char *odata = NULL cdef int olen cdef int offset cdef unsigned char mac_buf[32] assert sizeof(mac_buf) == self.mac_len - cdef Py_buffer idata = ro_buffer(envelope) + try: + odata = PyMem_Malloc(ilen + self.cipher_blk_len) # play safe, 1 extra blk + if not odata: + raise MemoryError + + idata = ro_buffer(envelope) + idata_acquired = True + self.mac_verify( idata.buf+aoffset, alen, idata.buf+hlen+self.mac_len, ilen-hlen-self.mac_len, mac_buf, idata.buf+hlen) @@ -329,8 +349,10 @@ cdef class AES256_CTR_BASE: self.blocks += self.block_count(offset) return odata[:offset] finally: - PyMem_Free(odata) - PyBuffer_Release(&idata) + if odata: + PyMem_Free(odata) + if idata_acquired: + PyBuffer_Release(&idata) def block_count(self, length): return num_cipher_blocks(length, self.cipher_blk_len) @@ -463,14 +485,21 @@ cdef class AES: if iv is not None: self.set_iv(iv) assert self.blocks == 0, 'iv needs to be set before encrypt is called' - cdef Py_buffer idata = ro_buffer(data) + cdef Py_buffer idata + cdef bint idata_acquired = False + cdef unsigned char *odata = NULL cdef int ilen = len(data) - cdef int offset cdef int olen = 0 - cdef unsigned char *odata = PyMem_Malloc(ilen + self.cipher_blk_len) - if not odata: - raise MemoryError + cdef int offset + try: + odata = PyMem_Malloc(ilen + self.cipher_blk_len) + if not odata: + raise MemoryError + + idata = ro_buffer(data) + idata_acquired = True + if not EVP_EncryptInit_ex(self.ctx, self.cipher(), NULL, self.enc_key, self.iv): raise Exception('EVP_EncryptInit_ex failed') offset = 0 @@ -483,18 +512,27 @@ cdef class AES: self.blocks = self.block_count(offset) return odata[:offset] finally: - PyMem_Free(odata) - PyBuffer_Release(&idata) + if odata: + PyMem_Free(odata) + if idata_acquired: + PyBuffer_Release(&idata) def decrypt(self, data): - cdef Py_buffer idata = ro_buffer(data) + cdef Py_buffer idata + cdef bint idata_acquired = False + cdef unsigned char *odata = NULL cdef int ilen = len(data) cdef int offset cdef int olen = 0 - cdef unsigned char *odata = PyMem_Malloc(ilen + self.cipher_blk_len) - if not odata: - raise MemoryError + try: + odata = PyMem_Malloc(ilen + self.cipher_blk_len) + if not odata: + raise MemoryError + + idata = ro_buffer(data) + idata_acquired = True + # Set cipher type and mode if not EVP_DecryptInit_ex(self.ctx, self.cipher(), NULL, self.enc_key, self.iv): raise Exception('EVP_DecryptInit_ex failed') @@ -511,8 +549,10 @@ cdef class AES: self.blocks = self.block_count(ilen) return odata[:offset] finally: - PyMem_Free(odata) - PyBuffer_Release(&idata) + if odata: + PyMem_Free(odata) + if idata_acquired: + PyBuffer_Release(&idata) def block_count(self, length): return num_cipher_blocks(length, self.cipher_blk_len)