Skip to content

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
voidborne-d:fix/shell-injection-bundle-id-sites
Open

fix(security): close remaining /bin/sh -c shell-injection sites in bundle ID flows#390
voidborne-d wants to merge 1 commit intogetsentry:mainfrom
voidborne-d:fix/shell-injection-bundle-id-sites

Conversation

@voidborne-d
Copy link
Copy Markdown

Follow-up to #289 that closes the four shell-injection sites called out in #367.

Problem

PR #289 routed the useShell=true path of defaultExecutor through shellEscapeArg. That fix does not cover call sites that build their own /bin/sh -c command strings and hand them to the executor with useShell=false as ['/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.tsdefaults read \"${appPath}/Info\" CFBundleIdentifier and the PlistBuddy fallback
  • src/mcp/tools/project-discovery/get_mac_bundle_id.ts — same pattern, Contents/Info plist
  • src/mcp/tools/macos/build_macos.ts — same pattern in the post-build bundle-ID extraction
  • src/utils/macos-steps.ts — same pattern after open (used by launch_mac_app and build_run_macos)

The accompanying UNFIXED: regression tests in bundle-id-injection.test.ts and mac-bundle-id-injection.test.ts documented the live attack vectors (\$(id), ; rm -rf /, backticks).

Fix

Each site now invokes defaults or PlistBuddy directly 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 a useShell plumbing 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,
);
```

defaults and /usr/libexec/PlistBuddy accept paths and key arguments positionally, so no shell features are needed. A malicious appPath becomes a single literal argv element and is never seen by a shell parser.

Tests

  • The 4 UNFIXED: regression cases are flipped to safe assertions: command does not start with /bin/sh, useShell is false, the argv array matches the expected defaults / PlistBuddy shape exactly.
  • New PlistBuddy-fallback cases in both injection suites assert the same shape on the fallback branch (when defaults returns failure).
  • Existing mock-command 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 (the createCommandMatchingMockExecutor matches on command.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

  • `npm run typecheck` — clean (`npx tsc --noEmit && npx tsc -p tsconfig.test.json`)
  • `npm run lint` — clean
  • `npm run format:check` — clean
  • `npm test` — 1772 passed / 0 failed across 167 files (no other tests touched bundle-id command shape)

Notes

  • This PR does not modify defaultExecutor itself; the existing useShell=true hardening 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.
  • CHANGELOG entry added under `[Unreleased] / Fixed`.
  • AI assistance disclosure: I used Claude (Opus 4.7) while drafting and verifying this change. All edits, regression-test pre/post-fix verification, and local gate runs were reviewed by me before commit.

Closes #367

…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
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.

Security: Remaining /bin/sh -c shell-injection sites not covered by #289

1 participant