Skip to content

Commit c01451a

Browse files
committed
Merge branch 'am/p4-apply-commit-shell-injection-fix' into seen
The `applyCommit()` function inside 'git-p4.py' has been updated to use direct subprocess pipes in order to avoid shell interpolation, plugging a potential shell injection vulnerability when processing commit IDs. * am/p4-apply-commit-shell-injection-fix: git-p4: avoid shell interpretation of commit ids in applyCommit
2 parents bd4a0cc + 46ec7ad commit c01451a

2 files changed

Lines changed: 41 additions & 10 deletions

File tree

‎git-p4.py‎

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -465,6 +465,26 @@ def p4_system(cmd, *k, **kw):
465465
raise subprocess.CalledProcessError(retcode, real_cmd)
466466

467467

468+
def diffTreeApply(id, applyArgs, ignore_error=False):
469+
"""Pipe `git diff-tree --full-index -p <id>` into `git apply <applyArgs>`
470+
without a shell, so id can never be interpreted as shell syntax. Returns
471+
the exit status of git apply, raising CalledProcessError on a non-zero
472+
status unless ignore_error is set."""
473+
diffArgv = ["git", "diff-tree", "--full-index", "-p", id]
474+
applyArgv = ["git", "apply"] + applyArgs
475+
if verbose:
476+
print("TryPatch: %s | %s" % (" ".join(diffArgv), " ".join(applyArgv)))
477+
diffProc = subprocess.Popen(diffArgv, stdout=subprocess.PIPE)
478+
applyProc = subprocess.Popen(applyArgv, stdin=diffProc.stdout)
479+
diffProc.stdout.close()
480+
applyProc.wait()
481+
diffProc.wait()
482+
retcode = applyProc.returncode
483+
if retcode and not ignore_error:
484+
raise subprocess.CalledProcessError(retcode, applyArgv)
485+
return retcode
486+
487+
468488
def die_bad_access(s):
469489
die("failure accessing depot: {0}".format(s.rstrip()))
470490

@@ -2234,16 +2254,11 @@ def applyCommit(self, id):
22342254
else:
22352255
die("unknown modifier %s for %s" % (modifier, path))
22362256

2237-
diffcmd = "git diff-tree --full-index -p \"%s\"" % (id)
2238-
patchcmd = diffcmd + " | git apply "
2239-
tryPatchCmd = patchcmd + "--check -"
2240-
applyPatchCmd = patchcmd + "--check --apply -"
2257+
tryPatchArgs = ["--check", "-"]
2258+
applyPatchArgs = ["--check", "--apply", "-"]
22412259
patch_succeeded = True
22422260

2243-
if verbose:
2244-
print("TryPatch: %s" % tryPatchCmd)
2245-
2246-
if os.system(tryPatchCmd) != 0:
2261+
if diffTreeApply(id, tryPatchArgs, ignore_error=True) != 0:
22472262
fixed_rcs_keywords = False
22482263
patch_succeeded = False
22492264
print("Unfortunately applying the change failed!")
@@ -2279,7 +2294,7 @@ def applyCommit(self, id):
22792294

22802295
if fixed_rcs_keywords:
22812296
print("Retrying the patch with RCS keywords cleaned up")
2282-
if os.system(tryPatchCmd) == 0:
2297+
if diffTreeApply(id, tryPatchArgs, ignore_error=True) == 0:
22832298
patch_succeeded = True
22842299
print("Patch succeesed this time with RCS keywords cleaned")
22852300

@@ -2291,7 +2306,7 @@ def applyCommit(self, id):
22912306
#
22922307
# Apply the patch for real, and do add/delete/+x handling.
22932308
#
2294-
system(applyPatchCmd, shell=True)
2309+
diffTreeApply(id, applyPatchArgs)
22952310

22962311
for f in filesToChangeType:
22972312
p4_edit(f, "-t", "auto")

‎t/t9803-git-p4-shell-metachars.sh‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,4 +105,20 @@ test_expect_success 'branch with shell char' '
105105
)
106106
'
107107

108+
test_expect_success 'git p4 submit --commit does not execute shell metachars in commit id' '
109+
git p4 clone --dest="$git" //depot &&
110+
test_when_finished cleanup_git &&
111+
(
112+
cd "$git" &&
113+
git config git-p4.skipSubmitEditCheck true &&
114+
echo f3 >file3 &&
115+
git add file3 &&
116+
git commit -m "add file3" &&
117+
name='"'"'$(touch${IFS}injection-marker)'"'"' &&
118+
git branch "$name" HEAD &&
119+
P4EDITOR="test-tool chmtime +5" git p4 submit --commit "$name"
120+
) &&
121+
test_path_is_missing "$cli/injection-marker"
122+
'
123+
108124
test_done

0 commit comments

Comments
 (0)