fix: validate GCP project and region before interpolating into deploy Dockerfile - #6930
Open
prasanna8585 wants to merge 1 commit into
Open
Conversation
… Dockerfile Independent finding, found while checking a same-day fix (commit 6eb1d35, "fix: validate app_name before interpolating into deploy Dockerfile") for completeness during a routine commit-batch audit. That fix closed a Dockerfile-instruction-injection vulnerability where an unvalidated app_name was spliced verbatim into _DOCKERFILE_TEMPLATE's COPY instructions and CMD. Checking for sibling gaps found the same template also interpolates --project and --region verbatim into ENV GOOGLE_CLOUD_PROJECT={gcp_project_id} ENV GOOGLE_CLOUD_LOCATION={gcp_region} with zero validation, across all three deploy targets (to_cloud_run, to_agent_engine, to_gke). Dynamically confirmed with the real, unmodified _DOCKERFILE_TEMPLATE extracted from the source: a --project value containing a newline followed by a RUN instruction produced a generated Dockerfile where that RUN instruction appeared as its own, independent line -- meaning docker build processing that Dockerfile would execute the attacker-supplied command as part of the build. Fix adds _validate_gcp_project_id/_validate_gcp_region, mirroring the already-merged _validate_app_name (same character-set-only restriction, deliberately not attempting to fully replicate GCP's own project-ID length/format rules, since the security goal is excluding characters that can break out of a Dockerfile instruction). Applied at all three deploy functions. In to_agent_engine specifically, project is validated only after its own onboarding flow (triggered when --project is not supplied) has had a chance to run and resolve a real value -- validating immediately after the initial _resolve_project() call incorrectly rejected that legitimate empty-then-resolved-later case during development. Verified: re-ran the PoC against the patched validation -- the malicious --project value is now rejected with a clear error before reaching the template. Added 8 new regression tests: acceptance of plain identifiers (including the existing suite's own short/ underscored fixtures, e.g. "proj", "fake_region"), rejection of the injection payload and several other unsafe characters, and end-to-end rejection through to_cloud_run and to_gke. Full existing test_cli_deploy.py suite: 117/117 pass (109 pre-existing + 8 new), no regressions.
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
2 participants
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.
_DOCKERFILE_TEMPLATEinterpolates--projectand--regionverbatim intoENVinstructions:Neither value is validated before reaching the template, across all three deploy targets (
to_cloud_run,to_agent_engine,to_gke). A value containing a newline breaks out of theENVinstruction's line, injecting an arbitrary new Dockerfile instruction — confirmed directly against the real template: a--projectvalue with an embedded newline followed by aRUNinstruction produced a generated Dockerfile where thatRUNappeared as its own, independent line.docker buildprocessing that file would execute the injected command.This adds
_validate_gcp_project_id/_validate_gcp_region, restricting both to a safe identifier character set (letters, digits, hyphens, underscores) before they reach the template — mirroring the existingapp_namevalidation this file already applies for the same reason.Testing: Added 8 new tests covering acceptance of realistic project/region values, rejection of the injection payload and other unsafe characters (quotes, spaces, semicolons, pipes, command substitution), and end-to-end rejection through
to_cloud_runandto_gkewith a malicious project value. Full existing test suite: 117/117 pass, no regressions.