fix(security): close remaining /bin/sh -c shell-injection sites in bundle ID flows#390
Open
voidborne-d wants to merge 1 commit intogetsentry:mainfrom
Open
Conversation
…ndle ID flows Follow-up to getsentry#289. PR getsentry#289 hardened the `useShell=true` path through `shellEscapeArg`, but four call sites still hand-built `/bin/sh -c` strings interpolating user-controlled paths and passed them to the executor with `useShell=false`: - `src/utils/bundle-id.ts` - `src/mcp/tools/project-discovery/get_mac_bundle_id.ts` - `src/mcp/tools/macos/build_macos.ts` - `src/utils/macos-steps.ts` All four now invoke `defaults` and `PlistBuddy` directly with an argv array, removing shell parsing entirely (option 1 from the issue). The malicious appPath becomes a single positional argument to `defaults` / `PlistBuddy` and is never interpreted as a shell expression. Tests: - The four `UNFIXED:` regression cases in `bundle-id-injection.test.ts` and `mac-bundle-id-injection.test.ts` are flipped to safe assertions (no `/bin/sh`, exact argv shape, `useShell=false`). - New PlistBuddy-fallback cases assert the same shape on the second branch. - Existing fixtures in `get_app_bundle_id.test.ts`, `get_mac_bundle_id.test.ts`, and `build_run_device.test.ts` updated to the new argv-shape command keys. - Pre-fix verification: stashed the four source edits and re-ran the injection suites — 8/8 fail. Restored the fix — 8/8 pass. Local gates: typecheck, lint, prettier --check, full vitest run (1772 passed / 0 failed). Closes getsentry#367
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.
Follow-up to #289 that closes the four shell-injection sites called out in #367.
Problem
PR #289 routed the
useShell=truepath ofdefaultExecutorthroughshellEscapeArg. That fix does not cover call sites that build their own/bin/sh -ccommand strings and hand them to the executor withuseShell=falseas['/bin/sh', '-c', command]. Those sites interpolate user-controlled paths into the shell string and are vulnerable to CWE-78.Four sites are flagged in #367 and confirmed on
main:src/utils/bundle-id.ts—defaults read \"${appPath}/Info\" CFBundleIdentifierand the PlistBuddy fallbacksrc/mcp/tools/project-discovery/get_mac_bundle_id.ts— same pattern,Contents/Infoplistsrc/mcp/tools/macos/build_macos.ts— same pattern in the post-build bundle-ID extractionsrc/utils/macos-steps.ts— same pattern afteropen(used bylaunch_mac_appandbuild_run_macos)The accompanying
UNFIXED:regression tests inbundle-id-injection.test.tsandmac-bundle-id-injection.test.tsdocumented the live attack vectors (\$(id),; rm -rf /, backticks).Fix
Each site now invokes
defaultsorPlistBuddydirectly with an argv array, picking option (1) from the issue ("stop using/bin/sh -c") rather than option (2) (shellEscapeArg-everywhere). Removing shell parsing entirely is the smaller, safer surface — there is nothing left to escape, and we do not need to keep auseShellplumbing path alive for these helpers.```ts
// before
await executor(
['/bin/sh', '-c', `defaults read "${appPath}/Info" CFBundleIdentifier`],
'Bundle ID Extraction',
);
// after
await executor(
['defaults', 'read', `${appPath}/Info`, 'CFBundleIdentifier'],
'Bundle ID Extraction',
false,
);
```
defaultsand/usr/libexec/PlistBuddyaccept paths and key arguments positionally, so no shell features are needed. A maliciousappPathbecomes a single literal argv element and is never seen by a shell parser.Tests
UNFIXED:regression cases are flipped to safe assertions: command does not start with/bin/sh,useShellisfalse, the argv array matches the expecteddefaults/PlistBuddyshape exactly.defaultsreturns failure).get_app_bundle_id.test.ts,get_mac_bundle_id.test.ts, andbuild_run_device.test.tsupdated to the new argv-shape command keys (thecreateCommandMatchingMockExecutormatches oncommand.join(' '), so the old double-quoted strings no longer match the new argv form).Pre-fix verification
```
$ git stash push -- src/utils/bundle-id.ts \
src/mcp/tools/project-discovery/get_mac_bundle_id.ts \
src/mcp/tools/macos/build_macos.ts \
src/utils/macos-steps.ts
$ npx vitest run src/utils/tests/bundle-id-injection.test.ts \
src/utils/tests/mac-bundle-id-injection.test.ts
... 8 failed (8) ...
$ git stash pop
$ npx vitest run src/utils/tests/bundle-id-injection.test.ts \
src/utils/tests/mac-bundle-id-injection.test.ts
... 8 passed (8) ...
```
Local gates
Notes
defaultExecutoritself; the existinguseShell=truehardening in fix: CWE-78 shell injection via unsafe argument escaping in command executor #289 stays as-is for the call sites that genuinely need it.Closes #367