Repository navigation
Harden cloud workshop runner against untrusted student code - #293
Open
AydinHassan wants to merge 1 commit into
Open
AydinHassan wants to merge 1 commit into
AydinHassan wants to merge 1 commit into
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
The cloud workshop (on
online/ PR #239) ran student-submitted PHP as plainphp solution.phporphp-cgi, as the PHP-FPM user, with no restrictions. The only limit was the 10s parent timeout. Any GitHub-authenticated student could:/var/www/phpschool.io/shared/.env, which holds the DB password,JWT_SECRET(enough to mint admin JWTs) and the GitHub, SendGrid and Slack tokensshell_exec/proc_opendetached processes that outlive the timeoutcomposer_depsto makecomposer updaterun arbitrary packages, plugins included, inside the FPM requestDeployment status:
masteris what Deployer deploys (deploy.phppinsmasterand the workflow triggers onmaster), 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 targetsonlineso the problem is fixed before #239 merges. Koshki lists the deploy branch asonline, which disagrees withdeploy.php; that entry should be corrected.What this changes
src/Online/Sandbox).CloudInstalledWorkshopswaps php-workshop'sRunnerManagerforSandboxedRunnerFactory, 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/symlinkmemory_limit=128M,max_execution_time=10,allow_url_fopen=0,allow_url_include=0,ffi.enable=0,enable_dl=0, andsys_temp_dir/session.save_pathpointed at the project dirgetenv(),$_ENVand$_SERVERkey is unset, so dotenv secrets no longer reach child processesecho … | php-cgishell pipelinecomposer_depsis validated byComposerPackageAllowlist:var/packages.json(from the existingdownload-composer-packagescommand). If that file is missing, all packages are rejected.--no-plugins --no-scripts,allow-plugins: false, a 120s timeout and a stripped environment.RunExerciseandVerifyExerciseremove the project directory infinally.RunExerciseused to leak it on theCouldNotRunExceptionpath. Malformedscriptsandcomposer_depspayloads now get a clean error instead of a TypeError that left the directory behind.php-school/php-workshopmaster already has a configurableProcessFactory(plus aDockerProcessFactory), 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/SandboxedCgiRunnerTestrun realphp/php-cgiprocesses. They check that:ini_setat runtimeCloudInstalledWorkshopTest::testStudentCodeIsRunInTheSandboxchecks the wiring end to end through the real framework container.ProjectUploaderTestcovers packages outside the allowlist, bad names and versions, too many packages, malformed payloads, a missing package list, and cleanup after a composer failure.RunExerciseTest/VerifyExerciseTestcheck the project dir is removed when the dispatcher throws or can't run.What this does NOT fix
open_basediranddisable_functionsare hardening, not a security boundary. Students still run code as the FPM user on the app host.disable_classesis gone:new Redis(),new mysqli(),new PDO('mysql:…'),SoapClient, and rawsocket_createfor listening. Local Redis is reachable if it has no password. MySQL needs credentials, which are no longer readable.Plan for the real fix
--network none(loopback only for TimeServer)--cap-drop ALL,no-new-privilegesand seccomp--memory 128m --cpus 0.5 --pids-limit 32, with a hard wall-clock killphp-school/php-workshopto master and use itsProcessFactoryhook, extendingDockerProcessFactorywith the flags above. Alternatively, have the website call a small runner service over HTTP on the isolated host.Manual actions
shared/.envand 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 ofonlinewas ever deployed (Capistrano could deploy any branch), treat every secret as exposed and check logs for/api/online/workshop/run|verify.bin/app download-composer-packagesruns, via cron or deploy, wherever the cloud workshop is deployed, otherwise all composer deps are rejected.🤖 Generated with Claude Code
https://claude.ai/code/session_014zXtVDd6uQ3wyds72NfxP4