git-p4: avoid shell interpretation of commit ids in applyCommit

From: Anupam Mediratta <[email protected]>

applyCommit() builds a git diff-tree ... | git apply ... pipeline as a shell command string, interpolating the commit id and running it via os.system()/system(shell=True). The id usually comes from git rev-list output (safe, plain SHA-1s), but it can also come verbatim from the user-supplied --commit option, which is never validated (git-p4.py:2620-2631). A value such as $(some-command) passed to --commit is executed by the shell during command substitution, even though the value is wrapped in double quotes.

Replace the shell pipeline with two argument-vector subprocess calls connected directly through a pipe, matching the pattern already used throughout this file (read_pipe, read_pipe_lines, p4_system). This removes the shell entirely, rather than relying on quoting the interpolated value.

Add a regression test exercising git p4 submit --commit with a shell metacharacter payload, verifying it is never interpreted.

---
git-p4: avoid shell interpretation of commit ids in applyCommit

P4Submit.applyCommit() builds a git diff-tree | git apply pipeline as a shell command string and runs it with os.system()/system(shell=True). The commit id it interpolates is usually a plain SHA-1 from git rev-list, but it can also come straight from the unvalidated --commit command-line option, so a value such as $(some-command) passed to --commit gets executed by the shell during command substitution.

This replaces the shell pipeline with two argument-vector subprocess calls connected directly through a pipe, the same pattern already used everywhere else in this file (read_pipe, read_pipe_lines, p4_system), so there's no shell left to escape correctly. It also adds a regression test in t9803 that submits a commit id crafted with shell metacharacters and checks they're never executed.

I have read https://git-scm.com/docs/SubmittingPatches#ai and confirm this contribution complies with it.

Changes since v1: corrected the description of the affected data (it's the unvalidated --commit argument, not Perforce server data, that was ever exploitable here), removed the shell entirely instead of quoting the interpolated value, replaced a test that never reached the vulnerable code with one that drives it through git p4 submit --commit, and fixed the commit message format (subsystem prefix, rationale, sign-off).

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2411%2Fanupamme%2Ffix-repo-git-git-p4-cwe-78-shell-injection-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2411/anupamme/fix-repo-git-git-p4-cwe-78-shell-injection-v1
Pull-Request: https://github.com/git/git/pull/2411

 git-p4.py                         | 31 +++++++++++++++++++++----------
 t/t9803-git-p4-shell-metachars.sh | 16 ++++++++++++++++
 2 files changed, 37 insertions(+), 10 deletions(-)

index c0ca7becaf..b09f5cd740 100755
@@ -465,6 +465,22 @@ def p4_system(cmd, *k, **kw):
         raise subprocess.CalledProcessError(retcode, real_cmd)
 
 
+def diffTreeApply(id, applyArgs):
+    """Pipe `git diff-tree --full-index -p <id>` into `git apply <applyArgs>`
+    without a shell, so id can never be interpreted as shell syntax. Returns
+    the exit status of git apply."""
+    diffArgv = ["git", "diff-tree", "--full-index", "-p", id]
+    applyArgv = ["git", "apply"] + applyArgs
+    if verbose:
+        print("TryPatch: %s | %s" % (" ".join(diffArgv), " ".join(applyArgv)))
+    diffProc = subprocess.Popen(diffArgv, stdout=subprocess.PIPE)
+    applyProc = subprocess.Popen(applyArgv, stdin=diffProc.stdout)
+    diffProc.stdout.close()
+    applyProc.wait()
+    diffProc.wait()
+    return applyProc.returncode
+
+
 def die_bad_access(s):
     die("failure accessing depot: {0}".format(s.rstrip()))
 

@@ -2234,16 +2250,11 @@ class P4Submit(Command, P4UserMap):
             else:
                 die("unknown modifier %s for %s" % (modifier, path))
 
-        diffcmd = "git diff-tree --full-index -p \"%s\"" % (id)
-        patchcmd = diffcmd + " | git apply "
-        tryPatchCmd = patchcmd + "--check -"
-        applyPatchCmd = patchcmd + "--check --apply -"
+        tryPatchArgs = ["--check", "-"]
+        applyPatchArgs = ["--check", "--apply", "-"]
         patch_succeeded = True
 
-        if verbose:
-            print("TryPatch: %s" % tryPatchCmd)
-
-        if os.system(tryPatchCmd) != 0:
+        if diffTreeApply(id, tryPatchArgs) != 0:
             fixed_rcs_keywords = False
             patch_succeeded = False
             print("Unfortunately applying the change failed!")

@@ -2279,7 +2290,7 @@ class P4Submit(Command, P4UserMap):
 
             if fixed_rcs_keywords:
                 print("Retrying the patch with RCS keywords cleaned up")
-                if os.system(tryPatchCmd) == 0:
+                if diffTreeApply(id, tryPatchArgs) == 0:
                     patch_succeeded = True
                     print("Patch succeesed this time with RCS keywords cleaned")
 

@@ -2291,7 +2302,7 @@ class P4Submit(Command, P4UserMap):
         #
         # Apply the patch for real, and do add/delete/+x handling.
         #
-        system(applyPatchCmd, shell=True)
+        diffTreeApply(id, applyPatchArgs)
 
         for f in filesToChangeType:
             p4_edit(f, "-t", "auto")

index 2913277013..ef8fd6e094 100755
@@ -105,4 +105,20 @@ test_expect_success 'branch with shell char' '
 	)
 '
 
