Skip to content

Harden cloud workshop runner against untrusted student code - #293

Open
AydinHassan wants to merge 1 commit into
onlinefrom
harden-cloud-runner
Open

AydinHassan wants to merge 1 commit into
onlinefrom
harden-cloud-runner

Conversation

@AydinHassan

Copy link
Copy Markdown
Member

Context

The cloud workshop (on online / PR #239) ran student-submitted PHP as plain php solution.php or php-cgi, as the PHP-FPM user, with no restrictions. The only limit was the 10s parent timeout. Any GitHub-authenticated student could:

  • read /var/www/phpschool.io/shared/.env, which holds the DB password, JWT_SECRET (enough to mint admin JWTs) and the GitHub, SendGrid and Slack tokens
  • reach Redis and MySQL on localhost
  • shell_exec/proc_open detached processes that outlive the timeout
  • use composer_deps to make composer update run arbitrary packages, plugins included, inside the FPM request

Deployment status: master is what Deployer deploys (deploy.php pins master and the workflow triggers on master), and it does not contain the cloud runner. Every /api/online/* route returns 404 on www.phpschool.io, so the vulnerable code isn't live today, as far as I can tell. This PR therefore targets online so the problem is fixed before #239 merges. Koshki lists the deploy branch as online, which disagrees with deploy.php; that entry should be corrected.

What this changes

  • Sandboxed runners (src/Online/Sandbox). CloudInstalledWorkshop swaps php-workshop's RunnerManager for SandboxedRunnerFactory, so student CLI and CGI runs get:
    • open_basedir = the student's project dir plus any exercise argument paths under the php-school temp dir (MyFirstIo, FilteredLs and the sqlite DSN)
    • disable_functions: exec/system/passthru/shell_exec/proc_open/popen, pcntl_exec/fork, posix_kill/setsid/setuid…, putenv, mail, error_log, dl, fsockopen, stream_socket_client, socket_connect/sendto, curl_exec, ftp/imap/ldap connects, mysqli_connect, DNS lookups, link/symlink
    • memory_limit=128M, max_execution_time=10, allow_url_fopen=0, allow_url_include=0, ffi.enable=0, enable_dl=0, and sys_temp_dir/session.save_path pointed at the project dir
    • an environment with nothing inherited: every getenv(), $_ENV and $_SERVER key is unset, so dotenv secrets no longer reach child processes
    • php-cgi run without the old echo … | php-cgi shell pipeline
    • Reference solutions are trusted and run unsandboxed, but still get the stripped environment.
  • Composer. composer_deps is validated by ComposerPackageAllowlist:
    • names must match composer's name regex and appear in var/packages.json (from the existing download-composer-packages command). If that file is missing, all packages are rejected.
    • version constraints must match a safe character set, and at most 10 packages are allowed.
    • the install runs with --no-plugins --no-scripts, allow-plugins: false, a 120s timeout and a stripped environment.
    • if composer fails, the upload fails and the project directory is removed.
  • Cleanup. RunExercise and VerifyExercise remove the project directory in finally. RunExercise used to leak it on the CouldNotRunException path. Malformed scripts and composer_deps payloads now get a clean error instead of a TypeError that left the directory behind.

php-school/php-workshop master already has a configurable ProcessFactory (plus a DockerProcessFactory), but this branch pins an older commit and upgrading means a larger refactor. So the change stays inside phpschool.io and doesn't need anything upstream.

Tests

  • SandboxedCliRunnerTest / SandboxedCgiRunnerTest run real php/php-cgi processes. They check that:
    • reading outside the project dir fails, as does reading app files or loosening open_basedir with ini_set at runtime
    • shell, process, putenv, mail and socket functions are undefined
    • the limits are applied and a dotenv-style secret isn't inherited
    • exercise argument paths are still readable
  • CloudInstalledWorkshopTest::testStudentCodeIsRunInTheSandbox checks the wiring end to end through the real framework container.
  • ProjectUploaderTest covers packages outside the allowlist, bad names and versions, too many packages, malformed payloads, a missing package list, and cleanup after a composer failure.
  • RunExerciseTest / VerifyExerciseTest check the project dir is removed when the dispatcher throws or can't run.
  • A mutation check with the sandbox arguments emptied makes 14 of the sandbox tests fail.
  • The full suite passes: 171 tests on PHP 8.4, with the CLI sandbox tests also run on 8.3. phpstan max and php-cs-fixer are clean.
  • Every learn-you-php and php8-appreciate exercise was verified with its own reference solution as the student submission. Results are identical before and after this change. Database Read, FilteredLs, MyFirstIo, Array We Go!, the CGI exercises and the composer exercises all still pass.

What this does NOT fix

open_basedir and disable_functions are hardening, not a security boundary. Students still run code as the FPM user on the app host.

  • Network. Class-based clients can't be disabled on PHP 8.5, where disable_classes is gone: new Redis(), new mysqli(), new PDO('mysql:…'), SoapClient, and raw socket_create for listening. Local Redis is reachable if it has no password. MySQL needs credentials, which are no longer readable.
  • PHP bypasses. open_basedir and disable_functions have a long bypass history, including interpreter memory-corruption bugs. One bypass means full FPM-user access again.
  • Resources. There are no pids or CPU cgroup limits, and no disk quota (a student can fill the temp dir). The 10s timeout and memory_limit bound each run.
  • Composer. Packages are still arbitrary public Packagist packages, and downloads happen on the app host.
  • Uploads. Uploaded files still land in the shared system temp dir, readable by the FPM user.

Plan for the real fix

  1. Run each student execution, composer install included, in a throwaway sandbox on a host (or VM) that has no app secrets or DB/Redis access. Options are Docker/Podman with gVisor, nsjail or bubblewrap, using:
    • --network none (loopback only for TimeServer)
    • a read-only rootfs, with only the project dir mounted read-write as a size-capped tmpfs
    • a non-root UID, --cap-drop ALL, no-new-privileges and seccomp
    • cgroup limits such as --memory 128m --cpus 0.5 --pids-limit 32, with a hard wall-clock kill
  2. Install composer deps in a separate sandbox step that has network access only to Packagist/GitHub, sharing a read-only cache. Or pre-bake the packages the exercises need and drop arbitrary deps.
  3. Implementation path: upgrade php-school/php-workshop to master and use its ProcessFactory hook, extending DockerProcessFactory with the flags above. Alternatively, have the website call a small runner service over HTTP on the isolated host.
  4. Keep this PR's ini hardening inside the container as an extra layer.

Manual actions

  • Rotate all prod secrets in shared/.env and anywhere else the FPM user can read: DB password, JWT_SECRET, GitHub OAuth client secret and token, SendGrid key, Slack token. The live site 404s the cloud endpoints, but this needs checking. If any build of online was ever deployed (Capistrano could deploy any branch), treat every secret as exposed and check logs for /api/online/workshop/run|verify.
  • Make sure bin/app download-composer-packages runs, via cron or deploy, wherever the cloud workshop is deployed, otherwise all composer deps are rejected.
  • Put a password on Redis, bind it to localhost, and make sure the MySQL user is limited to the app DB.
  • Don't merge or deploy PHP School Cloud #239 until the container isolation above is in place, or at least until this PR is in.

🤖 Generated with Claude Code

https://claude.ai/code/session_014zXtVDd6uQ3wyds72NfxP4

Student code submitted to the cloud workshop was executed with plain
`php`/`php-cgi` as the PHP-FPM user with no restrictions, so it could
read the shared .env (DB password, JWT secret, API tokens), reach local
services and spawn processes that outlive the timeout.

- Run student CLI/CGI processes through sandboxed runners that pass
  open_basedir (solution dir + exercise temp paths), disable_functions
  (process, shell, mail, putenv, outbound socket, DNS), memory_limit,
  max_execution_time, allow_url_fopen=0, ffi.enable=0, enable_dl=0.
- Strip the inherited environment (including dotenv $_ENV) from student,
  reference and composer processes.
- Run php-cgi without a shell pipeline.
- Validate composer_deps: Packagist-list allowlist (fail closed when the
  list is missing), package name/version format, max 10 packages;
  install with --no-plugins --no-scripts, allow-plugins=false, a timeout
  and a stripped env; fail the upload if composer fails.
- Always remove the uploaded project dir (try/finally) in run/verify,
  including the CouldNotRunException path that previously leaked it.

This is defence in depth, not isolation. See the PR for what it does not
cover and the plan for per-run container isolation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014zXtVDd6uQ3wyds72NfxP4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant