From 1148b34bce4290be882ad568d4eed34197a9802d Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Fri, 3 Jul 2026 13:17:49 +1000 Subject: [PATCH] More hardening --- enroll/ignore.py | 44 +++++++++++++++- enroll/remote.py | 42 ++++++++++++++- tests/test_remote.py | 96 ++++++++++++++++++++++++++++++++++ tests/test_secret_detection.py | 71 +++++++++++++++++++++++++ 4 files changed, 251 insertions(+), 2 deletions(-) diff --git a/enroll/ignore.py b/enroll/ignore.py index 4cc6683..96b2ac9 100644 --- a/enroll/ignore.py +++ b/enroll/ignore.py @@ -193,10 +193,52 @@ HIGH_CONFIDENCE_SECRET_PATTERNS = [ # start-of-input or any non-identifier character, which is comment-marker # agnostic and still avoids matching an identifier such as # ``my_authorization:``. + # + # Both the header form (``Authorization: Bearer ...``) and the config + # assignment form (``authorization = Bearer ...`` / ``http_authorization=...``) + # are matched: a populated Authorization header is an obvious credential + # whether it is written HTTP-style with ``:`` or config-style with ``=``. The + # ``(?:proxy|http)`` prefix accepts ``proxy-``/``proxy_``/``http_`` spellings + # (e.g. ``proxy_authorization``) that appear in real client/daemon configs. + # A scheme keyword (bearer/basic/token/digest) plus a value is still required, + # so ordinary keys such as ``AuthorizationEnabled = true`` do not match. re.compile( - rb"(?im)(?:^|[^A-Za-z0-9_.-])(?:proxy-)?authorization\s*:\s*" + rb"(?im)(?:^|[^A-Za-z0-9_.-])(?:(?:proxy|http)[-_])?authorization\s*[:=]\s*" rb"(?:bearer|basic|token|digest)\s+\S" ), + # Token-key credential assignments that the general assignment pattern above + # misses because of a leading underscore or a camelCase/prefix spelling. + # + # _authToken=npm_xxxxxxxx + # //registry.npmjs.org/:_authToken=npm_xxxxxxxx (npm .npmrc form) + # authToken=xxxxxxxx + # bearerToken=xxxxxxxx / bearer_token=xxxxxxxx + # + # The general assignment pattern requires a non-key boundary + # (``[^A-Za-z0-9_.-]``) immediately before the credential keyword, which a + # leading ``_`` (a word char) defeats -- so ``_authToken`` slips past it -- + # and its keyword list does not include a ``bearer[_-]?token`` spelling. This + # dedicated pattern covers exactly those ``auth``/``bearer`` token keys and, + # like the general one, requires a *populated, value-like* right-hand side + # (>= 8 non-space chars, or a value containing a digit / token-ish + # character), so a bare mention or a short boolean/number such as + # ``authTokenEnabled = true`` does not trip it. It is intentionally scoped to + # ``auth``/``bearer`` token keys only; broadening the general boundary to + # accept a leading underscore everywhere would risk false positives on + # value-less identifiers in ordinary config. + re.compile( + rb"""(?ix) + (?:^|[^A-Za-z0-9.-]) + _? + (?:auth|bearer)[_-]?token + (?:[_.-][A-Za-z0-9]+)* + [\"']? + \s*[:=]\s* + [\"']? + (?=\S) + (?:\S{8,}|\S*[0-9/+=._-]\S*) + """ + ), ] SENSITIVE_CONTENT_PATTERNS = [ diff --git a/enroll/remote.py b/enroll/remote.py index 78549e6..57d0672 100644 --- a/enroll/remote.py +++ b/enroll/remote.py @@ -189,14 +189,35 @@ def remote_harvest( ) +# Resource caps for untrusted tar extraction. These mirror the directory-bundle +# freeze limits (see manifest_safety._FREEZE_MAX_FILES / _FREEZE_MAX_FILE_BYTES) +# so a harvest delivered as a tarball is bounded the same way as one delivered as +# a directory. The total-size cap additionally guards against a decompression +# bomb whose members are each individually under the per-file cap. +_TAR_MAX_MEMBERS = 200_000 +_TAR_MAX_FILE_BYTES = 64 * 1024 * 1024 +_TAR_MAX_TOTAL_BYTES = 10 * 1024 * 1024 * 1024 +_TAR_MAX_PATH_DEPTH = 64 + + def _safe_extract_tar(tar: tarfile.TarFile, dest: Path) -> None: """Safely extract a tar archive into dest. - Protects against path traversal (e.g. entries containing ../). + Protects against path traversal (e.g. entries containing ../) and, as + availability defence-in-depth, against resource-exhaustion by a structurally + valid but abusive archive (a decompression bomb, a huge member, or millions + of tiny members). The caps mirror the directory-bundle freeze limits so a + tar bundle and a directory bundle are bounded the same way. A remote or + user-supplied harvest tarball is untrusted input, so these limits keep a + malicious archive from exhausting disk, inodes, memory, or time during local + extraction/validation. """ # Note: tar member names use POSIX separators regardless of platform. dest = dest.resolve() + member_count = 0 + total_size = 0 + for m in tar.getmembers(): name = m.name @@ -205,16 +226,35 @@ def _safe_extract_tar(tar: tarfile.TarFile, dest: Path) -> None: if name in {".", "./"}: continue + member_count += 1 + if member_count > _TAR_MAX_MEMBERS: + raise RuntimeError( + f"tar archive has too many members (> {_TAR_MAX_MEMBERS})" + ) + # Reject absolute paths and any '..' components up front. p = PurePosixPath(name) if p.is_absolute() or ".." in p.parts: raise RuntimeError(f"Unsafe tar member path: {name}") + if len(p.parts) > _TAR_MAX_PATH_DEPTH: + raise RuntimeError(f"tar member path is too deeply nested: {name}") + # Refuse to extract links or device nodes from an untrusted archive. # (A symlink can be used to redirect subsequent writes outside dest.) if m.issym() or m.islnk() or m.isdev(): raise RuntimeError(f"Refusing to extract special tar member: {name}") + if m.isfile(): + if m.size > _TAR_MAX_FILE_BYTES: + raise RuntimeError(f"tar member is too large: {name}") + total_size += int(m.size) + if total_size > _TAR_MAX_TOTAL_BYTES: + raise RuntimeError( + "tar archive uncompressed size exceeds limit " + f"(> {_TAR_MAX_TOTAL_BYTES} bytes)" + ) + member_path = (dest / Path(*p.parts)).resolve() if member_path != dest and not str(member_path).startswith(str(dest) + os.sep): raise RuntimeError(f"Unsafe tar member path: {name}") diff --git a/tests/test_remote.py b/tests/test_remote.py index dc486ac..8d2fe6c 100644 --- a/tests/test_remote.py +++ b/tests/test_remote.py @@ -1183,3 +1183,99 @@ def test_build_enroll_pyz_excludes_tests_and_caches_and_returns_sha(tmp_path: Pa assert "/tests/" not in n assert not leaf.startswith("test_") assert leaf != "conftest.py" + + +def test_safe_extract_tar_rejects_too_many_members(tmp_path: Path, monkeypatch): + import enroll.remote as r + + monkeypatch.setattr(r, "_TAR_MAX_MEMBERS", 3) + + bio = io.BytesIO() + with tarfile.open(fileobj=bio, mode="w:gz") as tf: + for i in range(10): + ti = tarfile.TarInfo(name=f"f{i}") + ti.size = 1 + tf.addfile(ti, io.BytesIO(b"x")) + + bio.seek(0) + with tarfile.open(fileobj=bio, mode="r:gz") as tf: + with pytest.raises(RuntimeError, match="too many members"): + r._safe_extract_tar(tf, tmp_path) + + +def test_safe_extract_tar_rejects_oversized_member(tmp_path: Path, monkeypatch): + import enroll.remote as r + + monkeypatch.setattr(r, "_TAR_MAX_FILE_BYTES", 100) + + bio = io.BytesIO() + with tarfile.open(fileobj=bio, mode="w:gz") as tf: + ti = tarfile.TarInfo(name="big") + payload = b"x" * 500 + ti.size = len(payload) + tf.addfile(ti, io.BytesIO(payload)) + + bio.seek(0) + with tarfile.open(fileobj=bio, mode="r:gz") as tf: + with pytest.raises(RuntimeError, match="too large"): + r._safe_extract_tar(tf, tmp_path) + + +def test_safe_extract_tar_rejects_aggregate_size_bomb(tmp_path: Path, monkeypatch): + import enroll.remote as r + + # Each member is under the per-file cap, but the sum exceeds the total cap. + monkeypatch.setattr(r, "_TAR_MAX_TOTAL_BYTES", 1000) + + bio = io.BytesIO() + with tarfile.open(fileobj=bio, mode="w:gz") as tf: + for i in range(10): + ti = tarfile.TarInfo(name=f"f{i}") + payload = b"x" * 300 + ti.size = len(payload) + tf.addfile(ti, io.BytesIO(payload)) + + bio.seek(0) + with tarfile.open(fileobj=bio, mode="r:gz") as tf: + with pytest.raises(RuntimeError, match="uncompressed size exceeds"): + r._safe_extract_tar(tf, tmp_path) + + +def test_safe_extract_tar_rejects_excessive_path_depth(tmp_path: Path, monkeypatch): + import enroll.remote as r + + monkeypatch.setattr(r, "_TAR_MAX_PATH_DEPTH", 4) + + deep = "/".join(f"d{i}" for i in range(10)) + "/file" + bio = io.BytesIO() + with tarfile.open(fileobj=bio, mode="w:gz") as tf: + ti = tarfile.TarInfo(name=deep) + ti.size = 1 + tf.addfile(ti, io.BytesIO(b"x")) + + bio.seek(0) + with tarfile.open(fileobj=bio, mode="r:gz") as tf: + with pytest.raises(RuntimeError, match="too deeply nested"): + r._safe_extract_tar(tf, tmp_path) + + +def test_safe_extract_tar_allows_normal_bundle(tmp_path: Path): + """A normal, modestly-sized bundle still extracts cleanly under the caps.""" + import enroll.remote as r + + bio = io.BytesIO() + with tarfile.open(fileobj=bio, mode="w:gz") as tf: + for name, data in ( + ("state.json", b"{}"), + ("artifacts/etc_custom/etc/app.conf", b"k=1\n"), + ): + ti = tarfile.TarInfo(name=name) + ti.size = len(data) + tf.addfile(ti, io.BytesIO(data)) + + bio.seek(0) + with tarfile.open(fileobj=bio, mode="r:gz") as tf: + r._safe_extract_tar(tf, tmp_path) + + assert (tmp_path / "state.json").is_file() + assert (tmp_path / "artifacts" / "etc_custom" / "etc" / "app.conf").is_file() diff --git a/tests/test_secret_detection.py b/tests/test_secret_detection.py index 20a41bf..cd8e545 100644 --- a/tests/test_secret_detection.py +++ b/tests/test_secret_detection.py @@ -371,3 +371,74 @@ def test_binary_keyring_allowlist_still_scans_for_private_key_material(): dangerous._content_deny_reason("/etc/apt/keyrings/mykey.gpg", private_binary) is None ) + + +def test_authorization_assignment_form_is_denied(): + """A populated Authorization/Proxy-Authorization credential is refused + whether written HTTP-style with ':' or config-style with '='. + + The header scanner previously only anchored on ':', so a config assignment + such as ``Authorization = Bearer `` (and ``proxy_authorization``/ + ``http_authorization`` spellings) slipped through safe mode. + """ + pol = IgnorePolicy(dangerous=False) + leaking = [ + b"Authorization = Bearer supersecretvalue\n", + b"Authorization: Bearer supersecretvalue\n", + b"authorization=Bearer abcdef123456\n", + b"proxy_authorization = Basic abcdef123456\n", + b"Proxy-Authorization: Bearer abcdef123456\n", + b"http_authorization = Bearer abcdef123456\n", + ] + for data in leaking: + assert ( + pol._content_deny_reason("/etc/example.conf", data) == "sensitive_content" + ), data + + +def test_underscore_and_camelcase_token_keys_are_denied(): + """Token keys the general assignment pattern misses -- a leading-underscore + npm token, a camelCase ``authToken``, or a ``bearerToken`` spelling -- are + refused when they carry a populated value.""" + pol = IgnorePolicy(dangerous=False) + leaking = [ + b"_authToken=npm_abcdefghijklmnopqrstuvwxyz\n", + b"//registry.npmjs.org/:_authToken=npm_abcdefghijklmnop\n", + b"authToken=abcdefghijklmnopqrstuvwxyz\n", + b"bearerToken=abcdefghijklmnopqrstuvwxyz\n", + b"bearer_token = abcdef123456\n", + b"auth_token: ya29.aRealLookingToken\n", + ] + for data in leaking: + assert ( + pol._content_deny_reason("/etc/app/app.conf", data) == "sensitive_content" + ), data + + +def test_authorization_and_token_keys_no_false_positives(): + """The broadened Authorization/token-key rules must not fire on ordinary + config: a scheme keyword + value is required for the header form, and a + populated value is required for the token-key form, so boolean/numeric + settings and value-less mentions stay collectable.""" + pol = IgnorePolicy(dangerous=False) + allowed = [ + # Authorization-ish keys with no bearer/basic/... scheme + value. + b"AuthorizationEnabled = true\n", + b"authorization = true\n", + b"my_authorization: somevalue\n", + # Token-ish keys that are not populated credentials. + b"authTokenEnabled = true\n", + b"authTokenTimeout = 30\n", + b"AuthorizedKeysFile .ssh/authorized_keys\n", + ] + for data in allowed: + assert pol._content_deny_reason("/etc/app/app.conf", data) is None, data + + # --dangerous still collects everything. + dangerous = IgnorePolicy(dangerous=True) + assert ( + dangerous._content_deny_reason( + "/etc/example.conf", b"Authorization = Bearer secret\n" + ) + is None + )