{"thread":{"id":"66368","subject":"[PATCH] git-p4: avoid shell interpretation of commit ids in applyCommit","startedAt":"2026-09-22T16:11:49Z","lastAt":"2026-09-24T08:28:04Z","messageCount":3,"participants":["Anupam Mediratta via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"552998","messageId":"pull.2411.git.git.1790093506966.gitgitgadget@gmail.com","threadId":"66368","inReplyTo":null,"subject":"[PATCH] git-p4: avoid shell interpretation of commit ids in applyCommit","fromName":"Anupam Mediratta via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-22T16:11:46Z","receivedAt":"2026-09-22T16:11:49Z","isPatch":true,"body":"From: Anupam Mediratta <mediratta@gmail.com>\n\napplyCommit() builds a `git diff-tree ... | git apply ...` pipeline as a\nshell command string, interpolating the commit id and running it via\nos.system()/system(shell=True). The id usually comes from `git rev-list`\noutput (safe, plain SHA-1s), but it can also come verbatim from the\nuser-supplied `--commit` option, which is never validated\n(git-p4.py:2620-2631). A value such as `$(some-command)` passed to\n`--commit` is executed by the shell during command substitution, even\nthough the value is wrapped in double quotes.\n\nReplace the shell pipeline with two argument-vector subprocess calls\nconnected directly through a pipe, matching the pattern already used\nthroughout this file (read_pipe, read_pipe_lines, p4_system). This\nremoves the shell entirely, rather than relying on quoting the\ninterpolated value.\n\nAdd a regression test exercising `git p4 submit --commit` with a shell\nmetacharacter payload, verifying it is never interpreted.\n\nSigned-off-by: Anupam Mediratta <mediratta@gmail.com>\n---\n    git-p4: avoid shell interpretation of commit ids in applyCommit\n    \n    P4Submit.applyCommit() builds a git diff-tree | git apply pipeline as a\n    shell command string and runs it with os.system()/system(shell=True).\n    The commit id it interpolates is usually a plain SHA-1 from git\n    rev-list, but it can also come straight from the unvalidated --commit\n    command-line option, so a value such as $(some-command) passed to\n    --commit gets executed by the shell during command substitution.\n    \n    This replaces the shell pipeline with two argument-vector subprocess\n    calls connected directly through a pipe, the same pattern already used\n    everywhere else in this file (read_pipe, read_pipe_lines, p4_system), so\n    there's no shell left to escape correctly. It also adds a regression\n    test in t9803 that submits a commit id crafted with shell metacharacters\n    and checks they're never executed.\n    \n    I have read https://git-scm.com/docs/SubmittingPatches#ai and confirm\n    this contribution complies with it.\n    \n    Changes since v1: corrected the description of the affected data (it's\n    the unvalidated --commit argument, not Perforce server data, that was\n    ever exploitable here), removed the shell entirely instead of quoting\n    the interpolated value, replaced a test that never reached the\n    vulnerable code with one that drives it through git p4 submit --commit,\n    and fixed the commit message format (subsystem prefix, rationale,\n    sign-off).\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2411%2Fanupamme%2Ffix-repo-git-git-p4-cwe-78-shell-injection-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2411/anupamme/fix-repo-git-git-p4-cwe-78-shell-injection-v1\nPull-Request: https://github.com/git/git/pull/2411\n\n git-p4.py                         | 31 +++++++++++++++++++++----------\n t/t9803-git-p4-shell-metachars.sh | 16 ++++++++++++++++\n 2 files changed, 37 insertions(+), 10 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex c0ca7becaf..b09f5cd740 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -465,6 +465,22 @@ def p4_system(cmd, *k, **kw):\n         raise subprocess.CalledProcessError(retcode, real_cmd)\n \n \n+def diffTreeApply(id, applyArgs):\n+    \"\"\"Pipe `git diff-tree --full-index -p <id>` into `git apply <applyArgs>`\n+    without a shell, so id can never be interpreted as shell syntax. Returns\n+    the exit status of git apply.\"\"\"\n+    diffArgv = [\"git\", \"diff-tree\", \"--full-index\", \"-p\", id]\n+    applyArgv = [\"git\", \"apply\"] + applyArgs\n+    if verbose:\n+        print(\"TryPatch: %s | %s\" % (\" \".join(diffArgv), \" \".join(applyArgv)))\n+    diffProc = subprocess.Popen(diffArgv, stdout=subprocess.PIPE)\n+    applyProc = subprocess.Popen(applyArgv, stdin=diffProc.stdout)\n+    diffProc.stdout.close()\n+    applyProc.wait()\n+    diffProc.wait()\n+    return applyProc.returncode\n+\n+\n def die_bad_access(s):\n     die(\"failure accessing depot: {0}\".format(s.rstrip()))\n \n@@ -2234,16 +2250,11 @@ class P4Submit(Command, P4UserMap):\n             else:\n                 die(\"unknown modifier %s for %s\" % (modifier, path))\n \n-        diffcmd = \"git diff-tree --full-index -p \\\"%s\\\"\" % (id)\n-        patchcmd = diffcmd + \" | git apply \"\n-        tryPatchCmd = patchcmd + \"--check -\"\n-        applyPatchCmd = patchcmd + \"--check --apply -\"\n+        tryPatchArgs = [\"--check\", \"-\"]\n+        applyPatchArgs = [\"--check\", \"--apply\", \"-\"]\n         patch_succeeded = True\n \n-        if verbose:\n-            print(\"TryPatch: %s\" % tryPatchCmd)\n-\n-        if os.system(tryPatchCmd) != 0:\n+        if diffTreeApply(id, tryPatchArgs) != 0:\n             fixed_rcs_keywords = False\n             patch_succeeded = False\n             print(\"Unfortunately applying the change failed!\")\n@@ -2279,7 +2290,7 @@ class P4Submit(Command, P4UserMap):\n \n             if fixed_rcs_keywords:\n                 print(\"Retrying the patch with RCS keywords cleaned up\")\n-                if os.system(tryPatchCmd) == 0:\n+                if diffTreeApply(id, tryPatchArgs) == 0:\n                     patch_succeeded = True\n                     print(\"Patch succeesed this time with RCS keywords cleaned\")\n \n@@ -2291,7 +2302,7 @@ class P4Submit(Command, P4UserMap):\n         #\n         # Apply the patch for real, and do add/delete/+x handling.\n         #\n-        system(applyPatchCmd, shell=True)\n+        diffTreeApply(id, applyPatchArgs)\n \n         for f in filesToChangeType:\n             p4_edit(f, \"-t\", \"auto\")\ndiff --git a/t/t9803-git-p4-shell-metachars.sh b/t/t9803-git-p4-shell-metachars.sh\nindex 2913277013..ef8fd6e094 100755\n--- a/t/t9803-git-p4-shell-metachars.sh\n+++ b/t/t9803-git-p4-shell-metachars.sh\n@@ -105,4 +105,20 @@ test_expect_success 'branch with shell char' '\n \t)\n '\n \n+test_expect_success 'git p4 submit --commit does not execute shell metachars in commit id' '\n+\tgit p4 clone --dest=\"$git\" //depot &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit config git-p4.skipSubmitEditCheck true &&\n+\t\techo f3 >file3 &&\n+\t\tgit add file3 &&\n+\t\tgit commit -m \"add file3\" &&\n+\t\tname='\"'\"'$(touch${IFS}injection-marker)'\"'\"' &&\n+\t\tgit branch \"$name\" HEAD &&\n+\t\tP4EDITOR=\"test-tool chmtime +5\" git p4 submit --commit \"$name\"\n+\t) &&\n+\ttest_path_is_missing \"$cli/injection-marker\"\n+'\n+\n test_done\n\nbase-commit: d38352cd43ab9745686d697872408bc3249a153f\n-- \ngitgitgadget\n"},{"id":"553099","messageId":"xmqqv77vailx.fsf@gitster.g","threadId":"66368","inReplyTo":"pull.2411.git.git.1790093506966.gitgitgadget@gmail.com","subject":"Re: [PATCH] git-p4: avoid shell interpretation of commit ids in applyCommit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-23T19:11:06Z","receivedAt":"2026-09-23T19:11:09Z","isPatch":true,"body":"\"Anupam Mediratta via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> @@ -2279,7 +2290,7 @@ class P4Submit(Command, P4UserMap):\n>  \n>              if fixed_rcs_keywords:\n>                  print(\"Retrying the patch with RCS keywords cleaned up\")\n> -                if os.system(tryPatchCmd) == 0:\n> +                if diffTreeApply(id, tryPatchArgs) == 0:\n>                      patch_succeeded = True\n>                      print(\"Patch succeesed this time with RCS keywords cleaned\")\n\nBoth of these check the result of running diff|apply pipeline and\nreact to a failure.\n\n> @@ -2291,7 +2302,7 @@ class P4Submit(Command, P4UserMap):\n>          #\n>          # Apply the patch for real, and do add/delete/+x handling.\n>          #\n> -        system(applyPatchCmd, shell=True)\n> +        diffTreeApply(id, applyPatchArgs)\n\nIt is a bit hard to discover, but the original code catches a failed\n\"diff|apply\" pipeline invocation, because the \"system()\" used here\nis what git-p4.py defines for itself.  When the pipeline fails, this\nsystem() raises subprocess.CalledProcessError().\n\nThe new one ignores the exit status from the pipeline, so even after\na failure to apply the change, the program continues.\n\nWhich may not be what you want to see.\n\n>  \n>          for f in filesToChangeType:\n>              p4_edit(f, \"-t\", \"auto\")\n> diff --git a/t/t9803-git-p4-shell-metachars.sh b/t/t9803-git-p4-shell-metachars.sh\n> index 2913277013..ef8fd6e094 100755\n> --- a/t/t9803-git-p4-shell-metachars.sh\n> +++ b/t/t9803-git-p4-shell-metachars.sh\n> @@ -105,4 +105,20 @@ test_expect_success 'branch with shell char' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'git p4 submit --commit does not execute shell metachars in commit id' '\n> +\tgit p4 clone --dest=\"$git\" //depot &&\n> +\ttest_when_finished cleanup_git &&\n> +\t(\n> +\t\tcd \"$git\" &&\n> +\t\tgit config git-p4.skipSubmitEditCheck true &&\n> +\t\techo f3 >file3 &&\n> +\t\tgit add file3 &&\n> +\t\tgit commit -m \"add file3\" &&\n> +\t\tname='\"'\"'$(touch${IFS}injection-marker)'\"'\"' &&\n> +\t\tgit branch \"$name\" HEAD &&\n> +\t\tP4EDITOR=\"test-tool chmtime +5\" git p4 submit --commit \"$name\"\n> +\t) &&\n> +\ttest_path_is_missing \"$cli/injection-marker\"\n> +'\n> +\n>  test_done\n>\n> base-commit: d38352cd43ab9745686d697872408bc3249a153f\n"},{"id":"553152","messageId":"pull.2411.v2.git.git.1790238482045.gitgitgadget@gmail.com","threadId":"66368","inReplyTo":"pull.2411.git.git.1790093506966.gitgitgadget@gmail.com","subject":"[PATCH v2] git-p4: avoid shell interpretation of commit ids in applyCommit","fromName":"Anupam Mediratta via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-24T08:28:02Z","receivedAt":"2026-09-24T08:28:04Z","isPatch":true,"body":"From: Anupam Mediratta <mediratta@gmail.com>\n\napplyCommit() builds a `git diff-tree ... | git apply ...` pipeline as a\nshell command string, interpolating the commit id and running it via\nos.system()/system(shell=True). The id usually comes from `git rev-list`\noutput (safe, plain SHA-1s), but it can also come verbatim from the\nuser-supplied `--commit` option, which is never validated\n(git-p4.py:2620-2631). A value such as `$(some-command)` passed to\n`--commit` is executed by the shell during command substitution, even\nthough the value is wrapped in double quotes.\n\nReplace the shell pipeline with two argument-vector subprocess calls\nconnected directly through a pipe, matching the pattern already used\nthroughout this file (read_pipe, read_pipe_lines, p4_system). This\nremoves the shell entirely, rather than relying on quoting the\ninterpolated value.\n\nThe shell-based call that applied the patch for real went through\ngit-p4.py's own system() helper, which raises CalledProcessError on a\nnon-zero exit status, so a failed apply aborted the submit. Keep that\nbehaviour by giving the new helper the same ignore_error contract\nsystem() uses: it raises unless the caller asks for the status, and the\ntwo callers that test the status for themselves ask for it.\n\nAdd a regression test exercising `git p4 submit --commit` with a shell\nmetacharacter payload, verifying it is never interpreted.\n\nSigned-off-by: Anupam Mediratta <mediratta@gmail.com>\n---\n    git-p4: avoid shell interpretation of commit ids in applyCommit\n    \n    P4Submit.applyCommit() builds a git diff-tree | git apply pipeline as a\n    shell command string and runs it with os.system()/system(shell=True).\n    The commit id it interpolates is usually a plain SHA-1 from git\n    rev-list, but it can also come straight from the unvalidated --commit\n    command-line option, so a value such as $(some-command) passed to\n    --commit gets executed by the shell during command substitution.\n    \n    This replaces the shell pipeline with two argument-vector subprocess\n    calls connected directly through a pipe, the same pattern already used\n    everywhere else in this file (read_pipe, read_pipe_lines, p4_system), so\n    there's no shell left to escape correctly. It also adds a regression\n    test in t9803 that submits a commit id crafted with shell metacharacters\n    and checks they're never executed.\n    \n    I have read https://git-scm.com/docs/SubmittingPatches#ai and confirm\n    this contribution complies with it.\n    \n    Changes since v1: as Junio pointed out, the call that applies the patch\n    for real used to go through git-p4.py's own system() helper, which\n    raises CalledProcessError on a non-zero exit status, so a failed git\n    apply aborted the submit; v1 dropped that and silently carried on. The\n    new helper now takes the same ignore_error argument system() does and\n    raises by default, and the two callers that inspect the exit status\n    themselves pass ignore_error=True.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2411%2Fanupamme%2Ffix-repo-git-git-p4-cwe-78-shell-injection-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2411/anupamme/fix-repo-git-git-p4-cwe-78-shell-injection-v2\nPull-Request: https://github.com/git/git/pull/2411\n\nRange-diff vs v1:\n\n 1:  391e429a2a ! 1:  6fcacc71b8 git-p4: avoid shell interpretation of commit ids in applyCommit\n     @@ Commit message\n          removes the shell entirely, rather than relying on quoting the\n          interpolated value.\n      \n     +    The shell-based call that applied the patch for real went through\n     +    git-p4.py's own system() helper, which raises CalledProcessError on a\n     +    non-zero exit status, so a failed apply aborted the submit. Keep that\n     +    behaviour by giving the new helper the same ignore_error contract\n     +    system() uses: it raises unless the caller asks for the status, and the\n     +    two callers that test the status for themselves ask for it.\n     +\n          Add a regression test exercising `git p4 submit --commit` with a shell\n          metacharacter payload, verifying it is never interpreted.\n      \n     @@ git-p4.py: def p4_system(cmd, *k, **kw):\n               raise subprocess.CalledProcessError(retcode, real_cmd)\n       \n       \n     -+def diffTreeApply(id, applyArgs):\n     ++def diffTreeApply(id, applyArgs, ignore_error=False):\n      +    \"\"\"Pipe `git diff-tree --full-index -p <id>` into `git apply <applyArgs>`\n      +    without a shell, so id can never be interpreted as shell syntax. Returns\n     -+    the exit status of git apply.\"\"\"\n     ++    the exit status of git apply, raising CalledProcessError on a non-zero\n     ++    status unless ignore_error is set.\"\"\"\n      +    diffArgv = [\"git\", \"diff-tree\", \"--full-index\", \"-p\", id]\n      +    applyArgv = [\"git\", \"apply\"] + applyArgs\n      +    if verbose:\n     @@ git-p4.py: def p4_system(cmd, *k, **kw):\n      +    diffProc.stdout.close()\n      +    applyProc.wait()\n      +    diffProc.wait()\n     -+    return applyProc.returncode\n     ++    retcode = applyProc.returncode\n     ++    if retcode and not ignore_error:\n     ++        raise subprocess.CalledProcessError(retcode, applyArgv)\n     ++    return retcode\n      +\n      +\n       def die_bad_access(s):\n     @@ git-p4.py: class P4Submit(Command, P4UserMap):\n      -            print(\"TryPatch: %s\" % tryPatchCmd)\n      -\n      -        if os.system(tryPatchCmd) != 0:\n     -+        if diffTreeApply(id, tryPatchArgs) != 0:\n     ++        if diffTreeApply(id, tryPatchArgs, ignore_error=True) != 0:\n                   fixed_rcs_keywords = False\n                   patch_succeeded = False\n                   print(\"Unfortunately applying the change failed!\")\n     @@ git-p4.py: class P4Submit(Command, P4UserMap):\n                   if fixed_rcs_keywords:\n                       print(\"Retrying the patch with RCS keywords cleaned up\")\n      -                if os.system(tryPatchCmd) == 0:\n     -+                if diffTreeApply(id, tryPatchArgs) == 0:\n     ++                if diffTreeApply(id, tryPatchArgs, ignore_error=True) == 0:\n                           patch_succeeded = True\n                           print(\"Patch succeesed this time with RCS keywords cleaned\")\n       \n\n\n git-p4.py                         | 35 ++++++++++++++++++++++---------\n t/t9803-git-p4-shell-metachars.sh | 16 ++++++++++++++\n 2 files changed, 41 insertions(+), 10 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex c0ca7becaf..e716831554 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -465,6 +465,26 @@ def p4_system(cmd, *k, **kw):\n         raise subprocess.CalledProcessError(retcode, real_cmd)\n \n \n+def diffTreeApply(id, applyArgs, ignore_error=False):\n+    \"\"\"Pipe `git diff-tree --full-index -p <id>` into `git apply <applyArgs>`\n+    without a shell, so id can never be interpreted as shell syntax. Returns\n+    the exit status of git apply, raising CalledProcessError on a non-zero\n+    status unless ignore_error is set.\"\"\"\n+    diffArgv = [\"git\", \"diff-tree\", \"--full-index\", \"-p\", id]\n+    applyArgv = [\"git\", \"apply\"] + applyArgs\n+    if verbose:\n+        print(\"TryPatch: %s | %s\" % (\" \".join(diffArgv), \" \".join(applyArgv)))\n+    diffProc = subprocess.Popen(diffArgv, stdout=subprocess.PIPE)\n+    applyProc = subprocess.Popen(applyArgv, stdin=diffProc.stdout)\n+    diffProc.stdout.close()\n+    applyProc.wait()\n+    diffProc.wait()\n+    retcode = applyProc.returncode\n+    if retcode and not ignore_error:\n+        raise subprocess.CalledProcessError(retcode, applyArgv)\n+    return retcode\n+\n+\n def die_bad_access(s):\n     die(\"failure accessing depot: {0}\".format(s.rstrip()))\n \n@@ -2234,16 +2254,11 @@ class P4Submit(Command, P4UserMap):\n             else:\n                 die(\"unknown modifier %s for %s\" % (modifier, path))\n \n-        diffcmd = \"git diff-tree --full-index -p \\\"%s\\\"\" % (id)\n-        patchcmd = diffcmd + \" | git apply \"\n-        tryPatchCmd = patchcmd + \"--check -\"\n-        applyPatchCmd = patchcmd + \"--check --apply -\"\n+        tryPatchArgs = [\"--check\", \"-\"]\n+        applyPatchArgs = [\"--check\", \"--apply\", \"-\"]\n         patch_succeeded = True\n \n-        if verbose:\n-            print(\"TryPatch: %s\" % tryPatchCmd)\n-\n-        if os.system(tryPatchCmd) != 0:\n+        if diffTreeApply(id, tryPatchArgs, ignore_error=True) != 0:\n             fixed_rcs_keywords = False\n             patch_succeeded = False\n             print(\"Unfortunately applying the change failed!\")\n@@ -2279,7 +2294,7 @@ class P4Submit(Command, P4UserMap):\n \n             if fixed_rcs_keywords:\n                 print(\"Retrying the patch with RCS keywords cleaned up\")\n-                if os.system(tryPatchCmd) == 0:\n+                if diffTreeApply(id, tryPatchArgs, ignore_error=True) == 0:\n                     patch_succeeded = True\n                     print(\"Patch succeesed this time with RCS keywords cleaned\")\n \n@@ -2291,7 +2306,7 @@ class P4Submit(Command, P4UserMap):\n         #\n         # Apply the patch for real, and do add/delete/+x handling.\n         #\n-        system(applyPatchCmd, shell=True)\n+        diffTreeApply(id, applyPatchArgs)\n \n         for f in filesToChangeType:\n             p4_edit(f, \"-t\", \"auto\")\ndiff --git a/t/t9803-git-p4-shell-metachars.sh b/t/t9803-git-p4-shell-metachars.sh\nindex 2913277013..ef8fd6e094 100755\n--- a/t/t9803-git-p4-shell-metachars.sh\n+++ b/t/t9803-git-p4-shell-metachars.sh\n@@ -105,4 +105,20 @@ test_expect_success 'branch with shell char' '\n \t)\n '\n \n+test_expect_success 'git p4 submit --commit does not execute shell metachars in commit id' '\n+\tgit p4 clone --dest=\"$git\" //depot &&\n+\ttest_when_finished cleanup_git &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit config git-p4.skipSubmitEditCheck true &&\n+\t\techo f3 >file3 &&\n+\t\tgit add file3 &&\n+\t\tgit commit -m \"add file3\" &&\n+\t\tname='\"'\"'$(touch${IFS}injection-marker)'\"'\"' &&\n+\t\tgit branch \"$name\" HEAD &&\n+\t\tP4EDITOR=\"test-tool chmtime +5\" git p4 submit --commit \"$name\"\n+\t) &&\n+\ttest_path_is_missing \"$cli/injection-marker\"\n+'\n+\n test_done\n\nbase-commit: d38352cd43ab9745686d697872408bc3249a153f\n-- \ngitgitgadget\n"}]}