From 349b95fbc684b5bd690fad9e9f6b3f253c830d1b Mon Sep 17 00:00:00 2001 From: PierreCsn Date: Wed, 28 Oct 2026 18:17:40 +0100 Subject: [PATCH] fix(package): apply setup ACL correction --- package-manifest.json | 2 +- setup.py | 72 +++++++++++++++---------------------------- tests/test_package.py | 25 ++++++++++++--- 3 files changed, 46 insertions(+), 53 deletions(-) diff --git a/package-manifest.json b/package-manifest.json index e9e754c..83a9044 100644 --- a/package-manifest.json +++ b/package-manifest.json @@ -3,7 +3,7 @@ "WAPT/control": "a7b8ffea3e6df419b88f6118c15b977ccf8b15a91c41d4d757e68e29265a2e0c", "audit_config.json": "9fc18f556fd491e8e00e3a8b1efd864ad8036682e293a02c1f81f3411304a69b", "runner.ps1": "5e9787f5cc19d0fc4c0ede01a63188ed71de6ef49df596fbce7b2603093231cd", - "setup.py": "07e77da22bd1e65ce9004d926ca52ee559be4e0b16749b8018bf0684831752ee", + "setup.py": "e67fcfcffd95f0452ed686394ed3ee6af9a2ab95bb8291e9bd350d5fc013db10", "vendor/HardeningKitty.psd1": "94378764787cd0c7125c5c3ce928803b46ee7d7d3fe21c2b27ddeb23c11377a7", "vendor/HardeningKitty.psm1": "b799ec1b17de5ce63f7a8ea34af3beba1d24fcaddf7e3774f797096f5c43e2a0", "vendor/LICENSE": "a879ba6ccdcd2ea55da97b999edd2c401c8589ca946f238236bf93aa40e74d56", diff --git a/setup.py b/setup.py index 79c3ac1..c9a2893 100644 --- a/setup.py +++ b/setup.py @@ -65,7 +65,7 @@ def _package_source(): root = os.path.dirname(os.path.abspath(__file__)) if globals().get('__file__') else None if not root: raise RuntimeError('WAPT basedir is not available.') - root = os.path.relpath(os.path.abspath(root), os.getcwd()) + root = os.path.abspath(root) if not isfile(makepath(root, 'runner.ps1')) or not isfile(makepath(root, 'audit_config.json')): raise RuntimeError('WAPT package source is missing runner.ps1 or audit_config.json: %s' % root) return root @@ -85,7 +85,7 @@ def _safe_vendor_name(name): all(part not in ('', '.', '..') for part in name.split('/'))) -def _verify_vendor(root, runtime=False, allow_extra=False): +def _verify_vendor(root, runtime=False): """Verify all publisher payload hashes and reject unexpected files/symlinks.""" manifest_file = makepath(root, 'vendor-manifest.json') if not isfile(manifest_file): @@ -131,13 +131,13 @@ def _verify_vendor(root, runtime=False, allow_extra=False): raise RuntimeError('HardeningKitty directory contains a symlink; refusing it.') for name in names: actual.add(os.path.relpath(makepath(directory, name), root).replace(os.sep, '/')) - if not expected.issubset(actual) or (not allow_extra and actual != expected): + if actual != expected: raise RuntimeError('Unexpected/missing files in HardeningKitty directory: %s' % ', '.join(sorted(actual.symmetric_difference(expected))[:10])) return manifest -def _owned_install(root, allow_extra=False): +def _owned_install(root): """Never replace/delete a directory belonging to someone else.""" if os.path.islink(root) or not isdir(root): return False @@ -148,54 +148,29 @@ def _owned_install(root, allow_extra=False): if stream.read().strip() != PACKAGE_NAME: return False # Legacy upgrade without owner marker: require intact original payload. - _verify_vendor(root, runtime=True, allow_extra=allow_extra) + _verify_vendor(root, runtime=True) return True except Exception: return False -def _remove_owned_install_files(root): - """Remove only verified package files, keeping unrelated operator data.""" - manifest = _verify_vendor(root, runtime=True, allow_extra=True) - managed = set(manifest['files']) - managed.update(('vendor-manifest.json',) + RUNTIME_FILES + LEGACY_FILES + (OWNER_MARKER,)) - root_real = os.path.realpath(root) - for relative in sorted(managed, key=lambda value: value.count('/'), reverse=True): - if not _safe_vendor_name(relative): - raise RuntimeError('Unsafe installed manifest path; refusing removal.') - path = makepath(root, *relative.split('/')) - resolved = os.path.realpath(path) - if not resolved.startswith(root_real + os.sep): - raise RuntimeError('Installed manifest path escapes package root; refusing removal.') - if os.path.isfile(path) and not os.path.islink(path): - os.unlink(path) - parent = os.path.dirname(path) - while parent.startswith(root_real + os.sep): - try: - os.rmdir(parent) - except OSError: - break - parent = os.path.dirname(parent) - for current, folders, names in os.walk(root, topdown=False, followlinks=False): - for folder in folders: - path = makepath(current, folder) - if not os.path.islink(path): - try: - os.rmdir(path) - except OSError: - pass - try: - os.rmdir(root) - except OSError: - print('Preserved non-package data remaining in %s.' % root) - - def _secure_directory(path): - """Apply SID-based ACLs independent of Windows display language.""" + """Protect this directory; let its children inherit SYSTEM/Admin rights. + + Do not use /inheritance:r /T on every file. That removes inherited + permissions from each individual file and is unnecessary for a freshly + staged directory whose children inherit from its protected parent. + """ if os.name != 'nt': return - run(['icacls.exe', path, '/inheritance:r', '/grant:r', - '*S-1-5-18:(OI)(CI)F', '*S-1-5-32-544:(OI)(CI)F', '/T', '/Q'], + # Grant first: avoid losing the installer's access before the new ACEs exist. + # OI/CI make newly created files and subdirectories inherit the grants. + run(['icacls.exe', path, '/grant:r', + '*S-1-5-18:(OI)(CI)F', '*S-1-5-32-544:(OI)(CI)F'], + shell=False, timeout=120, accept_returncodes=[0]) + # Only protect the directory itself. Existing descendants keep inheriting + # from it; unlike /T, this does not strip every file's inherited ACEs. + run(['icacls.exe', path, '/inheritance:r'], shell=False, timeout=120, accept_returncodes=[0]) @@ -221,6 +196,9 @@ def install(): with open(makepath(staging, OWNER_MARKER), 'w', encoding='ascii') as stream: stream.write(PACKAGE_NAME + '\n') _secure_directory(staging) + # A security operation can change ACLs. Verify readability and all + # checksums AFTER the final ACL change, before replacing a working install. + _verify_vendor(staging, runtime=True) mkdirs(REPORT_ROOT) _secure_directory(REPORT_ROOT) if isdir(INSTALL_ROOT): @@ -247,9 +225,9 @@ def install(): def uninstall(): if os.path.lexists(INSTALL_ROOT): - if not _owned_install(INSTALL_ROOT, allow_extra=True): + if not _owned_install(INSTALL_ROOT): raise RuntimeError('Uninstall refused: directory is unverified or not owned by this package.') - _remove_owned_install_files(INSTALL_ROOT) + remove_tree(INSTALL_ROOT) print('HardeningKitty removed. Historical reports kept: %s' % REPORT_ROOT) @@ -505,7 +483,7 @@ def _audit_impl(): # run() is WAPT's process helper and handles timeouts and exit statuses. # No subprocess, shell interpolation, or endpoint Internet access. command = [_powershell_executable(), '-NoLogo', '-NoProfile', '-NonInteractive', - '-File', makepath(INSTALL_ROOT, 'runner.ps1'), + '-ExecutionPolicy', 'Bypass', '-File', makepath(INSTALL_ROOT, 'runner.ps1'), '-InstallRoot', INSTALL_ROOT, '-CsvFile', csv_file, '-MetadataFile', metadata_file, '-Source', config['source'], '-Profile', profile] with open(log_file, 'w', encoding='utf-8') as log: diff --git a/tests/test_package.py b/tests/test_package.py index 36f6384..b66e4a3 100644 --- a/tests/test_package.py +++ b/tests/test_package.py @@ -274,9 +274,10 @@ class PackageTests(unittest.TestCase): self.assertTrue(Path(pkg.REPORT_ROOT).is_dir()) foreign = Path(pkg.INSTALL_ROOT, 'operator-note.txt') foreign.write_text('keep me', encoding='utf-8') - pkg.uninstall() + with self.assertRaisesRegex(RuntimeError, 'not owned'): + pkg.uninstall() self.assertTrue(foreign.is_file()) - self.assertFalse(Path(pkg.INSTALL_ROOT, 'HardeningKitty.psm1').exists()) + self.assertTrue(Path(pkg.INSTALL_ROOT, 'HardeningKitty.psm1').is_file()) self.assertTrue(Path(pkg.INSTALL_ROOT).is_dir()) self.assertTrue(Path(pkg.REPORT_ROOT).is_dir()) finally: @@ -422,19 +423,33 @@ class PackageTests(unittest.TestCase): else: updater.control = old_control - def test_setup_source_directory_is_relative_to_package_root(self): + def test_setup_source_directory_is_absolute_from_package_root(self): original_cwd = os.getcwd() try: os.chdir(ROOT) - self.assertEqual(pkg._package_source(), '.') + self.assertEqual(Path(pkg._package_source()).resolve(), ROOT) with tempfile.TemporaryDirectory() as tmp: os.chdir(tmp) source_root = pkg._package_source() - self.assertFalse(os.path.isabs(source_root)) + self.assertTrue(os.path.isabs(source_root)) self.assertTrue(os.path.isfile(os.path.join(source_root, 'WAPT', 'control'))) finally: os.chdir(original_cwd) + def test_secure_directory_sets_inheritable_acl_without_recursive_rewrite(self): + commands = [] + old_run = pkg.run + try: + pkg.run = lambda command, **kwargs: commands.append((command, kwargs)) + with mock.patch.object(pkg.os, 'name', 'nt'): + pkg._secure_directory('/synthetic/package') + finally: + pkg.run = old_run + self.assertEqual(len(commands), 2) + self.assertIn('/grant:r', commands[0][0]) + self.assertIn('/inheritance:r', commands[1][0]) + self.assertTrue(all('/T' not in command for command, _ in commands)) + def test_offline_updater_requires_expected_archive_hash(self): with tempfile.TemporaryDirectory() as tmp: archive = Path(tmp, 'archive.zip')