+test_expect_success 'git p4 submit --commit does not execute shell metachars in commit id' '
+	git p4 clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEditCheck true &&
+		echo f3 >file3 &&
+		git add file3 &&
+		git commit -m "add file3" &&
+		name='"'"'$(touch${IFS}injection-marker)'"'"' &&
+		git branch "$name" HEAD &&
+		P4EDITOR="test-tool chmtime +5" git p4 submit --commit "$name"
+	) &&
+	test_path_is_missing "$cli/injection-marker"
+'
+
 test_done

base-commit: d38352cd43ab9745686d697872408bc3249a153f

gitgitgadget

From: Anupam Mediratta <[email protected]>

applyCommit() builds a git diff-tree ... | git apply ... pipeline as a shell command string, interpolating the commit id and running it via os.system()/system(shell=True). The id usually comes from git rev-list output (safe, plain SHA-1s), but it can also come verbatim from the user-supplied --commit option, which is never validated (git-p4.py:2620-2631). A value such as $(some-command) passed to --commit is executed by the shell during command substitution, even though the value is wrapped in double quotes.

Replace the shell pipeline with two argument-vector subprocess calls connected directly through a pipe, matching the pattern already used throughout this file (read_pipe, read_pipe_lines, p4_system). This removes the shell entirely, rather than relying on quoting the interpolated value.

The shell-based call that applied the patch for real went through git-p4.py's own system() helper, which raises CalledProcessError on a non-zero exit status, so a failed apply aborted the submit. Keep that behaviour by giving the new helper the same ignore_error contract system() uses: it raises unless the caller asks for the status, and the two callers that test the status for themselves ask for it.

Add a regression test exercising git p4 submit --commit with a shell metacharacter payload, verifying it is never interpreted.

---
git-p4: avoid shell interpretation of commit ids in applyCommit

P4Submit.applyCommit() builds a git diff-tree | git apply pipeline as a shell command string and runs it with os.system()/system(shell=True). The commit id it interpolates is usually a plain SHA-1 from git rev-list, but it can also come straight from the unvalidated --commit command-line option, so a value such as $(some-command) passed to --commit gets executed by the shell during command substitution.

This replaces the shell pipeline with two argument-vector subprocess calls connected directly through a pipe, the same pattern already used everywhere else in this file (read_pipe, read_pipe_lines, p4_system), so there's no shell left to escape correctly. It also adds a regression test in t9803 that submits a commit id crafted with shell metacharacters and checks they're never executed.

