treewide: add challenge config for privileged - #89
Conversation
There was a problem hiding this comment.
Pull request overview
Adds per-challenge configuration to pwnshop to allow running certain challenges’ Docker containers in --privileged mode (configured via challenge.yml), and enables this mode for a set of legacy “intercepting-communication” challenges.
Changes:
- Add
challenge.ymlparsing (load_challenge_config) and aprivilegedflag tolib.run_challenge. - Wire the
privilegedsetting intopwnshop runandpwnshop test. - Add
pyyamldependency and introducechallenge.ymlfiles settingprivileged: truefor specific challenges.
Reviewed changes
Copilot reviewed 33 out of 34 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/pwnshop/uv.lock | Bumps lock revision and adds PyYAML package metadata. |
| tools/pwnshop/src/pwnshop/lib/init.py | Adds YAML config loading and switches Docker run options based on privileged. |
| tools/pwnshop/src/pwnshop/commands/test.py | Loads per-challenge config and passes privileged into container runner. |
| tools/pwnshop/src/pwnshop/commands/run.py | Loads per-challenge config and passes privileged into container runner. |
| tools/pwnshop/pyproject.toml | Adds pyyaml dependency. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/udp-spoof-host/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/udp-spoof-host-4/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/udp-spoof-host-3/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/udp-spoof-host-2/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/udp-2/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/udp-1/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/sniff-cookie/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/level-9/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/level-8/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/level-7/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/level-6/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/level-5/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/level-4/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/level-3/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/level-2/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/level-1b/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/level-1a/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/level-14/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/level-13/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/level-12/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/level-11/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/level-10/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/level-1/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/firewall-3/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/firewall-2/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/firewall-1/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/dos-3/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/dos-2/challenge.yml | Enables privileged mode for this challenge. |
| challenges/legacy/intro-to-cybersecurity/intercepting-communication/dos-1/challenge.yml | Enables privileged mode for this challenge. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def load_challenge_config(challenge_path: pathlib.Path) -> Dict[str, Any]: | ||
| config = dict(CHALLENGE_CONFIG_DEFAULTS) | ||
| config_file = challenge_path / "challenge.yml" | ||
| if config_file.is_file(): | ||
| logger.debug("loading challenge config from %s", config_file) | ||
| with open(config_file) as f: | ||
| user_config = yaml.safe_load(f) or {} | ||
| config.update(user_config) | ||
| return config |
There was a problem hiding this comment.
yaml.safe_load() can return non-mapping values (e.g., a list/string) or a mapping with unexpected types. In those cases config.update(user_config) will raise at runtime, and privileged could end up non-bool (e.g., quoted "false" becomes a truthy string). Consider validating that user_config is a dict and that user_config.get("privileged") is either absent or a bool (otherwise raise a clear error or ignore with a warning).
| logger.debug("loading challenge config from %s", config_file) | ||
| with open(config_file) as f: | ||
| user_config = yaml.safe_load(f) or {} |
There was a problem hiding this comment.
This reads challenge.yml with open(config_file) using the platform default encoding. To avoid locale-dependent failures, prefer config_file.open(encoding="utf-8") (or an explicit encoding consistent with the rest of the file loader logic).
| @@ -159,8 +177,7 @@ def run_challenge( | |||
| "--detach", | |||
| "--init", | |||
| "--user=0:0", | |||
| "--device=/dev/kvm", | |||
| "--cap-add=SYS_PTRACE", | |||
| *(["--privileged"] if privileged else ["--device=/dev/kvm", "--cap-add=SYS_PTRACE"]), | |||
There was a problem hiding this comment.
Allowing challenge.yml to enable --privileged means an untrusted PR can cause CI to run fully privileged containers on the GitHub runner. That materially increases the blast radius vs the previous --device=/dev/kvm + --cap-add=SYS_PTRACE. Consider gating privileged mode behind an explicit CLI flag / env var that is disabled in CI by default (or an allowlist of known-safe challenge paths), and fail with a clear message when privileged is requested but not permitted.
There was a problem hiding this comment.
I don't think we actually care about the integrity of the CI host, at least not currently.
There was a problem hiding this comment.
Are we sure we adequately wipe the decryption keys? :-)
6015e47 to
71d5b6c
Compare
|
I'm currently working on adding kata support to this repo, do we want to hold off for that? For example, some challenges make demands of what features are available in the kernel (landlock, ebpf, etc). Also no tests were actually run here. I just manually triggered a run: https://github.com/pwncollege/challenges/actions/runs/21769956407. This causes all challenges to be tested, which is generally what we want to do when we make bigger changes (maybe we can automatically detect this somehow). |
|
Oh, if you're implementing kata, let's go with that instead of this PR. |
|
Resolved by #111 |
No description provided.