From be914253552a79481b298f1218b80a15bd88473f Mon Sep 17 00:00:00 2001 From: d9pouces Date: Wed, 9 Sep 2026 23:18:24 +0200 Subject: [PATCH] feat: sanitize filenames in apt and yum modules to prevent path traversal Signed-off-by: d9pouces --- id_configurator/modules/apt.py | 6 ++-- id_configurator/modules/base.py | 15 +++++++++ id_configurator/modules/certificates.py | 8 ++--- id_configurator/modules/yum.py | 7 ++-- .../dockerfiles/repos/apt-signing-key.key.asc | 33 +++++++++++++++++++ .../dockerfiles/repos/apt-signing-key.pub.asc | 19 +++++++++++ tests/test_apt.py | 24 ++++++++++++-- tests/test_certificates.py | 21 ++++++++++++ tests/test_modules_base.py | 19 ++++++++++- tests/test_yum.py | 30 +++++++++++++++++ 10 files changed, 169 insertions(+), 13 deletions(-) create mode 100644 tests/dockerfiles/repos/apt-signing-key.key.asc create mode 100644 tests/dockerfiles/repos/apt-signing-key.pub.asc diff --git a/id_configurator/modules/apt.py b/id_configurator/modules/apt.py index 40ac11f..8f6167a 100644 --- a/id_configurator/modules/apt.py +++ b/id_configurator/modules/apt.py @@ -29,7 +29,7 @@ from pathlib import Path from id_configurator.config import AptSource from id_configurator.context import Context -from id_configurator.modules.base import Module, register +from id_configurator.modules.base import Module, register, sanitize_filename logger = logging.getLogger(__name__) @@ -122,7 +122,7 @@ class AptModule(Module): def _resolve_signed_by(self, context: Context, source: AptSource) -> str | None: if source.signed_by: keyring = _dearmor_gpg_key(source.name, source.signed_by.encode("utf-8")) - relative = f"{_KEYRINGS_DIR}/{source.name}.gpg" + relative = f"{_KEYRINGS_DIR}/{sanitize_filename(source.name)}.gpg" context.write_file(relative, keyring) logger.info("installed apt keyring for source %s", source.name) return f"/{relative}" @@ -134,7 +134,7 @@ class AptModule(Module): for source in context.config.configurations.apt.sources: signed_by_path = self._resolve_signed_by(context, source) context.write_file( - f"{_SOURCES_LIST_D}/{source.name}.sources", + f"{_SOURCES_LIST_D}/{sanitize_filename(source.name)}.sources", source.to_deb822(signed_by_path).encode("utf-8"), ) logger.info("added apt source %s", source.name) diff --git a/id_configurator/modules/base.py b/id_configurator/modules/base.py index 1d9055a..4a13bde 100644 --- a/id_configurator/modules/base.py +++ b/id_configurator/modules/base.py @@ -34,3 +34,18 @@ def register(module_cls: type[Module]) -> type[Module]: def all_modules() -> list[type[Module]]: return list(_REGISTRY) + + +def sanitize_filename(name: str) -> str: + """Make a user-supplied name safe to use as a single filesystem path + component. + + Modules that derive a file's name from configuration data (a + certificate's name, a repository's name, ...) must run it through this + first: without it, a name containing `/` (or `\\`, harmless on the + target systems this tool writes to but stripped for good measure) + would silently turn into extra, unintended path segments - creating + nested directories a careless or malicious name shouldn't be able to + create - instead of the single file the caller asked for. + """ + return name.replace("/", "_").replace("\\", "_") diff --git a/id_configurator/modules/certificates.py b/id_configurator/modules/certificates.py index 8ecf64a..7db4607 100644 --- a/id_configurator/modules/certificates.py +++ b/id_configurator/modules/certificates.py @@ -49,7 +49,7 @@ import posixpath from pathlib import Path from id_configurator.context import Context -from id_configurator.modules.base import Module, register +from id_configurator.modules.base import Module, register, sanitize_filename logger = logging.getLogger(__name__) @@ -142,7 +142,7 @@ class CertificatesModule(Module): def apply(self, context: Context) -> None: configured_names = { - certificate.name + sanitize_filename(certificate.name) for certificate in context.config.configurations.certificates } for existing in context.iter_src_dir(_LOCAL_CERTS_DIR): @@ -152,7 +152,7 @@ class CertificatesModule(Module): ) for certificate in context.config.configurations.certificates: context.write_file( - f"{_LOCAL_CERTS_DIR}/{certificate.name}.crt", + f"{_LOCAL_CERTS_DIR}/{sanitize_filename(certificate.name)}.crt", certificate.content.encode(), ) logger.info("installed root certificate %s", certificate.name) @@ -237,7 +237,7 @@ class CertificatesModule(Module): ) for certificate in context.config.configurations.certificates: context.write_file( - f"{_RHEL_ANCHORS_DIR}/{certificate.name}.crt", + f"{_RHEL_ANCHORS_DIR}/{sanitize_filename(certificate.name)}.crt", certificate.content.encode(), ) diff --git a/id_configurator/modules/yum.py b/id_configurator/modules/yum.py index 29d5666..d0caa7a 100644 --- a/id_configurator/modules/yum.py +++ b/id_configurator/modules/yum.py @@ -30,7 +30,7 @@ from io import StringIO from id_configurator.config import NamedContent, YumRepository from id_configurator.context import Context -from id_configurator.modules.base import Module, register +from id_configurator.modules.base import Module, register, sanitize_filename logger = logging.getLogger(__name__) @@ -71,7 +71,7 @@ def _write_repo_file(context: Context, repository: YumRepository) -> None: else: lines.append("gpgcheck=0") context.write_file( - f"{_REPOS_DIR}/{repository.name}.repo", + f"{_REPOS_DIR}/{sanitize_filename(repository.name)}.repo", ("\n".join(lines) + "\n").encode("utf-8"), ) logger.info("added yum/dnf repository %s", repository.name) @@ -100,7 +100,8 @@ def _validate_gpg_key(name: str, content: bytes) -> None: def _import_key(context: Context, key: NamedContent) -> None: _validate_gpg_key(key.name, key.content.encode("utf-8")) context.write_file( - f"{_RPM_GPG_DIR}/RPM-GPG-KEY-{key.name}", key.content.encode("utf-8") + f"{_RPM_GPG_DIR}/RPM-GPG-KEY-{sanitize_filename(key.name)}", + key.content.encode("utf-8"), ) logger.info("imported yum/dnf GPG key %s", key.name) diff --git a/tests/dockerfiles/repos/apt-signing-key.key.asc b/tests/dockerfiles/repos/apt-signing-key.key.asc new file mode 100644 index 0000000..b91e21a --- /dev/null +++ b/tests/dockerfiles/repos/apt-signing-key.key.asc @@ -0,0 +1,33 @@ +-----BEGIN PGP PRIVATE KEY BLOCK----- + +lQOYBGqg6rMBCADaMUd27S41+kInPXuRZ0aL41aMZjqr+mw4W34w8i/JRYMJ6eGb +sXf40AaFfS7zkbUpQZpmh3aYPF4jUAdWV03ZNNYdcGerIAKc2xsugSKjJIm1XPQq +upz914b4/hp/UslDUH7SPZ+3Tq17TXW+xP6aUM9VcWMnlJT/DFgxh5sZB3nfDeYr +yBV+PmEVfuQaUSIwKAh3Hv0ZaBHS8KzsawLzbJ+Pa9W9wjVME1izi9ctloGSxtLc +p1suS2IS33QB0jGgj4VbuMcjo2iCpzW3Xd7jQ7WpXGjrl0bHnJTnWWB0ihDD3cJe +tJlk9GScMQu3Z3th67DmqTXQru3btrDjxMifABEBAAEAB/sG3JffwJO8dNNRwpg5 +OWM5kPuuzKQ5tlVi7FWqmmMwVDRLwoQqzkj0lS4vUoGAfWuSEZMWeFodbYCUYbbp +WzOCiivVnyxTcF1Eq4tOShdZxBNklbjEAzxeq0F+YRdaO7NCn6xtVizIqfvAre3v +Aujy12m9ySQSIZUR9ERtwlL5VXj4yzcXciyM9aNbh2D4BFAoo+fBL1B5mhpKQ16B +BafkpOf0myWZrwLdcn1t7BQ6UcLqsPJj6COCXXRDHoZvim38UBMMajNP4+8eC2Vk +DDR6iFz53FRvIREQWkoEEW4P80v1lgcnP1apJGhkrJYCPM7ObZk0MoQkBiBdlPPQ +VLiRBADhNsKjNd7v3Af5XGPJGWWZxCMxPlgwc6EMjCX3SQDql042eSIqwn1FFP6H +Hi8lyq/XWz0JcUwcrpFxpIC3OLUsBleX3Z4E17Z6AotSV0FC6y23cqgtweMeLA0L +tayMRmXJ7Aan+xfvi8MbW6lYGzM2uYHl27rVBuCt0kvxn+U4lwQA+ATO8KaBreCL +E/vXGZaPWJs2GoiDBavKwqdYtlXDKlVP5XvBgaPfY6zKLtCsC6N+DKkVo42Stwu3 +oB6HSSLvXvFIhvUReh5PLpijmOroAJzvLQN9qmZBvF4IOUgaHmlYlL0DGzlxcwLr +SdzqPaGRa+EgraFDDGGu5oQ5A2wNKTkEAMDS2w5SibOfsvnfK4aDc0mi8/fftzCO +SwsvVzR0SmbXtaCPBnBjlA7YJhRZ6n/7nDpswrWPk8g125A0vK1cNRGM0ufKYW1f +DMbB/aYrJjAKz8zWH6cgOD4fqM2gA9JounkIcJvWOToGume8pXhmem94u2FP8aik +A6hL/Os+WRQcMNi0NWlkLWNvbmZpZ3VyYXRvci10ZXN0cyA8dGVzdHNAaWQtY29u +ZmlndXJhdG9yLmludmFsaWQ+iQFuBBMBCABYFiEEaZF4ALsB1xUU3IP5rl3Himib +1RgFAmqg6rMbFIAAAAAABAAObWFudTIsMi41KzEuMTIsMCwzAxsvBAULCQgHAgIi +AgYVCgkICwIEFgIDAQIeBwIXgAAKCRCuXceKaJvVGFRrB/wIbG1HHQANDCHgyl4T +RTPuKBX2c9lmJi2plp+T5L5sXyTMbAMLfTCkJWL/zHLM3N2v6GZuzbwZM8Dzj9zL +LMpC6newmZe4YX+ZtyZGufBI963kh8PvlSWWUSyH+oYoIhlyJqj7tCQXoijEvQnS +ReKGHUTvQGvQK2Ybp735NmAiDOM3/3KKh2Dqas/Tc2mxec4whr8RyUidsF+Yp6wT +esfMxeax6DmWIcSLDWpUBndeahZ2Ff0c96BzbpF1v2hbvyI/IaP5snb4Nm1xpREm ++y8iDAQhOklfXCyXrWgN3sIrk3tDndGrn+nw0iusnaBVki0keqjlH0camoUP28tu +3sxG +=TrE4 +-----END PGP PRIVATE KEY BLOCK----- diff --git a/tests/dockerfiles/repos/apt-signing-key.pub.asc b/tests/dockerfiles/repos/apt-signing-key.pub.asc new file mode 100644 index 0000000..610a719 --- /dev/null +++ b/tests/dockerfiles/repos/apt-signing-key.pub.asc @@ -0,0 +1,19 @@ +-----BEGIN PGP PUBLIC KEY BLOCK----- + +mQENBGqg6rMBCADaMUd27S41+kInPXuRZ0aL41aMZjqr+mw4W34w8i/JRYMJ6eGb +sXf40AaFfS7zkbUpQZpmh3aYPF4jUAdWV03ZNNYdcGerIAKc2xsugSKjJIm1XPQq +upz914b4/hp/UslDUH7SPZ+3Tq17TXW+xP6aUM9VcWMnlJT/DFgxh5sZB3nfDeYr +yBV+PmEVfuQaUSIwKAh3Hv0ZaBHS8KzsawLzbJ+Pa9W9wjVME1izi9ctloGSxtLc +p1suS2IS33QB0jGgj4VbuMcjo2iCpzW3Xd7jQ7WpXGjrl0bHnJTnWWB0ihDD3cJe +tJlk9GScMQu3Z3th67DmqTXQru3btrDjxMifABEBAAG0NWlkLWNvbmZpZ3VyYXRv +ci10ZXN0cyA8dGVzdHNAaWQtY29uZmlndXJhdG9yLmludmFsaWQ+iQFuBBMBCABY +FiEEaZF4ALsB1xUU3IP5rl3Himib1RgFAmqg6rMbFIAAAAAABAAObWFudTIsMi41 +KzEuMTIsMCwzAxsvBAULCQgHAgIiAgYVCgkICwIEFgIDAQIeBwIXgAAKCRCuXceK +aJvVGFRrB/wIbG1HHQANDCHgyl4TRTPuKBX2c9lmJi2plp+T5L5sXyTMbAMLfTCk +JWL/zHLM3N2v6GZuzbwZM8Dzj9zLLMpC6newmZe4YX+ZtyZGufBI963kh8PvlSWW +USyH+oYoIhlyJqj7tCQXoijEvQnSReKGHUTvQGvQK2Ybp735NmAiDOM3/3KKh2Dq +as/Tc2mxec4whr8RyUidsF+Yp6wTesfMxeax6DmWIcSLDWpUBndeahZ2Ff0c96Bz +bpF1v2hbvyI/IaP5snb4Nm1xpREm+y8iDAQhOklfXCyXrWgN3sIrk3tDndGrn+nw +0iusnaBVki0keqjlH0camoUP28tu3sxG +=qZrp +-----END PGP PUBLIC KEY BLOCK----- diff --git a/tests/test_apt.py b/tests/test_apt.py index e51f8be..c9aad78 100644 --- a/tests/test_apt.py +++ b/tests/test_apt.py @@ -32,7 +32,7 @@ def make_config() -> Configuration: return Configuration.model_validate(SOURCE_CONTENT) -def make_config_with(source_extra: dict) -> Configuration: +def make_config_with(source_extra: dict, name: str = "interdiode") -> Configuration: data = { "uuid": "12345678-1234-5678-1234-567812345678", "organisation": "InterDiode", @@ -40,7 +40,7 @@ def make_config_with(source_extra: dict) -> Configuration: "apt": { "sources": [ { - "name": "interdiode", + "name": name, "uris": ["http://example.invalid/debian"], "suites": ["stable"], **source_extra, @@ -227,6 +227,26 @@ def test_apply_without_signed_by_writes_no_keyring_or_field( ) +def test_apply_sanitizes_source_name_with_slash(tmp_path, monkeypatch) -> None: + mock_run = _mock_gpg_dearmor_success(monkeypatch) + dst = tmp_path / "dst" + context = Context( + config=make_config_with({"signed-by": KEY_CONTENT}, name="../../evil"), + dst=dst, + src=None, + ) + + AptModule().apply(context) + + keyring = dst / "etc/apt/keyrings/.._.._evil.gpg" + assert keyring.read_bytes() == KEYRING_BYTES + source_file = dst / "etc/apt/sources.list.d/.._.._evil.sources" + assert "Signed-By: /etc/apt/keyrings/.._.._evil.gpg" in source_file.read_text( + encoding="utf-8" + ) + mock_run.assert_called_once() + + def test_resolve_signed_by_raises_when_gpg_not_on_path(tmp_path, monkeypatch) -> None: monkeypatch.setattr("id_configurator.modules.apt.shutil.which", lambda _: None) dst = tmp_path / "dst" diff --git a/tests/test_certificates.py b/tests/test_certificates.py index 3810685..6f5af38 100644 --- a/tests/test_certificates.py +++ b/tests/test_certificates.py @@ -54,6 +54,27 @@ def test_apply_writes_certificate_and_rebuilds_bundle(tmp_path) -> None: assert openssl_default.readlink().as_posix() == "/etc/ssl/certs/ca-certificates.crt" +def test_apply_sanitizes_certificate_name_with_slash(tmp_path) -> None: + data = { + "uuid": "12345678-1234-5678-1234-567812345678", + "organisation": "InterDiode", + "configurations": { + "certificates": [{"name": "../../evil", "content": CERT_CONTENT}] + }, + } + dst = tmp_path / "dst" + context = Context(config=Configuration.model_validate(data), dst=dst, src=None) + + CertificatesModule().apply(context) + + local_cert = dst / "usr/local/share/ca-certificates/.._.._evil.crt" + assert local_cert.read_bytes() == b"fake-cert-bytes" + # The "/" must never be interpreted as a path separator: without + # sanitizing, "../../evil" would have reshuffled the write up to + # usr/local/evil.crt instead of staying under ca-certificates/. + assert not (dst / "usr/local/evil.crt").exists() + + def test_apply_merges_conf_listed_and_local_certificates(tmp_path) -> None: src = tmp_path / "src" local_certs_dir = src / "usr/local/share/ca-certificates" diff --git a/tests/test_modules_base.py b/tests/test_modules_base.py index 109d482..6eba11d 100644 --- a/tests/test_modules_base.py +++ b/tests/test_modules_base.py @@ -1,5 +1,10 @@ from id_configurator.modules import base as base_module -from id_configurator.modules.base import Module, all_modules, register +from id_configurator.modules.base import ( + Module, + all_modules, + register, + sanitize_filename, +) def test_register_and_all_modules_are_isolated(monkeypatch) -> None: @@ -17,3 +22,15 @@ def test_register_and_all_modules_are_isolated(monkeypatch) -> None: register(DummyModule) assert all_modules() == [DummyModule] + + +def test_sanitize_filename_leaves_plain_names_untouched() -> None: + assert sanitize_filename("interdiode") == "interdiode" + + +def test_sanitize_filename_replaces_forward_slash() -> None: + assert sanitize_filename("../../etc/passwd") == ".._.._etc_passwd" + + +def test_sanitize_filename_replaces_backslash() -> None: + assert sanitize_filename(r"a\b") == "a_b" diff --git a/tests/test_yum.py b/tests/test_yum.py index 4e3dc1f..fa727a4 100644 --- a/tests/test_yum.py +++ b/tests/test_yum.py @@ -165,6 +165,36 @@ def test_apply_disables_existing_repos_before_adding_configured_ones(tmp_path) - ) +def test_apply_sanitizes_repository_and_key_names_with_slash( + tmp_path, monkeypatch +) -> None: + mock_run = _mock_gpg_success(monkeypatch) + data = { + "uuid": "12345678-1234-5678-1234-567812345678", + "organisation": "InterDiode", + "configurations": { + "yum": { + "repositories": [ + { + "name": "../../evil-repo", + "index-url": "https://example.invalid/yum", + } + ], + "keys": [{"name": "../../evil-key", "content": KEY_CONTENT}], + } + }, + } + dst = tmp_path / "dst" + context = Context(config=make_config(data), dst=dst, src=None) + + YumModule().apply(context) + + assert (dst / "etc/yum.repos.d/.._.._evil-repo.repo").exists() + key_content = (dst / "etc/pki/rpm-gpg/RPM-GPG-KEY-.._.._evil-key").read_bytes() + assert key_content == b"fake-rpm-gpg-key-bytes" + mock_run.assert_called_once() + + def test_import_key_raises_when_gpg_binary_missing(tmp_path, monkeypatch) -> None: monkeypatch.setattr( "id_configurator.modules.yum.subprocess.run",