I have read https://git-scm.com/docs/SubmittingPatches#ai and confirm this contribution complies with it.

Changes since v1: as Junio pointed out, the call that applies the patch for real used to go through git-p4.py's own system() helper, which raises CalledProcessError on a non-zero exit status, so a failed git apply aborted the submit; v1 dropped that and silently carried on. The new helper now takes the same ignore_error argument system() does and raises by default, and the two callers that inspect the exit status themselves pass ignore_error=True.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2411%2Fanupamme%2Ffix-repo-git-git-p4-cwe-78-shell-injection-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2411/anupamme/fix-repo-git-git-p4-cwe-78-shell-injection-v2
Pull-Request: https://github.com/git/git/pull/2411

Range-diff vs v1:

1: 391e429a2a ! 1: 6fcacc71b8 git-p4: avoid shell interpretation of commit ids in applyCommit
@@ Commit message
removes the shell entirely, rather than relying on quoting the interpolated value.

+ The shell-based call that applied the patch for real went through
+ git-p4.py's own system() helper, which raises CalledProcessError on a
+ non-zero exit status, so a failed apply aborted the submit. Keep that
+ behaviour by giving the new helper the same ignore_error contract
+ system() uses: it raises unless the caller asks for the status, and the
+ two callers that test the status for themselves ask for it.
+
Add a regression test exercising git p4 submit --commit with a shell metacharacter payload, verifying it is never interpreted.

@@ git-p4.py: def p4_system(cmd, *k, **kw):
raise subprocess.CalledProcessError(retcode, real_cmd)

