From ac0c6e9dd31166c43fbca3ea29f44102b3591db0 Mon Sep 17 00:00:00 2001 From: liquidsec Date: Fri, 8 May 2026 16:35:26 -0400 Subject: [PATCH 1/2] fix gitdumper regex_files cwd-walk via mutable default args MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit regex_files defaulted folder=Path() and file=Path(); both evaluate as PosixPath('.') which is truthy AND a directory. Calling regex_files(file=foo) silently ALSO walked the entire current working directory, decoding every file into a Python string and running regex over it. With many CODE_REPOSITORY events this allocated 100+ GB (observed in production: 105 GB across 5 calls). Fix uses None defaults with explicit `is not None` checks. Also caps per-file regex scan at 10 MB — real git ref/object/info files are small; oversized usually means a webserver returned an HTML error page instead of the requested git path. Adds a regression test that fails on the old code (asserts a decoy file in cwd is NOT scanned when only file= is passed). --- bbot/modules/gitdumper.py | 43 ++++++++++++------- .../test_step_1/test_gitdumper_regex_files.py | 36 ++++++++++++++++ 2 files changed, 63 insertions(+), 16 deletions(-) create mode 100644 bbot/test/test_step_1/test_gitdumper_regex_files.py diff --git a/bbot/modules/gitdumper.py b/bbot/modules/gitdumper.py index 71ce02a338..bf10120d87 100644 --- a/bbot/modules/gitdumper.py +++ b/bbot/modules/gitdumper.py @@ -201,27 +201,38 @@ async def download_git_packs(self, url, folder): if url_list: await self.download_files(url_list, folder) - async def regex_files(self, regex, folder=Path(), file=Path(), files=[]): + # Skip regex scan for files larger than this — real git ref/object/info + # files are small; oversized is almost always a webserver returning HTML. + _regex_file_max_bytes = 10 * 1024 * 1024 + + async def regex_files(self, regex, folder=None, file=None, files=()): results = [] - if folder: - if folder.is_dir(): - for file_path in folder.rglob("*"): - if file_path.is_file(): - results.extend(await self.regex_file(regex, file_path)) + if folder is not None and folder.is_dir(): + for file_path in folder.rglob("*"): + if file_path.is_file(): + results.extend(await self.regex_file(regex, file_path)) if files: - for file in files: - results.extend(await self.regex_file(regex, file)) - if file: + for f in files: + results.extend(await self.regex_file(regex, f)) + if file is not None: results.extend(await self.regex_file(regex, file)) return results - async def regex_file(self, regex, file=Path()): - if file.exists() and file.is_file(): - with file.open("r", encoding="utf-8", errors="ignore") as file: - content = file.read() - matches = await self.helpers.re.findall(regex, content) - if matches: - return matches + async def regex_file(self, regex, file=None): + if file is None or not (file.exists() and file.is_file()): + return [] + try: + size = file.stat().st_size + except OSError: + return [] + if size > self._regex_file_max_bytes: + self.debug(f"Skipping regex scan of {file} ({size} bytes)") + return [] + with file.open("r", encoding="utf-8", errors="ignore") as fh: + content = fh.read() + matches = await self.helpers.re.findall(regex, content) + if matches: + return matches return [] async def download_object(self, object, repo_url, repo_folder): diff --git a/bbot/test/test_step_1/test_gitdumper_regex_files.py b/bbot/test/test_step_1/test_gitdumper_regex_files.py new file mode 100644 index 0000000000..e6587fe4b2 --- /dev/null +++ b/bbot/test/test_step_1/test_gitdumper_regex_files.py @@ -0,0 +1,36 @@ +"""Regression test for the cwd-walking bug in ``gitdumper.regex_files``. + +When the ``folder=`` arg defaulted to ``Path()``, calling +``regex_files(file=foo)`` silently scanned the entire current working +directory in addition to the requested file — decoding every file in +the cwd into a Python string and running regex over it. With many +``CODE_REPOSITORY`` events that allocated 100+ GB. +""" + +import pytest + +from ..bbot_fixtures import * # noqa: F401, F403 + + +@pytest.mark.asyncio +async def test_regex_files_does_not_walk_cwd(bbot_scanner, tmp_path, monkeypatch): + """``regex_files(file=foo)`` must scan only ``foo`` — not the cwd.""" + target = tmp_path / "head" + target.write_text("ref: refs/heads/main\n") + + decoy_cwd = tmp_path / "cwd" + decoy_cwd.mkdir() + (decoy_cwd / "decoy.txt").write_text("ref: refs/heads/should_not_match\n") + monkeypatch.chdir(decoy_cwd) + + scan = bbot_scanner("evilcorp.com", modules=["gitdumper"]) + await scan._prep() + try: + gitdumper = scan.modules["gitdumper"] + regex = gitdumper.helpers.re.compile(r"ref: refs/heads/([a-zA-Z\d_-]+)") + results = await gitdumper.regex_files(regex, file=target) + + assert "main" in results, "expected the requested file to be scanned" + assert "should_not_match" not in results, "regex_files walked the cwd in addition to the requested file" + finally: + await scan._cleanup() From 5df3903836a2d26d39634ec5dfa532620c813bd8 Mon Sep 17 00:00:00 2001 From: liquidsec Date: Fri, 8 May 2026 16:45:56 -0400 Subject: [PATCH 2/2] gitdumper: add download size cap, recursion limit, cycle detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three additional safeguards layered on top of the cwd-walk fix: 1. download_files now passes max_size=10MB to helpers.download. Previous default (500MB from web helper) accepted arbitrarily large responses for any probed git path. 2. download_object recursion is capped at 100 levels. Real git object graphs are shallow; deeper means an unbounded chain of object references (malicious or corrupt repo). 3. download_object now tracks visited object hashes and skips duplicates. Cyclic tree references (A → B → A) no longer recurse forever. Each safeguard has a regression test in test_gitdumper_safeguards.py that fails on the unfixed code (verified) and passes after. --- bbot/modules/gitdumper.py | 25 +++- .../test_step_1/test_gitdumper_regex_files.py | 36 ----- .../test_step_1/test_gitdumper_safeguards.py | 141 ++++++++++++++++++ 3 files changed, 163 insertions(+), 39 deletions(-) delete mode 100644 bbot/test/test_step_1/test_gitdumper_regex_files.py create mode 100644 bbot/test/test_step_1/test_gitdumper_safeguards.py diff --git a/bbot/modules/gitdumper.py b/bbot/modules/gitdumper.py index bf10120d87..9fe23a20af 100644 --- a/bbot/modules/gitdumper.py +++ b/bbot/modules/gitdumper.py @@ -204,6 +204,13 @@ async def download_git_packs(self, url, folder): # Skip regex scan for files larger than this — real git ref/object/info # files are small; oversized is almost always a webserver returning HTML. _regex_file_max_bytes = 10 * 1024 * 1024 + # Cap download size per file. Real git files are tiny (refs/HEAD <1KB, + # index a few KB) or pack files (10s of MB legit). Anything bigger is + # a misconfigured / malicious server returning an error page. + _download_max_size = "10MB" + # Cap how deep ``download_object`` recursion can go. Real git object + # graphs are shallow (commit → tree → blob, depth 10-20 in practice). + _download_object_max_depth = 100 async def regex_files(self, regex, folder=None, file=None, files=()): results = [] @@ -235,13 +242,25 @@ async def regex_file(self, regex, file=None): return matches return [] - async def download_object(self, object, repo_url, repo_folder): + async def download_object(self, object, repo_url, repo_folder, _seen=None, _depth=0): + if _seen is None: + _seen = set() + # cycle detection — git tree objects can reference each other in + # malformed/malicious repos + if object in _seen: + return + _seen.add(object) + # depth cap — even without cycles, an unbounded tree would burn + # stack frames + memory per frame + if _depth >= self._download_object_max_depth: + self.debug(f"download_object: hit max recursion depth at {object}") + return await self.download_files( [self.helpers.urlparse(self.helpers.urljoin(repo_url, f"objects/{object[:2]}/{object[2:]}"))], repo_folder ) output = await self.git_catfile(object, option="-p", folder=repo_folder) for obj in await self.helpers.re.findall(self.obj_regex, output): - await self.download_object(obj, repo_url, repo_folder) + await self.download_object(obj, repo_url, repo_folder, _seen=_seen, _depth=_depth + 1) async def download_files(self, urls, folder): for url in urls: @@ -251,7 +270,7 @@ async def download_files(self, urls, folder): self.helpers.mkdir(filename.parent) if hash(str(file_url)) not in self.urls_downloaded: self.verbose(f"Downloading {file_url} to {filename}") - await self.helpers.download(file_url, filename=filename, warn=False) + await self.helpers.download(file_url, filename=filename, warn=False, max_size=self._download_max_size) self.urls_downloaded.add(hash(str(file_url))) if any(folder.rglob("*")): return True diff --git a/bbot/test/test_step_1/test_gitdumper_regex_files.py b/bbot/test/test_step_1/test_gitdumper_regex_files.py deleted file mode 100644 index e6587fe4b2..0000000000 --- a/bbot/test/test_step_1/test_gitdumper_regex_files.py +++ /dev/null @@ -1,36 +0,0 @@ -"""Regression test for the cwd-walking bug in ``gitdumper.regex_files``. - -When the ``folder=`` arg defaulted to ``Path()``, calling -``regex_files(file=foo)`` silently scanned the entire current working -directory in addition to the requested file — decoding every file in -the cwd into a Python string and running regex over it. With many -``CODE_REPOSITORY`` events that allocated 100+ GB. -""" - -import pytest - -from ..bbot_fixtures import * # noqa: F401, F403 - - -@pytest.mark.asyncio -async def test_regex_files_does_not_walk_cwd(bbot_scanner, tmp_path, monkeypatch): - """``regex_files(file=foo)`` must scan only ``foo`` — not the cwd.""" - target = tmp_path / "head" - target.write_text("ref: refs/heads/main\n") - - decoy_cwd = tmp_path / "cwd" - decoy_cwd.mkdir() - (decoy_cwd / "decoy.txt").write_text("ref: refs/heads/should_not_match\n") - monkeypatch.chdir(decoy_cwd) - - scan = bbot_scanner("evilcorp.com", modules=["gitdumper"]) - await scan._prep() - try: - gitdumper = scan.modules["gitdumper"] - regex = gitdumper.helpers.re.compile(r"ref: refs/heads/([a-zA-Z\d_-]+)") - results = await gitdumper.regex_files(regex, file=target) - - assert "main" in results, "expected the requested file to be scanned" - assert "should_not_match" not in results, "regex_files walked the cwd in addition to the requested file" - finally: - await scan._cleanup() diff --git a/bbot/test/test_step_1/test_gitdumper_safeguards.py b/bbot/test/test_step_1/test_gitdumper_safeguards.py new file mode 100644 index 0000000000..0ba36e849f --- /dev/null +++ b/bbot/test/test_step_1/test_gitdumper_safeguards.py @@ -0,0 +1,141 @@ +"""Regression tests for ``gitdumper`` safeguards against pathological / malicious +git layouts. + +Each test demonstrates a failure mode that would either leak memory, hang, or +recurse without bound on the unfixed code. +""" + +import pytest + +from ..bbot_fixtures import * # noqa: F401, F403 + + +@pytest.mark.asyncio +async def test_regex_files_does_not_walk_cwd(bbot_scanner, tmp_path, monkeypatch): + """``regex_files(file=foo)`` must scan only ``foo`` — not the cwd.""" + target = tmp_path / "head" + target.write_text("ref: refs/heads/main\n") + + decoy_cwd = tmp_path / "cwd" + decoy_cwd.mkdir() + (decoy_cwd / "decoy.txt").write_text("ref: refs/heads/should_not_match\n") + monkeypatch.chdir(decoy_cwd) + + scan = bbot_scanner("evilcorp.com", modules=["gitdumper"]) + await scan._prep() + try: + gitdumper = scan.modules["gitdumper"] + regex = gitdumper.helpers.re.compile(r"ref: refs/heads/([a-zA-Z\d_-]+)") + results = await gitdumper.regex_files(regex, file=target) + + assert "main" in results, "expected the requested file to be scanned" + assert "should_not_match" not in results, "regex_files walked the cwd in addition to the requested file" + finally: + await scan._cleanup() + + +@pytest.mark.asyncio +async def test_download_files_caps_max_size(bbot_scanner, tmp_path, monkeypatch): + """``download_files`` must pass an explicit ``max_size`` to ``helpers.download``. + + Without it, a misconfigured / malicious server can return up to 500 MB + (the web helper default) per probed git path. Real git refs/info files + are tiny, so capping at a few MB shuts down a whole class of abuse. + """ + scan = bbot_scanner("evilcorp.com", modules=["gitdumper"]) + await scan._prep() + try: + gitdumper = scan.modules["gitdumper"] + seen_kwargs = [] + + async def traced_download(url, **kwargs): + seen_kwargs.append(kwargs) + return None + + monkeypatch.setattr(gitdumper.helpers, "download", traced_download) + + url = gitdumper.helpers.urlparse("http://example.com/.git/HEAD") + await gitdumper.download_files([url], tmp_path) + + assert seen_kwargs, "expected helpers.download to be called" + for call in seen_kwargs: + assert "max_size" in call, "max_size must be passed to helpers.download" + finally: + await scan._cleanup() + + +@pytest.mark.asyncio +async def test_download_object_caps_recursion_depth(bbot_scanner, tmp_path, monkeypatch): + """``download_object`` must bound recursion depth. + + Mocks ``git_catfile`` so every object's output yields a fresh new hash, + creating an infinite chain. With no cap we'd recurse until Python's + stack limit (or the OS kills us). With a cap we stop at a finite depth. + """ + scan = bbot_scanner("evilcorp.com", modules=["gitdumper"]) + await scan._prep() + try: + gitdumper = scan.modules["gitdumper"] + + async def nop_download_files(urls, folder): + return True + + monkeypatch.setattr(gitdumper, "download_files", nop_download_files) + + counter = [0] + + async def fake_catfile(hash_, option="-t", folder=None): + counter[0] += 1 + new_hash = f"{counter[0]:040x}" + return f"object content with {new_hash}" + + monkeypatch.setattr(gitdumper, "git_catfile", fake_catfile) + + await gitdumper.download_object("0" * 40, "http://example.com", tmp_path) + + # If recursion is bounded, counter stays well below Python's stack limit. + # We just need to confirm it didn't spiral out — anything < 500 means + # the cap is in effect. + assert counter[0] < 500, f"download_object recursed {counter[0]} levels — cap not in effect" + finally: + await scan._cleanup() + + +@pytest.mark.asyncio +async def test_download_object_detects_cycles(bbot_scanner, tmp_path, monkeypatch): + """``download_object`` must not re-process an object hash it has + already visited. Malicious or corrupt git repos can have cyclic + references.""" + scan = bbot_scanner("evilcorp.com", modules=["gitdumper"]) + await scan._prep() + try: + gitdumper = scan.modules["gitdumper"] + + async def nop_download_files(urls, folder): + return True + + monkeypatch.setattr(gitdumper, "download_files", nop_download_files) + + hash_a = "a" * 40 + hash_b = "b" * 40 + catfile_calls = [] + + async def fake_catfile(hash_, option="-t", folder=None): + catfile_calls.append(hash_) + if hash_ == hash_a: + return f"references {hash_b}" + if hash_ == hash_b: + return f"references {hash_a}" # cycle back + return "" + + monkeypatch.setattr(gitdumper, "git_catfile", fake_catfile) + + await gitdumper.download_object(hash_a, "http://example.com", tmp_path) + + # With cycle detection: A -> B -> stop (A already seen). 2 catfile calls. + # Without: infinite, eventually RecursionError. + assert len(catfile_calls) <= 2, ( + f"download_object did not detect cycle (called catfile {len(catfile_calls)} times)" + ) + finally: + await scan._cleanup()