-+def diffTreeApply(id, applyArgs):
++def diffTreeApply(id, applyArgs, ignore_error=False):
+ """Pipe git diff-tree --full-index -p <id> into git apply <applyArgs>
+ without a shell, so id can never be interpreted as shell syntax. Returns
-+ the exit status of git apply."""
++ the exit status of git apply, raising CalledProcessError on a non-zero
++ status unless ignore_error is set."""
+ diffArgv = ["git", "diff-tree", "--full-index", "-p", id]
+ applyArgv = ["git", "apply"] + applyArgs
+ if verbose:
@@ git-p4.py: def p4_system(cmd, *k, **kw):
+ diffProc.stdout.close()
+ applyProc.wait()
+ diffProc.wait()
-+ return applyProc.returncode
++ retcode = applyProc.returncode
++ if retcode and not ignore_error:
++ raise subprocess.CalledProcessError(retcode, applyArgv)
++ return retcode
+
+
def die_bad_access(s):
@@ git-p4.py: class P4Submit(Command, P4UserMap):
- print("TryPatch: %s" % tryPatchCmd)
-
- if os.system(tryPatchCmd) != 0:
-+ if diffTreeApply(id, tryPatchArgs) != 0:
++ if diffTreeApply(id, tryPatchArgs, ignore_error=True) != 0:
fixed_rcs_keywords = False
patch_succeeded = False
print("Unfortunately applying the change failed!")
@@ git-p4.py: class P4Submit(Command, P4UserMap):
if fixed_rcs_keywords:
print("Retrying the patch with RCS keywords cleaned up")
- if os.system(tryPatchCmd) == 0:
-+ if diffTreeApply(id, tryPatchArgs) == 0:
++ if diffTreeApply(id, tryPatchArgs, ignore_error=True) == 0:
patch_succeeded = True
print("Patch succeesed this time with RCS keywords cleaned")

 git-p4.py                         | 35 ++++++++++++++++++++++---------
 t/t9803-git-p4-shell-metachars.sh | 16 ++++++++++++++
 2 files changed, 41 insertions(+), 10 deletions(-)

index c0ca7becaf..e716831554 100755
@@ -465,6 +465,26 @@ def p4_system(cmd, *k, **kw):
         raise subprocess.CalledProcessError(retcode, real_cmd)
 
 
+def diffTreeApply(id, applyArgs, ignore_error=False):
+    """Pipe `git diff-tree --full-index -p <id>` into `git apply <applyArgs>`
+    without a shell, so id can never be interpreted as shell syntax. Returns
+    the exit status of git apply, raising CalledProcessError on a non-zero
+    status unless ignore_error is set."""
+    diffArgv = ["git", "diff-tree", "--full-index", "-p", id]
+    applyArgv = ["git", "apply"] + applyArgs
+    if verbose:
+        print("TryPatch: %s | %s" % (" ".join(diffArgv), " ".join(applyArgv)))
+    diffProc = subprocess.Popen(diffArgv, stdout=subprocess.PIPE)
+    applyProc = subprocess.Popen(applyArgv, stdin=diffProc.stdout)
+    diffProc.stdout.close()
+    applyProc.wait()
+    diffProc.wait()
+    retcode = applyProc.returncode
+    if retcode and not ignore_error:
+        raise subprocess.CalledProcessError(retcode, applyArgv)
+    return retcode
+
+
 def die_bad_access(s):
     die("failure accessing depot: {0}".format(s.rstrip()))
 

@@ -2234,16 +2254,11 @@ class P4Submit(Command, P4UserMap):
             else:
                 die("unknown modifier %s for %s" % (modifier, path))
 
-        diffcmd = "git diff-tree --full-index -p \"%s\"" % (id)
-        patchcmd = diffcmd + " | git apply "
-        tryPatchCmd = patchcmd + "--check -"
-        applyPatchCmd = patchcmd + "--check --apply -"
+        tryPatchArgs = ["--check", "-"]
+        applyPatchArgs = ["--check", "--apply", "-"]
         patch_succeeded = True
 
-        if verbose:
-            print("TryPatch: %s" % tryPatchCmd)
-
-        if os.system(tryPatchCmd) != 0:
+        if diffTreeApply(id, tryPatchArgs, ignore_error=True) != 0:
             fixed_rcs_keywords = False
             patch_succeeded = False
             print("Unfortunately applying the change failed!")

@@ -2279,7 +2294,7 @@ class P4Submit(Command, P4UserMap):
 
             if fixed_rcs_keywords:
                 print("Retrying the patch with RCS keywords cleaned up")
-                if os.system(tryPatchCmd) == 0:
+                if diffTreeApply(id, tryPatchArgs, ignore_error=True) == 0:
                     patch_succeeded = True
                     print("Patch succeesed this time with RCS keywords cleaned")
 

@@ -2291,7 +2306,7 @@ class P4Submit(Command, P4UserMap):
         #
         # Apply the patch for real, and do add/delete/+x handling.
         #
-        system(applyPatchCmd, shell=True)
+        diffTreeApply(id, applyPatchArgs)
 
         for f in filesToChangeType:
             p4_edit(f, "-t", "auto")

index 2913277013..ef8fd6e094 100755
@@ -105,4 +105,20 @@ test_expect_success 'branch with shell char' '
 	)
 '
 
+test_expect_success 'git p4 submit --commit does not execute shell metachars in commit id' '
+	git p4 clone --dest="$git" //depot &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEditCheck true &&
+		echo f3 >file3 &&
+		git add file3 &&
+		git commit -m "add file3" &&
+		name='"'"'$(touch${IFS}injection-marker)'"'"' &&
+		git branch "$name" HEAD &&
+		P4EDITOR="test-tool chmtime +5" git p4 submit --commit "$name"
+	) &&
+	test_path_is_missing "$cli/injection-marker"
+'
+
 test_done

base-commit: d38352cd43ab9745686d697872408bc3249a153f

gitgitgadget

I do not know what this means, but anyway.

This round unfortunately saw interest and reviews on the list from nobody. Unless we see some volunteers to give it a review in a few weeks, I will discard the topic out of my tree, as 'seen' has been crowded with topics that have been collecting dust without getting many reviews.

Thanks.